diff --git a/controllers/openstackcluster_controller.go b/controllers/openstackcluster_controller.go index 3251a95b40..9ec63477bc 100644 --- a/controllers/openstackcluster_controller.go +++ b/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) } diff --git a/controllers/openstackcluster_controller_test.go b/controllers/openstackcluster_controller_test.go index 21dc0dbe3d..362794e13a 100644 --- a/controllers/openstackcluster_controller_test.go +++ b/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", diff --git a/controllers/openstackmachine_controller.go b/controllers/openstackmachine_controller.go index 8638b72f5a..81faaaaef7 100644 --- a/controllers/openstackmachine_controller.go +++ b/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) } diff --git a/pkg/cloud/services/networking/port.go b/pkg/cloud/services/networking/port.go index 0c86e7854b..310685c966 100644 --- a/pkg/cloud/services/networking/port.go +++ b/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 +} diff --git a/pkg/cloud/services/networking/port_test.go b/pkg/cloud/services/networking/port_test.go index bbdc1cd018..2cee4c84a3 100644 --- a/pkg/cloud/services/networking/port_test.go +++ b/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()) diff --git a/pkg/cloud/services/networking/service.go b/pkg/cloud/services/networking/service.go index 3a01c48b85..d1b206fde2 100644 --- a/pkg/cloud/services/networking/service.go +++ b/pkg/cloud/services/networking/service.go @@ -18,7 +18,6 @@ package networking import ( "fmt" - "sort" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/attributestags" "k8s.io/apimachinery/pkg/runtime" @@ -65,28 +64,15 @@ func (s *Service) replaceAllAttributesTags(eventObject runtime.Object, resourceT record.Warnf(eventObject, "FailedReplaceAllAttributesTags", "Invalid resourceType argument in function call") panic(fmt.Errorf("invalid argument: resourceType, %s, does not match allowed arguments: %s or %s", resourceType, trunkResource, portResource)) } - // 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) - } - - // Sort the tags so that we always get fixed order of tags to make UT easier - sort.Strings(uniqueTags) _, err := s.client.ReplaceAllAttributesTags(resourceType, resourceID, attributestags.ReplaceAllOpts{ - Tags: uniqueTags, + Tags: tags, }) if err != nil { record.Warnf(eventObject, "FailedReplaceAllAttributesTags", "Failed to replace all attributestags, %s: %v", resourceID, err) return err } - record.Eventf(eventObject, "SuccessfulReplaceAllAttributeTags", "Replaced all attributestags for %s with tags %s", resourceID, uniqueTags) + record.Eventf(eventObject, "SuccessfulReplaceAllAttributeTags", "Replaced all attributestags for %s with tags %s", resourceID, tags) return nil } diff --git a/test/e2e/data/e2e_conf.yaml b/test/e2e/data/e2e_conf.yaml index f7aa4214e0..c57568a8d6 100644 --- a/test/e2e/data/e2e_conf.yaml +++ b/test/e2e/data/e2e_conf.yaml @@ -1,4 +1,3 @@ ---- # E2E test scenario using local dev images and manifests built from the source tree for following providers: # - cluster-api # - bootstrap kubeadm