Iteration 2 after Matt's comments

Author: EmilienM

api/v1alpha5/conversion.go

@@ -463,7 +463,8 @@ func Convert_v1alpha7_OpenStackMachineStatus_To_v1alpha5_OpenStackMachineStatus(
 		return err
 	}
 
-	in.SecurityGroups = nil
+	in.AppliedSecurityGroupIDs = nil
+	in.MachineSecurityGroupIDs = nil
 
 	return nil
 }

api/v1alpha5/zz_generated.conversion.go

@@ -689,7 +689,8 @@ func Convert_v1alpha7_OpenStackMachineStatus_To_v1alpha6_OpenStackMachineStatus(
 		return err
 	}
 
-	in.SecurityGroups = nil
+	in.AppliedSecurityGroupIDs = nil
+	in.MachineSecurityGroupIDs = nil
 
 	return nil
 }

api/v1alpha6/conversion.go

@@ -107,10 +107,14 @@ 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.
+	// AppliedSecurityGroupIDs is a list of the security group IDs applied to the instance.
 	// +optional
-	SecurityGroups []SecurityGroup `json:"securityGroups,omitempty"`
+	AppliedSecurityGroupIDs []string `json:"appliedSecurityGroupIDs,omitempty"`
+
+	// MachineSecurityGroupIDs is a list of IDs of the security groups that have been
+	// defined in the OpenStackMachine.Spec.SecurityGroups field.
+	// +optional
+	MachineSecurityGroupIDs []string `json:"machineSecurityGroupIDs,omitempty"`
 
 	FailureReason *errors.MachineStatusError `json:"failureReason,omitempty"`
 

api/v1alpha6/zz_generated.conversion.go

@@ -1529,6 +1529,12 @@ spec:
                   - type
                   type: object
                 type: array
+              appliedSecurityGroupIDs:
+                description: AppliedSecurityGroupIDs is a list of the security group
+                  IDs applied to the instance.
+                items:
+                  type: string
+                type: array
               conditions:
                 description: Conditions provide observations of the operational state
                   of a Cluster API resource.
@@ -1598,64 +1604,16 @@ spec:
                 description: InstanceState is the state of the OpenStack instance
                   for this machine.
                 type: string
+              machineSecurityGroupIDs:
+                description: MachineSecurityGroupIDs is a list of IDs of the security
+                  groups that have been defined in the OpenStackMachine.Spec.SecurityGroups
+                  field.
+                items:
+                  type: string
+                type: array
               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

@@ -328,6 +328,14 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 		return ctrl.Result{}, err
 	}
 
+	if len(openStackMachine.Spec.SecurityGroups) != len(openStackMachine.Status.MachineSecurityGroupIDs) {
+		machineSecurityGroups, err := networkingService.GetSecurityGroups(openStackMachine.Spec.SecurityGroups)
+		if err != nil {
+			return ctrl.Result{}, fmt.Errorf("get security groups %q: %w", openStackMachine.Spec.SecurityGroups, err)
+		}
+		openStackMachine.Status.MachineSecurityGroupIDs = machineSecurityGroups
+	}
+
 	instanceStatus, err := r.getOrCreate(scope.Logger(), cluster, openStackCluster, machine, openStackMachine, computeService, userData)
 	if err != nil {
 		// Conditions set in getOrCreate
@@ -436,50 +444,46 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 	return ctrl.Result{}, nil
 }
 
+func normalize(set []string) []string {
+	seen := make(map[string]struct{}, len(set))
+	normalized := make([]string, 0, len(set))
+	for _, s := range set {
+		if _, ok := seen[s]; !ok {
+			seen[s] = struct{}{}
+			normalized = append(normalized, s)
+		}
+	}
+	return normalized
+}
+
 func (r *OpenStackMachineReconciler) reconcileSecurityGroupsToInstance(logger logr.Logger, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, instanceStatus *compute.InstanceStatus, computeService *compute.Service, networkingService *networking.Service) error {
 	logger.Info("Reconciling security groups to instance")
 
-	desiredSecurityGroups := []*infrav1.SecurityGroup{}
+	desiredSecurityGroupIDs := []string{}
 
-	// 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,
-		})
-	}
+	desiredSecurityGroupIDs = append(desiredSecurityGroupIDs, openStackMachine.Status.MachineSecurityGroupIDs...)
 
-	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 util.IsControlPlaneMachine(machine) && openStackCluster.Status.ControlPlaneSecurityGroup != nil {
+		desiredSecurityGroupIDs = append(desiredSecurityGroupIDs, openStackCluster.Status.ControlPlaneSecurityGroup.ID)
+	} else if openStackCluster.Status.WorkerSecurityGroup != nil {
+		desiredSecurityGroupIDs = append(desiredSecurityGroupIDs, openStackCluster.Status.WorkerSecurityGroup.ID)
 	}
 
