-
Notifications
You must be signed in to change notification settings - Fork 19
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
ARSN-439: Vaut/authenticateV2(4)Request error handling #2265
base: development/7.70
Are you sure you want to change the base?
ARSN-439: Vaut/authenticateV2(4)Request error handling #2265
Conversation
Return ArsenalError in case of InvalidAccessKeyId
Hello dvasilas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Branches have divergedThis pull request's source branch To avoid any integration risks, please re-synchronize them using one of the
Note: If you choose to rebase, you may have to ask me to rebuild |
ping |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command:
Alternatively, the |
if (err.code && err.code === 'InvalidAccessKeyId') { | ||
return callback(errors.InvalidAccessKeyId); | ||
} |
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.
Can we also try this change in cloudserver (I guess cloudserver CI should be enough, but because we don't really use vault there, maybe Integration/Zenko are needed) - just to be on the safe side, so we do not have any issue when the function returns an arsenal error
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.
That's a good point. Not sure if we test this code path, I will check.
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 feel like the issue we are facing is that we are sometimes managing Arsenal errors and other times 'request' errors, depending on the type of error and the backend being used. Should we consider translating 'request' errors into Arsenal errors so that we only need to manage one type of error?
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.
Yes. Adding another note: we recently moved out of the arsenal errors in vaultclient, because we were importing the repo only to use these Arsenal errors (based on some metaprogramming logic, with perf impact). Maybe this is a more global thing to discuss here...
Vaultclient's
verifySignatureV2(4)
returns an error that contains acode
and adescription
field.This error is propagated to the callers of
authenticateV2(4)Request
.However, callers expect an
ArsenalError
.This change fixes this specifically for the case of
InvalidAccessKeyId
(when a non-existing access key is given).Issue: ARSN-439