Skip to content
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

Merged
merged 5 commits into from May 6, 2022

Conversation

gdavison
Copy link
Contributor

@gdavison gdavison commented Apr 25, 2022

Setting the EC2IMDSClientEnableState to imds.ClientDisabled does not disable the EC2 IMDS Client when resolving configuration using config.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) where cfg has no ConfigSources set, and the config providers are passed in configs.

This works until the resolvers need to create a service client, such as imds.Client. The clients are created using their NewFromConfig function. The aws.Config with zero ConfigSources is then passed in, ignoring the config providers in configs.

This PR works, but doesn't make any deep changes. For instance, either of the following could be done:

  1. Since resolveCredentials is no longer called in a loop with other awsConfigResolver functions, it doesn't need to match the interface, and could replace uses of config with the ConfigSources on the aws.Config
  2. Change how the aws.Config is populated in the loop of awsConfigResolver functions. This could also allow the use of the ConfigSources on the aws.Config instead of the config parameter.

Closes #1398

@gdavison
Copy link
Contributor Author

I'm not able to reproduce the test errors

@jasdel jasdel force-pushed the load-config-imds-client-state branch from 9ca4e62 to a17440a Compare May 5, 2022 21:47
@jasdel jasdel requested a review from skmcgrail May 5, 2022 21:47
Copy link
Contributor

@jasdel jasdel left a 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.

@jasdel
Copy link
Contributor

jasdel commented May 5, 2022

Not sure why unit tests are failing. I'm not able to reproduce that locally. They seem to only fail in GitHub actions.

@jasdel
Copy link
Contributor

jasdel commented May 5, 2022

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.

@gdavison
Copy link
Contributor Author

gdavison commented May 6, 2022

@jasdel One twist to this approach is that the subsequent awsConfigResolver functions no longer need the configs parameter, since the configs are now contained in the aws.Config. That was what I meant by my option 2 above

@jasdel
Copy link
Contributor

jasdel commented May 6, 2022

Thanks for the feedback @gdavison. There is a subtle usage difference between ConfigSources in aws.Config and configs used by the resolvers. configs is the set of configuration providers the config resolvers will search through when initializing the SDK's generic aws.Config properties. Whereas ConfigSources was added later as a way to provide API client with lazy configuration data without exposing API client specific fields in aws.Config directly.

These two values do overlap currently, but the intention is that ConfigSources could be appended with additional configuration sources by resolvers to be used by API clients. Whereas the set configs are only intended to be the initial set of configuration providers such as shared config, environment variables, and LoadOptions.

SharedConfig, EnvConfig, and LoadOptions play double duty as both config providers when resolving configuration, and lazy configuration sources for API clients.

@jasdel jasdel merged commit 8afaa75 into aws:main May 6, 2022
@gdavison gdavison deleted the load-config-imds-client-state branch February 7, 2023 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disabling EC2 IMDS Client in config.LoadDefaultConfig() does not disable EC2 IMDS Client
3 participants