Gracefully handle v4 (unmanaged) ENIs on IPv6 node #3489
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where cross-VPC IPv4-only ENIs fail to initialize on IPv6-enabled nodes by making getCIDR return nil instead of an error when encountering 404 Not Found responses.
Key changes:
- Modified
getCIDRfunction to return*net.IPNetinstead ofnet.IPNetand handle 404 errors gracefully by returningnil - Updated
GetSubnetIPv4CIDRBlockandGetSubnetIPv6CIDRBlocksreturn types to*net.IPNet - Added nil checks in
getENIMetadatato handle cases where IPv6 CIDR blocks are not available - Added comprehensive test coverage for the cross-VPC ENI scenario
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/awsutils/imds.go | Changed getCIDR to return pointer and handle 404 errors gracefully; updated method signatures |
| pkg/awsutils/imds_test.go | Updated tests to work with pointer return types |
| pkg/awsutils/awsutils.go | Added nil checks for IPv6 CIDR and removed unused isNotFoundError function |
| pkg/awsutils/awsutils_test.go | Added comprehensive test case for cross-VPC ENI with IPv6 enabled nodes |
Comments suppressed due to low confidence (1)
pkg/awsutils/awsutils.go:713
- Potential nil pointer dereference. Since
GetSubnetIPv4CIDRBlocknow returns*net.IPNet, it can returnnilon 404 errors. The code should check ifcidr != nilbefore callingcidr.String()on line 713, similar to the nil check added for IPv6 CIDR on lines 742-744.
cidr, err := cache.imds.GetSubnetIPv4CIDRBlock(ctx, eniMAC)
if err != nil {
awsAPIErrInc("GetSubnetIPv4CIDRBlock", err)
return ENIMetadata{}, err
}
subnetV4Cidr = cidr.String()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/awsutils/awsutils.go
Outdated
| // isNotFoundError checks if the error is a 404 Not Found error | ||
| func isNotFoundError(err error) bool { | ||
| if err == nil { | ||
| return false | ||
| } | ||
|
|
||
| // Check for HTTP status code 404 | ||
| type httpStatusCoder interface { | ||
| HTTPStatusCode() int | ||
| } | ||
| if httpErr, ok := err.(httpStatusCoder); ok && httpErr.HTTPStatusCode() == 404 { | ||
| return true | ||
| } | ||
|
|
||
| // Check for error code "NotFound" | ||
| type errorCoder interface { | ||
| ErrorCode() string | ||
| } | ||
| if codeErr, ok := err.(errorCoder); ok && codeErr.ErrorCode() == "NotFound" { | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
The isNotFoundError function appears to be unused and duplicates functionality already provided by the existing IsNotFound function in pkg/awsutils/imds.go (line 655). This creates code duplication and potential maintenance issues. Consider removing this function.
| // isNotFoundError checks if the error is a 404 Not Found error | |
| func isNotFoundError(err error) bool { | |
| if err == nil { | |
| return false | |
| } | |
| // Check for HTTP status code 404 | |
| type httpStatusCoder interface { | |
| HTTPStatusCode() int | |
| } | |
| if httpErr, ok := err.(httpStatusCoder); ok && httpErr.HTTPStatusCode() == 404 { | |
| return true | |
| } | |
| // Check for error code "NotFound" | |
| type errorCoder interface { | |
| ErrorCode() string | |
| } | |
| if codeErr, ok := err.(errorCoder); ok && codeErr.ErrorCode() == "NotFound" { | |
| return true | |
| } | |
| return false | |
| } |
pkg/awsutils/awsutils.go
Outdated
| imdsIPv6s, err := cache.imds.GetIPv6s(ctx, eniMAC) | ||
| if err != nil { | ||
| awsAPIErrInc("GetIPv6s", err) | ||
| return ENIMetadata{}, err |
There was a problem hiding this comment.
[nitpick] The error handling logic at line 750 is inconsistent with the pattern used for GetSubnetIPv6CIDRBlocks. For GetIPv6s, errors are returned immediately, while GetSubnetIPv6CIDRBlocks errors were previously handled but are now followed by a nil check. Consider maintaining consistency by continuing execution after GetIPv6s errors in case some ENIs have IPv6 CIDRs but no IPv6 addresses assigned.
| return ENIMetadata{}, err | |
| // Continue execution: some ENIs may have IPv6 CIDRs but no IPv6 addresses assigned | |
| ec2ip6s = []ec2types.NetworkInterfaceIpv6Address{} |
pkg/awsutils/awsutils.go
Outdated
| imdsIPv6s, err := cache.imds.GetIPv6s(ctx, eniMAC) | ||
| if err != nil { | ||
| awsAPIErrInc("GetIPv6s", err) | ||
| return ENIMetadata{}, err |
There was a problem hiding this comment.
Can you remove this line ? Even IPv6s will be missing from an IPv4 ENI
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
pkg/awsutils/awsutils.go:713
- Potential nil pointer dereference. The function
GetSubnetIPv4CIDRBlocknow returns*net.IPNetand can returnnilwhen a 404 error occurs (as seen in the updatedgetCIDRimplementation). This code should check ifcidr != nilbefore calling.String(), similar to how the IPv6 case is handled at lines 742-744.
subnetV4Cidr = cidr.String()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2 participants
What type of PR is this?
bug
Which issue does this PR fix?:
What does this PR do / Why do we need it?:
Gracefully handle v4 unmanaged ENI on ipv6 node
Testing done on this change:
Ran ipv6 integration tests successfully
Will this PR introduce any new dependencies?:
Will this break upgrades or downgrades? Has updating a running cluster been tested?:
Does this change require updates to the CNI daemonset config files to work?:
No
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.