Skip to content

Commit

Permalink
Bug fix: Ensure exact IP match between IMDS and local datastore. (#3033)
Browse files Browse the repository at this point in the history 
* adding function to compare list

* adding ut for functions

* go fmt
  • Loading branch information
@yash97
yash97 authored Sep 18, 2024
1 parent 27ce136 commit 5c471fe
Show file tree
Hide file tree
Showing 3 changed files with 188 additions and 7 deletions.
15 changes: 8 additions & 7 deletions pkg/ipamd/ipamd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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
}
Expand Down
49 changes: 49 additions & 0 deletions pkg/utils/cniutils/cni_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -145,3 +146,51 @@ 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 {
// 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
}
if *ip.Primary {
continue
}
if _, exists := ipPoolSet[*ip.PrivateIpAddress]; !exists {
return false
}
}
return true
}
131 changes: 131 additions & 0 deletions pkg/utils/cniutils/cni_utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
}

0 comments on commit 5c471fe

Please sign in to comment.