From 2703db7ff3f0a73e9841582353a7feab26cd04fc Mon Sep 17 00:00:00 2001 From: Eduardo Lopez Date: Wed, 19 Jun 2019 15:36:54 -0600 Subject: [PATCH] AWS account IDS should be strings (#302) [bug] AWS account IDS should be strings### Summary AWS account IDS should be strings (since they can start with 0). We use json.Number (a type alias for string) to allow for backwards compatibility with fogg configs that have ints already. ### Test Plan unittests + apply in existing repos. ### References --- cmd/exp/aws_config.go | 3 +-- config/config_test.go | 17 ++++++++++++----- config/v1/config.go | 8 ++++---- config/v1/config_test.go | 3 ++- config/v2/config.go | 23 +++++++---------------- config/v2/resolvers.go | 18 ++++++++++++++++-- config/v2/validation.go | 2 +- plan/plan.go | 19 ++++++++++--------- plan/plan_test.go | 9 +++++---- plan/travisci.go | 5 +++-- plan/travisci_test.go | 15 ++++++++------- util/testing.go | 7 +++++++ 12 files changed, 76 insertions(+), 53 deletions(-) diff --git a/cmd/exp/aws_config.go b/cmd/exp/aws_config.go index ce4bd1a04..d34ad7991 100644 --- a/cmd/exp/aws_config.go +++ b/cmd/exp/aws_config.go @@ -5,7 +5,6 @@ import ( "fmt" "os" "os/exec" - "strconv" "strings" "text/template" @@ -87,7 +86,7 @@ output = json roleARN := arn.ARN{ Partition: "aws", Service: "iam", - AccountID: strconv.Itoa(int(*account.Providers.AWS.AccountID)), + AccountID: account.Providers.AWS.AccountID.String(), Resource: fmt.Sprintf("role/%s", role), } diff --git a/config/config_test.go b/config/config_test.go index b58072daa..a15de5223 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -1,7 +1,9 @@ package config import ( + "encoding/json" "os" + "strconv" "testing" v1 "github.com/chanzuckerberg/fogg/config/v1" @@ -61,6 +63,11 @@ func intptr(i int64) *int64 { return &i } +func jsonNumberPtr(i int) *json.Number { + j := json.Number(strconv.Itoa(i)) + return &j +} + func strptr(s string) *string { return &s } @@ -88,7 +95,7 @@ func TestUpgradeConfigVersion(t *testing.T) { Providers: &v2.Providers{ AWS: &v2.AWSProvider{ - AccountID: intptr(1), + AccountID: jsonNumberPtr(1), Profile: strptr("czi"), Region: strptr("us-west-1"), Version: strptr("0.1.0"), @@ -124,7 +131,7 @@ func TestUpgradeConfigVersion(t *testing.T) { Providers: &v2.Providers{ AWS: &v2.AWSProvider{ - AccountID: intptr(2), + AccountID: jsonNumberPtr(2), Profile: strptr("czi-foo"), Region: strptr("us-west-foo1"), Version: strptr("0.12.0"), @@ -148,7 +155,7 @@ func TestUpgradeConfigVersion(t *testing.T) { Providers: &v2.Providers{ AWS: &v2.AWSProvider{ - AccountID: intptr(3), + AccountID: jsonNumberPtr(3), Profile: strptr("czi-bar"), Region: strptr("us-west-bar1"), Version: strptr("0.13.0"), @@ -175,7 +182,7 @@ func TestUpgradeConfigVersion(t *testing.T) { Providers: &v2.Providers{ AWS: &v2.AWSProvider{ - AccountID: intptr(4), + AccountID: jsonNumberPtr(4), Profile: strptr("czi-stage"), Region: strptr("us-west-stage1"), Version: strptr("0.14.0"), @@ -199,7 +206,7 @@ func TestUpgradeConfigVersion(t *testing.T) { Providers: &v2.Providers{ AWS: &v2.AWSProvider{ - AccountID: intptr(5), + AccountID: jsonNumberPtr(5), Profile: strptr("czi-env"), Region: strptr("us-west-env1"), Version: strptr("0.15.0"), diff --git a/config/v1/config.go b/config/v1/config.go index bb5999908..ad1cbc934 100644 --- a/config/v1/config.go +++ b/config/v1/config.go @@ -17,7 +17,7 @@ type TfLint struct { } type Defaults struct { - AccountID int64 `json:"account_id,omitempty" validate:"required"` + AccountID json.Number `json:"account_id,omitempty" validate:"required"` AWSProfileBackend string `json:"aws_profile_backend" validate:"required"` AWSProfileProvider string `json:"aws_profile_provider" validate:"required"` AWSProviderVersion string `json:"aws_provider_version" validate:"required"` @@ -34,7 +34,7 @@ type Defaults struct { } type Account struct { - AccountID *int64 `json:"account_id"` + AccountID *json.Number `json:"account_id"` AWSProfileBackend *string `json:"aws_profile_backend"` AWSProfileProvider *string `json:"aws_profile_provider"` AWSProviderVersion *string `json:"aws_provider_version,omitempty"` @@ -51,7 +51,7 @@ type Account struct { } type Env struct { - AccountID *int64 `json:"account_id"` + AccountID *json.Number `json:"account_id"` AWSProfileBackend *string `json:"aws_profile_backend"` AWSProfileProvider *string `json:"aws_profile_provider"` AWSProviderVersion *string `json:"aws_provider_version,omitempty"` @@ -95,7 +95,7 @@ type EKSConfig struct { } type Component struct { - AccountID *int64 `json:"account_id"` + AccountID *json.Number `json:"account_id"` AWSProfileBackend *string `json:"aws_profile_backend"` AWSProfileProvider *string `json:"aws_profile_provider"` AWSProviderVersion *string `json:"aws_provider_version,omitempty"` diff --git a/config/v1/config_test.go b/config/v1/config_test.go index f96e7a90d..05f1efe1f 100644 --- a/config/v1/config_test.go +++ b/config/v1/config_test.go @@ -1,6 +1,7 @@ package v1 import ( + "encoding/json" "io/ioutil" "strings" "testing" @@ -57,7 +58,7 @@ func TestParse(t *testing.T) { c, e := ReadConfig(b) assert.Nil(t, e) assert.NotNil(t, c.Defaults) - assert.Equal(t, int64(1), c.Defaults.AccountID) + assert.Equal(t, json.Number("1"), c.Defaults.AccountID) assert.Equal(t, "us-west-2", c.Defaults.AWSRegionBackend) assert.Equal(t, "us-west-1", c.Defaults.AWSRegionProvider) assert.Equal(t, "0.1.0", c.Defaults.AWSProviderVersion) diff --git a/config/v2/config.go b/config/v2/config.go index 8ee925234..d7620832e 100644 --- a/config/v2/config.go +++ b/config/v2/config.go @@ -92,11 +92,11 @@ type BlessProvider struct { type AWSProvider struct { // the aws provider is optional (above) but if supplied you must set account id and region - AccountID *int64 `json:"account_id,omitempty"` - AdditionalRegions []string `json:"additional_regions,omitempty"` - Profile *string `json:"profile,omitempty"` - Region *string `json:"region,omitempty"` - Version *string `json:"version,omitempty"` + AccountID *json.Number `json:"account_id,omitempty"` + AdditionalRegions []string `json:"additional_regions,omitempty"` + Profile *string `json:"profile,omitempty"` + Region *string `json:"region,omitempty"` + Version *string `json:"version,omitempty"` } type SnowflakeProvider struct { @@ -150,16 +150,6 @@ func (c *Config) Generate(r *rand.Rand, size int) reflect.Value { return map[string]string{} } - randInt64Ptr := func(r *rand.Rand, s int) *int64 { - if r.Float32() < 0.5 { - i := r.Int63n(int64(size)) - return &i - } else { - var i *int64 - return i - } - } - randOktaProvider := func(r *rand.Rand, s int) *OktaProvider { if r.Float32() < 0.5 { return nil @@ -184,8 +174,9 @@ func (c *Config) Generate(r *rand.Rand, size int) reflect.Value { randAWSProvider := func(r *rand.Rand, s int) *AWSProvider { if r.Float32() < 0.5 { + accountID := json.Number(randString(r, s)) return &AWSProvider{ - AccountID: randInt64Ptr(r, size), + AccountID: &accountID, Region: randStringPtr(r, s), Profile: randStringPtr(r, s), Version: randStringPtr(r, s), diff --git a/config/v2/resolvers.go b/config/v2/resolvers.go index af4ce4ccc..57fe44849 100644 --- a/config/v2/resolvers.go +++ b/config/v2/resolvers.go @@ -1,6 +1,8 @@ package v2 import ( + "encoding/json" + v1 "github.com/chanzuckerberg/fogg/config/v1" ) @@ -28,6 +30,18 @@ func lastNonNilInt64(getter func(Common) *int64, commons ...Common) *int64 { return s } +// lastNonNilJsonNumber, despite its name can return nil if all results are nil +func lastNonNilJsonNumber(getter func(Common) *json.Number, commons ...Common) *json.Number { + var jsonNumber *json.Number + for _, c := range commons { + j := getter(c) + if j != nil { + jsonNumber = j + } + } + return jsonNumber +} + // lastNonNilStringSlice, despite its name can return nil if all results are nil func lastNonNilStringSlice(getter func(Common) []string, commons ...Common) []string { var s []string @@ -93,7 +107,7 @@ func ResolveAWSProvider(commons ...Common) *AWSProvider { Version: version, // optional fields - AccountID: lastNonNilInt64(AWSProviderAccountIdGetter, commons...), + AccountID: lastNonNilJsonNumber(AWSProviderAccountIdGetter, commons...), AdditionalRegions: ResolveOptionalStringSlice(AWSProviderAdditionalRegionsGetter, commons...), } } @@ -221,7 +235,7 @@ func AWSProviderProfileGetter(comm Common) *string { return nil } -func AWSProviderAccountIdGetter(comm Common) *int64 { +func AWSProviderAccountIdGetter(comm Common) *json.Number { if comm.Providers != nil && comm.Providers.AWS != nil { return comm.Providers.AWS.AccountID } diff --git a/config/v2/validation.go b/config/v2/validation.go index 9746e729d..8b90ff13d 100644 --- a/config/v2/validation.go +++ b/config/v2/validation.go @@ -86,7 +86,7 @@ func ValidateAWSProvider(p *AWSProvider, component string) error { errs = multierror.Append(errs, fmt.Errorf("aws provider version for %s ", component)) } - if p.AccountID == nil || *p.AccountID == 0 { + if p.AccountID == nil || *p.AccountID == "" { errs = multierror.Append(errs, fmt.Errorf("aws provider account id for %s", component)) } return errs diff --git a/plan/plan.go b/plan/plan.go index 4a8055a07..016ff4fa4 100644 --- a/plan/plan.go +++ b/plan/plan.go @@ -1,6 +1,7 @@ package plan import ( + "encoding/json" "errors" "fmt" @@ -46,11 +47,11 @@ type Providers struct { } type AWSProvider struct { - AccountID int64 `yaml:"account_id"` - Profile string `yaml:"profile"` - Version string `yaml:"version"` - Region string `yaml:"region"` - AdditionalRegions []string `yaml:"additional_regions"` + AccountID json.Number `yaml:"account_id"` + Profile string `yaml:"profile"` + Version string `yaml:"version"` + Region string `yaml:"region"` + AdditionalRegions []string `yaml:"additional_regions"` } type SnowflakeProvider struct { @@ -90,8 +91,8 @@ type Module struct { type Account struct { ComponentCommon `yaml:",inline"` - AllAccounts map[string]int64 `yaml:"all_accounts"` - AccountName string `yaml:"account_name"` + AllAccounts map[string]json.Number `yaml:"all_accounts"` + AccountName string `yaml:"account_name"` Global *Component } @@ -361,8 +362,8 @@ func resolveExtraVars(vars ...map[string]string) map[string]string { return resolved } -func resolveAccounts(accounts map[string]v2.Account) map[string]int64 { - a := make(map[string]int64) +func resolveAccounts(accounts map[string]v2.Account) map[string]json.Number { + a := make(map[string]json.Number) for name, account := range accounts { if account.Providers != nil && account.Providers.AWS != nil && account.Providers.AWS.AccountID != nil { a[name] = *account.Providers.AWS.AccountID diff --git a/plan/plan_test.go b/plan/plan_test.go index bb5416493..64a264b0e 100644 --- a/plan/plan_test.go +++ b/plan/plan_test.go @@ -1,11 +1,12 @@ package plan import ( + "encoding/json" "testing" "github.com/chanzuckerberg/fogg/config" - "github.com/chanzuckerberg/fogg/config/v1" - "github.com/chanzuckerberg/fogg/config/v2" + v1 "github.com/chanzuckerberg/fogg/config/v1" + v2 "github.com/chanzuckerberg/fogg/config/v2" "github.com/chanzuckerberg/fogg/util" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -29,7 +30,7 @@ func init() { // } func TestResolveAccounts(t *testing.T) { - foo, bar := int64(123), int64(456) + foo, bar := json.Number("123"), json.Number("456") accounts := map[string]v2.Account{ "foo": { @@ -55,7 +56,7 @@ func TestResolveAccounts(t *testing.T) { other := resolveAccounts(accounts) assert.NotNil(t, other) - assert.Equal(t, map[string]int64{"bar": bar, "foo": foo}, other) + assert.Equal(t, map[string]json.Number{"bar": bar, "foo": foo}, other) } func TestPlanBasicV1(t *testing.T) { diff --git a/plan/travisci.go b/plan/travisci.go index 596382747..9aa3c9e57 100644 --- a/plan/travisci.go +++ b/plan/travisci.go @@ -1,15 +1,16 @@ package plan import ( + "encoding/json" "path" - "github.com/chanzuckerberg/fogg/config/v2" + v2 "github.com/chanzuckerberg/fogg/config/v2" "github.com/chanzuckerberg/fogg/util" ) type AWSProfile struct { Name string - ID int64 + ID json.Number Role string } diff --git a/plan/travisci_test.go b/plan/travisci_test.go index cc08f1a37..634f95a56 100644 --- a/plan/travisci_test.go +++ b/plan/travisci_test.go @@ -1,19 +1,20 @@ package plan import ( + "encoding/json" "testing" - "github.com/chanzuckerberg/fogg/config/v1" - "github.com/chanzuckerberg/fogg/config/v2" + v1 "github.com/chanzuckerberg/fogg/config/v1" + v2 "github.com/chanzuckerberg/fogg/config/v2" "github.com/chanzuckerberg/fogg/util" "github.com/stretchr/testify/assert" ) -var id1, id2 int64 +var id1, id2 json.Number func init() { - id1 = int64(123456789) - id2 = int64(987654321) + id1 = json.Number("123456789") + id2 = json.Number("987654321") } func Test_buildTravisCI_Disabled(t *testing.T) { @@ -50,7 +51,7 @@ func Test_buildTravisCI_Profiles(t *testing.T) { TerraformVersion: util.StrPtr("0.1.0"), Providers: &v2.Providers{ AWS: &v2.AWSProvider{ - AccountID: util.Intptr(123), + AccountID: util.JsonNumberPtr(123), Region: util.StrPtr("us-west-2"), Profile: util.StrPtr("foo"), Version: util.StrPtr("0.12.0"), @@ -99,7 +100,7 @@ func Test_buildTravisCI_TestBuckets(t *testing.T) { TerraformVersion: util.StrPtr("0.1.0"), Providers: &v2.Providers{ AWS: &v2.AWSProvider{ - AccountID: util.Intptr(123), + AccountID: util.JsonNumberPtr(123), Region: util.StrPtr("us-west-2"), Profile: util.StrPtr("foo"), Version: util.StrPtr("0.12.0"), diff --git a/util/testing.go b/util/testing.go index e9fef4e21..10e04ce91 100644 --- a/util/testing.go +++ b/util/testing.go @@ -1,10 +1,12 @@ package util import ( + "encoding/json" "io/ioutil" "os" "path/filepath" "runtime" + "strconv" "github.com/spf13/afero" ) @@ -13,6 +15,11 @@ func Intptr(i int64) *int64 { return &i } +func JsonNumberPtr(i int) *json.Number { + j := json.Number(strconv.Itoa(i)) + return &j +} + func StrPtr(s string) *string { if s == "" { return nil