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

Update enimetadata for interfaces with no ip info #3041

Closed
wants to merge 4 commits into from
Closed

Update enimetadata for interfaces with no ip info #3041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7687d01
fix enimetadata for interfaces with no ip
Pavani-Panakanti Sep 26, 2024
664f81e
update with ipv6s
Pavani-Panakanti Sep 26, 2024
d99a85d
Update comment
Pavani-Panakanti Sep 26, 2024
71215dc
update checks with ec2 ips
Pavani-Panakanti Sep 26, 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
57 changes: 46 additions & 11 deletions pkg/awsutils/awsutils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,12 +585,6 @@ 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)
Expand All @@ -608,7 +602,39 @@ 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)
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
Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is if the ENI is truly missing local-ipv4

Copy link
Member

Choose a reason for hiding this comment

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

field == "local-ipv4s"

I agree. If these kind of 'structural' definitions is supported by any documentation or spec, that will be best to rely upon.

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 {
isEFAOnlyInterface = false
log.Debugf("Found IPv6 addresses associated with interface. This is not efa-only interface")
break
}
}
}

var subnetV4Cidr string
var ec2ip4s []*ec2.NetworkInterfacePrivateIpAddress
Expand All @@ -617,10 +643,8 @@ func (cache *EC2InstanceMetadataCache) getENIMetadata(eniMAC string) (ENIMetadat
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 {
// 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 {
Expand Down Expand Up @@ -694,6 +718,8 @@ func (cache *EC2InstanceMetadataCache) getENIMetadata(eniMAC string) (ENIMetadat
})
}
}
} else {
log.Debugf("No ipv4 or ipv6 info associated with this interface. This might be efa-only interface")
}

