From 9fa0e70aa41ea3fd48732158e125c0de91363601 Mon Sep 17 00:00:00 2001 From: Yash Thakkar Date: Tue, 17 Sep 2024 21:13:13 +0000 Subject: [PATCH 1/3] adding function to compare list --- pkg/ipamd/ipamd.go | 15 ++++++----- pkg/utils/cniutils/cni_utils.go | 48 +++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 7 deletions(-) diff --git a/pkg/ipamd/ipamd.go b/pkg/ipamd/ipamd.go index b57dec2a1b..deedcc4fff 100644 --- a/pkg/ipamd/ipamd.go +++ b/pkg/ipamd/ipamd.go @@ -43,6 +43,7 @@ import ( "github.com/aws/amazon-vpc-cni-k8s/pkg/ipamd/datastore" "github.com/aws/amazon-vpc-cni-k8s/pkg/k8sapi" "github.com/aws/amazon-vpc-cni-k8s/pkg/networkutils" + "github.com/aws/amazon-vpc-cni-k8s/pkg/utils/cniutils" "github.com/aws/amazon-vpc-cni-k8s/pkg/utils/logger" "github.com/aws/amazon-vpc-cni-k8s/utils" "github.com/aws/amazon-vpc-cni-k8s/utils/prometheusmetrics" @@ -1455,8 +1456,8 @@ func (c *IPAMContext) eniIPPoolReconcile(ipPool []string, attachedENI awsutils.E attachedENIIPs := attachedENI.IPv4Addresses needEC2Reconcile := true // Here we can't trust attachedENI since the IMDS metadata can be stale. We need to check with EC2 API. - // +1 is for the primary IP of the ENI that is not added to the ipPool and not available for pods to use. - if 1+len(ipPool) != len(attachedENIIPs) { + // IPsSimilar will exclude primary IP of the ENI that is not added to the ipPool and not available for pods to use. + if !cniutils.IPsSimilar(ipPool,attachedENIIPs) { log.Warnf("Instance metadata does not match data store! ipPool: %v, metadata: %v", ipPool, attachedENIIPs) log.Debugf("We need to check the ENI status by calling the EC2 control plane.") // Call EC2 to verify IPs on this ENI @@ -1492,14 +1493,14 @@ func (c *IPAMContext) eniIPPoolReconcile(ipPool []string, attachedENI awsutils.E } } -func (c *IPAMContext) eniPrefixPoolReconcile(ipPool []string, attachedENI awsutils.ENIMetadata, eni string) { +func (c *IPAMContext) eniPrefixPoolReconcile(prefixPool []string, attachedENI awsutils.ENIMetadata, eni string) { attachedENIIPs := attachedENI.IPv4Prefixes needEC2Reconcile := true // Here we can't trust attachedENI since the IMDS metadata can be stale. We need to check with EC2 API. - log.Debugf("Found prefix pool count %d for eni %s\n", len(ipPool), eni) + log.Debugf("Found prefix pool count %d for eni %s\n", len(prefixPool), eni) - if len(ipPool) != len(attachedENIIPs) { - log.Warnf("Instance metadata does not match data store! ipPool: %v, metadata: %v", ipPool, attachedENIIPs) + if !cniutils.PrefixSimilar(prefixPool, attachedENIIPs) { + log.Warnf("Instance metadata does not match data store! ipPool: %v, metadata: %v", prefixPool, attachedENIIPs) log.Debugf("We need to check the ENI status by calling the EC2 control plane.") // Call EC2 to verify IPs on this ENI ec2Addresses, err := c.awsClient.GetIPv4PrefixesFromEC2(eni) @@ -1515,7 +1516,7 @@ func (c *IPAMContext) eniPrefixPoolReconcile(ipPool []string, attachedENI awsuti seenIPs := c.verifyAndAddPrefixesToDatastore(eni, attachedENIIPs, needEC2Reconcile) // Sweep phase, delete remaining Prefixes since they should not remain in the datastore - for _, existingIP := range ipPool { + for _, existingIP := range prefixPool { if seenIPs[existingIP] { continue } diff --git a/pkg/utils/cniutils/cni_utils.go b/pkg/utils/cniutils/cni_utils.go index 11337dbceb..4260e3bd04 100644 --- a/pkg/utils/cniutils/cni_utils.go +++ b/pkg/utils/cniutils/cni_utils.go @@ -13,6 +13,7 @@ import ( "github.com/aws/amazon-vpc-cni-k8s/pkg/netlinkwrapper" "github.com/aws/amazon-vpc-cni-k8s/pkg/procsyswrapper" "github.com/aws/amazon-vpc-cni-k8s/utils/imds" + "github.com/aws/aws-sdk-go/service/ec2" ) const ( @@ -145,3 +146,50 @@ func IsIptableTargetNotExist(err error) bool { } return e.IsNotExist() } + +// PrefixSimilar checks if prefix pool and eni prefix are equivalent. +func PrefixSimilar(prefixPool []string, eniPrefixes []*ec2.Ipv4PrefixSpecification) bool { + if len(prefixPool) != len(eniPrefixes) { + return false + } + + prefixPoolSet := make(map[string]struct{}, len(prefixPool)) + for _, ip := range prefixPool { + prefixPoolSet[ip] = struct{}{} + } + + for _, prefix := range eniPrefixes { + if prefix == nil || prefix.Ipv4Prefix == nil { + return false + } + if _, exists := prefixPoolSet[*prefix.Ipv4Prefix]; !exists { + return false + } + } + return true +} + +// IPsSimilar checks if ipPool and eniIPs are equivalent. +func IPsSimilar(ipPool []string, eniIPs []*ec2.NetworkInterfacePrivateIpAddress) bool { + if len(ipPool) != len(eniIPs) { + return false + } + + ipPoolSet := make(map[string]struct{}, len(ipPool)) + for _, ip := range ipPool { + ipPoolSet[ip] = struct{}{} + } + + for _, ip := range eniIPs { + if ip == nil || ip.PrivateIpAddress == nil || ip.Primary == nil { + return false + } + if *ip.Primary { + continue + } + if _, exists := ipPoolSet[*ip.PrivateIpAddress]; !exists { + return false + } + } + return true +} From 429b96583574c223287868e4d6403328ecaf899e Mon Sep 17 00:00:00 2001 From: Yash Thakkar Date: Tue, 17 Sep 2024 17:56:55 -0700 Subject: [PATCH 2/3] adding ut for functions --- pkg/utils/cniutils/cni_utils.go | 11 ++- pkg/utils/cniutils/cni_utils_test.go | 131 +++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 5 deletions(-) diff --git a/pkg/utils/cniutils/cni_utils.go b/pkg/utils/cniutils/cni_utils.go index 4260e3bd04..5b44bb451f 100644 --- a/pkg/utils/cniutils/cni_utils.go +++ b/pkg/utils/cniutils/cni_utils.go @@ -152,12 +152,12 @@ func PrefixSimilar(prefixPool []string, eniPrefixes []*ec2.Ipv4PrefixSpecificati if len(prefixPool) != len(eniPrefixes) { return false } - + prefixPoolSet := make(map[string]struct{}, len(prefixPool)) for _, ip := range prefixPool { prefixPoolSet[ip] = struct{}{} } - + for _, prefix := range eniPrefixes { if prefix == nil || prefix.Ipv4Prefix == nil { return false @@ -171,15 +171,16 @@ func PrefixSimilar(prefixPool []string, eniPrefixes []*ec2.Ipv4PrefixSpecificati // IPsSimilar checks if ipPool and eniIPs are equivalent. func IPsSimilar(ipPool []string, eniIPs []*ec2.NetworkInterfacePrivateIpAddress) bool { - if len(ipPool) != len(eniIPs) { + // Here we do +1 in ipPool because eniIPs will also have primary IP which is not used by pods. + if len(ipPool) +1 != len(eniIPs) { return false } - + ipPoolSet := make(map[string]struct{}, len(ipPool)) for _, ip := range ipPool { ipPoolSet[ip] = struct{}{} } - + for _, ip := range eniIPs { if ip == nil || ip.PrivateIpAddress == nil || ip.Primary == nil { return false diff --git a/pkg/utils/cniutils/cni_utils_test.go b/pkg/utils/cniutils/cni_utils_test.go index 4b0e81ac03..46e063ac42 100644 --- a/pkg/utils/cniutils/cni_utils_test.go +++ b/pkg/utils/cniutils/cni_utils_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" current "github.com/containernetworking/cni/pkg/types/100" "github.com/stretchr/testify/assert" ) @@ -208,3 +209,133 @@ func Test_FindIPConfigsByIfaceIndex(t *testing.T) { }) } } + +func TestPrefixSimilar(t *testing.T) { + tests := []struct { + name string + prefixPool []string + eniPrefixes []*ec2.Ipv4PrefixSpecification + want bool + }{ + { + name: "Empty slices", + prefixPool: []string{}, + eniPrefixes: []*ec2.Ipv4PrefixSpecification{}, + want: true, + }, + { + name: "Different lengths", + prefixPool: []string{"192.168.1.0/24"}, + eniPrefixes: []*ec2.Ipv4PrefixSpecification{}, + want: false, + }, + { + name: "Equivalent prefixes", + prefixPool: []string{"192.168.1.0/24", "10.0.0.0/16"}, + eniPrefixes: []*ec2.Ipv4PrefixSpecification{ + {Ipv4Prefix: stringPtr("192.168.1.0/24")}, + {Ipv4Prefix: stringPtr("10.0.0.0/16")}, + }, + want: true, + }, + { + name: "Different prefixes", + prefixPool: []string{"192.168.1.0/24", "10.0.0.0/16"}, + eniPrefixes: []*ec2.Ipv4PrefixSpecification{ + {Ipv4Prefix: stringPtr("192.168.1.0/24")}, + {Ipv4Prefix: stringPtr("172.16.0.0/16")}, + }, + want: false, + }, + { + name: "Nil prefix", + prefixPool: []string{"192.168.1.0/24"}, + eniPrefixes: []*ec2.Ipv4PrefixSpecification{ + nil, + }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := PrefixSimilar(tt.prefixPool, tt.eniPrefixes); got != tt.want { + t.Errorf("in test %s PrefixSimilar() = %v, want %v", tt.name, got, tt.want) + } + }) + } +} + +func TestIPsSimilar(t *testing.T) { + tests := []struct { + name string + ipPool []string + eniIPs []*ec2.NetworkInterfacePrivateIpAddress + want bool + }{ + { + name: "Empty IP pool", + ipPool: []string{}, + eniIPs: []*ec2.NetworkInterfacePrivateIpAddress{ + {PrivateIpAddress: stringPtr("10.0.0.1"), Primary: boolPtr(true)}, + }, + want: true, + }, + { + name: "Different lengths", + ipPool: []string{"192.168.1.1"}, + eniIPs: []*ec2.NetworkInterfacePrivateIpAddress{ + {PrivateIpAddress: stringPtr("10.0.0.1"), Primary: boolPtr(true)}, + {PrivateIpAddress: stringPtr("192.168.1.1"), Primary: boolPtr(false)}, + {PrivateIpAddress: stringPtr("192.168.1.2"), Primary: boolPtr(false)}, + }, + want: false, + }, + { + name: "Equivalent IPs", + ipPool: []string{"192.168.1.1", "10.0.0.2"}, + eniIPs: []*ec2.NetworkInterfacePrivateIpAddress{ + {PrivateIpAddress: stringPtr("10.0.0.1"), Primary: boolPtr(true)}, + {PrivateIpAddress: stringPtr("192.168.1.1"), Primary: boolPtr(false)}, + {PrivateIpAddress: stringPtr("10.0.0.2"), Primary: boolPtr(false)}, + }, + want: true, + }, + { + name: "Different IPs", + ipPool: []string{"192.168.1.1", "10.0.0.2"}, + eniIPs: []*ec2.NetworkInterfacePrivateIpAddress{ + {PrivateIpAddress: stringPtr("10.0.0.1"), Primary: boolPtr(true)}, + {PrivateIpAddress: stringPtr("192.168.1.1"), Primary: boolPtr(false)}, + {PrivateIpAddress: stringPtr("172.16.0.1"), Primary: boolPtr(false)}, + }, + want: false, + }, + { + name: "Nil IP", + ipPool: []string{"192.168.1.1"}, + eniIPs: []*ec2.NetworkInterfacePrivateIpAddress{ + {PrivateIpAddress: stringPtr("10.0.0.1"), Primary: boolPtr(true)}, + nil, + }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IPsSimilar(tt.ipPool, tt.eniIPs); got != tt.want { + t.Errorf("in test %s IPsSimilar() = %v, want %v", tt.name, got, tt.want) + } + }) + } +} + +// Helper functions for creating pointers +func stringPtr(s string) *string { + return &s +} + +func boolPtr(b bool) *bool { + return &b +} From 11c2c98ac48f82e103e26b23875bce52e5fced2a Mon Sep 17 00:00:00 2001 From: Yash Thakkar Date: Wed, 18 Sep 2024 10:28:24 -0700 Subject: [PATCH 3/3] go fmt --- pkg/ipamd/ipamd.go | 2 +- pkg/utils/cniutils/cni_utils.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/ipamd/ipamd.go b/pkg/ipamd/ipamd.go index deedcc4fff..588bc3870a 100644 --- a/pkg/ipamd/ipamd.go +++ b/pkg/ipamd/ipamd.go @@ -1457,7 +1457,7 @@ func (c *IPAMContext) eniIPPoolReconcile(ipPool []string, attachedENI awsutils.E needEC2Reconcile := true // Here we can't trust attachedENI since the IMDS metadata can be stale. We need to check with EC2 API. // IPsSimilar will exclude primary IP of the ENI that is not added to the ipPool and not available for pods to use. - if !cniutils.IPsSimilar(ipPool,attachedENIIPs) { + if !cniutils.IPsSimilar(ipPool, attachedENIIPs) { log.Warnf("Instance metadata does not match data store! ipPool: %v, metadata: %v", ipPool, attachedENIIPs) log.Debugf("We need to check the ENI status by calling the EC2 control plane.") // Call EC2 to verify IPs on this ENI diff --git a/pkg/utils/cniutils/cni_utils.go b/pkg/utils/cniutils/cni_utils.go index 5b44bb451f..bf5520d12a 100644 --- a/pkg/utils/cniutils/cni_utils.go +++ b/pkg/utils/cniutils/cni_utils.go @@ -172,7 +172,7 @@ func PrefixSimilar(prefixPool []string, eniPrefixes []*ec2.Ipv4PrefixSpecificati // IPsSimilar checks if ipPool and eniIPs are equivalent. func IPsSimilar(ipPool []string, eniIPs []*ec2.NetworkInterfacePrivateIpAddress) bool { // Here we do +1 in ipPool because eniIPs will also have primary IP which is not used by pods. - if len(ipPool) +1 != len(eniIPs) { + if len(ipPool)+1 != len(eniIPs) { return false }