vault: always renew tokens using the renewal loop

[lgfa29]

Previously, a Vault token could renewed either periodically via the renewal loop or immediately by calling RenewToken().

But a race condition in the renewal loop could cause an attempt to renew an expired token. If both updateCh and renewalCh are active (such as when a task stops at the same time its token is waiting for renewal), the following select picks a case at random.

nomad/client/vaultclient/vaultclient.go

Lines 557 to 564 in 78f0c6b

	select {
	case <-renewalCh:
	if err := c.renew(renewalReq); err != nil {
	c.logger.Error("error renewing token", "error", err)
	metrics.IncrCounter([]string{"client", "vault", "renew_token_error"}, 1)
	}
	case <-c.updateCh:
	continue

If case <-renewalCh is picked, the token is incorrectly re-added to the heap, causing unnecessary renewals of a token that is already expired.

nomad/client/vaultclient/vaultclient.go

Lines 505 to 510 in 1604dba

	// If the identifier is not already tracked, this is a first
	// renewal request. In this case, add an entry into the heap
	// with the next renewal time.
	if err := c.heap.Push(req, next); err != nil {
	return fmt.Errorf("failed to push an entry to heap. err: %v", err)
	}

To prevent this situation, the renew() function should only renew tokens that are currently in the heap, so RenewToken() must first push the token to the heap and wait for the renewal to happen instead of calling renew() directly since this could cause another race condition where the token is renewed twice: once by RenewToken() calling renew() directly and a second time if the renewal happens to pick the token as soon as RenewToken() adds it to the heap.

Extracted from #18985

[tgross]

Closing a closed channel causes a panic. If the renewal loop has ownership over closing the channel, should we be more firm with this comment and say that the caller of the function must not close the channel?

[lgfa29]

This part of the comment already existed, so I just kept it as is, but reading again it's indeed weirdly phrased 😅

It's also not necessary because RenewToken returns a read channel and trying to close it is a compilation error: https://go.dev/play/p/MQxK_XyGa-7

I will remove this last sentence.

[tgross]

We're using a variable renewalCh in the run method to pick up timer events to start a renew call (something is about to happen in the near future). Whereas this channel with the same name is signaling that this particular request has been successfully renewed (something has just happened in the recent past). So maybe for clarity we should name this something like completeCh or renewedCh?

[lgfa29]

Hum...good point. I renamed it to renewalLoopCh to be more explicit, but I found very tricky to find a good name 😅

I feel like completeCh and renewedCh are a bit misleading because the operation is not complete (the channel will still receive values) and the token may not have been renewed due to an error.

renewalLoopCh is kind of too specific because it leaks the notion that the renewal loop exists, but this is an internal struct so probably OK?

[tgross]

I'm not sure I understand this block. If we send on the channel and it's blocked, we'll fall through the default and then return. Otherwise, we'll send on the channel and then loop again and try to send again. Only until the channel is blocked do we end up returning? If we're going to return if we're blocked anyways, why have the loop instead of something like this?

defer func() {
	select {
	case req.renewalCh <- struct{}{}:
	default:
	}
}()

Also, it looks like RenewToken is the only method that populates renewalCh and it doesn't return the channel for another caller. So we only ever read from this channel once (in RenewToken), right?

[lgfa29]

Ah yeah, I used a loop here in case there were multiple listeners but never actually happens so I removed the outer for loop.

[tgross]

LGTM 👍