implement multi region support for cpa #289

shenmo3 · 2023-08-03T22:56:53Z

Description

This PR adds support for multi region configuration in CloudProviderAccount CRD.

Changes

Change account config to have multiple service configs based on regions.
Modify webhook to validate all regions in the account config. For AWS it checks if region is supported or not, for Azure it only checks if its empty or not. Also added region validation in service config creation, where both AWS and Azure are checked if region is supported.
Adding handling to create AWS service config for each region and single Azure service config with multi-region filter.
Change account operations to support multiple service configs and run in parallel, such as inventory poll and cloud sync.
Add multi error in multi region operations, so success regions will be processed and error regions will be reported separately.
Change security group operations to retrieve regions using vpc/vnet IDs instead of from cloud account.
Change Azure cloud sync process to use ASG ID internally when syncing to avoid ASGs with the same name in different region being grouped into one.
Update unit-tests to work with multi region.

Anandkumar26 · 2023-08-06T16:06:15Z

pkg/cloudprovider/plugins/aws/aws_account_mgmt.go

 	if strings.Compare(existingConfig.endpoint, newConfig.endpoint) != 0 {
 		credsChanged = true
 		awsPluginLogger().Info("Endpoint url updated", "account", accountName)
 	}
+	sort.Strings(existingConfig.regions)
+	sort.Strings(newConfig.regions)


Check if we need to convert newConfig.regions to lowercase or not?

I actually added to lower in aws_account_impl.go and azure_account_impl.go. This converts regions to lower before any processing so we don't need to worry about to lower in actual logics. However regions from the cloud, either in VM, VNET or region list still needs to be converted to lower.

Anandkumar26 · 2023-08-06T16:09:37Z

pkg/cloudprovider/plugins/aws/aws_ec2.go