return ENIMetadata{
Expand Down Expand Up @@ -1374,6 +1400,15 @@ func (cache *EC2InstanceMetadataCache) DescribeAllENIs() (DescribeAllENIsResult,
if interfaceType == "efa" || interfaceType == "efa-only" {
efaENIs[eniID] = true
}
if interfaceType != "efa-only" {
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 IP addresses found")
return DescribeAllENIsResult{}, outOfSyncErr
} else {
log.Infof("Found IP addresses associated with interface %s : %s", eniID, interfaceType)
}
}
// Check IPv4 addresses
logOutOfSyncState(eniID, eniMetadata.IPv4Addresses, ec2res.PrivateIpAddresses)
tagMap[eniMetadata.ENIID] = convertSDKTagsToTags(ec2res.TagSet)
Expand Down
112 changes: 53 additions & 59 deletions pkg/awsutils/awsutils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ const (
metadataSubnetID = "/subnet-id"
metadataVpcID = "/vpc-id"
metadataVPCcidrs = "/vpc-ipv4-cidr-blocks"
metadataNetworkCard = "/network-card"
metadataDeviceNum = "/device-number"
metadataInterface = "/interface-id"
metadataSubnetCIDR = "/subnet-ipv4-cidr-block"
Expand All @@ -72,40 +71,37 @@ const (
primaryeniID = "eni-00000000"
eniID = primaryeniID
eniAttachID = "eni-attach-beb21856"
eni1NetworkCard = "0"
eni1Device = "0"
eni1PrivateIP = "10.0.0.1"
eni1Prefix = "10.0.1.0/28"
eni2NetworkCard = "0"
eni2Device = "1"
eni2PrivateIP = "10.0.0.2"
eni2Prefix = "10.0.2.0/28"
eni2v6Prefix = "2001:db8::/64"
eni2ID = "eni-12341234"
eni3NetworkCard = "1"
eni3Device = "1"
eni3ID = "eni-67896789"
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,
metadataMACPath + primaryMAC + metadataDeviceNum: eni1Device,
metadataMACPath + primaryMAC + metadataInterface: primaryeniID,
metadataMACPath + primaryMAC + metadataNetworkCard: eni1NetworkCard,
metadataMACPath + primaryMAC + metadataSGs: sgs,
metadataMACPath + primaryMAC + metadataIPv4s: eni1PrivateIP,
metadataMACPath + primaryMAC + metadataSubnetID: subnetID,
metadataMACPath + primaryMAC + metadataVpcID: vpcID,
metadataMACPath + primaryMAC + metadataSubnetCIDR: subnetCIDR,
metadataMACPath + primaryMAC + metadataVPCcidrs: metadataVPCIPv4CIDRs,
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,
metadataMACPath + primaryMAC + metadataIPv4s: eni1PrivateIP,
metadataMACPath + primaryMAC + metadataSubnetID: subnetID,
metadataMACPath + primaryMAC + metadataVpcID: vpcID,
metadataMACPath + primaryMAC + metadataSubnetCIDR: subnetCIDR,
metadataMACPath + primaryMAC + metadataVPCcidrs: metadataVPCIPv4CIDRs,
}

for k, v := range overrides {
Expand All @@ -117,12 +113,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 @@ -211,12 +208,12 @@ func TestInitWithEC2metadataErr(t *testing.T) {

func TestGetAttachedENIs(t *testing.T) {
mockMetadata := testMetadata(map[string]interface{}{
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC + metadataNetworkCard: eni2NetworkCard,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataSubnetCIDR: subnetCIDR,
metadataMACPath + eni2MAC + metadataIPv4s: eni2PrivateIP,
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC: imdsMACFields,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataSubnetCIDR: subnetCIDR,
metadataMACPath + eni2MAC + metadataIPv4s: eni2PrivateIP,
})

cache := &EC2InstanceMetadataCache{imds: TypedIMDS{mockMetadata}}
Expand All @@ -226,17 +223,13 @@ func TestGetAttachedENIs(t *testing.T) {
}
}

func TestGetAttachedENIsWithEfa(t *testing.T) {
func TestGetAttachedENIsWithEfaOnly(t *testing.T) {
mockMetadata := testMetadata(map[string]interface{}{
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC + metadataNetworkCard: eni2NetworkCard,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataSubnetCIDR: subnetCIDR,
metadataMACPath + eni2MAC + metadataIPv4s: eni2PrivateIP,
metadataMACPath + eni3MAC + metadataNetworkCard: eni3NetworkCard,
metadataMACPath + eni3MAC + metadataDeviceNum: eni3Device,
metadataMACPath + eni3MAC + metadataInterface: eni3ID,
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC: imdsMACFieldsEfaOnly,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataSubnetCIDR: subnetCIDR,
})

cache := &EC2InstanceMetadataCache{imds: TypedIMDS{mockMetadata}}
Expand All @@ -248,8 +241,8 @@ func TestGetAttachedENIsWithEfa(t *testing.T) {

func TestGetAttachedENIsWithPrefixes(t *testing.T) {
mockMetadata := testMetadata(map[string]interface{}{
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC + metadataNetworkCard: eni2NetworkCard,
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC: imdsMACFields,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataSubnetCIDR: subnetCIDR,
Expand Down Expand Up @@ -373,6 +366,7 @@ func TestDescribeAllENIs(t *testing.T) {
Attachment: &ec2.NetworkInterfaceAttachment{
NetworkCardIndex: aws.Int64(0),
},
NetworkInterfaceId: aws.String(primaryeniID),
}},
}

Expand All @@ -387,7 +381,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 @@ -1036,12 +1030,12 @@ func TestEC2InstanceMetadataCache_waitForENIAndIPsAttached(t *testing.T) {
}
fmt.Println("eniips", eniIPs)
mockMetadata := testMetadata(map[string]interface{}{
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC + metadataNetworkCard: eni2NetworkCard,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataSubnetCIDR: subnetCIDR,
metadataMACPath + eni2MAC + metadataIPv4s: eniIPs,
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC: imdsMACFields,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataSubnetCIDR: subnetCIDR,
metadataMACPath + eni2MAC + metadataIPv4s: eniIPs,
})
cache := &EC2InstanceMetadataCache{imds: TypedIMDS{mockMetadata}, ec2SVC: mockEC2}
gotEniMetadata, err := cache.waitForENIAndIPsAttached(tt.args.eni, tt.args.wantedSecondaryIPs, tt.args.maxBackoffDelay)
Expand Down Expand Up @@ -1132,13 +1126,13 @@ func TestEC2InstanceMetadataCache_waitForENIAndPrefixesAttached(t *testing.T) {
eniPrefixes = ""
}
mockMetadata := testMetadata(map[string]interface{}{
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC + metadataNetworkCard: eni2NetworkCard,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataSubnetCIDR: subnetCIDR,
metadataMACPath + eni2MAC + metadataIPv4s: eniIPs,
metadataMACPath + eni2MAC + metaDataPrefixPath: eniPrefixes,
metadataMACPath: primaryMAC + " " + eni2MAC,
metadataMACPath + eni2MAC: imdsMACFields,
metadataMACPath + eni2MAC + metadataDeviceNum: eni2Device,
metadataMACPath + eni2MAC + metadataInterface: eni2ID,
metadataMACPath + eni2MAC + metadataSubnetCIDR: subnetCIDR,
metadataMACPath + eni2MAC + metadataIPv4s: eniIPs,
metadataMACPath + eni2MAC + metaDataPrefixPath: eniPrefixes,
})
cache := &EC2InstanceMetadataCache{imds: TypedIMDS{mockMetadata}, ec2SVC: mockEC2,
enablePrefixDelegation: true, v4Enabled: tt.args.v4Enabled, v6Enabled: tt.args.v6Enabled}
Expand Down
24 changes: 18 additions & 6 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
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we use GetMACs function and check for it.

  • Introducing yet another GetMACImdsFields - only to check for the fields might be introduced a too specialized a function for our purpose here.

Copy link
Member

Choose a reason for hiding this comment

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

It will be preferable to use GetMACs to continue with coverage here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orsenthil Can you add more details on using GetMACs.. So efa-only will not have any IP fields associated with them. So checking local-ipv4s field. Want to understand how we can use GetMACs here

Copy link
Member

Choose a reason for hiding this comment

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

@Pavani-Panakanti - you are right. I thought, we were using the network/interfaces/macs/ and some property of that output. This is different. Sorry for the distraction.

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 Expand Up @@ -166,12 +184,6 @@ func (imds TypedIMDS) getInt(ctx context.Context, key string) (int, error) {
return dataInt, err
}

// GetNetworkCard returns the unique network card number associated with an interface.
func (imds TypedIMDS) GetNetworkCard(ctx context.Context, mac string) (int, error) {
key := fmt.Sprintf("network/interfaces/macs/%s/network-card", mac)
return imds.getInt(ctx, key)
}

// GetDeviceNumber returns the unique device number associated with an interface. The primary interface is 0.
func (imds TypedIMDS) GetDeviceNumber(ctx context.Context, mac string) (int, error) {
key := fmt.Sprintf("network/interfaces/macs/%s/device-number", mac)
Expand Down