AUTOSCALE-839: Add E2E tests for centralized TLS profiles#487
Open
joelsmith wants to merge 1 commit into
Open
Conversation
|
@joelsmith: This pull request references AUTOSCALE-839 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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-robot
added
the
jira/valid-reference
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (359)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdded an in-cluster TLS probe harness and serial Ginkgo tests that verify Cluster Autoscaler webhook cipher-suite behavior while changing OpenShift API server TLS profiles and adherence policies. ChangesWebhook TLS adherence testing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GinkgoSuite
participant APIServer
participant TLSProbe
participant WebhookService
GinkgoSuite->>APIServer: Patch TLS profile and adherence policy
GinkgoSuite->>TLSProbe: Create and wait for probe pod
TLSProbe->>WebhookService: Resolve HTTPS service address
GinkgoSuite->>TLSProbe: Execute curl with selected cipher suite
TLSProbe->>WebhookService: Attempt HTTPS handshake
WebhookService-->>TLSProbe: Accept or reject negotiation
TLSProbe-->>GinkgoSuite: Return curl result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/operators/cluster-autoscaler-operator.go (1)
227-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
:latestfor the probe image.Pulling
ubi9-minimal:latestmakes the test non-reproducible and prone to flakes/breakage on upstream image changes. Pin to a specific tag (or digest).🤖 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 `@pkg/operators/cluster-autoscaler-operator.go` at line 227, The probe image is using an unpinned `:latest` tag, which makes the test non-reproducible. Update the image reference in the `cluster-autoscaler-operator` probe configuration to use a specific immutable tag or digest instead of `registry.access.redhat.com/ubi9-minimal:latest`, keeping the change local to the image field used by that probe setup.
🤖 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.
Inline comments:
In `@pkg/operators/cluster-autoscaler-operator.go`:
- Line 147: Fix the remaining golangci-lint issues in
cluster-autoscaler-operator.go by cleaning up formatting around the affected
symbols: remove the extra blank lines before the return statements in the
relevant functions (including the one returning apiServer) to satisfy nlreturn,
end the affected comments with periods to satisfy godot, and adjust
whitespace/cuddling around the flagged blocks to satisfy wsl_v5. Run the project
formatter/linter after updating the nearby functions and comment blocks so all
reported findings are cleared before merge.
- Around line 357-374: Guard the `AfterEach` cleanup in
`cluster-autoscaler-operator.go` against partial `BeforeEach` setup by checking
whether `g` and `probe` were successfully initialized before using them. In the
`AfterEach` block, skip `g.WithSpecReport(specReport).GatherAll()` when `g` is
nil, and only call `probe.teardown(ctx)` when `probe` is non-nil (and its
clientset is available via `tlsProbe.teardown`). Keep the existing
restore/reconcile cleanup, but make the teardown path resilient so failures in
`NewGatherer`, `LoadClient`, or `LoadClientset` do not panic in `AfterEach`.
- Around line 312-319: `expectHandshakeRejected` currently treats every
`probe.handshake(ctx, cipherSuite)` error as a successful rejection, which both
trips the `nilerr` lint and can hide unrelated failures. Update this helper to
distinguish a real TLS/cipher negotiation failure from connectivity or setup
errors by checking the handshake error details/exit code before returning
success, and only return nil for the expected reject case. If the non-nil error
is intentionally ignored in the reject path, add the appropriate `nilerr`
suppression/comment at the `probe.handshake` check so golangci-lint is
satisfied.
- Around line 216-238: The TLS probe pod is configured to run non-root but does
not set an explicit UID, so kubelet may reject it when using
ubi9-minimal:latest. Update the PodSpec in cluster-autoscaler-operator.go to set
a numeric RunAsUser alongside RunAsNonRoot in the pod-level SecurityContext,
keeping the existing probe container security settings unchanged. This change
should be made in the PodSpec for the "probe" container so the pod can start
reliably without depending on SCC-injected IDs.
---
Nitpick comments:
In `@pkg/operators/cluster-autoscaler-operator.go`:
- Line 227: The probe image is using an unpinned `:latest` tag, which makes the
test non-reproducible. Update the image reference in the
`cluster-autoscaler-operator` probe configuration to use a specific immutable
tag or digest instead of `registry.access.redhat.com/ubi9-minimal:latest`,
keeping the change local to the image field used by that probe setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
joelsmith
force-pushed
the
master
branch
5 times, most recently
from
July 9, 2026 20:57
b1c8a6f to
24f65cd
Compare
Author
|
/test e2e-aws-periodic-pre |
Member
|
/test e2e-aws-autoscaler i think this should work? |
maxcao13
reviewed
Jul 15, 2026
Member
There was a problem hiding this comment.
Generally lgtm save for one comment. I like that we are probing the actual webhook server with different TLS profiles and checking which ones the server would accept!
Though I'm unsure where the test is running currently. Seems like e2e-aws-operator does not run this test nor e2e-periodic-pre, so I started an e2e-aws-autoscaler run, but I'm not even sure that will run the test.
Seems like it runs periodic autoscaler tests, but we don't have a release job for non-periodic autoscaler tests? We will see in the morning :-)
| "cluster-autoscaler-operator deployment is not available") | ||
| }) | ||
|
|
||
| AfterEach(func() { |
Member
There was a problem hiding this comment.
Should we wait until all the APIServer changes have fully propagated and that the apiserver clusteroperator finally is fully available again in an AfterAll block? I've noticed that a lot of the TLSServer config change PRs have test jobs that eventually made post steps fail such as must-gathers, and my assumption is that we don't wait for the entire rollout to complete and certain components don't report available because of a degraded API server, but my assumption could be wrong.
Author
There was a problem hiding this comment.
I think that's a good idea. If it does smooth out the clean-up tests, then we should take the idea back to the other repos. I've added an AfterAll block like you suggested, and we'll see how it goes once openshift/release#82046 merges.
Author
|
/retest |
Author
|
/test e2e-aws-autoscaler |
Author
|
/test e2e-aws-autoscaler-periodic-pre |
Author
|
Oh, right, I forgot that openshift/release#82046 hadn't merged yet. I'm not sure how long it takes for the new prow config to take effect. Let's see if the test exists yet: /test e2e-aws-autoscaler-periodic-pre |
Author
|
This time I think it should run the non-periodic autoscaler tests /test e2e-aws-autoscaler |
Add E2E tests to verify that the cluster-autoscaler-operator correctly applies centralized TLS profiles as directed by APIServer config's TLSAdherence field. Also bump openshift/api and openshift/client-go to versions that include the TLSAdherence field on APIServerSpec.
Author
|
/testwith e2e-aws-autoscaler openshift/cluster-autoscaler-operator#378 |
Contributor
|
@joelsmith, |
Author
|
/testwith github.com/openshift/cluster-api-actuator-pkg/e2e-aws-autoscaler openshift/cluster-autoscaler-operator#378 |
Contributor
|
@joelsmith, |
Author
|
/testwith openshift/cluster-api-actuator-pkg/e2e-aws-autoscaler openshift/cluster-autoscaler-operator#378 |
Contributor
|
@joelsmith, |
Author
|
/testwith joelsmith/cluster-api-actuator-pkg/master/e2e-aws-autoscaler openshift/cluster-autoscaler-operator#378 |
Contributor
|
@joelsmith, |
Contributor
|
@joelsmith: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add E2E tests to verify that the cluster-autoscaler-operator correctly applies centralized TLS profiles as directed by APIServer cluster config's TLSAdherence field.
Also bump openshift/api and openshift/client-go to versions that include the TLSAdherence field on APIServerSpec.
Summary by CodeRabbit