✨Refactor CreateInstance and CreateBastion

controllers/openstackcluster_controller.go

@@ -222,8 +222,8 @@ func deleteBastion(scope *scope.Scope, cluster *clusterv1.Cluster, openStackClus
 		}
 	}
 
-	machineSpec := &openStackCluster.Spec.Bastion.Instance
-	if err = computeService.DeleteInstance(openStackCluster, machineSpec, instanceName, instanceStatus); err != nil {
+	instanceSpec := bastionToInstanceSpec(openStackCluster, cluster.Name)
+	if err = computeService.DeleteInstance(openStackCluster, instanceSpec, instanceStatus); err != nil {
 		handleUpdateOSCError(openStackCluster, errors.Errorf("failed to delete bastion: %v", err))
 		return errors.Errorf("failed to delete bastion: %v", err)
 	}
@@ -320,7 +320,8 @@ func reconcileBastion(scope *scope.Scope, cluster *clusterv1.Cluster, openStackC
 		return nil
 	}
 
-	instanceStatus, err = computeService.CreateBastion(openStackCluster, cluster.Name)
+	instanceSpec := bastionToInstanceSpec(openStackCluster, cluster.Name)
+	instanceStatus, err = computeService.CreateInstance(openStackCluster, openStackCluster, instanceSpec, cluster.Name)
 	if err != nil {
 		return errors.Errorf("failed to reconcile bastion: %v", err)
 	}
@@ -356,6 +357,31 @@ func reconcileBastion(scope *scope.Scope, cluster *clusterv1.Cluster, openStackC
 	return nil
 }
 
+func bastionToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, clusterName string) *compute.InstanceSpec {
+	name := fmt.Sprintf("%s-bastion", clusterName)
+	instanceSpec := &compute.InstanceSpec{
+		Name:          name,
+		Flavor:        openStackCluster.Spec.Bastion.Instance.Flavor,
+		SSHKeyName:    openStackCluster.Spec.Bastion.Instance.SSHKeyName,
+		Image:         openStackCluster.Spec.Bastion.Instance.Image,
+		ImageUUID:     openStackCluster.Spec.Bastion.Instance.ImageUUID,
+		FailureDomain: openStackCluster.Spec.Bastion.AvailabilityZone,
+		RootVolume:    openStackCluster.Spec.Bastion.Instance.RootVolume,
+	}
+
+	instanceSpec.SecurityGroups = openStackCluster.Spec.Bastion.Instance.SecurityGroups
+	if openStackCluster.Spec.ManagedSecurityGroups {
+		instanceSpec.SecurityGroups = append(instanceSpec.SecurityGroups, infrav1.SecurityGroupParam{
+			UUID: openStackCluster.Status.BastionSecurityGroup.ID,
+		})
+	}
+
+	instanceSpec.Networks = openStackCluster.Spec.Bastion.Instance.Networks
+	instanceSpec.Ports = openStackCluster.Spec.Bastion.Instance.Ports
+
+	return instanceSpec
+}
+
 func reconcileNetworkComponents(scope *scope.Scope, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster) error {
 	clusterName := fmt.Sprintf("%s-%s", cluster.Namespace, cluster.Name)
 

controllers/openstackmachine_controller.go

@@ -248,7 +248,14 @@ func (r *OpenStackMachineReconciler) reconcileDelete(ctx context.Context, scope
 		}
 	}
 
