-
Notifications
You must be signed in to change notification settings - Fork 123
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
Add expiration buffer to prevent credentials to expire earlier than request may finish. #678
Add expiration buffer to prevent credentials to expire earlier than request may finish. #678
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I have some asks below.
I think we need to document this in https://github.com/opensearch-project/opensearch-js/blob/main/USER_GUIDE.md#authenticate-with-amazon-opensearch-service at the very least, please?
Does it make sense to add an option that lets you configure this value per client instance? Thinking of this *2
particularly.
If you feel super motivated we do want to extract authentication into a more detailed user guide similar to https://github.com/opensearch-project/opensearch-py/blob/main/guides/auth.md, so we could keep a short version in the main user guide and all the detailed options and options like this one in the detailed one.
lib/aws/AwsSigv4Signer.js
Outdated
@@ -102,6 +103,9 @@ function AwsSigv4Signer(opts = {}) { | |||
} | |||
|
|||
const currentCredentials = credentialsState.credentials; | |||
// For AWS SDK V3 make sure token is valid at least until request has timed out. | |||
const EXPIRATION_BUFFER_MS = toMs(options.requestTimeout || this.requestTimeout) * 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a constant, lowercase and maybe name it to something more specific, e.g. credentialsExpireIn
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will lowercase it, but personally I'd leave buffer
in its name, so it could be, e.g. expiryBufferMs
.
Because buffer
gives instant understanding of what's the purpose of this variable, unlike credentialsExpireIn
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A buffer usually a space/memory thing. But not a hill I want to die on :)
I don't think it makes sense to allow altering this value - one can already do this by changing requestTimeout, plus that's the whole point - make sure credentials are valid until request is made. Updates:
I could help with detailed user guide, but I think it should be done in a standalone PR, that I can work on during Holidays. |
Signed-off-by: Ivan <[email protected]>
Signed-off-by: Ivan <[email protected]>
Signed-off-by: Ivan <[email protected]>
Signed-off-by: Ivan <[email protected]>
a5bfec6
to
37abd8e
Compare
Signed-off-by: Ivan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thank for making this :)
Description
Use 2x
requestTimeout
value asexpiration_buffer_ms
to make sure that whenever we make request credentials are valid for longer than request may be active and not time out (with added safety by using 2x value).Issues Resolved
Fixes this:
#677
Check List
yarn run lint
doesn't show any errorsBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.