WIP Revert " fix(azure): prevent externalDNSDomain from shadowing cluster apps domain" - #8586
Conversation
…dowing cluster apps domain"
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughThis PR removes Azure DNS base domain shadowing validation and detection across the HyperShift stack. The changes eliminate the kubebuilder validation rule that rejected HostedCluster configurations where the external DNS domain overlaps the base domain, remove the CLI-level Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 6 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bryan-cox 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}
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 `@control-plane-operator/controllers/azureprivatelinkservice/controller.go`:
- Around line 337-338: Reinstate the base-domain shadowing protection that was
removed: before calling reconcileBaseDomainDNS( ctx, azPLS, hcp.Name, log )
ensure you run the prior shadowing validation (the logic removed around the
former 759-784 range) and only call reconcileBaseDomainDNS when that validation
reports "not shadowed"; if the validation detects a shadowed/overlapping base
domain, set the degraded condition and return the appropriate result/error
instead of reconciling DNS. Locate the shadow-checking logic previously adjacent
to reconcileBaseDomainDNS and rewire or extract it into a helper (e.g.,
validateBaseDomainShadowing or similar) so the controller uses that helper to
gate calls to reconcileBaseDomainDNS and surface degradation when overlapping
domains are detected.
🪄 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: d8c4d2bf-5977-4c8e-b438-4f4209b8efa8
⛔ Files ignored due to path filters (19)
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/**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/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (7)
api/hypershift/v1beta1/hostedcluster_types.gocmd/cluster/azure/create.gocmd/cluster/azure/create_test.gocontrol-plane-operator/controllers/azureprivatelinkservice/controller.gocontrol-plane-operator/controllers/azureprivatelinkservice/controller_test.godocs/content/how-to/azure/create-self-managed-azure-cluster.mddocs/content/how-to/azure/deploy-azure-private-clusters.md
💤 Files with no reviewable changes (2)
- api/hypershift/v1beta1/hostedcluster_types.go
- docs/content/how-to/azure/create-self-managed-azure-cluster.md
| if result, err := r.reconcileBaseDomainDNS(ctx, azPLS, hcp.Name, log); err != nil || !result.IsZero() { | ||
| return result, err |
There was a problem hiding this comment.
Restore base-domain shadowing protection in base-domain DNS reconciliation.
Line 759 drops the context needed for shadowing checks, and Line 337 now calls this path unconditionally for any non-empty base domain. That re-allows overlapping domain configurations to reconcile DNS records instead of surfacing a degraded condition, which can break private-cluster API/OAuth/apps name resolution.
Suggested fix
- if result, err := r.reconcileBaseDomainDNS(ctx, azPLS, hcp.Name, log); err != nil || !result.IsZero() {
+ if result, err := r.reconcileBaseDomainDNS(ctx, azPLS, hcp.Name, hcp.Spec.DNS.BaseDomain, log); err != nil || !result.IsZero() {
return result, err
}
-func (r *AzurePrivateLinkServiceReconciler) reconcileBaseDomainDNS(ctx context.Context, azPLS *hyperv1.AzurePrivateLinkService, clusterName string, log logr.Logger) (ctrl.Result, error) {
+func (r *AzurePrivateLinkServiceReconciler) reconcileBaseDomainDNS(ctx context.Context, azPLS *hyperv1.AzurePrivateLinkService, clusterName, hcpBaseDomain string, log logr.Logger) (ctrl.Result, error) {
+ if baseDomainShadowsClusterDomain(azPLS.Spec.BaseDomain, hcpBaseDomain) {
+ patch := client.MergeFrom(azPLS.DeepCopy())
+ meta.SetStatusCondition(&azPLS.Status.Conditions, metav1.Condition{
+ Type: string(hyperv1.AzurePrivateDNSAvailable),
+ Status: metav1.ConditionFalse,
+ Reason: "BaseDomainShadowsClusterDomain",
+ Message: "base domain overlaps cluster domain and may shadow private DNS resolution",
+ ObservedGeneration: azPLS.Generation,
+ })
+ if err := r.Status().Patch(ctx, azPLS, patch); err != nil {
+ return ctrl.Result{}, err
+ }
+ return ctrl.Result{}, nil
+ }
recordNames, err := r.recordNamesForCR(ctx, azPLS, clusterName, log)
if err != nil {
return ctrl.Result{}, err
}As per coding guidelines, Design APIs contract-first following OpenShift and Kubernetes best practices and Enforce API and base code best practices aligned with OpenShift and Kubernetes standards.
Also applies to: 759-784
🤖 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/azureprivatelinkservice/controller.go`
around lines 337 - 338, Reinstate the base-domain shadowing protection that was
removed: before calling reconcileBaseDomainDNS( ctx, azPLS, hcp.Name, log )
ensure you run the prior shadowing validation (the logic removed around the
former 759-784 range) and only call reconcileBaseDomainDNS when that validation
reports "not shadowed"; if the validation detects a shadowed/overlapping base
domain, set the degraded condition and return the appropriate result/error
instead of reconciling DNS. Locate the shadow-checking logic previously adjacent
to reconcileBaseDomainDNS and rewire or extract it into a helper (e.g.,
validateBaseDomainShadowing or similar) so the controller uses that helper to
gate calls to reconcileBaseDomainDNS and surface degradation when overlapping
domains are detected.
|
/close |
|
@bryan-cox: Closed this PR. 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. |
1 participant
Reverts #8480
Summary by CodeRabbit
Release Notes