-
Notifications
You must be signed in to change notification settings - Fork 670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fully populates aws.Config
before calling resolveCredentials
#1682
Conversation
I'm not able to reproduce the test errors |
9ca4e62
to
a17440a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to create this PR @gdavison. While reviewing the change I noticed the ConfigSources
were being after before the config resolvers were being processed. This was incorrect. By moving assigning ConfigSources in resolveDefaultAWSConfig
, it ensures the config sources are available for all downstream config resolvers, such as resolveCredentials
. This also allows resolvedCredentials
to stay in the set of resolvers, without being special cased.
Not sure why unit tests are failing. I'm not able to reproduce that locally. They seem to only fail in GitHub actions. |
I think the reason the tests were failing in GitHub actions but not locally, must have something to do with environment. I updated the tests to use stub HTTP clients instead of real clients. This should prevent the issue. |
@jasdel One twist to this approach is that the subsequent |
Thanks for the feedback @gdavison. There is a subtle usage difference between These two values do overlap currently, but the intention is that
|
Setting the
EC2IMDSClientEnableState
toimds.ClientDisabled
does not disable the EC2 IMDS Client when resolving configuration usingconfig.LoadDefaultConfig()
.Setting
AWS_EC2_METADATA_DISABLED = true
does correctly disable the EC2 IMDS Client.The
awsConfigResolver
functions take the parameters(ctx context.Context, cfg *aws.Config, configs configs)
wherecfg
has noConfigSources
set, and the config providers are passed inconfigs
.This works until the resolvers need to create a service client, such as
imds.Client
. The clients are created using theirNewFromConfig
function. Theaws.Config
with zeroConfigSources
is then passed in, ignoring the config providers inconfigs
.This PR works, but doesn't make any deep changes. For instance, either of the following could be done:
resolveCredentials
is no longer called in a loop with otherawsConfigResolver
functions, it doesn't need to match the interface, and could replace uses ofconfig
with theConfigSources
on theaws.Config
aws.Config
is populated in the loop ofawsConfigResolver
functions. This could also allow the use of theConfigSources
on theaws.Config
instead of theconfig
parameter.Closes #1398