Filter out interfaces with no ip info #3047
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -603,89 +603,130 @@ 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 | ||
| } | ||
| isEFAOnlyInterface := true | ||
| // 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 n | ||
| 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 { | ||
| isEFAOnlyInterface = false | ||
| 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 { | ||
|
There was a problem hiding this comment. why we treat IPv4 and IPv6 differently. 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) There was a problem hiding this comment. @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 { | ||
| isEFAOnlyInterface = false | ||
| log.Debugf("Found IPv6 addresses associated with interface. This is not efa-only interface") | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| var subnetV4Cidr string | ||
| var ec2ip4s []*ec2.NetworkInterfacePrivateIpAddress | ||
| var ec2ip6s []*ec2.NetworkInterfaceIpv6Address | ||
| var subnetV6Cidr string | ||
| var ec2ipv4Prefixes []*ec2.Ipv4PrefixSpecification | ||
| var ec2ipv6Prefixes []*ec2.Ipv6PrefixSpecification | ||
|
|
||
| // Do no fetch ip related info for Efa-only interfaces | ||
| if !isEFAOnlyInterface { | ||
| // Get IPv4 and IPv6 addresses assigned to interface | ||
| cidr, err := cache.imds.GetSubnetIPv4CIDRBlock(ctx, eniMAC) | ||
| if err != nil { | ||
| awsAPIErrInc("GetSubnetIPv4CIDRBlock", err) | ||
| return ENIMetadata{}, err | ||
| } else { | ||
| 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()), | ||
| } | ||
| } | ||
|
|
||
| if cache.v6Enabled { | ||
| // For IPv6 ENIs, do not error on missing IPv6 information | ||
| v6cidr, err := cache.imds.GetSubnetIPv6CIDRBlocks(ctx, eniMAC) | ||
| if err != nil { | ||
| awsAPIErrInc("GetSubnetIPv6CIDRBlocks", err) | ||
| } else { | ||
| subnetV6Cidr = v6cidr.String() | ||
| } | ||
|
|
||
| imdsIPv6s, err := cache.imds.GetIPv6s(ctx, eniMAC) | ||
| if err != nil { | ||
| awsAPIErrInc("GetIPv6s", err) | ||
| } else { | ||
| ec2ip6s = make([]*ec2.NetworkInterfaceIpv6Address, len(imdsIPv6s)) | ||
| for i, ip6 := range imdsIPv6s { | ||
| ec2ip6s[i] = &ec2.NetworkInterfaceIpv6Address{ | ||
| Ipv6Address: aws.String(ip6.String()), | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If IPv6 is enabled, get attached v6 prefixes. | ||
| if cache.v6Enabled { | ||
| imdsIPv6Prefixes, err := cache.imds.GetIPv6Prefixes(ctx, eniMAC) | ||
| if err != nil { | ||
| awsAPIErrInc("GetIPv6Prefixes", err) | ||
| return ENIMetadata{}, err | ||
| } | ||
| for _, ipv6prefix := range imdsIPv6Prefixes { | ||
| ec2ipv6Prefixes = append(ec2ipv6Prefixes, &ec2.Ipv6PrefixSpecification{ | ||
| Ipv6Prefix: aws.String(ipv6prefix.String()), | ||
| }) | ||
| } | ||
| } else if cache.v4Enabled && ((eniMAC == primaryMAC && !cache.useCustomNetworking) || (eniMAC != primaryMAC)) { | ||
| // Get prefix on primary ENI when custom networking is enabled is not needed. | ||
| // If primary ENI has prefixes attached and then we move to custom networking, we don't need to fetch | ||
| // the prefix since recommendation is to terminate the nodes and that would have deleted the prefix on the | ||
| // primary ENI. | ||
| imdsIPv4Prefixes, err := cache.imds.GetIPv4Prefixes(ctx, eniMAC) | ||
| if err != nil { | ||
| awsAPIErrInc("GetIPv4Prefixes", err) | ||
| return ENIMetadata{}, err | ||
| } | ||
| for _, ipv4prefix := range imdsIPv4Prefixes { | ||
| ec2ipv4Prefixes = append(ec2ipv4Prefixes, &ec2.Ipv4PrefixSpecification{ | ||
| Ipv4Prefix: aws.String(ipv4prefix.String()), | ||
| }) | ||
| } | ||
| } | ||
| } else { | ||
| log.Debugf("No ipv4 or ipv6 info associated with this interface. This might be efa-only interface") | ||
| } | ||
|
|
||
| return ENIMetadata{ | ||
| ENIID: eniID, | ||
| MAC: eniMAC, | ||
| DeviceNumber: deviceNum, | ||
| SubnetIPv4CIDR: subnetV4Cidr, | ||
| IPv4Addresses: ec2ip4s, | ||
| IPv4Prefixes: ec2ipv4Prefixes, | ||
| SubnetIPv6CIDR: subnetV6Cidr, | ||
|
|
@@ -1356,9 +1397,16 @@ func (cache *EC2InstanceMetadataCache) DescribeAllENIs() (DescribeAllENIsResult, | |
| if interfaceType == "trunk" { | ||
| trunkENI = eniID | ||
| } | ||
| if interfaceType == "efa" || interfaceType == "efa-only" { | ||
| efaENIs[eniID] = true | ||
| } | ||
| if interfaceType != "efa-only" { | ||
| if len(eniMetadata.IPv4Addresses) == 0 && len(eniMetadata.IPv6Addresses) == 0 { | ||
|
There was a problem hiding this comment. Should this condition be a 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. There was a problem hiding this comment. There are 2 cases,
There was a problem hiding this comment. Updated. We will not be returning error if we miss ipv6addresses to maintain the behavior There was a problem hiding this comment. 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) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we flip the logic here?
And if it true, then immediately return with appropriate empty fields
Rest look good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we can do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated