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.

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

Merged
merged 2 commits into from
Nov 21, 2024
Merged

[fix] : fix internal ip ordering #247

merged 2 commits into from
Nov 21, 2024

Conversation

rahulait
Copy link
Collaborator

@rahulait rahulait commented Nov 21, 2024

General:

  • Have you removed all sensitive information, including but not limited to access keys and passwords?
  • Have you checked to ensure there aren't other open or closed Pull Requests for the same bug/feature/question?

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:

  1. Does your submission pass tests?
  2. Have you added tests?
  3. Are you addressing a single feature in this PR?
  4. Are your commits atomic, addressing one change per commit?
  5. Are you following the conventions of the language?
  6. Have you saved your large formatting changes for a different PR, so we can focus on your work?
  7. Have you explained your rationale for why this feature is needed?
  8. Have you linked your PR to an open issue

for _, ip := range ips {
if _, ok := uniqueAddrs[ip.ip]; ok {
addresses = append(addresses, v1.NodeAddress{Type: ip.ipType, Address: ip.ip})
}
Copy link
Collaborator Author

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

Copy link
Contributor

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 😄

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 56.08%. Comparing base (9212805) to head (aaaf170).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cloud/linode/instances.go 87.50% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Contributor

@rosskirkpat rosskirkpat left a 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

Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Contributor

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:

if instance.IPv6 != "" {
ips = append(ips, nodeIP{ip: strings.TrimSuffix(instance.IPv6, "/128"), ipType: v1.NodeExternalIP})
}

Copy link
Collaborator Author

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.

@rahulait rahulait merged commit 66afcae into main Nov 21, 2024
6 checks passed
@AshleyDumaine AshleyDumaine deleted the fix-ip-ordering branch November 21, 2024 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants