Issue #274: Avoid premature vault token renewals #314
Conversation
Don't attempt Vault token renewals if the token isn't renewable. If it is, don't renew the token with each refresh; only do so if the token would expire shortly. Increase the token's lifetime by its original TTL.
|
The builds failed because of network issues between Travis and HashiCorp:
I don't think I can retry them. |
cert/vault_source.go
Outdated
| return c, nil | ||
| } | ||
|
|
||
| func (s *VaultSource) setToken(c *api.Client) error { | ||
| func (s *VaultSource) setAuth(c *api.Client) error { | ||
| firstCall := true |
There was a problem hiding this comment.
This looks racy and should probably be inside the lock.
There was a problem hiding this comment.
Technically not a race (all reads and writes are inside the lock), but I moved it down anyway to remove any doubts.
There was a problem hiding this comment.
Right, since this is a local var but then this doesn't make sense. The defer function should then read and you can drop firstCall.
defer func() {
c.SetToken(s.auth.token)
if s.auth.token != "" {
s.checkRenewal(c)
}
s.mu.Unlock()
}There was a problem hiding this comment.
s.auth.token will never be empty when the deferred function executes (assuming there is no error). Apparently I made this is too confusing. I think we can use sync.Once to make it obvious. Let my try that.
There was a problem hiding this comment.
I also suggest to remove the SetToken call from the defer method and put it where necessary. There is too much magic going on here which is hard to follow.
cert/vault_source.go
Outdated
| // checkRenewal checks if the Vault token can be renewed, and if so when it | ||
| // expires and how big the renewal increment should be. | ||
| func (s *VaultSource) checkRenewal(c *api.Client) { | ||
| response, err := c.Auth().Token().LookupSelf() |
cert/vault_source.go
Outdated
| return | ||
| } | ||
|
|
||
| if data.Renewable { |
There was a problem hiding this comment.
pls rewrite as switch statement. I try to avoid if/else cascades.
cert/vault_source.go
Outdated
| return nil | ||
| } | ||
|
|
||
| response, err := c.Auth().Token().RenewSelf(s.auth.renewIncrement) |
cert/vault_source.go
Outdated
| case ttl < 2*s.Refresh: | ||
| // Renew the token if it isn't valid for two more refresh intervals. | ||
| break | ||
| case ttl < 1*time.Minute: |
There was a problem hiding this comment.
pls drop the 1* since it is redundant.
There was a problem hiding this comment.
It is redundant, but it reads as "one minute" which I prefer. This is a constant expression that will be evaluated at compile time, so there is no runtime impact. If you insist I'll change it though.
There was a problem hiding this comment.
I don't insist but I'd prefer since it is redundant :)
cert/vault_source.go
Outdated
| auth struct { | ||
| token string // actual token | ||
| expireTime time.Time // zero value if the token isn't renewable | ||
| renewIncrement int |
There was a problem hiding this comment.
pls add a comment and explain the unit. Maybe a shorter name, e.g. renewTTL
|
Please take another look. I will fixup into a single commit once you're good with the PR. |
cert/vault_source.go
Outdated
| token string // actual token | ||
| expireTime time.Time // zero value if the token isn't renewable | ||
| renewIncrement int | ||
| token string // actual token |
There was a problem hiding this comment.
pls use the Go doc standard here:
// token is the actual token
token string
// expireTime is ...
expireTime time.Time
...
cert/vault_source.go
Outdated
|
|
||
| ttl := time.Until(data.ExpireTime) | ||
| ttl = ttl / time.Second * time.Second // truncate to seconds | ||
| log.Printf("[WARN] vault: Token is not renewable and will expire %s from now (at %s)", |
There was a problem hiding this comment.
The logging is the default case, right?
pls change the log msg to Token is not renewable and expires in %s from now at %s
|
Don't bother. I can squash the merge myself. |
|
@pschultz Cheeky side question: Would you mind me adding your logo to the README? :) |
|
Sure, you can add the logo. We don't have an English site you can link to though, only the one in German. |
|
I think this is more readable. What do you think? func (s *VaultSource) setAuth(c *api.Client) error {
s.mu.Lock()
defer s.mu.Unlock()
if s.auth.token != "" {
return nil
}
if s.vaultToken == "" {
return errors.New("vault: no token")
}
// did we get a wrapped token?
resp, err := c.Logical().Unwrap(s.vaultToken)
if err != nil {
// unwrapping failed?
if !strings.HasPrefix(err.Error(), "no value found at") {
return err
}
s.auth.token = s.vaultToken
} else {
log.Printf("[INFO] vault: Unwrapped token %s", s.vaultToken)
s.auth.token = resp.Auth.ClientToken
}
s.auth.once.Do(func() { s.checkRenewal(c) })
c.SetToken(s.auth.token)
return nil
} |
|
Yeah, that's better. The last if/else statements can even be done with a nice switch. See f6e2463. |
|
LGTM |
Don't attempt Vault token renewals if the token isn't renewable. If it
is, don't renew the token with each refresh; only do so if the token
would expire shortly. Increase the token's lifetime by its original TTL.
This now requires the "read" capability for the "auth/token/lookup-self" path in Vault. That is granted by default to all tokens. If the lookup-self request fails for any reason, fabio will assume that the token is not renewable.
No new configuration is required. Automatic renewal will work as long as the refresh interval is smaller than the token TTL, same as before.