Skip to content

Commit 76ef0ad

Browse files
haouchaouc
committed
primary ENI can't be easily ignored
1 parent 81844e8 commit 76ef0ad

7 files changed

Lines changed: 69 additions & 101 deletions

File tree

‎pkg/awsutils/awsutils.go‎

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ type APIs interface {
237237
GetVpcSubnets(ctx context.Context) ([]ec2types.Subnet, error)
238238

239239
// IsSubnetExcluded returns if a subnet is excluded for pod IPs based on its tags
240-
IsSubnetExcluded(ctx context.Context, subnetID string, isPrimarySubnet bool) (bool, error)
240+
IsSubnetExcluded(ctx context.Context, subnetID string) (bool, error)
241241
}
242242

243243
// EC2InstanceMetadataCache caches instance metadata
@@ -1206,7 +1206,7 @@ func (cache *EC2InstanceMetadataCache) createENI(ctx context.Context, sg []*stri
12061206
log.Info("Defaulting to same subnet as the primary interface for the new ENI")
12071207

12081208
// Even in fallback, check if primary subnet is excluded
1209-
excluded, checkErr := cache.IsSubnetExcluded(ctx, cache.subnetID, true)
1209+
excluded, checkErr := cache.IsSubnetExcluded(ctx, cache.subnetID)
12101210
if checkErr != nil {
12111211
return "", fmt.Errorf("Failed to check if primary subnet is excluded: %w. Quit ENI creation attempt.", checkErr)
12121212
} else if excluded {
@@ -1260,7 +1260,7 @@ func (cache *EC2InstanceMetadataCache) createENI(ctx context.Context, sg []*stri
12601260
} else {
12611261
log.Info("Using same security group config as the primary interface for the new ENI")
12621262
// When subnet discovery is disabled, check if primary subnet is excluded
1263-
excluded, checkErr := cache.IsSubnetExcluded(ctx, cache.subnetID, true)
1263+
excluded, checkErr := cache.IsSubnetExcluded(ctx, cache.subnetID)
12641264
if checkErr != nil {
12651265
// If we can't determine exclusion status, log warning and proceed
12661266
log.Warnf("Failed to check if primary subnet is excluded: %v. Proceeding with ENI creation attempt.", checkErr)
@@ -1339,7 +1339,7 @@ func isSubnetValidForENICreation(subnet ec2types.Subnet, isPrimarySubnet bool) b
13391339
}
13401340

13411341
// Rule 3: Check cluster-specific tags
1342-
if ValidSubnetForCluster(subnet, isPrimarySubnet) {
1342+
if ValidSubnetTagsMatchingClusterName(subnet) {
13431343
return true
13441344
}
13451345

@@ -1349,13 +1349,8 @@ func isSubnetValidForENICreation(subnet ec2types.Subnet, isPrimarySubnet bool) b
13491349
}
13501350

13511351
// ValidSubnetForCluster checks if a subnet is valid for use by this cluster
1352-
// For primary subnets, they are always valid for any cluster
13531352
// For secondary subnets, they must either have no cluster tags or have a matching cluster tag
1354-
func ValidSubnetForCluster(subnet ec2types.Subnet, isPrimarySubnet bool) bool {
1355-
if isPrimarySubnet {
1356-
// Primary subnets are always valid for any cluster
1357-
return true
1358-
}
1353+
func ValidSubnetTagsMatchingClusterName(subnet ec2types.Subnet) bool {
13591354
// Get cluster name for cluster-specific tag checks
13601355
localClusterName := os.Getenv(clusterNameEnvVar)
13611356
localClusterTagKey := clusterTagKeyPrefix + localClusterName
@@ -2627,7 +2622,7 @@ func checkAPIErrorAndBroadcastEvent(err error, api string) {
26272622
}
26282623

26292624
// IsSubnetExcluded checks if a subnet is excluded by examining its kubernetes.io/role/cni tag
2630-
func (cache *EC2InstanceMetadataCache) IsSubnetExcluded(ctx context.Context, subnetID string, isPrimarySubnet bool) (bool, error) {
2625+
func (cache *EC2InstanceMetadataCache) IsSubnetExcluded(ctx context.Context, subnetID string) (bool, error) {
26312626
// Get all VPC subnets with their tags
26322627
subnets, err := cache.GetVpcSubnets(ctx)
26332628
if err != nil {
@@ -2637,8 +2632,8 @@ func (cache *EC2InstanceMetadataCache) IsSubnetExcluded(ctx context.Context, sub
26372632
// Find the specific subnet and check its tags
26382633
for _, subnet := range subnets {
26392634
if *subnet.SubnetId == subnetID {
2640-
if !ValidSubnetForCluster(subnet, isPrimarySubnet) {
2641-
log.Debugf("IsSubnetExcluded: subnet %s doesn't have valid cluster tag", subnetID)
2635+
if !ValidSubnetTagsMatchingClusterName(subnet) {
2636+
log.Debugf("IsSubnetExcluded: subnet %s doesn't have valid cluster name tag", subnetID)
26422637
return true, nil
26432638
}
26442639
// Check if the subnet has the exclusion tag kubernetes.io/role/cni=0

‎pkg/awsutils/awsutils_test.go‎

Lines changed: 33 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -2645,12 +2645,11 @@ func TestIsSubnetExcluded(t *testing.T) {
26452645
os.Setenv(clusterNameEnvVar, "test-cluster")
26462646

26472647
tests := []struct {
2648-
name string
2649-
subnetTags []ec2types.Tag
2650-
isPrimarySubnet bool
2651-
describeError error
2652-
want bool
2653-
wantErr bool
2648+
name string
2649+
subnetTags []ec2types.Tag
2650+
describeError error
2651+
want bool
2652+
wantErr bool
26542653
}{
26552654
{
26562655
name: "subnet with tag value 0 - excluded",
@@ -2660,9 +2659,8 @@ func TestIsSubnetExcluded(t *testing.T) {
26602659
Value: aws.String("0"),
26612660
},
26622661
},
2663-
isPrimarySubnet: false,
2664-
want: true,
2665-
wantErr: false,
2662+
want: true,
2663+
wantErr: false,
26662664
},
26672665
{
26682666
name: "subnet with tag value 1 - not excluded",
@@ -2672,9 +2670,8 @@ func TestIsSubnetExcluded(t *testing.T) {
26722670
Value: aws.String("1"),
26732671
},
26742672
},
2675-
isPrimarySubnet: false,
2676-
want: false,
2677-
wantErr: false,
2673+
want: false,
2674+
wantErr: false,
26782675
},
26792676
{
26802677
name: "subnet without tag - not excluded",
@@ -2684,12 +2681,11 @@ func TestIsSubnetExcluded(t *testing.T) {
26842681
Value: aws.String("value"),
26852682
},
26862683
},
2687-
isPrimarySubnet: false,
2688-
want: false,
2689-
wantErr: false,
2684+
want: false,
2685+
wantErr: false,
26902686
},
26912687
{
2692-
name: "secondary subnet with different cluster tag - excluded by cluster validation",
2688+
name: "subnet with different cluster tag - excluded by cluster validation",
26932689
subnetTags: []ec2types.Tag{
26942690
{
26952691
Key: aws.String("kubernetes.io/role/cni"),
@@ -2700,12 +2696,11 @@ func TestIsSubnetExcluded(t *testing.T) {
27002696
Value: aws.String("shared"),
27012697
},
27022698
},
2703-
isPrimarySubnet: false,
2704-
want: true,
2705-
wantErr: false,
2699+
want: true,
2700+
wantErr: false,
27062701
},
27072702
{
2708-
name: "secondary subnet with matching cluster tag - not excluded",
2703+
name: "subnet with matching cluster tag - not excluded",
27092704
subnetTags: []ec2types.Tag{
27102705
{
27112706
Key: aws.String("kubernetes.io/role/cni"),
@@ -2716,46 +2711,26 @@ func TestIsSubnetExcluded(t *testing.T) {
27162711
Value: aws.String("shared"),
27172712
},
27182713
},
2719-
isPrimarySubnet: false,
2720-
want: false,
2721-
wantErr: false,
2714+
want: false,
2715+
wantErr: false,
27222716
},
27232717
{
2724-
name: "primary subnet always valid regardless of cluster tag",
2725-
subnetTags: []ec2types.Tag{
2726-
{
2727-
Key: aws.String("kubernetes.io/role/cni"),
2728-
Value: aws.String("1"),
2729-
},
2730-
{
2731-
Key: aws.String("kubernetes.io/cluster/other-cluster"),
2732-
Value: aws.String("shared"),
2733-
},
2734-
},
2735-
isPrimarySubnet: true,
2736-
want: false,
2737-
wantErr: false,
2738-
},
2739-
{
2740-
name: "GetVpcSubnets API error",
2741-
describeError: errors.New("API error"),
2742-
isPrimarySubnet: false,
2743-
want: false,
2744-
wantErr: true,
2718+
name: "GetVpcSubnets API error",
2719+
describeError: errors.New("API error"),
2720+
want: false,
2721+
wantErr: true,
27452722
},
27462723
{
2747-
name: "no subnets returned",
2748-
subnetTags: []ec2types.Tag{},
2749-
isPrimarySubnet: false,
2750-
want: false,
2751-
wantErr: false,
2724+
name: "no subnets returned",
2725+
subnetTags: []ec2types.Tag{},
2726+
want: false,
2727+
wantErr: false,
27522728
},
27532729
{
2754-
name: "subnet not found in VPC - should error",
2755-
subnetTags: []ec2types.Tag{},
2756-
isPrimarySubnet: false,
2757-
want: false,
2758-
wantErr: true,
2730+
name: "subnet not found in VPC - should error",
2731+
subnetTags: []ec2types.Tag{},
2732+
want: false,
2733+
wantErr: true,
27592734
},
27602735
}
27612736

@@ -2790,7 +2765,7 @@ func TestIsSubnetExcluded(t *testing.T) {
27902765
mockEC2.EXPECT().DescribeSubnets(gomock.Any(), gomock.Any()).Return(subnetResult, nil)
27912766
}
27922767

2793-
got, err := cache.IsSubnetExcluded(context.Background(), subnetID, tt.isPrimarySubnet)
2768+
got, err := cache.IsSubnetExcluded(context.Background(), subnetID)
27942769
if tt.wantErr {
27952770
assert.Error(t, err)
27962771
} else {
@@ -3877,7 +3852,7 @@ func TestDataStoreENISubnetID(t *testing.T) {
38773852

38783853
func TestIsPrimaryENI(t *testing.T) {
38793854
cache := &EC2InstanceMetadataCache{primaryENI: "eni-primary"}
3880-
3855+
38813856
assert.True(t, cache.IsPrimaryENI("eni-primary"))
38823857
assert.False(t, cache.IsPrimaryENI("eni-secondary"))
38833858
assert.False(t, cache.IsPrimaryENI(""))
@@ -3887,7 +3862,7 @@ func TestIsEfaOnlyENI(t *testing.T) {
38873862
cache := &EC2InstanceMetadataCache{
38883863
efaOnlyENIsByNetworkCard: []string{"", "eni-efa"},
38893864
}
3890-
3865+
38913866
assert.True(t, cache.IsEfaOnlyENI(1, "eni-efa"))
38923867
assert.False(t, cache.IsEfaOnlyENI(0, "eni-efa"))
38933868
assert.False(t, cache.IsEfaOnlyENI(1, "eni-other"))
@@ -3896,7 +3871,7 @@ func TestIsEfaOnlyENI(t *testing.T) {
38963871
func TestIsUnmanagedENI(t *testing.T) {
38973872
cache := &EC2InstanceMetadataCache{unmanagedENIs: StringSet{}}
38983873
cache.unmanagedENIs.Set([]string{"eni-unmanaged"})
3899-
3874+
39003875
assert.True(t, cache.IsUnmanagedENI("eni-unmanaged"))
39013876
assert.False(t, cache.IsUnmanagedENI("eni-managed"))
39023877
assert.False(t, cache.IsUnmanagedENI(""))

‎pkg/awsutils/mocks/awsutils_mocks.go‎

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/ipamd/datastore/data_store_test.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2335,4 +2335,3 @@ func TestDataStore_FindFreeableCidrs(t *testing.T) {
23352335
cidrs := ds.FindFreeableCidrs(eniID)
23362336
assert.Len(t, cidrs, 1)
23372337
}
2338-

‎pkg/ipamd/ipamd.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -396,7 +396,6 @@ func (c *IPAMContext) inInsufficientCidrCoolingPeriod() bool {
396396
// then initializes IP address pool data store
397397
func New(ctx context.Context, k8sClient client.Client, withApiServer bool) (*IPAMContext, error) {
398398
prometheusRegister()
399-
ctx := context.Background()
400399
c := &IPAMContext{}
401400
c.k8sClient = k8sClient
402401
c.networkClient = networkutils.New()
@@ -2871,7 +2870,7 @@ func (c *IPAMContext) SetAPIServerConnectivity(connected bool) {
28712870
func (c *IPAMContext) excludedENIBasedOnSubnetTags(ctx context.Context, eni string, eniMetadata awsutils.ENIMetadata) (bool, error) {
28722871
primaryENI := c.awsClient.GetPrimaryENI()
28732872
// Check if this ENI (primary or secondary) is in an excluded subnet and mark it for exclusion
2874-
excluded, err := c.awsClient.IsSubnetExcluded(ctx, eniMetadata.SubnetID, eni == primaryENI)
2873+
excluded, err := c.awsClient.IsSubnetExcluded(ctx, eniMetadata.SubnetID)
28752874
if err != nil {
28762875
log.Warnf("setupENI: failed to check subnet exclusion for ENI %s (subnet %s): %v", eni, eniMetadata.SubnetID, err)
28772876
return false, err

0 commit comments

Comments
 (0)