From 8e6f1bc7c9f750285cba8f6c5e042532b07d0165 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Sat, 30 May 2020 15:02:38 -0400 Subject: [PATCH] Enable additional golangci-lint linters and fix reports Previously: ``` $ make lint awsauth_test.go:127:32: Using the variable on range scope `testCase` in function literal (scopelint) awsTs := awsMetadataApiMock(testCase.EC2MetadataEndpoints) ^ awsauth_test.go:130:60: Using the variable on range scope `testCase` in function literal (scopelint) closeIam, iamSess, err := GetMockedAwsApiSession("IAM", testCase.IAMEndpoints) ^ awsauth_test.go:136:60: Using the variable on range scope `testCase` in function literal (scopelint) closeSts, stsSess, err := GetMockedAwsApiSession("STS", testCase.STSEndpoints) ^ awsauth_test.go:145:76: Using the variable on range scope `testCase` in function literal (scopelint) accountID, partition, err := GetAccountIDAndPartition(iamConn, stsConn, testCase.AuthProviderName) ^ awsauth_test.go:146:21: Using the variable on range scope `testCase` in function literal (scopelint) if err != nil && testCase.ErrCount == 0 { ^ awsauth_test.go:149:21: Using the variable on range scope `testCase` in function literal (scopelint) if err == nil && testCase.ErrCount > 0 { ^ awsauth_test.go:150:53: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Expected %d error(s), received none", testCase.ErrCount) ^ awsauth_test.go:152:20: Using the variable on range scope `testCase` in function literal (scopelint) if accountID != testCase.ExpectedAccountID { ^ awsauth_test.go:153:85: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Parsed account ID doesn't match with expected (%q != %q)", accountID, testCase.ExpectedAccountID) ^ awsauth_test.go:155:20: Using the variable on range scope `testCase` in function literal (scopelint) if partition != testCase.ExpectedPartition { ^ awsauth_test.go:156:84: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Parsed partition doesn't match with expected (%q != %q)", partition, testCase.ExpectedPartition) ^ awsauth_test.go:227:60: Using the variable on range scope `testCase` in function literal (scopelint) closeIam, iamSess, err := GetMockedAwsApiSession("IAM", testCase.MockEndpoints) ^ awsauth_test.go:236:21: Using the variable on range scope `testCase` in function literal (scopelint) if err != nil && testCase.ErrCount == 0 { ^ awsauth_test.go:239:21: Using the variable on range scope `testCase` in function literal (scopelint) if err == nil && testCase.ErrCount > 0 { ^ awsauth_test.go:240:53: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Expected %d error(s), received none", testCase.ErrCount) ^ awsauth_test.go:242:20: Using the variable on range scope `testCase` in function literal (scopelint) if accountID != testCase.ExpectedAccountID { ^ awsauth_test.go:243:85: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Parsed account ID doesn't match with expected (%q != %q)", accountID, testCase.ExpectedAccountID) ^ awsauth_test.go:245:20: Using the variable on range scope `testCase` in function literal (scopelint) if partition != testCase.ExpectedPartition { ^ awsauth_test.go:246:84: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Parsed partition doesn't match with expected (%q != %q)", partition, testCase.ExpectedPartition) ^ awsauth_test.go:285:60: Using the variable on range scope `testCase` in function literal (scopelint) closeIam, iamSess, err := GetMockedAwsApiSession("IAM", testCase.MockEndpoints) ^ awsauth_test.go:294:21: Using the variable on range scope `testCase` in function literal (scopelint) if err != nil && testCase.ErrCount == 0 { ^ awsauth_test.go:297:21: Using the variable on range scope `testCase` in function literal (scopelint) if err == nil && testCase.ErrCount > 0 { ^ awsauth_test.go:298:53: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Expected %d error(s), received none", testCase.ErrCount) ^ awsauth_test.go:300:20: Using the variable on range scope `testCase` in function literal (scopelint) if accountID != testCase.ExpectedAccountID { ^ awsauth_test.go:301:85: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Parsed account ID doesn't match with expected (%q != %q)", accountID, testCase.ExpectedAccountID) ^ awsauth_test.go:303:20: Using the variable on range scope `testCase` in function literal (scopelint) if partition != testCase.ExpectedPartition { ^ awsauth_test.go:304:84: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Parsed partition doesn't match with expected (%q != %q)", partition, testCase.ExpectedPartition) ^ awsauth_test.go:343:60: Using the variable on range scope `testCase` in function literal (scopelint) closeSts, stsSess, err := GetMockedAwsApiSession("STS", testCase.MockEndpoints) ^ awsauth_test.go:352:21: Using the variable on range scope `testCase` in function literal (scopelint) if err != nil && testCase.ErrCount == 0 { ^ awsauth_test.go:355:21: Using the variable on range scope `testCase` in function literal (scopelint) if err == nil && testCase.ErrCount > 0 { ^ awsauth_test.go:356:53: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Expected %d error(s), received none", testCase.ErrCount) ^ awsauth_test.go:358:20: Using the variable on range scope `testCase` in function literal (scopelint) if accountID != testCase.ExpectedAccountID { ^ awsauth_test.go:359:85: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Parsed account ID doesn't match with expected (%q != %q)", accountID, testCase.ExpectedAccountID) ^ awsauth_test.go:361:20: Using the variable on range scope `testCase` in function literal (scopelint) if partition != testCase.ExpectedPartition { ^ awsauth_test.go:362:84: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Parsed partition doesn't match with expected (%q != %q)", partition, testCase.ExpectedPartition) ^ awsauth_test.go:403:67: Using the variable on range scope `testCase` in function literal (scopelint) accountID, partition, err := parseAccountIDAndPartitionFromARN(testCase.InputARN) ^ awsauth_test.go:404:21: Using the variable on range scope `testCase` in function literal (scopelint) if err != nil && testCase.ErrCount == 0 { ^ awsauth_test.go:407:21: Using the variable on range scope `testCase` in function literal (scopelint) if err == nil && testCase.ErrCount > 0 { ^ awsauth_test.go:408:70: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Expected %d error(s) when parsing ARN, received none", testCase.ErrCount) ^ awsauth_test.go:410:20: Using the variable on range scope `testCase` in function literal (scopelint) if accountID != testCase.ExpectedAccountID { ^ awsauth_test.go:411:85: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Parsed account ID doesn't match with expected (%q != %q)", accountID, testCase.ExpectedAccountID) ^ awsauth_test.go:413:20: Using the variable on range scope `testCase` in function literal (scopelint) if partition != testCase.ExpectedPartition { ^ awsauth_test.go:414:84: Using the variable on range scope `testCase` in function literal (scopelint) t.Fatalf("Parsed partition doesn't match with expected (%q != %q)", partition, testCase.ExpectedPartition) ^ awserr_test.go:107:20: Using the variable on range scope `testCase` in function literal (scopelint) got := IsAWSErr(testCase.Err, testCase.Code, testCase.Message) ^ awserr_test.go:109:14: Using the variable on range scope `testCase` in function literal (scopelint) if got != testCase.Expected { ^ awserr_test.go:110:42: Using the variable on range scope `testCase` in function literal (scopelint) t.Errorf("got %t, expected %t", got, testCase.Expected) ^ awserr_test.go:333:28: Using the variable on range scope `testCase` in function literal (scopelint) got := IsAWSErrExtended(testCase.Err, testCase.Code, testCase.Message, testCase.ExtendedMessage) ^ awserr_test.go:335:14: Using the variable on range scope `testCase` in function literal (scopelint) if got != testCase.Expected { ^ awserr_test.go:336:42: Using the variable on range scope `testCase` in function literal (scopelint) t.Errorf("got %t, expected %t", got, testCase.Expected) ^ session_test.go:34:35: Using the variable on range scope `tc` in function literal (scopelint) opts, err := GetSessionOptions(tc.config) ^ awsauth_test.go:735:20: `invalidAwsEnv` - `t` is unused (unparam) func invalidAwsEnv(t *testing.T) func() { ^ mock.go:22:18: mnd: Magic number: 500, in detected (gomnd) w.WriteHeader(500) ^ mock.go:46:17: mnd: Magic number: 400, in detected (gomnd) w.WriteHeader(400) ^ mock.go:83:17: mnd: Magic number: 400, in detected (gomnd) w.WriteHeader(400) ^ session.go:87:21: mnd: Magic number: 9, in detected (gomnd) if r.RetryCount < 9 { ^ awsauth.go:241:19: mnd: Magic number: 100, in detected (gomnd) client.Timeout = 100 * time.Millisecond ^ make: *** [lint] Error 1 ``` --- .golangci.yml | 7 +++++++ awsauth.go | 8 +++++--- awsauth_test.go | 28 +++++++++++++++++++++------- awserr_test.go | 4 ++++ mock.go | 6 +++--- session.go | 13 ++++++++++--- session_test.go | 12 ++++++++---- validation_test.go | 4 ++++ 8 files changed, 62 insertions(+), 20 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 59ef674c..687b0c78 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -6,14 +6,21 @@ linters: disable-all: true enable: - deadcode + - dogsled - errcheck + - goconst - gofmt + - gomnd - gosimple - ineffassign + - interfacer - misspell + - scopelint - staticcheck - structcheck - unconvert + - unparam - unused + - typecheck - varcheck - vet diff --git a/awsauth.go b/awsauth.go index 53116203..daab3f8e 100644 --- a/awsauth.go +++ b/awsauth.go @@ -27,6 +27,10 @@ const ( errMsgNoValidCredentialSources = `No valid credential sources found for AWS Provider. Please see https://terraform.io/docs/providers/aws/index.html for more information on providing credentials for the AWS Provider` + + // Default amount of time for EC2/ECS metadata client operations. + // Keep this value low to prevent long delays in non-EC2/ECS environments. + DefaultMetadataClientTimeout = 100 * time.Millisecond ) var ( @@ -236,9 +240,7 @@ func GetCredentials(c *Config) (*awsCredentials.Credentials, error) { // Build isolated HTTP client to avoid issues with globally-shared settings client := cleanhttp.DefaultClient() - - // Keep the default timeout (100ms) low as we don't want to wait in non-EC2 environments - client.Timeout = 100 * time.Millisecond + client.Timeout = DefaultMetadataClientTimeout const userTimeoutEnvVar = "AWS_METADATA_TIMEOUT" userTimeout := os.Getenv(userTimeoutEnvVar) diff --git a/awsauth_test.go b/awsauth_test.go index 00be5158..aa2c65aa 100644 --- a/awsauth_test.go +++ b/awsauth_test.go @@ -120,6 +120,8 @@ func TestGetAccountIDAndPartition(t *testing.T) { } for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.Description, func(t *testing.T) { resetEnv := unsetEnv(t) defer resetEnv() @@ -223,6 +225,8 @@ func TestGetAccountIDAndPartitionFromIAMGetUser(t *testing.T) { } for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.Description, func(t *testing.T) { closeIam, iamSess, err := GetMockedAwsApiSession("IAM", testCase.MockEndpoints) defer closeIam() @@ -281,6 +285,8 @@ func TestGetAccountIDAndPartitionFromIAMListRoles(t *testing.T) { } for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.Description, func(t *testing.T) { closeIam, iamSess, err := GetMockedAwsApiSession("IAM", testCase.MockEndpoints) defer closeIam() @@ -339,6 +345,8 @@ func TestGetAccountIDAndPartitionFromSTSGetCallerIdentity(t *testing.T) { } for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.Description, func(t *testing.T) { closeSts, stsSess, err := GetMockedAwsApiSession("STS", testCase.MockEndpoints) defer closeSts() @@ -399,6 +407,8 @@ func TestAWSParseAccountIDAndPartitionFromARN(t *testing.T) { } for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.InputARN, func(t *testing.T) { accountID, partition, err := parseAccountIDAndPartitionFromARN(testCase.InputARN) if err != nil && testCase.ErrCount == 0 { @@ -434,7 +444,7 @@ func TestAWSGetCredentials_shouldErrorWhenBlank(t *testing.T) { } func TestAWSGetCredentials_shouldBeStatic(t *testing.T) { - simple := []struct { + testCases := []struct { Key, Secret, Token string }{ { @@ -447,7 +457,9 @@ func TestAWSGetCredentials_shouldBeStatic(t *testing.T) { }, } - for _, c := range simple { + for _, testCase := range testCases { + c := testCase + cfg := Config{ AccessKey: c.Key, SecretKey: c.Secret, @@ -526,7 +538,7 @@ func TestAWSGetCredentials_shouldIgnoreIAM(t *testing.T) { // capture the test server's close method, to call after the test returns ts := awsMetadataApiMock(append(ec2metadata_securityCredentialsEndpoints, ec2metadata_instanceIdEndpoint, ec2metadata_iamInfoEndpoint)) defer ts() - simple := []struct { + testCases := []struct { Key, Secret, Token string }{ { @@ -539,7 +551,9 @@ func TestAWSGetCredentials_shouldIgnoreIAM(t *testing.T) { }, } - for _, c := range simple { + for _, testCase := range testCases { + c := testCase + cfg := Config{ AccessKey: c.Key, SecretKey: c.Secret, @@ -574,7 +588,7 @@ func TestAWSGetCredentials_shouldErrorWithInvalidEndpoint(t *testing.T) { resetEnv := unsetEnv(t) defer resetEnv() // capture the test server's close method, to call after the test returns - ts := invalidAwsEnv(t) + ts := invalidAwsEnv() defer ts() _, err := GetCredentials(&Config{}) @@ -591,7 +605,7 @@ func TestAWSGetCredentials_shouldIgnoreInvalidEndpoint(t *testing.T) { resetEnv := unsetEnv(t) defer resetEnv() // capture the test server's close method, to call after the test returns - ts := invalidAwsEnv(t) + ts := invalidAwsEnv() defer ts() creds, err := GetCredentials(&Config{AccessKey: "accessKey", SecretKey: "secretKey"}) @@ -732,7 +746,7 @@ func TestAWSGetCredentials_shouldBeENV(t *testing.T) { // invalidAwsEnv establishes a httptest server to simulate behaviour // when endpoint doesn't respond as expected -func invalidAwsEnv(t *testing.T) func() { +func invalidAwsEnv() func() { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(400) })) diff --git a/awserr_test.go b/awserr_test.go index 94e9cf61..94a1b424 100644 --- a/awserr_test.go +++ b/awserr_test.go @@ -103,6 +103,8 @@ func TestIsAwsErr(t *testing.T) { } for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.Name, func(t *testing.T) { got := IsAWSErr(testCase.Err, testCase.Code, testCase.Message) @@ -329,6 +331,8 @@ func TestIsAwsErrExtended(t *testing.T) { } for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.Name, func(t *testing.T) { got := IsAWSErrExtended(testCase.Err, testCase.Code, testCase.Message, testCase.ExtendedMessage) diff --git a/mock.go b/mock.go index e3c80266..7584353d 100644 --- a/mock.go +++ b/mock.go @@ -19,7 +19,7 @@ func MockAwsApiServer(svcName string, endpoints []*MockEndpoint) *httptest.Serve ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { buf := new(bytes.Buffer) if _, err := buf.ReadFrom(r.Body); err != nil { - w.WriteHeader(500) + w.WriteHeader(http.StatusInternalServerError) fmt.Fprintf(w, "Error reading from HTTP Request Body: %s", err) return } @@ -43,7 +43,7 @@ func MockAwsApiServer(svcName string, endpoints []*MockEndpoint) *httptest.Serve } } - w.WriteHeader(400) + w.WriteHeader(http.StatusBadRequest) })) return ts @@ -80,7 +80,7 @@ func awsMetadataApiMock(responses []*MetadataResponse) func() { return } } - w.WriteHeader(400) + w.WriteHeader(http.StatusBadRequest) })) os.Setenv("AWS_METADATA_URL", ts.URL+"/latest") diff --git a/session.go b/session.go index 92671e13..881d0ad1 100644 --- a/session.go +++ b/session.go @@ -15,6 +15,15 @@ import ( "github.com/hashicorp/go-cleanhttp" ) +const ( + // Maximum network retries. + // We depend on the AWS Go SDK DefaultRetryer exponential backoff. + // Ensure that if the AWS Config MaxRetries is set high (which it is by + // default), that we only retry for a few seconds with typically + // unrecoverable network errors, such as DNS lookup failures. + MaxNetworkRetryCount = 9 +) + // GetSessionOptions attempts to return valid AWS Go SDK session authentication // options based on pre-existing credential provider, configured profile, or // fallback to automatically a determined session via the AWS Go SDK. @@ -82,9 +91,7 @@ func GetSession(c *Config) (*session.Session, error) { // NOTE: This logic can be fooled by other request errors raising the retry count // before any networking error occurs sess.Handlers.Retry.PushBack(func(r *request.Request) { - // We currently depend on the DefaultRetryer exponential backoff here. - // ~10 retries gives a fair backoff of a few seconds. - if r.RetryCount < 9 { + if r.RetryCount < MaxNetworkRetryCount { return } // RequestError: send request failed diff --git a/session_test.go b/session_test.go index cb841353..2c5df7ec 100644 --- a/session_test.go +++ b/session_test.go @@ -10,7 +10,7 @@ func TestGetSessionOptions(t *testing.T) { oldEnv := initSessionTestEnv() defer PopEnv(oldEnv) - tt := []struct { + testCases := []struct { desc string config *Config expectError bool @@ -29,7 +29,9 @@ func TestGetSessionOptions(t *testing.T) { }, } - for _, tc := range tt { + for _, testCase := range testCases { + tc := testCase + t.Run(tc.desc, func(t *testing.T) { opts, err := GetSessionOptions(tc.config) if err != nil && tc.expectError == false { @@ -91,7 +93,7 @@ func TestGetSessionWithAccountIDAndPartition(t *testing.T) { }) defer ts.Close() - tt := []struct { + testCases := []struct { desc string config *Config expectedAcctID string @@ -138,7 +140,9 @@ func TestGetSessionWithAccountIDAndPartition(t *testing.T) { "", "", "No valid credential sources found for AWS Provider."}, } - for _, tc := range tt { + for _, testCase := range testCases { + tc := testCase + t.Run(tc.desc, func(t *testing.T) { sess, acctID, part, err := GetSessionWithAccountIDAndPartition(tc.config) if err != nil { diff --git a/validation_test.go b/validation_test.go index 7ea7972a..4b869c71 100644 --- a/validation_test.go +++ b/validation_test.go @@ -50,6 +50,8 @@ func TestValidateAccountID(t *testing.T) { } for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.Description, func(t *testing.T) { err := ValidateAccountID(testCase.AccountID, testCase.AllowedAccountIDs, testCase.ForbiddenAccountIDs) if err != nil && !testCase.ExpectError { @@ -86,6 +88,8 @@ func TestValidateRegion(t *testing.T) { } for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.Region, func(t *testing.T) { err := ValidateRegion(testCase.Region) if err != nil && !testCase.ExpectError {