Reuse common code in CreateBastion and CreateInstance

Common network and security group handling between CreateBastion() and
CreateInstance().

A principal advantage of this refactor is that it makes the marshalling
of OpenStackMachineSpec and Instance respectively into an InstanceSpec a
cheap operation which makes no API calls.

Author: mdbooth

pkg/cloud/services/compute/bastion.go

@@ -34,31 +34,15 @@ func (s *Service) CreateBastion(openStackCluster *infrav1.OpenStackCluster, clus
 		RootVolume:    openStackCluster.Spec.Bastion.Instance.RootVolume,
 	}
 
-	securityGroups, err := s.networkingService.GetSecurityGroups(openStackCluster.Spec.Bastion.Instance.SecurityGroups)
-	if err != nil {
-		return nil, err
-	}
+	instanceSpec.SecurityGroups = openStackCluster.Spec.Bastion.Instance.SecurityGroups
 	if openStackCluster.Spec.ManagedSecurityGroups {
-		securityGroups = append(securityGroups, openStackCluster.Status.BastionSecurityGroup.ID)
+		instanceSpec.SecurityGroups = append(instanceSpec.SecurityGroups, infrav1.SecurityGroupParam{
+			UUID: openStackCluster.Status.BastionSecurityGroup.ID,
+		})
 	}
-	instanceSpec.SecurityGroups = securityGroups
 
-	var nets []infrav1.Network
-	if len(openStackCluster.Spec.Bastion.Instance.Networks) > 0 {
-		var err error
-		nets, err = s.getServerNetworks(openStackCluster.Spec.Bastion.Instance.Networks)
-		if err != nil {
-			return nil, err
-		}
-	} else {
-		nets = []infrav1.Network{{
-			ID: openStackCluster.Status.Network.ID,
-			Subnet: &infrav1.Subnet{
-				ID: openStackCluster.Status.Network.Subnet.ID,
-			},
-		}}
-	}
-	instanceSpec.Networks = nets
+	instanceSpec.Networks = openStackCluster.Spec.Bastion.Instance.Networks
+	instanceSpec.Ports = openStackCluster.Spec.Bastion.Instance.Ports
 
-	return s.createInstance(openStackCluster, clusterName, instanceSpec, retryIntervalInstanceStatus)
+	return s.createInstance(openStackCluster, openStackCluster, clusterName, instanceSpec, retryIntervalInstanceStatus)
 }

pkg/cloud/services/compute/instance.go

@@ -70,19 +70,9 @@ func (s *Service) createInstanceImpl(openStackCluster *infrav1.OpenStackCluster,
 		RootVolume:    openStackMachine.Spec.RootVolume,
 		Subnet:        openStackMachine.Spec.Subnet,
 		ServerGroupID: openStackMachine.Spec.ServerGroupID,
+		Trunk:         openStackMachine.Spec.Trunk,
 	}
 
-	// verify that trunk is supported if set at instance level.
-	if openStackMachine.Spec.Trunk {
-		trunkSupported, err := s.isTrunkExtSupported()
-		if err != nil {
-			return nil, err
-		}
-		if !trunkSupported {
-			return nil, fmt.Errorf("there is no trunk support. please ensure that the trunk extension is enabled in your OpenStack deployment")
-		}
-		instanceSpec.Trunk = true
-	}
 	machineTags := []string{}
 
 	// Append machine specific tags
