Add aws-pod-identity-webhook e2e tls cases#8953
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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:
📝 WalkthroughWalkthroughThe PR refactors the control-plane TLS E2E test to use shared component metadata and reusable helpers for pod lookup, in-pod TLS probing, protocol assertions, and Sequence Diagram(s)sequenceDiagram
participant TLSScenario
participant KubernetesAPI
participant ComponentPod
TLSScenario->>KubernetesAPI: Find running component pod
KubernetesAPI-->>TLSScenario: Return pod metadata
TLSScenario->>ComponentPod: Execute openssl s_client
ComponentPod-->>TLSScenario: Return TLS handshake output
TLSScenario->>KubernetesAPI: Read ConfigMap or Deployment TLS minimum
KubernetesAPI-->>TLSScenario: Return configured minimum TLS version
TLSScenario->>TLSScenario: Validate acceptance or rejection expectations
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
gangwgr
changed the title
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr 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 |
|
/pipeline-required |
|
/pipeline required |
|
Scheduling tests matching the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/e2e/v2/tests/control_plane_tls_operator_test.go`:
- Around line 142-153: The ConfigMap check in the `configMapName` branch is
silently returning on `apierrors.IsNotFound`, which can make the `Eventually`
assertion pass vacuously. Update this path to fail loudly like the
`deploymentName` branch in `control_plane_tls_operator_test.go` by asserting on
the `mgmtClient.Get` result instead of returning early, and keep the
`cm.Data["config.yaml"]` minTLSVersion check tied to a successfully retrieved
ConfigMap so a missing object causes the test to fail.
- Around line 88-105: The getFirstRunningPod helper currently assumes
podList.Items[0] is the running pod, which can fail during rollouts because list
ordering is not guaranteed. Update getFirstRunningPod to iterate over all pods
returned by mgmtClient.List and return the first pod whose Status.Phase is
PodRunning, while keeping the existing error behavior when no pods are found or
none are running. Use the existing getFirstRunningPod function and
corev1.PodStatus checks to locate and fix the selection logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3e67f4d0-3cb3-4ded-a4fe-82832c52fd9c
📒 Files selected for processing (1)
test/e2e/v2/tests/control_plane_tls_operator_test.go
|
/test all |
|
/pipeline required |
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
Test Resultse2e-aws
Failed TestsTotal failed tests: 4
e2e-aks
|
gangwgr
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/pipeline required |
|
Scheduling tests matching the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8953 +/- ##
==========================================
+ Coverage 43.45% 44.11% +0.65%
==========================================
Files 771 772 +1
Lines 95718 96227 +509
==========================================
+ Hits 41597 42449 +852
+ Misses 51234 50832 -402
- Partials 2887 2946 +59 see 52 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:
|
|
/retest |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest |
openshift-ci
Bot
added
the
area/platform/azure
|
/pipeline required |
|
Scheduling tests matching the |
|
/test e2e-v2-aws |
|
/retest-required |
|
Unresolved feedback from this review comment:
g.Expect(expectation.expectedProtocol).NotTo(BeEmpty(),
"expectedProtocol must be non-empty when rejectConnection is false") |
|
/pipeline required |
|
Scheduling tests matching the |
fixed |
|
/pipeline required |
|
Scheduling tests matching the |
|
Now I have all the evidence I need. Let me produce the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe The pod initially had FailedScheduling events ("0/2 nodes are available: 1 Insufficient memory, 1 node(s) had untolerated taint(s)"), which delayed scheduling. Once scheduled, the container was created and started 3 times (first at 08:09:40Z, last at 08:24:20Z), with the penultimate instance running only ~67 seconds (08:22:58Z to 08:24:05Z) before exiting cleanly. The test's This is a flaky/infrastructure-related failure that is independent of the PR's changes. Recommendations
Evidence
|
|
/pipeline required |
|
Scheduling tests matching the |
cblecker
left a comment
cblecker
left a comment
There was a problem hiding this comment.
Good refactoring to a reusable multi-component TLS test framework. The controlPlaneTLSComponent struct and shared helpers significantly reduce duplication. Previous review feedback (pod list ordering, vacuous-pass guards) was properly addressed.
A few items below — the IPv6 host:port and pod-restart verification gaps are the substantive ones.
cblecker
left a comment
cblecker
left a comment
There was a problem hiding this comment.
Good refactoring to a reusable multi-component TLS test framework. The controlPlaneTLSComponent struct and shared helpers significantly reduce duplication. Previous review feedback (pod list ordering, vacuous-pass guards) was properly addressed.
A few items below — the IPv6 host:port and pod-restart verification gaps are the substantive ones.
| return v2util.RunCommandInPod(ctx, mgmtKubeClient, mgmtRestConfig, | ||
| namespace, podName, containerName, | ||
| "sh", "-c", | ||
| fmt.Sprintf("timeout 5 openssl s_client -connect %s:%s -%s 2>&1 || true", |
There was a problem hiding this comment.
When connectViaPodIP is true, connectHost will be targetPod.Status.PodIP which can be an IPv6 address. The %s:%s formatting here would produce fd00::1:4443 instead of [fd00::1]:4443, breaking the openssl connect on IPv6/dual-stack clusters.
Use net.JoinHostPort(connectHost, port) instead:
fmt.Sprintf("timeout 5 openssl s_client -connect %s -%s 2>&1 || true",
net.JoinHostPort(connectHost, port), tlsFlag)| }) | ||
| }) | ||
|
|
||
| It("aws-pod-identity-webhook should accept TLS 1.3 but reject TLS 1.2 with Modern profile", func() { |
There was a problem hiding this comment.
The PKI operator tests (lines 506-545) wait for a new pod UID before probing TLS, but the webhook tests here go straight to verifyComponentTLSConnectivity without checking that the kube-apiserver pod has restarted with the new TLS config.
Since both Intermediate and Modern profiles accept TLS 1.3, the acceptance assertion can pass vacuously against a stale pod that hasn't picked up the new config yet. The same gap exists in the downgrade path at line 718.
Consider tracking the kube-apiserver pod UID before mutation and adding the same restart-wait pattern used for the PKI operator pod.
| // to the HostedCluster, the control-plane-pki-operator config reflects the correct minTLSVersion | ||
| // and that the control-plane-pki-operator's HTTPS endpoint enforces those TLS versions correctly. | ||
| func VerifyPKIOperatorTLSConfigTest(getTestCtx internal.TestContextGetter) { | ||
| When("control plane TLS configuration is modified", Ordered, Serial, func() { |
There was a problem hiding this comment.
nit: The old code had Label("Lifecycle") on this When block since it mutates the HostedCluster TLS profile. The label was dropped in the rename. While CI selection uses LabelFilter: "control-plane-pki-operator" from the Describe, the Lifecycle label serves as self-documentation that this block modifies cluster state.
| When("control plane TLS configuration is modified", Ordered, Serial, func() { | |
| When("control plane TLS configuration is modified", Ordered, Serial, Label("Lifecycle"), func() { |
There was a problem hiding this comment.
This still doesn't have the label — line 372 adds a comment saying the omission is intentional for e2e-v2-aws compatibility, which contradicts the "updated" response here. The Lifecycle label should be present since this block mutates cluster state.
Extend control plane TLS operator e2e tests to verify aws-pod-identity-webhook deployment flags and TLS connectivity when the HostedCluster TLS profile changes. Signed-off-by: gangwgr <rgangwar@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
/pipeline required |
|
Scheduling tests matching the |
|
/test e2e-v2-gke |
|
@gangwgr: 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. |
| Modern: &configv1.ModernTLSProfile{}, | ||
| } | ||
| err = mgmtClient.Update(tc.Context, hostedCluster) | ||
| if apierrors.IsConflict(err) { |
There was a problem hiding this comment.
The return on conflict inside Eventually(func(g Gomega){...}).Should(Succeed()) causes premature success — when the Update returns a conflict, no g.Expect assertion has failed for that iteration, so Eventually declares the callback successful and moves on without actually applying the update.
The same pattern appears at lines 635 and 758 (AfterAll cleanup). In the cleanup path, a conflict silently leaves the cluster with a mutated TLS profile.
Dropping the conflict guard and letting the error hit g.Expect(err).NotTo(HaveOccurred()) is the correct fix — the failed assertion triggers an Eventually retry, which re-gets the latest resourceVersion and reattempts the update.
3 participants
Summary by CodeRabbit