:seedling: OPRUN-4550: Replace generated mozilla_data.go with go:embed + runtime parsing by tmshort · Pull Request #2634 · operator-framework/operator-controller

tmshort · 2026-04-08T14:13:23Z

Embed mozilla_data.json and parse it at init() time using the existing cipherSuiteId()/curveId() helpers. Unsupported ciphers are skipped and recorded in skippedCiphers, validated by TestNoSkippedCiphers. Simplify update-tls-profiles.sh to a curl download and drop the gojq dependency.

When the profile changes, make verify will show a diff of the profile. The build will still complete using the old data. The updated profile will need to be commited in order for it to be included in the build.

Description

Reviewer Checklist

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

netlify · 2026-04-08T14:13:31Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`26ad8ee`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/69d8fd720fec9a0008352330
😎 Deploy Preview	https://deploy-preview-2634--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

This PR replaces the generated mozilla_data.go file with an embedded JSON approach, simplifying the TLS profile management. Instead of generating Go code from the Mozilla SSL/TLS Configuration Guidelines, the PR embeds the JSON data and parses it at runtime during package initialization. This eliminates the dependency on gojq and makes profile updates clearer through simple JSON diffs.

Changes:

Embed mozilla_data.json using go:embed and parse at init() time instead of generating Go code
Simplify update-tls-profiles.sh to just download the JSON file, removing all jq-based code generation
Add TestNoSkippedCiphers to validate that all ciphers in the Mozilla data are supported by Go's crypto/tls
Remove gojq dependency from the Makefile

Reviewed changes

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

Show a summary per file

File	Description
`internal/shared/util/tlsprofiles/mozilla_data.go`	Rewritten to embed and parse `mozilla_data.json` at runtime, with helper functions to convert Mozilla cipher/curve names to Go TLS identifiers
`internal/shared/util/tlsprofiles/mozilla_data.json`	New embedded JSON file containing Mozilla TLS profile configurations for "modern" and "intermediate" profiles
`hack/tools/update-tls-profiles.sh`	Simplified from 131 lines to 12 lines - now just downloads JSON from Mozilla instead of generating Go code
`Makefile`	Removed `gojq` dependency from the `update-tls-profiles` target
`internal/shared/util/tlsprofiles/tlsprofiles_test.go`	Added `TestNoSkippedCiphers` to ensure all ciphers in the embedded data are supported by Go

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

codecov · 2026-04-08T14:36:46Z

Codecov Report

❌ Patch coverage is 67.50000% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.94%. Comparing base (8614e3b) to head (d3e0d27).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/shared/util/tlsprofiles/mozilla_data.go	67.50%	7 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2634      +/-   ##
==========================================
+ Coverage   68.93%   68.94%   +0.01%     
==========================================
  Files         139      140       +1     
  Lines        9891     9931      +40     
==========================================
+ Hits         6818     6847      +29     
- Misses       2563     2569       +6     
- Partials      510      515       +5

Flag	Coverage Δ
e2e	`37.66% <67.50%> (+0.37%)`	⬆️
experimental-e2e	`52.32% <67.50%> (+0.11%)`	⬆️
unit	`53.66% <67.50%> (+0.05%)`	⬆️

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.

pedjak

/lgtm

rashmigottipati

/lgtm

openshift-ci · 2026-04-09T23:14:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rashmigottipati
Once this PR has been reviewed and has the lgtm label, please ask for approval from pedjak. 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

fgiudici · 2026-04-10T10:03:06Z

Makefile


 .PHONY: update-tls-profiles
-update-tls-profiles: $(GOJQ) #EXHELP Update TLS profiles from the Mozilla wiki
-	env JQ=$(GOJQ) hack/tools/update-tls-profiles.sh


Seems to me here we drop the only usage of gojq, but we keep it still in bingo files (.bingo/gojq.mod, .bingo/Variables.mk, ...).
Should we drop it also from bingo?
Should be something like:

bingo-v0.9.0 get gojq@none

Good catch, if not used anywhere else, we should remove it from bingo files as well.

Done, and rebased, please re-review @pedjak

Embed mozilla_data.json and parse it at init() time using the existing cipherSuiteId()/curveId() helpers. Unsupported ciphers are skipped and recorded in skippedCiphers, validated by TestNoSkippedCiphers. Simplify update-tls-profiles.sh to a curl download and drop the gojq dependency. When the profile changes, `make verify` will show a diff of the profile. The build will still complete using the old data. The updated profile will need to be commited in order for it to be included in the build. Remove gojq from bingo. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>

fgiudici

/lgtm

