CNTRLPLANE-3250,CNTRLPLANE-430: API-driven Azure topology and private connectivity (Phase 1)#8537
Open
muraee wants to merge 7 commits into
Open
CNTRLPLANE-3250,CNTRLPLANE-430: API-driven Azure topology and private connectivity (Phase 1)#8537muraee wants to merge 7 commits into
muraee wants to merge 7 commits into
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 |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: muraee 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 |
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
from
8a0850e to
5e9c12c
Compare
Contributor
Author
|
/test e2e-aks |
openshift-ci
Bot
added
the
needs-rebase
muraee
force-pushed
the
api-driven-azure-topology
branch
2 times, most recently
from
cb1a5ee to
85d7b5b
Compare
openshift-ci
Bot
removed
the
needs-rebase
muraee
force-pushed
the
api-driven-azure-topology
branch
2 times, most recently
from
029b997 to
c307083
Compare
Contributor
Author
|
/test e2e-aks |
Member
saschagrunert
left a comment
saschagrunert
left a comment
There was a problem hiding this comment.
Withdrawing my previous ratcheting concern: PR #8490 (now merged) introduced the PrivateLink ternary change. This PR only adds the Swift ternary (new field, nothing to ratchet) and loosens the type immutability rule. No validation tightening here.
Three remaining items on the API side, all on new code in this PR.
muraee
force-pushed
the
api-driven-azure-topology
branch
from
eee04ec to
080c56a
Compare
muraee
force-pushed
the
api-driven-azure-topology
branch
from
080c56a to
8323431
Compare
…ruct Add Swift as a new AzurePrivateType enum value and introduce AzureSwiftSpec struct with topology and pod network instance fields. Add PublicEndpointExposed condition type for shared ingress convergence tracking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add per-cluster helper functions that derive topology behavior from the Topology and Private API fields instead of isAroHCP() env-var checks: - UseSwiftNetworkingHCP/HC: checks Private.Type == Swift with legacy annotation fallback for unmigrated clusters - UseSharedIngressHCP/HC: UseSwiftNetworking && IsPublic, with env-var fallback for ARO HCP without Swift API fields (e.g. CI) - SwiftPodNetworkInstanceHCP: API field first, annotation fallback - IsAroHCPByHCP/HC: checks ManagedIdentities auth type as definitive per-cluster ARO HCP indicator, decoupled from Swift detection Update IsPrivateHCP/HC with Phase 1 fallback for Topology="" + Swift. Update LabelHCPRoutes to use UseSharedIngress || UseSwiftNetworking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace azureutil.IsAroHCP() env-var checks with API-driven per-cluster helpers across all CPO controllers: - v2 components: CNO, CCM, storage, registry operator, ingress operator predicates and adapt functions now use IsAroHCPByHCP(cpContext.HCP) - KAS, CPO deployment, router config adapt functions - Router deployment: SwiftPodNetworkInstanceHCP instead of annotation read - UseHCPRouter: UseSwiftNetworkingHCP for Swift routing, with UseSharedIngressHCP guard for shared ingress clusters - reconcileHCPRouterServices: UseSwiftNetworkingHCP for ClusterIP vs LB - reconcileExternalRouterServiceStatus: UseSharedIngressHCP - reconcileAPIServerServiceStatus: UseSharedIngressHCP - KAS healthcheck: UseSharedIngressHCP - HCCO credential and platform resource reconciliation - verifyResourceGroupLocationsMatch: reverted to IsAroHCP() env var (management-cluster-level check, not per-cluster) Management-cluster-level calls in main.go are intentionally kept. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace env-var and parameterless checks with per-cluster API-driven helpers in HO controllers: - Config-generator: add UseSharedIngressHC cluster-level filter to skip Private clusters entirely from HAProxy config (security-critical) - NodePool HAProxy: UseSharedIngressHC for worker node backend selection - NodePool controller: UseSharedIngressHC for HAProxy image selection, preventing unnecessary rollouts on Private+Swift clusters - Network policies: UseSharedIngressHC for shared ingress policy - HostedCluster controller: UseSwiftNetworkingHC and IsAroHCPByHC for PrivateLink validation, SecretProviderClass, and hosted resources Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add reconciliation logic for the PublicEndpointExposed condition on HostedCluster. The HC controller probes the shared ingress Service's ClusterIP with the cluster's KAS hostname as SNI to determine whether public API server endpoints are actually reachable. The probe function is injectable via ProbeSharedIngressEndpoint field on the reconciler for unit testing. Condition truth table (observed state): - LB not ready + UseSharedIngress: False/ConvergenceInProgress - Probe success + UseSharedIngress: True/SharedIngressConfigured - Probe failure + UseSharedIngress: False/ConvergenceInProgress - Probe success + !UseSharedIngress: True/ConvergenceInProgress - Probe failure + !UseSharedIngress: False/TopologyPrivate Convergence cases requeue after 10s to re-check. Only set on Swift clusters; non-Swift clusters (PrivateLink) don't get this condition. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
muraee
force-pushed
the
api-driven-azure-topology
branch
from
8323431 to
81c25e6
Compare
Contributor
Author
|
/test e2e-aks |
Update test objects across CPO and HO controllers to use Swift API fields (Private.Type=Swift, Topology) for API-driven Swift detection. Add unit tests for UseSwiftNetworkingHCP/HC, UseSharedIngressHCP/HC, IsAroHCPByHCP/HC, and SwiftPodNetworkInstanceHCP covering API field, annotation fallback, env-var fallback, and negative cases. Split infra test cases into annotation-only, API-only, and both variants for comprehensive Swift detection coverage. Update test fixtures to set AzureAuthenticationConfigType=ManagedIdentities when simulating ARO HCP clusters. Add envtest test suite for Azure Swift topology validation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
muraee
force-pushed
the
api-driven-azure-topology
branch
from
81c25e6 to
189a75a
Compare
Contributor
Author
|
/test e2e-aks |
|
Now I have all the information needed. Let me compile the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 9 failing CI jobs were caused by a single compilation error: a missing Root CauseThe PR refactors ~30 call sites across the codebase to replace environment-variable-based ARO HCP detection ( In commit // MISSING from the import block:
"github.com/openshift/hypershift/support/netutil"This caused the Go compiler to emit
The fix was a one-line addition of the import statement, included in the force-pushed commit set now at PR HEAD Recommendations
Evidence
|
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. |
Testing Report — Swift Private TypeTested by: zgalor Setup
Test Clusters
Verified
Suspected Bug: Konnectivity Route Hostname Not Auto-PopulatedSymptom: Both clusters were stuck on Root cause: The Context: For Swift-enabled clusters, the Cluster Service intentionally omits the Konnectivity hostname from the HostedCluster's Workaround: Manually patched the konnectivity route: kubectl patch route konnectivity-server -n <hcp-namespace> \
--type=merge -p '{"spec":{"host":"konnectivity-server.apps.<cluster-name>.hypershift.local"}}'After patching, Same pattern for Ignition route: The Recommendation: Investigate why the CPO's Additional Setup Notes
|
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