Add ability to disable creation of dns zone for unmanaged installs #5666

sadasu · 2025-06-02T20:38:22Z

Similar to managed installs, add ability to optionally create DNS zones for unmanged installs.

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds the ability to optionally create private DNS zones for unmanaged clusters. Without this feature they are always created. There are some instances we would like to use a DNS service other than Azure DNS and would like CAPI to withhold creation of private DNS Zones at those times. This feature adds a new field to NetworkSpec that allows us to skip creation of the DNS zone. Default behavior remains unchanged where DNS zones are created during cluster creation.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

TODOs:

squashed commits
includes documentation
adds unit tests
cherry-pick candidate

Release note:

Add ability to optionally create the Private DNS Zone for unmanaged clusters instead of always creating one. Setting `PrivateDNSZone` within the `NetworkSpec` to `PrivateDNSZoneiCreationModeNone` will skip creating the Private DNS zone.

k8s-ci-robot · 2025-06-02T20:38:32Z

Hi @sadasu. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

codecov · 2025-06-02T20:48:02Z

Codecov Report

❌ Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.06%. Comparing base (2acf550) to head (59c26c9).
⚠️ Report is 63 commits behind head on main.

Files with missing lines	Patch %	Lines
api/v1beta1/azurecluster_webhook.go	50.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5666      +/-   ##
==========================================
+ Coverage   52.83%   53.06%   +0.22%     
==========================================
  Files         278      279       +1     
  Lines       29610    29677      +67     
==========================================
+ Hits        15645    15747     +102     
+ Misses      13148    13114      -34     
+ Partials      817      816       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

willie-yao · 2025-06-02T23:25:04Z

/ok-to-test

willie-yao · 2025-06-18T23:24:21Z

/retest

@sadasu Can you please run make generate to fix the verify error?

willie-yao · 2025-07-08T21:22:07Z

/retest

sadasu · 2025-07-10T14:02:36Z

@willie-yao could you please take another look? Thanks!

jhixson74 · 2025-07-17T22:08:31Z

LGTM, I don't have anything to add here.

api/v1beta1/types_class.go

sadasu · 2025-07-23T17:58:07Z

Thanks for your work on this @sadasu! One small design suggestion: Would you be able to change the PrivateDNSZoneModes to a custom type definition like how UpgradeChannel is defined right below?

@willie-yao This change is causing pull-cluster-api-provider-azure-apidiff to fail.

nrb · 2025-07-23T18:17:00Z

This change is causing pull-cluster-api-provider-azure-apidiff to fail.

For what it's worth, that's not a required check. It's up to maintainers of a given project, but if the api diff is failing because of a change they know will break, then the PR can still be merged.

For this specific change, I think it's alright because it's a) a Go-level change and b) it's a type alias around a string, so the YAML values that most users interact with will not be affected.

willie-yao · 2025-07-23T18:28:00Z

@nrb is correct! The failure is expected since it is a change to a type, but it won't have any functional differences to the user. This change will help users not be confused on what the field can be set to

api/v1beta1/azuremanagedcontrolplane_types.go

willie-yao · 2025-07-23T18:31:11Z

api/v1beta1/azuremanagedcontrolplane_types.go

