diff --git a/api/v1alpha7/types.go b/api/v1alpha7/types.go index 3ef5305ff2..345035007f 100644 --- a/api/v1alpha7/types.go +++ b/api/v1alpha7/types.go @@ -92,6 +92,8 @@ type PortOpts struct { // The names, uuids, filters or any combination these of the security groups to assign to the instance SecurityGroupFilters []SecurityGroupFilter `json:"securityGroupFilters,omitempty"` AllowedAddressPairs []AddressPair `json:"allowedAddressPairs,omitempty"` + // SymmetricAllowedAddressPairs if set to true, also updates the allowed_address_pairs of the VIP port with the IP address of the VM port for bidirectional security + SymmetricAllowedAddressPairs bool `json:"symmetricAllowedAddressPairs,omitempty"` // Enables and disables trunk at port level. If not provided, openStackMachine.Spec.Trunk is inherited. Trunk *bool `json:"trunk,omitempty"` diff --git a/pkg/cloud/services/networking/port.go b/pkg/cloud/services/networking/port.go index dd65f30333..e0675c2b00 100644 --- a/pkg/cloud/services/networking/port.go +++ b/pkg/cloud/services/networking/port.go @@ -27,6 +27,7 @@ import ( "github.com/gophercloud/gophercloud/openstack/networking/v2/ports" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/klog/v2" infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7" "sigs.k8s.io/cluster-api-provider-openstack/pkg/record" @@ -188,9 +189,69 @@ func (s *Service) GetOrCreatePort(eventObject runtime.Object, clusterName string } } + if len(addressPairs) > 0 && portOpts.SymmetricAllowedAddressPairs { + err := s.updateVIPsForSymmetricAllowedAddressPair(err, networkID, port, addressPairs) + if err != nil { + return nil, err + } + } + return port, nil } +// updateVIPsWithAllowedAddressPair updates the allowed_address_pairs of the VIP port with the IP address of the port +func (s *Service) updateVIPsForSymmetricAllowedAddressPair(err error, networkID string, port *ports.Port, addressPairs []ports.AddressPair) error { + if len(port.FixedIPs) == 0 { + return fmt.Errorf("the port %v is created but it has no IP address", port.ID) + } + + allPorts, err := s.client.ListPort(ports.ListOpts{ + NetworkID: networkID, + }) + if err != nil { + return err + } + + // Find matching VIP ports and update + for _, ap := range addressPairs { + for _, vip := range allPorts { + if vip.FixedIPs[0].IPAddress == ap.IPAddress { + err := s.updateVipWithAllowedAddressPairs(vip, port.FixedIPs[0].IPAddress, port.MACAddress) + if err != nil { + return err + } + } + } + } + return nil +} + +func (s *Service) updateVipWithAllowedAddressPairs(vip ports.Port, portIPAddress, portMACAddress string) error { + // If IP Address is found, not to update VIP + isfound := false + for _, raw := range vip.AllowedAddressPairs { + if raw.IPAddress == portIPAddress { + isfound = true + break + } + } + if !isfound { + vip.AllowedAddressPairs = append(vip.AllowedAddressPairs, ports.AddressPair{ + IPAddress: portIPAddress, + MACAddress: portMACAddress, + }) + updateOpts := ports.UpdateOpts{ + AllowedAddressPairs: &vip.AllowedAddressPairs, + } + klog.Infof("Updating VIP port %v with allowed_address_pairs: %+v", vip.ID, vip.AllowedAddressPairs) + _, err := s.client.UpdatePort(vip.ID, updateOpts) + if err != nil { + return err + } + } + return nil +} + func (s *Service) getSubnetIDForFixedIP(subnet *infrav1.SubnetFilter, networkID string) (string, error) { if subnet == nil { return "", nil diff --git a/pkg/cloud/services/networking/port_test.go b/pkg/cloud/services/networking/port_test.go index c0756296dc..3ffe1b545d 100644 --- a/pkg/cloud/services/networking/port_test.go +++ b/pkg/cloud/services/networking/port_test.go @@ -17,6 +17,7 @@ limitations under the License. package networking import ( + "fmt" "testing" "github.com/golang/mock/gomock" @@ -28,6 +29,8 @@ import ( "github.com/gophercloud/gophercloud/openstack/networking/v2/subnets" . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7" "sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock" ) @@ -667,3 +670,432 @@ func Test_GarbageCollectErrorInstancesPort(t *testing.T) { func pointerTo(b bool) *bool { return &b } + +// newEventObject returns a minimal runtime.Object for the eventObject argument. +func newEventObject() *corev1.Node { + return &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "test-node"}} +} + +// portWithIP builds a ports.Port fixture with one FixedIP and optional +// pre-existing AllowedAddressPairs. +func portWithIP(id, networkID, ip string, existingPairs ...ports.AddressPair) ports.Port { + return ports.Port{ + ID: id, + NetworkID: networkID, + MACAddress: "fa:16:3e:00:00:" + id[len(id)-2:], + FixedIPs: []ports.IP{{IPAddress: ip, SubnetID: "subnet-" + id}}, + AllowedAddressPairs: existingPairs, + } +} + +// minimalPortOpts returns a PortOpts that will cause GetOrCreatePort to create +// a new port. It sets only the fields required by the function under test. +func minimalPortOpts(networkID string, symmetric bool, allowedPairs ...infrav1.AddressPair) *infrav1.PortOpts { + return &infrav1.PortOpts{ + Network: &infrav1.NetworkFilter{ + ID: networkID, + }, + AllowedAddressPairs: allowedPairs, + SymmetricAllowedAddressPairs: symmetric, + } +} + +// newService constructs a networking.Service wired to the provided mock client. +func newService(t *testing.T, mc *mock.MockNetworkClient) *Service { + t.Helper() + return &Service{client: mc} +} + +// TestGetOrCreatePort_SymmetricDisabled checks that when +// SymmetricAllowedAddressPairs=false the VIP discovery ListPort and UpdatePort +// are never called, even when AllowedAddressPairs is populated. +func TestGetOrCreatePort_SymmetricDisabled(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mc := mock.NewMockNetworkClient(mockCtrl) + svc := newService(t, mc) + + const networkID = "net-1" + const portName = "test-port" + created := portWithIP("port-1", networkID, "10.0.0.100") + + // First ListPort: check for pre-existing port → none. + mc.EXPECT(). + ListPort(ports.ListOpts{Name: portName, NetworkID: networkID}). + Return([]ports.Port{}, nil) + // CreatePort: return the new machine port. + mc.EXPECT(). + CreatePort(gomock.Any()). + Return(&created, nil) + + // VIP discovery ListPort and UpdatePort must NOT be called. + // gomock will fail the test automatically if they are called unexpectedly. + + opts := minimalPortOpts(networkID, false, infrav1.AddressPair{IPAddress: "10.0.0.5"}) + port, err := svc.GetOrCreatePort(newEventObject(), "cluster", portName, opts, nil, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if port.ID != created.ID { + t.Errorf("got port ID %q, want %q", port.ID, created.ID) + } +} + +// TestGetOrCreatePort_SymmetricEnabled_UpdatesMatchingVIP is the core happy +// path: symmetric=true, one VIP IP in AllowedAddressPairs, the corresponding +// VIP port exists in the network → one UpdatePort call that adds the machine +// IP to the VIP's AllowedAddressPairs. +func TestGetOrCreatePort_SymmetricEnabled_UpdatesMatchingVIP(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mc := mock.NewMockNetworkClient(mockCtrl) + svc := newService(t, mc) + + const networkID = "net-2" + const portName = "test-port" + const machineIP = "10.0.0.100" + const vipIP = "10.0.0.5" + + machinePort := portWithIP("machine-port", networkID, machineIP) + vipPort := portWithIP("vip-port", networkID, vipIP) + + // 1. Check for pre-existing port. + mc.EXPECT(). + ListPort(ports.ListOpts{Name: portName, NetworkID: networkID}). + Return([]ports.Port{}, nil) + + // 2. Create the machine port. + mc.EXPECT(). + CreatePort(gomock.Any()). + Return(&machinePort, nil) + + // 3. VIP discovery: return all ports on the network. + mc.EXPECT(). + ListPort(ports.ListOpts{NetworkID: networkID}). + Return([]ports.Port{machinePort, vipPort}, nil) + + // 4. UpdatePort: VIP port must be updated with the machine IP. + mc.EXPECT(). + UpdatePort("vip-port", gomock.Any()). + DoAndReturn(func(id string, opts ports.UpdateOpts) (*ports.Port, error) { + if opts.AllowedAddressPairs == nil { + t.Errorf("UpdatePort called with nil AllowedAddressPairs") + return nil, fmt.Errorf("nil pairs") + } + found := false + for _, p := range *opts.AllowedAddressPairs { + if p.IPAddress == machineIP { + found = true + } + } + if !found { + t.Errorf("machine IP %q not found in UpdatePort opts %+v", machineIP, *opts.AllowedAddressPairs) + } + updated := vipPort + updated.AllowedAddressPairs = *opts.AllowedAddressPairs + return &updated, nil + }) + + opts := minimalPortOpts(networkID, true, infrav1.AddressPair{IPAddress: vipIP}) + port, err := svc.GetOrCreatePort(newEventObject(), "cluster", portName, opts, nil, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if port.ID != machinePort.ID { + t.Errorf("got port ID %q, want %q", port.ID, machinePort.ID) + } +} + +// TestGetOrCreatePort_SymmetricEnabled_NoAddressPairs verifies that when +// symmetric=true but AllowedAddressPairs is empty, the len(addressPairs) > 0 +// guard in GetOrCreatePort short-circuits: no VIP discovery ListPort and no +// UpdatePort are called. +func TestGetOrCreatePort_SymmetricEnabled_NoAddressPairs(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mc := mock.NewMockNetworkClient(mockCtrl) + svc := newService(t, mc) + + const networkID = "net-3" + const portName = "test-port" + created := portWithIP("port-3", networkID, "10.0.0.6") + + mc.EXPECT(). + ListPort(ports.ListOpts{Name: portName, NetworkID: networkID}). + Return([]ports.Port{}, nil) + mc.EXPECT(). + CreatePort(gomock.Any()). + Return(&created, nil) + + // No second ListPort or UpdatePort expected. + + opts := minimalPortOpts(networkID, true /* no AllowedAddressPairs */) + _, err := svc.GetOrCreatePort(newEventObject(), "cluster", portName, opts, nil, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// TestGetOrCreatePort_SymmetricEnabled_VIPNotInNetwork verifies that when no +// port in the network has an IP matching an allowed address pair, UpdatePort +// is never called. +func TestGetOrCreatePort_SymmetricEnabled_VIPNotInNetwork(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mc := mock.NewMockNetworkClient(mockCtrl) + svc := newService(t, mc) + + const networkID = "net-4" + const portName = "test-port" + machinePort := portWithIP("port-4", networkID, "10.0.0.100") + + mc.EXPECT(). + ListPort(ports.ListOpts{Name: portName, NetworkID: networkID}). + Return([]ports.Port{}, nil) + mc.EXPECT(). + CreatePort(gomock.Any()). + Return(&machinePort, nil) + // VIP discovery: only the machine port is on this network; + // no port has IP 10.0.0.5 so no match is found. + mc.EXPECT(). + ListPort(ports.ListOpts{NetworkID: networkID}). + Return([]ports.Port{machinePort}, nil) + + // UpdatePort must NOT be called. + + opts := minimalPortOpts(networkID, true, infrav1.AddressPair{IPAddress: "10.0.0.5"}) + _, err := svc.GetOrCreatePort(newEventObject(), "cluster", portName, opts, nil, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// TestGetOrCreatePort_SymmetricEnabled_MachineIPAlreadyInVIP verifies +// idempotency: updateVipWithAllowedAddressPairs skips UpdatePort when the +// machine IP is already present in the VIP's AllowedAddressPairs. +func TestGetOrCreatePort_SymmetricEnabled_MachineIPAlreadyInVIP(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mc := mock.NewMockNetworkClient(mockCtrl) + svc := newService(t, mc) + + const networkID = "net-5" + const portName = "test-port" + const machineIP = "10.0.0.100" + const vipIP = "10.0.0.5" + + machinePort := portWithIP("machine-port-5", networkID, machineIP) + // VIP already carries the machine IP — no update should be needed. + vipPort := portWithIP("vip-port-5", networkID, vipIP, + ports.AddressPair{IPAddress: machineIP, MACAddress: machinePort.MACAddress}, + ) + + mc.EXPECT(). + ListPort(ports.ListOpts{Name: portName, NetworkID: networkID}). + Return([]ports.Port{}, nil) + mc.EXPECT(). + CreatePort(gomock.Any()). + Return(&machinePort, nil) + mc.EXPECT(). + ListPort(ports.ListOpts{NetworkID: networkID}). + Return([]ports.Port{machinePort, vipPort}, nil) + + // Machine IP already present → UpdatePort must NOT be called. + + opts := minimalPortOpts(networkID, true, infrav1.AddressPair{IPAddress: vipIP}) + _, err := svc.GetOrCreatePort(newEventObject(), "cluster", portName, opts, nil, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// TestGetOrCreatePort_SymmetricEnabled_PreservesExistingVIPPairs verifies +// that pre-existing pairs on the VIP port are kept when the machine IP is +// appended. +func TestGetOrCreatePort_SymmetricEnabled_PreservesExistingVIPPairs(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mc := mock.NewMockNetworkClient(mockCtrl) + svc := newService(t, mc) + + const networkID = "net-6" + const portName = "test-port" + const machineIP = "10.0.0.100" + const vipIP = "10.0.0.5" + const existingPairIP = "192.168.1.50" + + machinePort := portWithIP("machine-port-6", networkID, machineIP) + vipPort := portWithIP("vip-port-6", networkID, vipIP, + ports.AddressPair{IPAddress: existingPairIP}, + ) + + mc.EXPECT(). + ListPort(ports.ListOpts{Name: portName, NetworkID: networkID}). + Return([]ports.Port{}, nil) + mc.EXPECT(). + CreatePort(gomock.Any()). + Return(&machinePort, nil) + mc.EXPECT(). + ListPort(ports.ListOpts{NetworkID: networkID}). + Return([]ports.Port{machinePort, vipPort}, nil) + + mc.EXPECT(). + UpdatePort("vip-port-6", gomock.Any()). + DoAndReturn(func(id string, opts ports.UpdateOpts) (*ports.Port, error) { + pairIPs := make(map[string]bool) + for _, p := range *opts.AllowedAddressPairs { + pairIPs[p.IPAddress] = true + } + if !pairIPs[existingPairIP] { + t.Errorf("pre-existing pair IP %q was dropped from UpdatePort opts", existingPairIP) + } + if !pairIPs[machineIP] { + t.Errorf("machine IP %q was not added to UpdatePort opts", machineIP) + } + updated := vipPort + updated.AllowedAddressPairs = *opts.AllowedAddressPairs + return &updated, nil + }) + + opts := minimalPortOpts(networkID, true, infrav1.AddressPair{IPAddress: vipIP}) + _, err := svc.GetOrCreatePort(newEventObject(), "cluster", portName, opts, nil, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// TestGetOrCreatePort_SymmetricEnabled_MultipleVIPs verifies that when +// AllowedAddressPairs lists multiple VIP IPs, each matching port on the +// network gets its own UpdatePort call. +func TestGetOrCreatePort_SymmetricEnabled_MultipleVIPs(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mc := mock.NewMockNetworkClient(mockCtrl) + svc := newService(t, mc) + + const networkID = "net-7" + const portName = "test-port" + const machineIP = "10.0.0.100" + + machinePort := portWithIP("machine-port-7", networkID, machineIP) + vipPort1 := portWithIP("vip-port-7a", networkID, "10.0.0.5") + vipPort2 := portWithIP("vip-port-7b", networkID, "10.0.0.7") + + mc.EXPECT(). + ListPort(ports.ListOpts{Name: portName, NetworkID: networkID}). + Return([]ports.Port{}, nil) + mc.EXPECT(). + CreatePort(gomock.Any()). + Return(&machinePort, nil) + mc.EXPECT(). + ListPort(ports.ListOpts{NetworkID: networkID}). + Return([]ports.Port{machinePort, vipPort1, vipPort2}, nil) + + // Both VIP ports must receive exactly one UpdatePort call each. + mc.EXPECT().UpdatePort("vip-port-7a", gomock.Any()).Return(&vipPort1, nil) + mc.EXPECT().UpdatePort("vip-port-7b", gomock.Any()).Return(&vipPort2, nil) + + opts := minimalPortOpts(networkID, true, + infrav1.AddressPair{IPAddress: "10.0.0.5"}, + infrav1.AddressPair{IPAddress: "10.0.0.7"}, + ) + _, err := svc.GetOrCreatePort(newEventObject(), "cluster", portName, opts, nil, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// TestGetOrCreatePort_SymmetricEnabled_VIPDiscoveryListPortError verifies +// that when the VIP discovery ListPort call fails, the error is propagated +// and UpdatePort is never attempted. +func TestGetOrCreatePort_SymmetricEnabled_VIPDiscoveryListPortError(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mc := mock.NewMockNetworkClient(mockCtrl) + svc := newService(t, mc) + + const networkID = "net-8" + const portName = "test-port" + created := portWithIP("port-8", networkID, "10.0.0.11") + + mc.EXPECT(). + ListPort(ports.ListOpts{Name: portName, NetworkID: networkID}). + Return([]ports.Port{}, nil) + mc.EXPECT(). + CreatePort(gomock.Any()). + Return(&created, nil) + mc.EXPECT(). + ListPort(ports.ListOpts{NetworkID: networkID}). + Return(nil, fmt.Errorf("neutron: internal server error")) + + opts := minimalPortOpts(networkID, true, infrav1.AddressPair{IPAddress: "10.0.0.5"}) + _, err := svc.GetOrCreatePort(newEventObject(), "cluster", portName, opts, nil, nil) + if err == nil { + t.Fatal("expected an error from VIP discovery ListPort failure, got nil") + } +} + +// TestGetOrCreatePort_SymmetricEnabled_UpdatePortError verifies that a +// Neutron error from UpdatePort is propagated back through GetOrCreatePort. +func TestGetOrCreatePort_SymmetricEnabled_UpdatePortError(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mc := mock.NewMockNetworkClient(mockCtrl) + svc := newService(t, mc) + + const networkID = "net-9" + const portName = "test-port" + const machineIP = "10.0.0.100" + const vipIP = "10.0.0.5" + + machinePort := portWithIP("machine-port-9", networkID, machineIP) + vipPort := portWithIP("vip-port-9", networkID, vipIP) + + mc.EXPECT(). + ListPort(ports.ListOpts{Name: portName, NetworkID: networkID}). + Return([]ports.Port{}, nil) + mc.EXPECT(). + CreatePort(gomock.Any()). + Return(&machinePort, nil) + mc.EXPECT(). + ListPort(ports.ListOpts{NetworkID: networkID}). + Return([]ports.Port{machinePort, vipPort}, nil) + mc.EXPECT(). + UpdatePort("vip-port-9", gomock.Any()). + Return(nil, fmt.Errorf("neutron: quota exceeded")) + + opts := minimalPortOpts(networkID, true, infrav1.AddressPair{IPAddress: vipIP}) + _, err := svc.GetOrCreatePort(newEventObject(), "cluster", portName, opts, nil, nil) + if err == nil { + t.Fatal("expected an error from UpdatePort failure, got nil") + } +} + +// TestGetOrCreatePort_ReturnsExistingPort verifies the short-circuit path: +// when ListPort finds exactly one pre-existing port it is returned directly +// with no CreatePort, VIP discovery, or UpdatePort calls. +func TestGetOrCreatePort_ReturnsExistingPort(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mc := mock.NewMockNetworkClient(mockCtrl) + svc := newService(t, mc) + + const networkID = "net-11" + const portName = "test-port" + existing := portWithIP("existing-port", networkID, "10.0.0.20") + + mc.EXPECT(). + ListPort(ports.ListOpts{Name: portName, NetworkID: networkID}). + Return([]ports.Port{existing}, nil) + + // None of these must be called. + + opts := minimalPortOpts(networkID, true, infrav1.AddressPair{IPAddress: "10.0.0.5"}) + port, err := svc.GetOrCreatePort(newEventObject(), "cluster", portName, opts, nil, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if port.ID != existing.ID { + t.Errorf("got port ID %q, want %q", port.ID, existing.ID) + } +}