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