From 374ce0fa3606bee4883ad65b3acda8f056b781ce Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 25 Apr 2022 12:15:46 -0700 Subject: [PATCH 1/5] Resolves credentials after setting config sources on Config --- config/config.go | 22 +++--- config/resolve_credentials_test.go | 105 +++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 8 deletions(-) diff --git a/config/config.go b/config/config.go index 79f067017e2..aa0d9088f44 100644 --- a/config/config.go +++ b/config/config.go @@ -64,14 +64,14 @@ var defaultAWSConfigResolvers = []awsConfigResolver{ // configured auto mode. resolveDefaultsModeOptions, - // Sets the resolved credentials the API clients will use for - // authentication. Provides the SDK's default credential chain. - // - // Should probably be the last step in the resolve chain to ensure that all - // other configurations are resolved first in case downstream credentials - // implementations depend on or can be configured with earlier resolved - // configuration options. - resolveCredentials, + // // Sets the resolved credentials the API clients will use for + // // authentication. Provides the SDK's default credential chain. + // // + // // Should probably be the last step in the resolve chain to ensure that all + // // other configurations are resolved first in case downstream credentials + // // implementations depend on or can be configured with earlier resolved + // // configuration options. + // resolveCredentials, } // A Config represents a generic configuration value or set of values. This type @@ -148,6 +148,12 @@ func (cs configs) ResolveAWSConfig(ctx context.Context, resolvers []awsConfigRes } cfg.ConfigSources = sources + err := resolveCredentials(ctx, &cfg, cs) + if err != nil { + // TODO provide better error? + return aws.Config{}, err + } + return cfg, nil } diff --git a/config/resolve_credentials_test.go b/config/resolve_credentials_test.go index 268d3a6ae61..68b9309b3d0 100644 --- a/config/resolve_credentials_test.go +++ b/config/resolve_credentials_test.go @@ -2,6 +2,7 @@ package config import ( "context" + "errors" "fmt" "io/ioutil" "net/http" @@ -15,9 +16,12 @@ import ( "time" "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/aws/retry" + "github.com/aws/aws-sdk-go-v2/feature/ec2/imds" "github.com/aws/aws-sdk-go-v2/internal/awstesting" "github.com/aws/aws-sdk-go-v2/service/sso" "github.com/aws/aws-sdk-go-v2/service/sts" + "github.com/aws/smithy-go" "github.com/aws/smithy-go/middleware" smithytime "github.com/aws/smithy-go/time" ) @@ -471,3 +475,104 @@ func TestResolveCredentialsCacheOptions(t *testing.T) { t.Errorf("expect options to be called") } } + +func TestResolveCredentialsIMDSClient(t *testing.T) { + restoreEnv := awstesting.StashEnv() + defer awstesting.PopEnv(restoreEnv) + + expectEnabled := func(t *testing.T, err error) { + var re *retry.MaxAttemptsError + if !(err == nil || // If running in EC2, there will be no error + errors.As(err, &re) || // When not running in EC2, the IMDS call can either reach max attempts + errors.Is(err, context.DeadlineExceeded)) { // or reach the context deadline + t.Fatalf("unexpected error: %v", err) + } + } + + expectDisabled := func(t *testing.T, err error) { + var oe *smithy.OperationError + if !errors.As(err, &oe) { + t.Fatalf("unexpected error: %v", err) + } else { + e := errors.Unwrap(oe) + if e == nil { + t.Fatalf("unexpected empty operation error: %v", oe) + } else { + if !strings.HasPrefix(e.Error(), "access disabled to EC2 IMDS") { + t.Fatalf("unexpected operation error: %v", oe) + } + } + } + } + + testcases := map[string]struct { + enabledState imds.ClientEnableState + envvar string + expectedState imds.ClientEnableState + expectedError func(*testing.T, error) + }{ + "nothing": { + expectedState: imds.ClientDefaultEnableState, + expectedError: expectEnabled, + }, + + "state enabled": { + enabledState: imds.ClientEnabled, + expectedState: imds.ClientEnabled, + expectedError: expectEnabled, + }, + "state disabled": { + enabledState: imds.ClientDisabled, + expectedState: imds.ClientDisabled, + expectedError: expectDisabled, + }, + + "DISABLED true": { + envvar: "true", + expectedState: imds.ClientDisabled, + expectedError: expectDisabled, + }, + "DISABLED false": { + envvar: "false", + expectedState: imds.ClientEnabled, + expectedError: expectEnabled, + }, + + "state enabled overrides DISABLED true": { + enabledState: imds.ClientEnabled, + envvar: "true", + expectedState: imds.ClientEnabled, + expectedError: expectEnabled, + }, + "state disabled overrides DISABLED false": { + enabledState: imds.ClientDisabled, + envvar: "false", + expectedState: imds.ClientDisabled, + expectedError: expectDisabled, + }, + } + + for name, tc := range testcases { + t.Run(name, func(t *testing.T) { + os.Clearenv() + + opts := []func(*LoadOptions) error{} + if tc.enabledState != imds.ClientDefaultEnableState { + opts = append(opts, WithEC2IMDSClientEnableState(tc.enabledState)) + } + if tc.envvar != "" { + os.Setenv("AWS_EC2_METADATA_DISABLED", tc.envvar) + } + + c, err := LoadDefaultConfig(context.TODO(), opts...) + if err != nil { + t.Fatalf("could not load config: %s", err) + } + + creds := c.Credentials + + _, err = creds.Retrieve(context.TODO()) + tc.expectedError(t, err) + }) + } +} From d631a21e96e49f20f2ded091cc69203dcae921a1 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 25 Apr 2022 13:27:48 -0700 Subject: [PATCH 2/5] Updates comment on `resolveCredentials` --- config/config.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/config/config.go b/config/config.go index aa0d9088f44..444b9598358 100644 --- a/config/config.go +++ b/config/config.go @@ -63,15 +63,6 @@ var defaultAWSConfigResolvers = []awsConfigResolver{ // before resolving credentials so that those subsequent clients use the // configured auto mode. resolveDefaultsModeOptions, - - // // Sets the resolved credentials the API clients will use for - // // authentication. Provides the SDK's default credential chain. - // // - // // Should probably be the last step in the resolve chain to ensure that all - // // other configurations are resolved first in case downstream credentials - // // implementations depend on or can be configured with earlier resolved - // // configuration options. - // resolveCredentials, } // A Config represents a generic configuration value or set of values. This type @@ -148,6 +139,11 @@ func (cs configs) ResolveAWSConfig(ctx context.Context, resolvers []awsConfigRes } cfg.ConfigSources = sources + // Sets the resolved credentials the API clients will use for + // authentication. Provides the SDK's default credential chain. + // + // Called after setting the ConfigSources on the aws.Config because some credentials + // use a service client which requires a fully-configured aws.Config err := resolveCredentials(ctx, &cfg, cs) if err != nil { // TODO provide better error? From 58d85fc25a67cb84aaec848e53aced780695c3c9 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte <961963+jasdel@users.noreply.github.com> Date: Thu, 5 May 2022 14:43:05 -0700 Subject: [PATCH 3/5] update config sources to be assigned in resolvedDefaultAWSConfig to be available for following config resolvers --- config/config.go | 27 +++++++++------------------ config/resolve.go | 10 ++++++++-- config/resolve_credentials_test.go | 6 ++---- 3 files changed, 19 insertions(+), 24 deletions(-) diff --git a/config/config.go b/config/config.go index 444b9598358..3e9c20009fe 100644 --- a/config/config.go +++ b/config/config.go @@ -63,6 +63,15 @@ var defaultAWSConfigResolvers = []awsConfigResolver{ // before resolving credentials so that those subsequent clients use the // configured auto mode. resolveDefaultsModeOptions, + + // Sets the resolved credentials the API clients will use for + // authentication. Provides the SDK's default credential chain. + // + // Should probably be the last step in the resolve chain to ensure that all + // other configurations are resolved first in case downstream credentials + // implementations depend on or can be configured with earlier resolved + // configuration options. + resolveCredentials, } // A Config represents a generic configuration value or set of values. This type @@ -128,28 +137,10 @@ func (cs configs) ResolveAWSConfig(ctx context.Context, resolvers []awsConfigRes for _, fn := range resolvers { if err := fn(ctx, &cfg, cs); err != nil { - // TODO provide better error? return aws.Config{}, err } } - var sources []interface{} - for _, s := range cs { - sources = append(sources, s) - } - cfg.ConfigSources = sources - - // Sets the resolved credentials the API clients will use for - // authentication. Provides the SDK's default credential chain. - // - // Called after setting the ConfigSources on the aws.Config because some credentials - // use a service client which requires a fully-configured aws.Config - err := resolveCredentials(ctx, &cfg, cs) - if err != nil { - // TODO provide better error? - return aws.Config{}, err - } - return cfg, nil } diff --git a/config/resolve.go b/config/resolve.go index 4a802476949..4428ba49c20 100644 --- a/config/resolve.go +++ b/config/resolve.go @@ -21,9 +21,15 @@ import ( // This should be used as the first resolver in the slice of resolvers when // resolving external configuration. func resolveDefaultAWSConfig(ctx context.Context, cfg *aws.Config, cfgs configs) error { + var sources []interface{} + for _, s := range cfgs { + sources = append(sources, s) + } + *cfg = aws.Config{ - Credentials: aws.AnonymousCredentials{}, - Logger: logging.NewStandardLogger(os.Stderr), + Credentials: aws.AnonymousCredentials{}, + Logger: logging.NewStandardLogger(os.Stderr), + ConfigSources: sources, } return nil } diff --git a/config/resolve_credentials_test.go b/config/resolve_credentials_test.go index 68b9309b3d0..d4ee6af3a43 100644 --- a/config/resolve_credentials_test.go +++ b/config/resolve_credentials_test.go @@ -477,9 +477,6 @@ func TestResolveCredentialsCacheOptions(t *testing.T) { } func TestResolveCredentialsIMDSClient(t *testing.T) { - restoreEnv := awstesting.StashEnv() - defer awstesting.PopEnv(restoreEnv) - expectEnabled := func(t *testing.T, err error) { var re *retry.MaxAttemptsError if !(err == nil || // If running in EC2, there will be no error @@ -554,7 +551,8 @@ func TestResolveCredentialsIMDSClient(t *testing.T) { for name, tc := range testcases { t.Run(name, func(t *testing.T) { - os.Clearenv() + restoreEnv := awstesting.StashEnv() + defer awstesting.PopEnv(restoreEnv) opts := []func(*LoadOptions) error{} if tc.enabledState != imds.ClientDefaultEnableState { From a17440ac7c513f6b4bf262dd2c39b710569c522a Mon Sep 17 00:00:00 2001 From: Jason Del Ponte <961963+jasdel@users.noreply.github.com> Date: Thu, 5 May 2022 14:46:48 -0700 Subject: [PATCH 4/5] add changelog --- .changelog/4cb8965feb3b48b6afcfa099607e4bb0.json | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .changelog/4cb8965feb3b48b6afcfa099607e4bb0.json diff --git a/.changelog/4cb8965feb3b48b6afcfa099607e4bb0.json b/.changelog/4cb8965feb3b48b6afcfa099607e4bb0.json new file mode 100644 index 00000000000..ad3411473f3 --- /dev/null +++ b/.changelog/4cb8965feb3b48b6afcfa099607e4bb0.json @@ -0,0 +1,8 @@ +{ + "id": "4cb8965f-eb3b-48b6-afcf-a099607e4bb0", + "type": "bugfix", + "description": "Fixes a bug in LoadDefaultConfig to correctly assign ConfigSources so all config resolvers have access to the config sources. This fixes the feature/ec2/imds client not having configuration applied via config.LoadOptions such as EC2IMDSClientEnableState. PR [#1682](https://github.com/aws/aws-sdk-go-v2/pull/1682)", + "modules": [ + "config" + ] +} \ No newline at end of file From a964590bd19f4b95f6d324715b1c015f48129c02 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte <961963+jasdel@users.noreply.github.com> Date: Thu, 5 May 2022 16:56:37 -0700 Subject: [PATCH 5/5] fixup unit test to be stable and not impacted by environment --- config/resolve_credentials_test.go | 45 +++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/config/resolve_credentials_test.go b/config/resolve_credentials_test.go index d4ee6af3a43..b2454250928 100644 --- a/config/resolve_credentials_test.go +++ b/config/resolve_credentials_test.go @@ -16,7 +16,6 @@ import ( "time" "github.com/aws/aws-sdk-go-v2/aws" - "github.com/aws/aws-sdk-go-v2/aws/retry" "github.com/aws/aws-sdk-go-v2/feature/ec2/imds" "github.com/aws/aws-sdk-go-v2/internal/awstesting" "github.com/aws/aws-sdk-go-v2/service/sso" @@ -478,11 +477,11 @@ func TestResolveCredentialsCacheOptions(t *testing.T) { func TestResolveCredentialsIMDSClient(t *testing.T) { expectEnabled := func(t *testing.T, err error) { - var re *retry.MaxAttemptsError - if !(err == nil || // If running in EC2, there will be no error - errors.As(err, &re) || // When not running in EC2, the IMDS call can either reach max attempts - errors.Is(err, context.DeadlineExceeded)) { // or reach the context deadline - t.Fatalf("unexpected error: %v", err) + if err == nil { + t.Fatalf("expect error got none") + } + if e, a := "expected HTTP client error", err.Error(); !strings.Contains(a, e) { + t.Fatalf("expected %v error in %v", e, a) } } @@ -508,7 +507,7 @@ func TestResolveCredentialsIMDSClient(t *testing.T) { expectedState imds.ClientEnableState expectedError func(*testing.T, error) }{ - "nothing": { + "default no options": { expectedState: imds.ClientDefaultEnableState, expectedError: expectEnabled, }, @@ -524,24 +523,24 @@ func TestResolveCredentialsIMDSClient(t *testing.T) { expectedError: expectDisabled, }, - "DISABLED true": { + "env var DISABLED true": { envvar: "true", expectedState: imds.ClientDisabled, expectedError: expectDisabled, }, - "DISABLED false": { + "env var DISABLED false": { envvar: "false", expectedState: imds.ClientEnabled, expectedError: expectEnabled, }, - "state enabled overrides DISABLED true": { + "option state enabled overrides env var DISABLED true": { enabledState: imds.ClientEnabled, envvar: "true", expectedState: imds.ClientEnabled, expectedError: expectEnabled, }, - "state disabled overrides DISABLED false": { + "option state disabled overrides env var DISABLED false": { enabledState: imds.ClientDisabled, envvar: "false", expectedState: imds.ClientDisabled, @@ -554,10 +553,24 @@ func TestResolveCredentialsIMDSClient(t *testing.T) { restoreEnv := awstesting.StashEnv() defer awstesting.PopEnv(restoreEnv) - opts := []func(*LoadOptions) error{} + var httpClient HTTPClient + if tc.expectedState == imds.ClientDisabled { + httpClient = stubErrorClient{err: fmt.Errorf("expect HTTP client not to be called")} + } else { + httpClient = stubErrorClient{err: fmt.Errorf("expected HTTP client error")} + } + + opts := []func(*LoadOptions) error{ + WithRetryer(func() aws.Retryer { return aws.NopRetryer{} }), + WithHTTPClient(httpClient), + } + if tc.enabledState != imds.ClientDefaultEnableState { - opts = append(opts, WithEC2IMDSClientEnableState(tc.enabledState)) + opts = append(opts, + WithEC2IMDSClientEnableState(tc.enabledState), + ) } + if tc.envvar != "" { os.Setenv("AWS_EC2_METADATA_DISABLED", tc.envvar) } @@ -574,3 +587,9 @@ func TestResolveCredentialsIMDSClient(t *testing.T) { }) } } + +type stubErrorClient struct { + err error +} + +func (c stubErrorClient) Do(*http.Request) (*http.Response, error) { return nil, c.err }