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

Shared Configuration Support #1178

Merged
merged 4 commits into from
Jul 18, 2016
Merged

Shared Configuration Support #1178

merged 4 commits into from
Jul 18, 2016

Conversation

awood45
Copy link
Member

@awood45 awood45 commented May 6, 2016

This change adds the Aws::SharedConfig file, which parses both the
Shared Credentials file and the Shared Config file. From that, functions
are provided to support:

  • Credentials from shared credentials or shared config.
  • Default region selection from shared config.
  • Assume role credentials from shared config.

Shared config support is toggled behind the AWS_SDK_LOAD_CONFIG
environment variable.

Assume role credentials support is working, but not yet wired in to any
default credential provider chain. I'd also still like to test edge
cases, as CLI configuration supports many more values than the shared
credential file.

This change adds the `Aws::SharedConfig` file, which parses both the
Shared Credentials file and the Shared Config file. From that, functions
are provided to support:

* Credentials from shared credentials or shared config.
* Default region selection from shared config.
* Assume role credentials from shared config.

Shared config support is toggled behind the `AWS_SDK_LOAD_CONFIG`
environment variable.

Assume role credentials support is working, but not yet wired in to any
default credential provider chain. I'd also still like to test edge
cases, as CLI configuration supports many more values than the shared
credential file.
@@ -51,4 +51,5 @@ end

group :repl do
gem 'pry'
gem 'byebug'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will want to remove byebug from the Gemfile and gitignore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed it from the Gemfile, I still think it's fine in the .gitignore file unless we would intend on tracking some sort of byebug history in the repo at any point.

@trevorrowe
Copy link
Member

This doesn't appear to support any of the other possible shared configuration values from ~/.aws/config besides region and credentials. These should be released together as we don't want the opt-in flag to initially only add partial support and then later pull additional configuration.

Brings this change up to spec for credentials, assume role, and region
support. Includes code review feedback to this point, and relevant test
improvements.

Still to be done is the addition of the Amazon Simple Storage Service
specific configuration values, and the wiring of Assume Role to the
credential provider chain.
@temujin9
Copy link

temujin9 commented Jul 2, 2016

Is there an ETA on this getting finished and merged? We have a large environment (including several ruby tools) which is moving to role assumption for our core architecture.

Adds `:assume_role_credentials` to default credential provider chain,
used only if the `AWS_SDK_LOAD_CONFIG` environment variable is set.

Improves usability of the default assume role behavior, and creates a
credential provider chain test to ensure resolution ordering.
@awood45
Copy link
Member Author

awood45 commented Jul 7, 2016

@temujin9 This last commit is the end of the planning/development cycle. Once we finish reviewing and ensuring that the test harness is solid (since this touches a lot of code that's run by just about every single client construction) we're intending to release.

@temujin9
Copy link

temujin9 commented Jul 8, 2016

@awood45 Thanks for the follow-up. Can I ask you ping here when it does release?

@awood45
Copy link
Member Author

awood45 commented Jul 8, 2016

When we release this, you'll definitely see a note here and it will be a minor version bump, for e.g. to 2.4.0.

enabled = ENV["AWS_SDK_LOAD_CONFIG"] ? true : false
@shared_config ||= SharedConfig.new(config_enabled: true)
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick. I try to limit the top-level utility functions. A simple change would be to keep this function inside the Aws::SharedConfig module. You could name the method something like .default_config, e.g Aws::SharedConfig.default_config.

@trevorrowe
Copy link
Member

I finished going though this today. I have some additional feedback from simply a code-organization perspective that should affect functionality. I can go over this with you in person. If you add @api private to the Aws::SharedConfig class, then I would say we are good to ship (given the feedback above has been addressed).

Also fixes bad fallback condition for Assume Role.
@awood45
Copy link
Member Author

awood45 commented Jul 18, 2016

I've marked the Aws::SharedConfig class as API private.

@trevorrowe
Copy link
Member

Ship it!

@awood45 awood45 merged commit e9276ee into master Jul 18, 2016
awood45 added a commit that referenced this pull request Jul 19, 2016
@temujin9
Copy link

Shipment received. Thanks, guys.

@cjyclaire cjyclaire deleted the cli-config branch May 5, 2017 20:30
@diehlaws diehlaws added the feature-request A feature should be added or improved. label Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants