Enhanced Subnet Discovery #3380

dshehbaj · 2025-07-30T16:37:46Z

What type of PR is this?
feature

Which issue does this PR fix?:

#3067
#2904

What does this PR do / Why do we need it?:

This PR adds a requested customer feature to allow customers to

Include or exclude subnets/security groups by tagging them kubernetes.io/role/cni=1 or kubernetes.io/role/cni=0 respectively.
Apply custom security groups for secondary ENIs created using kubernetes.io/role/cni=1 tagged subnet if the alternate security group is also tagged kubernetes.io/role/cni=1.
Reserve subnets for specific cluster based on the kubernetes.io/cluster/<my-example-cluster> tag. See Enhanced subnet discovery should use configurable tags #2904 more.

Testing done on this change:

Added unit tests and integration tests.
Manual testing pending.

Will this PR introduce any new dependencies?:

Will this break upgrades or downgrades? Has updating a running cluster been tested?:

Manual testing pending.

Does this change require updates to the CNI daemonset config files to work?:

Requires ENABLE_SUBNET_DISCOVERY enabled which is already done by default.

Does this PR introduce any user-facing change?:

Users were already tagging their subnets as the part of the original subnet discovery feature. This enhanced feature will also allow users to tag alternate security groups.
Make sure that IPv6 documentation and managed IPv4 policy are updated.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Integration tests passing in IPv4 cluster.

Currently adapting our test framework to work with IPv6 cluster. Will paste results here.

dshehbaj · 2025-07-30T21:04:38Z

pkg/ipamd/ipamd.go

