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

Jump to bottom

[fix] : fix internal ip ordering #247

Merged
merged 2 commits into from
Nov 21, 2024
Merged

[fix] : fix internal ip ordering #247

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e7d9bb
fix internal ip ordering
rahulait Nov 21, 2024
aaaf170
add test to make sure ip's are in specific order
rahulait Nov 21, 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
18 changes: 10 additions & 8 deletions cloud/linode/instances.go
View file
Open in desktop
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:

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})
}

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.

Original file line number Diff line number Diff line change
Expand Up @@ -284,12 +284,18 @@ func (i *instances) InstanceMetadata(ctx context.Context, node *v1.Node) (*cloud
return nil, err
}

uniqueAddrs := make(map[string]v1.NodeAddressType, len(node.Status.Addresses)+len(ips))
addresses := []v1.NodeAddress{{Type: v1.NodeHostName, Address: linode.Label}}
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 😄


// create temporary uniqueAddrs cache just for reference
uniqueAddrs := make(map[string]v1.NodeAddressType, len(node.Status.Addresses)+len(ips))
for _, ip := range addresses {
if _, ok := uniqueAddrs[ip.Address]; ok {
continue
}
uniqueAddrs[ip.ip] = ip.ipType
uniqueAddrs[ip.Address] = ip.Type
}

// include IPs set by kubelet for internal node IP
Expand All @@ -299,14 +305,10 @@ func (i *instances) InstanceMetadata(ctx context.Context, node *v1.Node) (*cloud
}
if addr.Type == v1.NodeInternalIP {
uniqueAddrs[addr.Address] = v1.NodeInternalIP
addresses = append(addresses, v1.NodeAddress{Type: addr.Type, Address: addr.Address})
}
}

addresses := []v1.NodeAddress{{Type: v1.NodeHostName, Address: linode.Label}}
for k, v := range uniqueAddrs {
addresses = append(addresses, v1.NodeAddress{Type: v, Address: k})
}

klog.Infof("Instance %s, assembled IP addresses: %v", node.Name, addresses)
// note that Zone is omitted as it's not a thing in Linode
meta := &cloudprovider.InstanceMetadata{
Expand Down
Loading