Add jitter to Default token expiration #260
Conversation
pkg/constants.go
Outdated
| // 24hrs as that is max for EKS | ||
| MaxTokenExpiration = int64(86400) | ||
| // Default token expiration in seconds if none is defined, 22hrs | ||
| DefaultTokenExpiration = int64(79200) |
There was a problem hiding this comment.
The DefaultTokenExpiration is used by both IRSAv1 and IRSAv2 (Pod Identity):
- In IRSAv1, it has one informer cache
- In IRSAv2, we are only using from
GetCommonConfigurations, which is being added from onboarding to IRSAv2 Add support for container credentials method #189
I believe we can add the randomization inside the GetCommonConfigurations function and not change the DefaultTokenExpiration for IRSAv1 use
There was a problem hiding this comment.
I think I have updated GetCommonConfigurations the way you are describing in the most recent commit now. Let me know if I missed somethign
pkg/handler/handler.go
Outdated
| @@ -479,6 +488,26 @@ func (m *Modifier) buildPodPatchConfig(pod *corev1.Pod) *podPatchConfig { | |||
| return nil | |||
| } | |||
|
|
|||
| func addJitter(val int64, jitterPercent int64, min int64, max int64) (int64, error) { | |||
There was a problem hiding this comment.
I think this can be just simplified to a randomization between min and max, we don't need the val. And jitterPercentage and (min, max) is kinda similar, just to add randomization
There was a problem hiding this comment.
Rewrote the function and renamed to addJitterToDefaultToken now that it is more specific
notSoWiseOldMan
force-pushed
the
tokenExp_jitter
branch
from
162e46e to
3eb38ec
Compare
pkg/handler/handler.go
Outdated
| @@ -459,6 +467,7 @@ func (m *Modifier) buildPodPatchConfig(pod *corev1.Pod) *podPatchConfig { | |||
| klog.V(5).Infof("Value of roleArn after after cache retrieval for service account %s: %s", request.CacheKey(), response.RoleARN) | |||
| if response.RoleARN != "" { | |||
| tokenExpiration, containersToSkip := m.parsePodAnnotations(pod, response.TokenExpiration) | |||
| tokenExpiration = m.addJitterToDefaultToken(tokenExpiration) | |||
There was a problem hiding this comment.
We don't require this, this is IRSAv1 path
|
/lgtm |
|
/approve |
|
/approve |
|
/lgtm |
Issue #, if available:
N/A
Description of changes:
Adds a jitter to service account tokens when using the default expiration
Additionally to handle test cases where the randInt function is called I've added mocks and create a mockery config to the root of the repo to help with generating mock interfaces (mockery is recommended by http://github.com/stretchr/testify)
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.