@@ -96,56 +86,44 @@ func (s *Service) createInstanceImpl(openStackCluster *infrav1.OpenStackCluster,
 
 	instanceSpec.Tags = machineTags
 
-	// Get security groups
-	securityGroups, err := s.networkingService.GetSecurityGroups(openStackMachine.Spec.SecurityGroups)
-	if err != nil {
-		return nil, err
-	}
+	instanceSpec.SecurityGroups = openStackMachine.Spec.SecurityGroups
 	if openStackCluster.Spec.ManagedSecurityGroups {
+		var managedSecurityGroup string
 		if util.IsControlPlaneMachine(machine) {
-			securityGroups = append(securityGroups, openStackCluster.Status.ControlPlaneSecurityGroup.ID)
+			managedSecurityGroup = openStackCluster.Status.ControlPlaneSecurityGroup.ID
 		} else {
-			securityGroups = append(securityGroups, openStackCluster.Status.WorkerSecurityGroup.ID)
+			managedSecurityGroup = openStackCluster.Status.WorkerSecurityGroup.ID
 		}
-	}
-	instanceSpec.SecurityGroups = securityGroups
 
-	nets, err := s.constructNetworks(openStackCluster, openStackMachine)
-	if err != nil {
-		return nil, err
+		instanceSpec.SecurityGroups = append(instanceSpec.SecurityGroups, infrav1.SecurityGroupParam{
+			UUID: managedSecurityGroup,
+		})
 	}
 
-	trunkConfigured := s.isTrunkConfigured(nets, instanceSpec.Trunk)
-	if trunkConfigured {
-		trunkSupported, err := s.isTrunkExtSupported()
-		if err != nil {
-			return nil, err
-		}
-		if !trunkSupported {
-			return nil, fmt.Errorf("there is no trunk support. please ensure that the trunk extension is enabled in your OpenStack deployment")
-		}
-	}
-	instanceSpec.Networks = nets
+	instanceSpec.Networks = openStackMachine.Spec.Networks
+	instanceSpec.Ports = openStackMachine.Spec.Ports
 
-	return s.createInstance(openStackMachine, clusterName, &instanceSpec, retryInterval)
+	return s.createInstance(openStackMachine, openStackCluster, clusterName, &instanceSpec, retryInterval)
 }
 
