-
Notifications
You must be signed in to change notification settings - Fork 62
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.
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
[fix] : fix internal ip ordering #247
Conversation
for _, ip := range ips { | ||
if _, ok := uniqueAddrs[ip.ip]; ok { | ||
addresses = append(addresses, v1.NodeAddress{Type: ip.ipType, Address: ip.ip}) | ||
} |
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.
Adding ips first here as the ordering is already done in getInstanceAddresses() https://github.com/linode/linode-cloud-controller-manager/blob/main/cloud/linode/instances.go#L41-L63
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.
thanks for clarifying this, I was about to leave a comment asking about that 😄
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #247 +/- ##
==========================================
+ Coverage 56.06% 56.08% +0.01%
==========================================
Files 12 12
Lines 2349 2350 +1
==========================================
+ Hits 1317 1318 +1
Misses 881 881
Partials 151 151 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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.
overall, LGTM. Requested a new test to cover the specific scenario where this bug was observed
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.
I noticed that there is no test covering the node scenario where we found this bug. Would you be able to add a test for it?
Here are the details:
Nodes have four IPs
- VPC Internal IP
- Private Internal IP
- ipv4 External IP
- ipv6 External IP
We need the VPC internal IP listed first in InternalIP and the ipv4 external IP listed first in ExternalIP
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.
Not sure about first externalIP as ipv4 only as we never did that before, but let me see if we can add such logic easily to the code. Maybe remove external ipv6's from slice and add it to the end of slice.
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.
I believe the code is already doing that here:
linode-cloud-controller-manager/cloud/linode/instances.go
Lines 58 to 60 in 9212805
if instance.IPv6 != "" { | |
ips = append(ips, nodeIP{ip: strings.TrimSuffix(instance.IPv6, "/128"), ipType: v1.NodeExternalIP}) | |
} |
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, true. Added test to make sure vpc ip is first and ipv6 at the end. external ipv4 and other internal ip's can be in any order.
General:
Node's internal ip is set based on whatever ip is returned as first internal ip. In case of VPC, we always want that ip to be node's internal ip. We do that ordering when returning node's ip-addresses. However, we were storing things in map and re-generating which was changing the order. Instead, we can just rely on map to tell if its unique or not and then append to the slice. This PR fixes that by making sure node's actual ip's are listed first and then the ones set by kubelet.
Pull Request Guidelines: