Skip to content

Retry on 412 status codes#333

Merged
tomhjp merged 2 commits into
mainfrom
vault-6556/retry-on-412
Jun 29, 2022
Merged

Retry on 412 status codes#333
tomhjp merged 2 commits into
mainfrom
vault-6556/retry-on-412

Conversation

@tomhjp

@tomhjp tomhjp commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

Fixes #332 - see issue for details.

412 means the token used has not yet been replicated to the performance replica queried, so we can append it to the list of retryable HTTP codes.

@swenson swenson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread src/action.js
// Vault returns 412 when the token in use hasn't yet been replicated
// to the performance replica queried. See issue #332.
412,
]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we retry on 500's (except 501) too? This is what the Vault Go API client does via go-retryablehttp.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe 500 and a few other 5XX are already included in the defaults for the got dependency: https://github.com/sindresorhus/got/blob/HEAD/documentation/7-retry.md#retry

I'd like to give that a test to be sure we're preserving that default properly though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's interesting. I wouldn't recommend retrying on 500 on POST/PATCH messages, because they aren't guaranteed to be idempotent and it is possible that the server is in a weird state after the 500. GET, PUT, and DELETE should be safe to retry on 500 though.

But I would recommend retrying 400 and 404 for all verbs.

@tvoran tvoran Jun 20, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How many times will this retry? Should we set a default limit? https://github.com/sindresorhus/got/blob/HEAD/documentation/7-retry.md#limit

We might want a configurable retry number as in this PR as well: #337

@tomhjp tomhjp Jun 27, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't recommend retrying on 500 on POST/PATCH messages

By default it never retries on POST or PATCH, although vault-action doesn't support any methods other than GET anyway.

But I would recommend retrying 400 and 404 for all verbs.

Is this so we get reads after writes for pre-1.10? If we want to go down that route I think we should take advantage of the replication state headers from Vault instead, i.e. do similar to ReadYourWrites from the api package.

How many times will this retry? Should we set a default limit?

The default is up to 2 retries, for a total of 3 attempts. Yeah it would be nice to add a configurable for that, although I'll leave that feature for a separate PR.

@tvoran tvoran mentioned this pull request Jun 21, 2022
@tomhjp
tomhjp force-pushed the vault-6556/retry-on-412 branch from fb9968a to c91860c Compare June 29, 2022 11:51
@tomhjp

tomhjp commented Jun 29, 2022

Copy link
Copy Markdown
Contributor Author

I'm going to merge for now, but happy to address any more follow ups in another PR.

@tomhjp
tomhjp merged commit 55a1167 into main Jun 29, 2022
@tomhjp
tomhjp deleted the vault-6556/retry-on-412 branch June 29, 2022 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Retry in case of 412 errors

4 participants