Reconcile SecurityGroups <-> Instances separately

Originally, we were adding the SecurityGroups to the instanceSpec.
Now, the openstackmachine controller will first create the instances
without security groups. Then we have a loop that will reconcile the
desired security groups that we want to apply in the cluster[1] with the
ones that are added to the instance's ports.

[1] for now, we have 2 sets of rules: the one that can be added to
  `OpenStackMachine.Spec.SecurityGroups` and the one managed
  by `OpenStackCluster.Spec.ManagedSecurityGroups`.

Author: EmilienM

api/v1alpha5/conversion.go

@@ -456,3 +456,14 @@ func Convert_v1alpha7_Bastion_To_v1alpha5_Bastion(in *infrav1.Bastion, out *Bast
 	in.FloatingIP = out.Instance.FloatingIP
 	return nil
 }
+
+func Convert_v1alpha7_OpenStackMachineStatus_To_v1alpha5_OpenStackMachineStatus(in *infrav1.OpenStackMachineStatus, out *OpenStackMachineStatus, s conversion.Scope) error {
+	err := autoConvert_v1alpha7_OpenStackMachineStatus_To_v1alpha5_OpenStackMachineStatus(in, out, s)
+	if err != nil {
+		return err
+	}
+
+	in.SecurityGroups = nil
+
+	return nil
+}

api/v1alpha5/zz_generated.conversion.go

@@ -682,3 +682,14 @@ func Convert_v1alpha7_Bastion_To_v1alpha6_Bastion(in *infrav1.Bastion, out *Bast
 	in.FloatingIP = out.Instance.FloatingIP
 	return nil
 }
+
+func Convert_v1alpha7_OpenStackMachineStatus_To_v1alpha6_OpenStackMachineStatus(in *infrav1.OpenStackMachineStatus, out *OpenStackMachineStatus, s apiconversion.Scope) error {
+	err := autoConvert_v1alpha7_OpenStackMachineStatus_To_v1alpha6_OpenStackMachineStatus(in, out, s)
+	if err != nil {
+		return err
+	}
+
+	in.SecurityGroups = nil
+
+	return nil
+}

api/v1alpha6/conversion.go

@@ -42,6 +42,8 @@ const (
 	InstanceDeleteFailedReason = "InstanceDeleteFailed"
 	// OpenstackErrorReason used when there is an error communicating with OpenStack.
 	OpenStackErrorReason = "OpenStackError"
+	// SecurityGroupAppliedFailedCondition used when the security group could not be applied.
+	SecurityGroupApplyFailedReason = "SecurityGroupAppliedFailed"
 )
 
 const (

api/v1alpha6/zz_generated.conversion.go

@@ -107,6 +107,11 @@ type OpenStackMachineStatus struct {
 	// +optional
 	InstanceState *InstanceState `json:"instanceState,omitempty"`
 
+	// SecurityGroups is the list of security groups that were applied to the ports
+	// attached to the instance.
+	// +optional
+	SecurityGroups []SecurityGroup `json:"securityGroups,omitempty"`
+
 	FailureReason *errors.MachineStatusError `json:"failureReason,omitempty"`
 
 	// FailureMessage will be set in the event that there is a terminal problem

api/v1alpha7/conditions_consts.go

@@ -1601,6 +1601,61 @@ spec:
               ready:
                 description: Ready is true when the provider resource is ready.
                 type: boolean
+              securityGroups:
+                description: SecurityGroups is the list of security groups that were
+                  applied to the ports attached to the instance.
+                items:
+                  description: SecurityGroup represents the basic information of the
+                    associated OpenStack Neutron Security Group.
+                  properties:
+                    id:
+                      type: string
+                    name:
+                      type: string
+                    rules:
+                      items:
+                        description: SecurityGroupRule represent the basic information
+                          of the associated OpenStack Security Group Role.
+                        properties:
+                          description:
+                            type: string
+                          direction:
+                            type: string
+                          etherType:
+                            type: string
+                          name:
+                            type: string
+                          portRangeMax:
+                            type: integer
+                          portRangeMin:
+                            type: integer
+                          protocol:
+                            type: string
+                          remoteGroupID:
+                            type: string
+                          remoteIPPrefix:
+                            type: string
+                          securityGroupID:
+                            type: string
+                        required:
+                        - description
+                        - direction
+                        - etherType
+                        - name
+                        - portRangeMax
+                        - portRangeMin
+                        - protocol
+                        - remoteGroupID
+                        - remoteIPPrefix
+                        - securityGroupID
+                        type: object
+                      type: array
+                  required:
+                  - id
+                  - name
+                  - rules
+                  type: object
+                type: array
             type: object
         type: object
     served: true

api/v1alpha7/openstackmachine_types.go

@@ -334,6 +334,12 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 		return ctrl.Result{}, err
 	}
 
+	err = r.reconcileSecurityGroupsToInstance(openStackCluster, machine, openStackMachine, instanceStatus, computeService, networkingService)
+	if err != nil {
+		conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.SecurityGroupApplyFailedReason, clusterv1.ConditionSeverityError, "Applying security groups failed: %v", err)
+		return ctrl.Result{}, fmt.Errorf("add security groups to instance: %w", err)
+	}
+
 	// TODO(sbueringer) From CAPA: TODO(ncdc): move this validation logic into a validating webhook (for us: create validation logic in webhook)
 
 	openStackMachine.Spec.ProviderID = pointer.String(fmt.Sprintf("openstack:///%s", instanceStatus.ID()))
