Update for review comments

pkg/executables/cmk.go

@@ -187,39 +187,34 @@ func (c *Cmk) ValidateAffinityGroupsPresent(ctx context.Context, domainId string
 	return nil
 }
 
-func (c *Cmk) ValidateZonesPresent(ctx context.Context, zones []v1alpha1.CloudStackZone) ([]v1alpha1.CloudStackResourceIdentifier, error) {
-	var zoneIdentifiers []v1alpha1.CloudStackResourceIdentifier
-	for _, zone := range zones {
-		command := newCmkCommand("list zones")
-		if len(zone.Id) > 0 {
-			applyCmkArgs(&command, withCloudStackId(zone.Id))
-		} else {
-			applyCmkArgs(&command, withCloudStackName(zone.Name))
-		}
-		result, err := c.exec(ctx, command...)
-		if err != nil {
-			return nil, fmt.Errorf("getting zones info - %s: %v", result.String(), err)
-		}
-		if result.Len() == 0 {
-			return nil, fmt.Errorf("zone %s not found", zone)
-		}
+func (c *Cmk) ValidateZonePresent(ctx context.Context, zone v1alpha1.CloudStackZone) (string, error) {
+	command := newCmkCommand("list zones")
+	if len(zone.Id) > 0 {
+		applyCmkArgs(&command, withCloudStackId(zone.Id))
+	} else {
+		applyCmkArgs(&command, withCloudStackName(zone.Name))
+	}
+	result, err := c.exec(ctx, command...)
+	if err != nil {
+		return "", fmt.Errorf("getting zones info - %s: %v", result.String(), err)
+	}
+	if result.Len() == 0 {
+		return "", fmt.Errorf("zone %s not found", zone)
+	}
 
-		response := struct {
-			CmkZones []cmkResourceIdentifier `json:"zone"`
-		}{}
-		if err = json.Unmarshal(result.Bytes(), &response); err != nil {
-			return nil, fmt.Errorf("parsing response into json: %v", err)
-		}
-		cmkZones := response.CmkZones
-		if len(cmkZones) > 1 {
-			return nil, fmt.Errorf("duplicate zone %s found", zone)
-		} else if len(zones) == 0 {
-			return nil, fmt.Errorf("zone %s not found", zone)
-		} else {
-			zoneIdentifiers = append(zoneIdentifiers, v1alpha1.CloudStackResourceIdentifier{Name: cmkZones[0].Name, Id: cmkZones[0].Id})
-		}
+	response := struct {
+		CmkZones []cmkResourceIdentifier `json:"zone"`
+	}{}
+	if err = json.Unmarshal(result.Bytes(), &response); err != nil {
+		return "", fmt.Errorf("parsing response into json: %v", err)
 	}
-	return zoneIdentifiers, nil
+	cmkZones := response.CmkZones
+	if len(cmkZones) > 1 {
+		return "", fmt.Errorf("duplicate zone %s found", zone)
+	} else if len(cmkZones) == 0 {
+		return "", fmt.Errorf("zone %s not found", zone)
+	}
+	return cmkZones[0].Id, nil
 }
 
 func (c *Cmk) ValidateDomainPresent(ctx context.Context, domain string) (v1alpha1.CloudStackResourceIdentifier, error) {
@@ -265,10 +260,10 @@ func (c *Cmk) ValidateDomainPresent(ctx context.Context, domain string) (v1alpha
 	return domainIdentifier, nil
 }
 
-func (c *Cmk) ValidateNetworkPresent(ctx context.Context, domainId string, zone v1alpha1.CloudStackZone, zones []v1alpha1.CloudStackResourceIdentifier, account string, multipleZone bool) error {
+func (c *Cmk) ValidateNetworkPresent(ctx context.Context, domainId string, network v1alpha1.CloudStackResourceIdentifier, zoneId string, account string, multipleZone bool) error {
 	command := newCmkCommand("list networks")
-	if len(zone.Network.Id) > 0 {
-		applyCmkArgs(&command, withCloudStackId(zone.Network.Id))
+	if len(network.Id) > 0 {
+		applyCmkArgs(&command, withCloudStackId(network.Id))
 	}
 	if multipleZone {
 		applyCmkArgs(&command, withCloudStackNetworkType(Shared))
@@ -281,26 +276,16 @@ func (c *Cmk) ValidateNetworkPresent(ctx context.Context, domainId string, zone
 			applyCmkArgs(&command, withCloudStackAccount(account))
 		}
 	}
-	var zoneId string
-	var err error
-	if len(zone.Id) > 0 {
-		zoneId = zone.Id
-	} else {
-		zoneId, err = getZoneIdByName(zones, zone.Name)
-		if err != nil {
-			return fmt.Errorf("getting zone id by name %s: %v", zone.Name, err)
-		}
-	}
 	applyCmkArgs(&command, withCloudStackZoneId(zoneId))
 	result, err := c.exec(ctx, command...)
 	if err != nil {
 		return fmt.Errorf("getting network info - %s: %v", result.String(), err)
 	}
 	if result.Len() == 0 {
 		if multipleZone {
-			return fmt.Errorf("%s network %s not found in zone %s", Shared, zone.Network, zone)
+			return fmt.Errorf("%s network %s not found in zone %s", Shared, network, zoneId)
 		} else {
-			return fmt.Errorf("network %s not found in zone %s", zone.Network, zone)
+			return fmt.Errorf("network %s not found in zone %s", network, zoneId)
 		}
 	}
 
@@ -316,36 +301,27 @@ func (c *Cmk) ValidateNetworkPresent(ctx context.Context, domainId string, zone
 	// if network id and name are both provided, the following code is to confirm name matches return value retrieved by id.
 	// if only name is provided, the following code is to only get networks with specified name.
 
-	if len(zone.Network.Name) > 0 {
+	if len(network.Name) > 0 {
 		networks = []cmkResourceIdentifier{}
 		for _, net := range response.CmkNetworks {
-			if net.Name == zone.Network.Name {
+			if net.Name == network.Name {
 				networks = append(networks, net)
 			}
 		}
 	}
 
 	if len(networks) > 1 {
-		return fmt.Errorf("duplicate network %s found", zone.Network)
+		return fmt.Errorf("duplicate network %s found", network)
 	} else if len(networks) == 0 {
 		if multipleZone {
-			return fmt.Errorf("%s network %s not found in zoneRef %s", Shared, zone.Network, zone)
+			return fmt.Errorf("%s network %s not found in zoneRef %s", Shared, network, zoneId)
 		} else {
-			return fmt.Errorf("network %s not found in zoneRef %s", zone.Network, zone)
+			return fmt.Errorf("network %s not found in zoneRef %s", network, zoneId)
 		}
 	}
 	return nil
 }
 
-func getZoneIdByName(zones []v1alpha1.CloudStackResourceIdentifier, zoneName string) (string, error) {
-	for _, zoneIdentifier := range zones {
-		if zoneName == zoneIdentifier.Name {
-			return zoneIdentifier.Id, nil
-		}
-	}
-	return "", fmt.Errorf("zoneId not found for zone %s", zoneName)
-}
-
 func (c *Cmk) ValidateAccountPresent(ctx context.Context, account string, domainId string) error {
 	command := newCmkCommand("list accounts")
 	applyCmkArgs(&command, withCloudStackName(account), withCloudStackDomainId(domainId))

pkg/executables/cmk_test.go

@@ -447,7 +447,7 @@ func TestCmkListOperations(t *testing.T) {
 				"list", "zones", fmt.Sprintf("name=\"%s\"", resourceName.Name),
 			},
 			cmkFunc: func(cmk executables.Cmk, ctx context.Context) error {
-				_, err := cmk.ValidateZonesPresent(ctx, []v1alpha1.CloudStackZone{zones[0]})
+				_, err := cmk.ValidateZonePresent(ctx, zones[0])
 				return err
 			},
 			cmkResponseError:      nil,
@@ -463,7 +463,7 @@ func TestCmkListOperations(t *testing.T) {
 				"list", "zones", fmt.Sprintf("id=\"%s\"", resourceId.Id),
 			},
 			cmkFunc: func(cmk executables.Cmk, ctx context.Context) error {
-				_, err := cmk.ValidateZonesPresent(ctx, []v1alpha1.CloudStackZone{zones[2]})
+				_, err := cmk.ValidateZonePresent(ctx, zones[2])
 				return err
 			},
 			cmkResponseError:      nil,
@@ -479,7 +479,7 @@ func TestCmkListOperations(t *testing.T) {
 				"list", "zones", fmt.Sprintf("name=\"%s\"", resourceName.Name),
 			},
 			cmkFunc: func(cmk executables.Cmk, ctx context.Context) error {
-				_, err := cmk.ValidateZonesPresent(ctx, zones)
+				_, err := cmk.ValidateZonePresent(ctx, zones[0])
 				return err
 			},
 			cmkResponseError:      nil,
@@ -495,7 +495,7 @@ func TestCmkListOperations(t *testing.T) {
 				"list", "zones", fmt.Sprintf("name=\"%s\"", resourceName.Name),
 			},
 			cmkFunc: func(cmk executables.Cmk, ctx context.Context) error {
-				_, err := cmk.ValidateZonesPresent(ctx, zones)
+				_, err := cmk.ValidateZonePresent(ctx, zones[0])
 				return err
 			},
 			cmkResponseError:      nil,
@@ -511,7 +511,7 @@ func TestCmkListOperations(t *testing.T) {
 				"list", "networks", fmt.Sprintf("domainid=\"%s\"", domainId), fmt.Sprintf("account=\"%s\"", accountName), fmt.Sprintf("zoneid=\"%s\"", "TEST_RESOURCE"),
 			},
 			cmkFunc: func(cmk executables.Cmk, ctx context.Context) error {
-				return cmk.ValidateNetworkPresent(ctx, domainId, zones[2], []v1alpha1.CloudStackResourceIdentifier{}, accountName, false)
+				return cmk.ValidateNetworkPresent(ctx, domainId, zones[2].Network, zones[2].Id, accountName, false)
 			},
 			cmkResponseError:      nil,
 			wantErr:               false,
@@ -526,7 +526,7 @@ func TestCmkListOperations(t *testing.T) {
 				"list", "networks", fmt.Sprintf("id=\"%s\"", resourceId.Id), fmt.Sprintf("domainid=\"%s\"", domainId), fmt.Sprintf("account=\"%s\"", accountName), fmt.Sprintf("zoneid=\"%s\"", "TEST_RESOURCE"),
 			},
 			cmkFunc: func(cmk executables.Cmk, ctx context.Context) error {
-				return cmk.ValidateNetworkPresent(ctx, domainId, zones[3], []v1alpha1.CloudStackResourceIdentifier{}, accountName, false)
+				return cmk.ValidateNetworkPresent(ctx, domainId, zones[3].Network, zones[3].Id, accountName, false)
 			},
 			cmkResponseError:      nil,
 			wantErr:               false,
@@ -541,7 +541,7 @@ func TestCmkListOperations(t *testing.T) {
 				"list", "networks", fmt.Sprintf("domainid=\"%s\"", domainId), fmt.Sprintf("account=\"%s\"", accountName), fmt.Sprintf("zoneid=\"%s\"", "TEST_RESOURCE"),
 			},
 			cmkFunc: func(cmk executables.Cmk, ctx context.Context) error {
-				return cmk.ValidateNetworkPresent(ctx, domainId, zones[2], []v1alpha1.CloudStackResourceIdentifier{}, accountName, false)
+				return cmk.ValidateNetworkPresent(ctx, domainId, zones[2].Network, zones[2].Id, accountName, false)
 			},
 			cmkResponseError:      nil,
 			wantErr:               true,
@@ -556,7 +556,7 @@ func TestCmkListOperations(t *testing.T) {
 				"list", "networks", fmt.Sprintf("domainid=\"%s\"", domainId), fmt.Sprintf("account=\"%s\"", accountName), fmt.Sprintf("zoneid=\"%s\"", "TEST_RESOURCE"),
 			},
 			cmkFunc: func(cmk executables.Cmk, ctx context.Context) error {
-				return cmk.ValidateNetworkPresent(ctx, domainId, zones[2], []v1alpha1.CloudStackResourceIdentifier{}, accountName, false)
+				return cmk.ValidateNetworkPresent(ctx, domainId, zones[2].Network, zones[2].Id, accountName, false)
 			},
 			cmkResponseError:      nil,
 			wantErr:               true,

pkg/providers/cloudstack/cloudstack_test.go

@@ -62,7 +62,7 @@ func givenWildcardCmk(mockCtrl *gomock.Controller) ProviderCmkClient {
 	cmk.EXPECT().ValidateTemplatePresent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
 	cmk.EXPECT().ValidateServiceOfferingPresent(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
 	cmk.EXPECT().ValidateDiskOfferingPresent(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
-	cmk.EXPECT().ValidateZonesPresent(gomock.Any(), gomock.Any()).AnyTimes()
+	cmk.EXPECT().ValidateZonePresent(gomock.Any(), gomock.Any()).AnyTimes()
 	cmk.EXPECT().ValidateAffinityGroupsPresent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
 	cmk.EXPECT().ValidateCloudStackConnection(gomock.Any()).AnyTimes()
 	cmk.EXPECT().ValidateDomainPresent(gomock.Any(), gomock.Any()).AnyTimes()

pkg/providers/cloudstack/decoder/decoder.go

@@ -31,16 +31,12 @@ func ParseCloudStackSecret() (*CloudStackExecConfig, error) {
 		return nil, fmt.Errorf("failed to extract values from %s with ini: %v", EksacloudStackCloudConfigB64SecretKey, err)
 	}
 
-	foundGlobalSection := false
 	cloudstackProfiles := []CloudStackProfileConfig{}
 	sections := cfg.Sections()
 	for _, section := range sections {
 		if section.Name() == "DEFAULT" {
 			continue
 		}
-		if section.Name() == CloudStackGlobalAZ {
-			foundGlobalSection = true
-		}
 
 		apiKey, err := section.GetKey("api-key")
 		if err != nil {
@@ -74,10 +70,6 @@ func ParseCloudStackSecret() (*CloudStackExecConfig, error) {
 		return nil, fmt.Errorf("no instance found from %s", EksacloudStackCloudConfigB64SecretKey)
 	}
 
-	if !foundGlobalSection {
-		return nil, fmt.Errorf("[Global] section not found from %s", EksacloudStackCloudConfigB64SecretKey)
-	}
-
 	return &CloudStackExecConfig{
 		Profiles: cloudstackProfiles,
 	}, nil

pkg/providers/cloudstack/decoder/decoder_test.go

@@ -108,11 +108,6 @@ func TestCloudStackConfigDecoder(t *testing.T) {
 				},
 			},
 		},
-		{
-			name:       "Missing global section",
-			configFile: "../testdata/cloudstack_config_missing_global_section.ini",
-			wantErr:    true,
-		},
 		{
 			name:       "Invalid INI format",
 			configFile: "../testdata/cloudstack_config_invalid_format.ini",

pkg/providers/cloudstack/testdata/cloudstack_config_invalid_format.ini

@@ -2,4 +2,4 @@
 verify-ssl ; false
 api-key ; test-key1
 secret-key ; test-secret1
-api-url ; http://127.16.0.1:8080/client/api
+api-url ; http://127.16.0.1:8080/client/api

pkg/providers/cloudstack/testdata/cloudstack_config_invalid_verifyssl.ini

@@ -2,4 +2,4 @@
 verify-ssl = xxx
 api-key = test-key1
 secret-key = test-secret1
-api-url = http://127.16.0.1:8080/client/api
+api-url = http://127.16.0.1:8080/client/api

pkg/providers/cloudstack/testdata/cloudstack_config_missing_apikey.ini

@@ -1,4 +1,4 @@
 [Global]
 verify-ssl = false
 secret-key = test-secret1
-api-url = http://127.16.0.1:8080/client/api
+api-url = http://127.16.0.1:8080/client/api

pkg/providers/cloudstack/testdata/cloudstack_config_missing_apiurl.ini

@@ -1,4 +1,4 @@
 [Global]
 verify-ssl = false
 api-key = test-key1
-secret-key = test-secret1
+secret-key = test-secret1

pkg/providers/cloudstack/testdata/cloudstack_config_missing_secretkey.ini

@@ -1,4 +1,4 @@
 [Global]
 verify-ssl = false
 api-key = test-key1
-api-url = http://127.16.0.1:8080/client/api
+api-url = http://127.16.0.1:8080/client/api

pkg/providers/cloudstack/testdata/cloudstack_config_missing_verifyssl.ini

@@ -1,4 +1,4 @@
 [Global]
 api-key = test-key1
 secret-key = test-secret1
-api-url = http://127.16.0.1:8080/client/api
+api-url = http://127.16.0.1:8080/client/api

pkg/providers/cloudstack/testdata/cloudstack_config_multiple_profiles.ini

@@ -8,4 +8,4 @@ api-url = http://127.16.0.1:8080/client/api
 verify-ssl = true
 api-key = test-key2
 secret-key = test-secret2
-api-url = http://127.16.0.2:8080/client/api
+api-url = http://127.16.0.2:8080/client/api

pkg/providers/cloudstack/testdata/cloudstack_config_valid.ini

@@ -2,4 +2,4 @@
 verify-ssl = false
 api-key = test-key1
 secret-key = test-secret1
-api-url = http://127.16.0.1:8080/client/api
+api-url = http://127.16.0.1:8080/client/api