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

Show file tree
Hide file tree
Changes from all commits
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
88 changes: 86 additions & 2 deletions cloud/linode/instances_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,90 @@ func TestMetadataRetrieval(t *testing.T) {
}, meta.NodeAddresses)
})

t.Run("should return data when linode is found (by name) and addresses must be in order", func(t *testing.T) {
instances := newInstances(client)
id := 123
name := "mock-instance"
node := nodeWithName(name)
publicIPv4 := net.ParseIP("45.76.101.25")
privateIPv4 := net.ParseIP("192.168.133.65")
ipv6Addr := "2001::8a2e:370:7348"
linodeType := "g6-standard-1"
region := "us-east"

Options.VPCNames = "test"
vpcIDs["test"] = 1
Options.EnableRouteController = true

instance := linodego.Instance{
ID: id,
Label: name,
Type: linodeType,
Region: region,
IPv4: []*net.IP{&publicIPv4, &privateIPv4},
IPv6: ipv6Addr,
}
vpcIP := "10.0.0.2"
addressRange1 := "10.192.0.0/24"
addressRange2 := "10.192.10.0/24"
routesInVPC := []linodego.VPCIP{
{
Address: &vpcIP,
AddressRange: nil,
VPCID: vpcIDs["test"],
NAT1To1: nil,
LinodeID: id,
},
{
Address: nil,
AddressRange: &addressRange1,
VPCID: vpcIDs["test"],
NAT1To1: nil,
LinodeID: id,
},
{
Address: nil,
AddressRange: &addressRange2,
VPCID: vpcIDs["test"],
NAT1To1: nil,
LinodeID: id,
},
}

client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{instance}, nil)
client.EXPECT().ListVPCIPAddresses(gomock.Any(), vpcIDs["test"], gomock.Any()).Return(routesInVPC, nil)

meta, err := instances.InstanceMetadata(ctx, node)
assert.NoError(t, err)
assert.Equal(t, providerIDPrefix+strconv.Itoa(id), meta.ProviderID)
assert.Equal(t, region, meta.Region)
assert.Equal(t, linodeType, meta.InstanceType)
assert.Equal(t, []v1.NodeAddress{
{
Type: v1.NodeHostName,
Address: name,
},
{
Type: v1.NodeInternalIP,
Address: vpcIP,
},
{
Type: v1.NodeExternalIP,
Address: publicIPv4.String(),
},
{
Type: v1.NodeInternalIP,
Address: privateIPv4.String(),
},
{
Type: v1.NodeExternalIP,
Address: ipv6Addr,
},
}, meta.NodeAddresses)

Options.VPCNames = ""
})

ipTests := []struct {
name string
inputIPv4s []string
Expand Down Expand Up @@ -226,7 +310,7 @@ func TestMetadataRetrieval(t *testing.T) {
nil,
},
{
"one private addresses, one existing internal IP set on the node",
"one private address, one existing internal IP set on the node",
[]string{"192.168.121.42"},
"",
"",
Expand Down Expand Up @@ -329,7 +413,7 @@ func TestMetadataRetrieval(t *testing.T) {
expectedInstance := linodego.Instance{Label: "expected-instance", ID: 12345, IPv4: []*net.IP{&publicIP, &privateIP}}

for _, test := range getByIPTests {
t.Run(fmt.Sprintf("gets lindoe by IP - %s", test.name), func(t *testing.T) {
t.Run(fmt.Sprintf("gets linode by IP - %s", test.name), func(t *testing.T) {
instances := newInstances(client)
client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{{ID: 3456, IPv4: []*net.IP{&wrongIP}}, expectedInstance}, nil)
node := v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "test-node-1"}, Status: v1.NodeStatus{Addresses: test.nodeAddresses}}
Expand Down
Loading