OCPBUGS-100054: Fix Azure Private clusters without external DNS - #9171
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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:
📝 WalkthroughWalkthroughAzure private clusters omit Sequence Diagram(s)sequenceDiagram
participant HostedControlPlane
participant ClusterCreation
participant ReconcileDNSConfig
participant applyPlatformDNSConfig
participant RouteReconciliation
participant RouteAPI
HostedControlPlane->>ClusterCreation: provide endpoint and DNS settings
ClusterCreation->>ReconcileDNSConfig: reconcile DNS configuration
ReconcileDNSConfig->>applyPlatformDNSConfig: apply platform overrides
applyPlatformDNSConfig-->>ReconcileDNSConfig: return DNS configuration
ClusterCreation->>RouteReconciliation: reconcile API server routes
RouteReconciliation->>RouteAPI: delete obsolete routes
RouteReconciliation->>RouteAPI: create internal route when required
RouteAPI-->>RouteReconciliation: return route result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
area/platform/azure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@support/globalconfig/dns_test.go`:
- Around line 129-130: Rename the three added subtests in the DNS tests,
including the case around “Azure Private cluster omits publicZone even when
PublicZoneID is set” and the two corresponding cases at the other affected
locations, to use the repository-required “When ... it should ...” description
format.
🪄 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: 6e2a6c8c-7dd2-4d39-ba27-ae0532089a5d
⛔ Files ignored due to path filters (1)
cmd/cluster/azure/testdata/zz_fixture_TestCreateCluster_When_endpoint_access_is_Private_with_endpoint_access_private_flags_it_should_render_HostedCluster_with_Private_endpoint_access.yamlis excluded by!**/testdata/**
📒 Files selected for processing (3)
cmd/cluster/azure/create.gosupport/globalconfig/dns.gosupport/globalconfig/dns_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9171 +/- ##
=======================================
Coverage 44.94% 44.95%
=======================================
Files 778 778
Lines 97427 97430 +3
=======================================
+ Hits 43790 43795 +5
+ Misses 50616 50614 -2
Partials 3021 3021
... and 2 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
23c54b5 to
fdbe328
Compare
openshift-ci-robot
added
jira/valid-reference
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-100054, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (jiezhao@redhat.com), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-azure-v2-self-managed |
fdbe328 to
98271e5
Compare
openshift-ci
Bot
added
area/platform/aws
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-100054, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (jiezhao@redhat.com), skipping review request. 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. |
0e473ff to
43f7f5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go (2)
503-503: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required
When ... it should ...format for both new test cases.
control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go#L503-L503: renameAzure_Private_Route_NoHostnameto a behavior-focused sentence.control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go#L1522-L1522: renameRoute strategy, private, no hostnameto a behavior-focused sentence.As per coding guidelines, test cases in
*_test.gomust use theWhen ... it should ...format.🤖 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_test.go` at line 503, Rename both test cases in control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go:503-503 and control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go:1522-1522 to behavior-focused descriptions using the required “When ... it should ...” format, preserving their existing test behavior.Source: Coding guidelines
1521-1538: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise deletion of obsolete Routes.
This test uses an empty fake client. It verifies that no external Route is created, but it does not execute the successful
Get/Deletepath indeleteRouteIfExists.Seed existing external public and private Routes, then assert that only
kasInternalRouteremains.As per coding guidelines,
*_test.gofiles must unit test any code changes and additions.🤖 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_test.go` around lines 1521 - 1538, Update the “Route strategy, private, no hostname” test to seed the fake client with existing external public and private Routes, exercising the successful Get/Delete path in deleteRouteIfExists. Assert the resulting client state contains only kasInternalRoute, while preserving the existing service expectations.Source: Coding guidelines
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/infra/infra.go`:
- Around line 211-223: Update deleteRouteIfExists so a NotFound error returned
by r.Client.Delete is treated as successful completion, while preserving wrapped
errors for other delete failures and the existing Get handling.
- Around line 264-291: Update the API server route reconciliation around
serviceStrategy.Type so transitioning away from Route, including to
hyperv1.LoadBalancer, deletes the existing KubeAPIServerExternalPublicRoute,
KubeAPIServerExternalPrivateRoute, and KubeAPIServerInternalRoute resources.
Ensure normal Route reconciliation remains unchanged, and add a test covering
cleanup on the strategy transition.
---
Nitpick comments:
In `@control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go`:
- Line 503: Rename both test cases in
control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go:503-503
and
control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go:1522-1522
to behavior-focused descriptions using the required “When ... it should ...”
format, preserving their existing test behavior.
- Around line 1521-1538: Update the “Route strategy, private, no hostname” test
to seed the fake client with existing external public and private Routes,
exercising the successful Get/Delete path in deleteRouteIfExists. Assert the
resulting client state contains only kasInternalRoute, while preserving the
existing service expectations.
🪄 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: 7831f81f-e925-4c2c-bb8a-85e173a309d7
⛔ Files ignored due to path filters (2)
cmd/cluster/azure/testdata/zz_fixture_TestCreateCluster_When_endpoint_access_is_Private_with_endpoint_access_private_flags_it_should_render_HostedCluster_with_Private_endpoint_access.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/infra/testdata/zz_fixture_TestReconcileInfrastructure_Azure_Private_Route_NoHostname.yamlis excluded by!**/testdata/**
📒 Files selected for processing (13)
cmd/cluster/aws/create.gocmd/cluster/azure/create.gocmd/cluster/core/create.gocmd/cluster/core/create_test.gocmd/cluster/gcp/create.gocmd/cluster/kubevirt/create.gocmd/cluster/none/create.gocmd/cluster/openstack/create.gocmd/cluster/powervs/create.gocontrol-plane-operator/controllers/hostedcontrolplane/infra/infra.gocontrol-plane-operator/controllers/hostedcontrolplane/infra/infra_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go
- hypershift-operator/controllers/hostedcluster/hostedcluster_webhook_test.go
- cmd/cluster/kubevirt/create.go
- cmd/cluster/none/create.go
- cmd/cluster/powervs/create.go
- cmd/cluster/aws/create.go
- cmd/cluster/gcp/create.go
- cmd/cluster/core/create_test.go
- cmd/cluster/azure/create.go
- cmd/cluster/core/create.go
- cmd/cluster/openstack/create.go
43f7f5b to
13c38b3
Compare
|
/test e2e-aro-hcp |
bryan-cox
changed the title
| return nil | ||
| } | ||
|
|
||
| func (r *Reconciler) deleteRouteIfExists(ctx context.Context, route *routev1.Route, routeDesc string) error { |
There was a problem hiding this comment.
please use the existing util function k8sutil.DeleteIfNeeded instead of this
There was a problem hiding this comment.
Done — removed deleteRouteIfExists entirely and replaced all three callsites with k8sutil.DeleteIfNeeded, which already handles NotFound on both Get and Delete plus checks deletion timestamp. This matches the pattern already used in the OAuth route reconciliation in the same file.
a805b3b to
2e0c4e6
Compare
…ate clusters
- Add isPrivate parameter to GetIngressServicePublishingStrategyMapping
so Azure Private clusters default KAS to Route strategy
- Update all callers (AWS, Azure, GCP, KubeVirt, None, OpenStack,
PowerVS, webhook defaulter) to pass the new parameter
- Fix private DNS zone name from {name}-azurecluster.{baseDomain} to
{name}.{baseDomain} to match the actual zone naming convention
- Update infra destroy to handle both current and legacy DNS zone name
formats when cleaning up cluster resources
- Set PublicZoneID unconditionally for all Azure topologies, aligning
with AWS behavior (Private only controls KAS endpoint visibility,
not ingress DNS)
- Refactor globalconfig DNS to extract platform-specific overrides into
applyPlatformDNSConfig and remove incorrect Azure Private PublicZone
filtering
Signed-off-by: Bryan Cox <brcox@redhat.com>
Commit-Message-Assisted-by: Claude (via Claude Code)
…ters without hostname - When a Private cluster uses Route strategy without a hostname (no external DNS), skip creating the external private route and clean up any obsolete external routes using k8sutil.DeleteIfNeeded - Handle KAS service status for Route strategy when no hostname is configured by falling back to load balancer ingress hostname or IP - Add test cases for Private cluster without hostname and for route cleanup of obsolete external routes Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
2e0c4e6 to
30d61df
Compare
|
/lgtm |
|
Scheduling tests matching the |
|
/retest-required |
|
/retest |
|
/verified by @bryan-cox Full test verification report here https://bryan-cox.github.io/architectural-artifact-sharing/test-verification-report-ocpbugs-100054/index.html |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@bryan-cox: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@bryan-cox: Jira Issue Verification Checks: Jira Issue OCPBUGS-100054 Jira Issue OCPBUGS-100054 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
Fix included in release 5.0.0-0.nightly-2026-08-06-135032 |
6 participants
What this PR does / why we need it:
Azure Private clusters created without
--external-dns-domainfail to complete. Several issues compound to prevent the hosted cluster from reachingCompleted:PublicZoneIDleaks into guest DNS config: The ingress operator tries to manage records in a public DNS zone it has no access to, causingAuthorizationFailederrors.KAS defaults to
NodePortinstead ofRoute: Without external DNS, the CLI doesn't set--api-server-address, soservicePublishingStrategydefaults toNodePortfor KAS. Azure Private clusters needRoutesince there are no public load balancers.CPO fails reconciling external KAS route: When the cluster is Private with Route strategy but no external DNS hostname,
reconcileExternalRouterequires a non-empty hostname and errors out.Private DNS zone name mismatch:
create infra azurecreates the private DNS zone as<name>-azurecluster.<baseDomain>. The ingress operator'sgetARecordNamestrips the zone suffix from the FQDN to produce a relative record name — but when the zone name doesn't match the cluster domain (due to-azurecluster), stripping is a no-op and the full FQDN becomes the record name. Azure then double-suffixes it, producing an unresolvable DNS entry.Fixes
PublicZoneIDfor Private clusters; default KAS toRoutewhen Private and no--api-server-addressis set.ReconcileDNSConfigto extract platform DNS overrides intoapplyPlatformDNSConfig. For Azure Private topology, clear the public zone even if set on the HCP spec.<name>-azurecluster.<baseDomain>to<name>.<baseDomain>so the zone matches the cluster domain. Update destroy path to match both current and legacy zone name patterns.Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/OCPBUGS-100054
Special notes for your reviewer:
Validated end-to-end: Created a Private Azure self-managed cluster without
--external-dns-domain. HC completed at5.0.0-ec.5with all cluster operators available and zero unhealthy pods — no manual DNS intervention required.DNS zone naming context: The
-azureclustersuffix was originally added to avoid shadowing*.appswhen the PLS controller creates a baseDomain zone with onlyapi/oauthrecords (see PR #8480). That concern doesn't apply here because the ingress operator populates the*.appswildcard in the infra-created zone, so there's no shadowing. The old-azureclusterzone is vestigial — CAPZ marks AzureCluster as externally managed and never uses it.Backward compatibility:
PrivateZoneIDis immutable on the HC API (CEL rule:oldSelf == "" || self == oldSelf), so existing clusters keep their-azureclusterzone. Only new clusters get the fixed zone name. The destroy path handles both patterns.Checklist: