use lifespan instead of duration for calculating when cert should be rotate

[kevinschoonover]

closes #1612

The current logic for determining how long a certificate rotates depends on the duration (time the certificate expires - the current time) which seems to have weird corner cases that cause the certificate to be renewed at much later than 86-93% of the ttl which was originally intended. This updates the code to determine a rotationTime (i.e. the time that is NotBefore + 86-93% of the lifespan of the certificate) and sleeps until the rotationTime occurs.

Once this is merged, I can follow with a fix for #1646

[divyaac]

Hi @kevinschoonover , this looks great so far!

It would be really nice to have a Unit Test to verify that the correct sleep duration is calculated. Maybe we can create a test in vault_pki_test.go, that tests the goodFor function?

[kevinschoonover]

Hi @divyaac! Thanks for taking a look. I went ahead and added a unit test in vault_pki_test.go for goodFor. Let me know if there is anything else I can add.

[VioletHynes]

Hey @kevinschoonover ! I've been following this with @divyaac and it looks like the Test_VaultPKI_refetch test is failing. Could you look into that and determine if it needs to be updated? Since your code updates the area I want to make sure we're not breaking any previous behaviour.

[VioletHynes]

Also, I wanted to apologize for the delay on getting this reviewed. I know that months is a long time, and this one slipped through the cracks a little bit. This looks good to us though and we'd love to work with you to get this merged. Thank you for your patience and contributions!

[kevinschoonover]

Hey @kevinschoonover ! I've been following this with @divyaac and it looks like the Test_VaultPKI_refetch test is failing. Could you look into that and determine if it needs to be updated? Since your code updates the area I want to make sure we're not breaking any previous behaviour.

It seems like the issue here is with the new behavior of goodFor, but I am not sure how to faithfully test the behavior. Consider the following:

1. d.Fetch gets a initial certificate with a TTL of 5 seconds. It will return a time to sleep on the d.sleepCh that is 4.35 seconds (87% of the lifetime)
2. we then later in the test run d.Fetch
   which waits the 4.35 seconds and then verifies if the certificate should be rotated by running goodFor internally; however, this time it calculates that it should sleep .11 seconds (93% of the lifetime of the certificate after waiting 4.35 seconds) and therefore doesn't get the new certificate

I added a commit which will fix the flakiness by always waiting 93% of the lifetime, but not sure if it keeps the fidelity of the test.

Also, I wanted to apologize for the delay on getting this reviewed. I know that months is a long time, and this one slipped through the cracks a little bit. This looks good to us though and we'd love to work with you to get this merged. Thank you for your patience and contributions!

Thanks for looking at it! No worries, I have a different PR in vault that has been waiting for 1.5 years so I have developed all the patience I need 😉