Fixup deletion and ag owner refs.

Author: Joshua Reed

controllers/cloudstackcluster_controller.go

@@ -92,24 +92,14 @@ func (r *CloudStackClusterReconciliationRunner) Reconcile() (res ctrl.Result, re
 		r.GetZones(r.Zones),
 		r.VerifyZoneCRDs,
 		r.SetFailureDomains,
-		r.ResolveClusterDetails)
+		r.SetReady)
 }
 
-// ResolveClusterDetails fetches cluster specific details like domain and account IDs.
-func (r *CloudStackClusterReconciliationRunner) ResolveClusterDetails() (ctrl.Result, error) {
-	// Ensure that CAPI won't prematurely delete this CloudStackCluster.
+// SetReady adds a finalizer and sets the cluster status to ready.
+func (r *CloudStackClusterReconciliationRunner) SetReady() (ctrl.Result, error) {
 	controllerutil.AddFinalizer(r.ReconciliationSubject, infrav1.ClusterFinalizer)
-
-	// Create and or fetch cluster components.
-	err := r.CSUser.GetOrCreateCluster(r.ReconciliationSubject)
-	if err == nil {
-		r.Log.Info("Fetched cluster info successfully.")
-		r.Log.V(1).Info("Post fetch cluster status.", "clusterStatus", r.ReconciliationSubject.Status)
-
-		// Set cluster to ready to indicate readiness to CAPI.
-		r.ReconciliationSubject.Status.Ready = true
-	}
-	return ctrl.Result{}, err
+	r.ReconciliationSubject.Status.Ready = true
+	return ctrl.Result{}, nil
 }
 
 // CheckZoneDetails verifies the Zone CRDs found match against those requested.
@@ -150,9 +140,6 @@ func (r *CloudStackClusterReconciliationRunner) ReconcileDelete() (ctrl.Result,
 		}
 		return r.RequeueWithMessage("Child Zones still present, requeueing.")
 	}
-	if err := r.CSUser.DisposeClusterResources(r.ReconciliationSubject); err != nil {
-		return ctrl.Result{}, err
-	}
 	controllerutil.RemoveFinalizer(r.ReconciliationSubject, infrav1.ClusterFinalizer)
 	return ctrl.Result{}, nil
 }

controllers/cloudstackmachine_controller.go

@@ -117,7 +117,7 @@ func (r *CloudStackMachineReconciliationRunner) ConsiderAffinity() (ctrl.Result,
 		return res, err
 	}
 	if !r.AffinityGroup.Status.Ready {
-		return r.RequeueWithMessage("Required afinity group not ready.")
+		return r.RequeueWithMessage("Required affinity group not ready.")
 	}
 	return ctrl.Result{}, nil
 }