-	if err := computeService.DeleteInstance(openStackMachine, &openStackMachine.Spec, openStackMachine.Name, instanceStatus); err != nil {
+	instanceSpec, err := machineToInstanceSpec(openStackCluster, machine, openStackMachine, "")
+	if err != nil {
+		err = errors.Errorf("machine spec is invalid: %v", err)
+		handleUpdateMachineError(scope.Logger, openStackMachine, err)
+		return ctrl.Result{}, err
+	}
+
+	if err := computeService.DeleteInstance(openStackMachine, instanceSpec, instanceStatus); err != nil {
 		handleUpdateMachineError(scope.Logger, openStackMachine, errors.Errorf("error deleting OpenStack instance %s with ID %s: %v", instanceStatus.Name(), instanceStatus.ID(), err))
 		return ctrl.Result{}, nil
 	}
@@ -392,7 +399,14 @@ func (r *OpenStackMachineReconciler) getOrCreate(logger logr.Logger, cluster *cl
 
 	if instanceStatus == nil {
 		logger.Info("Machine not exist, Creating Machine", "Machine", openStackMachine.Name)
-		instanceStatus, err = computeService.CreateInstance(openStackCluster, machine, openStackMachine, cluster.Name, userData)
+		instanceSpec, err := machineToInstanceSpec(openStackCluster, machine, openStackMachine, userData)
+		if err != nil {
+			err = errors.Errorf("machine spec is invalid: %v", err)
+			handleUpdateMachineError(logger, openStackMachine, err)
+			return nil, err
+		}
+
+		instanceStatus, err = computeService.CreateInstance(openStackMachine, openStackCluster, instanceSpec, cluster.Name)
 		if err != nil {
 			return nil, errors.Errorf("error creating Openstack instance: %v", err)
 		}
@@ -401,6 +415,75 @@ func (r *OpenStackMachineReconciler) getOrCreate(logger logr.Logger, cluster *cl
 	return instanceStatus, nil
 }
 
+func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, userData string) (*compute.InstanceSpec, error) {
+	if openStackMachine == nil {
+		return nil, fmt.Errorf("create Options need be specified to create instace")
+	}
+
+	if machine.Spec.FailureDomain == nil {
+		return nil, fmt.Errorf("failure domain not set")
+	}
+
+	instanceSpec := compute.InstanceSpec{
+		Name:          openStackMachine.Name,
+		Image:         openStackMachine.Spec.Image,
+		ImageUUID:     openStackMachine.Spec.ImageUUID,
+		Flavor:        openStackMachine.Spec.Flavor,
+		SSHKeyName:    openStackMachine.Spec.SSHKeyName,
+		UserData:      userData,
+		Metadata:      openStackMachine.Spec.ServerMetadata,
+		ConfigDrive:   openStackMachine.Spec.ConfigDrive != nil && *openStackMachine.Spec.ConfigDrive,
+		FailureDomain: *machine.Spec.FailureDomain,
+		RootVolume:    openStackMachine.Spec.RootVolume,
+		Subnet:        openStackMachine.Spec.Subnet,
+		ServerGroupID: openStackMachine.Spec.ServerGroupID,
+		Trunk:         openStackMachine.Spec.Trunk,
+	}
+
+	machineTags := []string{}
+
+	// Append machine specific tags
+	machineTags = append(machineTags, openStackMachine.Spec.Tags...)
+
+	// Append cluster scope tags
+	machineTags = append(machineTags, openStackCluster.Spec.Tags...)
+
+	// tags need to be unique or the "apply tags" call will fail.
+	deduplicate := func(tags []string) []string {
+		seen := make(map[string]struct{}, len(machineTags))
+		unique := make([]string, 0, len(machineTags))
+		for _, tag := range tags {
+			if _, ok := seen[tag]; !ok {
+				seen[tag] = struct{}{}
+				unique = append(unique, tag)
+			}
+		}
+		return unique
+	}
+	machineTags = deduplicate(machineTags)
+
+	instanceSpec.Tags = machineTags
+
+	instanceSpec.SecurityGroups = openStackMachine.Spec.SecurityGroups
+	if openStackCluster.Spec.ManagedSecurityGroups {
+		var managedSecurityGroup string
+		if util.IsControlPlaneMachine(machine) {
+			managedSecurityGroup = openStackCluster.Status.ControlPlaneSecurityGroup.ID
+		} else {
+			managedSecurityGroup = openStackCluster.Status.WorkerSecurityGroup.ID
+		}
+
+		instanceSpec.SecurityGroups = append(instanceSpec.SecurityGroups, infrav1.SecurityGroupParam{
+			UUID: managedSecurityGroup,
+		})
+	}
+
+	instanceSpec.Networks = openStackMachine.Spec.Networks
+	instanceSpec.Ports = openStackMachine.Spec.Ports
+
+	return &instanceSpec, nil
+}
+
 func handleUpdateMachineError(logger logr.Logger, openstackMachine *infrav1.OpenStackMachine, message error) {
 	err := capierrors.UpdateMachineError
 	openstackMachine.Status.FailureReason = &err

controllers/openstackmachine_controller_test.go

@@ -0,0 +1,226 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controllers
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/pointer"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+
+	infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
+	"sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/compute"
+)
+
+const (
+	networkUUID                   = "d412171b-9fd7-41c1-95a6-c24e5953974d"
+	subnetUUID                    = "d2d8d98d-b234-477e-a547-868b7cb5d6a5"
+	extraSecurityGroupUUID        = "514bb2d8-3390-4a3b-86a7-7864ba57b329"
+	controlPlaneSecurityGroupUUID = "c9817a91-4821-42db-8367-2301002ab659"
+	workerSecurityGroupUUID       = "9c6c0d28-03c9-436c-815d-58440ac2c1c8"
+	serverGroupUUID               = "7b940d62-68ef-4e42-a76a-1a62e290509c"
+
+	openStackMachineName = "test-openstack-machine"
+	namespace            = "test-namespace"
+	imageName            = "test-image"
+	flavorName           = "test-flavor"
+	sshKeyName           = "test-ssh-key"
+	failureDomain        = "test-failure-domain"
+)
+
+func getDefaultOpenStackCluster() *infrav1.OpenStackCluster {
+	return &infrav1.OpenStackCluster{
+		Spec: infrav1.OpenStackClusterSpec{},
+		Status: infrav1.OpenStackClusterStatus{
+			Network: &infrav1.Network{
+				ID: networkUUID,
+				Subnet: &infrav1.Subnet{
+					ID: subnetUUID,
+				},
+			},
+			ControlPlaneSecurityGroup: &infrav1.SecurityGroup{ID: controlPlaneSecurityGroupUUID},
+			WorkerSecurityGroup:       &infrav1.SecurityGroup{ID: workerSecurityGroupUUID},
+		},
+	}
+}
+
+func getDefaultMachine() *clusterv1.Machine {
+	return &clusterv1.Machine{
+		Spec: clusterv1.MachineSpec{
+			FailureDomain: pointer.StringPtr(failureDomain),
+		},
+	}
+}
+
+func getDefaultOpenStackMachine() *infrav1.OpenStackMachine {
+	return &infrav1.OpenStackMachine{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      openStackMachineName,
+			Namespace: namespace,
+		},
+		Spec: infrav1.OpenStackMachineSpec{
+			// ProviderID is set by the controller
+			// InstanceID is set by the controller
+			// FloatingIP is only used by the cluster controller for the Bastion
+			// TODO: Test Networks, Ports, Subnet, and Trunk separately
+			CloudName:  "test-cloud",
+			Flavor:     flavorName,
+			Image:      imageName,
+			SSHKeyName: sshKeyName,
+			Tags:       []string{"test-tag"},
+			ServerMetadata: map[string]string{
+				"test-metadata": "test-value",
+			},
+			ConfigDrive:   pointer.BoolPtr(true),
+			ServerGroupID: serverGroupUUID,
+		},
+	}
+}
+
+func getDefaultInstanceSpec() *compute.InstanceSpec {
+	return &compute.InstanceSpec{
+		Name:       openStackMachineName,
+		Image:      imageName,
+		Flavor:     flavorName,
+		SSHKeyName: sshKeyName,
+		UserData:   "user-data",
+		Metadata: map[string]string{
+			"test-metadata": "test-value",
+		},
+		ConfigDrive:   *pointer.BoolPtr(true),
+		FailureDomain: *pointer.StringPtr(failureDomain),
+		ServerGroupID: serverGroupUUID,
+		Tags:          []string{"test-tag"},
+	}
+}
+
+func Test_machineToInstanceSpec(t *testing.T) {
+	RegisterTestingT(t)
+
+	tests := []struct {
+		name             string
+		openStackCluster func() *infrav1.OpenStackCluster
+		machine          func() *clusterv1.Machine
+		openStackMachine func() *infrav1.OpenStackMachine
+		wantInstanceSpec func() *compute.InstanceSpec
+		wantErr          bool
+	}{
+		{
+			name:             "Defaults",
+			openStackCluster: getDefaultOpenStackCluster,
+			machine:          getDefaultMachine,
+			openStackMachine: getDefaultOpenStackMachine,
+			wantInstanceSpec: getDefaultInstanceSpec,
+			wantErr:          false,
+		},
+		{
+			name: "Control plane security group",
+			openStackCluster: func() *infrav1.OpenStackCluster {
+				c := getDefaultOpenStackCluster()
+				c.Spec.ManagedSecurityGroups = true
+				return c
+			},
+			machine: func() *clusterv1.Machine {
+				m := getDefaultMachine()
+				m.Labels = map[string]string{
+					clusterv1.MachineControlPlaneLabelName: "true",
+				}
+				return m
+			},
+			openStackMachine: getDefaultOpenStackMachine,
+			wantInstanceSpec: func() *compute.InstanceSpec {
+				i := getDefaultInstanceSpec()
+				i.SecurityGroups = []infrav1.SecurityGroupParam{{UUID: controlPlaneSecurityGroupUUID}}
+				return i
+			},
+			wantErr: false,
+		},
+		{
+			name: "Worker security group",
+			openStackCluster: func() *infrav1.OpenStackCluster {
+				c := getDefaultOpenStackCluster()
+				c.Spec.ManagedSecurityGroups = true
+				return c
+			},
+			machine:          getDefaultMachine,
+			openStackMachine: getDefaultOpenStackMachine,
+			wantInstanceSpec: func() *compute.InstanceSpec {
+				i := getDefaultInstanceSpec()
+				i.SecurityGroups = []infrav1.SecurityGroupParam{{UUID: workerSecurityGroupUUID}}
+				return i
+			},
+			wantErr: false,
+		},
+		{
+			name: "Extra security group",
+			openStackCluster: func() *infrav1.OpenStackCluster {
+				c := getDefaultOpenStackCluster()
+				c.Spec.ManagedSecurityGroups = true
+				return c
+			},
+			machine: getDefaultMachine,
+			openStackMachine: func() *infrav1.OpenStackMachine {
+				m := getDefaultOpenStackMachine()
+				m.Spec.SecurityGroups = []infrav1.SecurityGroupParam{{UUID: extraSecurityGroupUUID}}
+				return m
+			},
+			wantInstanceSpec: func() *compute.InstanceSpec {
+				i := getDefaultInstanceSpec()
+				i.SecurityGroups = []infrav1.SecurityGroupParam{
+					{UUID: extraSecurityGroupUUID},
+					{UUID: workerSecurityGroupUUID},
+				}
+				return i
+			},
+			wantErr: false,
+		},
+		{
+			name: "Tags",
+			openStackCluster: func() *infrav1.OpenStackCluster {
+				c := getDefaultOpenStackCluster()
+				c.Spec.Tags = []string{"cluster-tag", "duplicate-tag"}
+				return c
+			},
+			machine: getDefaultMachine,
+			openStackMachine: func() *infrav1.OpenStackMachine {
+				m := getDefaultOpenStackMachine()
+				m.Spec.Tags = []string{"machine-tag", "duplicate-tag"}
+				return m
+			},
+			wantInstanceSpec: func() *compute.InstanceSpec {
+				i := getDefaultInstanceSpec()
+				i.Tags = []string{"machine-tag", "duplicate-tag", "cluster-tag"}
+				return i
+			},
+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := machineToInstanceSpec(tt.openStackCluster(), tt.machine(), tt.openStackMachine(), "user-data")
+			if tt.wantErr {
+				Expect(err).To(HaveOccurred())
+			} else {
+				Expect(err).NotTo(HaveOccurred())
+			}
+
+			Expect(got).To(Equal(tt.wantInstanceSpec()))
+		})
+	}
+}