In the client, only source creds from the shared file when necessary #259
Conversation
| @@ -65,7 +64,7 @@ def self.get_credentials(profile_name, access_key_id, secret_access_key, session | |||
| ENV["AWS_SECRET_ACCESS_KEY"], | |||
| ENV["AWS_SESSION_TOKEN"] | |||
| ) | |||
| elsif shared_creds.loadable? | |||
There was a problem hiding this comment.
This method is already called in the Aws::SharedCredentials constructor. Cf. http://docs.aws.amazon.com/sdkforruby/api/Aws/SharedCredentials.html (view source)
|
added a commit to fix Rubocop stuff |
|
@jkeiser any thoughts on this? |
|
@cheeseplus? anyone? |
|
looks like the right thing to so but I'm not a big ec2 user. curious what @jkeiser and @tyler-ball might think. |
|
If this can get rebased we can look at getting this merged. |
davidcpell
force-pushed
the
load-shared-creds-lazily
branch
2 times, most recently
from
1a56107
to
a6033f8
Compare
|
Thanks @cheeseplus. I've rebased and fixed a few things up that were made irrelevant by #269. However, I've now noticed after looking at this again that during the time since I originally submitted the PR https://docs.aws.amazon.com/sdkforruby/api/Aws/SharedCredentials.html#loadable%3F-instance_method ...and is no longer used in the constructor as I mentioned in an in-line comment. If [3] pry(main)> creds = Aws::SharedCredentials.new(:profile_name => 'foo')
Aws::Errors::NoSuchProfileError: Profile `foo' not found in /Users/david/.aws/credentials or /Users/david/.aws/config
from /Users/david/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/aws-sdk-core-2.6.22/lib/aws-sdk-core/shared_config.rb:230:in `validate_profile_exists'I'm trying to decide what the ramifications of that are for the PR. It looks like it might be worth guarding against a failed initialization of |
davidcpell
force-pushed
the
load-shared-creds-lazily
branch
from
76f4594
to
7b60f89
Compare
|
@cheeseplus This is ready for another look. I'm not sure why the Ruby 2.0.0 build is failing, though. It looks like it's trying to install a version of |
|
Going through all the PRs and would love to get this one rebased - if not we can fix it up as I think it's a good addition. |
|
@cheeseplus working on rebase right now and almost done cleaning up. All of a sudden the tests are really slow and I'm getting a lot of this, though: Is that something I should be worried about? |
davidcpell
force-pushed
the
load-shared-creds-lazily
branch
from
3f2c8b1
to
561e505
Compare
|
@davidcpell so not sure what specifically would cause the slowness but I did merge a slew of PRs - the verbose testing seems to pre-date any of those PRs though as I validated against a checkout of release 1.2.0 Tests don't seem too bad to me and ironically I'm getting bit by the behaviour this PR resolves. |
|
Nice, well now tests just seem to be failing due to a conflict between rake 11.0.1 and rspec: |
|
@cheeseplus ^^ I've renamed and think I've got everything cleaned up. Tests passing locally but in Travis failing because of that error |
|
There are conflicts on this one still @davidcpell probably related to my merge/reverts earlier - should be a quick rebase though. |
|
Ok I'm out but will take a look when I get home |
|
No rush, I'm going to merge this and the Windows 2016 PR before releasing. |
|
Now that #291 is merged you should be able to rebase and fix the rake issue. |
WHY: When I cloned the repo, certain tests in `client_spec.rb` were failing because I have `ENV` variables set for `aws_access_key_id` and `aws_secret_access_key`. These were being sourced in specs that were meant to skip this and look for, eg, the shared creds file. Now `ClimateControl` is used in those specs to ensure that the variables aren't set. Includes extracting a helper method.
WHY: Currently in `client.rb` there a number of places are checked for creds. A local variable representing the shared creds file in `~/.aws/credentials` is created before its known whether that option is even necessary (i.e. before all the switching begins). This was causing the client to error out on my machine because I use `ENV` variables and didn't have a `[default]` profile in my shared creds file, which is what this was trying to find (see test-kitchen#258). Rather than create this variable when it's not even clear that we need it, we can wait to use `Aws::SharedCredentials` when the control flow reaches that point and more highly prioritized options have been ruled out. This also simplifies a number of specs in `client_spec.rb` since the call to `SharedCredentials` doesn't need to be stubbed out every time.
ALSO: Improve tests around this method by getting rid of `let` and changing stubs/spies
Aws::SharedCredentials raises an exception if you try to initialize it with a profile name that doesn't exist. If you initialize with no profile name, it will look for a [default] profile. This change allows that step in the credentials check to avoid an error if, for example, the user is intending to authenticate with an IAM instance profile and has no ~/.aws/credentials or, in other cases, for whatever reason has the credentials file but no [default] profile and hasn't supplied an alternative.
...so that aws shared credentials file on the testing system won't get picked up
RuboCop said that this instance of 'private' was a 'useless access modifier'
davidcpell
force-pushed
the
load-shared-creds-lazily
branch
from
07787a9
to
7c1b48a
Compare
|
@cheeseplus think this ready to review. Along with whatever else, let me know if you would prefer me to squash commits, etc... |
|
@davidcpell looks good to me but a squash would definitely be great |
|
Forgot about squash and merge being available :D |
|
@davidcpell it seems I'm still getting issues with local credentials getting in the way so I'm not sure this worked as intended - |
|
I'm not fully sure I understand the issue getting fixed here, but I upgraded to kitchen-ec2 1.3.0 in order to take advantage of PR #291 to get Windows 2016 AMI's working. Now when I'm trying to interact with the 2016 instance or my previously working 2012r2 instance, I get the following kitchen error: This is mostly new ground for me in interacting with AWS EC2 instances, but I do know that what I had was working with 1.2.0 kitchen-ec2 and now it's not. Should I make a new issue? keep conversation here? I'm running ChefDK 1.1.16 and Kitchen 1.14.2 and kitchen-ec2 1.3.0. |
|
@cheeseplus I am out of the country but will be able to look again tomorrow |
|
@stpitner if you could, please make a new issue to track this. One thing that definitely is goofy (definitely not @davidcpell fault) is that the rebase was against master before a another commit was reverted. I've toyed around locally with trying to reconcile the changes but tests only pass if I unset my ENV vars which seems against the original intention of the PR. As this seems to be currently broken, @stpitner is not the only report, I'm inclined to revert so we can cut a hotfix release as I'll be traveling the bulk of the day tomorrow. |
|
Looks like we others are hitting this and we have an issue -> let's move communication over there #295 |
* upstream/master: modernize winrm setup and fix for 2008r2 Updated readme based on issue 300 Correct the docs for image_id Fix 1 last chefstyle warning Require Ruby 2.2.2. Use chefstyle in Rake Remove rack constraint. Removes Ruby 2.1 support Move github_changelog_generator back to the gem spec Remove Rake constraint Switch to Chefstyle Test on Ruby 2.4.0 Remove test-kitchen from the Gemfile as it’s in the spec Cut 1.3.2 Don't try to set tags if there aren't any. Actually bumping version Release 1.3.1 hotfix reinstate default shared creds option Release 1.3.0 In the client, only source creds from the shared file when necessary (test-kitchen#259) fixes test-kitchen#250 and provides the option to set ssl_peer_verify to false
Note: the first 2 commits in this PR represent changes that I had to make in order to get the test suite to run on my machine. Several specs seem to assume that there aren't any AWS credentials set as environment variables on the developer's machine.
The last commit is re: #258 and improves the logic related to how the client sources its AWS credentials.