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

Jump to bottom

Filter out interfaces with no ip info #3047

Merged
merged 4 commits into from
Sep 27, 2024
+115 −18
Merged

Filter out interfaces with no ip info #3047

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4518356
Filter for interfaces with no ip info
Pavani-Panakanti Sep 26, 2024
673143c
Merge branch 'master' into efa-only-f
Pavani-Panakanti Sep 26, 2024
36a8991
code refactor
Pavani-Panakanti Sep 27, 2024
c386ba9
Donot return for missing ipv6
Pavani-Panakanti Sep 27, 2024
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
56 changes: 55 additions & 1 deletion pkg/awsutils/awsutils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,53 @@ func (cache *EC2InstanceMetadataCache) getENIMetadata(eniMAC string) (ENIMetadat

log.Debugf("Found ENI: %s, MAC %s, device %d", eniID, eniMAC, deviceNum)

// Get IMDS fields for the interface
macImdsFields, err := cache.imds.GetMACImdsFields(ctx, eniMAC)
if err != nil {
awsAPIErrInc("GetMACImdsFields", err)
return ENIMetadata{}, err
}
ipInfoAvailable := 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" {
imdsIPv4s, err := cache.imds.GetLocalIPv4s(ctx, eniMAC)
if err != nil {
awsAPIErrInc("GetLocalIPv4s", err)
return ENIMetadata{}, err
}
if len(imdsIPv4s) > 0 {
ipInfoAvailable = 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

why we treat IPv4 and IPv6 differently.
If GetLocalIPv4s failed, we return error directly. If GetIPv6s failed, we didn't return error.

If there is no particular reason, we should refactor the logic & code to make it have similar structure/logic for ipv4&ipv6. (otherwise such difference will became a maintaince burden and could easily cause bugs when changes are made here)

Copy link
Contributor Author

@Pavani-Panakanti Pavani-Panakanti Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@M00nF1sh We are not erroring out at missing ipv6 info in other parts of the code https://github.com/aws/amazon-vpc-cni-k8s/blob/master/pkg/awsutils/awsutils.go#L631. Did the same here to maintain consistency to same as earlier irrespective of this change.

we should add some context and comments around why it is different for ipv6

awsAPIErrInc("GetIPv6s", err)
} else if len(imdsIPv6s) > 0 {
ipInfoAvailable = true
log.Debugf("Found IPv6 addresses associated with interface. This is not efa-only interface")
break
}
}
}

if !ipInfoAvailable {
return ENIMetadata{
ENIID: eniID,
MAC: eniMAC,
DeviceNumber: deviceNum,
SubnetIPv4CIDR: "",
IPv4Addresses: make([]*ec2.NetworkInterfacePrivateIpAddress, 0),
IPv4Prefixes: make([]*ec2.Ipv4PrefixSpecification, 0),
SubnetIPv6CIDR: "",
IPv6Addresses: make([]*ec2.NetworkInterfaceIpv6Address, 0),
IPv6Prefixes: make([]*ec2.Ipv6PrefixSpecification, 0),
}, nil
}

