Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Option for retry-base-delay #30

Merged
merged 4 commits into from Jan 8, 2018
Merged

Option for retry-base-delay #30

merged 4 commits into from Jan 8, 2018

Conversation

yoricksijsling
Copy link
Contributor

Makes the base delay for the vault exponential backoff reconnection
strategy configurable, as requested in #11.

@yoricksijsling
Option for retry-base-delay
6606fcf 
Makes the base delay for the vault exponential backoff reconnection
strategy configurable, as requested in #11.
@yoricksijsling
Copy link
Contributor Author

Difference in delays can be seen when comparing the following (different values for --retry-base-delay result in different real times):

yorick@devm0:~/channable/vaultenv$ time stack exec vaultenv -- --token ABC --secrets-file ~/channable/warpmachine/package/config/warpmachine.secrets --retry-base-delay 10 env
[ERROR] ServerUnreachable error: HttpExceptionRequest Request {
  host                 = "localhost"
  port                 = 8200
  secure               = True
  requestHeaders       = [("x-vault-token","ABC")]
  path                 = "/v1/secret/warpmachine/jwt"
  queryString          = ""
  method               = "GET"
  proxy                = Nothing
  rawBody              = False
  redirectCount        = 10
  responseTimeout      = ResponseTimeoutDefault
  requestVersion       = HTTP/1.1
}
 (InternalException (HostCannotConnect "localhost" [Network.Socket.connect: <socket: 38>: does not exist (Connection refused)]))

real	0m4.298s
user	0m0.164s
sys	0m0.060s
yorick@devm0:~/channable/vaultenv$ time stack exec vaultenv -- --token ABC --secrets-file ~/channable/warpmachine/package/config/warpmachine.secrets --retry-base-delay 100 env
[ERROR] ServerUnreachable error: HttpExceptionRequest Request {
  host                 = "localhost"
  port                 = 8200
  secure               = True
  requestHeaders       = [("x-vault-token","ABC")]
  path                 = "/v1/secret/warpmachine/jwt"
  queryString          = ""
  method               = "GET"
  proxy                = Nothing
  rawBody              = False
  redirectCount        = 10
  responseTimeout      = ResponseTimeoutDefault
  requestVersion       = HTTP/1.1
}
 (InternalException (HostCannotConnect "localhost" [Network.Socket.connect: <socket: 38>: does not exist (Connection refused)]))

real	0m28.652s
user	0m0.228s
sys	0m0.144s

ruuda
ruuda reviewed Jan 3, 2018
View reviewed changes
app/Main.hs Outdated
@@ -108,6 +112,11 @@ optionsParser env = Options
<*> switch
( long "no-inherit-env"
<> help "don't merge the parent environment with the secrets file")
<*> (MilliSeconds <$> option auto
( long "retry-base-delay"
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be a good idea to also put the time unit in the flag: somebody who encounters

vaultenv --retry-base-delay 100 --token ABC --secrets-file ~/channable/warpmachine/package/config/warpmachine.secrets  env

in a shell script or systemd unit file, has no clue whether those are 100 seconds, milliseconds, nanoseconds, or Martian days.

vaultenv --retry-base-delay-milliseconds 100 --token ABC --secrets-file ~/channable/warpmachine/package/config/warpmachine.secrets  env

on the other hand is self-documenting and leaves no room for ambiguity.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @ruuda. I deeply dislike having no certainty about the units of values I have to pass to a program.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! I'll do that

duijf
duijf suggested changes Jan 5, 2018
View reviewed changes
Copy link
Contributor

@duijf duijf left a comment

Choose a reason for hiding this comment

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

Looks good in general! 👍 I have one request and a question (which increases the scope of the PR -- feel free to ignore if you want)

app/Main.hs
( long "retry-base-delay-milliseconds"
<> metavar "MILLISECONDS"
<> value (40 :: Int)
<> help "base delay for vault connection retrying. Defaults to 40ms because, in testing, we found out that fetching 50 secrets takes roughly 200 milliseconds"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also update the usage section in the README with the changed output? (Some other options might also be missing)

app/Main.hs Outdated
@@ -121,16 +130,12 @@ optionsInfo env =
-- | Retry configuration to use for network requests to Vault.
-- We use a limited exponential backoff with the policy
-- fullJitterBackoff that comes with the Retry package.
vaultRetryPolicy :: (MonadIO m) => Retry.RetryPolicyM m
vaultRetryPolicy =
vaultRetryPolicy :: (MonadIO m) => MilliSeconds -> Retry.RetryPolicyM m
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're at it, we can maybe also make maxRetries configurable?

@yoricksijsling
Copy link
Contributor Author

Changed the requested @duijf

fatho
fatho reviewed Jan 5, 2018
View reviewed changes
README.md
--retry-base-delay-milliseconds MILLISECONDS
base delay for vault connection retrying. Defaults to
40ms because, in testing, we found out that fetching
50 secrets takes roughly 200 milliseconds
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, how did we reach the conclusion that 40ms is the right base delay based on the fact that 50 secrets take 200 ms?

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember deciding this with @ruuda, but I don't know how we came to that number anymore and I can't seem to find the old discussion. (Maybe Ruud knows more details)

I remember it being a bit hand-wavy: "this normally takes 200ms to fetch 50 secrets, so if we wait a bit we can hopefully keep this reasonably snappy without running out of retry options or overloading Vault". (Our internal tooling had around that number of secrets at the time).

Copy link
Contributor

@duijf duijf Jan 5, 2018 •
edited

Choose a reason for hiding this comment

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

If you have a better way to think up a number, we can reconsider! 😄

duijf
duijf approved these changes Jan 5, 2018
View reviewed changes
Copy link
Contributor

@duijf duijf left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! Looks good

@yoricksijsling yoricksijsling merged commit 0a9756c into master Jan 8, 2018
@yoricksijsling yoricksijsling deleted the feature/retry-base-delay branch January 8, 2018 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruuda ruuda ruuda left review comments

@duijf duijf duijf approved these changes

@fatho fatho fatho left review comments

@NunoAlexandre NunoAlexandre NunoAlexandre left review comments

@rkrzr rkrzr Awaiting requested review from rkrzr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yoricksijsling @ruuda @duijf @fatho @NunoAlexandre