From 2b08b9430dbe95cac628d154fa6754cb99a5bd87 Mon Sep 17 00:00:00 2001 From: Lucas Barros Date: Sun, 8 Nov 2026 00:15:25 -0300 Subject: [PATCH] fix: match firewall devices by entity type as well as ID getNodeBalancerDeviceID looked for the NodeBalancer among the firewall devices by entity ID only. Linodes, Linode interfaces and NodeBalancers use separate ID spaces, so a firewall shared by a Linode and the NodeBalancer could return the Linode device when both IDs are equal. Replacing or removing the NodeBalancer firewall then detached the Linode from its firewall and left the NodeBalancer attached. Only match devices whose entity type is nodebalancer. Signed-off-by: Lucas Barros --- cloud/linode/services/firewalls.go | 4 +- .../linode/services/firewalls_device_test.go | 109 ++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 cloud/linode/services/firewalls_device_test.go diff --git a/cloud/linode/services/firewalls.go b/cloud/linode/services/firewalls.go index 009cfc9e..86883bd6 100644 --- a/cloud/linode/services/firewalls.go +++ b/cloud/linode/services/firewalls.go @@ -459,7 +459,9 @@ func (l *LinodeClient) getNodeBalancerDeviceID(ctx context.Context, firewallID, } for i := range devices { - if devices[i].Entity.ID == nbID { + // Linodes, Linode interfaces and NodeBalancers have separate ID spaces, so the + // entity type must be checked as well as the ID. + if devices[i].Entity.Type == linodego.FirewallDeviceNodeBalancer && devices[i].Entity.ID == nbID { return devices[i].ID, true, nil } } diff --git a/cloud/linode/services/firewalls_device_test.go b/cloud/linode/services/firewalls_device_test.go new file mode 100644 index 00000000..fc8ebb6f --- /dev/null +++ b/cloud/linode/services/firewalls_device_test.go @@ -0,0 +1,109 @@ +package services + +import ( + "testing" + + "github.com/golang/mock/gomock" + "github.com/linode/linodego/v2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/linode/linode-cloud-controller-manager/cloud/annotations" + "github.com/linode/linode-cloud-controller-manager/cloud/linode/client/mocks" +) + +// devicesWithIDCollision returns the devices of a firewall that protects both a Linode and a +// NodeBalancer which happen to share the same entity ID. +func devicesWithIDCollision(entityID, linodeDeviceID, nbDeviceID int) []linodego.FirewallDevice { + return []linodego.FirewallDevice{ + {ID: linodeDeviceID, Entity: linodego.FirewallDeviceEntity{ID: entityID, Type: linodego.FirewallDeviceLinode}}, + {ID: nbDeviceID, Entity: linodego.FirewallDeviceEntity{ID: entityID, Type: linodego.FirewallDeviceNodeBalancer}}, + } +} + +func TestGetNodeBalancerDeviceIDIgnoresOtherEntityTypes(t *testing.T) { + const ( + firewallID = 1 + nbID = 4242 + linodeDeviceID = 10 + nbDeviceID = 11 + ) + + t.Run("matches the nodebalancer device when a linode has the same ID", func(t *testing.T) { + ctrl := gomock.NewController(t) + client := mocks.NewMockClient(ctrl) + client.EXPECT().ListFirewallDevices(gomock.Any(), firewallID, gomock.Any()).Return(devicesWithIDCollision(nbID, linodeDeviceID, nbDeviceID), nil) + + l := &LinodeClient{Client: client} + deviceID, exists, err := l.getNodeBalancerDeviceID(t.Context(), firewallID, nbID) + require.NoError(t, err) + assert.True(t, exists) + assert.Equal(t, nbDeviceID, deviceID) + }) + + t.Run("does not match a linode or linode interface with the nodebalancer ID", func(t *testing.T) { + ctrl := gomock.NewController(t) + client := mocks.NewMockClient(ctrl) + client.EXPECT().ListFirewallDevices(gomock.Any(), firewallID, gomock.Any()).Return([]linodego.FirewallDevice{ + {ID: linodeDeviceID, Entity: linodego.FirewallDeviceEntity{ID: nbID, Type: linodego.FirewallDeviceLinode}}, + {ID: nbDeviceID, Entity: linodego.FirewallDeviceEntity{ID: nbID, Type: linodego.FirewallDeviceLinodeInterface}}, + }, nil) + + l := &LinodeClient{Client: client} + _, exists, err := l.getNodeBalancerDeviceID(t.Context(), firewallID, nbID) + require.NoError(t, err) + assert.False(t, exists) + }) +} + +func TestReplaceNBFirewallKeepsLinodeWithSameID(t *testing.T) { + const ( + oldFirewallID = 1 + newFirewallID = 2 + nbID = 4242 + linodeDeviceID = 10 + nbDeviceID = 11 + ) + + ctrl := gomock.NewController(t) + client := mocks.NewMockClient(ctrl) + client.EXPECT().ListNodeBalancerFirewalls(gomock.Any(), nbID, gomock.Any()).Return([]linodego.Firewall{{ID: oldFirewallID}}, nil) + client.EXPECT().CreateFirewallDevice(gomock.Any(), newFirewallID, linodego.FirewallDeviceCreateOptions{ID: nbID, Type: "nodebalancer"}).Return(&linodego.FirewallDevice{}, nil) + client.EXPECT().ListFirewallDevices(gomock.Any(), oldFirewallID, gomock.Any()).Return(devicesWithIDCollision(nbID, linodeDeviceID, nbDeviceID), nil) + // only the nodebalancer leaves the old firewall, the linode stays protected by it + client.EXPECT().DeleteFirewallDevice(gomock.Any(), oldFirewallID, nbDeviceID).Return(nil) + + svc := &v1.Service{ObjectMeta: metav1.ObjectMeta{ + Name: "svc", + Annotations: map[string]string{annotations.AnnLinodeCloudFirewallID: "2"}, + }} + l := &LinodeClient{Client: client} + err := l.UpdateNodeBalancerFirewall(t.Context(), "lb", nil, svc, &linodego.NodeBalancer{ID: nbID}) + require.NoError(t, err) +} + +func TestRemoveNBFirewallKeepsLinodeWithSameID(t *testing.T) { + const ( + firewallID = 1 + nbID = 4242 + linodeDeviceID = 10 + nbDeviceID = 11 + ) + + ctrl := gomock.NewController(t) + client := mocks.NewMockClient(ctrl) + client.EXPECT().ListNodeBalancerFirewalls(gomock.Any(), nbID, gomock.Any()).Return([]linodego.Firewall{{ID: firewallID}}, nil) + gomock.InOrder( + client.EXPECT().ListFirewallDevices(gomock.Any(), firewallID, gomock.Any()).Return(devicesWithIDCollision(nbID, linodeDeviceID, nbDeviceID), nil), + client.EXPECT().DeleteFirewallDevice(gomock.Any(), firewallID, nbDeviceID).Return(nil), + client.EXPECT().ListFirewallDevices(gomock.Any(), firewallID, gomock.Any()).Return(devicesWithIDCollision(nbID, linodeDeviceID, nbDeviceID)[:1], nil), + ) + // the linode is still attached, so the firewall is neither deleted nor touched further + + svc := &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "svc"}} + l := &LinodeClient{Client: client} + err := l.UpdateNodeBalancerFirewall(t.Context(), "lb", nil, svc, &linodego.NodeBalancer{ID: nbID}) + require.NoError(t, err) +}