From 68e7b13c236f5af71e997f078511720888ce9f4e Mon Sep 17 00:00:00 2001 From: Lennart Jern Date: Fri, 24 Jan 2025 11:05:01 +0000 Subject: [PATCH] Ensure that existing ports also have correct tags and trunks If port creation fails in the middle, and cleanup also fails, then we may end up with ports with missing tags or trunks. This could happen when hitting rate-limits for example or if there is a network outage. This commit addresses the issue by going through existing ports and ensuring that they have correct tags and trunks. Co-authored-by: Huy Mai Signed-off-by: Lennart Jern --- controllers/openstackcluster_controller.go | 6 +- .../openstackcluster_controller_test.go | 6 + controllers/openstackmachine_controller.go | 6 +- pkg/cloud/services/networking/port.go | 117 ++++++++++++++---- pkg/cloud/services/networking/port_test.go | 112 ++++++++++++++++- pkg/cloud/services/networking/service.go | 18 +-- test/e2e/data/e2e_conf.yaml | 1 - 7 files changed, 207 insertions(+), 59 deletions(-) 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