:seedling: Preserve Mozilla v5.8 old TLS profile and harden update script by tmshort · Pull Request #2632 · operator-framework/operator-controller

tmshort · 2026-04-07T15:45:59Z

Move oldTLSProfile to a static old_profile.go (removed from v6+ spec)
Add version-based early exit to update-tls-profiles.sh
Add profile existence and tls_versions field validation in script
Add unit tests for old profile content and X25519MLKEM768 curve

Description

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

openshift-ci · 2026-04-07T15:46:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kevinrizza for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

netlify · 2026-04-07T15:46:06Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`4340f84`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/69d5403237c43c000831370d
😎 Deploy Preview	https://deploy-preview-2632--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Preserves the deprecated Mozilla v5.8 “old” TLS profile as a static Go source while updating the generator to track Mozilla’s latest TLS guidelines and adding guards/tests to prevent accidental regressions.

Changes:

Move oldTLSProfile into a new static old_profile.go (sourced from Mozilla v5.8) and remove it from generated data.
Harden update-tls-profiles.sh with version-based early exit and schema validations; switch input to latest.json / v6.
Add unit tests for old profile invariants and for presence of the X25519MLKEM768 curve across profiles.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
internal/shared/util/tlsprofiles/tlsprofiles_test.go	Adds assertions to lock down old profile invariants and ensure `X25519MLKEM768` is present in profiles.
internal/shared/util/tlsprofiles/old_profile.go	Introduces a static copy of Mozilla v5.8 “old” profile for backward compatibility.
internal/shared/util/tlsprofiles/mozilla_data.go	Updates generated header to track `latest.json` (v6) and removes generated “old” profile.
hack/tools/update-tls-profiles.sh	Switches generator input to `latest.json`, adds early exit on unchanged version, and validates input structure before generation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/shared/util/tlsprofiles/tlsprofiles_test.go

hack/tools/update-tls-profiles.sh

internal/shared/util/tlsprofiles/tlsprofiles_test.go

grokspawn · 2026-04-07T16:04:16Z

hack/tools/update-tls-profiles.sh

+    echo "Mozilla TLS data is already at version ${NEW_VERSION}, skipping regeneration."
+    exit 0
+fi
+echo "Updating Mozilla TLS data from version ${STORED_VERSION:-unknown} to ${NEW_VERSION}"


Is there any case where we want to fail here for a schema migration? It would have been good info in the recent profile jump to 6.0 to have log output that said "Updating Mozilla TLS data from version v5.7 to v6.0", but we'd still have the schema fracture.

The additional checks that are added to the function now check for schema changes that impact generation.

grokspawn · 2026-04-07T16:05:53Z

hack/tools/update-tls-profiles.sh


 TMPFILE="$(mktemp)"
 trap 'rm -rf "$TMPFILE"' EXIT



Is there any value in ensuring that the output is valid JSON? What if the URL location changes? Then we'll see the version failure which won't clearly indicate the problem.

Not sure we need to go that far, if they change the format, they are messing with everyone. There will be a failure, though.

Those checks would manifest the cases

invalid JSON error retrieving schema

the same:

"ERROR: Could not read .version from ${INPUT}"

If the URL had an outage, or if they changed the schema again in another way that made it impossible to parse given the current assumptions, that would be the symptom.

Maybe just catch and test the curl failures?

It behaves better now.

grokspawn · 2026-04-07T16:10:05Z

internal/shared/util/tlsprofiles/tlsprofiles_test.go

+// X25519MLKEM768 (added in Go 1.24) is present in all three named profiles.
+// This curve is part of the Mozilla recommendation and its presence should be
+// guarded against accidental removal.
+func TestX25519MLKEM768InProfiles(t *testing.T) {


We should probably also verify that MLKEM is in the schema retrieved in the script as well, no?

Clarify? "MLKEM" itself as a string is not part of the profiles. Only "X25519MLKEM768"

The test code validates that a specific profile is in the generated code. We should detect an omission of this policy earlier in the tooling if we validate it is included when retrieving the schema via the script.

TBH, I'm not sure we should actually test against this particular curve in this way; and I'm also not sure we want to specify this kind of testing in the script itself, which should be somewhat agnostic to the resulting output. The only exception being ciphers that we know don't exist.

We still have a problem if a new curve is added, because we don't have the code for it (because we don't know what it is).

