aws/session: Add Assume role for credential process from aws profile #2674

skotambkar · 2019-06-28T21:16:59Z

Enabled support to assume role using Credential Process from AWS Profile.
Added and updated test files, configuration files accordingly.

…DK-543

ghost · 2019-06-29T04:06:23Z

How I’m lost

jasdel

Looks good, couple suggestion.

Also need to make sure we test for windows.

jasdel · 2019-07-01T16:42:15Z

awstesting/util.go

-	os.Clearenv()
+// StashEnv stashes the current environment variables except variables listed in envToKeep
+// Returns an array of all environment values as key=val strings.
+func StashEnv(envToKeep ...string) []string {


Suggest updating aws/credentials/processcreds test, preserveImportantStashEnv function to use this updated utility.

jasdel · 2019-07-01T16:45:33Z

aws/session/testdata/credential_source_config

+
+[cred_proc_arn_set]
+role_arn = assume_role_w_creds_proc_role_arn
+credential_process = echo "{\"Version\": 1, \"AccessKeyId\": \"accesskey\", \"SecretAccessKey\": \"secretkey\"}"


will want to test this against windows to ensure tests work.

…hanges and added support for windows build on Travis

…hout linting

…r Windows

…ment

… bundle tests for travis - windows os

…l.Error

jasdel

Looks good, minor suggestions with how the environments are stashed between tests.

jasdel · 2019-07-09T00:02:27Z

aws/corehandlers/handlers_test.go

-	os.Clearenv()
-
+	restoreEnvFn := sdktesting.StashEnv()
+	restoreEnvFn()


I think this is missing the defer

jasdel · 2019-07-09T00:02:43Z

aws/corehandlers/handlers_test.go

-	os.Clearenv()
-
+	restoreEnvFn := sdktesting.StashEnv()
+	restoreEnvFn()


missing defer

jasdel · 2019-07-09T00:02:52Z

aws/corehandlers/handlers_test.go

@@ -69,7 +69,9 @@ func (m *mockCredsProvider) IsExpired() bool {
 }

 func TestAfterRetryRefreshCreds(t *testing.T) {
-	os.Clearenv()
+	restoreEnvFn := sdktesting.StashEnv()
+	restoreEnvFn()


missing defer

jasdel · 2019-07-09T00:04:25Z

aws/credentials/env_provider_test.go

@@ -29,7 +31,8 @@ func TestEnvProviderRetrieve(t *testing.T) {
 }

 func TestEnvProviderIsExpired(t *testing.T) {
-	os.Clearenv()
+	sdktesting.StashEnv()


Should these stashes also restore the env after the test runs?

jasdel · 2019-07-09T00:06:19Z

aws/credentials/shared_credentials_provider_test.go

 	"github.com/aws/aws-sdk-go/internal/shareddefaults"
 )

 func TestSharedCredentialsProvider(t *testing.T) {
-	os.Clearenv()
+
+	sdktesting.StashEnv()


Should these restore the stashed env after the test runs?

jasdel · 2019-07-09T00:07:19Z

aws/request/request.go

+		if netErr, ok := err.(*net.OpError); ok && netErr.Op == "dial" {
+			return true
+		}
+			// If the error is temporary, we want to allow continuation of the


nit, minor spacing/gofmt here

jasdel · 2019-07-09T00:08:15Z

aws/request/request_retry_test.go

+	}
+
+	awsError := awserr.New("ErrorTestShouldRetry", "Test should retry when error received", &urlError)
+	origError := awsError.OrigErr()


is awsError needed in this test?

jasdel · 2019-07-09T00:08:36Z

aws/request/request_test.go

@@ -1101,6 +1101,11 @@ func TestRequestNoConnection(t *testing.T) {
 		t.Fatal("expect error, but got none")
 	}

+	t.Log(err)


nit, would be good to include prefix for this log statement.

jasdel · 2019-07-09T00:17:10Z

aws/session/env_config_test.go

@@ -76,8 +77,7 @@ func TestLoadEnvConfig_Creds(t *testing.T) {
 	}

 	for _, c := range cases {
-		os.Clearenv()
-
+		sdktesting.StashEnv()


Should these restore the env after test is done?

Suggest adding, t.Run for this test with // +build go1.7

jasdel

Looks good, minor issue with the t.Run in test case.

jasdel · 2019-07-09T21:58:37Z

aws/session/env_config_test.go

-		for k, v := range c.Env {
-			os.Setenv(k, v)
-		}
+		t.Run("", func(t *testing.T) {


the first param of t.Run in this case should be the case index number. e.g.

for i, c := range cases { t.Run(strconv.Itoa(i), func(t *testing.T) { ...

Or change the cases from a slice to map, and provide name for each test case.

Adds #2674, and #2677 to pending change log.

Kotambkar added 5 commits June 28, 2019 11:54

Added support for Credential Process to assume role for AWS-SDK-GO

6fe148c

Added support for Credential Process to assume role for AWS-SDK-GO

9db3f0a

Merge branch 'GoSDK-543' of github.com:skotambkar/aws-sdk-go into GoS…

4020e0d

…DK-543

Modified config file to unable support for Windows as well as Linux.

f3b7a90

code cleanup

d659cb3

ghost approved these changes Jun 29, 2019

View reviewed changes

jasdel suggested changes Jul 1, 2019

View reviewed changes

Kotambkar added 9 commits July 1, 2019 10:35

Eliminated redundant code from credential process tests, formatting c…

f1658b4

…hanges and added support for windows build on Travis

Updated travis to support make for windows

0def7bf

Updated travis configuration file to support unit test on Windows wit…

8a9d14d

…hout linting

Fixed syntax error in Travis config file

7a1f20e

resolved dependency issue for travis configuration

cb77c1e

Updated environment stashing logic to preserve SYSTEMROOT settings fo…

2ec081f

…r Windows

Added system root in environments to keep when stashing

44f511f

changes in stashing systemroot for windows

0759f56

removed additional print statements

d9cb70f

jasdel added the pr/work-in-progress This PR is a draft and needs further work. label Jul 2, 2019

Kotambkar added 9 commits July 2, 2019 15:38

Refactored code with sdktesting with platform specific config environ…

b8555c4

…ment

Fixed minor bugs for tests and config files

8150197

Fix for TestNoConnection retry bug on Windows OS

a0e6b4c

Code cleanup, added unit test for request connection retry, custom ca…

4e8dc55

… bundle tests for travis - windows os

Updated readme file for awstesting

f313908

Updated request retry test

a6e326a

updated test log for supporting Go version 1.5

3ea246b

Fixed TestShouldRetry to solve nil pointer dereference issue with *ur…

2c88c3f

…l.Error

code cleanup

a7dc165

skotambkar requested a review from jasdel July 8, 2019 19:39

jasdel added needs-review This issue or pull request needs review from a core team member. and removed pr/work-in-progress This PR is a draft and needs further work. labels Jul 9, 2019

jasdel suggested changes Jul 9, 2019

View reviewed changes

Kotambkar added 2 commits July 9, 2019 10:29

Added suggested changes

3c5b464

Added suggested changes

26d07ad

jasdel suggested changes Jul 9, 2019

View reviewed changes

Added case index number in t.Run() for env_config_test

2239254

jasdel changed the title ~~Assume role for credential process from aws profile~~ aws/session: Add Assume role for credential process from aws profile Jul 10, 2019

jasdel approved these changes Jul 10, 2019

View reviewed changes

skotambkar merged commit 5402768 into aws:master Jul 10, 2019

skotambkar deleted the GoSDK-543 branch July 10, 2019 19:49

jasdel added a commit that referenced this pull request Jul 10, 2019

Update pending change log with merged changes

518c519

Adds #2674, and #2677 to pending change log.

aws-sdk-go-automation mentioned this pull request Jul 11, 2019

v1.20.19 #2691

Merged

bflad mentioned this pull request Jul 23, 2019

AWS profile does not work with role_arn and credential_source = Ec2InstanceMetadata hashicorp/terraform-provider-aws#5018

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aws/session: Add Assume role for credential process from aws profile #2674

aws/session: Add Assume role for credential process from aws profile #2674

skotambkar commented Jun 28, 2019

ghost commented Jun 29, 2019

jasdel left a comment

jasdel Jul 1, 2019

jasdel Jul 1, 2019

jasdel left a comment

jasdel Jul 9, 2019

jasdel Jul 9, 2019

jasdel Jul 9, 2019

jasdel Jul 9, 2019

jasdel Jul 9, 2019

jasdel Jul 9, 2019

jasdel Jul 9, 2019

jasdel Jul 9, 2019

jasdel Jul 9, 2019

jasdel Jul 9, 2019

jasdel left a comment

jasdel Jul 9, 2019

jasdel Jul 9, 2019

aws/session: Add Assume role for credential process from aws profile #2674

aws/session: Add Assume role for credential process from aws profile #2674

Conversation

skotambkar commented Jun 28, 2019

ghost commented Jun 29, 2019

jasdel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasdel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasdel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment