[MAR-3131] Set grace to 0 on non-positive lease duration

[pmmukh]

• There currently exists an arm64 bug, as when converting a negative float into a uint64 value on arm64 results in a 0, causing the grace calculation on L390 to panic due to a modulo by 0 error. ( ref cmd/compile: invalid conversion from a negative float to uint64 on arm64 golang/go#43047 )
• Expanding the current check, to set the grace value to 0 when the lease duration is negative, fixes this error by exiting the function on a negative value of leaseDuration, without causing any regression in the existing tests
• note that there is further discussion around this on this thread https://github.com/hashicorp/vault-enterprise/pull/2095#discussion_r691512265

[ncabatoff]

I wasn't sure about this change initially because I wanted to understand if the negative remainingLeaseDuration hinted at a bug. It looks like it must occur as a result of us reaching the end of the lease's lifetime without having successfully renewed. In that case with this change we'll set grace to 0, and then we'll return nil since the lease has expired, which is what we want.

An alternative that might be a little clearer would be, on line 305, when we assign the new remainingLeaseDuration, check if it's negative, and return nil right away if so.

[pmmukh]

thanks for looking into this @ncabatoff , i really wasn't sure how exactly the negative leaseDuration was passed in, appreciate the eyes! For that L305 potential fix you mentioned, I'm going to give that a shot, see if that stops the invocation of calcGrace with negativeLeaseDuration, if so would also prefer to fix at the source there.

[pmmukh]

@ncabatoff hmm I don't think it's L305 that is causing the negative leaseDuration, at least in the context of the TestLeaseCacheRestore_expired test ( which is the one that was failing for the arm64 job ), so I added the change you suggested, added a print line Got leasedur to calculateGrace, and still seeing negative values getting passed in.

[ncabatoff]

Sorry, I should've cited the line rather than the number. I forgot I'd added some comments to my local checkout to make sense of what was happening. I meant this line:

				remainingLeaseDuration = initialTime.Add(time.Duration(initLeaseDuration) * time.Second).Sub(time.Now())

[pmmukh]

@ncabatoff ah yeah, so that's the line i was looking at too, i added the negative check n return nil right after it. I'm not exactly sure what the TestSendResponse object is used for, but based on this setup line in the test https://github.com/hashicorp/vault/blob/main/command/agent/cache/lease_cache_test.go#L1007, is it possible that we're creating a lease up front with a negative duration ?

[pmmukh]

To add some more context on what i've checked so far, calculateGrace is invoked in 2 places, https://github.com/hashicorp/vault/blob/main/api/lifetime_watcher.go#L260 and https://github.com/hashicorp/vault/blob/main/api/lifetime_watcher.go#L348. From some extra logs, noted that its the first call that is invoked by TestLeaseCacheRestore_expired, when passing negative values. And adding a if value < 0 return nil block there resulted in test failures, so think that might not be a valid thing to do.

[calvn]

Can you included a test in TestLifetimeWatcher (from renewer_test.go) that sets leaseDurationSeconds to a negative value to make sure that doRenewWithOptions performs as expected?