Skip to content

Commit 4d4162b

Browse files
EmilienMEmilienM
committed
Iteration 3
1 parent ad74d32 commit 4d4162b

File tree

5 files changed

+70
-36
lines changed

5 files changed

+70
-36
lines changed

‎controllers/openstackmachine_controller.go

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"errors"
2323
"fmt"
2424
"reflect"
25+
"slices"
2526
"time"
2627

2728
"github.com/go-logr/logr"
@@ -342,7 +343,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
342343
return ctrl.Result{}, err
343344
}
344345

345-
err = r.reconcileSecurityGroupsToInstance(scope.Logger(), openStackCluster, machine, openStackMachine, instanceStatus, computeService, networkingService)
346+
err = r.reconcileSecurityGroupsToInstance(scope.Logger(), openStackCluster, machine, openStackMachine, instanceStatus, networkingService)
346347
if err != nil {
347348
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.SecurityGroupApplyFailedReason, clusterv1.ConditionSeverityError, "Applying security groups failed: %v", err)
348349
return ctrl.Result{}, fmt.Errorf("add security groups to instance: %w", err)
@@ -444,6 +445,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
444445
return ctrl.Result{}, nil
445446
}
446447

448+
// normalize returns a new sorted slice with the unique values of the given slice.
447449
func normalize(set []string) []string {
448450
seen := make(map[string]struct{}, len(set))
449451
normalized := make([]string, 0, len(set))
@@ -453,10 +455,11 @@ func normalize(set []string) []string {
453455
normalized = append(normalized, s)
454456
}
455457
}
458+
slices.Sort(normalized)
456459
return normalized
457460
}
458461

