Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues with handling unmanaged ENIs with IPv6 only #3122

Merged
merged 1 commit into from
Dec 3, 2024
Merged

Fix issues with handling unmanaged ENIs with IPv6 only #3122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 34 additions & 23 deletions pkg/awsutils/awsutils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,9 @@ func (cache *EC2InstanceMetadataCache) getENIMetadata(eniMAC string) (ENIMetadat
awsAPIErrInc("GetMACImdsFields", err)
return ENIMetadata{}, err
}
ipInfoAvailable := false

ipv4Available := false
ipv6Available := false
// Efa-only interfaces do not have any ipv4s or ipv6s associated with it. If we don't find any local-ipv4 or ipv6 info in imds we assume it to be efa-only interface and validate this later via ec2 call
for _, field := range macImdsFields {
if field == "local-ipv4s" {
Expand All @@ -620,7 +622,7 @@ func (cache *EC2InstanceMetadataCache) getENIMetadata(eniMAC string) (ENIMetadat
return ENIMetadata{}, err
}
if len(imdsIPv4s) > 0 {
ipInfoAvailable = true
ipv4Available = true
log.Debugf("Found IPv4 addresses associated with interface. This is not efa-only interface")
break
}
Expand All @@ -630,14 +632,14 @@ func (cache *EC2InstanceMetadataCache) getENIMetadata(eniMAC string) (ENIMetadat
if err != nil {
awsAPIErrInc("GetIPv6s", err)
} else if len(imdsIPv6s) > 0 {
ipInfoAvailable = true
ipv6Available = true
log.Debugf("Found IPv6 addresses associated with interface. This is not efa-only interface")
break
}
}
}

if !ipInfoAvailable {
if !ipv4Available && !ipv6Available {
return ENIMetadata{
ENIID: eniID,
MAC: eniMAC,
Expand All @@ -652,23 +654,29 @@ func (cache *EC2InstanceMetadataCache) getENIMetadata(eniMAC string) (ENIMetadat
}

// Get IPv4 and IPv6 addresses assigned to interface
cidr, err := cache.imds.GetSubnetIPv4CIDRBlock(ctx, eniMAC)
if err != nil {
awsAPIErrInc("GetSubnetIPv4CIDRBlock", err)
return ENIMetadata{}, err
}
var ec2ip4s []*ec2.NetworkInterfacePrivateIpAddress
var subnetV4Cidr string
if ipv4Available {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem I realized much later is, the way in which we check for ipv4Available and ipv6Available - seems to depend on the order in which imdsFields are returned.

	for _, field := range macImdsFields {
		if field == "local-ipv4s" {
			imdsIPv4s, err := cache.imds.GetLocalIPv4s(ctx, eniMAC)
			if err != nil {
				awsAPIErrInc("GetLocalIPv4s", err)
				return ENIMetadata{}, err
			}
			if len(imdsIPv4s) > 0 {
				ipv4Available = true
				log.Debugf("Found IPv4 addresses associated with interface. This is not efa-only interface")
				break
			}
		}
		if field == "ipv6s" {
			imdsIPv6s, err := cache.imds.GetIPv6s(ctx, eniMAC)
			if err != nil {
				awsAPIErrInc("GetIPv6s", err)
			} else if len(imdsIPv6s) > 0 {
				ipv6Available = true
				log.Debugf("Found IPv6 addresses associated with interface. This is not efa-only interface")
				break
			}
		}
	}

Previously we assumed the ENI (or primary) always will have an IPV4 and went with that approach. Now, since we are checking ipv4Available explicitly, if for some reason order of checking of imds returned IPV6 and not ipv4 (due to break condition in the above code. then this introduces bug.

We will need to remove the break condition in the above loop to remain compatible with the previous behavior, that is, to always get the IPV4 address for the primary ENI.

cidr, err := cache.imds.GetSubnetIPv4CIDRBlock(ctx, eniMAC)
if err != nil {
awsAPIErrInc("GetSubnetIPv4CIDRBlock", err)
return ENIMetadata{}, err
}

imdsIPv4s, err := cache.imds.GetLocalIPv4s(ctx, eniMAC)
if err != nil {
awsAPIErrInc("GetLocalIPv4s", err)
return ENIMetadata{}, err
}
subnetV4Cidr = cidr.String()

imdsIPv4s, err := cache.imds.GetLocalIPv4s(ctx, eniMAC)
if err != nil {
awsAPIErrInc("GetLocalIPv4s", err)
return ENIMetadata{}, err
}

ec2ip4s := make([]*ec2.NetworkInterfacePrivateIpAddress, len(imdsIPv4s))
for i, ip4 := range imdsIPv4s {
ec2ip4s[i] = &ec2.NetworkInterfacePrivateIpAddress{
Primary: aws.Bool(i == 0),
PrivateIpAddress: aws.String(ip4.String()),
ec2ip4s = make([]*ec2.NetworkInterfacePrivateIpAddress, len(imdsIPv4s))
for i, ip4 := range imdsIPv4s {
ec2ip4s[i] = &ec2.NetworkInterfacePrivateIpAddress{
Primary: aws.Bool(i == 0),
PrivateIpAddress: aws.String(ip4.String()),
}
}
}

Expand Down Expand Up @@ -732,7 +740,7 @@ func (cache *EC2InstanceMetadataCache) getENIMetadata(eniMAC string) (ENIMetadat
ENIID: eniID,
MAC: eniMAC,
DeviceNumber: deviceNum,
SubnetIPv4CIDR: cidr.String(),
SubnetIPv4CIDR: subnetV4Cidr,
IPv4Addresses: ec2ip4s,
IPv4Prefixes: ec2ipv4Prefixes,
SubnetIPv6CIDR: subnetV6Cidr,
Expand Down Expand Up @@ -1407,14 +1415,17 @@ func (cache *EC2InstanceMetadataCache) DescribeAllENIs() (DescribeAllENIsResult,
efaENIs[eniID] = true
}
if interfaceType != "efa-only" {
if len(eniMetadata.IPv4Addresses) == 0 {
if len(eniMetadata.IPv4Addresses) == 0 && len(eniMetadata.IPv6Addresses) == 0 {
log.Errorf("Missing IP addresses from IMDS. Non efa-only interface should have IP address associated with it %s", eniID)
outOfSyncErr := errors.New("DescribeAllENIs: No IPv4 address found")
outOfSyncErr := errors.New("DescribeAllENIs: No IPv4 and IPv6 addresses found")
return DescribeAllENIsResult{}, outOfSyncErr
}
}

// Check IPv4 addresses
logOutOfSyncState(eniID, eniMetadata.IPv4Addresses, ec2res.PrivateIpAddresses)
if len(eniMetadata.IPv4Addresses) > 0 {
logOutOfSyncState(eniID, eniMetadata.IPv4Addresses, ec2res.PrivateIpAddresses)
}
tagMap[eniMetadata.ENIID] = convertSDKTagsToTags(ec2res.TagSet)
}
return DescribeAllENIsResult{
Expand Down
20 changes: 20 additions & 0 deletions pkg/awsutils/awsutils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ const (
metadataSubnetCIDR = "/subnet-ipv4-cidr-block"
metadataIPv4s = "/local-ipv4s"
metadataIPv4Prefixes = "/ipv4-prefix"
metadataIPv6s = "/ipv6s"
metadataIPv6Prefixes = "/ipv6-prefix"

az = "us-east-1a"
Expand All @@ -79,12 +80,14 @@ const (
eni2Device = "1"
eni2PrivateIP = "10.0.0.2"
eni2Prefix = "10.0.2.0/28"
eni2v6IP = "2001:db8:8:4::2"
eni2v6Prefix = "2001:db8::/64"
eni2ID = "eni-12341234"
metadataVPCIPv4CIDRs = "192.168.0.0/16 100.66.0.0/1"
myNodeName = "testNodeName"
imdsMACFields = "security-group-ids subnet-id vpc-id vpc-ipv4-cidr-blocks device-number interface-id subnet-ipv4-cidr-block local-ipv4s ipv4-prefix ipv6-prefix"
imdsMACFieldsEfaOnly = "security-group-ids subnet-id vpc-id vpc-ipv4-cidr-blocks device-number interface-id subnet-ipv4-cidr-block ipv4-prefix ipv6-prefix"
imdsMACFieldsV6Only = "security-group-ids subnet-id vpc-id vpc-ipv4-cidr-blocks device-number interface-id subnet-ipv6-cidr-blocks ipv6s ipv6-prefix"
)

func testMetadata(overrides map[string]interface{}) FakeIMDS {
Expand Down Expand Up @@ -241,6 +244,23 @@ func TestGetAttachedENIsWithEfaOnly(t *testing.T) {
}
}

func TestGetAttachedENIsWithIPv6Only(t *testing.T) {
mockMetadata := testMetadata(map[string]interface{}{
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC: imdsMACFieldsV6Only,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataIPv6s: eni2v6IP,
metadataMACPath + eni2MAC + metadataIPv6Prefixes: eni2v6Prefix,
})

cache := &EC2InstanceMetadataCache{imds: TypedIMDS{mockMetadata}}
ens, err := cache.GetAttachedENIs()
if assert.NoError(t, err) {
assert.Equal(t, len(ens), 2)
}
}

func TestGetAttachedENIsWithPrefixes(t *testing.T) {
mockMetadata := testMetadata(map[string]interface{}{
metadataMACPath: primaryMAC + " " + eni2MAC,
Expand Down
Loading