OCPBUGS-98571: Skip Azure topology LB scope override for ARO HCP IngressController#8992
OCPBUGS-98571: Skip Azure topology LB scope override for ARO HCP IngressController#8992bryan-cox wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-98571, 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. |
📝 WalkthroughWalkthroughAzure ingress parameter generation now excludes ARO hosted control planes from internal load balancer selection for private and public-and-private topologies. Table-driven tests verify external load balancer scope for both ARO Azure topologies using managed identities. Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8992 +/- ##
=======================================
Coverage 43.79% 43.80%
=======================================
Files 772 772
Lines 96045 96046 +1
=======================================
+ Hits 42067 42071 +4
+ Misses 51063 51061 -2
+ Partials 2915 2914 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…logy PR openshift#7821 (CNTRLPLANE-2172) added code to force InternalLoadBalancer scope on the guest-cluster IngressController for Azure clusters with Private or PublicAndPrivate topology. This was correct for self-managed Azure but also affected ARO HCP clusters, which use shared ingress and should keep ExternalLoadBalancer scope (or defer to the annotation set by Cluster Service). Add an IsAroHCPByHCP guard so the topology-based scope override only applies to self-managed Azure. ARO HCP clusters now respect the IngressControllerLoadBalancerScope annotation propagated from the HostedCluster, matching the pattern already used in router.go. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover the ARO HCP guard added in the previous commit: - PublicAndPrivate topology → ExternalLoadBalancer (not Internal) - Private topology → ExternalLoadBalancer (shared ingress) - Annotation override to Internal is still respected Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-98571, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-98571, which is valid. 3 validation(s) were run on this bug
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. |
|
/verified later @zgalor |
|
@bryan-cox: This PR has been marked to be verified later 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. |
|
/test all |
|
/pipeline required |
|
Scheduling tests matching the |
|
/verfied by @zgalor
|
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
Test Resultse2e-aws
Failed TestsTotal failed tests: 3
e2e-aks
|
|
/test e2e-aws |
|
@bryan-cox: 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. |
|
I now have all the evidence I need. Let me produce the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe
Why this is unrelated to PR #8992:
Why only one hosted cluster was affected: Recommendations
Evidence
|
|
/test e2e-aws |
What this PR does / why we need it:
PR #7821 (CNTRLPLANE-2172) added code in the HCCO's
NewIngressParams()to forceInternalLoadBalancerscope on the guest-cluster IngressController for Azure clusters withPrivateorPublicAndPrivatetopology. This was correct for self-managed Azure but also affected ARO HCP clusters, which use shared ingress and should keepExternalLoadBalancerscope (or defer to the annotation set by Cluster Service).When CS sets
topology=PublicAndPrivateon ARO HCP clusters (as required by the enhancement openshift/enhancements#1985), the HCCO creates the IngressController withloadBalancerScope=Internal. This causes:*.appsDNS points to this private IP instead of the shared ingress public IPThis PR adds an
IsAroHCPByHCPguard so the topology-based scope override only applies to self-managed Azure. ARO HCP clusters now respect theIngressControllerLoadBalancerScopeannotation propagated from the HostedCluster, matching the pattern already used inrouter.go.The fix commit is authored by @zgalor who identified the root cause and wrote the initial fix in #8993.
Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/OCPBUGS-98571
Special notes for your reviewer:
Root cause analysis
There is exactly one code path that sets the IngressController LB scope —
params.go:50-54. The fix is a one-line guard addition.Empirical validation on a live ARO HCP cluster
The fix was validated on @zgalor's ARO HCP cluster (
zg-fix-ingress):azureAuthenticationConfigType = ManagedIdentitieson the HCP — confirmedIsAroHCPByHCPreturnstrueMANAGED_SERVICE = ARO-HCPset on the HCCO deploymentscope: ExternalWhy the initial fix attempt appeared to fail
@zgalor's initial fix attempt (PR #8993) appeared not to work for two compounding reasons:
ReconcileDefaultIngressControllerinreconcile.go:18-20skips reconciliation when the IngressController already exists (ResourceVersion != ""). Deleting the IngressController is required.params.gocode runs inside the HCCO, which is a separate Deployment. The CPO sets the HCCO image fromHOSTED_CLUSTER_CONFIG_OPERATOR_IMAGE(set by the hypershift-operator tocpo.Image), not from its own container image. Patching the CPO container doesn't change the HCCO. Once the HCCO image was explicitly overridden, the fix worked.Why the guard is correct
AzureAuthenticationTypeWorkloadIdentities→IsAroHCPByHCPreturnsfalse→ topology override fires as before (no regression)AzureAuthenticationTypeManagedIdentities→IsAroHCPByHCPreturnstrue→ topology override is skipped → scope comes from annotation (lines 38-40) or defaults toExternalLoadBalancerrouter.go:61:!azureutil.IsAroHCPByHCP(hcp)Note on CodeRabbit review comment (PR #8993)
CodeRabbit suggested also gating the annotation path (
IngressControllerLoadBalancerScope) on!IsAroHCPByHCP. This is incorrect — the annotation is the mechanism Cluster Service uses to control the scope. Blocking it for ARO HCP would break CS's ability to configure private ingress when needed. The unit test "When ARO HCP has annotation set to Internal it should respect it" explicitly validates this behavior.Checklist:
Summary by CodeRabbit