OCPBUGS-85351: Revert "fix(azure): prevent externalDNSDomain from shadowing cluster apps domain" - #8585
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughThis PR removes DNS domain shadowing validation that previously prevented Azure private hosted clusters from using external DNS domains that overlapped with the cluster's base domain. The change removes validation rules across four layers: the API schema definition removes a kubebuilder constraint, the CLI validator removes external DNS domain conflict checking, the control-plane-operator removes shadowing detection logic and simplifies base-domain DNS reconciliation, and documentation removes corresponding warnings and remediation guidance. Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@Nirshal: This pull request explicitly references no jira issue. 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. |
|
/approve This regressed ARO HCP. |
|
Scheduling tests matching the |
|
/verified bypass |
|
@bryan-cox: 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/cluster/azure/create_test.go (1)
28-83: ⚡ Quick winAdd a regression test for overlapping external/base domain acceptance.
This revert addresses a real behavior break; please add one focused test that uses an overlapping parent-domain
ExternalDNSDomainand assertsValidatedoes not fail, so the regression does not return later.As per coding guidelines, "Always include unit tests when creating new functions or modifying existing ones".
Also applies to: 338-387
🤖 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 `@cmd/cluster/azure/create_test.go` around lines 28 - 83, Add a regression unit test to cmd/cluster/azure/create_test.go that constructs DefaultOptions(), sets CredentialsFile and an overlapping ExternalDNSDomain (a parent domain of BaseDomain) and calls opts.Validate(context.Background(), &core.CreateOptions{}), asserting that Validate returns no error; follow the existing TestValidateEndpointAccess pattern (use t.Run, set opts.ExternalDNSDomain and opts.BaseDomain accordingly) so the test ensures overlapping parent-domain ExternalDNSDomain is accepted and the regression cannot reappear.control-plane-operator/controllers/azureprivatelinkservice/controller_test.go (1)
185-207: ⚡ Quick winPreserve DNS zone visibility in
mockRecordSetsassertions.Line 199 currently ignores
privateDnsZoneName, which makes it harder for tests to catch records being created in the wrong zone. Keep zone capture in the mock and assert zone+record combinations in base-domain/hypershift-local tests.🤖 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_test.go` around lines 185 - 207, The mockRecordSets.CreateOrUpdate currently ignores the DNS zone argument (the second string parameter) so tests can't assert zone+record combos; update the mockRecordSets struct to capture the zone (e.g., add a createdRecordZones or store zone-prefixed names) and modify CreateOrUpdate to record the incoming privateDnsZoneName along with relativeRecordSetName (populate a new field like createdRecordZones or append zone+"|"+relativeRecordSetName to createdRecordNames), and update any base-domain/hypershift-local tests to assert the zone+record combination using the new field(s) instead of only relativeRecordSetName; reference: mockRecordSets struct and its CreateOrUpdate method, plus createdRecordNames/createdRecordZones fields.
🤖 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.
Nitpick comments:
In `@cmd/cluster/azure/create_test.go`:
- Around line 28-83: Add a regression unit test to
cmd/cluster/azure/create_test.go that constructs DefaultOptions(), sets
CredentialsFile and an overlapping ExternalDNSDomain (a parent domain of
BaseDomain) and calls opts.Validate(context.Background(),
&core.CreateOptions{}), asserting that Validate returns no error; follow the
existing TestValidateEndpointAccess pattern (use t.Run, set
opts.ExternalDNSDomain and opts.BaseDomain accordingly) so the test ensures
overlapping parent-domain ExternalDNSDomain is accepted and the regression
cannot reappear.
In
`@control-plane-operator/controllers/azureprivatelinkservice/controller_test.go`:
- Around line 185-207: The mockRecordSets.CreateOrUpdate currently ignores the
DNS zone argument (the second string parameter) so tests can't assert
zone+record combos; update the mockRecordSets struct to capture the zone (e.g.,
add a createdRecordZones or store zone-prefixed names) and modify CreateOrUpdate
to record the incoming privateDnsZoneName along with relativeRecordSetName
(populate a new field like createdRecordZones or append
zone+"|"+relativeRecordSetName to createdRecordNames), and update any
base-domain/hypershift-local tests to assert the zone+record combination using
the new field(s) instead of only relativeRecordSetName; reference:
mockRecordSets struct and its CreateOrUpdate method, plus
createdRecordNames/createdRecordZones fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1f19ac3e-628d-4a17-a15d-0096c2725d5a
⛔ 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)
- docs/content/how-to/azure/create-self-managed-azure-cluster.md
- api/hypershift/v1beta1/hostedcluster_types.go
|
@Nirshal: This pull request references Jira Issue OCPBUGS-85351, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/cc @enxebre |
Test Resultse2e-aws
e2e-aks
|
|
can we please follow up to:
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, enxebre, Nirshal 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 |
|
/test e2e-aks |
|
/jira refresh |
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-85351, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aws |
|
/test e2e-aws |
|
Now I have all the evidence I need. Let me produce the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 7 test failures stem from two independent infrastructure-level issues completely unrelated to the PR changes. PR #8585 only modifies Azure-specific CRD validation, Azure CLI code, Azure private link service controller, and documentation — none of which affect AWS e2e test execution. The first root failure is Root CauseFailure 1 — TestCreateCluster (EnsureGlobalPullSecret): The
Failure 2 — TestKarpenter (Teardown): All Karpenter functional subtests passed successfully. The failure occurred exclusively during teardown when the test framework attempted to clean up AWS infrastructure resources. 10 resources timed out during deletion: 9 EBS volumes (associated with various Karpenter nodepools: Relationship to PR #8585: None. The PR changes are entirely scoped to Azure platform: Azure CEL validation in the HostedCluster CRD, Azure CLI Recommendations
Evidence
|
|
/retest |
|
@Nirshal: 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. |
|
@Nirshal: Jira Issue OCPBUGS-85351: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-85351 has not been moved to the MODIFIED state. This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are 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. |
Unpin hypershift operator now that the domain shadowing revert has landed upstream (openshift/hypershift#8585). The new image (89e19f83) includes the fix. Also bumps ACM operator v2.16.2-440 and MCE v2.11.2-480.
Unpin hypershift operator now that the domain shadowing revert has landed upstream (openshift/hypershift#8585). The new image (89e19f83) includes the fix. Also bumps ACM operator v2.16.2-440 and MCE v2.11.2-480.
Summary
api-int.{name}.domain) that share a parent domain with the cluster baseDomain (aro.{name}.domain), which the validation incorrectly flags as DNS zone shadowing.References
/cc @bryan-cox @enxebre