Filter out interfaces with no ip info

[Pavani-Panakanti]

What type of PR is this?
Enhancement
-->

Which issue does this PR fix?:
Do not fetch ip info from imds for interfaces that will not have ip fields

What does this PR do / Why do we need it?:

Testing done on this change:
Tested with multi card and single card instance by deploying pods

normal interface imds fields

sh-4.2$ curl http://169.254.169.254/latest/meta-data/network/interfaces/macs/0e:bf:84:ab:27:65/
device-number
interface-id
local-hostname
local-ipv4s
mac
network-card
owner-id
security-group-ids
security-groups
subnet-id
subnet-ipv4-cidr-block
vpc-id
vpc-ipv4-cidr-block
vpc-ipv4-cidr-blocks

imds fields for interfaces with no ip info

sh-4.2$ curl http://169.254.169.254/latest/meta-data/network/interfaces/macs/0e:46:1b:72:19:89
device-number
interface-id
local-hostname
mac
network-card
owner-id
security-group-ids
security-groups
subnet-id
subnet-ipv4-cidr-block
vpc-id
vpc-ipv4-cidr-block
vpc-ipv4-cidr-blocks

Will this PR introduce any new dependencies?:
No

Will this break upgrades or downgrades? Has updating a running cluster been tested?:
No

Does this change require updates to the CNI daemonset config files to work?:
No

Does this PR introduce any user-facing change?:
No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[Pavani-Panakanti]

Few comments on earlier PR #3041

[Pavani-Panakanti]

Integration tests successful https://github.com/aws/amazon-vpc-cni-k8s/actions/runs/11061575263/job/30734454177

[orsenthil]

Why don't we flip the logic here?

	isEFAOnlyInterface := false

And if it true, then immediately return with appropriate empty fields

return ENIMetadata{
		ENIID:          eniID,
		MAC:            eniMAC,
		DeviceNumber:   deviceNum,
		SubnetIPv4CIDR: [],
		IPv4Addresses:  [],
		IPv4Prefixes:   [],
		SubnetIPv6CIDR: [],

Rest look good to me.

[Pavani-Panakanti]

Yeah we can do that

[Pavani-Panakanti]

Updated

[orsenthil]

In the description, please paste the output of the IMDS call made against an EFA instance that shows the "local-ipv4s" field vs a normal instance not having. For testing verification.

[Pavani-Panakanti]

In the description, please paste the output of the IMDS call made against an EFA instance that shows the "local-ipv4s" field vs a normal instance not having. For testing verification.

Added

[M00nF1sh]

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)

[Pavani-Panakanti]

@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

[orsenthil]

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.

[jayanthvn]

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)

[Pavani-Panakanti]

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

[orsenthil]

Okay, this reasoning sounds good to me.

[orsenthil]

Changes LGTM.

[orsenthil]

LGTM.