@@ -230,7 +230,10 @@ func (r *CloudStackMachineReconciliationRunner) AddToLBIfNeeded() (retRes ctrl.R
 
 func (r *CloudStackMachineReconciliationRunner) ReconcileDelete() (retRes ctrl.Result, reterr error) {
 	r.Log.Info("Deleting instance", "instance-id", r.ReconciliationSubject.Spec.InstanceID)
-	if err := r.CSUser.DestroyVMInstance(r.ReconciliationSubject); err != nil {
+	// Use CSClient instead of CSUser here to expunge as admin.
+	// The CloudStack-Go API does not return an error, but the VM won't delete with Expunge set if requested by
+	// non-domain admin user.
+	if err := r.CSClient.DestroyVMInstance(r.ReconciliationSubject); err != nil {
 		if err.Error() == "VM deletion in progress" {
 			r.Log.Info(err.Error())
 			return ctrl.Result{RequeueAfter: utils.DestoryVMRequeueInterval}, nil

controllers/utils/affinity_group.go

@@ -22,6 +22,7 @@ import (
 
 	infrav1 "github.com/aws/cluster-api-provider-cloudstack/api/v1beta1"
 	"github.com/pkg/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	capiv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -40,25 +41,30 @@ func (r *ReconciliationRunner) GetOrCreateAffinityGroup(name string, affinityTyp
 			return ctrl.Result{}, nil
 		} // Didn't find a group, so create instead.
 
+		// Set affinity group type.
 		if affinityType == infrav1.ProAffinity {
 			ag.Spec.Type = "host affinity"
 		} else if affinityType == infrav1.AntiAffinity {
 			ag.Spec.Type = "host anti-affinity"
 		} else {
 			return ctrl.Result{}, errors.Errorf("unrecognized affinity type %s", affinityType)
 		}
+
+		// Setup basic metadata.
+		ag.Name = name
 		ag.Spec.Name = name
+		ag.ObjectMeta = r.NewChildObjectMeta(lowerName)
 
+		// Replace owner reference with controller of CAPI and CloudStack machines.
 		for _, ref := range r.ReconciliationSubject.GetOwnerReferences() {
-			if ref.Kind == "EtcdadmCluster" || ref.Kind == "KubeadmControlPlane" || ref.Kind == "MachineSet" {
-				ag.OwnerReferences = append(ag.OwnerReferences, ref)
+			if strings.EqualFold(ref.Kind, "EtcdadmCluster") ||
+				strings.EqualFold(ref.Kind, "KubeadmControlPlane") ||
+				strings.EqualFold(ref.Kind, "MachineSet") {
+				ag.OwnerReferences = []metav1.OwnerReference{ref}
 				break
 			}
 		}
 
-		ag.Name = name
-		ag.Spec.Name = name
-		ag.ObjectMeta = r.NewChildObjectMeta(lowerName)
 		if err := r.K8sClient.Create(r.RequestCtx, ag); err != nil && !ContainsAlreadyExistsSubstring(err) {
 			return r.ReturnWrappedError(err, "creating affinity group CRD:")
 		}

controllers/utils/base_reconciler.go

@@ -225,15 +225,13 @@ func (r *ReconciliationRunner) SetCSUser() (ctrl.Result, error) {
 	r.CSUser = r.CSClient
 	if r.CSCluster.Spec.Account != "" {
 		user := &cloud.User{}
-		user.Account.Domain.Name = r.CSCluster.Spec.Domain
-		user.Account.Domain.ID = r.CSCluster.Spec.Domain
+		user.Account.Domain.Path = r.CSCluster.Spec.Domain
 		user.Account.Name = r.CSCluster.Spec.Account
-		user.Account.ID = r.CSCluster.Spec.Account
 		if found, err := r.CSClient.GetUserWithKeys(user); err != nil {
 			return ctrl.Result{}, err
 		} else if !found {
 			return ctrl.Result{}, errors.Errorf("could not find sufficient user (with API keys) in domain/account %s/%s",
-				r.CSCluster.Spec.Account, r.CSCluster.Spec.Domain)
+				r.CSCluster.Spec.Domain, r.CSCluster.Spec.Account)
 		}
 		cfg := cloud.Config{APIKey: user.APIKey, SecretKey: user.SecretKey}
 		if client, err := r.CSClient.NewClientFromSpec(cfg); err != nil {

pkg/cloud/client.go

@@ -27,7 +27,6 @@ import (
 //go:generate mockgen -destination=../mocks/mock_client.go -package=mocks github.com/aws/cluster-api-provider-cloudstack/pkg/cloud Client
 
 type Client interface {
-	ClusterIface
 	VMIface
 	NetworkIface
 	AffinityGroupIface

pkg/cloud/cluster.go

@@ -15,114 +15,3 @@ limitations under the License.
 */
 
 package cloud
-
-import (
-	"strings"
-
-	infrav1 "github.com/aws/cluster-api-provider-cloudstack/api/v1beta1"
-	"github.com/hashicorp/go-multierror"
-	"github.com/pkg/errors"
-)
-
-const rootDomain = "ROOT"
-const domainDelimiter = "/"
-
-type ClusterIface interface {
-	GetOrCreateCluster(*infrav1.CloudStackCluster) error
-	DisposeClusterResources(cluster *infrav1.CloudStackCluster) error
-	ResolveZones(*infrav1.CloudStackCluster) error
-}
-
-func (c *client) ResolveZones(csCluster *infrav1.CloudStackCluster) (retErr error) {
-	for _, specZone := range csCluster.Spec.Zones {
-		if zoneID, count, err := c.cs.Zone.GetZoneID(specZone.Name); err != nil {
-			retErr = multierror.Append(retErr, errors.Wrapf(err, "could not get Zone ID from %s", specZone))
-		} else if count != 1 {
-			retErr = multierror.Append(retErr, errors.Errorf(
-				"expected 1 Zone with name %s, but got %d", specZone.Name, count))
-		} else {
-			specZone.ID = zoneID
-		}
-
-		if resp, count, err := c.cs.Zone.GetZoneByID(specZone.ID); err != nil {
-			return multierror.Append(retErr, errors.Wrapf(err, "could not get Zone by ID %s", specZone.ID))
-		} else if count != 1 {
-			return multierror.Append(retErr, errors.Errorf(
-				"expected 1 Zone with UUID %s, but got %d", specZone.ID, count))
-		} else {
-			zone := infrav1.Zone{Network: specZone.Network}
-			zone.Name = resp.Name
-			zone.ID = resp.Id
-			csCluster.Status.Zones[resp.Id] = zone
-		}
-	}
-
-	return nil
-}
-
-func (c *client) GetOrCreateCluster(csCluster *infrav1.CloudStackCluster) (retErr error) {
-	if csCluster.Status.Zones == nil {
-		csCluster.Status.Zones = make(map[string]infrav1.Zone)
-	}
-
-	if retErr := c.ResolveDomainAndAccount(csCluster); retErr != nil {
-		return retErr
-	}
-
-	return nil
-}
-
-func (c *client) ResolveDomainAndAccount(csCluster *infrav1.CloudStackCluster) error {
-	if (csCluster.Spec.Domain != "" && csCluster.Spec.Account == "") ||
-		(csCluster.Spec.Domain == "" && csCluster.Spec.Account != "") {
-		return errors.Errorf("Both domain and account must be specified or none of them must be specified")
-	}
-
-	if csCluster.Spec.Domain != "" && csCluster.Spec.Account != "" {
-		p := c.cs.Domain.NewListDomainsParams()
-		tokens := strings.Split(csCluster.Spec.Domain, domainDelimiter)
-		domainName := tokens[len(tokens)-1]
-		p.SetListall(true)
-		p.SetName(domainName)
-
-		resp, retErr := c.cs.Domain.ListDomains(p)
-		if retErr != nil {
-			return retErr
-		}
-
-		var domainPath string
-		if csCluster.Spec.Domain == rootDomain {
-			domainPath = rootDomain
-		} else {
-			domainPath = strings.Join([]string{rootDomain, csCluster.Spec.Domain}, domainDelimiter)
-		}
-		for _, domain := range resp.Domains {
-			if domain.Path == domainPath {
-				csCluster.Status.DomainID = domain.Id
-				break
-			}
-		}
-		if csCluster.Status.DomainID == "" {
-			return errors.Errorf("domain not found for domain path %s", csCluster.Spec.Domain)
-		}
-
-		listAccountParams := c.cs.Account.NewListAccountsParams()
-		listAccountParams.SetDomainid(csCluster.Status.DomainID)
-		listAccountParams.SetName(csCluster.Spec.Account)
-		listAccountResp, retErr := c.cs.Account.ListAccounts(listAccountParams)
-		if retErr != nil {
-			return retErr
-		} else if listAccountResp.Count == 0 {
-			return errors.Errorf("could not find account %s in domain ID %s",
-				csCluster.Spec.Account, csCluster.Status.DomainID)
-		} else if listAccountResp.Count > 1 {
-			return errors.Errorf("expected 1 Account with account name %s in domain ID %s, but got %d",
-				csCluster.Spec.Account, csCluster.Status.DomainID, listAccountResp.Count)
-		}
-	}
-	return nil
-}
-
-func (c *client) DisposeClusterResources(csCluster *infrav1.CloudStackCluster) (retError error) {
-	return nil
-}

pkg/cloud/instance.go

@@ -109,7 +109,7 @@ func (c *client) ResolveTemplate(
 	zoneID string,
 ) (templateID string, retErr error) {
 	if len(csMachine.Spec.Template.ID) > 0 {
-		csTemplate, count, err := c.cs.Template.GetTemplateByID(csMachine.Spec.Template.ID, "all")
+		csTemplate, count, err := c.cs.Template.GetTemplateByID(csMachine.Spec.Template.ID, "executable")
 		if err != nil {
 			return "", multierror.Append(retErr, errors.Wrapf(
 				err, "could not get Template by ID %s", csMachine.Spec.Template.ID))
@@ -124,7 +124,7 @@ func (c *client) ResolveTemplate(
 		}
 		return csMachine.Spec.Template.ID, nil
 	}
-	templateID, count, err := c.cs.Template.GetTemplateID(csMachine.Spec.Template.Name, "all", zoneID)
+	templateID, count, err := c.cs.Template.GetTemplateID(csMachine.Spec.Template.Name, "executable", zoneID)
 	if err != nil {
 		return "", multierror.Append(retErr, errors.Wrapf(
 			err, "could not get Template ID from %s", csMachine.Spec.Template.Name))
@@ -162,8 +162,6 @@ func (c *client) GetOrCreateVMInstance(
 
 	// Create VM instance.
 	p := c.cs.VirtualMachine.NewDeployVirtualMachineParams(offeringID, templateID, csMachine.Status.ZoneID)
-	setIfNotEmpty(csCluster.Spec.Account, p.SetAccount)
-	setIfNotEmpty(csCluster.Status.DomainID, p.SetDomainid)
 	p.SetNetworkids([]string{zone.Spec.Network.ID})
 	setIfNotEmpty(csMachine.Name, p.SetName)
 	setIfNotEmpty(csMachine.Name, p.SetDisplayname)
@@ -199,8 +197,6 @@ func (c *client) GetOrCreateVMInstance(
 		listVirtualMachineParams.SetZoneid(csMachine.Status.ZoneID)
 		listVirtualMachineParams.SetNetworkid(zone.Spec.Network.ID)
 		listVirtualMachineParams.SetName(csMachine.Name)
-		setIfNotEmpty(csCluster.Status.DomainID, listVirtualMachineParams.SetDomainid)
-		setIfNotEmpty(csCluster.Spec.Account, listVirtualMachineParams.SetAccount)
 		if listVirtualMachinesResponse, err2 := c.cs.VirtualMachine.ListVirtualMachines(listVirtualMachineParams); err2 == nil && listVirtualMachinesResponse.Count > 0 {
 			csMachine.Spec.InstanceID = pointer.StringPtr(listVirtualMachinesResponse.VirtualMachines[0].Id)
 		}
@@ -215,10 +211,10 @@ func (c *client) GetOrCreateVMInstance(
 
 // DestroyVMInstance Destroys a VM instance. Assumes machine has been fetched prior and has an instance ID.
 func (c *client) DestroyVMInstance(csMachine *infrav1.CloudStackMachine) error {
-
 	// Attempt deletion regardless of machine state.
-	p := c.cs.VirtualMachine.NewDestroyVirtualMachineParams(*csMachine.Spec.InstanceID)
+	p := c.csAsync.VirtualMachine.NewDestroyVirtualMachineParams(*csMachine.Spec.InstanceID)
 	p.SetExpunge(true)
+
 	if _, err := c.csAsync.VirtualMachine.DestroyVirtualMachine(p); err != nil &&
 		strings.Contains(strings.ToLower(err.Error()), "unable to find uuid for id") {
 		// VM doesn't exist. Success...
@@ -239,5 +235,6 @@ func (c *client) DestroyVMInstance(csMachine *infrav1.CloudStackMachine) error {
 		return err
 	}
 
+	return nil
 	return errors.New("VM deletion in progress")
 }

pkg/cloud/instance_test.go

@@ -149,12 +149,11 @@ var _ = Describe("Instance", func() {
 				ShouldNot(Succeed())
 		})
 
-		const allFilter = "all"
 		It("returns errors while fetching template", func() {
 			expectVMNotFound()
 			sos.EXPECT().GetServiceOfferingID(dummies.CSMachine1.Spec.Offering.Name).
 				Return(dummies.CSMachine1.Spec.Offering.ID, 1, nil)
-			ts.EXPECT().GetTemplateID(dummies.CSMachine1.Spec.Template.Name, allFilter, dummies.Zone1.ID).
+			ts.EXPECT().GetTemplateID(dummies.CSMachine1.Spec.Template.Name, "", dummies.Zone1.ID).
 				Return("", -1, unknownError)
 			Ω(client.GetOrCreateVMInstance(
 				dummies.CSMachine1, dummies.CAPIMachine, dummies.CSCluster, dummies.CSZone1, dummies.CSAffinityGroup, "")).
@@ -165,7 +164,7 @@ var _ = Describe("Instance", func() {
 			expectVMNotFound()
 			sos.EXPECT().GetServiceOfferingID(dummies.CSMachine1.Spec.Offering.Name).
 				Return(dummies.CSMachine1.Spec.Offering.ID, 1, nil)
-			ts.EXPECT().GetTemplateID(dummies.CSMachine1.Spec.Template.Name, allFilter, dummies.Zone1.ID).Return("", 2, nil)
+			ts.EXPECT().GetTemplateID(dummies.CSMachine1.Spec.Template.Name, "", dummies.Zone1.ID).Return("", 2, nil)
 			Ω(client.GetOrCreateVMInstance(
 				dummies.CSMachine1, dummies.CAPIMachine, dummies.CSCluster, dummies.CSZone1, dummies.CSAffinityGroup, "")).
 				ShouldNot(Succeed())
@@ -175,7 +174,7 @@ var _ = Describe("Instance", func() {
 			expectVMNotFound()
 			sos.EXPECT().GetServiceOfferingID(dummies.CSMachine1.Spec.Offering.Name).
 				Return(offeringFakeID, 1, nil)
-			ts.EXPECT().GetTemplateID(dummies.CSMachine1.Spec.Template.Name, allFilter, dummies.Zone1.ID).
+			ts.EXPECT().GetTemplateID(dummies.CSMachine1.Spec.Template.Name, "", dummies.Zone1.ID).
 				Return(templateFakeID, 1, nil)
 			vms.EXPECT().NewDeployVirtualMachineParams(offeringFakeID, templateFakeID, dummies.Zone1.ID).
 				Return(&cloudstack.DeployVirtualMachineParams{})
@@ -200,7 +199,7 @@ var _ = Describe("Instance", func() {
 				dummies.CSCluster.Status.DomainID = domainID
 				vms.EXPECT().GetVirtualMachinesMetricByID(*dummies.CSMachine1.Spec.InstanceID).Return(nil, -1, notFoundError)
 				sos.EXPECT().GetServiceOfferingID(dummies.CSMachine1.Spec.Offering.Name).Return(offeringFakeID, 1, nil)
-				ts.EXPECT().GetTemplateID(dummies.CSMachine1.Spec.Template.Name, allFilter, dummies.Zone1.ID).
+				ts.EXPECT().GetTemplateID(dummies.CSMachine1.Spec.Template.Name, "", dummies.Zone1.ID).
 					Return(templateFakeID, 1, nil)
 				vms.EXPECT().GetVirtualMachinesMetricByID(*dummies.CSMachine1.Spec.InstanceID).
 					Return(&cloudstack.VirtualMachinesMetric{}, 1, nil)
@@ -248,7 +247,7 @@ var _ = Describe("Instance", func() {
 				dummies.CSMachine1.Spec.Template.Name = "template"
 
 				sos.EXPECT().GetServiceOfferingID(dummies.CSMachine1.Spec.Offering.Name).Return(offeringFakeID, 1, nil)
-				ts.EXPECT().GetTemplateID(dummies.CSMachine1.Spec.Template.Name, allFilter, dummies.Zone1.ID).
+				ts.EXPECT().GetTemplateID(dummies.CSMachine1.Spec.Template.Name, "", dummies.Zone1.ID).
 					Return(templateFakeID, 1, nil)
 
 				ActionAndAssert()
@@ -261,7 +260,7 @@ var _ = Describe("Instance", func() {
 				dummies.CSMachine1.Spec.Template.Name = "template"
 
 				sos.EXPECT().GetServiceOfferingByID(dummies.CSMachine1.Spec.Offering.ID).Return(&cloudstack.ServiceOffering{Name: ""}, 1, nil)
-				ts.EXPECT().GetTemplateID(dummies.CSMachine1.Spec.Template.Name, allFilter, dummies.Zone1.ID).
+				ts.EXPECT().GetTemplateID(dummies.CSMachine1.Spec.Template.Name, "", dummies.Zone1.ID).
 					Return(templateFakeID, 1, nil)
 
 				ActionAndAssert()
@@ -274,7 +273,7 @@ var _ = Describe("Instance", func() {
 				dummies.CSMachine1.Spec.Template.Name = ""
 
 				sos.EXPECT().GetServiceOfferingID(dummies.CSMachine1.Spec.Offering.Name).Return(offeringFakeID, 1, nil)
-				ts.EXPECT().GetTemplateByID(dummies.CSMachine1.Spec.Template.ID, allFilter).Return(&cloudstack.Template{Name: ""}, 1, nil)
+				ts.EXPECT().GetTemplateByID(dummies.CSMachine1.Spec.Template.ID, "").Return(&cloudstack.Template{Name: ""}, 1, nil)
 
 				ActionAndAssert()
 			})
@@ -286,7 +285,7 @@ var _ = Describe("Instance", func() {
 				dummies.CSMachine1.Spec.Template.Name = ""
 
 				sos.EXPECT().GetServiceOfferingByID(dummies.CSMachine1.Spec.Offering.ID).Return(&cloudstack.ServiceOffering{Name: "offering"}, 1, nil)
-				ts.EXPECT().GetTemplateByID(dummies.CSMachine1.Spec.Template.ID, allFilter).Return(&cloudstack.Template{Name: "template"}, 1, nil)
+				ts.EXPECT().GetTemplateByID(dummies.CSMachine1.Spec.Template.ID, "").Return(&cloudstack.Template{Name: "template"}, 1, nil)
 
 				ActionAndAssert()
 			})
@@ -298,7 +297,7 @@ var _ = Describe("Instance", func() {
 				dummies.CSMachine1.Spec.Template.Name = "template"
 
 				sos.EXPECT().GetServiceOfferingByID(dummies.CSMachine1.Spec.Offering.ID).Return(&cloudstack.ServiceOffering{Name: "offering"}, 1, nil)
-				ts.EXPECT().GetTemplateByID(dummies.CSMachine1.Spec.Template.ID, allFilter).Return(&cloudstack.Template{Name: "template"}, 1, nil)
+				ts.EXPECT().GetTemplateByID(dummies.CSMachine1.Spec.Template.ID, "").Return(&cloudstack.Template{Name: "template"}, 1, nil)
 
 				ActionAndAssert()
 			})
@@ -331,7 +330,7 @@ var _ = Describe("Instance", func() {
 				dummies.CSMachine1.Spec.Template.Name = "template"
 
 				sos.EXPECT().GetServiceOfferingByID(dummies.CSMachine1.Spec.Offering.ID).Return(&cloudstack.ServiceOffering{Name: "offering"}, 1, nil)
-				ts.EXPECT().GetTemplateByID(dummies.CSMachine1.Spec.Template.ID, allFilter).Return(&cloudstack.Template{Name: "template-not-match"}, 1, nil)
+				ts.EXPECT().GetTemplateByID(dummies.CSMachine1.Spec.Template.ID, "").Return(&cloudstack.Template{Name: "template-not-match"}, 1, nil)
 				requiredRegexp := "template name %s does not match name %s returned using UUID %s"
 				Ω(client.GetOrCreateVMInstance(
 					dummies.CSMachine1, dummies.CAPIMachine, dummies.CSCluster, dummies.CSZone1, dummies.CSAffinityGroup, "")).