From 6e59991a6f3c41499a80b9d757802418457b62cc Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Wed, 29 Mar 2023 14:21:40 +0100 Subject: [PATCH] Infer port network from subnet NetworkID is a required field when creating a neutron port. We previously passed on this requirement in the Ports API. However we didn't have this restriction in the Networks API and inferred the network from a subnet if one was defined. This change eases the transition from Networks to Ports by removing this restriction for Ports. --- .../src/clusteropenstack/configuration.md | 74 +++- go.mod | 2 +- pkg/cloud/services/compute/instance.go | 192 +++++++--- pkg/cloud/services/compute/instance_test.go | 362 ++++++++++++++++++ 4 files changed, 579 insertions(+), 51 deletions(-) diff --git a/docs/book/src/clusteropenstack/configuration.md b/docs/book/src/clusteropenstack/configuration.md index e00ad78951..6cad4659d1 100644 --- a/docs/book/src/clusteropenstack/configuration.md +++ b/docs/book/src/clusteropenstack/configuration.md @@ -358,9 +358,6 @@ spec: ports: - network: id: - nameSuffix: - description: - vnicType: normal fixedIPs: - subnet: id: @@ -370,6 +367,9 @@ spec: tags: - tag1 - tag2 + nameSuffix: + description: + vnicType: normal securityGroups: - profile: @@ -379,7 +379,70 @@ spec: Any such ports are created in addition to ports used for connections to networks or subnets. -Also, `port security` can be applied to specific port to enable/disable the `port security` on that port; When not set, it takes the value of the corresponding field at the network level. +### Port network and IP addresses + +Together, `network` and `fixedIPs` define the network a port will be created on, and the addresses which will be assigned to the port on that network. + +`network` is a filter which uniquely describes the Neutron network the port will be created be on. Machine creation will fail if the result is empty or not unique. If a network `id` is specified in the filter then no separate OpenStack query is required. This has the advantages of being both faster and unambiguous in all circumstances, so it is the preferred way to specify a network where possible. + +The available fields are described in [the CRD](https://doc.crds.dev/github.com/kubernetes-sigs/cluster-api-provider-openstack/infrastructure.cluster.x-k8s.io/OpenStackMachine/v1alpha6@v0.7.1#spec-ports-network). + +If `network` is not specified at all, it may be possible to infer the network from any uniquely defined subnets in `fixedIPs`. As this may result in additional OpenStack queries and the potential for ambiguity is greater, this is not recommended. + +`fixedIPs` describes a list of addresses from the target `network` which will be allocated to the port. A `fixedIP` is either a specific `ipAddress`, a `subnet` from which an ip address will be allocated, or both. If only `ipAddress` is specified, it must be valid in at least one of the subnets defined in the current network. If both are defined, `ipAddress` must be valid in the specified subnet. + +`subnet` is a filter which uniquely describe the Neutron subnet an address will be allocated from. Its operation is analogous to `network`, described above. + +`fixedIPs`, including all fields available in the `subnet` filter, are described in [the CRD](https://doc.crds.dev/github.com/kubernetes-sigs/cluster-api-provider-openstack/infrastructure.cluster.x-k8s.io/OpenStackMachine/v1alpha6@v0.7.1#spec-ports-fixedIPs). + +If no `fixedIPs` are specified, the port will get an address from every subnet in the network. + +#### Examples + +A single explicit network with a single explicit subnet. +```yaml +ports: +- tags: + - control-plane + network: + id: 0686143b-f0a7-481a-86f5-cc1f8ccde692 + fixedIPs: + - subnet: + id: a5e50a9c-58f9-4b6f-b8ee-2e7b4e4414ee +``` + +No network or fixed IPs: the port will be created on the cluster default network, and will get a single address from the cluster default subnet. +```yaml +ports: +- tags: + - control-plane +``` + +Network and subnet are specified by filter. They will be looked up. Note that this is not as efficient or reliable as specifying the network by `id`. +```yaml +ports: +- tags: + - storage + network: + name: storage-network + fixedIPs: + - subnet: + name: storage-subnet +``` + +No network, but a fixed IP with a subnet. The network will be inferred from the network of the subnet. Note that this is not as efficient or reliable as specifying the network explicitly. +```yaml +ports: +- tags: + - control-plane + fixedIPs: + - subnet: + id: a5e50a9c-58f9-4b6f-b8ee-2e7b4e4414ee +``` + +### Port Security + +`port security` can be applied to specific port to enable/disable the `port security` on that port; When not set, it takes the value of the corresponding field at the network level. ```yaml apiVersion: infrastructure.cluster.x-k8s.io/v1alpha7 @@ -389,7 +452,8 @@ metadata: namespace: spec: ports: - - networkId: + - network: + id: ... disablePortSecurity: true ... diff --git a/go.mod b/go.mod index 33aadb278a..d5dda0ee15 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ require ( github.com/davecgh/go-spew v1.1.1 github.com/go-logr/logr v1.2.4 github.com/golang/mock v1.6.0 + github.com/google/go-cmp v0.5.9 github.com/google/gofuzz v1.2.0 github.com/gophercloud/gophercloud v1.3.0 github.com/gophercloud/utils v0.0.0-20221207145018-e8fba78967ca @@ -68,7 +69,6 @@ require ( github.com/golang/protobuf v1.5.3 // indirect github.com/google/cel-go v0.14.0 // indirect github.com/google/gnostic v0.6.9 // indirect - github.com/google/go-cmp v0.5.9 // indirect github.com/google/go-github/v48 v48.2.0 // indirect github.com/google/go-querystring v1.1.0 // indirect github.com/google/pprof v0.0.0-20210407192527-94a9f03dee38 // indirect diff --git a/pkg/cloud/services/compute/instance.go b/pkg/cloud/services/compute/instance.go index 24662b3bfc..13b6ff2d21 100644 --- a/pkg/cloud/services/compute/instance.go +++ b/pkg/cloud/services/compute/instance.go @@ -17,6 +17,7 @@ limitations under the License. package compute import ( + "errors" "fmt" "os" "strconv" @@ -35,6 +36,7 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7" "sigs.k8s.io/cluster-api-provider-openstack/pkg/clients" + "sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/networking" "sigs.k8s.io/cluster-api-provider-openstack/pkg/record" capoerrors "sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/errors" "sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/hash" @@ -46,58 +48,149 @@ const ( timeoutInstanceDelete = 5 * time.Minute ) -// constructNetworks builds an array of networks from the network, subnet and ports items in the instance spec. -// If no networks or ports are in the spec, returns a single network item for a network connection to the default cluster network. -func (s *Service) constructNetworks(openStackCluster *infrav1.OpenStackCluster, instanceSpec *InstanceSpec) ([]infrav1.Network, error) { - trunkRequired := false +// normalizePortTarget ensures that the port has a network ID. +func (s *Service) normalizePortTarget(port *infrav1.PortOpts, openStackCluster *infrav1.OpenStackCluster, portIdx int) error { + // Treat no Network and empty Network the same + noNetwork := port.Network == nil || (*port.Network == infrav1.NetworkFilter{}) - nets, err := s.getServerNetworks(instanceSpec.Networks) - if err != nil { - return nil, err - } - - for i := range instanceSpec.Ports { - port := &instanceSpec.Ports[i] - // No Trunk field specified for the port, inherit openStackMachine.Spec.Trunk. - if port.Trunk == nil { - port.Trunk = &instanceSpec.Trunk + // No network or subnets defined: use cluster defaults + if noNetwork && len(port.FixedIPs) == 0 { + port.Network = &infrav1.NetworkFilter{ + ID: openStackCluster.Status.Network.ID, } - if *port.Trunk { - trunkRequired = true + port.FixedIPs = []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + ID: openStackCluster.Status.Network.Subnet.ID, + }, + }, } - if port.Network != nil { - netID := port.Network.ID - if netID == "" { - networkingService, err := s.getNetworkingService() - if err != nil { - return nil, err + + return nil + } + + // No network, but fixed IPs are defined(we handled the no fixed + // IPs case above): try to infer network from a subnet + if noNetwork { + s.scope.Logger().V(4).Info("No network defined for port %d, attempting to infer from subnet", portIdx) + + // Look for a unique subnet defined in FixedIPs. If we find one + // we can use it to infer the network ID. We don't need to worry + // here about the case where different FixedIPs have different + // networks because that will cause an error later when we try + // to create the port. + networkID, err := func() (string, error) { + networkingService, err := s.getNetworkingService() + if err != nil { + return "", err + } + + for i, fixedIP := range port.FixedIPs { + if fixedIP.Subnet == nil { + continue } - netIDs, err := networkingService.GetNetworkIDsByFilter(port.Network.ToListOpt()) + subnet, err := networkingService.GetSubnetByFilter(fixedIP.Subnet) if err != nil { - return nil, err - } - if len(netIDs) > 1 { - return nil, fmt.Errorf("network filter for port %s returns more than one result", port.NameSuffix) - } else if len(netIDs) == 0 { - return nil, fmt.Errorf("network filter for port %s returns no networks", port.NameSuffix) + // Multiple matches might be ok later when we restrict matches to a single network + if errors.Is(err, networking.ErrMultipleMatches) { + s.scope.Logger().V(4).Info("Can't infer network from subnet %d: %s", i, err) + continue + } + + return "", err } - netID = netIDs[0] + + // Cache the subnet ID in the FixedIP + fixedIP.Subnet.ID = subnet.ID + return subnet.NetworkID, nil } - nets = append(nets, infrav1.Network{ - ID: netID, - Subnet: &infrav1.Subnet{}, - PortOpts: port, - }) - } else { - nets = append(nets, infrav1.Network{ - ID: openStackCluster.Status.Network.ID, - Subnet: &infrav1.Subnet{ - ID: openStackCluster.Status.Network.Subnet.ID, - }, - PortOpts: port, - }) + + // TODO: This is a spec error: it should set the machine to failed + return "", fmt.Errorf("port %d has no network and unable to infer from fixed IPs", portIdx) + }() + if err != nil { + return err } + + port.Network = &infrav1.NetworkFilter{ + ID: networkID, + } + + return nil + } + + // Nothing to do if network ID is already set + if port.Network.ID != "" { + return nil + } + + // Network is defined by Filter + networkingService, err := s.getNetworkingService() + if err != nil { + return err + } + + netIDs, err := networkingService.GetNetworkIDsByFilter(port.Network.ToListOpt()) + if err != nil { + return err + } + + // TODO: These are spec errors: they should set the machine to failed + if len(netIDs) > 1 { + return fmt.Errorf("network filter for port %d returns more than one result", portIdx) + } else if len(netIDs) == 0 { + return fmt.Errorf("network filter for port %d returns no networks", portIdx) + } + + port.Network.ID = netIDs[0] + + return nil +} + +// normalizePorts ensures that a user-specified PortOpts has all required fields set. Specifically it: +// - sets the Trunk field to the instance spec default if not specified +// - sets the Network ID field if not specified. +func (s *Service) normalizePorts(ports []infrav1.PortOpts, openStackCluster *infrav1.OpenStackCluster, instanceSpec *InstanceSpec) ([]infrav1.PortOpts, error) { + normalizedPorts := make([]infrav1.PortOpts, 0, len(ports)) + for i := range ports { + // Deep copy the port to avoid mutating the original + port := ports[i].DeepCopy() + + // No Trunk field specified for the port, inherit the machine default + if port.Trunk == nil { + port.Trunk = &instanceSpec.Trunk + } + + if err := s.normalizePortTarget(port, openStackCluster, i); err != nil { + return nil, err + } + + normalizedPorts = append(normalizedPorts, *port) + } + return normalizedPorts, nil +} + +// constructNetworks builds an array of networks from the network, subnet and ports items in the instance spec. +// If no networks or ports are in the spec, returns a single network item for a network connection to the default cluster network. +func (s *Service) constructNetworks(openStackCluster *infrav1.OpenStackCluster, instanceSpec *InstanceSpec) ([]infrav1.Network, error) { + nets, err := s.getServerNetworks(instanceSpec.Networks) + if err != nil { + return nil, err + } + + // Ensure user-specified ports have all required fields + ports, err := s.normalizePorts(instanceSpec.Ports, openStackCluster, instanceSpec) + if err != nil { + return nil, err + } + for i := range ports { + port := &ports[i] + nets = append(nets, infrav1.Network{ + ID: port.Network.ID, + Subnet: &infrav1.Subnet{}, + PortOpts: port, + }) } // no networks or ports found in the spec, so create a port on the cluster network @@ -111,10 +204,19 @@ func (s *Service) constructNetworks(openStackCluster *infrav1.OpenStackCluster, Trunk: &instanceSpec.Trunk, }, }} - trunkRequired = instanceSpec.Trunk } - if trunkRequired { + // trunk support is required if any port has trunk enabled + portUsesTrunk := func() bool { + for _, net := range nets { + port := net.PortOpts + if port != nil && port.Trunk != nil && *port.Trunk { + return true + } + } + return false + } + if portUsesTrunk() { trunkSupported, err := s.isTrunkExtSupported() if err != nil { return nil, err diff --git a/pkg/cloud/services/compute/instance_test.go b/pkg/cloud/services/compute/instance_test.go index c4b3f196c0..aa765180fb 100644 --- a/pkg/cloud/services/compute/instance_test.go +++ b/pkg/cloud/services/compute/instance_test.go @@ -24,6 +24,7 @@ import ( "github.com/go-logr/logr" "github.com/golang/mock/gomock" + "github.com/google/go-cmp/cmp" "github.com/gophercloud/gophercloud" "github.com/gophercloud/gophercloud/openstack/blockstorage/v3/volumes" common "github.com/gophercloud/gophercloud/openstack/common/extensions" @@ -1116,3 +1117,364 @@ func TestService_DeleteInstance(t *testing.T) { }) } } + +func TestService_normalizePorts(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + const ( + defaultNetworkID = "3c66f3ca-2d26-4d9d-ae3b-568f54129773" + defaultSubnetID = "d8dbba89-8c39-4192-a571-e702fca35bac" + + networkID = "afa54944-1443-4132-9ef5-ce37eb4d6ab6" + subnetID = "d786e715-c299-4a97-911d-640c10fc0392" + ) + + openStackCluster := &infrav1.OpenStackCluster{ + Status: infrav1.OpenStackClusterStatus{ + Network: &infrav1.Network{ + ID: defaultNetworkID, + Subnet: &infrav1.Subnet{ + ID: defaultSubnetID, + }, + }, + }, + } + + tests := []struct { + name string + ports []infrav1.PortOpts + instanceTrunk bool + expectNetwork func(m *mock.MockNetworkClientMockRecorder) + want []infrav1.PortOpts + wantErr bool + }{ + { + name: "No ports: no ports", + ports: []infrav1.PortOpts{}, + want: []infrav1.PortOpts{}, + }, + { + name: "Nil network, no fixed IPs: cluster defaults", + ports: []infrav1.PortOpts{ + { + Network: nil, + FixedIPs: nil, + }, + }, + want: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{ + ID: defaultNetworkID, + }, + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + ID: defaultSubnetID, + }, + }, + }, + Trunk: pointer.BoolPtr(false), + }, + }, + }, + { + name: "Empty network, no fixed IPs: cluster defaults", + ports: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{}, + FixedIPs: nil, + }, + }, + want: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{ + ID: defaultNetworkID, + }, + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + ID: defaultSubnetID, + }, + }, + }, + Trunk: pointer.BoolPtr(false), + }, + }, + }, + { + name: "Port inherits trunk from instance", + ports: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{}, + FixedIPs: nil, + }, + }, + instanceTrunk: true, + want: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{ + ID: defaultNetworkID, + }, + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + ID: defaultSubnetID, + }, + }, + }, + Trunk: pointer.BoolPtr(true), + }, + }, + }, + { + name: "Port overrides trunk from instance", + ports: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{}, + FixedIPs: nil, + Trunk: pointer.BoolPtr(true), + }, + }, + want: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{ + ID: defaultNetworkID, + }, + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + ID: defaultSubnetID, + }, + }, + }, + Trunk: pointer.BoolPtr(true), + }, + }, + }, + { + name: "Network defined by ID: unchanged", + ports: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{ + ID: networkID, + }, + }, + }, + want: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{ + ID: networkID, + }, + Trunk: pointer.BoolPtr(false), + }, + }, + }, + { + name: "Network defined by filter: add ID from network lookup", + ports: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{ + Name: "test-network", + }, + }, + }, + expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { + m.ListNetwork(networks.ListOpts{Name: "test-network"}).Return([]networks.Network{ + {ID: networkID}, + }, nil) + }, + want: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{ + ID: networkID, + Name: "test-network", + }, + Trunk: pointer.BoolPtr(false), + }, + }, + }, + { + name: "No network, fixed IP has subnet by ID: add ID from subnet", + ports: []infrav1.PortOpts{ + { + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + ID: subnetID, + }, + }, + }, + }, + }, + expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { + m.GetSubnet(subnetID).Return(&subnets.Subnet{ID: subnetID, NetworkID: networkID}, nil) + }, + want: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{ + ID: networkID, + }, + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + ID: subnetID, + }, + }, + }, + Trunk: pointer.BoolPtr(false), + }, + }, + }, + { + name: "No network, fixed IP has subnet by filter: add ID from subnet", + ports: []infrav1.PortOpts{ + { + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + Name: "test-subnet", + }, + }, + }, + }, + }, + expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { + m.ListSubnet(subnets.ListOpts{Name: "test-subnet"}).Return([]subnets.Subnet{ + {ID: subnetID, NetworkID: networkID}, + }, nil) + }, + want: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{ + ID: networkID, + }, + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + ID: subnetID, + Name: "test-subnet", + }, + }, + }, + Trunk: pointer.BoolPtr(false), + }, + }, + }, + { + name: "No network, fixed IP subnet returns no matches: error", + ports: []infrav1.PortOpts{ + { + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + Name: "test-subnet", + }, + }, + }, + }, + }, + expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { + m.ListSubnet(subnets.ListOpts{Name: "test-subnet"}).Return([]subnets.Subnet{}, nil) + }, + wantErr: true, + }, + { + name: "No network, only fixed IP subnet returns multiple matches: error", + ports: []infrav1.PortOpts{ + { + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + Name: "test-subnet", + }, + }, + }, + }, + }, + expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { + m.ListSubnet(subnets.ListOpts{Name: "test-subnet"}).Return([]subnets.Subnet{ + {ID: subnetID, NetworkID: networkID}, + {ID: "8008494c-301e-4e5c-951b-a8ab568447fd", NetworkID: "5d48bfda-db28-42ee-8374-50e13d1fe5ea"}, + }, nil) + }, + wantErr: true, + }, + { + name: "No network, first fixed IP subnet returns multiple matches: used ID from second fixed IP", + ports: []infrav1.PortOpts{ + { + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + Name: "test-subnet1", + }, + }, + { + Subnet: &infrav1.SubnetFilter{ + Name: "test-subnet2", + }, + }, + }, + }, + }, + expectNetwork: func(m *mock.MockNetworkClientMockRecorder) { + m.ListSubnet(subnets.ListOpts{Name: "test-subnet1"}).Return([]subnets.Subnet{ + {ID: subnetID, NetworkID: networkID}, + {ID: "8008494c-301e-4e5c-951b-a8ab568447fd", NetworkID: "5d48bfda-db28-42ee-8374-50e13d1fe5ea"}, + }, nil) + m.ListSubnet(subnets.ListOpts{Name: "test-subnet2"}).Return([]subnets.Subnet{ + {ID: subnetID, NetworkID: networkID}, + }, nil) + }, + want: []infrav1.PortOpts{ + { + Network: &infrav1.NetworkFilter{ + ID: networkID, + }, + FixedIPs: []infrav1.FixedIP{ + { + Subnet: &infrav1.SubnetFilter{ + Name: "test-subnet1", + }, + }, + { + Subnet: &infrav1.SubnetFilter{ + ID: subnetID, + Name: "test-subnet2", + }, + }, + }, + Trunk: pointer.BoolPtr(false), + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + // MockScopeFactory also implements Scope so no need to create separate Scope from it. + mockScope := scope.NewMockScopeFactory(mockCtrl, "", logr.Discard()) + if tt.expectNetwork != nil { + tt.expectNetwork(mockScope.NetworkClient.EXPECT()) + } + + s := &Service{ + scope: mockScope, + } + instanceSpec := &InstanceSpec{ + Trunk: tt.instanceTrunk, + } + + got, err := s.normalizePorts(tt.ports, openStackCluster, instanceSpec) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + return + } + + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(got).To(Equal(tt.want), cmp.Diff(got, tt.want)) + }) + } +}