In other words, I don't think this is really a valid test, and I think I'll remove it, because that curve may be superseded, and that's just fine. Explicit testing will just cause issues.

Claude agrees:

Does the TestX25519MLKEM768InProfiles function really add any value, when the set of key exchanges could be changed at any time?

No, not really — especially for the generated profiles. If Mozilla updates their curve recommendations and X25519MLKEM768 is removed or replaced in a future spec version, the test fails even though the generated code is correct. It's coupling the test to a specific curve choice that's entirely Mozilla's to change.

For the static oldTLSProfile it's slightly more defensible (the file won't change unless someone edits it), but that's already guarded by TestOldProfileCiphers verifying the full cipher list. The curves aren't tested there, but an accidental edit to old_profile.go would likely show up in git diff during make verify anyway.

Overall the test encodes a transient implementation detail rather than an invariant. I'd remove it.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

hack/tools/update-tls-profiles.sh

codecov · 2026-04-07T16:44:34Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.95%. Comparing base (4510b1b) to head (4340f84).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2632   +/-   ##
=======================================
  Coverage   68.95%   68.95%           
=======================================
  Files         139      139           
  Lines        9891     9891           
=======================================
  Hits         6820     6820           
  Misses       2562     2562           
  Partials      509      509

Flag	Coverage Δ
e2e	`37.27% <ø> (-0.29%)`	⬇️
experimental-e2e	`52.25% <ø> (ø)`
unit	`53.60% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Move oldTLSProfile to a static old_profile.go (removed from v6+ spec) - Add version-based early exit to update-tls-profiles.sh - Validate profile existence, tls_versions, ciphers, and curves fields - Make jq/sed/cat invocations null-safe and consistently quote variables - Add unit tests for old profile content; fix global state leak in tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>

fgiudici

/lgtm

fgiudici · 2026-04-08T08:29:16Z

hack/tools/update-tls-profiles.sh


-version=$(${JQ} -r '.version' ${TMPFILE})
+# Extract stored version from current output file (may be empty on first run)
+STORED_VERSION=$(grep '^// DATA VERSION:' "${OUTPUT}" 2>/dev/null | awk '{print $4}' || true)


nit: the "|| true" may be dropped here as the grep is in a pipe with awk and the overall result would never fail (as the awk command would not fail). Still looks more robust this way.

Copilot AI review requested due to automatic review settings April 7, 2026 15:46

openshift-ci bot requested review from ankitathomas and bentito April 7, 2026 15:46

Copilot AI reviewed Apr 7, 2026

View reviewed changes

Copilot started reviewing on behalf of tmshort April 7, 2026 15:55 View session

grokspawn reviewed Apr 7, 2026

View reviewed changes

tmshort force-pushed the tls-profiles-old-profile-preservation branch from fbafd47 to 2de30ac Compare April 7, 2026 16:09

grokspawn reviewed Apr 7, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings April 7, 2026 16:23

tmshort force-pushed the tls-profiles-old-profile-preservation branch from 2de30ac to b4a4e74 Compare April 7, 2026 16:23

Copilot started reviewing on behalf of tmshort April 7, 2026 16:23 View session

Copilot AI reviewed Apr 7, 2026

View reviewed changes

hack/tools/update-tls-profiles.sh Outdated Show resolved Hide resolved

hack/tools/update-tls-profiles.sh Outdated Show resolved Hide resolved

hack/tools/update-tls-profiles.sh Outdated Show resolved Hide resolved

hack/tools/update-tls-profiles.sh Outdated Show resolved Hide resolved

tmshort force-pushed the tls-profiles-old-profile-preservation branch from b4a4e74 to 4340f84 Compare April 7, 2026 17:34

fgiudici reviewed Apr 8, 2026

View reviewed changes

openshift-ci bot assigned fgiudici Apr 8, 2026

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2026

Conversation

tmshort commented Apr 7, 2026

Description

Reviewer Checklist

Uh oh!

openshift-ci bot commented Apr 7, 2026

Uh oh!

netlify bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmshort Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmshort Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fgiudici left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify bot commented Apr 7, 2026 •

edited

Loading

tmshort Apr 7, 2026 •

edited

Loading

tmshort Apr 7, 2026 •

edited

Loading

codecov bot commented Apr 7, 2026 •

edited

Loading