@@ -200,11 +209,7 @@ func (ec2Cfg *ec2ServiceConfig) getVpcPeers(vpcId string) []string {
 // getInstances gets instances from cloud matching the given selector configuration.
 func (ec2Cfg *ec2ServiceConfig) getInstances(namespacedName *types.NamespacedName) ([]*ec2.Instance, error) {
 	var instances []*ec2.Instance
-	filters, found := ec2Cfg.instanceFilters[*namespacedName]
-	if found && len(filters) != 0 {
-		awsPluginLogger().V(1).Info("Fetching vm resources from cloud", "account", ec2Cfg.accountNamespacedName,


any specific reason as to why you removed this log message?

Anandkumar26 · 2023-08-08T14:38:40Z

pkg/apiserver/webhook/cloudprovideraccount_webhook.go

-		for key := range regions {
-			supportedRegions = append(supportedRegions, key)
+	awsRegionMap := endpoints.AwsPartition().Regions()
+	awsRegionMap["all"] = endpoints.Region{}


We are not supporting the keyword "all" for now. Please remove.

Anandkumar26 · 2023-08-08T14:49:11Z

pkg/cloudprovider/plugins/aws/aws_security_impl.go

@@ -37,7 +37,11 @@ func (c *awsCloud) CreateSecurityGroup(securityGroupIdentifier *cloudresource.Cl
 	defer accCfg.UnlockMutex()

 	cloudSgName := securityGroupIdentifier.GetCloudName(membershipOnly)
-	ec2Service := accCfg.GetServiceConfig().(*ec2ServiceConfig)
+	region := accCfg.FindRegion(vpcID)


May be return an error, rather than relying on empty value.

Anandkumar26 · 2023-08-09T13:53:50Z

pkg/accountmanager/manager.go

@@ -223,7 +223,9 @@ func (a *AccountManager) RemoveResourceFiltersFromAccount(accNamespacedName *typ
 		return fmt.Errorf(fmt.Sprintf("failed to delete selector %v, account %v: %v",
 			selectorNamespacedName, accNamespacedName, errorMsgAccountPollerNotFound))
 	}
+	accPoller.mutex.Lock()


Do we really need to lock here?
Since we are not modifying the accPoller structure, we are accessing vmStore

As we discussed offline, there is a race condition that needs this lock to be avoided. Will move the lock to the top of the func to make sure we are in lock when modifying anything.

Anandkumar26 · 2023-08-09T13:53:55Z

pkg/accountmanager/manager.go

@@ -285,7 +287,9 @@ func (a *AccountManager) removeAccountPoller(namespacedName *types.NamespacedNam
 	delete(a.accPollers, *namespacedName)

 	accPoller.stopPoller()
+	accPoller.mutex.Lock()


same as above

This part actually doesn't need a lock since the poller is already stopped, will remove it.

Anandkumar26 · 2023-08-09T13:57:35Z

pkg/accountmanager/poller.go

@@ -145,7 +145,7 @@ func (p *accountPoller) doAccountPolling() {
 		p.updateAccountStatus(p.cloudInterface)
 	}()

-	cloudInventory, err := p.cloudInterface.GetCloudInventory(p.accountNamespacedName)
+	cloudInventory, err := p.cloudInterface.GetAccountCloudInventory(p.accountNamespacedName)


Any specific reason for changing the function name?
Seems a bit off while reading it.
If you want to have account, then may be try GetCloudInventoryByAccout()

I changed it because service config also has func GetCloudInventory, but thats just for a single service config. Since account config can now have multiple service configs and this func combines all service configs' inventory, I want to change this func name to differentiate the two.

Don't quite like ByAccount also, since we are already got a specific account config, how about GetCloudInventoryForAccount?

Anandkumar26 · 2023-08-09T14:02:09Z

pkg/cloudprovider/plugins/aws/aws_account_impl.go

@@ -23,6 +25,9 @@ import (

 // AddProviderAccount adds and initializes given account of a cloud provider.
 func (c *awsCloud) AddProviderAccount(client client.Client, account *crdv1alpha1.CloudProviderAccount) error {
+	for idx := range account.Spec.AWSConfig.Region {


Modifying the retrieved CR here, seems inappropriate.
Going forward, we may consume the region field before calling this function.

Anandkumar26 · 2023-08-09T14:06:32Z

pkg/cloudprovider/plugins/aws/aws_api_wrappers.go

+
+// getRegion returns the region of the ec2 client.
+func (ec2Wrapper *awsEC2WrapperImpl) getRegion() string {
+	return *ec2Wrapper.ec2.Config.Region


Since you are dereferencing the pointer, check if there id any scenario when region could be empty.

I'll add a check, but for AWS clients, the region is a required field.

Anandkumar26 · 2023-08-09T14:07:30Z

pkg/cloudprovider/plugins/aws/aws_cloudcommonhelper_impl.go

+	return setAccountConfig
+}
+
+func (h *awsCloudCommonHelperImpl) GetCloudConfigComparatorFunc() internal.CloudConfigComparatorFunc {


nit:

Suggested change

func (h *awsCloudCommonHelperImpl) GetCloudConfigComparatorFunc() internal.CloudConfigComparatorFunc {

func (h *awsCloudCommonHelperImpl) GetAccountConfigComparatorFunc() internal.CloudConfigComparatorFunc {

Anandkumar26 · 2023-08-09T14:09:10Z

pkg/cloudprovider/plugins/aws/aws_ec2.go

+		// create ec2 sdk api client.
+		apiClient, err := service.compute()
+		if err != nil {
+			err = fmt.Errorf("error creating ec2 sdk api client for account: %s, region: %s, err: %v", accountNamespacedName.String(),


Can you try/simulae a scenario where this function returns a multi error (i.e. multiple errors) and check how it would look on the CPA status field?

Anandkumar26 · 2023-08-09T14:13:35Z

pkg/cloudprovider/plugins/aws/aws_services.go


-	awsServiceClientCreator, err := awsServicesHelper.newServiceSdkConfigProvider(awsAccountCredentials)
+	// check regions valid and process regions.


Suggested change

// check regions valid and process regions.

// check valid regions.

Anandkumar26 · 2023-08-09T14:20:42Z

pkg/cloudprovider/plugins/azure/azure_api_wrappers.go

+	for listResultIterator.More() {
+		nextResult, err := listResultIterator.NextPage(ctx)
+		if err != nil {
+			return nil, fmt.Errorf("failed to iterate list of asgs, reason %v", err)


nit: instead of asgs, location

github-actions · 2023-11-08T00:16:36Z

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

Signed-off-by: Alexander Liu <alliu@vmware.com> Signed-off-by: alliu <alliu@vmware.com>

shenmo3 · 2023-11-16T18:53:34Z

/nephe-test-e2e-all

reachjainrahul

/LGTM

shenmo3 requested review from Anandkumar26 and reachjainrahul August 3, 2023 22:56

shenmo3 self-assigned this Aug 3, 2023

shenmo3 force-pushed the multi-region branch 2 times, most recently from 531e968 to 3672cdc Compare August 7, 2023 07:35

Anandkumar26 reviewed Aug 7, 2023

View reviewed changes

Anandkumar26 reviewed Aug 8, 2023

View reviewed changes

shenmo3 force-pushed the multi-region branch 2 times, most recently from d30566b to 754cf3e Compare August 9, 2023 07:14

Anandkumar26 reviewed Aug 9, 2023

View reviewed changes

shenmo3 marked this pull request as ready for review August 9, 2023 16:28

github-actions bot added the lifecycle/stale label Nov 8, 2023

shenmo3 force-pushed the multi-region branch from 754cf3e to 979d843 Compare November 13, 2023 22:28

shenmo3 removed the lifecycle/stale label Nov 13, 2023

implement multi region support for cpa

95a121d

Signed-off-by: Alexander Liu <alliu@vmware.com> Signed-off-by: alliu <alliu@vmware.com>

shenmo3 force-pushed the multi-region branch from 979d843 to 95a121d Compare November 16, 2023 18:40

reachjainrahul approved these changes Dec 1, 2023

View reviewed changes

reachjainrahul merged commit 9faeba2 into main Dec 1, 2023

reachjainrahul deleted the multi-region branch December 1, 2023 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement multi region support for cpa #289

implement multi region support for cpa #289

shenmo3 commented Aug 3, 2023 •

edited

Loading

Anandkumar26 Aug 6, 2023

shenmo3 Aug 7, 2023

Anandkumar26 Aug 6, 2023

Anandkumar26 Aug 8, 2023

shenmo3 Aug 9, 2023

Anandkumar26 Aug 8, 2023

Anandkumar26 Aug 9, 2023

shenmo3 Aug 9, 2023

Anandkumar26 Aug 9, 2023

shenmo3 Aug 9, 2023

Anandkumar26 Aug 9, 2023

shenmo3 Aug 9, 2023

Anandkumar26 Aug 9, 2023

Anandkumar26 Aug 9, 2023

shenmo3 Aug 9, 2023

Anandkumar26 Aug 9, 2023

Anandkumar26 Aug 9, 2023

Anandkumar26 Aug 9, 2023

Anandkumar26 Aug 9, 2023

github-actions bot commented Nov 8, 2023

shenmo3 commented Nov 16, 2023

reachjainrahul left a comment

	func (h *awsCloudCommonHelperImpl) GetCloudConfigComparatorFunc() internal.CloudConfigComparatorFunc {
	func (h *awsCloudCommonHelperImpl) GetAccountConfigComparatorFunc() internal.CloudConfigComparatorFunc {


		awsServiceClientCreator, err := awsServicesHelper.newServiceSdkConfigProvider(awsAccountCredentials)
		// check regions valid and process regions.

	// check regions valid and process regions.
	// check valid regions.

implement multi region support for cpa #289

implement multi region support for cpa #289

Conversation

shenmo3 commented Aug 3, 2023 • edited Loading

Description

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 8, 2023

shenmo3 commented Nov 16, 2023

reachjainrahul left a comment

Choose a reason for hiding this comment

shenmo3 commented Aug 3, 2023 •

edited

Loading