From 421932dad26479e912dd14939f3d373c93fd1b7b Mon Sep 17 00:00:00 2001 From: jayanandagit Date: Wed, 18 Nov 2020 11:30:32 -0500 Subject: [PATCH] Fix account pagination and its api functional test (#388) --- CHANGELOG.md | 5 + cmd/lambda/accounts/list_test.go | 7 +- .../fan_out_update_lease_status/main.go | 3 +- cmd/lambda/populate_reset_queue/main.go | 3 +- cmd/lambda/populate_reset_queue/main_test.go | 14 +- go.mod | 7 +- go.sum | 6 + pkg/account/model.go | 5 + pkg/data/accounts.go | 30 ++++- pkg/data/accounts_test.go | 120 +++++++++++++++++- .../v0.21.0_rename_db_tables_dce/main.go | 7 +- 11 files changed, 185 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b250b6c6a..d0556efd8 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +## v0.33.1 +- Fix populate reset queue when dynamodb returns paginated result +- Add account status to last evaluated key when querying account table using global secondary index +- Fix gosec issue related to G601 (CWE-118): Implicit memory aliasing in for loop + ## v0.33.0 - Upgrade aws-nuke to v1.3.0 diff --git a/cmd/lambda/accounts/list_test.go b/cmd/lambda/accounts/list_test.go index 1812ad355..990bd2ef0 100755 --- a/cmd/lambda/accounts/list_test.go +++ b/cmd/lambda/accounts/list_test.go @@ -29,6 +29,7 @@ func TestGetAccounts(t *testing.T) { retAccounts *account.Accounts retErr error nextID *string + status *account.Status }{ { name: "get all accounts", @@ -53,7 +54,8 @@ func TestGetAccounts(t *testing.T) { }, }, nextID: ptrString("234567890123"), - expLink: "; rel=\"next\"", + status: account.StatusNotReady.StatusPtr(), + expLink: "; rel=\"next\"", retErr: nil, }, { @@ -81,6 +83,7 @@ func TestGetAccounts(t *testing.T) { values := url.Values{} err := schema.NewEncoder().Encode(tt.query, values) + assert.Nil(t, err) r.URL.RawQuery = values.Encode() @@ -94,6 +97,8 @@ func TestGetAccounts(t *testing.T) { if (input.ID != nil && tt.query.ID != nil && *input.ID == *tt.query.ID) || input.ID == tt.query.ID { if tt.nextID != nil { input.NextID = tt.nextID + input.Status = tt.status + input.Limit = ptr64(1) } return true diff --git a/cmd/lambda/fan_out_update_lease_status/main.go b/cmd/lambda/fan_out_update_lease_status/main.go index d47233d43..27ff197c2 100755 --- a/cmd/lambda/fan_out_update_lease_status/main.go +++ b/cmd/lambda/fan_out_update_lease_status/main.go @@ -68,7 +68,8 @@ func handler(cloudWatchEvent events.CloudWatchEvent) error { err = services.LeaseService().ListPages(query, func(leases *lease.Leases) bool { for _, ls := range *leases { - leaseJSON, err := json.Marshal(&ls) + l := ls + leaseJSON, err := json.Marshal(&l) // save any errors to handle later if err != nil { errs = append(errs, err) diff --git a/cmd/lambda/populate_reset_queue/main.go b/cmd/lambda/populate_reset_queue/main.go index 751af3622..f8e305797 100644 --- a/cmd/lambda/populate_reset_queue/main.go +++ b/cmd/lambda/populate_reset_queue/main.go @@ -66,8 +66,9 @@ func Handler(cloudWatchEvent events.CloudWatchEvent) error { func(accts *account.Accounts) bool { for _, acct := range *accts { + a := acct // Send Message - err := api.AccountReset(&acct) + err := api.AccountReset(&a) if err != nil { errs = append(errs, err) } diff --git a/cmd/lambda/populate_reset_queue/main_test.go b/cmd/lambda/populate_reset_queue/main_test.go index 3395a548d..6179998d9 100755 --- a/cmd/lambda/populate_reset_queue/main_test.go +++ b/cmd/lambda/populate_reset_queue/main_test.go @@ -30,7 +30,8 @@ func TestPopulateResetQeue(t *testing.T) { listAccounts *account.Accounts listErr error alertErr error - nextID *string + nextId *string + status *account.Status }{ { name: "should send accounts to reset queue", @@ -54,7 +55,8 @@ func TestPopulateResetQeue(t *testing.T) { PrincipalRoleArn: arn.New("aws", "iam", "", "123456789012", "role/AdminRole"), }, }, - nextID: ptrString("123456789013"), + nextId: ptrString("123456789013"), + status: account.StatusNotReady.StatusPtr(), listErr: nil, }, { @@ -93,18 +95,22 @@ func TestPopulateResetQeue(t *testing.T) { svcBldr := &config.ServiceBuilder{Config: cfgBldr} mocksRwd := &mocks.ReaderWriterDeleter{} + mocksRwd.On("List", mock.MatchedBy(func(input *account.Account) bool { if input.Status.String() == "NotReady" { if input.NextID == nil { - input.NextID = tt.nextID + input.NextID = tt.nextId + input.Status = tt.status + return true } } return false })).Return(tt.listAccounts, tt.listErr) + mocksRwd.On("List", mock.MatchedBy(func(input *account.Account) bool { if input.Status.String() == "NotReady" { - if input.NextID == tt.nextID { + if input.NextID == tt.nextId { input.NextID = nil return true } diff --git a/go.mod b/go.mod index 8b8420f5a..d20cc185f 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.15 require ( github.com/360EntSecGroup-Skylar/excelize v1.4.1 + github.com/AlekSi/gocov-xml v0.0.0-20190121064608-3a14fb1c4737 // indirect github.com/Bowery/prompt v0.0.0-20190419144237-972d0ceb96f5 // indirect github.com/Joker/jade v1.0.0 // indirect github.com/asaskevich/govalidator v0.0.0-20200907205600-7a23bdc65eef // indirect @@ -11,9 +12,9 @@ require ( github.com/aws/aws-lambda-go v1.19.1 github.com/aws/aws-sdk-go v1.34.20 github.com/awslabs/aws-lambda-go-api-proxy v0.8.0 + github.com/axw/gocov v1.0.0 // indirect github.com/caarlos0/env v3.5.0+incompatible github.com/dchest/safefile v0.0.0-20151022103144-855e8d98f185 // indirect - github.com/fatih/color v1.9.0 // indirect github.com/flosch/pongo2 v0.0.0-20190707114632-bbf5a6c351f4 // indirect github.com/go-ozzo/ozzo-validation v3.6.0+incompatible github.com/google/shlex v0.0.0-20181106134648-c34317bd91bf // indirect @@ -33,16 +34,12 @@ require ( github.com/mitchellh/mapstructure v1.3.3 github.com/oleiade/reflections v1.0.0 // indirect github.com/pkg/errors v0.9.1 - github.com/pquerna/otp v1.2.0 // indirect github.com/rebuy-de/aws-nuke v1.4.0 github.com/securego/gosec v0.0.0-20200316084457-7da9f46445fd // indirect - github.com/sirupsen/logrus v1.6.0 // indirect - github.com/spf13/cobra v1.0.0 // indirect github.com/stretchr/objx v0.3.0 // indirect github.com/stretchr/testify v1.6.1 golang.org/x/crypto v0.0.0-20200820211705-5c72a883971a golang.org/x/net v0.0.0-20200904194848-62affa334b73 // indirect - golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208 // indirect golang.org/x/sys v0.0.0-20200909081042-eff7692f9009 // indirect google.golang.org/appengine v1.6.6 // indirect gopkg.in/alexcesaro/quotedprintable.v3 v3.0.0-20150716171945-2caba252f4dc // indirect diff --git a/go.sum b/go.sum index a15089fe4..e3435f104 100644 --- a/go.sum +++ b/go.sum @@ -13,6 +13,8 @@ cloud.google.com/go/storage v1.0.0/go.mod h1:IhtSnM/ZTZV8YYJWCY8RULGVqBDmpoyjwiy dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU= github.com/360EntSecGroup-Skylar/excelize v1.4.1 h1:l55mJb6rkkaUzOpSsgEeKYtS6/0gHwBYyfo5Jcjv/Ks= github.com/360EntSecGroup-Skylar/excelize v1.4.1/go.mod h1:vnax29X2usfl7HHkBrX5EvSCJcmH3dT9luvxzu8iGAE= +github.com/AlekSi/gocov-xml v0.0.0-20190121064608-3a14fb1c4737 h1:JZHBkt0GhM+ARQykshqpI49yaWCHQbJonH3XpDTwMZQ= +github.com/AlekSi/gocov-xml v0.0.0-20190121064608-3a14fb1c4737/go.mod h1:w1KSuh2JgIL3nyRiZijboSUwbbxOrTzWwyWVFUHtXBQ= github.com/AndreasBriese/bbloom v0.0.0-20190306092124-e2d15f34fcf9/go.mod h1:bOvUY6CB00SOBii9/FifXqc0awNKxLFCL/+pkDPuyl8= github.com/Azure/azure-sdk-for-go v35.0.0+incompatible/go.mod h1:9XXNKU+eRnpl9moKnB4QOLf1HestfXbmab5FXxiDBjc= github.com/Azure/azure-sdk-for-go v38.0.0+incompatible/go.mod h1:9XXNKU+eRnpl9moKnB4QOLf1HestfXbmab5FXxiDBjc= @@ -87,6 +89,8 @@ github.com/awslabs/aws-lambda-go-api-proxy v0.5.0 h1:mmzE5dJ2yt23lmWr6QNtCCAA3H0 github.com/awslabs/aws-lambda-go-api-proxy v0.5.0/go.mod h1:9ZpbR64sd0A73+ylC1tP63Kyz2VhijeDw1O8naJqehA= github.com/awslabs/aws-lambda-go-api-proxy v0.8.0 h1:XUx+5PMwtZEIWc7oyMduXUfAhumHFU/xbSPwB2csYx0= github.com/awslabs/aws-lambda-go-api-proxy v0.8.0/go.mod h1:V3jj7BZnRY8y2QTKSABIwBc+dTjPkX7vLxz61Id7vsQ= +github.com/axw/gocov v1.0.0 h1:YsqYR66hUmilVr23tu8USgnJIJvnwh3n7j5zRn7x4LU= +github.com/axw/gocov v1.0.0/go.mod h1:LvQpEYiwwIb2nYkXY2fDWhg9/AsYqkhmrCshjlUJECE= github.com/aymerick/raymond v2.0.2+incompatible/go.mod h1:osfaiScAUVup+UC9Nfq76eWqDhXlp+4UYaA8uhTBO6g= github.com/aymerick/raymond v2.0.3-0.20180322193309-b565731e1464+incompatible/go.mod h1:osfaiScAUVup+UC9Nfq76eWqDhXlp+4UYaA8uhTBO6g= github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973/go.mod h1:Dwedo/Wpr24TaqPxmxbtue+5NUziq4I4S80YR8gNf3Q= @@ -640,6 +644,7 @@ golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20190227155943-e225da77a7e6/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e h1:vcxGaoTs7kV8m5Np9uUNQin4BrLOthgV7252N8V+FwY= golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208 h1:qwRHBd0NqMbJxfbotnDhm2ByMI1Shq4Y6oRJo21SGJA= golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= @@ -709,6 +714,7 @@ golang.org/x/tools v0.0.0-20190506145303-2d16b83fe98c/go.mod h1:RgjU9mgBXZiqYHBn golang.org/x/tools v0.0.0-20190524140312-2c0ae7006135/go.mod h1:RgjU9mgBXZiqYHBnxXauZ1Gv1EHHAz9KjViQ78xBX0Q= golang.org/x/tools v0.0.0-20190606124116-d0a3d012864b/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= golang.org/x/tools v0.0.0-20190614205625-5aca471b1d59/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= +golang.org/x/tools v0.0.0-20190617190820-da514acc4774/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= golang.org/x/tools v0.0.0-20190621195816-6e04913cbbac/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= golang.org/x/tools v0.0.0-20190628153133-6cdbf07be9d0/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= golang.org/x/tools v0.0.0-20190706070813-72ffa07ba3db/go.mod h1:jcCCGcm9btYwXyDqrUWc6MKQKKGJCWEQ3AfLSRIbEuI= diff --git a/pkg/account/model.go b/pkg/account/model.go index 8db4c9c90..f4097c9ac 100644 --- a/pkg/account/model.go +++ b/pkg/account/model.go @@ -149,6 +149,11 @@ type Accounts []Account // Status is an account status type type Status string +type LastEvaluatedKey struct { + ID dynamodb.AttributeValue + AccountStatus dynamodb.AttributeValue +} + const ( // StatusNone status StatusNone Status = "None" diff --git a/pkg/data/accounts.go b/pkg/data/accounts.go index 8f3f2eb79..9afc5e703 100644 --- a/pkg/data/accounts.go +++ b/pkg/data/accounts.go @@ -1,6 +1,8 @@ package data import ( + "encoding/json" + "github.com/Optum/dce/pkg/account" "github.com/Optum/dce/pkg/errors" "github.com/aws/aws-sdk-go/aws" @@ -43,12 +45,16 @@ func (a *Account) queryAccounts(query *account.Account, keyName string, index st } queryInput.SetLimit(*query.Limit) - if query.NextID != nil { + + if query.NextID != nil && query.Status != nil { // Should be more dynamic queryInput.SetExclusiveStartKey(map[string]*dynamodb.AttributeValue{ "Id": &dynamodb.AttributeValue{ S: query.NextID, }, + "AccountStatus": &dynamodb.AttributeValue{ + S: query.Status.StringPtr(), + }, }) } @@ -89,6 +95,7 @@ func (a *Account) scanAccounts(query *account.Account) (*queryScanOutput, error) } scanInput.SetLimit(*query.Limit) + if query.NextID != nil { // Should be more dynamic scanInput.SetExclusiveStartKey(map[string]*dynamodb.AttributeValue{ @@ -129,9 +136,24 @@ func (a *Account) List(query *account.Account) (*account.Accounts, error) { return nil, err } - query.NextID = nil - for _, v := range outputs.lastEvaluatedKey { - query.NextID = v.S + if outputs.lastEvaluatedKey != nil { + jsondata, err := json.Marshal(outputs.lastEvaluatedKey) + + if err != nil { + return nil, errors.NewInternalServer("failed marshaling of last evaluated key", err) + } + + lastEvaluatedKey := account.LastEvaluatedKey{} + + // set last evaluated key to next id for next query/scan + if err := json.Unmarshal(jsondata, &lastEvaluatedKey); err != nil { + return nil, errors.NewInternalServer("failed unmarshaling of last evaluated key to next ID", err) + } + + query.NextID = lastEvaluatedKey.ID.S + } else { + // clear next id and account status if there is no more page + query.NextID = nil } accounts := &account.Accounts{} diff --git a/pkg/data/accounts_test.go b/pkg/data/accounts_test.go index fe4a1fe25..a357f94a2 100644 --- a/pkg/data/accounts_test.go +++ b/pkg/data/accounts_test.go @@ -2,6 +2,8 @@ package data import ( "fmt" + "math/rand" + "reflect" "testing" "github.com/Optum/dce/pkg/account" @@ -14,6 +16,8 @@ import ( ) func TestGetAccountsScan(t *testing.T) { + testAccounts := GetRandomStrings(20) + tests := []struct { name string query *account.Account @@ -108,6 +112,27 @@ func TestGetAccountsScan(t *testing.T) { expAccounts: nil, expErr: errors.NewInternalServer("error getting accounts", fmt.Errorf("failure")), }, + { + name: "scan get all accounts with pagination", + query: &account.Account{}, + sInput: &dynamodb.ScanInput{ + ConsistentRead: aws.Bool(false), + TableName: aws.String("Accounts"), + Limit: aws.Int64(25), + }, + sOutputRec: &dynamodb.ScanOutput{ + Items: GetRandomAccounts(testAccounts), + LastEvaluatedKey: map[string]*dynamodb.AttributeValue{ + "Id": { + S: ptrString("123456789012"), + }, + "AccountStatus": { + S: ptrString("NotReady"), + }, + }, + }, + expAccounts: MakeAccounts(testAccounts), + }, } for _, tt := range tests { @@ -123,17 +148,22 @@ func TestGetAccountsScan(t *testing.T) { accountData := &Account{ DynamoDB: &mockDynamo, TableName: "Accounts", - Limit: 5, + Limit: *tt.sInput.Limit, } accounts, err := accountData.List(tt.query) assert.True(t, errors.Is(err, tt.expErr)) - assert.Equal(t, tt.expAccounts, accounts) + assert.True(t, reflect.DeepEqual(tt.expAccounts, accounts)) + if tt.query.NextID != nil { + assert.Equal(t, *tt.query.NextID, *tt.sOutputRec.LastEvaluatedKey["Id"].S) + } }) } } func TestGetAccountsQuery(t *testing.T) { + testAccounts := GetRandomStrings(20) + tests := []struct { name string query *account.Account @@ -252,6 +282,39 @@ func TestGetAccountsQuery(t *testing.T) { expAccounts: nil, expErr: errors.NewInternalServer("failed to query accounts", fmt.Errorf("failure")), }, + { + name: "query all accounts by status with pagination", + query: &account.Account{ + Status: account.StatusNotReady.StatusPtr(), + }, + qInput: &dynamodb.QueryInput{ + ConsistentRead: aws.Bool(false), + TableName: aws.String("Accounts"), + IndexName: aws.String("AccountStatus"), + ExpressionAttributeNames: map[string]*string{ + "#0": aws.String("AccountStatus"), + }, + ExpressionAttributeValues: map[string]*dynamodb.AttributeValue{ + ":0": { + S: aws.String("NotReady"), + }, + }, + KeyConditionExpression: aws.String("#0 = :0"), + Limit: aws.Int64(25), + }, + qOutputRec: &dynamodb.QueryOutput{ + Items: GetRandomAccounts(testAccounts), + LastEvaluatedKey: map[string]*dynamodb.AttributeValue{ + "Id": { + S: ptrString("123456789012"), + }, + "AccountStatus": { + S: ptrString("NotReady"), + }, + }, + }, + expAccounts: MakeAccounts(testAccounts), + }, } for _, tt := range tests { @@ -267,12 +330,61 @@ func TestGetAccountsQuery(t *testing.T) { accountData := &Account{ DynamoDB: &mockDynamo, TableName: "Accounts", - Limit: 5, + Limit: *tt.qInput.Limit, } accounts, err := accountData.List(tt.query) assert.True(t, errors.Is(err, tt.expErr)) - assert.Equal(t, tt.expAccounts, accounts) + assert.True(t, reflect.DeepEqual(tt.expAccounts, accounts)) + if tt.query.NextID != nil { + assert.Equal(t, *tt.query.NextID, *tt.qOutputRec.LastEvaluatedKey["Id"].S) + } }) } } + +func GetRandomAccounts(accounts []string) []map[string]*dynamodb.AttributeValue { + var items []map[string]*dynamodb.AttributeValue + + for _, account := range accounts { + item := map[string]*dynamodb.AttributeValue{ + "Id": { + S: aws.String(account), + }, + } + + items = append(items, item) + } + + return items +} + +func GetRandomStrings(n int) []string { + accounts := make([]string, n) + + for i := 0; i < n; i++ { + var letter = []rune("0123456789") + + b := make([]rune, 12) + for i := range b { + b[i] = letter[rand.Intn(len(letter))] + } + + accounts[i] = string(b) + } + + return accounts +} + +func MakeAccounts(accountSlice []string) *account.Accounts { + accounts := make(account.Accounts, len(accountSlice)) + + for i := 0; i < len(accountSlice); i++ { + accounts[i] = account.Account{ + ID: &accountSlice[i], + PrincipalPolicyArn: arn.New("aws", "iam", "", accountSlice[i], "policy/DCEPrincipalDefaultPolicy"), + } + } + + return &accounts +} diff --git a/scripts/migrations/v0.21.0_rename_db_tables_dce/main.go b/scripts/migrations/v0.21.0_rename_db_tables_dce/main.go index d012b4831..2a6903ada 100644 --- a/scripts/migrations/v0.21.0_rename_db_tables_dce/main.go +++ b/scripts/migrations/v0.21.0_rename_db_tables_dce/main.go @@ -2,12 +2,13 @@ package main import ( "fmt" + "log" + errors2 "github.com/Optum/dce/pkg/errors" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/dynamodb" "github.com/pkg/errors" - "log" ) /* @@ -63,9 +64,11 @@ func migrate(input *migrateInput) error { // Create records in the new table var deferredErrors []error + t := dstTableName + for i, item := range scanRes.Items { _, err = input.db.PutItem(&dynamodb.PutItemInput{ - TableName: &dstTableName, + TableName: &t, Item: item, }) if err != nil {