Added support for credential_process
#6430
Conversation
- This only affects the aws provider - Resolves [Issue serverless#4838](serverless#4838) - Retrieving credentials from the AWS provider is now an asynchronous process
|
I still need to write some tests for the new functionality, but I wanted to get some feedback on the implementation before doing that. I was able to confirm the functionality manually by running the CLI locally. |
|
This functionality caused us to abandon serverless for our project, our CI/CD integration with AWS required profiles in the config since we have a lot of AWS subscriptions, this would be amazing to have support on! Love to do anything I can to help! |
|
Any thoughts from the maintainers on this one? |
There was a problem hiding this comment.
Hey @gshively11!
Thanks for working on this! 👍
This looks really promising. We've changed the awsProvider a lot in the past and I'm not quite sure where we landed. @dschep can you take a look into this PR to see how it fits into the overall picture of credential handling?
There was a problem hiding this comment.
Hey @gshively11 thanks again for working on this 👍
I just merged the master branch into this PR so that it's up to date. Do you have a simple step-by-step instruction handy so that we can test real quick without the need to study the docs and the tools necessary to make this work?
Would love to get this into master ASPA.
@medikoo what do you think? Any objections here?
There was a problem hiding this comment.
Great thanks @gshively11 it's really a worthwhile improvement!
Still, I spotted a few issues. If you have any problems in addressing that, let us know, we're more than open to help in that.
| ? resolve({}) // should not reject, since process creds are optional | ||
| : resolve(credential); | ||
| }; | ||
| credential = new AWS.ProcessCredentials(params); |
There was a problem hiding this comment.
It'll be nice to fallback to this check only when new AWS.SharedIniFileCredentials(params) doesn't return credentials.
|
|
||
| impl.addCredentials(results, profileCredentials); | ||
| const profileCredentials = [ | ||
| BbPromise.resolve(new AWS.SharedIniFileCredentials(params)), |
There was a problem hiding this comment.
I would avoid using BbPromis.resolve, there's no real use case here for that
If function is promise returning, I would wrap it's body, with BbPromise.try, and just use regular return and throw inside. It'll make future async/await migration very straightforward (which can be just about removing the wrap and adding async in front of a function)
| }), | ||
| ]; | ||
| let profileFound = false; | ||
| return BbPromise.each(profileCredentials, credential => { |
There was a problem hiding this comment.
Best if we do not rely on Bluebird's specific API, as it'll make future migration to native promises, more cumbersome.
| } | ||
| return BbPromise.resolve(); |
There was a problem hiding this comment.
Same here, it'll be more migration friendly to just wrap it with BbPromise.try
| ) { | ||
| result.signatureVersion = 'v4'; | ||
| } | ||
| return impl |
There was a problem hiding this comment.
This function was converted from sync to async, and now the memoization (in form of this.cachedCredentials handling) is broken (as this value is now set in async way at not certain point in a future)
Ideally if we convert it, so the whole method is memoized, and abort handling of this.cachedCredentials,
Probably easiest way, is via configuring following in a constructor:
this.getCredentials = _.memoize(this.getCredentials.bind(this))| // req.on('send', function (r) {console.log(r)}); | ||
| persistentRequest(() => | ||
| BbPromise.resolve() | ||
| .then(() => that.getCredentials()) |
There was a problem hiding this comment.
BbPromise.resolve().then(() => that.getCredentials()).then(... is unnecessarily verbose.
It can be just that.getCredentials().then(...
| }) | ||
| .then(() => { | ||
| impl.addEnvironmentCredentials(result, `AWS_${stageUpper}`); // stage specific creds | ||
| return impl.addEnvironmentProfile(result, `AWS_${stageUpper}`); |
There was a problem hiding this comment.
It's probably a good moment to refactor this logic into something more efficient.
What we're currenlty doing is gathering profile credentials for all possible options, one by one. from one of least priority to top priority. If there are credentials for currently processed option, then it overrides the data in result object. So in the end we we have the top priority credential that exists resolved.
What should happen instead is - start with top priority, and stop after first option resolves with credentials, so we do not bother to resolve credentials for profiles which we will not use at all
As this PR now implies a possible async calls for profile creds resolution, then it's quite important we do not make ineffective requests.
|
Thanks for the review @medikoo! We've moved away from serverless, so this is lower on my list of priorities to work on. I'll try to get to this within a few weeks, but if you want to take it over to speed the release, by all means do so, with my thanks. |
|
@gshively11 great thanks for that info, and so far help on that. It's much appreciated! |
|
Hi, any chance to get this merged soon? Or do you need any help to get this finished? |
We will appreciate any help. Thank you! |
|
Rather than use |
|
Closing as it's no longer worked on, it's not close to be done, and hard to take over given amount of changes we had in Framework during last months. Anyway big thanks @gshively11 for giving this a spin! |
|
That's a shame but understandable. I may try and pick it up someday as it still slows us down when switching between terraform and serverless operations. Not enough time at the moment thougt unfortunately |
|
@mungojam PR will be very welcome! Anyway I've added this to #4838 to Serverless team board, so hopefully if not addressed by community we will also address it internally at some point |
What did you implement:
Added support for
credential_processin the AWS provider.was supposed to close: #4838
How did you implement it:
getCredentialsnow returns a promise, since retrieving credentials viacredential_processis an asynchronous process. This does not appear to be a breaking change, since it was only ever referenced inawsProvider.jsand all existing references were updated to handle the new interface.addProfileCredentialsnow also returns a promise, since that's where the newAWS.ProcessCredentialsis implemented.How can we verify it:
You'll need a command that returns credential data in the format described in the AWS docs. Then you'll need to update your
~/.aws/credentialsfile to execute the command usingcredential_process(also described in the above docs).Todos:
Note: Run
npm run test-cito run all validation checks on proposed changesValidate via
npm testValidate via
npm run lint-updatedNote: Some reported issues can be automatically fixed by running
npm run lint:fixValidate via
npm run prettier-check-updatedNote: All reported issues can be automatically fixed by running
npm run prettify-updatedIs this ready for review?: YES (still needs new tests though)
Is it a breaking change?: NO