@@ -430,6 +436,53 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 	return ctrl.Result{}, nil
 }
 
+func (r *OpenStackMachineReconciler) reconcileSecurityGroupsToInstance(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, instanceStatus *compute.InstanceStatus, computeService *compute.Service, networkingService *networking.Service) error {
+	desiredSecurityGroups := []*infrav1.SecurityGroup{}
+
+	// These security groups are currently not managed by CAPO and provided by the user
+	// Later we'll have an API to manage them.
+	for _, sg := range openStackMachine.Spec.SecurityGroups {
+		if sg.ID == "" {
+			// lookup security group by name
+			securityGroup, err := networkingService.GetSecurityGroupByName(sg.Name)
+			if err != nil {
+				return fmt.Errorf("get security group %q: %w", sg.Name, err)
+			}
+			sg.ID = securityGroup.ID
+		}
+		desiredSecurityGroups = append(desiredSecurityGroups, &infrav1.SecurityGroup{
+			ID: sg.ID,
+		})
+	}
+
+	if openStackCluster.Spec.ManagedSecurityGroups {
+		if util.IsControlPlaneMachine(machine) && openStackCluster.Status.ControlPlaneSecurityGroup != nil {
+			desiredSecurityGroups = append(desiredSecurityGroups, openStackCluster.Status.ControlPlaneSecurityGroup)
+		} else if openStackCluster.Status.WorkerSecurityGroup != nil {
+			desiredSecurityGroups = append(desiredSecurityGroups, openStackCluster.Status.WorkerSecurityGroup)
+		}
+	}
+
+	if len(desiredSecurityGroups) > 0 {
+		instanceID := instanceStatus.ID()
+		desiredSecurityGroupsIDs := make([]string, len(desiredSecurityGroups))
+		for i, sg := range desiredSecurityGroups {
+			desiredSecurityGroupsIDs[i] = sg.ID
+		}
+		// Security Groups are attached to ports, not instances so we need to get the ports first
+		attachedInterfaces, err := computeService.GetAttachedInterfaces(instanceID)
+		if err != nil {
+			return fmt.Errorf("get attached interfaces for instance %q: %w", instanceStatus.Name(), err)
+		}
+		err = networkingService.ReconcileSecurityGroupsToInstanceAttachedInterfaces(openStackMachine, attachedInterfaces, desiredSecurityGroupsIDs)
+		if err != nil {
+			return fmt.Errorf("reconcile security groups %q to instance %q: %w", desiredSecurityGroupsIDs, instanceStatus.Name(), err)
+		}
+	}
+
+	return nil
+}
+
 func (r *OpenStackMachineReconciler) getOrCreate(logger logr.Logger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, computeService *compute.Service, userData string) (*compute.InstanceStatus, error) {
 	instanceStatus, err := computeService.GetInstanceStatusByName(openStackMachine, openStackMachine.Name)
 	if err != nil {
@@ -496,20 +549,6 @@ func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine *
 
 	instanceSpec.Tags = machineTags
 
-	instanceSpec.SecurityGroups = openStackMachine.Spec.SecurityGroups
-	if openStackCluster.Spec.ManagedSecurityGroups {
-		var managedSecurityGroup string
-		if util.IsControlPlaneMachine(machine) && openStackCluster.Status.ControlPlaneSecurityGroup != nil {
-			managedSecurityGroup = openStackCluster.Status.ControlPlaneSecurityGroup.ID
-		} else if openStackCluster.Status.WorkerSecurityGroup != nil {
-			managedSecurityGroup = openStackCluster.Status.WorkerSecurityGroup.ID
-		}
-
-		instanceSpec.SecurityGroups = append(instanceSpec.SecurityGroups, infrav1.SecurityGroupFilter{
-			ID: managedSecurityGroup,
-		})
-	}
-
 	instanceSpec.Ports = openStackMachine.Spec.Ports
 
 	return &instanceSpec

api/v1alpha7/zz_generated.deepcopy.go

@@ -26,15 +26,14 @@ import (
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7"
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/compute"
+	"sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/networking"
 )
 
 const (
-	networkUUID                   = "d412171b-9fd7-41c1-95a6-c24e5953974d"
-	subnetUUID                    = "d2d8d98d-b234-477e-a547-868b7cb5d6a5"
-	extraSecurityGroupUUID        = "514bb2d8-3390-4a3b-86a7-7864ba57b329"
-	controlPlaneSecurityGroupUUID = "c9817a91-4821-42db-8367-2301002ab659"
-	workerSecurityGroupUUID       = "9c6c0d28-03c9-436c-815d-58440ac2c1c8"
-	serverGroupUUID               = "7b940d62-68ef-4e42-a76a-1a62e290509c"
+	networkUUID            = "d412171b-9fd7-41c1-95a6-c24e5953974d"
+	subnetUUID             = "d2d8d98d-b234-477e-a547-868b7cb5d6a5"
+	extraSecurityGroupUUID = "514bb2d8-3390-4a3b-86a7-7864ba57b329"
+	serverGroupUUID        = "7b940d62-68ef-4e42-a76a-1a62e290509c"
 
 	openStackMachineName = "test-openstack-machine"
 	namespace            = "test-namespace"
@@ -56,8 +55,6 @@ func getDefaultOpenStackCluster() *infrav1.OpenStackCluster {
 					{ID: subnetUUID},
 				},
 			},
-			ControlPlaneSecurityGroup: &infrav1.SecurityGroup{ID: controlPlaneSecurityGroupUUID},
-			WorkerSecurityGroup:       &infrav1.SecurityGroup{ID: workerSecurityGroupUUID},
 		},
 	}
 }
