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.

Author: mdbooth

docs/book/src/clusteropenstack/configuration.md

@@ -358,9 +358,6 @@ spec:
   ports:
   - network:
       id: <your-network-id>
-    nameSuffix: <your-port-name>
-    description: <your-custom-port-description>
-    vnicType: normal
     fixedIPs:
     - subnet:
         id: <your-subnet-id>
@@ -370,6 +367,9 @@ spec:
         tags:
           - tag1
           - tag2
+    nameSuffix: <your-port-name>
+    description: <your-custom-port-description>
+    vnicType: normal
     securityGroups:
     - <your-security-group-id>
     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/[email protected]#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/[email protected]#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: <cluster-name>
 spec:
   ports:
-  - networkId: <your-network-id>
+  - network:
+      id: <your-network-id>
     ...
     disablePortSecurity: true
     ...

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

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