feat: add AdditionalVPCs field to HostedZoneSpec by ecordell · Pull Request #99 · aws-controllers-k8s/route53-controller

ecordell · 2026-03-06T22:37:50Z

I'd like to be able to associate additional VPCs with a hosted zone - the current APIs force you to manually make the association after.

I've tested in an AWS environment, but I'm not familiar with how testing works for this repo generally. Is there a process to follow?

ack-prow · 2026-03-06T22:37:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ecordell
Once this PR has been reviewed and has the lgtm label, please assign knottnt for approval by writing /assign @knottnt in a comment. For more information see the Kubernetes Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

ack-prow · 2026-03-06T22:38:01Z

Hi @ecordell. Thanks for your PR.

I'm waiting for a aws-controllers-k8s 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.

Details

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/test-infra repository.

michaelhtm

Thanks @ecordell

Just one high level comment on whether or not we want to support primaryVPC updates.
Other than that, Great work!!

cc: @knottnt @a-hilaly

michaelhtm · 2026-03-11T22:28:08Z

+	{
+		primaryVPCID := ""
+		if ko.Spec.VPC != nil && ko.Spec.VPC.VPCID != nil {
+			primaryVPCID = *ko.Spec.VPC.VPCID
+		}
+		var additionalVPCs []*svcapitypes.VPC
+		for _, v := range resp.VPCs {
+			if v.VPCId == nil || *v.VPCId == primaryVPCID {
+				continue
+			}
+			region := string(v.VPCRegion)
+			additionalVPCs = append(additionalVPCs, &svcapitypes.VPC{
+				VPCID:     v.VPCId,
+				VPCRegion: &region,
+			})
+		}
+		ko.Spec.AdditionalVPCs = additionalVPCs


should we support cases where the user would want to disassociate the primaryVPC as well?

If the original vpc specified by spec.vpc isn't special and can be disassociated I agree it doesn't make sense for the ACK controller to prevent users from disassociating that VPC. But we'd need to be careful in how that is implemented. As-is the logic for create and delete could become stuck in a broken state if we allow spec.vpc to be disassociated by un-setting the field.

If spec.vpc is unset after creating the resource and later the ACK needs to recreate the hosted zone after it as been deleted in AWS the ACK controller will attempt to create hosted zone without an initial VPC and fail if an initial VPC is required.

When the ACK controller is deleting the hosted zone it first attempts to disassociate all VPCs defined in spec.additionalVPCs. If spec.vpc has been unset the controller will attempt to disassociate all VPCs from the hosted zone before deletion which based on the DisassociateVPCFromHostedZone API docs will result in a validation failure.

Additionally, we might want to change the name of spec.additionalVPCs if we allow user to disassociate the VPC defined by spec.vpc. If that disassociation is expressed by un-setting spec.vpc having spec.additionalVPCs by itself could be confusing.

This is good feedback, thanks

What do we think about this:

Enforce an invariant that spec.vpc must be non-nil if len(spec.additionalVPCs) > 0

When syncing VPC associations, treat spec.vpc + []spec.additionalVPCs as one set to reconcile; first associating new VPCs and then disassociating any not in the desired set.

(side affect of the first two) Changing spec.vpc will associate the new vpc before disassociating the old vpc and is effectively mutable, but can't be empty.

I think it may also make sense to enforce spec.vpc is non-nil for private hosted zones?

Unless I'm missing it, I don't see a good way to enforce what I want - I don't see where I can easily set custom cel rules in the generator.

So some options:

I could update the generator to allow passing custom cel validation (not against this but would be a longer lead time than I'd like)

Do what I listed above, but nothing stops you from cleaning out the spec.vpc. That will remove the association but means you can have spec.vpc = nil, additionalVPCs = [a,b] which is a little weird. But, there will be an error condition if you try and disassociate the final VPC.

Leave the current behavior - only changes to additionalVPCs will disassociate VPCs. You can clear out spec.vpc if you want but it won't disassociate that VPC.

LMK which sounds best and I will address it.

I've opened aws-controllers-k8s/code-generator#677 as well

For the sake of review I've updated this PR to generate with my fork ^

It will fail the codegen check until that PR is merged. Or, if we don't like that direction, I can work on a different option.

Managing the set of associated VPCs across two fields seems like it could be a bit awkward from a usability standpoint. Naively, if I had the below manifest and wanted to disassociate the VPC associated with spec.vpc I'd be a bit surprised that I have to "pop" a VPC out of spec.additionalVPCs to make it work.

apiVersion: route53.services.k8s.aws/v1alpha1 kind: HostedZone metadata: name: test-zone spec: name: test-zone vpc: vpcID: vpc-1 vpcRegion: us-west-2 additionalVPCs: - vpcID: vpc-2 vpcRegion: us-west-2 - vpcID: vpc-3 vpcRegion: us-west-2

What do you think about adding a spec.vpcs field instead of spec.additionalVPCs that this mutually exclusive with spec.vpc? That way users would have the ability to only interact with a single field when managing one or more associated VPCs.

# managing multiple VPCs apiVersion: route53.services.k8s.aws/v1alpha1 kind: HostedZone metadata: name: test-zone spec: name: test-zone vpcs: - vpcID: vpc-1 vpcRegion: us-west-2 - vpcID: vpc-2 vpcRegion: us-west-2 - vpcID: vpc-3 vpcRegion: us-west-2 ----- # managing a single VPC can use either spec.vpcs or spec.vpc apiVersion: route53.services.k8s.aws/v1alpha1 kind: HostedZone metadata: name: test-zone spec: name: test-zone vpcs: - vpcID: vpc-1 vpcRegion: us-west-2 or apiVersion: route53.services.k8s.aws/v1alpha1 kind: HostedZone metadata: name: test-zone spec: name: test-zone vpc: vpcID: vpc-1 vpcRegion: us-west-2

I like spec.vpcs as a mutually exclusive alternative to spec.vpc - I shied away from this because CreateHostedZone does have a first-class vpc field that mirrors the current ACK API and is required for private hosted zones. So if they're mutually exclusive, we'd need some rule like "spec.vpcs[0] is used as the vpc for the initial CreateHostedZone."

I don't have a strong opinion about which is more or less awkward, neither seems perfect to me (better would be if this was implemented in the AWS HostedZone API directly 😄). I'll defer to whatever you prefer.

There's also the alternative I mentioned in another comment - a new top-level HostedZoneAssociatedVPC type.

There's also the alternative I mentioned in another comment - a new top-level HostedZoneAssociatedVPC type.

Hmm how would disassociating the VPC defined in the HostedZone's spec.vpc work with this approach? I think we'd need to ensure that we don't allow user's to naively unset spec.vpc since that could result in the controller entering a state where the HostedZone can't be re-created after an accidental deletion.

a-hilaly · 2026-03-12T05:47:05Z

/test all

a-hilaly · 2026-03-16T22:36:02Z

/test all

ecordell · 2026-03-17T18:23:02Z

/test all

ack-prow · 2026-03-17T18:23:20Z

@ecordell: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/test all

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/test-infra repository.

michaelhtm · 2026-03-17T18:25:34Z

/ok-to-test

ack-prow · 2026-03-17T18:43:41Z

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

Test name	Commit	Details	Required	Rerun command
route53-verify-code-gen	`2f4d39a`	link	true	`/test route53-verify-code-gen`
route53-release-test	`2f4d39a`	link	true	`/test route53-release-test`
route53-kind-e2e	`2f4d39a`	link	true	`/test route53-kind-e2e`

Full PR test history. Your PR dashboard.

Details

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

knottnt · 2026-03-17T21:53:10Z

      custom_method_name: customUpdateRecordSet
  HostedZone:
+    custom_cel_rules:
+      - rule: "!has(self.hostedZoneConfig) || !self.hostedZoneConfig.privateZone || has(self.vpc)"


p: This case is probably fairly stable, but normally we delegate validation of the resource to the AWS API itself as its the source of truth for the service's validation rules. Performing the validation within the ACK controller runs the risk of our validation becoming out of sync with what the AWS service itself actually allows.

I think the thing that's weird about this case is that each AssociateVPCWithHostedZone is a separate API call and the additionalVPCs don't have a direct representation in CreateHostedZone.

One alternative would be to make a brand new ACK type, like HostedZoneAssociatedVPC that just has spec.vpc and spec.hostedZone?

michaelhtm · 2026-03-18T18:53:39Z

+	// Fetch current VPC associations from AWS.
+	resp, err := client.GetHostedZone(ctx, &svcsdk.GetHostedZoneInput{Id: hostedZoneID})
+	if rm.metrics != nil {
+		rm.metrics.RecordAPICall("READ_ONE", "GetHostedZone", err)
+	}
+	if err != nil {
+		return err
+	}


why do we want to fetch all VPCs again? wouldn't we populate latest from sdkFind?

This was to work around the AWS api not actually having a top-level VPC field (and to address the earlier comment that spec.VPC should be mutable).

latest is a DeepCopy of desired but with values from the AWS API filled in. The upstream api doesn't distinguish a "primary" associated VPC - the associated VPCs are a flat list. So when we get the response we don't know what we should fill in for spec.VPC. There's not enough information between latest and desired to figure out a diff in spec.VPC

An alternative to a second API call here is to store the full associated VPC list as a status field as well, which we can then assume is accurate and diff with the desired state. But that is basically the design in #102 if you prefer it.

michaelhtm · 2026-03-18T18:54:41Z

 // HostedZoneSpec defines the desired state of HostedZone.
 //
 // A complex type that contains general information about the hosted zone.
+// +kubebuilder:validation:XValidation:rule="!has(self.hostedZoneConfig) || !self.hostedZoneConfig.privateZone || has(self.vpc)",message="spec.vpc is required for private hosted zones"


can we validate spec.AdditionalVPCs is also empty if the hostedZone is public?

ecordell · 2026-03-24T13:53:15Z

@michaelhtm @knottnt I appreciate the reviews on this. After reflecting I think that the API in #102 provides a better UX and addresses all of the concerns raised here (and as a bonus, doesn't require the cel validation support in code-generator). Closing this in favor of the other, PTAL?

ack-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 6, 2026

ack-prow bot requested review from a-hilaly and jlbutler March 6, 2026 22:37

ack-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 6, 2026

ecordell marked this pull request as ready for review March 11, 2026 22:08

ack-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2026

ack-prow bot requested a review from michaelhtm March 11, 2026 22:08

michaelhtm reviewed Mar 11, 2026

View reviewed changes

ecordell force-pushed the additionalvpchostedzone branch 3 times, most recently from 228d3b6 to fed7184 Compare March 16, 2026 19:20

ecordell force-pushed the additionalvpchostedzone branch from fed7184 to 0732d39 Compare March 17, 2026 18:10

ack-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 17, 2026

feat: add AdditionalVPCs field to HostedZoneSpec

2f4d39a

ecordell force-pushed the additionalvpchostedzone branch from 0732d39 to 2f4d39a Compare March 17, 2026 18:30

knottnt reviewed Mar 17, 2026

View reviewed changes

michaelhtm reviewed Mar 18, 2026

View reviewed changes

ecordell mentioned this pull request Mar 18, 2026

feat: add spec.vpcs for managing multiple VPC associations on private hosted zones #102

Merged

ecordell closed this Mar 24, 2026

Conversation

ecordell commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-prow bot commented Mar 6, 2026

Uh oh!

ack-prow bot commented Mar 6, 2026

Uh oh!

michaelhtm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecordell Mar 13, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecordell Mar 18, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Mar 12, 2026

Uh oh!

a-hilaly commented Mar 16, 2026

Uh oh!

ecordell commented Mar 17, 2026

Uh oh!

ack-prow bot commented Mar 17, 2026

Uh oh!

michaelhtm commented Mar 17, 2026

Uh oh!

ack-prow bot commented Mar 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecordell commented Mar 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ecordell commented Mar 6, 2026 •

edited

Loading

ecordell Mar 13, 2026 •

edited

Loading

ecordell Mar 18, 2026 •

edited

Loading