Fix fetching enimetadata #3035
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -585,6 +585,12 @@ func (cache *EC2InstanceMetadataCache) getENIMetadata(eniMAC string) (ENIMetadat | |
| return ENIMetadata{}, err | ||
| } | ||
|
|
||
| networkCard, err := cache.imds.GetNetworkCard(ctx, eniMAC) | ||
| if err != nil { | ||
| awsAPIErrInc("GetNetworkCard", err) | ||
| return ENIMetadata{}, err | ||
| } | ||
|
|
||
| deviceNum, err = cache.imds.GetDeviceNumber(ctx, eniMAC) | ||
| if err != nil { | ||
| awsAPIErrInc("GetDeviceNumber", err) | ||
|
|
@@ -602,90 +608,99 @@ func (cache *EC2InstanceMetadataCache) getENIMetadata(eniMAC string) (ENIMetadat | |
| deviceNum = 0 | ||
| } | ||
|
|
||
| log.Debugf("Found ENI: %s, MAC %s, device %d, network card %d", eniID, eniMAC, deviceNum, networkCard) | ||
|
|
||
| var subnetV4Cidr string | ||
| var ec2ip4s []*ec2.NetworkInterfacePrivateIpAddress | ||
| var ec2ip6s []*ec2.NetworkInterfaceIpv6Address | ||
| var subnetV6Cidr string | ||
| var ec2ipv4Prefixes []*ec2.Ipv4PrefixSpecification | ||
| var ec2ipv6Prefixes []*ec2.Ipv6PrefixSpecification | ||
|
|
||
| // CNI only manages ENI's on network card 0. We need to get complete metadata info only for ENI's on network card 0. | ||
| // For ENI's on other network cards, there might not be IP related info present at all like 'efa-only' interfaces | ||
| // So we are skipping fetching IP related info for all ENI's other than card 0 | ||
| if networkCard == 0 { | ||
| // Get IPv4 and IPv6 addresses assigned to interface | ||
|
There was a problem hiding this comment. Need to remove IPv6 from the comment There was a problem hiding this comment. @jayanthvn we are fetching both IPv4 and IPv6 addresses for ENI's on network card 0. Comment is same as earlier to this change. Github side by side comparison is a bit messed for this PR |
||
| 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()), | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return ENIMetadata{ | ||
| ENIID: eniID, | ||
| MAC: eniMAC, | ||
| DeviceNumber: deviceNum, | ||
| SubnetIPv4CIDR: subnetV4Cidr, | ||
| IPv4Addresses: ec2ip4s, | ||
| IPv4Prefixes: ec2ipv4Prefixes, | ||
| SubnetIPv6CIDR: subnetV6Cidr, | ||
|
|
@@ -1356,7 +1371,7 @@ func (cache *EC2InstanceMetadataCache) DescribeAllENIs() (DescribeAllENIsResult, | |
| if interfaceType == "trunk" { | ||
| trunkENI = eniID | ||
| } | ||
| if interfaceType == "efa" || interfaceType == "efa-only" { | ||
|
There was a problem hiding this comment. eventually we should move the interfaceType to some enum for trunk, efa, efa-only.. There was a problem hiding this comment. Makes sense @jayanthvn. I will add a backlog task for this |
||
| efaENIs[eniID] = true | ||
| } | ||
| // Check IPv4 addresses | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What are we verifying with a
if networkCard == 0here?Also, we need to test this change in some variety of instances - Multi Card Instance, regular instance, and ensuring this is not leading to any unexpected behavior.
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.
@orsenthil Thanks for the review
On a multicard instance this condition would be false for non-0 cards. For non-0 cards we are just getting basic eni info and not fetching any ip related info as some of the efa interfaces might not have ip info
Here https://github.com/aws/amazon-vpc-cni-k8s/blob/master/pkg/awsutils/awsutils.go#L1344 we already assume CNI always manages only network card 0 and rest are just added as unmanaged ENI's so it is safe to make the assumption that we need complete ENIMetadata only for network card 0
I have tested this on multiple instances including multi-card, single card, different efa interfaces and verified the functionality.
I will add comments in code explaining why we are checking for network card 0