CNTRLPLANE-3250,CNTRLPLANE-430: API-driven Azure topology and private connectivity (Phase 1)#8537
Merged
openshift-merge-bot[bot] merged 8 commits intoJun 10, 2026
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@muraee: This pull request references CNTRLPLANE-3250 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 epic 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. |
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR adds Azure Swift support (AzureSwiftSpec) and XValidation for AzurePrivateSpec, JSON N-1 compatibility tests, netutil helpers for per-HCP/HC Swift and shared-ingress detection, per-cluster azureutil ARO helpers, migrates many ARO/shared-ingress checks to HCP/HC-scoped predicates across control-plane and hypershift controllers, and introduces a PublicEndpointExposed condition with probing logic and unit tests. Sequence Diagram(s)sequenceDiagram
participant Reconciler as HostedClusterReconciler
participant Router as SharedIngressService
participant Probe as ProbeSharedIngressEndpoint
participant HC as HostedCluster
Reconciler->>Router: fetch router Service (ClusterIP / LB status)
Router-->>Reconciler: service IP / LB hostname
Reconciler->>Probe: probe(serviceIP, port, kasHostname)
Probe-->>Reconciler: probe result (success/fail)
Reconciler->>HC: update PublicEndpointExposed condition (Status/Reason)
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
hypershift-operator/controllers/hostedcluster/network_policies.go (1)
80-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelete
SharedIngressNetworkPolicywhen shared ingress is no longer used.This branch only creates the policy. With per-HC gating, a true→false transition leaves a stale policy that still permits ingress from the shared-ingress namespace.
Suggested fix
if netutil.UseSharedIngressHC(hcluster) { policy := networkpolicy.SharedIngressNetworkPolicy(controlPlaneNamespaceName) if _, err := createOrUpdate(ctx, r.Client, policy, func() error { return reconcileSharedIngressNetworkPolicy(policy, hcluster) }); err != nil { return fmt.Errorf("failed to reconcile sharedingress network policy: %w", err) } + } else { + policy := networkpolicy.SharedIngressNetworkPolicy(controlPlaneNamespaceName) + if _, err := k8sutil.DeleteIfNeeded(ctx, r.Client, policy); err != nil { + return fmt.Errorf("failed to delete sharedingress network policy: %w", err) + } }🤖 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 `@hypershift-operator/controllers/hostedcluster/network_policies.go` around lines 80 - 87, The code only creates/updates SharedIngressNetworkPolicy when netutil.UseSharedIngressHC(hcluster) is true, so when that function returns false existing policies remain; update the logic in the enclosing function to handle the false branch by locating the SharedIngressNetworkPolicy (use networkpolicy.SharedIngressNetworkPolicy(controlPlaneNamespaceName) or the same name/labels used by reconcileSharedIngressNetworkPolicy) and delete it via the controller client (e.g., r.Client.Delete) if it exists, ignoring NotFound errors and returning other errors; keep the existing createOrUpdate/reconcile path unchanged for the true branch and ensure error messages reference the same resources (SharedIngressNetworkPolicy) so failures to delete are surfaced.hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (1)
970-988:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear stale Azure PLS conditions when Swift networking is active.
At Line 970, Swift-enabled clusters skip this condition sync path, so previously set
AzurePrivateLinkService*conditions can linger and misreport topology state after migration or fallback changes.Suggested fix
- if hcluster.Spec.Platform.Type == hyperv1.AzurePlatform && !netutil.UseSwiftNetworkingHC(hcluster) { + if hcluster.Spec.Platform.Type == hyperv1.AzurePlatform && !netutil.UseSwiftNetworkingHC(hcluster) { hcpNamespace := manifests.HostedControlPlaneNamespace(hcluster.Namespace, hcluster.Name) var azPLSList hyperv1.AzurePrivateLinkServiceList if err := r.List(ctx, &azPLSList, &client.ListOptions{Namespace: hcpNamespace}); err != nil { condition := metav1.Condition{ Type: string(hyperv1.AzurePrivateLinkServiceAvailable), Status: metav1.ConditionUnknown, Reason: hyperv1.NotFoundReason, Message: fmt.Sprintf("error listing AzurePrivateLinkService in namespace %s: %v", hcpNamespace, err), } meta.SetStatusCondition(&hcluster.Status.Conditions, condition) } else if len(azPLSList.Items) > 0 { meta.SetStatusCondition(&hcluster.Status.Conditions, computeAzurePLSCondition(azPLSList, hyperv1.AzurePrivateLinkServiceAvailable)) meta.SetStatusCondition(&hcluster.Status.Conditions, computeAzurePLSCondition(azPLSList, hyperv1.AzurePLSCreated)) meta.SetStatusCondition(&hcluster.Status.Conditions, computeAzurePLSCondition(azPLSList, hyperv1.AzureInternalLoadBalancerAvailable)) meta.SetStatusCondition(&hcluster.Status.Conditions, computeAzurePLSCondition(azPLSList, hyperv1.AzurePrivateEndpointAvailable)) meta.SetStatusCondition(&hcluster.Status.Conditions, computeAzurePLSCondition(azPLSList, hyperv1.AzurePrivateDNSAvailable)) } + } else if hcluster.Spec.Platform.Type == hyperv1.AzurePlatform { + for _, t := range []hyperv1.ConditionType{ + hyperv1.AzurePrivateLinkServiceAvailable, + hyperv1.AzurePLSCreated, + hyperv1.AzureInternalLoadBalancerAvailable, + hyperv1.AzurePrivateEndpointAvailable, + hyperv1.AzurePrivateDNSAvailable, + } { + meta.RemoveStatusCondition(&hcluster.Status.Conditions, string(t)) + } }🤖 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 `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go` around lines 970 - 988, When Swift networking is active the controller currently skips the Azure PLS sync and can leave stale AzurePrivateLinkService* conditions; update the logic around netutil.UseSwiftNetworkingHC(hcluster) so that when it returns true you explicitly clear/reset the Azure PLS conditions (AzurePrivateLinkServiceAvailable, AzurePLSCreated, AzureInternalLoadBalancerAvailable, AzurePrivateEndpointAvailable, AzurePrivateDNSAvailable) from hcluster.Status.Conditions instead of leaving them unchanged—use meta.RemoveStatusCondition(...) for each condition type (or set them to a neutral/Unknown condition via meta.SetStatusCondition if you prefer a reset message) in the hostedcluster_controller.go code path that checks UseSwiftNetworkingHC to ensure stale state is removed.
🧹 Nitpick comments (1)
support/netutil/visibility_test.go (1)
106-118: ⚡ Quick winRemove duplicated Azure topology table cases.
These two entries duplicate scenarios already covered earlier in
baseVisibilityCases()and add noise without new behavior coverage.Proposed cleanup
- { - name: "When Azure topology is PublicAndPrivate it should be public and private", - platformType: hyperv1.AzurePlatform, - azureTopology: hyperv1.AzureTopologyPublicAndPrivate, - wantPrivate: true, - wantPublic: true, - }, - { - name: "When Azure topology is Private it should be private and not public", - platformType: hyperv1.AzurePlatform, - azureTopology: hyperv1.AzureTopologyPrivate, - wantPrivate: true, - wantPublic: false, - },🤖 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 `@support/netutil/visibility_test.go` around lines 106 - 118, Remove the duplicated Azure topology table cases from the test table: delete the two entries with names "When Azure topology is PublicAndPrivate it should be public and private" and "When Azure topology is Private it should be private and not public" in visibility_test.go because they duplicate scenarios already covered by baseVisibilityCases(); rely on baseVisibilityCases() for those cases to avoid redundant test rows.
🤖 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.
Outside diff comments:
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 970-988: When Swift networking is active the controller currently
skips the Azure PLS sync and can leave stale AzurePrivateLinkService*
conditions; update the logic around netutil.UseSwiftNetworkingHC(hcluster) so
that when it returns true you explicitly clear/reset the Azure PLS conditions
(AzurePrivateLinkServiceAvailable, AzurePLSCreated,
AzureInternalLoadBalancerAvailable, AzurePrivateEndpointAvailable,
AzurePrivateDNSAvailable) from hcluster.Status.Conditions instead of leaving
them unchanged—use meta.RemoveStatusCondition(...) for each condition type (or
set them to a neutral/Unknown condition via meta.SetStatusCondition if you
prefer a reset message) in the hostedcluster_controller.go code path that checks
UseSwiftNetworkingHC to ensure stale state is removed.
In `@hypershift-operator/controllers/hostedcluster/network_policies.go`:
- Around line 80-87: The code only creates/updates SharedIngressNetworkPolicy
when netutil.UseSharedIngressHC(hcluster) is true, so when that function returns
false existing policies remain; update the logic in the enclosing function to
handle the false branch by locating the SharedIngressNetworkPolicy (use
networkpolicy.SharedIngressNetworkPolicy(controlPlaneNamespaceName) or the same
name/labels used by reconcileSharedIngressNetworkPolicy) and delete it via the
controller client (e.g., r.Client.Delete) if it exists, ignoring NotFound errors
and returning other errors; keep the existing createOrUpdate/reconcile path
unchanged for the true branch and ensure error messages reference the same
resources (SharedIngressNetworkPolicy) so failures to delete are surfaced.
---
Nitpick comments:
In `@support/netutil/visibility_test.go`:
- Around line 106-118: Remove the duplicated Azure topology table cases from the
test table: delete the two entries with names "When Azure topology is
PublicAndPrivate it should be public and private" and "When Azure topology is
Private it should be private and not public" in visibility_test.go because they
duplicate scenarios already covered by baseVisibilityCases(); rely on
baseVisibilityCases() for those cases to avoid redundant test rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d1a66cca-7f97-4534-947f-4982768bfce8
⛔ Files ignored due to path filters (42)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/azureprivatespec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/azureswiftspec.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.azure.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/azure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (35)
api/hypershift/v1beta1/azure.goapi/hypershift/v1beta1/azure_private_spec_test.goapi/hypershift/v1beta1/hostedcluster_conditions.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/infra/infra.gocontrol-plane-operator/controllers/hostedcontrolplane/ingress/router.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/healthcheck.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/service.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cno/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cno/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/controlplaneoperator/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/controlplaneoperator/role.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ingressoperator/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ingressoperator/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/registryoperator/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/registryoperator/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/router/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/router/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/router/util/util.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/storage/azure.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/storage/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/storage/deployment.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/network_policies.gohypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.gosharedingress-config-generator/config.gosupport/azureutil/azureutil.gosupport/netutil/visibility.gosupport/netutil/visibility_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 5360-5362: The lbReady calculation only checks
svc.Status.LoadBalancer.Ingress[0] and can miss readiness on other ingress
entries; change the lbReady logic (where lbReady is computed) to iterate over
svc.Status.LoadBalancer.Ingress and set lbReady true if any ingress element has
a non-empty Hostname or IP, preserving the existing short-circuit with
netutil.UseSharedIngressHC(hc) usage and any surrounding logic that depends on
lbReady.
- Around line 2092-2096: The current code swallows errors from
reconcilePublicEndpointExposedCondition by only logging them; instead propagate
the error so the controller-runtime retries properly. In the reconcile loop
where reconcilePublicEndpointExposedCondition(ctx, hcluster) is called, change
the error branch to return the error (e.g., return ctrl.Result{}, err) rather
than just logging, and keep the existing logic that updates requeueAfter when
pubEndpointRequeue is non-nil; reference
reconcilePublicEndpointExposedCondition, pubEndpointRequeue, and requeueAfter to
locate and update the error-handling path.
- Around line 5342-5358: The early returns when kasStrategy is nil or missing
route info and when the shared ingress Service is missing or has no ClusterIP
leave the PublicEndpointExposed condition stale; change these code paths (the
checks around kasStrategy/kasHostname and the svc/svcKey lookup and clusterIP
check) to explicitly set the HostedCluster status condition
PublicEndpointExposed to False (with a clear Reason like "NoSharedIngress" or
"RouteNotReady"), persist the status update before returning, and return a
non-error requeue (so the controller will reconcile again) instead of silently
returning nil; ensure you use the existing status update helpers in this
controller to update the condition so retries and removal transitions observe
the explicit state.
🪄 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: 5724ac97-9b6c-412c-88a0-22faceb6d3b5
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (2)
api/hypershift/v1beta1/hostedcluster_conditions.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/hypershift/v1beta1/hostedcluster_conditions.go
muraee
force-pushed
the
api-driven-azure-topology
branch
from
6448520 to
a4756a0
Compare
muraee
force-pushed
the
api-driven-azure-topology
branch
from
a4756a0 to
2b04196
Compare
muraee
force-pushed
the
api-driven-azure-topology
branch
from
2b04196 to
589ce0d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
control-plane-operator/controllers/hostedcontrolplane/infra/infra.go (1)
458-464:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the standard router service reconciliation in the Swift path.
This branch bypasses
ingress.ReconcileRouterServiceand only mutates a fewServicefields, so the private router service no longer gets the usual ownership/metadata setup and can be orphaned onHostedControlPlanedeletion.One way to keep the existing contract
if netutil.UseSwiftNetworkingHCP(hcp) { if _, err := k8sutil.DeleteIfNeeded(ctx, r.Client, pubSvc); err != nil { return fmt.Errorf("failed to delete public router service: %w", err) } if _, err := createOrUpdate(ctx, r.Client, privSvc, func() error { - return reconcileAROPrivateRouterService(privSvc, hcp) + if err := ingress.ReconcileRouterService(privSvc, true, true, hcp); err != nil { + return err + } + return reconcileAROPrivateRouterService(privSvc, hcp) }); err != nil { return fmt.Errorf("failed to reconcile private router service: %w", err) }As per coding guidelines, "Follow owner reference patterns for proper garbage collection of resources".
🤖 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/infra/infra.go` around lines 458 - 464, The Swift networking branch skips the standard ingress.ReconcileRouterService flow and instead manually mutates the Service (privSvc), losing ownership/metadata so the Service can be orphaned on HostedControlPlane deletion; restore the normal contract by invoking ingress.ReconcileRouterService (or reusing its logic) for the router Services in the netutil.UseSwiftNetworkingHCP(hcp) path (rather than only calling reconcileAROPrivateRouterService), and ensure you use createOrUpdate/DeleteIfNeeded around pubSvc/privSvc while preserving owner references and standard metadata so the HostedControlPlane becomes the controller for garbage collection.
♻️ Duplicate comments (2)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (2)
5365-5366:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEvaluate all LoadBalancer ingress entries for readiness.
At Line 5365-5366, readiness checks only
Ingress[0]. If another ingress entry has IP/hostname, this produces false negatives.💡 Suggested fix
- lbReady := len(svc.Status.LoadBalancer.Ingress) > 0 && - (svc.Status.LoadBalancer.Ingress[0].Hostname != "" || svc.Status.LoadBalancer.Ingress[0].IP != "") + lbReady := false + for _, ing := range svc.Status.LoadBalancer.Ingress { + if ing.Hostname != "" || ing.IP != "" { + lbReady = true + break + } + }🤖 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 `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go` around lines 5365 - 5366, The lbReady check currently only inspects svc.Status.LoadBalancer.Ingress[0], causing false negatives; change the logic that sets lbReady to scan all entries in svc.Status.LoadBalancer.Ingress and set lbReady to true if any entry has a non-empty Hostname or IP. Update the code that assigns lbReady (reference variable lbReady and the slice svc.Status.LoadBalancer.Ingress) to iterate over the slice (handle nil/empty slice as false) and break early when an ingress with Hostname != "" or IP != "" is found. Ensure the new logic preserves existing behavior when the ingress list is empty.
5347-5363:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet
PublicEndpointExposedexplicitly on missing prerequisites.At Line 5348 and Line 5356-5362, the function returns without updating condition state. That can leave
PublicEndpointExposedstale during convergence/removal transitions. Set it toFalsewith a transition reason and requeue when shared ingress is expected.💡 Suggested fix direction
- if kasStrategy == nil || kasStrategy.Route == nil || kasStrategy.Route.Hostname == "" { - return nil, nil - } + if kasStrategy == nil || kasStrategy.Route == nil || kasStrategy.Route.Hostname == "" { + cond := metav1.Condition{ + Type: string(hyperv1.PublicEndpointExposed), + Status: metav1.ConditionFalse, + Reason: hyperv1.PublicEndpointConvergenceInProgressReason, + Message: "API server route hostname is not yet available", + ObservedGeneration: hc.Generation, + } + if meta.SetStatusCondition(&hc.Status.Conditions, cond) { + if err := r.Client.Status().Update(ctx, hc); err != nil { + return nil, fmt.Errorf("failed to update PublicEndpointExposed condition: %w", err) + } + } + d := 10 * time.Second + return &d, nil + }As per coding guidelines,
**/{hypershift-operator,control-plane-operator}/controllers/**/*.go: Follow standard controller-runtime patterns for operator controllers with proper error handling and requeuing.🤖 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 `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go` around lines 5347 - 5363, The controller early-returns when shared ingress is missing (checks using ServicePublishingStrategyByTypeByHC and the Service fetched via r.Client.Get) without updating the HostedCluster condition PublicEndpointExposed; modify the control flow so that when kasStrategy is nil or missing Route/Hostname, or when the shared ingress Service is not found or has no ClusterIP, you explicitly set the HostedCluster's PublicEndpointExposed condition to False with an appropriate transition reason/message (e.g., "SharedIngressUnavailable"/"MissingServiceClusterIP"), persist the status update, and requeue the reconciliation (rather than returning nil,nil) so state converges during creation/removal; ensure updates occur where kasStrategy, svc retrieval (r.Client.Get) and svc.Spec.ClusterIP checks currently return early.
🤖 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 `@api/hypershift/v1beta1/azure.go`:
- Around line 665-680: AzurePrivateSpec currently allows AzurePrivateTypeSwift
even when the cluster is self-managed; add an XValidation rule to gate Swift to
managed Azure auth by ensuring when AzurePrivateType == "Swift" the
azureAuthenticationConfigType is the managed value (e.g. not
"WorkloadIdentities" / must equal "Managed"). Concretely, add a
kubebuilder:validation:XValidation rule to the AzurePrivateSpec similar to the
existing rules that enforces self.type != 'Swift' || has(self.swift) AND
self.type != 'Swift' || (self.azureAuthenticationConfigType == 'Managed') (or
equivalent != 'WorkloadIdentities') so Swift is only valid for managed-auth
clusters; reference AzurePrivateTypeSwift, AzurePrivateSpec, and
azureAuthenticationConfigType when making the change.
---
Outside diff comments:
In `@control-plane-operator/controllers/hostedcontrolplane/infra/infra.go`:
- Around line 458-464: The Swift networking branch skips the standard
ingress.ReconcileRouterService flow and instead manually mutates the Service
(privSvc), losing ownership/metadata so the Service can be orphaned on
HostedControlPlane deletion; restore the normal contract by invoking
ingress.ReconcileRouterService (or reusing its logic) for the router Services in
the netutil.UseSwiftNetworkingHCP(hcp) path (rather than only calling
reconcileAROPrivateRouterService), and ensure you use
createOrUpdate/DeleteIfNeeded around pubSvc/privSvc while preserving owner
references and standard metadata so the HostedControlPlane becomes the
controller for garbage collection.
---
Duplicate comments:
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 5365-5366: The lbReady check currently only inspects
svc.Status.LoadBalancer.Ingress[0], causing false negatives; change the logic
that sets lbReady to scan all entries in svc.Status.LoadBalancer.Ingress and set
lbReady to true if any entry has a non-empty Hostname or IP. Update the code
that assigns lbReady (reference variable lbReady and the slice
svc.Status.LoadBalancer.Ingress) to iterate over the slice (handle nil/empty
slice as false) and break early when an ingress with Hostname != "" or IP != ""
is found. Ensure the new logic preserves existing behavior when the ingress list
is empty.
- Around line 5347-5363: The controller early-returns when shared ingress is
missing (checks using ServicePublishingStrategyByTypeByHC and the Service
fetched via r.Client.Get) without updating the HostedCluster condition
PublicEndpointExposed; modify the control flow so that when kasStrategy is nil
or missing Route/Hostname, or when the shared ingress Service is not found or
has no ClusterIP, you explicitly set the HostedCluster's PublicEndpointExposed
condition to False with an appropriate transition reason/message (e.g.,
"SharedIngressUnavailable"/"MissingServiceClusterIP"), persist the status
update, and requeue the reconciliation (rather than returning nil,nil) so state
converges during creation/removal; ensure updates occur where kasStrategy, svc
retrieval (r.Client.Get) and svc.Spec.ClusterIP checks currently return early.
🪄 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: 16b9a6b0-a8fd-4187-9d8f-bea523d0ff8a
⛔ Files ignored due to path filters (42)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/azureprivatespec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/azureswiftspec.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.azure.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/azure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (36)
api/hypershift/v1beta1/azure.goapi/hypershift/v1beta1/azure_private_spec_test.goapi/hypershift/v1beta1/hostedcluster_conditions.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/infra/infra.gocontrol-plane-operator/controllers/hostedcontrolplane/ingress/router.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/healthcheck.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/service.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cno/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cno/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/controlplaneoperator/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/controlplaneoperator/role.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ingressoperator/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ingressoperator/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/registryoperator/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/registryoperator/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/router/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/router/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/router/util/util.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/storage/azure.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/storage/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/storage/deployment.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/network_policies.gohypershift-operator/controllers/hostedcluster/public_endpoint_condition_test.gohypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.gosharedingress-config-generator/config.gosupport/azureutil/azureutil.gosupport/netutil/visibility.gosupport/netutil/visibility_test.go
🚧 Files skipped from review as they are similar to previous changes (26)
- control-plane-operator/controllers/hostedcontrolplane/v2/registryoperator/deployment.go
- control-plane-operator/controllers/hostedcontrolplane/v2/ingressoperator/component.go
- control-plane-operator/controllers/hostedcontrolplane/v2/controlplaneoperator/role.go
- hypershift-operator/controllers/hostedcluster/network_policies.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go
- control-plane-operator/controllers/hostedcontrolplane/v2/router/deployment.go
- control-plane-operator/controllers/hostedcontrolplane/v2/cno/component.go
- control-plane-operator/controllers/hostedcontrolplane/kas/service.go
- control-plane-operator/controllers/hostedcontrolplane/kas/healthcheck.go
- sharedingress-config-generator/config.go
- control-plane-operator/controllers/hostedcontrolplane/v2/router/config.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
- hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.go
- control-plane-operator/controllers/hostedcontrolplane/v2/ingressoperator/deployment.go
- control-plane-operator/controllers/hostedcontrolplane/v2/controlplaneoperator/deployment.go
- control-plane-operator/controllers/hostedcontrolplane/ingress/router.go
- control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config.go
- support/azureutil/azureutil.go
- hypershift-operator/controllers/hostedcluster/public_endpoint_condition_test.go
- api/hypershift/v1beta1/hostedcluster_conditions.go
- control-plane-operator/controllers/hostedcontrolplane/v2/registryoperator/component.go
- control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/deployment.go
- control-plane-operator/controllers/hostedcontrolplane/v2/storage/azure.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
- api/hypershift/v1beta1/azure_private_spec_test.go
- support/netutil/visibility.go
muraee
force-pushed
the
api-driven-azure-topology
branch
from
589ce0d to
9a3435a
Compare
muraee
force-pushed
the
api-driven-azure-topology
branch
2 times, most recently
from
5e9c12c to
dd73d8f
Compare
Member
|
Other than the regression lgtm overall |
Add UseSwiftNetworkingHCP to reconcileAPIServerServiceStatus so Swift clusters resolve the KAS hostname from the Route strategy (via KasRouteHostname helper) instead of falling through to LB service lookup. Update infra tests to cover Swift Private and PublicAndPrivate topologies separately, and fix test helper to skip simulating LB provisioning for router services that are ClusterIP under Swift/shared ingress. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
|
/approve |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre, muraee 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 |
Re-test After Fix —
|
| Cluster | Topology | InfraReady | Etcd | KAS | KAS Public Reachability |
|---|---|---|---|---|---|
| zg-public-4 | PublicAndPrivate | True | True | True | HTTP 200 ok |
| zg-private-4 | Private | True | True | True | Connection refused (expected) |
Verified
- Private-router issue fixed —
Privatetopology cluster now reachesInfrastructureReady: Truewithout manual intervention. - Konnectivity routes auto-populated — Both clusters get
konnectivity-server.apps.<cluster>.hypershift.localset by the CPO automatically. - KAS visibility correct — Public cluster KAS reachable via shared ingress (
HTTP 200). Private cluster KAS not reachable from public network (connection refused). - Route structure correct for private cluster — No
kube-apiserverpublic route created; onlykube-apiserver-privateandkube-apiserver-internal(hypershift.local) routes exist.
LGTM from the CS testing side.
|
@zgalor: The 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 by @zgalor |
|
@zgalor: 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. |
|
/lgtm |
Contributor
|
Scheduling tests matching the |
Contributor
Author
|
/test "ci/prow/security" |
Contributor
4 tasks
Contributor
Contributor
Author
|
/retest-required |
Contributor
Contributor
|
/hold Revision e2aa9ff was retested 3 times: holding |
Contributor
Author
|
/retest-required |
|
Now let me produce the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThis job has 2 independent infrastructure-level flakes, neither caused by PR #8537. (1) TestNodePoolPrevReleaseN3: A NodePool using the 4.19 release image ( Root CauseFailure 1 — TestNodePoolPrevReleaseN3 (4.19 node never joined): The parent test Failure 2 — TestKarpenterUpgradeControlPlane/Main (NodeClaim never registered): PR #8537 is not the cause. The PR adds Azure-specific Swift networking support and changes Recommendations
Evidence
|
Contributor
Author
|
/override e2e-aws-4-22 |
Contributor
|
@muraee: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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. |
Contributor
|
@muraee: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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. |
9 tasks
Contributor
Author
|
/override "ci/prow/e2e-aws-4-22" |
Contributor
|
@muraee: Overrode contexts on behalf of muraee: ci/prow/e2e-aws-4-22 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. |
Contributor
|
@muraee: 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. |
This was referenced Jun 10, 2026
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
6 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.
Summary
AzurePrivateTypeenum withSwiftvariant and addAzureSwiftSpecstruct with immutablepodNetworkInstancefieldUseSwiftNetworkingHCP/HC,UseSharedIngressHCP/HC,IsAroHCPByHCP/HC) that derive behavior fromTopologyandPrivateAPI fields instead ofisAroHCP()env-var checksisAroHCP()/UseSharedIngress()calls in visibility, router, shared ingress, infra, nodepool, and CPO v2 component controllers with API-driven helpersPublicEndpointExposedcondition type for tracking public endpoint exposure stateUseSwiftNetworkingHCP()for unmigrated clustersThis is Phase 1 of the API-driven Azure topology and private connectivity enhancement. Phase 2 (ARO-RP data migration) and Phase 3 (legacy cleanup) are out of scope.
Key Changes
API (
api/hypershift/v1beta1/azure.go)AzurePrivateType:Enum=PrivateLink;SwiftAzureSwiftSpec:podNetworkInstance(required, immutable, max 255 chars)AzurePrivateSpec: struct-level type immutability (!has(oldSelf.type) || self.type == oldSelf.type) enabling Phase 2 migration, positive/negative union member rulesHelper Functions (
support/netutil/visibility.go,support/azureutil/azureutil.go)UseSwiftNetworkingHCP/HC()— API field first, env var + annotation fallbackUseSharedIngressHCP/HC()—UseSwiftNetworking && IsPublicIsAroHCPByHCP/HC()— per-cluster ARO wrapper for component predicatesIsPrivateHCP/HC()— Phase 1 fallback forTopology=""+ Swift annotationLabelHCPRoutes()—UseSharedIngress || UseSwiftNetworkingController Changes
UseSharedIngressHC()filter (security-critical — prevents Private cluster endpoints from leaking into public HAProxy config)IsAroHCPByHCP(cpContext.HCP)replaces env-var predicatesUseSwiftNetworkingHCP()for ClusterIP vs LB router service decisionUseSharedIngressHCP/HC()Not Changed (management-cluster-level, kept as env-var)
hypershift-operator/main.go— controller startupcontrol-plane-operator/main.go— controller startupcmd/cluster/core/dump.go— dump commandTest plan
UseSharedIngressHC()correctly filters Private+Swift clusters from HAProxy config🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Behavior/Validation
Tests