@@ -129,64 +126,6 @@ func Test_machineToInstanceSpec(t *testing.T) {
 			openStackMachine: getDefaultOpenStackMachine,
 			wantInstanceSpec: getDefaultInstanceSpec,
 		},
-		{
-			name: "Control plane security group",
-			openStackCluster: func() *infrav1.OpenStackCluster {
-				c := getDefaultOpenStackCluster()
-				c.Spec.ManagedSecurityGroups = true
-				return c
-			},
-			machine: func() *clusterv1.Machine {
-				m := getDefaultMachine()
-				m.Labels = map[string]string{
-					clusterv1.MachineControlPlaneLabel: "true",
-				}
-				return m
-			},
-			openStackMachine: getDefaultOpenStackMachine,
-			wantInstanceSpec: func() *compute.InstanceSpec {
-				i := getDefaultInstanceSpec()
-				i.SecurityGroups = []infrav1.SecurityGroupFilter{{ID: controlPlaneSecurityGroupUUID}}
-				return i
-			},
-		},
-		{
-			name: "Worker security group",
-			openStackCluster: func() *infrav1.OpenStackCluster {
-				c := getDefaultOpenStackCluster()
-				c.Spec.ManagedSecurityGroups = true
-				return c
-			},
-			machine:          getDefaultMachine,
-			openStackMachine: getDefaultOpenStackMachine,
-			wantInstanceSpec: func() *compute.InstanceSpec {
-				i := getDefaultInstanceSpec()
-				i.SecurityGroups = []infrav1.SecurityGroupFilter{{ID: workerSecurityGroupUUID}}
-				return i
-			},
-		},
-		{
-			name: "Extra security group",
-			openStackCluster: func() *infrav1.OpenStackCluster {
-				c := getDefaultOpenStackCluster()
-				c.Spec.ManagedSecurityGroups = true
-				return c
-			},
-			machine: getDefaultMachine,
-			openStackMachine: func() *infrav1.OpenStackMachine {
-				m := getDefaultOpenStackMachine()
-				m.Spec.SecurityGroups = []infrav1.SecurityGroupFilter{{ID: extraSecurityGroupUUID}}
-				return m
-			},
-			wantInstanceSpec: func() *compute.InstanceSpec {
-				i := getDefaultInstanceSpec()
-				i.SecurityGroups = []infrav1.SecurityGroupFilter{
-					{ID: extraSecurityGroupUUID},
-					{ID: workerSecurityGroupUUID},
-				}
-				return i
-			},
-		},
 		{
 			name: "Tags",
 			openStackCluster: func() *infrav1.OpenStackCluster {
@@ -214,3 +153,43 @@ func Test_machineToInstanceSpec(t *testing.T) {
 		})
 	}
 }
+
+func Test_reconcileSecurityGroupsToInstance(t *testing.T) {
+	RegisterTestingT(t)
+
+	openStackCluster := getDefaultOpenStackCluster()
+	machine := getDefaultMachine()
+	openStackMachine := getDefaultOpenStackMachine()
+	instanceStatus := &compute.InstanceStatus{}
+
+	computeService := &compute.Service{}
+	networkingService := &networking.Service{}
+
+	tests := []struct {
+		name                   string
+		openStackMachine       *infrav1.OpenStackMachine
+		expectedError          bool
+		expectedSecurityGroups []infrav1.SecurityGroup
+	}{
+		{
+			name:                   "NoSecurityGroups",
+			openStackMachine:       openStackMachine,
+			expectedError:          false,
+			expectedSecurityGroups: nil,
+		},
+	}
+	// TODO(emilien) Add more tests
+
+	for _, tt := range tests {
+		r := &OpenStackMachineReconciler{}
+		t.Run(tt.name, func(t *testing.T) {
+			err := r.reconcileSecurityGroupsToInstance(openStackCluster, machine, tt.openStackMachine, instanceStatus, computeService, networkingService)
+			if tt.expectedError {
+				Expect(err).To(HaveOccurred())
+			} else {
+				Expect(err).NotTo(HaveOccurred())
+			}
+			Expect(tt.openStackMachine.Status.SecurityGroups).To(Equal(tt.expectedSecurityGroups))
+		})
+	}
+}