port: don't add any SGs when port security is disabled

When port security is disabled on a port, don't add any security group
to the port options.

Skip the security groups when resolving the ports spec.
Report an error when a port tries to be created with both security
groups and disable port security to true.
Adds unit tests coverage for these scenarios.

Author: EmilienM

pkg/cloud/services/networking/port.go

@@ -170,6 +170,9 @@ func (s *Service) CreatePort(eventObject runtime.Object, portSpec *infrav1.Resol
 		createOpts.FixedIPs = fixedIPs
 	}
 	if portSpec.SecurityGroups != nil {
+		if ptr.Deref(portSpec.DisablePortSecurity, false) {
+			return nil, errors.New("security groups cannot be set when port security is disabled")
+		}
 		createOpts.SecurityGroups = &portSpec.SecurityGroups
 	}
 	builder = createOpts
@@ -434,13 +437,15 @@ func (s *Service) normalizePorts(ports []infrav1.PortOpts, clusterResourceName,
 			return nil, err
 		}
 
-		// Resolve security groups
-		if len(port.SecurityGroups) == 0 {
-			normalizedPort.SecurityGroups = defaultSecurityGroupIDs
-		} else {
-			normalizedPort.SecurityGroups, err = s.GetSecurityGroups(port.SecurityGroups)
-			if err != nil {
-				return nil, fmt.Errorf("error getting security groups: %v", err)
+		// Resolve security groups when port security is not disabled
+		if !ptr.Deref(port.DisablePortSecurity, false) {
+			if len(port.SecurityGroups) == 0 {
+				normalizedPort.SecurityGroups = defaultSecurityGroupIDs
+			} else {
+				normalizedPort.SecurityGroups, err = s.GetSecurityGroups(port.SecurityGroups)
+				if err != nil {
+					return nil, fmt.Errorf("error getting security groups: %v", err)
+				}
 			}
 		}
 	}

pkg/cloud/services/networking/port_test.go

@@ -192,6 +192,21 @@ func Test_CreatePort(t *testing.T) {
 			},
 			want: &ports.Port{ID: portID},
 		},
+		{
+			name: "disable port security with security groups produces an error",
+			port: infrav1.ResolvedPortSpec{
+				Name:      "test-port",
+				NetworkID: netID,
+				ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
+					DisablePortSecurity: ptr.To(true),
+				},
+				SecurityGroups: []string{portSecurityGroupID},
+			},
+			expect: func(m *mock.MockNetworkClientMockRecorder, _ Gomega) {
+				m.CreatePort(gomock.Any()).Times(0)
+			},
+			wantErr: true,
+		},
 		{
 			name: "disable port security also ignores allowed address pairs",
 			port: infrav1.ResolvedPortSpec{
@@ -780,6 +795,40 @@ func TestService_ConstructPorts(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "port security disabled override machine spec security groups",
+			spec: infrav1.OpenStackMachineSpec{
+				SecurityGroups: []infrav1.SecurityGroupParam{
+					{Filter: &infrav1.SecurityGroupFilter{Name: "machine-security-group"}},
+				},
+				Ports: []infrav1.PortOpts{
+					{
+						ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
+							DisablePortSecurity: ptr.To(true),
+						},
+					},
+				},
+			},
+			expectNetwork: func(m *mock.MockNetworkClientMockRecorder) {
+				m.ListSecGroup(groups.ListOpts{Name: "machine-security-group"}).Return([]groups.SecGroup{
+					{ID: securityGroupID1},
+				}, nil)
+			},
+			want: []infrav1.ResolvedPortSpec{
+				{
+					Name:      "test-instance-0",
+					NetworkID: defaultNetworkID,
+					FixedIPs: []infrav1.ResolvedFixedIP{
+						{SubnetID: ptr.To(defaultSubnetID)},
+					},
+					Description: defaultDescription,
+					Tags:        []string{"test-tag"},
+					ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
+						DisablePortSecurity: ptr.To(true),
+					},
+				},
+			},
+		},
 	}
 	for i := range tests {
 		tt := &tests[i]

pkg/webhooks/openstackmachine_webhook.go

@@ -24,6 +24,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -62,6 +63,12 @@ func (*openStackMachineWebhook) ValidateCreate(_ context.Context, objRaw runtime
 		}
 	}
 