-// constructNetworks builds an array of networks from the network, subnet and ports items in the machine spec.
+// 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, openStackMachine *infrav1.OpenStackMachine) ([]infrav1.Network, error) {
-	var nets []infrav1.Network
-	if len(openStackMachine.Spec.Networks) > 0 {
-		var err error
-		nets, err = s.getServerNetworks(openStackMachine.Spec.Networks)
-		if err != nil {
-			return nil, err
-		}
+func (s *Service) constructNetworks(openStackCluster *infrav1.OpenStackCluster, instanceSpec *InstanceSpec) ([]infrav1.Network, error) {
+	trunkRequired := false
+
+	nets, err := s.getServerNetworks(instanceSpec.Networks)
+	if err != nil {
+		return nil, err
 	}
-	for i, port := range openStackMachine.Spec.Ports {
-		pOpts := &openStackMachine.Spec.Ports[i]
+
+	for i := range instanceSpec.Ports {
+		port := &instanceSpec.Ports[i]
 		// No Trunk field specified for the port, inherit openStackMachine.Spec.Trunk.
-		if pOpts.Trunk == nil {
-			pOpts.Trunk = &openStackMachine.Spec.Trunk
+		if port.Trunk == nil {
+			port.Trunk = &instanceSpec.Trunk
+		}
+		if *port.Trunk {
+			trunkRequired = true
 		}
 		if port.Network != nil {
 			netID := port.Network.ID
@@ -164,18 +142,19 @@ func (s *Service) constructNetworks(openStackCluster *infrav1.OpenStackCluster,
 			nets = append(nets, infrav1.Network{
 				ID:       netID,
 				Subnet:   &infrav1.Subnet{},
-				PortOpts: pOpts,
+				PortOpts: port,
 			})
 		} else {
 			nets = append(nets, infrav1.Network{
 				ID: openStackCluster.Status.Network.ID,
 				Subnet: &infrav1.Subnet{
 					ID: openStackCluster.Status.Network.Subnet.ID,
 				},
-				PortOpts: pOpts,
+				PortOpts: port,
 			})
 		}
 	}
+
 	// no networks or ports found in the spec, so create a port on the cluster network
 	if len(nets) == 0 {
 		nets = []infrav1.Network{{
@@ -184,14 +163,26 @@ func (s *Service) constructNetworks(openStackCluster *infrav1.OpenStackCluster,
 				ID: openStackCluster.Status.Network.Subnet.ID,
 			},
 			PortOpts: &infrav1.PortOpts{
-				Trunk: &openStackMachine.Spec.Trunk,
+				Trunk: &instanceSpec.Trunk,
 			},
 		}}
+		trunkRequired = instanceSpec.Trunk
+	}
+
+	if trunkRequired {
+		trunkSupported, err := s.isTrunkExtSupported()
+		if err != nil {
+			return nil, err
+		}
+		if !trunkSupported {
+			return nil, fmt.Errorf("there is no trunk support. please ensure that the trunk extension is enabled in your OpenStack deployment")
+		}
 	}
+
 	return nets, nil
 }
 
-func (s *Service) createInstance(eventObject runtime.Object, clusterName string, instanceSpec *InstanceSpec, retryInterval time.Duration) (*InstanceStatus, error) {
+func (s *Service) createInstance(eventObject runtime.Object, openStackCluster *infrav1.OpenStackCluster, clusterName string, instanceSpec *InstanceSpec, retryInterval time.Duration) (*InstanceStatus, error) {
 	var server *ServerExt
 	accessIPv4 := ""
 	portList := []servers.Network{}
@@ -221,7 +212,17 @@ func (s *Service) createInstance(eventObject runtime.Object, clusterName string,
 		}
 	}()
 
-	for i, network := range instanceSpec.Networks {
+	nets, err := s.constructNetworks(openStackCluster, instanceSpec)
+	if err != nil {
+		return nil, err
+	}
+
+	securityGroups, err := s.networkingService.GetSecurityGroups(instanceSpec.SecurityGroups)
+	if err != nil {
+		return nil, fmt.Errorf("error getting security groups: %v", err)
+	}
+
+	for i, network := range nets {
 		if network.ID == "" {
 			return nil, fmt.Errorf("no network was found or provided. Please check your machine configuration and try again")
 		}
@@ -230,7 +231,7 @@ func (s *Service) createInstance(eventObject runtime.Object, clusterName string,
 			iTags = instanceSpec.Tags
 		}
 		portName := getPortName(instanceSpec.Name, network.PortOpts, i)
-		port, err := s.networkingService.GetOrCreatePort(eventObject, clusterName, portName, network, &instanceSpec.SecurityGroups, iTags)
+		port, err := s.networkingService.GetOrCreatePort(eventObject, clusterName, portName, network, &securityGroups, iTags)
 		if err != nil {
 			return nil, err
 		}
@@ -253,7 +254,7 @@ func (s *Service) createInstance(eventObject runtime.Object, clusterName string,
 		AvailabilityZone: instanceSpec.FailureDomain,
 		Networks:         portList,
 		UserData:         []byte(instanceSpec.UserData),
-		SecurityGroups:   instanceSpec.SecurityGroups,
+		SecurityGroups:   securityGroups,
 		Tags:             instanceSpec.Tags,
 		Metadata:         instanceSpec.Metadata,
 		ConfigDrive:      &instanceSpec.ConfigDrive,
@@ -791,19 +792,3 @@ func (s *Service) isTrunkExtSupported() (trunknSupported bool, err error) {
 	}
 	return true, nil
 }
-
-// isTrunkConfigured verifies trunk configuration at instance and port levels, useful for avoiding multple api calls to verify trunk support.
-func (s *Service) isTrunkConfigured(nets []infrav1.Network, instanceLevelTrunk bool) bool {
-	if instanceLevelTrunk {
-		return true
-	}
-	for _, net := range nets {
-		port := net.PortOpts
-		if port != nil {
-			if port.Trunk != nil && *port.Trunk {
-				return true
-			}
-		}
-	}
-	return false
-}

pkg/cloud/services/compute/instance_types.go

@@ -46,8 +46,9 @@ type InstanceSpec struct {
 	ServerGroupID  string
 	Trunk          bool
 	Tags           []string
-	SecurityGroups []string
-	Networks       []infrav1.Network
+	SecurityGroups []infrav1.SecurityGroupParam
+	Networks       []infrav1.NetworkParam
+	Ports          []infrav1.PortOpts
 }
 
 // InstanceIdentifier describes an instance which has not necessarily been fetched.