OCPBUGS-65730: add --tls-cipher-suites to oauth-apiserver deployment#8554
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-reference
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-65730, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci
Bot
added
do-not-merge/needs-area
area/control-plane-operator
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 ignored due to path filters (5)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates adaptDeployment to read cipher suites from the configured TLSSecurityProfile and append a --tls-cipher-suites= argument only when the returned list is non-empty (placed after --tls-min-version). Tests were updated: the existing TLS case now asserts the cipher-suites arg is absent for profiles that return no suites, and a new Intermediate profile case asserts --tls-min-version=VersionTLS12 and that --tls-cipher-suites= is present. 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
vsolanki12
force-pushed
the
OCPBUGS-65730-add-tls-cipher-suites-oauth-apiserver
branch
from
8a7a3f9 to
d2388a6
Compare
openshift-ci-robot
added
jira/valid-bug
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-65730, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (yli2@redhat.com), skipping review request. 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. |
|
/jira refresh |
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-65730, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (yli2@redhat.com), skipping review request. 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. |
vsolanki12
force-pushed
the
OCPBUGS-65730-add-tls-cipher-suites-oauth-apiserver
branch
from
d2388a6 to
c2fd13b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go (1)
111-152: 💤 Low valueConsider adding a negative assertion for cipher suites in the Modern TLS profile test.
The Modern TLS profile test validates
--tls-min-version=VersionTLS13but doesn't explicitly verify that--tls-cipher-suitesis absent. While the test would likely fail if cipher suites were incorrectly added, a negative assertion would make the expected behavior clearer and improve test coverage.💡 Suggested enhancement
container := podspec.FindContainer(ComponentName, deployment.Spec.Template.Spec.Containers) g.Expect(container).ToNot(BeNil()) g.Expect(container.Args).To(ContainElement("--tls-min-version=VersionTLS13")) +g.Expect(container.Args).ToNot(ContainElement(ContainSubstring("--tls-cipher-suites=")))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go` around lines 111 - 152, Add a negative assertion to the Modern TLS profile test to verify that cipher suites are not set: after locating the OAuth APIServer container (using podspec.FindContainer with ComponentName) and asserting the tls-min-version flag, also assert that container.Args does NOT contain any "--tls-cipher-suites=..." entry (e.g., check for absence of the exact "--tls-cipher-suites" argument or any arg prefixing with that string) so adaptDeployment and the container args are validated for the absence of cipher-suite configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go`:
- Around line 111-152: Add a negative assertion to the Modern TLS profile test
to verify that cipher suites are not set: after locating the OAuth APIServer
container (using podspec.FindContainer with ComponentName) and asserting the
tls-min-version flag, also assert that container.Args does NOT contain any
"--tls-cipher-suites=..." entry (e.g., check for absence of the exact
"--tls-cipher-suites" argument or any arg prefixing with that string) so
adaptDeployment and the container args are validated for the absence of
cipher-suite configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7d3ec71e-95a9-4394-bf2b-6d4e743a9f91
📒 Files selected for processing (2)
control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8554 +/- ##
==========================================
+ Coverage 40.41% 40.53% +0.12%
==========================================
Files 755 755
Lines 93235 93238 +3
==========================================
+ Hits 37679 37797 +118
+ Misses 52854 52740 -114
+ Partials 2702 2701 -1
... and 3 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:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go (2)
111-152: 💤 Low valueConsider verifying that Modern TLS profile does not include
--tls-cipher-suitesargument.The existing Modern profile test validates
--tls-min-version=VersionTLS13but doesn't explicitly verify the absence of the--tls-cipher-suitesargument. Since Modern profile (TLS 1.3) uses a different cipher suite mechanism and the implementation only adds--tls-cipher-suiteswhen the list is non-empty, adding an assertion for absence would make the test coverage more complete and explicitly document the expected behavior.🧪 Optional: Add assertion for cipher suites absence
container := podspec.FindContainer(ComponentName, deployment.Spec.Template.Spec.Containers) g.Expect(container).ToNot(BeNil()) g.Expect(container.Args).To(ContainElement("--tls-min-version=VersionTLS13")) + // Modern profile (TLS 1.3) should not include cipher suites argument + for _, arg := range container.Args { + g.Expect(arg).ToNot(ContainSubstring("--tls-cipher-suites"), "Modern profile should not configure cipher suites") + }This explicitly documents that TLS 1.3 (Modern) doesn't use the
--tls-cipher-suitesflag, making the test suite's coverage of both positive and negative cases clearer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go` around lines 111 - 152, The Modern TLS profile test currently asserts --tls-min-version=VersionTLS13 but not the absence of the cipher-suites flag; update the validation inside the test's validate function (where it calls adaptDeployment and locates the container via podspec.FindContainer(ComponentName, ...)) to also assert that container.Args does NOT contain any --tls-cipher-suites entry (e.g., use the test framework's negative containment assertion on container.Args to ensure no "--tls-cipher-suites=" flag is present for ModernTLSProfile/Modern).
194-194: 💤 Low valueConsider validating the actual cipher suite values in addition to argument presence.
The test currently uses
ContainElement(ContainSubstring("--tls-cipher-suites="))which only verifies the argument is present but not that it contains the expected Intermediate profile cipher suites. While this approach avoids brittleness if the cipher suite list changes, it doesn't catch potential issues where the argument is present but has incorrect or empty values.💡 Optional: More robust assertion
g.Expect(container.Args).To(ContainElement("--tls-min-version=VersionTLS12")) - g.Expect(container.Args).To(ContainElement(ContainSubstring("--tls-cipher-suites="))) + // Verify cipher suites argument exists and is non-empty + var cipherSuitesArg string + for _, arg := range container.Args { + if strings.HasPrefix(arg, "--tls-cipher-suites=") { + cipherSuitesArg = arg + break + } + } + g.Expect(cipherSuitesArg).ToNot(BeEmpty(), "should have --tls-cipher-suites argument") + g.Expect(strings.TrimPrefix(cipherSuitesArg, "--tls-cipher-suites=")).ToNot(BeEmpty(), "cipher suites list should not be empty")Note: This is optional since the current substring match is simpler and sufficient for verifying the feature works correctly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go` at line 194, Test only checks presence of the --tls-cipher-suites= argument; update the assertion in deployment_test.go so it validates the actual value rather than just the flag name (locate the assertion that inspects container.Args). Replace the current ContainElement(ContainSubstring("--tls-cipher-suites=")) check with a stricter assertion that either (a) matches a non-empty value via a regexp like --tls-cipher-suites=.+ or (b) compares against the expected Intermediate-profile cipher string (e.g. build expectedCipherSuites and assert ContainElement(ContainSubstring("--tls-cipher-suites="+expectedCipherSuites))). Ensure the change uses the same container.Args target so the test still finds the arg but now validates its value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go`:
- Around line 111-152: The Modern TLS profile test currently asserts
--tls-min-version=VersionTLS13 but not the absence of the cipher-suites flag;
update the validation inside the test's validate function (where it calls
adaptDeployment and locates the container via
podspec.FindContainer(ComponentName, ...)) to also assert that container.Args
does NOT contain any --tls-cipher-suites entry (e.g., use the test framework's
negative containment assertion on container.Args to ensure no
"--tls-cipher-suites=" flag is present for ModernTLSProfile/Modern).
- Line 194: Test only checks presence of the --tls-cipher-suites= argument;
update the assertion in deployment_test.go so it validates the actual value
rather than just the flag name (locate the assertion that inspects
container.Args). Replace the current
ContainElement(ContainSubstring("--tls-cipher-suites=")) check with a stricter
assertion that either (a) matches a non-empty value via a regexp like
--tls-cipher-suites=.+ or (b) compares against the expected Intermediate-profile
cipher string (e.g. build expectedCipherSuites and assert
ContainElement(ContainSubstring("--tls-cipher-suites="+expectedCipherSuites))).
Ensure the change uses the same container.Args target so the test still finds
the arg but now validates its value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4f83572e-787e-45ca-a557-520fcf1fd1d0
📒 Files selected for processing (2)
control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go
vsolanki12
force-pushed
the
OCPBUGS-65730-add-tls-cipher-suites-oauth-apiserver
branch
2 times, most recently
from
839ced3 to
6e74cbe
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/approve |
openshift-ci
Bot
added
the
approved
sdminonne
left a comment
sdminonne
left a comment
There was a problem hiding this comment.
This is a short solid security improvement PR. It follows other components flags/config.
Thanks
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: muraee, sdminonne, vsolanki12 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 |
|
/retest |
The openshift-oauth-apiserver was started with --tls-min-version but without --tls-cipher-suites, unlike standalone OCP and other CPO-managed components (KCM, kube-scheduler). This adds the cipher suites arg using config.CipherSuites(), following the same pattern as kube-controller-manager.
vsolanki12
force-pushed
the
OCPBUGS-65730-add-tls-cipher-suites-oauth-apiserver
branch
from
6e74cbe to
f6e4761
Compare
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryBoth Enterprise Contract checks fail because PR #8554's branch was created from commit Root CauseThe PR branch is missing two sets of 1. PR #8552 (merged 2026-05-20T13:52Z): Added the 2. PR #8557 (merged 2026-05-20T16:05Z): Updated 38 Tekton task bundle digests to their latest trusted versions and migrated The 6 failures break down as 3 distinct policy violations evaluated against 2 SLSA provenance attestations (one for the Why earlier runs only showed 2 failures: The previous EC run for this PR (and for PRs #8541, #8549, #8553) executed on 2026-05-19/20 and only flagged the This is not caused by the PR's code changes. The PR only modifies Recommendations
Evidence
|
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
|
/verified by me |
openshift-ci-robot
added
the
verified
|
@vsolanki12: 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. |
|
Before fix: After Fix: |
|
@vsolanki12: all tests passed! 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. |
|
@vsolanki12: Jira Issue Verification Checks: Jira Issue OCPBUGS-65730 Jira Issue OCPBUGS-65730 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
Fix included in release 5.0.0-0.nightly-2026-05-27-134409 |
6 participants
What this PR does / why we need it:
The
openshift-oauth-apiserverdeployed by the HyperShift Control Plane Operator (CPO) was started with--tls-min-versionbut without--tls-cipher-suites. In standalone OCP, the authentication-operator configures both flags. Other CPO-managed components likekube-controller-managerandkube-scheduleralready include cipher suites.This PR adds the
--tls-cipher-suitesargument to the oauth-apiserver deployment usingconfig.CipherSuites(), following the same pattern askube-controller-manager(v2/kcm/deployment.go).Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/OCPBUGS-65730
Special notes for your reviewer:
config.CipherSuites()defaults to Intermediate TLS profile when no profile is explicitly configured.--tls-cipher-suitesis now present on the oauth-apiserver deployment.Checklist:
Summary by CodeRabbit
New Features
Tests