pedjak

Overall this is a great simplification — removes fragile shell code-gen and a dependency (gojq) in favor of go:embed + runtime parsing with existing helpers. A couple of inline suggestions below.

pedjak · 2026-04-10T13:48:28Z

internal/shared/util/tlsprofiles/mozilla_data.go

+		}
+		cipherNums = append(cipherNums, id)
+	}
+


Unsupported ciphers are tracked in skippedCiphers and validated by TestNoSkippedCiphers — nice. But unsupported curves (where curveId() returns 0) are silently dropped here with no tracking or test.

If Mozilla adds a new curve that Go does not yet support, this would silently weaken the profile. Consider adding a skippedCurves slice with a corresponding test, mirroring the cipher handling:

id := curveId(c) if id == 0 { skipped = append(skipped, "curve:"+c) continue }

(Or a separate skippedCurves list if you prefer to keep them distinct.)

pedjak · 2026-04-10T13:48:28Z

internal/shared/util/tlsprofiles/mozilla_data.go

+	if err := json.Unmarshal(mozillaDataJSON, &spec); err != nil {
+		panic(fmt.Sprintf("tlsprofiles: failed to parse embedded mozilla_data.json: %v", err))
+	}
+


The init() panics are reasonable since the JSON is embedded at compile time — if it is invalid the binary should not start. Just confirming this is intentional and the team is comfortable with startup panics vs. lazy error returns.

pedjak · 2026-04-10T13:48:28Z

Makefile

 .PHONY: update-tls-profiles
-update-tls-profiles: $(GOJQ) #EXHELP Update TLS profiles from the Mozilla wiki
-	env JQ=$(GOJQ) hack/tools/update-tls-profiles.sh
+update-tls-profiles: #EXHELP Update TLS profiles from the Mozilla wiki


The PR description mentions that make verify will show a diff when the profile changes. How does that work? I don't see a new verify target or diff-checking logic added here. Is there an existing mechanism that re-runs update-tls-profiles and diffs the checked-in mozilla_data.json against the fresh download? Worth confirming this path works as described.

pedjak · 2026-04-10T13:48:28Z

internal/shared/util/tlsprofiles/tlsprofiles_test.go

+func TestNoSkippedCiphers(t *testing.T) {
+	require.Empty(t, skippedCiphers,
+		"cipher(s) in mozilla_data.json are not supported by Go's crypto/tls and were omitted: %v", skippedCiphers)
+}


Good test — catches newly-added ciphers that Go does not support yet.

Consider also adding a basic smoke test that asserts the parsed profile values match expectations (e.g. modernTLSProfile.minTLSVersion == tls.VersionTLS13, intermediate has ≥9 ciphers). This would catch upstream JSON schema changes that parse without error but produce wrong data. The existing tests exercise the profiles indirectly, so this is a nice-to-have rather than a blocker.

Copilot AI review requested due to automatic review settings April 8, 2026 14:13

openshift-ci bot requested review from anik120 and trgeiger April 8, 2026 14:13

Copilot started reviewing on behalf of tmshort April 8, 2026 14:14 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

tmshort changed the title ~~🌱 Replace generated mozilla_data.go with go:embed + runtime parsing~~ 🌱 OPRUN-4550: Replace generated mozilla_data.go with go:embed + runtime parsing Apr 9, 2026

pedjak reviewed Apr 9, 2026

View reviewed changes

openshift-ci bot assigned pedjak Apr 9, 2026

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

rashmigottipati approved these changes Apr 9, 2026

View reviewed changes

openshift-ci bot assigned rashmigottipati Apr 9, 2026

fgiudici reviewed Apr 10, 2026

View reviewed changes

tmshort force-pushed the use-embed-to-update-profiles branch from d3e0d27 to 26ad8ee Compare April 10, 2026 13:38

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

fgiudici reviewed Apr 10, 2026

View reviewed changes

openshift-ci bot assigned fgiudici Apr 10, 2026

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

pedjak reviewed Apr 10, 2026

View reviewed changes

Conversation

tmshort commented Apr 8, 2026

Description

Reviewer Checklist

Uh oh!

netlify bot commented Apr 8, 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!

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

Uh oh!

Codecov Report

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

rashmigottipati left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Apr 9, 2026

Uh oh!

fgiudici Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

pedjak Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

tmshort Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

fgiudici left a comment

Choose a reason for hiding this comment

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

pedjak Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

pedjak Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

pedjak Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

pedjak Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

netlify bot commented Apr 8, 2026 •

edited

Loading

codecov bot commented Apr 8, 2026 •

edited

Loading