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.

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-10T19:19:53Z

pkg/awsutils/awsutils.go

+
+// RefreshSGIDs retrieves security groups
+func (cache *EC2InstanceMetadataCache) RefreshSGIDs(mac string, dsAccess *datastore.DataStoreAccess) error {
+	ctx := context.TODO()


We should generalize the context is used across API calls.

Added shared context for all functions across awsutils and ipamd.

haouc · 2025-08-10T19:22:03Z

pkg/awsutils/awsutils.go

+}
+
+// RefreshSGIDs retrieves security groups
+func (cache *EC2InstanceMetadataCache) RefreshSGIDs(mac string, dsAccess *datastore.DataStoreAccess) error {


What event triggers this func?

We call this here in IPAMD: https://github.com/aws/amazon-vpc-cni-k8s/blob/master/pkg/ipamd/ipamd.go#L565-L579

SInce I also made a RefreshCustomSGID, I refactored this function to share the logic between the two.

haouc · 2025-08-10T19:23:56Z

pkg/awsutils/awsutils.go

+			for _, ds := range dsAccess.DataStores {
+				eniInfos := ds.GetENIInfos()
+				for eniID := range eniInfos.ENIs {
+					eniIDs = append(eniIDs, eniID)


Would eniIDs := append(eniIDs, ds.GetENIInfos()...) just work?

ds.GetENIInfos() returns *datastore.ENIInfos, and then you iterate through each ENI by going over the map

*datastore.ENIInfos.ENIs.

So not straightforward to do it in one line unless we want to do something like this.

eniIDs := slices.Collect(maps.Keys(ds.GetENIInfos().ENIs))

haouc · 2025-08-10T19:30:49Z

pkg/awsutils/awsutils.go

 					}
-					log.Infof("Creating ENI with security groups: %v in subnet: %s", input.Groups, aws.ToString(input.SubnetId))
+					log.Infof("Creating ENI with security groups: %v in subnet: %s", input.Groups, aws.ToString(subnet.SubnetId))


Looks we added many more log lines, can you recheck them to maintain them to minimal level.

haouc · 2025-08-10T19:31:45Z

pkg/awsutils/awsutils.go

+
+				// If no valid subnets found, return appropriate error
+				if !validSubnetsFound {
+					err = errors.New("no valid subnets available for ENI creation - all subnets are either not tagged or tagged with kubernetes.io/role/cni=0")


Can you check the similar pattern to use one line return instead of three lines.

haouc · 2025-08-10T19:37:32Z

pkg/ipamd/datastore/data_store.go

+
+	eni.IsExcludedForPodIPs = excluded
+	if excluded {
+		ds.log.Infof("ENI %s marked as excluded from pod IP allocation", eniID)


Let's avoid logging if possible especially when the code is under lock mechanism.

haouc · 2025-08-10T19:39:14Z

pkg/ipamd/datastore/data_store.go

@@ -636,6 +679,12 @@ func (ds *DataStore) AssignPodIPv6Address(ipamKey IPAMKey, ipamMetadata IPAMMeta

 	// In IPv6 Prefix Delegation mode, eniPool will only have Primary ENI.
 	for _, eni := range ds.eniPool {
+		// Skip ENIs that are excluded from pod IP allocation
+		if eni.IsExcludedForPodIPs {
+			ds.log.Debugf("Skipping ENI %s as it is excluded from pod IP allocation", eni.ID)


Unnecessary logging

haouc · 2025-08-10T19:41:12Z

pkg/awsutils/awsutils.go

-						awsAPIErrInc("IMDSMetaDataOutOfSync", err)
+	cache.applySecurityGroupsToENIs(eniIDs, primarySGs, "Applying primary security groups to")
+
+	return nil


If not returning errors, the func doesn't need error in return type.

haouc · 2025-08-10T19:41:27Z

pkg/awsutils/awsutils.go

+			newENIs := StringSet{}
+			newENIs.Set(eniIDs)
+			filteredENIs := newENIs.Difference(&cache.unmanagedENIs)
+			eniIDs = filteredENIs.SortedList()


I saw ENI IDs and SGs being sorted everywhere and not sure why they need to be sorted.

Currently the only two functions to convert a StringSet to a []string is either calling StringSet.Set() or StringSet.SortedList().

We have used SortedList() everywhere we are using StringSet even where sorting is not required so I just decided to stick with that convention.

haouc · 2025-08-10T19:41:39Z

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
+					err = errors.New("primary subnet is tagged with kubernetes.io/role/cni=0 - no valid subnets available for ENI creation")


return fmt.Errorf("") does the same with one line.

haouc · 2025-08-10T19:41:58Z

pkg/awsutils/awsutils.go

@@ -992,33 +1173,79 @@ func (cache *EC2InstanceMetadataCache) createENI(sg []*string, eniCfgSubnet stri
 			return networkInterfaceID, nil
 		}
 	} else {
-		if cache.useSubnetDiscovery && !cache.v6Enabled {


Should v6 check be removed at here? If new logic need check v6, should v6 check be maintained somewhere to keep existing flow?

Yes, we would need to remove the v6 check here to make it work with secondary ENIs/subnets.

Right now it would just create the secondary ENI using the same settings as the primary ENI:

amazon-vpc-cni-k8s/pkg/awsutils/awsutils.go

Lines 1020 to 1026 in 312b1d2

} else {

log.Info("Using same security group config as the primary interface for the new ENI")

networkInterfaceID, err = cache.tryCreateNetworkInterface(input)

if err == nil {

return networkInterfaceID, nil

}

}

haouc · 2025-08-10T19:42:21Z

pkg/awsutils/awsutils.go

+// IsPrimarySubnetExcluded implements the APIs interface to check if primary subnet is excluded
+func (cache *EC2InstanceMetadataCache) IsPrimarySubnetExcluded() (bool, error) {
+	// Always check tags - explicit user intent should be respected regardless of subnet discovery setting
+	return cache.isPrimarySubnetExcluded()


Not sure why we need two func which are doing same thing. If no extra logic, can't just use IsPrimarySubnetExcluded with isPrimarySubnetExcluded's code?

Combined these two functions.

Was trying to encapsulate the logic and have one function called internal only, and the other one externally, but since there is no difference in logic makes sense to combine them to keep things simple.

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

dshehbaj force-pushed the shehbaj/enhanced-subnet-discovery-rebase branch 7 times, most recently from fea4982 to e0c2575 Compare August 10, 2025 23:51

dshehbaj added 7 commits August 10, 2025 17:07

pkg awsutils

366c7f8

pkg ec2wrapper: add func to describe subnets

9742319

pkg ipamd: handle primary ENI exclusion

d9cec4c

test: add integration test

1b0fe7c

pkg: enable subnet discovery for IPv6 mode

5e3d679

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

5d01dc6

pkg: use shared context for awsutils and ipamd

b649f35

dshehbaj force-pushed the shehbaj/enhanced-subnet-discovery-rebase branch from e0c2575 to b649f35 Compare August 11, 2025 00:08

pkg awsutils

a0f7951

dshehbaj force-pushed the shehbaj/enhanced-subnet-discovery-rebase branch from 7432ce6 to a0f7951 Compare August 11, 2025 01:04

pkg awsutils: fix test

2a9afd4

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

	} else {
	log.Info("Using same security group config as the primary interface for the new ENI")
	networkInterfaceID, err = cache.tryCreateNetworkInterface(input)
	if err == nil {
	return networkInterfaceID, nil
	}
	}

Enhanced Subnet Discovery #3380

Are you sure you want to change the base?

Enhanced Subnet Discovery #3380

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!

haouc Aug 10, 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!

haouc Aug 10, 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!

Uh oh!

dshehbaj commented Jul 30, 2025 •

edited

Loading

dshehbaj Aug 4, 2025 •

edited

Loading

dshehbaj Aug 5, 2025 •

edited

Loading

haouc Aug 10, 2025 •

edited

Loading

haouc Aug 10, 2025 •

edited

Loading