introduce jitter to secret rotation grace ratio to smooth resource consumption #52102
Conversation
|
😊 Welcome @tkellen! This is either your first contribution to the Istio istio repo, or it's been You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
|
Hi @tkellen. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Overall idea looks good I think |
There was a problem hiding this comment.
Thanks for the contribution. I'm also +1 to this idea pending @howardjohn's comments and some tests
6aaf620 to
6e45601
Compare
|
Thanks ya'll! Updated per your comments and added some tests. |
6e45601 to
ca68a16
Compare
|
/ok-to-test |
| var rotateTime = func(secret security.SecretItem, graceRatio float64) time.Duration { | ||
| var rotateTime = func(secret security.SecretItem, graceRatio float64, graceRatioJitter float64) time.Duration { | ||
| // stagger rotation times to prevent large fleets of clients from renewing at the same moment. | ||
| jitter := (rand.Float64() * graceRatioJitter) * float64(rand.IntN(3)-1) |
There was a problem hiding this comment.
Linter isn't happy about math/rand/v2. What do ya'll think? Do we care about using crypo/rand for this purpose? I can update if so, or just put // # nosec G402 - cyrpto/rand is not worth the cost here on this line.
There was a problem hiding this comment.
I think just opting out here is fine
9b960d4 to
ab046da
Compare
| var rotateTime = func(secret security.SecretItem, graceRatio float64) time.Duration { | ||
| var rotateTime = func(secret security.SecretItem, graceRatio float64, graceRatioJitter float64) time.Duration { | ||
| // stagger rotation times to prevent large fleets of clients from renewing at the same moment. | ||
| jitter := (rand.Float64() * graceRatioJitter) * float64(rand.IntN(3)-1) |
There was a problem hiding this comment.
I think just opting out here is fine
ab046da to
1bf49e0
Compare
When running large fleets of pods deployed at the same time, the lack of jitter in mTLS workload certificate renewal can result in unnecessarily spike-y resource consumption (e.g. 100s of proxies renewing simultaneously). This PR is a quick and dirty suggested resolution for discussion. If something like this would be considered for inclusion I will happily write tests and modify documentation where needed. Your thoughts requested!