459-
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 {
462+
func (r *OpenStackMachineReconciler) reconcileSecurityGroupsToInstance(logger logr.Logger, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, instanceStatus *compute.InstanceStatus, networkingService *networking.Service) error {
460463
logger.Info("Reconciling security groups to instance")
461464

462465
desiredSecurityGroupIDs := []string{}
@@ -471,13 +474,13 @@ func (r *OpenStackMachineReconciler) reconcileSecurityGroupsToInstance(logger lo
471474

472475
desiredSecurityGroupIDs = normalize(desiredSecurityGroupIDs)
473476
appliedSecurityGroupIDs := normalize(openStackMachine.Status.AppliedSecurityGroupIDs)
474-
if !reflect.DeepEqual(desiredSecurityGroupIDs, appliedSecurityGroupIDs) {
477+
if !slices.Equal(desiredSecurityGroupIDs, appliedSecurityGroupIDs) {
475478
instanceID := instanceStatus.ID()
476-
attachedInterfaces, err := computeService.GetAttachedInterfaces(instanceID)
479+
attachedPorts, err := networkingService.GetPortIDsFromInstanceID(instanceID)
477480
if err != nil {
478-
return fmt.Errorf("get attached interfaces for instance %q: %w", instanceStatus.Name(), err)
481+
return fmt.Errorf("get ports for instance %q: %w", instanceID, err)
479482
}
480-
err = networkingService.ReconcileSecurityGroupsToInstanceAttachedInterfaces(logger, openStackMachine, attachedInterfaces, desiredSecurityGroupIDs)
483+
err = networkingService.ReconcileSecurityGroupsToInstanceAttachedPorts(openStackMachine, attachedPorts, desiredSecurityGroupIDs)
481484
if err != nil {
482485
return fmt.Errorf("reconcile security groups %q to instance %q: %w", desiredSecurityGroupIDs, instanceStatus.Name(), err)
483486
}

‎controllers/openstackmachine_controller_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,6 @@ func Test_reconcileSecurityGroupsToInstance(t *testing.T) {
165165
instanceStatus := &compute.InstanceStatus{}
166166
logger := logr.Logger{}
167167

168-
computeService := &compute.Service{}
169168
networkingService := &networking.Service{}
170169

171170
tests := []struct {
@@ -186,7 +185,7 @@ func Test_reconcileSecurityGroupsToInstance(t *testing.T) {
186185
for _, tt := range tests {
187186
r := &OpenStackMachineReconciler{}
188187
t.Run(tt.name, func(t *testing.T) {
189-
err := r.reconcileSecurityGroupsToInstance(logger, openStackCluster, machine, tt.openStackMachine, instanceStatus, computeService, networkingService)
188+
err := r.reconcileSecurityGroupsToInstance(logger, openStackCluster, machine, tt.openStackMachine, instanceStatus, networkingService)
190189
if tt.expectedError {
191190
Expect(err).To(HaveOccurred())
192191
} else {

‎pkg/cloud/services/networking/port.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,3 +330,19 @@ func GetPortName(instanceName string, opts *infrav1.PortOpts, netIndex int) stri
330330
}
331331
return fmt.Sprintf("%s-%d", instanceName, netIndex)
332332
}
333+
334+
func (s *Service) GetPortIDsFromInstanceID(instanceID string) ([]string, error) {
335+
portList, err := s.client.ListPort(ports.ListOpts{
336+
DeviceID: instanceID,
337+
})
338+
if err != nil {
339+
return nil, err
340+
}
341+
342+
portIDs := make([]string, len(portList))
343+
for i, port := range portList {
344+
portIDs[i] = port.ID
345+
}
346+
347+
return portIDs, nil
348+
}

‎pkg/cloud/services/networking/port_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -618,3 +618,38 @@ func Test_GarbageCollectErrorInstancesPort(t *testing.T) {
618618
func pointerTo(b bool) *bool {
619619
return &b
620620
}
621+
func Test_GetPortIDsFromInstanceID(t *testing.T) {
622+
mockCtrl := gomock.NewController(t)
623+
defer mockCtrl.Finish()
624+
625+
instanceID := "12345"
626+
expectedPortIDs := []string{"port1", "port2", "port3"}
627+
628+
mockClient := mock.NewMockNetworkClient(mockCtrl)
629+
mockClient.EXPECT().ListPort(ports.ListOpts{
630+
DeviceID: instanceID,
631+
}).Return([]ports.Port{
632+
{ID: "port1"},
633+
{ID: "port2"},
634+
{ID: "port3"},
635+
}, nil)
636+
637+
service := &Service{
638+
client: mockClient,
639+
}
640+
641+
portIDs, err := service.GetPortIDsFromInstanceID(instanceID)
642+
if err != nil {
643+
t.Fatalf("unexpected error: %v", err)
644+
}
645+
646+
if len(portIDs) != len(expectedPortIDs) {
647+
t.Fatalf("unexpected number of port IDs, got: %d, want: %d", len(portIDs), len(expectedPortIDs))
648+
}
649+
650+
for i, portID := range portIDs {
651+
if portID != expectedPortIDs[i] {
652+
t.Errorf("unexpected port ID at index %d, got: %s, want: %s", i, portID, expectedPortIDs[i])
653+
}
654+
}
655+
}

‎pkg/cloud/services/networking/securitygroups.go

Lines changed: 9 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,8 @@ package networking
1818

1919
import (
2020
"fmt"
21+
"slices"
2122

22-
"github.com/go-logr/logr"
23-
"github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/attachinterfaces"
2423
"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/attributestags"
2524
"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/security/groups"
2625
"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/security/rules"
@@ -402,41 +401,23 @@ func (s *Service) createRule(r infrav1.SecurityGroupRule) (infrav1.SecurityGroup
402401
return convertOSSecGroupRuleToConfigSecGroupRule(*rule), nil
403402
}
404403

405-
func contains(list []string, element string) bool {
406-
for _, e := range list {
407-
if e == element {
408-
return true
409-
}
410-
}
411-
return false
412-
}
413-
414-
func (s *Service) ReconcileSecurityGroupsToInstanceAttachedInterfaces(logger logr.Logger, openStackMachine *infrav1.OpenStackMachine, attachedInterfaces []attachinterfaces.Interface, desiredSecurityGroupIDs []string) error {
415-
for _, iface := range attachedInterfaces {
404+
func (s *Service) ReconcileSecurityGroupsToInstanceAttachedPorts(openStackMachine *infrav1.OpenStackMachine, attachedPorts []string, desiredSecurityGroupIDs []string) error {
405+
for _, portID := range attachedPorts {
416406
portUpdateNeeded := false
417-
portID := iface.PortID
418407
port, err := s.client.GetPort(portID)
419408
if err != nil {
420409
return fmt.Errorf("error getting port %s: %v", portID, err)
421410
}
422411

423-
// check if port already has desired security groups
424-
for _, securityGroupID := range desiredSecurityGroupIDs {
425-
if !contains(port.SecurityGroups, securityGroupID) {
426-
logger.Info("Port doesn't have desired security group", "portID", portID, "securityGroupID", securityGroupID)
427-
portUpdateNeeded = true
428-
}
429-
}
430-
// check if port has security groups that are not desired
431-
for _, securityGroupID := range port.SecurityGroups {
432-
if !contains(desiredSecurityGroupIDs, securityGroupID) {
433-
logger.Info("Port has security group that is not desired", "portID", portID, "securityGroupID", securityGroupID)
434-
portUpdateNeeded = true
435-
}
412+
portSecurityGroupIDs := port.SecurityGroups
413+
slices.Sort(portSecurityGroupIDs)
414+
if !slices.Equal(portSecurityGroupIDs, desiredSecurityGroupIDs) {
415+
s.scope.Logger().V(5).Info("Port has different security groups than desired", "portID", portID, "securityGroups", portSecurityGroupIDs)
416+
portUpdateNeeded = true
436417
}
437418

438419
if portUpdateNeeded {
439-
logger.Info("Updating port", "portID", portID, "securityGroups", desiredSecurityGroupIDs)
420+
s.scope.Logger().V(3).Info("Updating port", "portID", portID, "securityGroups", desiredSecurityGroupIDs)
440421
portOpts := ports.UpdateOpts{
441422
SecurityGroups: &desiredSecurityGroupIDs,
442423
}

0 commit comments

Comments
 (0)