+// PrivateDNSZoneMode determines if the Private DNS Zone gets created.
+// It is created by default on a private cluster and can be skipped based on a configured value.
+type PrivateDNSZoneMode string
+
 const (
 	// ManagedClusterFinalizer allows Reconcile to clean up Azure resources associated with the AzureManagedControlPlane before
 	// removing it from the apiserver.
 	ManagedClusterFinalizer = "azuremanagedcontrolplane.infrastructure.cluster.x-k8s.io"

 	// PrivateDNSZoneModeSystem represents mode System for azuremanagedcontrolplane.


Suggested change

// PrivateDNSZoneModeSystem represents mode System for azuremanagedcontrolplane.

// PrivateDNSZoneModeSystem represents mode System for Private DNS Zones.

I think this is a better description but feel free to disregard if not.

willie-yao · 2025-07-23T18:31:52Z

api/v1beta1/azuremanagedcontrolplane_types.go

 const (
 	// ManagedClusterFinalizer allows Reconcile to clean up Azure resources associated with the AzureManagedControlPlane before
 	// removing it from the apiserver.
 	ManagedClusterFinalizer = "azuremanagedcontrolplane.infrastructure.cluster.x-k8s.io"

 	// PrivateDNSZoneModeSystem represents mode System for azuremanagedcontrolplane.
-	PrivateDNSZoneModeSystem string = "System"
+	PrivateDNSZoneModeSystem PrivateDNSZoneMode = "System"

 	// PrivateDNSZoneModeNone represents mode None for azuremanagedcontrolplane.


Suggested change

// PrivateDNSZoneModeNone represents mode None for azuremanagedcontrolplane.

// PrivateDNSZoneModeNone represents mode None for Private DNS Zones.

Same as above

willie-yao · 2025-07-23T18:33:58Z

api/v1beta1/azuremanagedcontrolplane_types.go

@@ -21,16 +21,20 @@ import (
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 )

+// PrivateDNSZoneMode determines if the Private DNS Zone gets created.


Does this comment mean that if PrivateDNSZoneMode is not set, the Private DNS Zone is not created? Or is it not created if set to None? I think the comment is fine but may need a bit more clarification.

If PrivateDNSZoneMode is not set, it would follow the default behavior where Private DNS Zone will be created. That is the same behavior when its value ti set to PrivateDNSZoneModeSystem.
We set to PrivateDNSZoneModeNone, Private DNS Zone creation would be skipped.

Updated comment to hepefully make it clearer.

Sounds good! @sadasu Let me know when you push those changes and if it's ready for another round of review.

sadasu · 2025-07-23T18:56:31Z

This change is causing pull-cluster-api-provider-azure-apidiff to fail.

For what it's worth, that's not a required check. It's up to maintainers of a given project, but if the api diff is failing because of a change they know will break, then the PR can still be merged.

For this specific change, I think it's alright because it's a) a Go-level change and b) it's a type alias around a string, so the YAML values that most users interact with will not be affected.

Thank you @nrb and @willie-yao for the explanation.

Similar to managed installs, add ability to optionally create DNS zones for unmanged installs.

sadasu · 2025-07-28T20:23:36Z

@nrb and @willie-yao, upon further consideration, I decided to move PrivateDNSZone from NetworkClassSpec to NetworkSpec because within the NetworkClassSpec, the properties may be shared across multiple clusters. Moving it to within the NetworkSpec allows this config to be applied per Azure cluster.

nrb · 2025-07-29T15:52:24Z

I decided to move PrivateDNSZone from NetworkClassSpec to NetworkSpec because within the NetworkClassSpec, the properties may be shared across multiple clusters.

While I'm not very well versed in CAPZ and it's data structures, I think that it makes sense to have this in the NetworkSpec and possibly also in NetworkClassSpec, so that it can propagate from the NetworkClass to individual clusters. That's assuming that NetworkClass is something like CAPI's ClusterClass and behaves kind of like a template.

I'll leave it to the maintainers to decide if they want to put both in right now, but I think for single clusters NetworkSpec is the right place.

willie-yao · 2025-07-29T22:12:03Z

@sadasu @nrb The general rule of thumb is if the field is able to be shared across clusters in a ClusterClass, it should be put in the ClassSpec rather than just the Spec. A field being in the ClassSpec doesn't make it unable to be set per cluster if that makes sense. It just allows it to be set in a ClusterClass and shared across clusters.

willie-yao · 2025-07-29T22:12:32Z

For example, a Vnet is not in NetworkClassSpec because the same Vnet cannot be shared by multiple clusters

sadasu · 2025-07-31T15:52:00Z

For example, a Vnet is not in NetworkClassSpec because the same Vnet cannot be shared by multiple clusters

@willie-yao So, it appears that you agree with the move to NetworkSpec from NetworkClassSpec. The latest updates already reflect that. Could you PTAL?

sadasu · 2025-07-31T15:53:38Z

/test pull-cluster-api-provider-azure-apiversion-upgrade

sadasu · 2025-08-04T21:45:04Z

/test pull-cluster-api-provider-azure-apidiff

k8s-ci-robot · 2025-08-04T21:50:01Z

