CNTRLPLANE-3371: Fix AllowedCIDRs e2e test for Route-based KAS - #8469
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@bryan-cox: This pull request references CNTRLPLANE-3371 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. |
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughValidateKubeAPIServerAllowedCIDRs now passes the guest REST config into ensureAPIServerAllowedCIDRs. ensureAPIServerAllowedCIDRs first waits for the control-plane to reconcile HostedCluster.Spec.Networking.APIServer.AllowedCIDRBlocks into the downstream Service.spec.LoadBalancerSourceRanges (target Service selected by publishing strategy and cloud-specific rules). It then polls reachability by creating a fresh guest kubeclient on each attempt (copying the rest.Config with a custom Dial) and calling ServerVersion() to verify network restrictions. Sequence Diagram(s)sequenceDiagram
participant Test as Test Harness
participant CP as Control-Plane Reconciler
participant LB as Downstream Service/LoadBalancer
participant GuestAPI as Guest kube-apiserver
Test->>CP: Set HostedCluster.Spec.Networking.APIServer.AllowedCIDRBlocks
Note right of CP: Reconciler selects target Service based on publishing strategy/cloud
CP->>LB: Update Service.spec.LoadBalancerSourceRanges
loop Wait for reconciliation
Test->>LB: GET Service.spec.LoadBalancerSourceRanges
alt Ranges match expected
Note right of Test: Begin reachability polling
loop Reachability attempt
Test->>GuestAPI: Create fresh kubeclient (copy rest.Config + custom Dial) and call ServerVersion()
GuestAPI-->>Test: respond (reachable/unreachable)
end
else Not reconciled
Test-->>Test: sleep and retry
end
end
Suggested reviewers
🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
openshift-ci
Bot
added
approved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8469 +/- ##
==========================================
+ Coverage 40.07% 40.10% +0.02%
==========================================
Files 751 753 +2
Lines 92863 92985 +122
==========================================
+ Hits 37215 37288 +73
- Misses 52956 53001 +45
- Partials 2692 2696 +4 see 5 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:
|
|
/pipeline required |
51d7116 to
6b609b0
Compare
|
/pipeline required |
|
Scheduling tests matching the |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Test Resultse2e-aws
e2e-aks
Failed TestsTotal failed tests: 3
|
|
/retest |
|
/test e2e-aws |
|
/test e2e-aks-4-22 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
6b609b0 to
29672c9
Compare
|
/test e2e-aks |
|
/test e2e-aws |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/test e2e-aws |
|
I have all the evidence needed. Here is the complete analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThis is a CI infrastructure failure, not a test or code failure. The ci-operator pod for the Root CauseThe CI pod could not be scheduled on the
The 2 nodes that were actually eligible for this pod type did not have enough memory to schedule it. Preemption was also not possible — the scheduler found no viable preemption victims on the memory-constrained nodes. The pod waited for 30 minutes (the default Prow scheduling timeout) before being terminated. This is a transient cluster capacity issue on Recommendations
Evidence
|
|
/test security |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/util/azure.go`:
- Around line 82-89: The guestClient.Get call for the pod lookup only handles
the success path; change the logic in the pod existence check so that after
calling guestClient.Get(ctx, types.NamespacedName{Name: podTemplate.Name,
Namespace: podTemplate.Namespace}, existing) you fail fast on unexpected errors
(i.e. if err != nil and !apierrors.IsNotFound(err) then assert/fail the test
with the error) and only proceed with delete/retry when err == nil or continue
to create when apierrors.IsNotFound(err); use the existing symbols
guestClient.Get, apierrors.IsNotFound, guestClient.Delete, podTemplate and
existing to locate and update the branch accordingly.
🪄 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: 338e2fe9-6b55-468f-8ae6-7fd2d94cfed3
📒 Files selected for processing (4)
test/e2e/create_cluster_test.gotest/e2e/util/azure.gotest/e2e/util/util.gotest/e2e/util/util_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e/create_cluster_test.go
- test/e2e/util/util.go
- test/e2e/util/util_test.go
The ValidateKubeAPIServerAllowedCIDRs test fails on v2 Azure self-managed clusters because KAS uses Route publishing strategy (via external-dns-domain), not LoadBalancer. Two fixes: 1. Wait for the downstream LB service (router or KAS LB) to have its LoadBalancerSourceRanges updated by the CPO before asserting KAS reachability. The target service is determined by the HC's APIServer publishing strategy. 2. Create a fresh kubeclient per poll iteration to prevent HTTP/2 connection reuse. Go's HTTP/2 transport multiplexes all requests over a single persistent TCP connection — if a prior request succeeded before Azure NSG rules took effect, subsequent requests bypass the restriction on the same connection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
020c455 to
7d4d6db
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/util/util_test.go`:
- Around line 17-131: The test TestAllowedCIDRsTargetService runs serially; add
parallelization by calling t.Parallel() at the top of
TestAllowedCIDRsTargetService and by making each subtest run in parallel: change
each t.Run(...) callback to an anonymous func(t *testing.T) { t.Parallel(); ...
} so subtests call t.Parallel() before any setup (e.g. before calling
azureutil.SetAsAroHCPTest) and then execute the existing assertions that
exercise allowedCIDRsTargetService; keep the existing use of NewWithT and other
helpers unchanged.
🪄 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: 3a809ed5-d5cd-43da-810b-2561c32198f5
📒 Files selected for processing (4)
test/e2e/create_cluster_test.gotest/e2e/util/azure.gotest/e2e/util/util.gotest/e2e/util/util_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e/create_cluster_test.go
- test/e2e/util/azure.go
- test/e2e/util/util.go
|
/test e2e-aks |
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
/lgtm |
|
Scheduling tests matching the |
|
/retest |
|
/verified by e2e |
openshift-ci-robot
added
the
verified
|
@bryan-cox: 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. |
|
/override "Red Hat Konflux / hypershift-operator-main-enterprise-contract / hypershift-operator-main" |
|
/override "Red Hat Konflux / hypershift-operator-enterprise-contract / hypershift-operator-main" |
|
@bryan-cox: Overrode contexts on behalf of bryan-cox: Red Hat Konflux / hypershift-operator-main-enterprise-contract / hypershift-operator-main 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 kubernetes-sigs/prow repository. |
|
@bryan-cox: Overrode contexts on behalf of bryan-cox: Red Hat Konflux / hypershift-operator-enterprise-contract / hypershift-operator-main 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 kubernetes-sigs/prow repository. |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest |
|
/test e2e-aws |
1 similar comment
|
/test e2e-aws |
|
/test e2e-aws one more time |
|
@bryan-cox: 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. |
c23dbb8
into
openshift:main
7 participants
What
Fixes the
ValidateKubeAPIServerAllowedCIDRse2e test so it passes on v2 Azure self-managed clusters where KAS uses Route publishing strategy (via--external-dns-domain).Why
The test was skipped in v2 CI (
--ginkgo.skip="KAS allowed CIDRs") because it always failed. Both v1 and v2 Azure self-managed use Route strategy for KAS, but v1 passes while v2 fails due to a difference in cluster lifecycle timing combined with HTTP/2 connection reuse.Root cause: HTTP/2 connection reuse
The test reuses a single
kubeclient.Clientsetacross allServerVersion()poll iterations. Go's HTTP/2 transport multiplexes all requests over a single persistent TCP connection. If the first poll succeeds before Azure NSG rules take effect, all subsequent polls reuse that connection and never observe the expected failure.Why v1 passes but v2 fails: In v1, the cluster is created fresh inside
TestCreateCluster, so the CPO is in its initial reconciliation burst — the router service'sLoadBalancerSourceRangesand corresponding Azure NSG rules are updated before the firstServerVersion()call. In v2, the cluster is pre-created and shared across tests, so the CPO is in steady-state with longer reconciliation intervals. The firstServerVersion()call succeeds before the NSG rules catch up, and HTTP/2 holds that connection open for all subsequent polls.Additional fix: missing downstream service wait
The test waits for
AllowedCIDRBlocksto propagate from the HostedCluster to the HostedControlPlane, but does not wait for the CPO to reconcile the downstream LoadBalancer service'sLoadBalancerSourceRanges. This is a race condition that exists in both v1 and v2 — v1 just happens to win the race due to CPO being in active reconciliation. Adding an explicit wait makes the test correct rather than relying on timing.Changes
test/e2e/util/util.go— single file, three changes:ensureAPIServerAllowedCIDRssignature:*kubeclient.Clientset→*rest.Configto enable fresh client creation per pollServerVersion()iteration creates a new client viakubeclient.NewForConfig(rest.CopyConfig(guestConfig)), preventing HTTP/2 connection reuse.allowedCIDRsTargetService()helper determines the correct LB service based on APIServer publishing strategy (Route →router, LoadBalancer → platform-specific KAS LB). AnEventuallyblock waits for the service'sLoadBalancerSourceRangesto match before checking KAS reachability.Test Plan
go build -tags e2e ./test/e2e/...— compilesgo build -tags e2ev2 ./test/e2e/v2/...— compilesgo vet -tags e2e ./test/e2e/...— passes🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Chores