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

Fix assuming a role with an AWS profile #5739

Merged
merged 5 commits into from Jan 28, 2019

Conversation

Projects
3 participants
@piohhmy
Copy link
Contributor

piohhmy commented Jan 23, 2019

What did you implement:

Closes #5673 and #5675

How did you implement it:

Identified that the one line change in #5650 introduced the regression. Changed the clone to be less aggressive to allow tests in #5650 to still pass while fixing the usage of profiles. Honestly, did not look more into exactly why this fixes it, but confirmed it does fix it. I assume it has to do with the deepCopy breaking something once the credentials are passed to sdk in line 269 of lib/plugins/aws/provider/awsProvider.js

How can we verify it:

Try to do any serverless CLI action with an --aws-profile set. Even a simple sls info --aws-profile <profile-name> will hang in versions 1.36+. This PR fixes it.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@pmuens

pmuens approved these changes Jan 23, 2019

Copy link
Member

pmuens left a comment

Nice find. Thanks @piohhmy 👍

@pmuens pmuens added the pr/in-review label Jan 23, 2019

@pmuens pmuens self-assigned this Jan 23, 2019

@pmuens pmuens added this to In progress in Serverless via automation Jan 23, 2019

@pmuens
Copy link
Member

pmuens left a comment

Great! Thanks for working on a fix @piohhmy 👍 💯

Can you add a regression test which reproduced the old behavior so that we can be sure that we won't break it again?

I tried to reproduce the issue with my setup, but (un)forunately running (serverless info --aws-profile <my-profile> won't cause the info command to hang. I'm running the latest master 🤔

Serverless automation moved this from In progress to Needs review Jan 23, 2019

@pmuens
Copy link
Member

pmuens left a comment

D'oh just messed something with the reviews up. Sorry 😬

Pretty sure we're super close to get this in though.

@piohhmy

This comment has been minimized.

Copy link
Contributor Author

piohhmy commented Jan 24, 2019

Ah ok, thanks for taking a look at it @pmuens! I gave it another quick look and you're right, it appears this bug is only when assuming a role via STS. e.g. when you have an aws profile that looks like:

[dummy]
role_arn = arn:aws:iam::111111111111:role/TestRole
source_profile = source

At first glance it may be tricky to write a test case for this as it relies on the AWS SDK behavior of actually getting the async credentials. Perhaps a unit test with some stubbing would be the way to go over an integration test. I can give it a shot in next couple days if this can wait..

@piohhmy piohhmy changed the title Fix broken profiles Fix assuming a role with an AWS profile Jan 24, 2019

@piohhmy

This comment has been minimized.

Copy link
Contributor Author

piohhmy commented Jan 24, 2019

This also looks like it will fix #5713

@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented Jan 24, 2019

Great! Thanks for taking the time to look into this and explaining the behavior @piohhmy 👍

Perhaps a unit test with some stubbing would be the way to go over an integration test. I can give it a shot in next couple days if this can wait..

Yes that sounds like a good plan. I'd also vote for a simple unit test here. You should find enough examples where we stub the AWS SDK in the test file(s).

Let us know if you need any help 👍

piohhmy added some commits Jan 26, 2019

@piohhmy

This comment has been minimized.

Copy link
Contributor Author

piohhmy commented Jan 26, 2019

@pmuens test added as requested 👍

Serverless automation moved this from Needs review to Reviewer approved Jan 28, 2019

@pmuens

pmuens approved these changes Jan 28, 2019

Copy link
Member

pmuens left a comment

Great! Thanks for taking the time to update this @piohhmy 👍

I just pushed merged master into this branch and updated the tests so that it won't error out when one runs it on the local machine with AWS credentials setup.

Tested it and it works on my end.

Will merge once the build is green :shipit:

@pmuens pmuens merged commit 6e289d0 into serverless:master Jan 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Serverless automation moved this from Reviewer approved to Done Jan 28, 2019

@pmuens pmuens added this to the 1.36.4 milestone Feb 5, 2019

@shortjared shortjared added the bug label Feb 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment