Repository: chanzuckerberg/fogg
Pull request #564 (2021-04-01 01:12:49)

feature: allow multiple aws providers
### Summary
We want to be able to configure multiple AWS providers, for multiple accounts and regions within a same component. This code introduces a new configuration option to the aws provider block that allows us to do so. I know this config structure is not ideal and kind of painful but seems like an ok compromise until the time we decide to introduce a new config version.

```yaml
accounts:
  bar:
    providers:
      aws:
        account_id: 456
        additional_regions:
          - us-east-1
          - us-east-2
        role: foo
        additional_providers:
          another_account_different_role:
            account_id: 789
            role: different_role
          another_account_same_role:
            account_id: different_account
          another_account_more_regions:
            account_id: another_different_account
            additional_regions:
              - eu-west-2
```
produces:
```hcl
provider aws {
  alias  = "another_account_different_role"
  region = "us-west-2"


  assume_role {
    role_arn = "arn:aws:iam::789:role/different_role"
  }

  allowed_account_ids = [789]
}


provider aws {
  alias  = "another_account_different_role-us-east-1"
  region = "us-east-1"


  assume_role {
    role_arn = "arn:aws:iam::789:role/different_role"
  }

  allowed_account_ids = [789]
}
.....
.....
```
### Test Plan
unittests

### References



codecov[bot]: # [Codecov](https://codecov.io/gh/chanzuckerberg/fogg/pull/564?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`main@9a0522b`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/chanzuckerberg/fogg/pull/564/graphs/tree.svg?width=650&height=150&src=pr&token=57DhtZ7g2A)](https://codecov.io/gh/chanzuckerberg/fogg/pull/564?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##             main     #564   +/-   ##
=======================================
  Coverage        ?   55.48%           
=======================================
  Files           ?       21           
  Lines           ?     2069           
  Branches        ?        0           
