[spire-agent] Added a jitter in spire agent svid renewal #4534
Conversation
stevend-uber
requested review from
evan2645,
amartinezfayo,
azdagron,
MarcosDY and
rturner3
as code owners
stevend-uber
force-pushed
the
stevend/jitter-svid-renewal
branch
3 times, most recently
from
25bca1b
to
c56e46b
Compare
There was a problem hiding this comment.
Thank you @stevend-uber for this contribution, it looks good to me.
I have some minor suggestions/comments.
| @@ -29,6 +29,15 @@ func TestShouldRotateX509(t *testing.T) { | |||
| require.NoError(t, err) | |||
|
|
|||
| assert.True(t, ShouldRotateX509(mockClk.Now(), badCert)) | |||
|
|
|||
|
|
|||
| // Cert that's is already expired | |||
There was a problem hiding this comment.
| // Cert that's is already expired | |
| // Cert that's already expired |
|
|
||
| // halfLife(beginTime, expiryTime time.Time) | ||
| func TestHalfLife(t *testing.T) { | ||
| // JWT that's valid for 1hr |
There was a problem hiding this comment.
X509-SVID instead of JWT? The halfLife function is only used in the context of X509-SVIDs.
| lifetime := expiryTime.Sub(beginTime) | ||
| return ttl <= lifetime/2 | ||
| return lifetime/2 |
There was a problem hiding this comment.
| return lifetime/2 | |
| return lifetime / 2 |
| // The jitter is calculated as ± 10% of the half-life of the SVID. | ||
| func jitterDelta(halfLife time.Duration) time.Duration { | ||
| // ± 10% of the half-life | ||
| return time.Duration((halfLife)/10) |
There was a problem hiding this comment.
| return time.Duration((halfLife)/10) | |
| return time.Duration(halfLife / 10) |
| @@ -86,3 +95,13 @@ func TestJWTSVIDExpiresSoon(t *testing.T) { | |||
|
|
|||
| assert.True(t, JWTSVIDExpiresSoon(expiredJWT, mockClk.Now())) | |||
| } | |||
|
|
|||
| // halfLife(beginTime, expiryTime time.Time) | |||
There was a problem hiding this comment.
I'm not sure if you meant to keep this comment here?
| @@ -86,3 +95,13 @@ func TestJWTSVIDExpiresSoon(t *testing.T) { | |||
|
|
|||
| assert.True(t, JWTSVIDExpiresSoon(expiredJWT, mockClk.Now())) | |||
| } | |||
|
|
|||
| // halfLife(beginTime, expiryTime time.Time) | |||
| func TestHalfLife(t *testing.T) { | |||
There was a problem hiding this comment.
I'm wondering if we really want to have this test function. Ideally, we should not need to test non-exported functions (that usually leads to fragile tests), but rather cover them through test cases of the exported function. Are there more improvements that we could do in the test ShouldRotateX509 to make sure that a proper jitter is added, within the ranges that we expect?
There was a problem hiding this comment.
Ah, great point. I'll just go ahead and remove it.
stevend-uber
force-pushed
the
stevend/jitter-svid-renewal
branch
from
c56e46b
to
3080792
Compare
|
Thanks @amartinezfayo all comments have been addressed :) |
| // The jitter is calculated as ± 10% of the half-life of the SVID. | ||
| func jitterDelta(halfLife time.Duration) time.Duration { | ||
| // ± 10% of the half-life | ||
| return time.Duration(halfLife / 10) |
There was a problem hiding this comment.
| return time.Duration(halfLife / 10) | |
| return halfLife / 10 |
Signed-off-by: stevend-uber <stevend+github@uber.com>
stevend-uber
force-pushed
the
stevend/jitter-svid-renewal
branch
from
94d2da7
to
83d9cb2
Compare
Signed-off-by: stevend-uber <stevend+github@uber.com> Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Introduces a jitter to the renewal of SVIDs in spire-agent which will allow a more uniform distribution of requests to spire-server rather than thundering herds at the same time.
#4268