-	if len(desiredSecurityGroups) > 0 {
+	desiredSecurityGroupIDs = normalize(desiredSecurityGroupIDs)
+	appliedSecurityGroupIDs := normalize(openStackMachine.Status.AppliedSecurityGroupIDs)
+	if !reflect.DeepEqual(desiredSecurityGroupIDs, appliedSecurityGroupIDs) {
 		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(logger, openStackMachine, attachedInterfaces, desiredSecurityGroupsIDs)
+		err = networkingService.ReconcileSecurityGroupsToInstanceAttachedInterfaces(logger, openStackMachine, attachedInterfaces, desiredSecurityGroupIDs)
 		if err != nil {
-			return fmt.Errorf("reconcile security groups %q to instance %q: %w", desiredSecurityGroupsIDs, instanceStatus.Name(), err)
+			return fmt.Errorf("reconcile security groups %q to instance %q: %w", desiredSecurityGroupIDs, instanceStatus.Name(), err)
 		}
+	} else {
+		logger.Info("No security groups to instance to reconcile")
+		return nil
 	}
 
 	logger.Info("Reconciled security groups to instance successfully")

api/v1alpha7/zz_generated.deepcopy.go

@@ -17,6 +17,7 @@ limitations under the License.
 package controllers
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/go-logr/logr"
@@ -171,7 +172,7 @@ func Test_reconcileSecurityGroupsToInstance(t *testing.T) {
 		name                   string
 		openStackMachine       *infrav1.OpenStackMachine
 		expectedError          bool
-		expectedSecurityGroups []infrav1.SecurityGroup
+		expectedSecurityGroups []string
 	}{
 		{
 			name:                   "NoSecurityGroups",
@@ -191,7 +192,40 @@ func Test_reconcileSecurityGroupsToInstance(t *testing.T) {
 			} else {
 				Expect(err).NotTo(HaveOccurred())
 			}
-			Expect(tt.openStackMachine.Status.SecurityGroups).To(Equal(tt.expectedSecurityGroups))
+			Expect(tt.openStackMachine.Status.AppliedSecurityGroupIDs).To(Equal(tt.expectedSecurityGroups))
+		})
+	}
+}
+
+func Test_normalize(t *testing.T) {
+	tests := []struct {
+		name     string
+		set      []string
+		expected []string
+	}{
+		{
+			name:     "EmptySet",
+			set:      []string{},
+			expected: []string{},
+		},
+		{
+			name:     "NoDuplicates",
+			set:      []string{"a", "b", "c"},
+			expected: []string{"a", "b", "c"},
+		},
+		{
+			name:     "WithDuplicates",
+			set:      []string{"a", "b", "a", "c", "b"},
+			expected: []string{"a", "b", "c"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := normalize(tt.set)
+			if !reflect.DeepEqual(got, tt.expected) {
+				t.Errorf("normalize() = %v, want %v", got, tt.expected)
+			}
 		})
 	}
 }

config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachines.yaml

@@ -425,15 +425,13 @@ func (s *Service) ReconcileSecurityGroupsToInstanceAttachedInterfaces(logger log
 			if !contains(port.SecurityGroups, securityGroupID) {
 				logger.Info("Port doesn't have desired security group", "portID", portID, "securityGroupID", securityGroupID)
 				portUpdateNeeded = true
-				break
 			}
 		}
 		// check if port has security groups that are not desired
 		for _, securityGroupID := range port.SecurityGroups {
 			if !contains(desiredSecurityGroupIDs, securityGroupID) {
 				logger.Info("Port has security group that is not desired", "portID", portID, "securityGroupID", securityGroupID)
 				portUpdateNeeded = true
-				break
 			}
 		}
 
@@ -449,16 +447,7 @@ func (s *Service) ReconcileSecurityGroupsToInstanceAttachedInterfaces(logger log
 		}
 	}
 
-	appliedSecurityGroups := make([]infrav1.SecurityGroup, len(desiredSecurityGroupIDs))
-	for i, securityGroupID := range desiredSecurityGroupIDs {
-		securityGroup, err := s.client.GetSecGroup(securityGroupID)
-		if err != nil {
-			return fmt.Errorf("error getting security group %s: %v", securityGroupID, err)
-		}
-		appliedSecurityGroups[i] = *convertOSSecGroupToConfigSecGroup(*securityGroup)
-	}
-	logger.Info("Updating openStackMachine.Status.SecurityGroups", "securityGroups", appliedSecurityGroups)
-	openStackMachine.Status.SecurityGroups = appliedSecurityGroups
+	openStackMachine.Status.AppliedSecurityGroupIDs = desiredSecurityGroupIDs
 
 	return nil
 }