@sadasu: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-azure-apidiff	`59c26c9`	link	false	`/test pull-cluster-api-provider-azure-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

willie-yao

/lgtm

Thanks for your patience and work on this!
btw apidiff is supposed to fail since you made a change in a type file, although it's non-breaking as it's just changing the string type.

k8s-ci-robot · 2025-08-05T17:13:26Z

LGTM label has been added.

Git tree hash: 4bf0a9bf1f202c546d734bcf91d8d4795ccabbfc

mboersma

/lgtm
/approve

Thanks for doing this, and thanks for your patience!

k8s-ci-robot · 2025-08-05T20:37:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboersma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mboersma]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added kind/feature do-not-merge/release-note-label-needed cncf-cla: yes labels Jun 2, 2025

github-project-automation bot added this to CAPZ Planning Jun 2, 2025

k8s-ci-robot requested review from mboersma and willie-yao June 2, 2025 20:38

github-project-automation bot moved this to Todo in CAPZ Planning Jun 2, 2025

k8s-ci-robot added size/S needs-ok-to-test labels Jun 2, 2025

k8s-ci-robot added release-note and removed do-not-merge/release-note-label-needed labels Jun 2, 2025

sadasu force-pushed the unmanaged-privateDNSZoneMode branch from 4c7fe79 to 1ed0e07 Compare June 2, 2025 20:49

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jun 2, 2025

sadasu force-pushed the unmanaged-privateDNSZoneMode branch 2 times, most recently from 7ce774b to b660d6d Compare July 8, 2025 15:36

k8s-ci-robot added size/M and removed size/S labels Jul 8, 2025

sadasu force-pushed the unmanaged-privateDNSZoneMode branch from b660d6d to 58dae51 Compare July 8, 2025 21:35

k8s-ci-robot added size/L and removed size/M labels Jul 8, 2025

bryan-cox reviewed Jul 18, 2025

View reviewed changes

api/v1beta1/types_class.go Outdated Show resolved Hide resolved

sadasu force-pushed the unmanaged-privateDNSZoneMode branch from 58dae51 to 6f2bf33 Compare July 21, 2025 13:37

sadasu force-pushed the unmanaged-privateDNSZoneMode branch 4 times, most recently from 42e6cda to 487fa67 Compare July 23, 2025 16:26

willie-yao reviewed Jul 23, 2025

View reviewed changes

Add ability to disable creation of dns zone for unmanaged installs

59c26c9

Similar to managed installs, add ability to optionally create DNS zones for unmanged installs.

sadasu force-pushed the unmanaged-privateDNSZoneMode branch from 487fa67 to 59c26c9 Compare July 28, 2025 17:35

willie-yao approved these changes Aug 5, 2025

View reviewed changes

k8s-ci-robot assigned willie-yao Aug 5, 2025

k8s-ci-robot added the lgtm label Aug 5, 2025

mboersma approved these changes Aug 5, 2025

View reviewed changes

k8s-ci-robot assigned mboersma Aug 5, 2025

k8s-ci-robot added the approved label Aug 5, 2025

k8s-ci-robot merged commit b17b877 into kubernetes-sigs:main Aug 5, 2025
22 of 23 checks passed

k8s-ci-robot added this to the v1.21 milestone Aug 5, 2025

github-project-automation bot moved this from Todo to Done in CAPZ Planning Aug 5, 2025

	// PrivateDNSZoneModeSystem represents mode System for azuremanagedcontrolplane.
	// PrivateDNSZoneModeSystem represents mode System for Private DNS Zones.

	// PrivateDNSZoneModeNone represents mode None for azuremanagedcontrolplane.
	// PrivateDNSZoneModeNone represents mode None for Private DNS Zones.

Add ability to disable creation of dns zone for unmanaged installs #5666

Add ability to disable creation of dns zone for unmanaged installs #5666

Uh oh!

Conversation

sadasu commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 2, 2025

Uh oh!

codecov bot commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

willie-yao commented Jun 2, 2025

Uh oh!

willie-yao commented Jun 18, 2025

Uh oh!

willie-yao commented Jul 8, 2025

Uh oh!

sadasu commented Jul 10, 2025

Uh oh!

jhixson74 commented Jul 17, 2025

Uh oh!

Uh oh!

sadasu commented Jul 23, 2025

Uh oh!

nrb commented Jul 23, 2025

Uh oh!

willie-yao commented Jul 23, 2025

Uh oh!

Uh oh!

willie-yao Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

sadasu Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

willie-yao Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

willie-yao Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

sadasu Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willie-yao Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

sadasu commented Jul 23, 2025

Uh oh!

sadasu commented Jul 28, 2025

Uh oh!

nrb commented Jul 29, 2025

Uh oh!

willie-yao commented Jul 29, 2025

Uh oh!

willie-yao commented Jul 29, 2025

Uh oh!

sadasu commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sadasu commented Jul 31, 2025

Uh oh!

sadasu commented Aug 4, 2025

Uh oh!

k8s-ci-robot commented Aug 4, 2025

Uh oh!

willie-yao left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Aug 5, 2025

Uh oh!

mboersma left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Aug 5, 2025

Uh oh!

Uh oh!

sadasu commented Jun 2, 2025 •

edited

Loading

codecov bot commented Jun 2, 2025 •

edited

Loading

sadasu Jul 23, 2025 •

edited

Loading

sadasu commented Jul 31, 2025 •

edited

Loading