=======================================
  Hits            ?     1148           
  Misses          ?      763           
  Partials        ?      158           
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/chanzuckerberg/fogg/pull/564?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/chanzuckerberg/fogg/pull/564?src=pr&el=footer). Last update [9a0522b...c003a01](https://codecov.io/gh/chanzuckerberg/fogg/pull/564?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

kuannie1: wait, do we need this change?
kuannie1: seems like there's a failing test. Do you have the bandwidth to look at the test & determine how to fix/tweak it @edulop91 ?
jgadling: Just a note about the behavior here, not sure if it's expected:

```
    providers:
      aws:
        account_id: '123456'
        additional_providers:
          other-account:
            account_id: '789012'
        additional_regions:
          - ap-south-1
          - eu-west-3
```

This produces a set-product of accounts and additional_regions. So both 123456 and 789012 will have providers created for the default region, ap-south-1, and eu-west-3

Whereas this only creates a provider for 123456 in ap-south-1 and eu-west-3:

```
      aws:
        account_id: '123456'
        additional_providers:
          other-account:
            account_id: '789012'
            additional_regions: []
        additional_regions:
          - ap-south-1
          - eu-west-3

```

Patch:
diff --git a/config/v2/config.go b/config/v2/config.go
index f4a74142..f43a79ab 100644
--- a/config/v2/config.go
+++ b/config/v2/config.go
@@ -169,6 +169,10 @@ type AWSProvider struct {
 	Region            *string      `yaml:"region,omitempty"`
 	Role              *string      `yaml:"role,omitempty"` // FIXME validate format
 	Version           *string      `yaml:"version,omitempty"`
+
+	// HACK HACK(el): we can configure additional, aliased, AWS providers for other accounts
+	// 								A map of alias_name to provider configuration
+	AdditionalProviders map[string]*AWSProvider `yaml:"additional_providers,omitempty"`
 }
 
 type GithubProvider struct {
diff --git a/config/v2/resolvers.go b/config/v2/resolvers.go
index fab3d1f4..307b38a8 100644
--- a/config/v2/resolvers.go
+++ b/config/v2/resolvers.go
@@ -85,6 +85,7 @@ func ResolveAWSProvider(commons ...Common) *AWSProvider {
 	var profile, region, role, version *string
 	var accountID *json.Number
 	var additionalRegions []string
+	var additionalProviders map[string]*AWSProvider
 
 	for _, c := range commons {
 		if c.Providers != nil && c.Providers.AWS != nil {
@@ -116,6 +117,10 @@ func ResolveAWSProvider(commons ...Common) *AWSProvider {
 			if p.AdditionalRegions != nil {
 				additionalRegions = p.AdditionalRegions
 			}
+
+			if p.AdditionalProviders != nil {
+				additionalProviders = p.AdditionalProviders
+			}
 		}
 	}
 
@@ -127,8 +132,9 @@ func ResolveAWSProvider(commons ...Common) *AWSProvider {
 			Version: version,
 
 			// optional fields
-			AccountID:         accountID,
-			AdditionalRegions: additionalRegions,
+			AccountID:           accountID,
+			AdditionalRegions:   additionalRegions,
+			AdditionalProviders: additionalProviders,
 		}
 	}
 	return nil
diff --git a/plan/plan.go b/plan/plan.go
index 65396a8b..e9457855 100644
--- a/plan/plan.go
+++ b/plan/plan.go
@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"sort"
 
 	v2 "github.com/chanzuckerberg/fogg/config/v2"
 	"github.com/chanzuckerberg/fogg/errs"
@@ -175,6 +176,23 @@ type AWSProvider struct {
 	RoleArn   *string     `yaml:"role_arn"`
 }
 
+func (a *AWSProvider) String() string {
+	accountID := a.AccountID.String()
+
+	joined := util.JoinStrPointers(
+		"-",
+		&accountID,
+		a.Alias,
+		a.Profile,
+		&a.Region,
+		a.RoleArn,
+	)
+	if joined == nil {
+		return ""
+	}
+	return *joined
+}
+
 // GithubProvider represents a configuration of a github provider
 type GithubProvider struct {
 	Organization string  `yaml:"organization"`
@@ -516,40 +534,82 @@ func (p *Plan) buildEnvs(conf *v2.Config) (map[string]Env, error) {
 	return envPlans, nil
 }
 
-func resolveComponentCommon(commons ...v2.Common) ComponentCommon {
-	providerVersions := copyMap(utilityProviders)
-	var awsPlan *AWSProvider
+func resolveAWSProvider(commons ...v2.Common) (plan *AWSProvider, providers []AWSProvider, version *string) {
 	awsConfig := v2.ResolveAWSProvider(commons...)
-	additionalProviders := []AWSProvider{}
 	var roleArn *string
-	if awsConfig != nil {
-		if awsConfig.Role != nil {
-			tmp := fmt.Sprintf("arn:aws:iam::%s:role/%s", *awsConfig.AccountID, *awsConfig.Role)
-			roleArn = &tmp
+	// nothing to do
+	if awsConfig == nil {
+		return
+	}
+
+	// set the version
+	version = awsConfig.Version
+
+	// configure the main provider
+	if awsConfig.Role != nil {
+		tmp := fmt.Sprintf("arn:aws:iam::%s:role/%s", *awsConfig.AccountID, *awsConfig.Role)
+		roleArn = &tmp
+	}
+	plan = &AWSProvider{
+		AccountID: *awsConfig.AccountID,
+		Profile:   awsConfig.Profile,
+		Region:    *awsConfig.Region,
+		RoleArn:   roleArn,
+	}
+
+	// grab all aliased regions
+	for _, r := range awsConfig.AdditionalRegions {
+		// we have to take a reference here otherwise it gets overwritten by the loop
+		region := r
+		providers = append(providers,
+			AWSProvider{
+				AccountID: *awsConfig.AccountID,
+				Alias:     &region,
+				Profile:   awsConfig.Profile,
+				Region:    region,
+				RoleArn:   roleArn,
+			})
+	}
+
+	//HACK HACK(el): this is horrible: grab all extra accounts and configure aliased providers for them
+	for ap, aws := range awsConfig.AdditionalProviders {
+		aliasPrefix := ap
+		// HACK(el): we create this pseudo v2.Common for each additional provider and do the inheritance
+		pseudoCommon := v2.Common{
+			Providers: &v2.Providers{
+				AWS: aws,
+			},
 		}
-		awsPlan = &AWSProvider{
-			AccountID: *awsConfig.AccountID,
-			Profile:   awsConfig.Profile,
-			Region:    *awsConfig.Region,
-			RoleArn:   roleArn,
+		// set so not nil
+		pseudoCommon.Providers.AWS.AdditionalProviders = map[string]*v2.AWSProvider{}
+
+		extraCommons := append(commons[:], pseudoCommon)
+		extraPlan, extraProviders, _ := resolveAWSProvider(extraCommons...)
+		if extraPlan != nil {
+			extraPlan.Alias = util.JoinStrPointers("-", &aliasPrefix, extraPlan.Alias)
+			providers = append(providers, *extraPlan)
 		}
 
+		for _, eP := range extraProviders {
+			extraProvider := eP
+			extraProvider.Alias = util.JoinStrPointers("-", &aliasPrefix, extraProvider.Alias)
+			providers = append(providers, extraProvider)
+		}
+	}
+
+	sort.Slice(providers, func(i, j int) bool {
+		return providers[i].String() > providers[j].String()
+	})
+	return
+}
+
+func resolveComponentCommon(commons ...v2.Common) ComponentCommon {
+	providerVersions := copyMap(utilityProviders)
+	awsPlan, additionalProviders, awsVersion := resolveAWSProvider(commons...)
+	if awsVersion != nil {
 		providerVersions["aws"] = ProviderVersion{
 			Source:  "hashicorp/aws",
-			Version: awsConfig.Version,
-		}
-
-		for _, r := range awsConfig.AdditionalRegions {
-			// we have to take a reference here otherwise it gets overwritten by the loop
-			region := r
-			additionalProviders = append(additionalProviders,
-				AWSProvider{
-					AccountID: *awsConfig.AccountID,
-					Alias:     &region,
-					Profile:   awsConfig.Profile,
-					Region:    region,
-					RoleArn:   roleArn,
-				})
+			Version: awsVersion,
 		}
 	}
 
diff --git a/util/strings.go b/util/strings.go
index db8383d7..8483fb6e 100644
--- a/util/strings.go
+++ b/util/strings.go
@@ -1,5 +1,7 @@
 package util
 
+import "strings"
+
 func SliceContainsString(haystack []string, needle string) bool {
 	for _, s := range haystack {
 		if s == needle {
@@ -8,3 +10,21 @@ func SliceContainsString(haystack []string, needle string) bool {
 	}
 	return false
 }
+
+func JoinStrPointers(sep string, elems ...*string) *string {
+	nonNil := []string{}
+	for _, elem := range elems {
+		if elem != nil {
+			nonNil = append(nonNil, *elem)
+		}
+	}
+
+	// nothing to join
+	if len(nonNil) == 0 {
+		return nil
+	}
+
+	// join & take ptr
+	out := strings.Join(nonNil, sep)
+	return &out
+}


Test patch:
diff --git a/testdata/v2_full_yaml/fogg.yml b/testdata/v2_full_yaml/fogg.yml
index c1b7ce62..51c02321 100644
--- a/testdata/v2_full_yaml/fogg.yml
+++ b/testdata/v2_full_yaml/fogg.yml
@@ -7,6 +7,16 @@ accounts:
           - us-east-1
           - us-east-2
         role: foo
+        additional_providers:
+          another_account_different_role:
+            account_id: 789
+            role: different_role
+          another_account_same_role:
+            account_id: different_account
+          another_account_more_regions:
+            account_id: another_different_account
+            additional_regions:
+              - eu-west-2
       bless:
         role_arn: arn:aws:iam::1234567890:role/roll
         version: 0.4.2
diff --git a/testdata/v2_full_yaml/terraform/accounts/bar/fogg.tf b/testdata/v2_full_yaml/terraform/accounts/bar/fogg.tf
index c735e00a..9aaa14a2 100644
--- a/testdata/v2_full_yaml/terraform/accounts/bar/fogg.tf
+++ b/testdata/v2_full_yaml/terraform/accounts/bar/fogg.tf
@@ -15,15 +15,106 @@ provider aws {
 
 
 provider aws {
-  alias  = "us-east-1"
+  alias  = "another_account_same_role"
+  region = "us-west-2"
+
+
+  assume_role {
+    role_arn = "arn:aws:iam::different_account:role/foo"
+  }
+
+  allowed_account_ids = [different_account]
+}
+
+
+provider aws {
+  alias  = "another_account_same_role-us-east-2"
+  region = "us-east-2"
+
+
+  assume_role {
+    role_arn = "arn:aws:iam::different_account:role/foo"
+  }
+
+  allowed_account_ids = [different_account]
+}
+
+
+provider aws {
+  alias  = "another_account_same_role-us-east-1"
   region = "us-east-1"
 
 
   assume_role {
-    role_arn = "arn:aws:iam::456:role/foo"
+    role_arn = "arn:aws:iam::different_account:role/foo"
   }
 
-  allowed_account_ids = [456]
+  allowed_account_ids = [different_account]
+}
+
+
+provider aws {
+  alias  = "another_account_more_regions"
+  region = "us-west-2"
+
+
+  assume_role {
+    role_arn = "arn:aws:iam::another_different_account:role/foo"
+  }
+
+  allowed_account_ids = [another_different_account]
+}
+
+
+provider aws {
+  alias  = "another_account_more_regions-eu-west-2"
+  region = "eu-west-2"
+
+
+  assume_role {
+    role_arn = "arn:aws:iam::another_different_account:role/foo"
+  }
+
+  allowed_account_ids = [another_different_account]
+}
+
+
+provider aws {
+  alias  = "another_account_different_role"
+  region = "us-west-2"
+
+
+  assume_role {
+    role_arn = "arn:aws:iam::789:role/different_role"
+  }
+
+  allowed_account_ids = [789]
+}
+
+
+provider aws {
+  alias  = "another_account_different_role-us-east-2"
+  region = "us-east-2"
+
+
+  assume_role {
+    role_arn = "arn:aws:iam::789:role/different_role"
+  }
+
+  allowed_account_ids = [789]
+}
+
+
+provider aws {
+  alias  = "another_account_different_role-us-east-1"
+  region = "us-east-1"
+
+
+  assume_role {
+    role_arn = "arn:aws:iam::789:role/different_role"
+  }
+
+  allowed_account_ids = [789]
 }
 
 
@@ -40,6 +131,19 @@ provider aws {
 }
 
 
+provider aws {
+  alias  = "us-east-1"
+  region = "us-east-1"
+
+
+  assume_role {
+    role_arn = "arn:aws:iam::456:role/foo"
+  }
+
+  allowed_account_ids = [456]
+}
+
+
 provider bless {
   region   = "us-west-2"
   role_arn = "arn:aws:iam::1234567890:role/roll"