Fix/add listall list domains (kubernetes-sigs#60)

* Use ListDomains with listall=true instead of GetDomainID to get sub-domains as well * Update unit test for listing domains * Support domain path in domain: property * Narrow down the list results using setName and setLevel * Handle the edge case of ROOT domain * Add uni tests Remove SetLevel to be backword compatible Co-authored-by: Wonkun Kim <[email protected]>
shapeblue · Apr 4, 2022 · b3e79b6 · b3e79b6
1 parent 53b08d0
commit b3e79b6
Show file tree

Hide file tree

Showing 3 changed files with 116 additions and 12 deletions.
diff --git a/pkg/cloud/cluster.go b/pkg/cloud/cluster.go
@@ -17,12 +17,17 @@ 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"
 	capiv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 )
 
+const rootDomain = "ROOT"
+const domainDelimiter = "/"
+
 type ClusterIface interface {
 	GetOrCreateCluster(*infrav1.CloudStackCluster) error
 	DisposeClusterResources(cluster *infrav1.CloudStackCluster) error
@@ -67,16 +72,8 @@ func (c *client) GetOrCreateCluster(csCluster *infrav1.CloudStackCluster) (retEr
 		csCluster.Status.FailureDomains[zone.ID] = capiv1.FailureDomainSpec{ControlPlane: true}
 	}
 
-	// If provided, translate Domain name to Domain ID.
-	if csCluster.Spec.Domain != "" {
-		domainID, count, retErr := c.cs.Domain.GetDomainID(csCluster.Spec.Domain)
-		if retErr != nil {
-			return retErr
-		} else if count != 1 {
-			return errors.Errorf("expected 1 Domain with name %s, but got %d", csCluster.Spec.Domain, count)
-		} else {
-			csCluster.Status.DomainID = domainID
-		}
+	if retErr := c.ResolveDomainAndAccount(csCluster); retErr != nil {
+		return retErr
 	}
 
 	// Get current network statuses.
@@ -92,6 +89,55 @@ func (c *client) GetOrCreateCluster(csCluster *infrav1.CloudStackCluster) (retEr
 	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()
+
+		if csCluster.Spec.Domain == rootDomain {
+			p.SetName(csCluster.Spec.Domain)
+		} else {
+			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
+		} else if resp.Count == 1 {
+			csCluster.Status.DomainID = resp.Domains[0].Id
+		} else {
+			for _, domain := range resp.Domains {
+				if domain.Path == rootDomain+domainDelimiter+csCluster.Spec.Domain {
+					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 != 1 {
+			return errors.Errorf("expected 1 Account with account name %s in domain ID %s, but got %d",
+				csCluster.Spec.Account, csCluster.Status.DomainID, resp.Count)
+		}
+	}
+	return nil
+}
+
 func (c *client) DisposeClusterResources(csCluster *infrav1.CloudStackCluster) (retError error) {
 	if csCluster.Status.PublicIPID != "" {
 		if err := c.DeleteClusterTag(ResourceTypeIPAddress, csCluster.Status.PublicIPID, csCluster); err != nil {

diff --git a/pkg/cloud/cluster_test.go b/pkg/cloud/cluster_test.go
@@ -35,6 +35,7 @@ var _ = Describe("Cluster", func() {
 		mockClient *csapi.CloudStackClient
 		zs         *csapi.MockZoneServiceIface
 		ds         *csapi.MockDomainServiceIface
+		as         *csapi.MockAccountServiceIface
 		ns         *csapi.MockNetworkServiceIface
 	)
 
@@ -43,10 +44,12 @@ var _ = Describe("Cluster", func() {
 		mockClient = csapi.NewMockClient(mockCtrl)
 		zs = mockClient.Zone.(*csapi.MockZoneServiceIface)
 		ds = mockClient.Domain.(*csapi.MockDomainServiceIface)
+		as = mockClient.Account.(*csapi.MockAccountServiceIface)
 		ns = mockClient.Network.(*csapi.MockNetworkServiceIface)
 		client = cloud.NewClientFromCSAPIClient(mockClient)
 		dummies.SetDummyVars()
 		dummies.SetDummyDomainAndAccount()
+		dummies.SetDummyCSApiResponse()
 	})
 
 	AfterEach(func() {
@@ -72,10 +75,13 @@ var _ = Describe("Cluster", func() {
 				ContainSubstring("could not get Zone by ID "+dummies.Zone1.ID+": Not found"))))
 		})
 
-		It("translates Domain to DomainID when Domain is set", func() {
+		It("resolves domain and account when both are specified", func() {
 			zs.EXPECT().GetZoneID(dummies.Zone1.Name).Return(dummies.Zone1.ID, 1, nil)
 			zs.EXPECT().GetZoneByID(dummies.Zone1.ID).Return(dummies.CAPCZoneToCSAPIZone(&dummies.Zone1), 1, nil)
-			ds.EXPECT().GetDomainID(dummies.CSCluster.Spec.Domain).Return(dummies.DomainID, 1, nil)
+			ds.EXPECT().NewListDomainsParams().Return(dummies.ListDomainsParams)
+			ds.EXPECT().ListDomains(dummies.ListDomainsParams).Return(dummies.ListDomainsResp, nil)
+			as.EXPECT().NewListAccountsParams().Return(dummies.ListAccountsParams)
+			as.EXPECT().ListAccounts(dummies.ListAccountsParams).Return(dummies.ListAccountsResp, nil)
 			ns.EXPECT().GetNetworkByName(dummies.Net1.Name).Return(dummies.CAPCNetToCSAPINet(&dummies.Net1), 1, nil)
 
 			// Limit test to single zone.
@@ -85,5 +91,41 @@ var _ = Describe("Cluster", func() {
 			Ω(client.GetOrCreateCluster(dummies.CSCluster)).Should(Succeed())
 			Ω(dummies.CSCluster.Status.DomainID).Should(Equal(dummies.DomainID))
 		})
+
+		It("resolves domain and account when none are specified", func() {
+			zs.EXPECT().GetZoneID(dummies.Zone1.Name).Return(dummies.Zone1.ID, 1, nil)
+			zs.EXPECT().GetZoneByID(dummies.Zone1.ID).Return(dummies.CAPCZoneToCSAPIZone(&dummies.Zone1), 1, nil)
+			ns.EXPECT().GetNetworkByName(dummies.Net1.Name).Return(dummies.CAPCNetToCSAPINet(&dummies.Net1), 1, nil)
+
+			// Limit test to single zone.
+			dummies.CSCluster.Spec.Zones = []capcv1.Zone{dummies.Zone1}
+			dummies.CSCluster.Status.Zones = capcv1.ZoneStatusMap{}
+
+			dummies.CSCluster.Spec.Domain = ""
+			dummies.CSCluster.Spec.Account = ""
+
+			Ω(client.GetOrCreateCluster(dummies.CSCluster)).Should(Succeed())
+			Ω(dummies.CSCluster.Status.DomainID).Should(Equal(""))
+		})
+
+		It("fails when only one of domain or account is specified", func() {
+			zs.EXPECT().GetZoneID(dummies.Zone1.Name).Return(dummies.Zone1.ID, 1, nil).AnyTimes()
+			zs.EXPECT().GetZoneByID(dummies.Zone1.ID).Return(dummies.CAPCZoneToCSAPIZone(&dummies.Zone1), 1, nil).AnyTimes()
+			ns.EXPECT().GetNetworkByName(dummies.Net1.Name).Return(dummies.CAPCNetToCSAPINet(&dummies.Net1), 1, nil).AnyTimes()
+
+			// Limit test to single zone.
+			dummies.CSCluster.Spec.Zones = []capcv1.Zone{dummies.Zone1}
+			dummies.CSCluster.Status.Zones = capcv1.ZoneStatusMap{}
+
+			domainBackup := dummies.CSCluster.Spec.Domain
+			dummies.CSCluster.Spec.Domain = ""
+
+			Ω(client.GetOrCreateCluster(dummies.CSCluster)).ShouldNot(Succeed())
+
+			dummies.CSCluster.Spec.Domain = domainBackup
+			dummies.CSCluster.Spec.Account = ""
+
+			Ω(client.GetOrCreateCluster(dummies.CSCluster)).ShouldNot(Succeed())
+		})
 	})
 })
diff --git a/test/dummies/vars.go b/test/dummies/vars.go
@@ -47,6 +47,10 @@ var ( // Declare exported dummy vars.
 	PublicIPID         string
 	EndPointHost       string
 	EndPointPort       int32
+	ListDomainsParams  *csapi.ListDomainsParams
+	ListDomainsResp    *csapi.ListDomainsResponse
+	ListAccountsParams *csapi.ListAccountsParams
+	ListAccountsResp   *csapi.ListAccountsResponse
 )
 
 // SetDummyVars sets/resets all dummy vars.
@@ -262,3 +266,15 @@ func SetDummyCAPIMachineVars() {
 func SetDummyCSMachineStatuses() {
 	CSMachine1.Status = capcv1.CloudStackMachineStatus{ZoneID: Zone1.ID}
 }
+
+func SetDummyCSApiResponse() {
+	ListDomainsParams = &csapi.ListDomainsParams{}
+	ListDomainsResp = &csapi.ListDomainsResponse{}
+	ListDomainsResp.Count = 1
+	ListDomainsResp.Domains = []*csapi.Domain{{Id: DomainID}}
+
+	ListAccountsParams = &csapi.ListAccountsParams{}
+	ListAccountsResp = &csapi.ListAccountsResponse{}
+	ListAccountsResp.Count = 1
+	ListAccountsResp.Accounts = []*csapi.Account{{Name: Account}}
+}