+	for _, port := range newObj.Spec.Ports {
+		if ptr.Deref(port.DisablePortSecurity, false) && len(port.SecurityGroups) > 0 {
+			allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "ports"), "cannot have security groups when DisablePortSecurity is set to true"))
+		}
+	}
+
 	return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs)
 }
 

pkg/webhooks/openstackserver_webhook.go

@@ -24,6 +24,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/utils/ptr"
 	"sigs.k8s.io/cluster-api/util/topology"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -64,6 +65,12 @@ func (*openStackServerWebhook) ValidateCreate(_ context.Context, objRaw runtime.
 		}
 	}
 
+	for _, port := range newObj.Spec.Ports {
+		if ptr.Deref(port.DisablePortSecurity, false) && len(port.SecurityGroups) > 0 {
+			allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "ports"), "cannot have security groups when DisablePortSecurity is set to true"))
+		}
+	}
+
 	return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs)
 }
 

test/e2e/suites/apivalidations/openstackmachine_test.go

@@ -172,6 +172,23 @@ var _ = Describe("OpenStackMachine API validations", func() {
 			Expect(k8sClient.Create(ctx, machine)).NotTo(Succeed(), "OpenStackMachine creation with root device name in spec.AdditionalBlockDevices should not succeed")
 		})
 
+		It("should not allow to create machine with both SecurityGroups and DisablePortSecurity", func() {
+			machine := defaultMachine()
+			machine.Spec.Ports = []infrav1.PortOpts{
+				{
+					SecurityGroups: []infrav1.SecurityGroupParam{{
+						Filter: &infrav1.SecurityGroupFilter{Name: "test-security-group"},
+					}},
+					ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
+						DisablePortSecurity: ptr.To(true),
+					},
+				},
+			}
+
+			By("Creating a machine with both SecurityGroups and DisablePortSecurity")
+			Expect(k8sClient.Create(ctx, machine)).NotTo(Succeed(), "OpenStackMachine creation with both SecurityGroups and DisablePortSecurity should not succeed")
+		})
+
 		/* FIXME: These tests are failing
 		It("should not allow additional volume with empty name", func() {
 			machine.Spec.AdditionalBlockDevices = []infrav1.AdditionalBlockDevice{

test/e2e/suites/apivalidations/openstackserver_test.go

@@ -150,6 +150,23 @@ var _ = Describe("OpenStackServer API validations", func() {
 			Expect(k8sClient.Create(ctx, server)).NotTo(Succeed(), "OpenStackserver creation with root device name in spec.AdditionalBlockDevices should not succeed")
 		})
 
+		It("should not allow to create server with both SecurityGroups and DisablePortSecurity", func() {
+			server := defaultServer()
+			server.Spec.Ports = []infrav1.PortOpts{
+				{
+					SecurityGroups: []infrav1.SecurityGroupParam{{
+						Filter: &infrav1.SecurityGroupFilter{Name: "test-security-group"},
+					}},
+					ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
+						DisablePortSecurity: ptr.To(true),
+					},
+				},
+			}
+
+			By("Creating a server with both SecurityGroups and DisablePortSecurity")
+			Expect(k8sClient.Create(ctx, server)).NotTo(Succeed(), "OpenStackServer creation with both SecurityGroups and DisablePortSecurity should not succeed")
+		})
+
 		/* FIXME: These tests are failing
 		It("should not allow additional volume with empty name", func() {
 			server.Spec.AdditionalBlockDevices = []infrav1.AdditionalBlockDevice{