Merge pull request #2356 from Nordix/lentzi90/ensure-ports-tags-relea…

…se-0.10

🐛 Ensure that existing ports also have correct tags and trunks

controllers/openstackcluster_controller.go

@@ -616,11 +616,7 @@ func getOrCreateBastionPorts(openStackCluster *infrav1.OpenStackCluster, network
 		return errors.New("bastion resources are nil")
 	}
 
-	if len(desiredPorts) == len(resources.Ports) {
-		return nil
-	}
-
-	err := networkingService.CreatePorts(openStackCluster, desiredPorts, resources)
+	err := networkingService.EnsurePorts(openStackCluster, desiredPorts, resources)
 	if err != nil {
 		return fmt.Errorf("failed to create ports for bastion %s: %w", bastionName(openStackCluster.Name), err)
 	}

controllers/openstackcluster_controller_test.go

@@ -277,6 +277,8 @@ var _ = Describe("OpenStackCluster controller", func() {
 		server.Status = "ACTIVE"
 
 		networkClientRecorder := mockScopeFactory.NetworkClient.EXPECT()
+		// One list for adopting and one for ensuring the ports and tags are correct
+		networkClientRecorder.ListPort(gomock.Any()).Return([]ports.Port{{ID: "portID1"}}, nil)
 		networkClientRecorder.ListPort(gomock.Any()).Return([]ports.Port{{ID: "portID1"}}, nil)
 
 		computeClientRecorder := mockScopeFactory.ComputeClient.EXPECT()
@@ -362,6 +364,7 @@ var _ = Describe("OpenStackCluster controller", func() {
 
 		networkClientRecorder := mockScopeFactory.NetworkClient.EXPECT()
 		networkClientRecorder.ListPort(gomock.Any()).Return([]ports.Port{{ID: "portID1"}}, nil)
+		networkClientRecorder.ListPort(gomock.Any()).Return([]ports.Port{{ID: "portID1"}}, nil)
 
 		computeClientRecorder := mockScopeFactory.ComputeClient.EXPECT()
 		computeClientRecorder.GetServer("adopted-fip-bastion-uuid").Return(&server, nil)
@@ -445,6 +448,9 @@ var _ = Describe("OpenStackCluster controller", func() {
 		computeClientRecorder := mockScopeFactory.ComputeClient.EXPECT()
 		computeClientRecorder.GetServer("requeue-bastion-uuid").Return(&server, nil)
 
+		networkClientRecorder := mockScopeFactory.NetworkClient.EXPECT()
+		networkClientRecorder.ListPort(gomock.Any()).Return([]ports.Port{{ID: "portID1"}}, nil)
+
 		res, err := reconcileBastion(scope, capiCluster, testCluster)
 		Expect(testCluster.Status.Bastion).To(Equal(&infrav1.BastionStatus{
 			ID:    "requeue-bastion-uuid",

controllers/openstackmachine_controller.go

@@ -752,11 +752,7 @@ func getOrCreateMachinePorts(openStackMachine *infrav1.OpenStackMachine, network
 	}
 	desiredPorts := resolved.Ports
 
-	if len(desiredPorts) == len(resources.Ports) {
-		return nil
-	}
-
-	if err := networkingService.CreatePorts(openStackMachine, desiredPorts, resources); err != nil {
+	if err := networkingService.EnsurePorts(openStackMachine, desiredPorts, resources); err != nil {
 		return fmt.Errorf("creating ports: %w", err)
 	}
 

pkg/cloud/services/networking/port.go

@@ -123,7 +123,61 @@ func (s *Service) GetPortForExternalNetwork(instanceID string, externalNetworkID
 	return nil, nil
 }
 
-func (s *Service) CreatePort(eventObject runtime.Object, portSpec *infrav1.ResolvedPortSpec) (*ports.Port, error) {
+// ensurePortTagsAndTrunk ensures that the provided port has the tags and trunk defined in portSpec.
+func (s *Service) ensurePortTagsAndTrunk(port *ports.Port, eventObject runtime.Object, portSpec *infrav1.ResolvedPortSpec) error {
+	wantedTags := uniqueSortedTags(portSpec.Tags)
+	actualTags := uniqueSortedTags(port.Tags)
+	// Only replace tags if there is a difference
+	if !slices.Equal(wantedTags, actualTags) && len(wantedTags) > 0 {
+		if err := s.replaceAllAttributesTags(eventObject, portResource, port.ID, wantedTags); err != nil {
+			record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace port tags %s: %v", port.Name, err)
+			return err
+		}
+	}
+	if ptr.Deref(portSpec.Trunk, false) {
+		trunk, err := s.getOrCreateTrunkForPort(eventObject, port)
+		if err != nil {
+			record.Warnf(eventObject, "FailedCreateTrunk", "Failed to create trunk for port %s: %v", port.Name, err)
+			return err
+		}
+
+		if !slices.Equal(wantedTags, trunk.Tags) {
+			if err = s.replaceAllAttributesTags(eventObject, trunkResource, trunk.ID, wantedTags); err != nil {
+				record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace trunk tags %s: %v", port.Name, err)
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+// EnsurePort ensure that a port defined with portSpec Name and NetworkID exists,
+// and that the port has suitable tags and trunk. If the PortStatus is already known,
+// use the ID when filtering for existing ports.
+func (s *Service) EnsurePort(eventObject runtime.Object, portSpec *infrav1.ResolvedPortSpec, portStatus infrav1.PortStatus) (*ports.Port, error) {
+	opts := ports.ListOpts{
+		Name:      portSpec.Name,
+		NetworkID: portSpec.NetworkID,
+	}
+	if portStatus.ID != "" {
+		opts.ID = portStatus.ID
+	}
+
+	existingPorts, err := s.client.ListPort(opts)
+	if err != nil {
+		return nil, fmt.Errorf("searching for existing port for server: %v", err)
+	}
+	if len(existingPorts) > 1 {
+		return nil, fmt.Errorf("multiple ports found with name \"%s\"", portSpec.Name)
+	}
+
+	if len(existingPorts) == 1 {
+		port := &existingPorts[0]
+		if err = s.ensurePortTagsAndTrunk(port, eventObject, portSpec); err != nil {
+			return nil, err
+		}
+		return port, nil
+	}
 	var addressPairs []ports.AddressPair
 	if !ptr.Deref(portSpec.DisablePortSecurity, false) {
 		for _, ap := range portSpec.AllowedAddressPairs {
@@ -196,24 +250,10 @@ func (s *Service) CreatePort(eventObject runtime.Object, portSpec *infrav1.Resol
 		return nil, err
 	}
 
-	if len(portSpec.Tags) > 0 {
-		if err = s.replaceAllAttributesTags(eventObject, portResource, port.ID, portSpec.Tags); err != nil {
-			record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace port tags %s: %v", portSpec.Name, err)
-			return nil, err
-		}
+	if err = s.ensurePortTagsAndTrunk(port, eventObject, portSpec); err != nil {
+		return nil, err
 	}
 	record.Eventf(eventObject, "SuccessfulCreatePort", "Created port %s with id %s", port.Name, port.ID)
-	if ptr.Deref(portSpec.Trunk, false) {
-		trunk, err := s.getOrCreateTrunkForPort(eventObject, port)
-		if err != nil {
-			record.Warnf(eventObject, "FailedCreateTrunk", "Failed to create trunk for port %s: %v", port.Name, err)
-			return nil, err
-		}
-		if err = s.replaceAllAttributesTags(eventObject, trunkResource, trunk.ID, portSpec.Tags); err != nil {
-			record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace trunk tags %s: %v", port.Name, err)
-			return nil, err
-		}
-	}
 
 	return port, nil
 }
@@ -324,23 +364,30 @@ func getPortName(baseName string, portSpec *infrav1.PortOpts, netIndex int) stri
 	return fmt.Sprintf("%s-%d", baseName, netIndex)
 }
 
-func (s *Service) CreatePorts(eventObject runtime.Object, desiredPorts []infrav1.ResolvedPortSpec, resources *infrav1.MachineResources) error {
+// EnsurePorts ensures that every one of desiredPorts is created and has
+// expected trunk and tags.
+func (s *Service) EnsurePorts(eventObject runtime.Object, desiredPorts []infrav1.ResolvedPortSpec, resources *infrav1.MachineResources) error {
 	for i := range desiredPorts {
-		// Skip creation of ports which already exist
+		// If we already created the port, make use of the status
+		portStatus := infrav1.PortStatus{}
 		if i < len(resources.Ports) {
-			continue
+			portStatus = resources.Ports[i]
 		}
-
-		portSpec := &desiredPorts[i]
-		// Events are recorded in CreatePort
-		port, err := s.CreatePort(eventObject, portSpec)
+		// Events are recorded in EnsurePort
+		port, err := s.EnsurePort(eventObject, &desiredPorts[i], portStatus)
 		if err != nil {
 			return err
 		}
 
-		resources.Ports = append(resources.Ports, infrav1.PortStatus{
-			ID: port.ID,
-		})
+		// If we already have the status, replace it,
+		// otherwise append it.
+		if i < len(resources.Ports) {
+			resources.Ports[i] = portStatus
+		} else {
+			resources.Ports = append(resources.Ports, infrav1.PortStatus{
+				ID: port.ID,
+			})
+		}
 	}
 
 	return nil
@@ -609,3 +656,19 @@ func (s *Service) AdoptPorts(scope *scope.WithLogger, desiredPorts []infrav1.Res
 
 	return nil
 }
+
+// uniqueSortedTags returns a new, sorted slice where any duplicates have been removed.
+func uniqueSortedTags(tags []string) []string {
+	// remove duplicate values from tags
+	tagsMap := make(map[string]string)
+	for _, t := range tags {
+		tagsMap[t] = t
+	}
+
+	uniqueTags := []string{}
+	for k := range tagsMap {
+		uniqueTags = append(uniqueTags, k)
+	}
+	slices.Sort(uniqueTags)
+	return uniqueTags
+}

pkg/cloud/services/networking/port_test.go

@@ -40,7 +40,7 @@ import (
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/scope"
 )
 
-func Test_CreatePort(t *testing.T) {
+func Test_EnsurePort(t *testing.T) {
 	// Arbitrary values used in the tests
 	const (
 		netID               = "7fd24ceb-788a-441f-ad0a-d8e2f5d31a1d"
@@ -59,8 +59,8 @@ func Test_CreatePort(t *testing.T) {
 		name   string
 		port   infrav1.ResolvedPortSpec
 		expect func(m *mock.MockNetworkClientMockRecorder, g Gomega)
-		// Note the 'wanted' port isn't so important, since it will be whatever we tell ListPort or CreatePort to return.
-		// Mostly in this test suite, we're checking that CreatePort is called with the expected port opts.
+		// Note the 'wanted' port isn't so important, since it will be whatever we tell ListPort or EnsurePort to return.
+		// Mostly in this test suite, we're checking that EnsurePort is called with the expected port opts.
 		want    *ports.Port
 		wantErr bool
 	}{
@@ -156,6 +156,10 @@ func Test_CreatePort(t *testing.T) {
 					},
 				}
 
+				m.ListPort(ports.ListOpts{
+					Name:      "foo-port-1",
+					NetworkID: netID,
+				}).Return(nil, nil)
 				// The following allows us to use gomega to
 				// compare the argument instead of gomock.
 				// Gomock's output in the case of a mismatch is
@@ -183,6 +187,10 @@ func Test_CreatePort(t *testing.T) {
 				expectedCreateOpts = portsbinding.CreateOptsExt{
 					CreateOptsBuilder: expectedCreateOpts,
 				}
+				m.ListPort(ports.ListOpts{
+					Name:      "test-port",
+					NetworkID: netID,
+				}).Return(nil, nil)
 				m.CreatePort(gomock.Any()).DoAndReturn(func(builder ports.CreateOptsBuilder) (*ports.Port, error) {
 					gotCreateOpts := builder.(portsbinding.CreateOptsExt)
 					g.Expect(gotCreateOpts).To(Equal(expectedCreateOpts), cmp.Diff(gotCreateOpts, expectedCreateOpts))
@@ -219,6 +227,10 @@ func Test_CreatePort(t *testing.T) {
 				expectedCreateOpts = portsbinding.CreateOptsExt{
 					CreateOptsBuilder: expectedCreateOpts,
 				}
+				m.ListPort(ports.ListOpts{
+					Name:      "test-port",
+					NetworkID: netID,
+				}).Return(nil, nil)
 				m.CreatePort(gomock.Any()).DoAndReturn(func(builder ports.CreateOptsBuilder) (*ports.Port, error) {
 					gotCreateOpts := builder.(portsbinding.CreateOptsExt)
 					g.Expect(gotCreateOpts).To(Equal(expectedCreateOpts), cmp.Diff(gotCreateOpts, expectedCreateOpts))
@@ -261,6 +273,10 @@ func Test_CreatePort(t *testing.T) {
 				expectedCreateOpts = portsbinding.CreateOptsExt{
 					CreateOptsBuilder: expectedCreateOpts,
 				}
+				m.ListPort(ports.ListOpts{
+					Name:      "test-port",
+					NetworkID: netID,
+				}).Return(nil, nil)
 				m.CreatePort(gomock.Any()).DoAndReturn(func(builder ports.CreateOptsBuilder) (*ports.Port, error) {
 					gotCreateOpts := builder.(portsbinding.CreateOptsExt)
 					g.Expect(gotCreateOpts).To(Equal(expectedCreateOpts), cmp.Diff(gotCreateOpts, expectedCreateOpts))
@@ -270,7 +286,7 @@ func Test_CreatePort(t *testing.T) {
 			want: &ports.Port{ID: portID},
 		},
 		{
-			name: "tags and trunk",
+			name: "create port with tags and trunk",
 			port: infrav1.ResolvedPortSpec{
 				Name:      "test-port",
 				NetworkID: netID,
@@ -287,6 +303,10 @@ func Test_CreatePort(t *testing.T) {
 					CreateOptsBuilder: expectedCreateOpts,
 				}
 
+				m.ListPort(ports.ListOpts{
+					Name:      "test-port",
+					NetworkID: netID,
+				}).Return(nil, nil)
 				// Create the port
 				m.CreatePort(gomock.Any()).DoAndReturn(func(builder ports.CreateOptsBuilder) (*ports.Port, error) {
 					gotCreateOpts := builder.(portsbinding.CreateOptsExt)
@@ -318,6 +338,87 @@ func Test_CreatePort(t *testing.T) {
 			},
 			want: &ports.Port{ID: portID, Name: "test-port"},
 		},
+		{
+			name: "port with tags and trunk already exists",
+			port: infrav1.ResolvedPortSpec{
+				Name:      "test-port",
+				NetworkID: netID,
+				Tags:      []string{"tag1", "tag2"},
+				Trunk:     ptr.To(true),
+			},
+			expect: func(m *mock.MockNetworkClientMockRecorder, _ types.Gomega) {
+				m.ListPort(ports.ListOpts{
+					Name:      "test-port",
+					NetworkID: netID,
+				}).Return([]ports.Port{{
+					ID:        portID,
+					Name:      "test-port",
+					NetworkID: netID,
+					Tags:      []string{"tag1", "tag2"},
+				}}, nil)
+
+				// Look for existing trunk
+				m.ListTrunk(trunks.ListOpts{
+					PortID: portID,
+					Name:   "test-port",
+				}).Return([]trunks.Trunk{{
+					ID:   trunkID,
+					Tags: []string{"tag1", "tag2"},
+				}}, nil)
+			},
+			want: &ports.Port{
+				ID:        portID,
+				Name:      "test-port",
+				NetworkID: netID,
+				Tags:      []string{"tag1", "tag2"},
+			},
+		},
+		{
+			name: "partial port missing tags and trunk",
+			port: infrav1.ResolvedPortSpec{
+				Name:      "test-port",
+				NetworkID: netID,
+				Tags:      []string{"tag1", "tag2"},
+				Trunk:     ptr.To(true),
+			},
+			expect: func(m *mock.MockNetworkClientMockRecorder, _ types.Gomega) {
+				m.ListPort(ports.ListOpts{
+					Name:      "test-port",
+					NetworkID: netID,
+				}).Return([]ports.Port{{
+					ID:        portID,
+					Name:      "test-port",
+					NetworkID: netID,
+				}}, nil)
+
+				// Tag the port
+				m.ReplaceAllAttributesTags("ports", portID, attributestags.ReplaceAllOpts{
+					Tags: []string{"tag1", "tag2"},
+				})
+
+				// Look for existing trunk
+				m.ListTrunk(trunks.ListOpts{
+					PortID: portID,
+					Name:   "test-port",
+				}).Return([]trunks.Trunk{}, nil)
+
+				// Create the trunk
+				m.CreateTrunk(trunks.CreateOpts{
+					PortID: portID,
+					Name:   "test-port",
+				}).Return(&trunks.Trunk{ID: trunkID}, nil)
+
+				// Tag the trunk
+				m.ReplaceAllAttributesTags("trunks", trunkID, attributestags.ReplaceAllOpts{
+					Tags: []string{"tag1", "tag2"},
+				})
+			},
+			want: &ports.Port{
+				ID:        portID,
+				Name:      "test-port",
+				NetworkID: netID,
+			},
+		},
 	}
 
 	eventObject := &infrav1.OpenStackMachine{}
@@ -333,9 +434,10 @@ func Test_CreatePort(t *testing.T) {
 			s := Service{
 				client: mockClient,
 			}
-			got, err := s.CreatePort(
+			got, err := s.EnsurePort(
 				eventObject,
 				&tt.port,
+				infrav1.PortStatus{},
 			)
 			if tt.wantErr {
 				g.Expect(err).To(HaveOccurred())