Rework agent retry config, extend it to cover proxy cache as well #11113
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ch only has num_retries field; if num_retries is 0 or absent, default it to 12 for backwards compat. Configured retries are used for both templating and api proxy, though if template requests go through proxy (currently requires persistence enabled) we'll only configure retries for the latter to avoid duplicate retrying. Though there is some duplicate retrying already because whenever the template server does a retry when not going through the proxy, the Vault client it uses allows for two behind-the-scenes retries for some 400/500 http error codes. Missing: tests for proxy retries; tests for retries with persistent cache.
…ose startedCh instead of writing non-deterministically to it.
…efault NumRetries.
calvn
reviewed
Mar 17, 2021
| @@ -193,8 +188,19 @@ func LoadConfig(path string) (*Config, error) { | |||
| return nil, errwrap.Wrapf("error parsing 'vault':{{err}}", err) | |||
| } | |||
|
|
|||
| if err := parseRetry(result, list); err != nil { | |||
| return nil, errwrap.Wrapf("error parsing 'retry': {{err}}", err) | |||
| if result.Vault == nil { | |||
There was a problem hiding this comment.
Can this ever be nil if we're always assigning it within parseVault (L236, result.Vault = &v)?
There was a problem hiding this comment.
It can be nil if there's no vault{} stanza.
There was a problem hiding this comment.
Ah, I see. We could perform the assignment first within that func (similar to what we do in here), but I'll leave that up to you.
Comment on lines
+196
to
+203
| if result.Vault.Retry == nil { | ||
| result.Vault.Retry = &Retry{} | ||
| } | ||
| switch result.Vault.Retry.NumRetries { | ||
| case 0: | ||
| result.Vault.Retry.NumRetries = ctconfig.DefaultRetryAttempts | ||
| case -1: | ||
| result.Vault.Retry.NumRetries = 0 |
There was a problem hiding this comment.
Can we move this logic into parseRetry?
There was a problem hiding this comment.
I had it there initially, I just moved it out in the most recent commits so that we could have a consistent default behaviour even when there's no vault{} or no vault.retry{} stanza.
There was a problem hiding this comment.
That's fair. I was trying to think if we could keep the assignment/modification of result.Vault.Retry within its parseRetry func.
calvn
reviewed
Mar 17, 2021
Comment on lines
+252
to
+253
| // is enabled. The templating engine and the cache have some inconsistencies | ||
| // that need to be fixed for 1.7x/1.8 |
There was a problem hiding this comment.
Can you expand on the specific inconsitencies (backoff behavior, and perhaps turn the comment into a // TODO: ?
There was a problem hiding this comment.
I don't know what they are, I'm just citing a slack convo with Jason. I just put this comment in here because I was surprised that persistence was related to template use of cache, and decided to put his reply in here for the sake of others who might be similarly confused.
Co-authored-by: Calvin Leung Huang <[email protected]>
calvn
approved these changes
Mar 18, 2021
jasonodonnell
approved these changes
Mar 18, 2021
Will do |
ncabatoff
added a commit
that referenced
this pull request
Mar 18, 2021
…1113) Remove template_retry config section. Add new vault.retry section which only has num_retries field; if num_retries is 0 or absent, default it to 12 for backwards compat with pre-1.7 template retrying. Setting num_retries=-1 disables retries. Configured retries are used for both templating and api proxy, though if template requests go through proxy (currently requires persistence enabled) we'll only configure retries for the latter to avoid duplicate retrying. Though there is some duplicate retrying already because whenever the template server does a retry when not going through the proxy, the Vault client it uses allows for 2 behind-the-scenes retries for some 400/500 http error codes.
ncabatoff
added a commit
that referenced
this pull request
Mar 19, 2021
…1113) (#11136) Remove template_retry config section. Add new vault.retry section which only has num_retries field; if num_retries is 0 or absent, default it to 12 for backwards compat with pre-1.7 template retrying. Setting num_retries=-1 disables retries. Configured retries are used for both templating and api proxy, though if template requests go through proxy (currently requires persistence enabled) we'll only configure retries for the latter to avoid duplicate retrying. Though there is some duplicate retrying already because whenever the template server does a retry when not going through the proxy, the Vault client it uses allows for 2 behind-the-scenes retries for some 400/500 http error codes.
3 tasks
|
A side-effect from this PR that I noticed while working on #11711 is that moving template retry to be performed by the cache's client doesn't always behave the same as the internal retry via vs |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove template_retry config section. Add new vault.retry section which only has num_retries field; if num_retries is 0 or absent, default it to 12 for backwards compat.
Configured retries are used for both templating and api proxy, though if template requests go through proxy (currently requires persistence enabled) we'll only configure retries for the latter to avoid duplicate retrying. Though there is some duplicate retrying already because whenever the template server does a retry when not going through the proxy, the Vault client it uses allows for two behind-the-scenes retries for some 400/500 http error codes.