// Get IPv4 and IPv6 addresses assigned to interface
cidr, err := cache.imds.GetSubnetIPv4CIDRBlock(ctx, eniMAC)
if err != nil {
Expand Down Expand Up @@ -1356,9 +1403,16 @@ func (cache *EC2InstanceMetadataCache) DescribeAllENIs() (DescribeAllENIsResult,
if interfaceType == "trunk" {
trunkENI = eniID
}
if interfaceType == "efa" {
if interfaceType == "efa" || interfaceType == "efa-only" {
efaENIs[eniID] = true
}
if interfaceType != "efa-only" {
if len(eniMetadata.IPv4Addresses) == 0 && len(eniMetadata.IPv6Addresses) == 0 {
Copy link
Member

@orsenthil orsenthil Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should this condition be a && or an || ? Update: && is correct here.

This is the only new Error we are introducing here. Do we strictly require this ? cc: @jayanthvn (this was introduced from the review comment).

Rest of the change looks good to me with capturing EFA only instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are 2 cases, local-ipv4 field is missing or local-ipv4 value is empty

local-ipv4 value is empty -> we return error in 618

local-ipv4 field is missing (Eg: IMDS out of sync on new node or aws-node restart) -> previously we used to error out - https://github.com/aws/amazon-vpc-cni-k8s/blob/master/pkg/awsutils/imds.go#L293-L299 but now we return the ENI (line 640) and call describe ENI. So to keep the behavior consistent this additional check i.e, if not efa-only and missing primary IP we error out... But I think we don't need to check IPv6Addresses since we have been ignoring ipv6 missing data (need to check why that was introduced though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. We will not be returning error if we miss ipv6addresses to maintain the behavior

Copy link
Member

Choose a reason for hiding this comment

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

Okay, this reasoning sounds good to me.

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 IP addresses found")
return DescribeAllENIsResult{}, outOfSyncErr
}
}
// Check IPv4 addresses
logOutOfSyncState(eniID, eniMetadata.IPv4Addresses, ec2res.PrivateIpAddresses)
tagMap[eniMetadata.ENIID] = convertSDKTagsToTags(ec2res.TagSet)
Expand Down
59 changes: 42 additions & 17 deletions pkg/awsutils/awsutils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,16 +80,19 @@ const (
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"
)

func testMetadata(overrides map[string]interface{}) FakeIMDS {
data := map[string]interface{}{
metadataAZ: az,
metadataLocalIP: localIP,
metadataInstanceID: instanceID,
metadataInstanceType: instanceType,
metadataMAC: primaryMAC,
metadataMACPath: primaryMAC,
metadataAZ: az,
metadataLocalIP: localIP,
metadataInstanceID: instanceID,
metadataInstanceType: instanceType,
metadataMAC: primaryMAC,
metadataMACPath: primaryMAC,
metadataMACPath + primaryMAC: imdsMACFields,
metadataMACPath + primaryMAC + metadataDeviceNum: eni1Device,
metadataMACPath + primaryMAC + metadataInterface: primaryeniID,
metadataMACPath + primaryMAC + metadataSGs: sgs,
Expand All @@ -109,12 +112,13 @@ func testMetadata(overrides map[string]interface{}) FakeIMDS {

func testMetadataWithPrefixes(overrides map[string]interface{}) FakeIMDS {
data := map[string]interface{}{
metadataAZ: az,
metadataLocalIP: localIP,
metadataInstanceID: instanceID,
metadataInstanceType: instanceType,
metadataMAC: primaryMAC,
metadataMACPath: primaryMAC,
metadataAZ: az,
metadataLocalIP: localIP,
metadataInstanceID: instanceID,
metadataInstanceType: instanceType,
metadataMAC: primaryMAC,
metadataMACPath: primaryMAC,
metadataMACPath + primaryMAC: imdsMACFields,
metadataMACPath + primaryMAC + metadataDeviceNum: eni1Device,
metadataMACPath + primaryMAC + metadataInterface: primaryeniID,
metadataMACPath + primaryMAC + metadataSGs: sgs,
Expand Down Expand Up @@ -203,7 +207,8 @@ func TestInitWithEC2metadataErr(t *testing.T) {

func TestGetAttachedENIs(t *testing.T) {
mockMetadata := testMetadata(map[string]interface{}{
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC: imdsMACFields,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataSubnetCIDR: subnetCIDR,
Expand All @@ -217,9 +222,26 @@ func TestGetAttachedENIs(t *testing.T) {
}
}

func TestGetAttachedENIsWithEfaOnly(t *testing.T) {
mockMetadata := testMetadata(map[string]interface{}{
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC: imdsMACFieldsEfaOnly,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataSubnetCIDR: subnetCIDR,
})

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,
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC: imdsMACFields,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataSubnetCIDR: subnetCIDR,
Expand Down Expand Up @@ -343,6 +365,7 @@ func TestDescribeAllENIs(t *testing.T) {
Attachment: &ec2.NetworkInterfaceAttachment{
NetworkCardIndex: aws.Int64(0),
},
NetworkInterfaceId: aws.String(primaryeniID),
}},
}

Expand All @@ -357,7 +380,7 @@ func TestDescribeAllENIs(t *testing.T) {
awsErr error
expErr error
}{
{"Success DescribeENI", map[string]TagMap{"": {"foo": "foo-value"}}, 1, nil, nil},
{"Success DescribeENI", map[string]TagMap{"eni-00000000": {"foo": "foo-value"}}, 1, nil, nil},
{"Not found error", nil, maxENIEC2APIRetries, awserr.New("InvalidNetworkInterfaceID.NotFound", "no 'eni-xxx'", nil), expectedError},
{"Not found, no message", nil, maxENIEC2APIRetries, awserr.New("InvalidNetworkInterfaceID.NotFound", "no message", nil), noMessageError},
{"Other error", nil, maxENIEC2APIRetries, err, err},
Expand Down Expand Up @@ -1006,7 +1029,8 @@ func TestEC2InstanceMetadataCache_waitForENIAndIPsAttached(t *testing.T) {
}
fmt.Println("eniips", eniIPs)
mockMetadata := testMetadata(map[string]interface{}{
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC: imdsMACFields,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataSubnetCIDR: subnetCIDR,
Expand Down Expand Up @@ -1101,7 +1125,8 @@ func TestEC2InstanceMetadataCache_waitForENIAndPrefixesAttached(t *testing.T) {
eniPrefixes = ""
}
mockMetadata := testMetadata(map[string]interface{}{
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC: imdsMACFields,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataSubnetCIDR: subnetCIDR,
Expand Down
18 changes: 18 additions & 0 deletions pkg/awsutils/imds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,24 @@ func (imds TypedIMDS) GetMACs(ctx context.Context) ([]string, error) {
return list, err
}

// GetMACImdsFields returns the imds fields present for a MAC
func (imds TypedIMDS) GetMACImdsFields(ctx context.Context, mac string) ([]string, error) {
key := fmt.Sprintf("network/interfaces/macs/%s", mac)
list, err := imds.getList(ctx, key)
if err != nil {
if imdsErr, ok := err.(*imdsRequestError); ok {
log.Warnf("%v", err)
return nil, imdsErr.err
}
return nil, err
}
// Remove trailing /
for i, item := range list {
list[i] = strings.TrimSuffix(item, "/")
}
return list, err
}

// GetInterfaceID returns the ID of the network interface.
func (imds TypedIMDS) GetInterfaceID(ctx context.Context, mac string) (string, error) {
key := fmt.Sprintf("network/interfaces/macs/%s/interface-id", mac)
Expand Down
Loading