Feature/credential provider imds #26
Conversation
| { | ||
| "compilerOptions": { | ||
| "module": "commonjs", | ||
| "target": "es6", |
There was a problem hiding this comment.
Same general comments about setting target to ES5, and importHelpers: true if using ES2015 features like class/async
There was a problem hiding this comment.
Other than the promises in the CredentialProvider interface, the only ES2015 features in use appeared in tests. I'll replace those with ES5-compatible structures.
| @@ -0,0 +1,35 @@ | |||
| export const DEFAULT_TIMEOUT = 1000; | |||
| // TODO I think this should be 0 (in line with other SDKs) and not 3 (in line with JS v2) | |||
There was a problem hiding this comment.
I believe we updated it to be 3 to combat the 'brownouts' we sometimes saw in EC2. To be fair, I think Joyce also added the logic to make sure we weren't making multiple calls to the metadata service in parallel at the same time, so retries might have been overkill.
There was a problem hiding this comment.
The memoize function should have the same effect in V3, as a memoized credential provider will always return the same promise until the underlying credentials are within 5 minutes of their expiration time. Would you be OK with setting retries to 0 by default (but still leaving the code path in place)? Users would still be able to configure the number of retries if they wanted to, and we wouldn't need to push out a big code change (just a change to this constant) if we needed to reintroduce retries.
|
|
||
| const chunks: Array<Buffer> = []; | ||
| res.on('readable', () => { | ||
| chunks.push(res.read()); |
There was a problem hiding this comment.
Calling read() on a stream will return null when there's nothing else to read. With that in mind, we shouldn't push null onto the array of Buffers since Buffer.concat will throw an error if it encounters null.
There was a problem hiding this comment.
Yeah, we don't see any benefit from using the read event here. I'll replace it with the data event.
Feature/credential provider imds
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
Spun off from #5.
This PR adds credential providers that read from the IMDS on EC2 instances and ECS containers. These two providers are bundled together because most of the code pertains to HTTP/translating IMDS format to the SDK's credential format, and that logic does not change from EC2 to ECS.