+	// Mark primary ENI as excluded if primary subnet is excluded
+	if eni == primaryENI && c.isPrimarySubnetExcluded {
+		log.Infof("Marking primary ENI %s as excluded from pod IP allocation", eni)
+		err := c.dataStoreAccess.GetDataStore(eniMetadata.NetworkCard).SetENIExcludedForPodIPs(eni, true)


Check if eniMetadata.NetworkCard is the same network card as 0.

dshehbaj · 2025-08-04T14:59:40Z

pkg/awsutils/awsutils_test.go

+			name: "IPv6 enabled with subnet discovery and mixed IPv4/IPv6 subnets",
+			subnets: []ec2types.Subnet{


Need to check edge case in code if customer is on IPv4 cluster and the code picks up an IPv6 subnet because that's all they had tagged in the console and vice versa.

pkg/awsutils/awsutils.go

dshehbaj · 2025-08-05T21:47:03Z

pkg/awsutils/awsutils.go

+			// When subnet discovery is disabled, check if primary subnet is excluded
+			excluded, checkErr := cache.isPrimarySubnetExcluded()
+			if checkErr != nil {
+				// If we can't determine exclusion status, log warning and proceed
+				log.Warnf("Failed to check if primary subnet is excluded: %v. Proceeding with ENI creation attempt.", checkErr)
+			} else if excluded {
+				// Primary subnet is explicitly excluded
+				err = errors.New("primary subnet is tagged with kubernetes.io/role/cni=0 and subnet discovery is disabled - no valid subnets available for ENI creation")
+				log.Error(err.Error())
+				return "", err
+			}


This checks if Cx has a tagged subnet, but have not enable subnet discovery. Do we want to do this check?

dshehbaj · 2025-08-05T22:07:00Z

pkg/ipamd/ipamd.go

+		if c.enableIPv6 {
+			assignedIPs = primaryENIInfo.AssignedIPv6Addresses()
+			ipType = "IPv6"
+		} else {
+			assignedIPs = primaryENIInfo.AssignedIPv4Addresses()
+			ipType = "IPv4"
+		}


See if we can use hasPods() function.

Currently hasPods() only checks for IPv4 assigned addresses, will need to update that func to support IPv6.

dshehbaj · 2025-08-06T15:52:38Z

pkg/ipamd/datastore/data_store.go

+		// Skip ENIs that are excluded from pod IP allocation
+		if eni.IsExcludedForPodIPs {
+			ds.log.Debugf("Skip needs IP check for ENI %s as it is excluded from pod IP allocation", eni.ID)
+			continue
+		}


This will skip the primary ENI in GetAllocatableENIs function and prevent it from getting IPs assigned/allocated

dshehbaj · 2025-08-07T17:38:31Z

pkg/ipamd/ipamd_test.go

+	assert.False(t, wasUsed, "Primary ENI should be excluded and auto-cleanup prefixes")
+}
+
+func TestPrimaryENIAutoCleanupFailureHandling(t *testing.T) {


Add case for IPv6 mode too

haouc · 2025-08-10T02:55:37Z

pkg/awsutils/awsutils.go

+	describeSGInput := &ec2.DescribeSecurityGroupsInput{
+		Filters: []ec2types.Filter{
+			{
+				Name:   aws.String("vpc-id"),


We need support global SGs which could be in another VPCs.

I don't believe we support VPC peering in CNI. I could not find any existing logic that I could leverage.

I think keeping it local VPC only might keep things simpler, and we could do VPC peering as a follow up.

What do you think?

global SGs is not VPC peering. EC2 support using SG from another VPC.

haouc · 2025-08-10T02:57:02Z

pkg/awsutils/awsutils.go

+			},
+			{
+				Name:   aws.String("tag:" + subnetDiscoveryTagKey),
+				Values: []string{"1"},


nit: let's make them to const, such as selected = 1.

haouc · 2025-08-15T19:51:33Z

pkg/awsutils/awsutils.go

+		eniIDs = append(eniIDs, cache.getFilteredENIs(ctx, ds, true)...) // only secondary subnet ENIs
+	}
+
+	cache.applySecurityGroupsToENIs(ctx, eniIDs, primarySGs, "Applying primary security groups to")


We should have error to handle.

haouc · 2025-08-15T19:52:14Z

pkg/awsutils/awsutils.go

+		return err
+	}
+
+	addedCount, deletedCount := cache.detectSecurityGroupChanges(sgIDs, &cache.securityGroups, "primary")


Same arbitary argument being passed through.

haouc · 2025-08-15T19:53:40Z

pkg/awsutils/awsutils.go

+				primarySubnetENIs := cache.getFilteredENIs(ctx, ds, false)
+				for _, eniID := range primarySubnetENIs {
+					// Filter out unmanaged ENIs
+					if !cache.unmanagedENIs.Has(eniID) {


nit: I feel unmanaged ENIs should be handled in cache.getFilteredENIs().

haouc · 2025-08-15T19:55:43Z

pkg/awsutils/awsutils.go

 		}
+
+		// Apply security groups to the filtered ENIs
+		cache.applySecurityGroupsToENIs(ctx, eniIDs, sgIDs, "Update")


haouc · 2025-08-15T20:02:43Z

pkg/awsutils/awsutils.go

+					log.Warnf("Failed to check if primary subnet is excluded: %v. Proceeding with ENI creation attempt.", checkErr)
+				} else if excluded {
+					// Primary subnet is explicitly excluded
+					return "", fmt.Errorf("primary subnet is tagged with kubernetes.io/role/cni=0 - no valid subnets available for ENI creation")


I suggest surface this error to customers.

dshehbaj · 2025-08-18T08:18:39Z

pkg/awsutils/awsutils.go

 			if vpcErr != nil {
 				log.Warnf("Failed to call ec2:DescribeSubnets: %v", vpcErr)
 				log.Info("Defaulting to same subnet as the primary interface for the new ENI")

 				// Even in fallback, check if primary subnet is excluded
-				excluded, checkErr := cache.IsPrimarySubnetExcluded(ctx)
+				excluded, checkErr := cache.IsSubnetExcluded(ctx, cache.subnetID)


use primarySubnetID = cache.subnetID

…xcluded

dshehbaj requested a review from a team as a code owner July 30, 2025 16:37

dshehbaj force-pushed the shehbaj/enhanced-subnet-discovery-rebase branch from 96310d6 to 0a64e10 Compare July 30, 2025 20:54

dshehbaj changed the title ~~pkg awsutils: check for primary/secondary subnet exclusion~~ Enhanced Subnet Discovery Jul 30, 2025

dshehbaj commented Jul 30, 2025

View reviewed changes

dshehbaj force-pushed the shehbaj/enhanced-subnet-discovery-rebase branch 6 times, most recently from f1898c7 to 7945b5a Compare August 4, 2025 14:56

dshehbaj commented Aug 4, 2025

View reviewed changes

dshehbaj force-pushed the shehbaj/enhanced-subnet-discovery-rebase branch 6 times, most recently from 903a226 to 0368ae0 Compare August 4, 2025 16:31

dshehbaj commented Aug 5, 2025

View reviewed changes

pkg/awsutils/awsutils.go Outdated Show resolved Hide resolved

dshehbaj commented Aug 5, 2025

View reviewed changes

pkg/awsutils/awsutils.go Outdated Show resolved Hide resolved

dshehbaj commented Aug 5, 2025

View reviewed changes

dshehbaj commented Aug 6, 2025

View reviewed changes

dshehbaj force-pushed the shehbaj/enhanced-subnet-discovery-rebase branch from 0368ae0 to ea0763f Compare August 6, 2025 17:47

dshehbaj requested a review from haouc August 6, 2025 18:15

dshehbaj commented Aug 7, 2025

View reviewed changes

dshehbaj force-pushed the shehbaj/enhanced-subnet-discovery-rebase branch 3 times, most recently from 587b9f3 to ea0763f Compare August 8, 2025 18:11

haouc reviewed Aug 10, 2025

View reviewed changes

haouc reviewed Aug 15, 2025

View reviewed changes

dshehbaj commented Aug 18, 2025

View reviewed changes

dshehbaj force-pushed the shehbaj/enhanced-subnet-discovery-rebase branch 5 times, most recently from 9036276 to a188827 Compare August 18, 2025 16:30

dshehbaj added 13 commits August 20, 2025 13:47

pkg awsutils

f75e0d4

pkg ec2wrapper: add func to describe subnets

4cbb7e5

pkg ipamd: handle primary ENI exclusion

eb49aa0

test: add integration test

d2554ce

pkg: enable subnet discovery for IPv6 mode

566dbfb

test integration eni-subnet-discovery: extend tests for IPv6 mode

bdd3111

pkg: use shared context for awsutils and ipamd

ef3c335

pkg awsutils: fix compilation error

1e46f0b

pkg ipamd: clean up ENI if not previously used

e25150a

pkg ipamd: gracefully exclude primary ENI if existing pods

29c38b6

test integration: add integration test for primary ENI exclusion

876729c

pkg ipamd: fix bug when calculating currentENIs when primary ENI is e…

0ada9a6

…xcluded

pkg ipamd: implement secondary ENI exclusion logic

1d749c9

dshehbaj force-pushed the shehbaj/enhanced-subnet-discovery-rebase branch 2 times, most recently from f0bc83a to 1d749c9 Compare August 21, 2025 02:11

pkg ipamd: add unit test for secondary ENI exclusion

34be861

dshehbaj force-pushed the shehbaj/enhanced-subnet-discovery-rebase branch from 8baff5a to 34be861 Compare August 21, 2025 11:07

dshehbaj added 2 commits August 21, 2025 12:33

test integration: add integ test for secondary ENI exclusion testing

e430ecf

test integration: add test for security group refresh

892192a

		name: "IPv6 enabled with subnet discovery and mixed IPv4/IPv6 subnets",
		subnets: []ec2types.Subnet{

Enhanced Subnet Discovery #3380

Are you sure you want to change the base?

Enhanced Subnet Discovery #3380

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dshehbaj Aug 4, 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!

Uh oh!

Uh oh!

dshehbaj Aug 5, 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!

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!

Uh oh!

dshehbaj commented Jul 30, 2025 •

edited

Loading

dshehbaj Aug 4, 2025 •

edited

Loading

dshehbaj Aug 5, 2025 •

edited

Loading