From bb88bacabca812153f1f85983be16ceaa70d7239 Mon Sep 17 00:00:00 2001 From: zackhee997 <162941009+zackhee997@users.noreply.github.com> Date: Tue, 21 May 2024 16:46:57 +0800 Subject: [PATCH] fix: generic provider format (#303) * fix: generic provider roleARN format * fix: generic provider missing region * chore: Add docs and fix testdata * chore: Refactor generic provider assume_role map support - Align closer to TF assume role block to expect map in both cases - Add genericProvider validation to component validation - Add tests for genericProvider validation function * chore: don't override region if provided --------- Co-authored-by: Vincent De Smet --- config/v2/resolvers.go | 27 +++++-- config/v2/validation.go | 14 +++- config/v2/validation_test.go | 76 +++++++++++++++++++ testdata/generic_providers_yaml/fogg.yml | 13 +++- .../terraform/envs/prd/network/fogg.tf | 19 ++++- .../terraform/envs/stg/network/fogg.tf | 6 ++ .../terraform/global/fogg.tf | 6 ++ 7 files changed, 151 insertions(+), 10 deletions(-) diff --git a/config/v2/resolvers.go b/config/v2/resolvers.go index 2f9ee0ac2..0cdc41d7e 100644 --- a/config/v2/resolvers.go +++ b/config/v2/resolvers.go @@ -810,19 +810,36 @@ func resolveGenericProvider( if p.Enabled != nil { enabled = *p.Enabled } + // special assume role config block handling + // if assume_role map is provided, add region + requiresRegion := false for key, value := range p.Config { if value == nil { delete(config, key) } else { - // specially for AWS associate assume role - if key == "assume_role" { - tmp := fmt.Sprintf("arn:aws:iam::%s:role/%s", *awsConfig.AccountID, value) - config["assume_role"] = tmp - } else { + switch key { + case "assume_role": + // build assume_role_block + // ref: https://registry.terraform.io/providers/hashicorp/awscc/latest/docs#assume-role + // ValidateAWSProvider should ensure AccountID is not nil + // GenericProvider.Validate ensures can cast and "role" key exists + valueMap := value.(map[string]any) + if valueMap["role_arn"] == nil { + valueMap["role_arn"] = fmt.Sprintf("arn:aws:iam::%s:role/%s", *awsConfig.AccountID, valueMap["role"]) + delete(valueMap, "role") + } + config[key] = valueMap + requiresRegion = true + default: config[key] = value } } } + if requiresRegion && config["region"] == nil { + // inherit resolved awsConfig region and accountID + // TODO: handle additionalRegions/additionalProvider configuration? + config["region"] = *awsConfig.Region + } return source, customProvider, version, enabled } diff --git a/config/v2/validation.go b/config/v2/validation.go index 85659b1ec..67836a52c 100644 --- a/config/v2/validation.go +++ b/config/v2/validation.go @@ -50,6 +50,7 @@ func (c *Config) Validate() ([]string, error) { errs = multierror.Append(errs, c.ValidateSnowflakeProviders()) errs = multierror.Append(errs, c.ValidateBlessProviders()) errs = multierror.Append(errs, c.ValidateOktaProviders()) + errs = multierror.Append(errs, c.ValidateGenericProviders()) errs = multierror.Append(errs, c.validateModules()) errs = multierror.Append(errs, c.ValidateTravis()) errs = multierror.Append(errs, c.ValidateGithubActionsCI()) @@ -200,7 +201,16 @@ func (p *GenericProvider) Validate(name string, component string) error { if p.Version == nil || len(*p.Version) == 0 { errs = multierror.Append(errs, fmt.Errorf("required provider %q requires version in %s", name, component)) } - return errs + if p.Config["assume_role"] != nil { + // "role" key must be provided in assume_role map + valueMap, ok := p.Config["assume_role"].(map[string]any) + if !ok { + errs = multierror.Append(errs, fmt.Errorf("required provider %q requires 'assume_role' to be map (cast failed) in %s", name, component)) + } else if valueMap["role"] == nil && valueMap["role_arn"] == nil { + errs = multierror.Append(errs, fmt.Errorf("required provider %q requires 'assume_role' map to contain at least 'role' key in %s (received: %v)", name, component, valueMap)) + } + } + return errs.ErrorOrNil() } func (p *SnowflakeProvider) Validate(component string) error { @@ -277,7 +287,7 @@ func (c *Config) ValidateGenericProviders() error { } } }) - return errs + return errs.ErrorOrNil() } func (c *Config) ValidateTravis() error { diff --git a/config/v2/validation_test.go b/config/v2/validation_test.go index 77620342a..a1b858a49 100644 --- a/config/v2/validation_test.go +++ b/config/v2/validation_test.go @@ -269,6 +269,82 @@ func TestValidateAWSProvider(t *testing.T) { } } +func TestValidateGenericProvider(t *testing.T) { + validGeneric := &GenericProvider{ + Source: "foo", + CommonProvider: CommonProvider{ + Version: util.StrPtr("1.1.1"), + }, + } + validAssumeRole := &GenericProvider{ + Source: "foo", + CommonProvider: CommonProvider{ + Version: util.StrPtr("1.1.1"), + }, + Config: map[string]any{ + "assume_role": map[string]any{ + "role": "foo", + }, + }, + } + + invalidNothing := &GenericProvider{} + invalidSource := &GenericProvider{ + Source: "", + } + invalidAssumeRoleNothing := &GenericProvider{ + Source: "foo", + CommonProvider: CommonProvider{ + Version: util.StrPtr("1.1.1"), + }, + Config: map[string]any{ + "assume_role": map[string]string{}, + }, + } + invalidAssumeRoleConfig := &GenericProvider{ + Source: "foo", + CommonProvider: CommonProvider{ + Version: util.StrPtr("1.1.1"), + }, + Config: map[string]any{ + "assume_role": map[string]string{ + "foo": "bar", + }, + }, + } + + type args struct { + p *GenericProvider + component string + } + tests := []struct { + name string + args args + wantErr bool + }{ + {"valid generic", args{validGeneric, "valid"}, false}, + {"invalid generic nothing", args{invalidNothing, "invalid"}, true}, + {"invalid generic source", args{invalidSource, "invalid"}, true}, + {"valid generic assume_role", args{validAssumeRole, "valid-role"}, false}, + {"invalid generic assume_role nothing", args{invalidAssumeRoleNothing, "invalid-nothing"}, true}, + {"invalid generic assume_role missing role", args{invalidAssumeRoleConfig, "invalid-role-missing"}, true}, + } + for _, test := range tests { + tt := test + t.Run(tt.name, func(t *testing.T) { + assert := require.New(t) + err := tt.args.p.Validate("generic", tt.args.component) + + if tt.wantErr { + assert.Error(err) + } else { + assert.NoError(err) + } + // t.Errorf("genericProvider.Validate() error = %v, wantErr %v", err, tt.wantErr) + }) + } +} + func TestConfig_ValidateAWSProviders(t *testing.T) { tests := []struct { fileName string diff --git a/testdata/generic_providers_yaml/fogg.yml b/testdata/generic_providers_yaml/fogg.yml index 3a61ded2b..1116676e5 100644 --- a/testdata/generic_providers_yaml/fogg.yml +++ b/testdata/generic_providers_yaml/fogg.yml @@ -28,6 +28,9 @@ defaults: bar: version: "~> 0.1.0" source: "czi/bar" + fred: + version: "~> 0.1.0" + source: "czi/bar" qux: version: "~> 0.1.0" source: "czi/qux" @@ -48,7 +51,15 @@ envs: custom_provider: false config: baz_token: prod_token_arn - aws_assume_role: "TerraformExecutionRole" + assume_role: + role: TerraformExecutionRole + session_name: "foo" + fred: + custom_provider: false + config: + assume_role: + role_arn: arn:aws:iam::1111111111111111:role/TerraformExecutionRole + session_name: "foo" components: network: {} stg: diff --git a/testdata/generic_providers_yaml/terraform/envs/prd/network/fogg.tf b/testdata/generic_providers_yaml/terraform/envs/prd/network/fogg.tf index 578d3e395..ddcf1129c 100644 --- a/testdata/generic_providers_yaml/terraform/envs/prd/network/fogg.tf +++ b/testdata/generic_providers_yaml/terraform/envs/prd/network/fogg.tf @@ -20,13 +20,24 @@ provider "sops" {} provider "bar" { } provider "baz" { - aws_assume_role = "TerraformExecutionRole" - baz_token = "prod_token_arn" + assume_role = { + role_arn = "arn:aws:iam::0000000000000000:role/TerraformExecutionRole" + session_name = "foo" + } + baz_token = "prod_token_arn" + region = "ap-southeast-1" } provider "foo" { foo_host = "prod" foo_tls = true } +provider "fred" { + assume_role = { + role_arn = "arn:aws:iam::1111111111111111:role/TerraformExecutionRole" + session_name = "foo" + } + region = "ap-southeast-1" +} terraform { required_version = "=1.1.1" @@ -66,6 +77,10 @@ terraform { source = "czi/foo" version = "~> 0.2" } + fred = { + source = "czi/bar" + version = "~> 0.1.0" + } local = { source = "hashicorp/local" version = "~> 2.0" diff --git a/testdata/generic_providers_yaml/terraform/envs/stg/network/fogg.tf b/testdata/generic_providers_yaml/terraform/envs/stg/network/fogg.tf index 5cd355d50..02f0870ea 100644 --- a/testdata/generic_providers_yaml/terraform/envs/stg/network/fogg.tf +++ b/testdata/generic_providers_yaml/terraform/envs/stg/network/fogg.tf @@ -22,6 +22,8 @@ provider "bar" { provider "foo" { foo_host = "nonprod" } +provider "fred" { +} provider "qux" { } terraform { @@ -63,6 +65,10 @@ terraform { source = "czi/foo" version = "~> 0.2" } + fred = { + source = "czi/bar" + version = "~> 0.1.0" + } local = { source = "hashicorp/local" version = "~> 2.0" diff --git a/testdata/generic_providers_yaml/terraform/global/fogg.tf b/testdata/generic_providers_yaml/terraform/global/fogg.tf index c7eb6b666..2e86ed757 100644 --- a/testdata/generic_providers_yaml/terraform/global/fogg.tf +++ b/testdata/generic_providers_yaml/terraform/global/fogg.tf @@ -22,6 +22,8 @@ provider "bar" { provider "foo" { foo_host = "nonprod" } +provider "fred" { +} provider "qux" { } terraform { @@ -63,6 +65,10 @@ terraform { source = "czi/foo" version = "~> 0.2" } + fred = { + source = "czi/bar" + version = "~> 0.1.0" + } local = { source = "hashicorp/local" version = "~> 2.0"