Support BYOIP #4450

wweiwei-li · 2025-11-14T17:33:54Z

Description

Support BYOIP (Bring Your Own IP) for Global Accelerator

This change enables users to specify custom IP addresses from their own IP address ranges when creating Global Accelerators, instead of relying solely on AWS-provided IP addresses.

Key changes:

Added IpAddresses field to Accelerator spec to accept BYOIP CIDRs
Implemented CIDR comparison logic to determine when accelerator replacement is required:
1. If any BYOIP CIDR is shared between current and desired → Allow update to preserve traffic
2. If no BYOIP CIDR is shared between current and desired → Trigger replacement and document this behavior
Added HasSharedIPv4CIDR function to check if desired IPs share /24 CIDR blocks with current IPs
Supports API-allowed transitions: adding new BYOIPs or changing IPs within the same CIDR without replacement

Checklist

[x ] Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the docs directory)
[x ] Manually tested
[x ] Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

[feat aga] Add AGA accelerator deployer

k8s-ci-robot · 2025-11-14T17:34:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wweiwei-li

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~~ [wweiwei-li]

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 · 2025-11-15T00:26:30Z

PR needs rebase.

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.

shraddhabang · 2025-11-17T07:26:55Z

pkg/deploy/aga/accelerator_manager_test.go

+			desiredIPs:     []string{"169.254.8.16"},
+			actualIPSets:   makeIPSets([]string{"169.254.9.13", "99.82.158.217"}),
+			expectedResult: false,
+			description:    "Different BYOIP, count matches but IP doesn't",


NIT: Update the description or test case. Count does not match here.

Yes, Let me add one more ip to desiredIPs

shraddhabang · 2025-11-17T07:35:16Z

pkg/deploy/aga/accelerator_synthesizer.go

 }

 // isSDKAcceleratorRequiresReplacement checks whether a sdk Accelerator requires replacement to fulfill an Accelerator resource.
 func isSDKAcceleratorRequiresReplacement(sdkAccelerator AcceleratorWithTags, resAccelerator *agamodel.Accelerator) bool {


Can you add unit tests for isSDKAcceleratorRequiresReplacement so that we can cover different test cases ?
Also can we comment the behavior here on when we consider the replacement or add a task for us to document for it for later?

Yes, Adding it

shraddhabang · 2025-11-17T07:37:12Z

pkg/deploy/aga/accelerator_synthesizer.go

 func isSDKAcceleratorRequiresReplacement(sdkAccelerator AcceleratorWithTags, resAccelerator *agamodel.Accelerator) bool {
-	// The accelerator will only need replacement in BYOIP scenarios. I will implement this later as a separate PR
-	// TODO : BYOIP feature
+	if len(resAccelerator.Spec.IpAddresses) == 0 {


What happens if we had applied BYOIP initially and now we removed it? Do we need a replacement here since we are going from BYOIP to non-BYOIP?

For BYOIP to non-BYOIP transitions, updating is not supported. BYOIP to non-BYOIP will always need replacement. However, if we made it return true, then every non-BYOIP to non-BYOIP transition would cause replacement unless we have a mechanism like below to check if current IPs include BYOIP (we discussed this internally we try not adding this).

if len(resAccelerator.Spec.IpAddresses) == 0 && hasBYOIP(sdkAccelerator) { return true }

so I was thinking for this case we need to document that users need to remove the accelerator first and recreate a new one. This is my intent. Let me try it out to check if it is easy and put together internal documentation for the behavior for discussion. Also I wonder how likely they want to from For BYOIP to non-BYOIP transitions

shraddhabang · 2025-11-17T07:39:46Z

pkg/deploy/aga/accelerator_synthesizer.go

+
+// HasSharedIPv4CIDR checks if current and desired IPs share any IPv4 CIDR block.
+func HasSharedIPv4CIDR(currentIPSets []agatypes.IpSet, desiredIPs []string) bool {
+	if len(desiredIPs) == 0 {


This is not possible since we have already compared length of desiredIPs aka resAccelerator.Spec.IpAddresses to zero and return false before we call this function right?

Yes, since we have checked it at isSDKAcceleratorRequiresReplacement level. We don't need it

shraddhabang · 2025-11-17T07:41:30Z

pkg/deploy/aga/accelerator_synthesizer_test.go

+		description    string
+	}{
+		{
+			name:           "No desired IPs - has shared CIDR",


This test case wont be required if we remove zero len check for desired IPs.

Yes, removing it

shuqz · 2025-11-17T21:37:55Z

pkg/deploy/aga/accelerator_manager.go

-	//	createInput.IpAddresses = resAccelerator.Spec.IpAddresses
-	//}
+	// BYOIP feature: Set IP addresses if provided
+	if len(resAccelerator.Spec.IpAddresses) > 0 {


do we need to do nil check for resAccelerator.Spec and resAccelerator.Spec.IpAddresses

We don't need to do nil check for resAccelerator.Spec and resAccelerator.Spec.IpAddresses. For resAccelerator.Spec, it is a struct. so It can't be nil. No nil check needed. For resAccelerator.Spec.IpAddresses, it is a slice, the len(nil) will return 0 in go.

shuqz · 2025-11-17T21:40:29Z

pkg/deploy/aga/accelerator_synthesizer.go

+		currentIPv4s := extractIPv4Addresses(sdkAccelerator.Accelerator.IpSets)
+		desiredBYOIPs := resAccelerator.Spec.IpAddresses
+		if len(desiredBYOIPs) == 1 {
+			s.logger.Info("Accelerator requires replacement: BYOIP CIDR change",


do we want to keep it as v(1) logging

I am thinking we should keep it as it is since accelerator replacement is a significant operation that we want it to be accessible by default. Let me know what do you think

shuqz · 2025-11-17T21:44:23Z

pkg/deploy/aga/accelerator_synthesizer.go

 func isSDKAcceleratorRequiresReplacement(sdkAccelerator AcceleratorWithTags, resAccelerator *agamodel.Accelerator) bool {
-	// The accelerator will only need replacement in BYOIP scenarios. I will implement this later as a separate PR
-	// TODO : BYOIP feature
+	if len(resAccelerator.Spec.IpAddresses) == 0 {


shraddhabang and others added 7 commits November 9, 2025 17:56

setup AGA SDK client

b1e26b3

add aga crd status updates utils

656ad86

add aga tags reconciler

36b6306

add aga controller config flags

75d1eb7

add aga deployer

c588936

Merge pull request kubernetes-sigs#4434 from shraddhabang/agaaccdeployer

719a4cd

[feat aga] Add AGA accelerator deployer

Support BYOIP

74b2b3c

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 14, 2025

k8s-ci-robot requested a review from M00nF1sh November 14, 2025 17:34

k8s-ci-robot requested a review from zac-nixon November 14, 2025 17:34

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 14, 2025

shuqz force-pushed the AGAController branch from 719a4cd to a3b5439 Compare November 14, 2025 21:33

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2025

shraddhabang reviewed Nov 17, 2025

View reviewed changes

shuqz reviewed Nov 17, 2025

View reviewed changes

Support BYOIP #4450

Are you sure you want to change the base?

Support BYOIP #4450

Conversation

wweiwei-li commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Uh oh!

k8s-ci-robot commented Nov 14, 2025

Uh oh!

k8s-ci-robot commented Nov 15, 2025

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wweiwei-li Nov 18, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wweiwei-li commented Nov 14, 2025 •

edited

Loading

wweiwei-li Nov 18, 2025 •

edited

Loading