Vault API Client Tweaks #4817

notnoop · 2018-10-30T16:04:32Z

Some tweaks for Vault Client Token renewal:

Passing a UserAgent header when calling Vault
Increment counter for vault renewal errors
As token vault expiry approaches, set 1s floor for delay

client/vaultclient/vaultclient.go

nomad/vault.go

notnoop · 2018-11-01T14:35:09Z

nomad/vault.go

@@ -1228,8 +1239,18 @@ func (v *vaultClient) EmitStats(period time.Duration, stopCh chan struct{}) {
 		case <-time.After(period):
 			stats := v.Stats()
 			metrics.SetGauge([]string{"nomad", "vault", "distributed_tokens_revoking"}, float32(stats.TrackedForRevoke))
+			metrics.SetGauge([]string{"nomad", "vault", "token_ttl"}, float32(tokenTTL(v.tokenData)/time.Millisecond))


The metric here better has a server label to identify problematic nomad server - but looks like none of the other metrics have server id. I went with consistency in this case, so we can address all metrics together later. I suspect it'd be better to add a global label at the metrics agent level than pollute the metric key.

Users currently can set additional tags in the metrics agent level (e.g. datadog agent), or by setting server specific keys in the Nomad config (e.g. statsd_tags).

nomad/vault.go

dadgar · 2018-11-14T22:58:31Z

nomad/vault.go


-	// Copy all the stats
-	stats.TrackedForRevoke = v.stats.TrackedForRevoke
+	if v.tokenData != nil {


This is only set on establish connection, so after we pass the original expire time, the TTL we display will be negative. We need to update the tokenData's expire time on each renew. That is what lastRenewed was for: https://github.com/hashicorp/nomad/pull/4817/files#diff-87a4c76eca3de54716c55ba6b8c06475L558

nomad/vault.go

dadgar · 2018-11-14T23:02:58Z

nomad/vault.go

@@ -474,13 +477,10 @@ func (v *vaultClient) renewalLoop() {
 		case <-authRenewTimer.C:
 			// Renew the token and determine the new expiration
 			err := v.renew()
-			currentExpiration := v.lastRenewed.Add(time.Duration(v.tokenData.CreationTTL) * time.Second)
+			currentExpiration := v.tokenData.ExpireTime


This is never getting updated; renewal will break

Good catch - thanks. I think we'll need to update the field to protect against leases TTLs being different from creation ttl and/or when we get closer to the token ttl. I'll rework this.

notnoop

Updated the PR to avoid relying on tokenData to be latest, but to simply track expiration time as a field. This is because the renewSelf does not return the full token data and does not contain an expire_time field.

notnoop · 2018-11-16T19:10:00Z

nomad/vault_test.go

@@ -528,6 +530,49 @@ func TestVaultClient_RenewalLoop(t *testing.T) {
 	if ttl == 0 {
 		t.Fatalf("token renewal failed; ttl %v", ttl)
 	}
+
+	if client.currentExpiration.Before(time.Now()) {
+		t.Fatalf("found current expiration to be in past %s", time.Until(client.currentExpiration))


This added check to ensure that after the loop, we track the right expiration. In my earlier attempt, currentExpiration was an past value that caused renewal loop to renew at every loop without any pausing.

schmichael · 2018-11-20T00:45:40Z

nomad/vault.go

@@ -203,16 +210,12 @@ type vaultClient struct {
 	// childTTL is the TTL for child tokens.
 	childTTL string

-	// lastRenewed is the time the token was last renewed
-	lastRenewed time.Time
+	// currentExpiration is the time the current tokean lease expires


Suggested change

// currentExpiration is the time the current tokean lease expires

// currentExpiration is the time the current token lease expires

schmichael · 2018-11-20T20:56:05Z

nomad/vault.go

-func (v *vaultClient) renew() error {
+// returned.  The boolean indicates whether it's safe to attempt to renew again.
+// This method updates the currentExpiration time
+func (v *vaultClient) renew() (bool, error) {


Just FYI we have a RecoverableError type we sometimes use in circumstances like this. Since the recoverable aspect is handled within this package and doesn't cross public API boundaries, I don't think there's a compelling reason to switch to it.

schmichael · 2018-11-20T21:02:39Z

nomad/vault_test.go

+
+	_, err = client.renew()
+	require.NoError(t, err)
+	exp1 := client.currentExpiration


Does reading this variable without locks trigger the race detector?

Looking at race job in [1], I don't see reports of race issues - but I suspect technically there should be. However, given that in this test, we renew on the same goroutine we check the value, it's not an issue; even if there is a another goroutine that could update it later on.

[1] https://travis-ci.org/hashicorp/nomad/jobs/457677344

Add a gauge to track remaining time-to-live, duration of renewal request API call.

Seems like the stats field is a micro-optimization that doesn't justify the complexity it introduces. Removing it and computing the stats from revoking field directly.

Return Vault TTL info to /agent/self API and `nomad agent-info` command.

Keep attempting to renew Vault token past locally recorded expiry, just in case the token was renewed out of band, e.g. on another Nomad server, until Vault returns an unrecoverable error.

dadgar · 2018-12-03T18:16:24Z

nomad/vault.go

+	// TokenTTL is the time-to-live duration for the current token
+	TokenTTL time.Duration
+
+	// TokenExpiry Time is the recoreded expiry time of the current token


Suggested change

// TokenExpiry Time is the recoreded expiry time of the current token

// TokenExpiry Time is the recorded expiry time of the current token

dadgar · 2018-12-03T23:41:02Z

nomad/vault.go

 	data.Root = root
 	v.tokenData = &data
+	v.currentExpiration = time.Now().Add(time.Duration(data.TTL) * time.Second)


There is a read/write race between renew and stats(). Use a lock

github-actions · 2023-02-23T02:17:20Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

dadgar reviewed Oct 31, 2018

View reviewed changes

client/vaultclient/vaultclient.go Outdated Show resolved Hide resolved

preetapan reviewed Oct 31, 2018

View reviewed changes

nomad/vault.go Outdated Show resolved Hide resolved

nomad/vault.go Outdated Show resolved Hide resolved

notnoop commented Nov 1, 2018

View reviewed changes

notnoop force-pushed the b-vault-client-tweaks branch from 2ed23de to 650d04f Compare November 2, 2018 20:31

dadgar requested changes Nov 8, 2018

View reviewed changes

nomad/vault.go Outdated Show resolved Hide resolved

notnoop force-pushed the b-vault-client-tweaks branch from e105d17 to dfaea51 Compare November 9, 2018 18:29

dadgar requested changes Nov 14, 2018

View reviewed changes

notnoop force-pushed the b-vault-client-tweaks branch from 6213e56 to 7dcc554 Compare November 16, 2018 18:57

notnoop commented Nov 16, 2018

View reviewed changes

notnoop force-pushed the b-vault-client-tweaks branch from 7dcc554 to ac85cee Compare November 16, 2018 19:15

schmichael reviewed Nov 20, 2018

View reviewed changes

schmichael approved these changes Nov 20, 2018

View reviewed changes

Mahmood Ali added 14 commits November 20, 2018 17:10

Set User-Agent header when hitting Vault API

feaf621

Set a 1s floor for Vault renew operation backoff

05d9b39

Emit metric counters for Vault token and renewal failures

88c1698

Wrap Vault API api errors for easing debugging

f181f1a

address review comments

0fc84f4

More metrics for Server vault

a0921cc

Add a gauge to track remaining time-to-live, duration of renewal request API call.

Avoid explicit precomputed stats field

887704e

Seems like the stats field is a micro-optimization that doesn't justify the complexity it introduces. Removing it and computing the stats from revoking field directly.

Populate agent-info with vault

0816841

Return Vault TTL info to /agent/self API and `nomad agent-info` command.

reconcile interface

612825d

Track renewal expiration properly

df0cddd

round ttl duration for users

91500c0

fix typo

6445745

Renew past recorded expiry till unrecoverable error

118300a

Keep attempting to renew Vault token past locally recorded expiry, just in case the token was renewed out of band, e.g. on another Nomad server, until Vault returns an unrecoverable error.

nil secrets as recoverable to keep renew attempts

3a57b9c

notnoop force-pushed the b-vault-client-tweaks branch from 7838eb0 to 3a57b9c Compare November 20, 2018 22:13

notnoop merged commit d7f02ed into master Nov 20, 2018

notnoop deleted the b-vault-client-tweaks branch November 21, 2018 02:50

dadgar reviewed Dec 3, 2018

View reviewed changes

notnoop mentioned this pull request Dec 4, 2018

server/vault: Lock Vault expiration tracking #4956

Merged

mlehner616 mentioned this pull request Jan 22, 2019

[Enhancement] Add telemetry for vault token renewal #5222

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vault API Client Tweaks #4817

Vault API Client Tweaks #4817

notnoop commented Oct 30, 2018

notnoop Nov 1, 2018

dadgar Nov 14, 2018

dadgar Nov 14, 2018

notnoop Nov 14, 2018

notnoop left a comment

notnoop Nov 16, 2018

schmichael Nov 20, 2018

schmichael Nov 20, 2018

schmichael Nov 20, 2018

notnoop Nov 20, 2018

dadgar Dec 3, 2018

dadgar Dec 3, 2018

github-actions bot commented Feb 23, 2023

	// currentExpiration is the time the current tokean lease expires
	// currentExpiration is the time the current token lease expires

	// TokenExpiry Time is the recoreded expiry time of the current token
	// TokenExpiry Time is the recorded expiry time of the current token

Vault API Client Tweaks #4817

Vault API Client Tweaks #4817

Conversation

notnoop commented Oct 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notnoop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 23, 2023