OTA-1764: feat(cvo): add TLS cipher suites and minimum version flags#8013
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@DavidHurta: This pull request references OTA-1764 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
@DavidHurta: This pull request references OTA-1764 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
area/control-plane-operator
openshift-ci
Bot
added
the
needs-rebase
Add new flags to the hosted CVO to comply with the centralized TLS configuration in HyperShift. These flags are to override the CVO's internal TLS profile, which is used for its metrics server.
DavidHurta
force-pushed
the
cvo-add-tls-config-flags
branch
from
4571f71 to
c91d2de
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/test all |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8013 +/- ##
==========================================
+ Coverage 41.43% 42.47% +1.04%
==========================================
Files 756 774 +18
Lines 93647 97648 +4001
==========================================
+ Hits 38802 41479 +2677
- Misses 52124 53275 +1151
- Partials 2721 2894 +173
... and 109 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/lgtm |
|
Scheduling tests matching the |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
cblecker
left a comment
cblecker
left a comment
There was a problem hiding this comment.
Code looks good — correct, nil-safe, and consistent with how other v2 components handle TLS configuration. Left one suggestion about test coverage.
| if tlsMinVersion := config.MinTLSVersion(configuration.GetTLSSecurityProfile()); tlsMinVersion != "" { | ||
| c.Args = append(c.Args, fmt.Sprintf("--tls-min-version=%s", tlsMinVersion)) | ||
| } | ||
| if cipherSuites := config.CipherSuites(configuration.GetTLSSecurityProfile()); len(cipherSuites) != 0 { | ||
| c.Args = append(c.Args, fmt.Sprintf("--tls-cipher-suites=%s", strings.Join(cipherSuites, ","))) | ||
| } |
There was a problem hiding this comment.
The logic looks correct — follows the same pattern as kube-scheduler, machine-approver, and other v2 components.
One thing I noticed: all 5 test fixtures exercise the same TLS code path (nil Configuration → Intermediate default), so they all produce identical --tls-min-version=VersionTLS12 + cipher suites output. Might be worth adding a fixture variant (or unit test in deployment_test.go) that sets a Modern TLS profile — that's the interesting edge case where CipherSuites() returns empty and --tls-cipher-suites should be omitted. The oauth_apiserver component has this kind of coverage as a reference.
Not blocking — the underlying config.MinTLSVersion() / config.CipherSuites() functions are well-tested in support/config/cipher_test.go, so the risk is low. Just would be nice to have coverage at the component level too.
There was a problem hiding this comment.
That's a good point; thank you.
I have added unit tests to verify the functionality for more combinations than the fixture testing verifies in d3064db
There was a problem hiding this comment.
Thanks for adding the test cases! However, they all use TLSProfileCustomType with manually specified (or empty) cipher lists. That exercises the securityProfile.Custom.Ciphers branch in CipherSuites(), not the configv1.TLSProfiles[securityProfile.Type].Ciphers lookup path.
The Modern profile specifically is worth testing because its ciphers (TLS_AES_128_GCM_SHA256, etc.) are all TLS 1.3 ciphers that are commented out of openSSLToIANACiphersMap in cipher.go, so CipherSuites() returns empty through a different mechanism than Custom-with-empty-ciphers. A single test case with Type: configv1.TLSProfileModernType (no Custom field) expecting --tls-min-version=VersionTLS13 and no --tls-cipher-suites flag would cover that.
There was a problem hiding this comment.
Thank you for the feedback! I initially used custom profile test cases to directly control the Ciphers and minTLSVersion field values, making it straightforward to exercise flag presence/absence.
I've now added a modern profile test case that exercises the configv1.TLSProfiles[securityProfile.Type].Ciphers lookup path and verifies the TLS 1.3 cipher filtering behavior you described.
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
Additionally, the Codecov failure is expected — adaptDeployment gained new lines (the TLS flag logic) that aren't directly exercised by unit tests. The coverage actually comes from the fixture-based TestControlPlaneComponents snapshot tests, but Codecov can't always attribute YAML output validation back to the specific source lines inside closures that produced it. The behavior IS tested — it's a reporting gap, not a coverage gap.
| Value: dataPlaneReleaseImage, | ||
| }) | ||
|
|
||
| if tlsMinVersion := config.MinTLSVersion(configuration.GetTLSSecurityProfile()); tlsMinVersion != "" { |
There was a problem hiding this comment.
nit: configuration.GetTLSSecurityProfile() is called twice — once for min version, once for cipher suites. It's a trivial accessor so no real performance concern, but extracting it to a variable would be cleaner:
tlsProfile := configuration.GetTLSSecurityProfile()
if tlsMinVersion := config.MinTLSVersion(tlsProfile); tlsMinVersion != "" {
...
}
if cipherSuites := config.CipherSuites(tlsProfile); len(cipherSuites) != 0 {
...
}That said, machine_approver, kube_scheduler, and kcm all do the same double-call, so if you'd rather stay consistent with the existing pattern that's fine too. Your call.
There was a problem hiding this comment.
Thank you for ensuring clean code!
Now done in b07e703
| if tlsMinVersion := config.MinTLSVersion(configuration.GetTLSSecurityProfile()); tlsMinVersion != "" { | ||
| c.Args = append(c.Args, fmt.Sprintf("--tls-min-version=%s", tlsMinVersion)) | ||
| } | ||
| if cipherSuites := config.CipherSuites(configuration.GetTLSSecurityProfile()); len(cipherSuites) != 0 { |
There was a problem hiding this comment.
question: in oauth_apiserver/deployment.go:52, --tls-min-version is passed unconditionally (no != "" guard), because MinTLSVersion() never returns an empty string thanks to the Intermediate fallback. Here you use the conditional guard, which is what machine_approver and kube_scheduler do. Both are correct since the fallback to Intermediate always yields "VersionTLS12", but I wanted to confirm: is the intent to be more defensive so the CVO doesn't get an empty TLS flag if the profile somehow returns empty? If so, makes sense — it's safer than oauth_apiserver's approach.
There was a problem hiding this comment.
Yes, exactly, the intent is to be more defensive.
To keep lines concise and reuse the returned value from the function.
Test adaptDeployment with various combinations of set and unset cipher suites and MinTLSVersion flags. Co-authored-by: Claude Code <claude@anthropic.com>
Add test case that verifies Modern TLS profile behavior: sets TLS 1.3 as minimum version without adding cipher suites flag. This exercises the configv1.TLSProfiles[securityProfile.Type].Ciphers lookup path and confirms that Modern profile's TLS 1.3 ciphers (commented out in openSSLToIANACiphersMap) result in no --tls-cipher-suites argument. Co-authored-by: Claude Code <claude@anthropic.com>
cblecker
left a comment
cblecker
left a comment
There was a problem hiding this comment.
LGTM - the Modern profile test case exercises the configv1.TLSProfiles lookup path and TLS 1.3 cipher filtering as requested. Implementation follows established v2 component patterns cleanly.
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cblecker, DavidHurta The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest-required |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/test ? |
|
/test e2e-aws-metrics |
1 similar comment
|
/test e2e-aws-metrics |
|
/retest-required |
|
Now I have all the evidence I need. Let me compile the analysis. The key findings are:
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe
Then the failure occurred at step 8 — validating NodeClaims are ready. The replacement NodeClaim Additionally, 2 NodeClaims existed when only 1 was expected, indicating the old drifted NodeClaim ( This is unrelated to PR #8013. The PR adds Recommendations
Evidence
|
|
/verified by CI Unit tests ensure that flags are applied if applicable. |
openshift-ci-robot
added
the
verified
|
@DavidHurta: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
|
@DavidHurta: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-bot
Bot
merged commit 433a404
into
openshift:main
5 participants
What this PR does / why we need it:
Add new flags to the hosted CVO to comply with the centralized TLS configuration in HyperShift. These flags are to override the CVO's internal TLS profile, which is used for its metrics server. The flags were added with openshift/cluster-version-operator#1338.
Which issue(s) this PR fixes:
Fixes #OTA-1764
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests