Keep the last CA when creating a new one #287
Conversation
Keeps the last CA in the K8s API caBundle when generating a new CA. Should help cover when follower replicas haven't received a new leaf cert from the new CA.
helper/cert/source_gen.go
Outdated
| // appendCert returns a new CA bundle: | ||
| // [0] last CA cert from oldCABundle | ||
| // [1] newCACert | ||
| func appendCert(oldCABundle, newCACert []byte) ([]byte, error) { |
There was a problem hiding this comment.
I think this name could be a bit better to capture that it's smarter than a simple append. Maybe prependPreviousCA, prependLastCA, or includePreviousCA, possibly with the argument order swapped? As of course the ordering is important too, because we always want to keep the CAs in age order.
There was a problem hiding this comment.
prependLastCA sounds good to me, added in 5e2c53e
| if next == nil { | ||
| break | ||
| } | ||
| if next.Type == "CERTIFICATE" { |
There was a problem hiding this comment.
What might we throw away in the else clause, we would never expect anything right? Perhaps worth logging a warning to surface anything unexpected?
There was a problem hiding this comment.
It's just what's in the header of the pem block I believe, so it could be CERTIFICATE, PRIVATE KEY, etc. But now, we wouldn't expect anything besides CERTIFICATE. Added a warning log in 4344a67.
helper/cert/source_gen.go
Outdated
| @@ -217,6 +235,40 @@ func (s *GenSource) getBundleFromSecret() (Bundle, error) { | |||
| return bundle, nil | |||
| } | |||
|
|
|||
| func (s *GenSource) getExistingCA(ctx context.Context) []byte { | |||
| var caBundle []byte | |||
There was a problem hiding this comment.
nit: My personal preference is for less mutability where practical, and I think every case in this switch could just immediately return once it finds a result, rather than needing to track a variable throughout the function.
There was a problem hiding this comment.
I'm good with that, changed in 5e2c53e.
helper/cert/source_gen_test.go
Outdated
|
|
||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| result, err := appendCert(tc.oldCAs, newBundle.CACert) |
There was a problem hiding this comment.
Is it worth adding a test case where you go through an additional generation of appendCert? I was thinking yes because of the fact it consumes its own output (eventually), and so it would be nice to ensure by induction that it can continue forever, but after thinking about it a bit more I'm not sure if it really adds much to the coverage. Up to you if you think it has value 🙂
There was a problem hiding this comment.
Doesn't hurt to check, added in 5e2c53e.
- appendCert(oldCAs, newCA) -> prependLastCA(newCA, oldCAs) - immediately return in getExistingCA() switch - test prependLastCA() with its own output
- simpler returns in getExistingCA() - add logging to decodeCerts()
Keeps the last CA in the K8s API caBundle when generating a new CA. Should help cover when follower replicas haven't received a new leaf cert from the new CA.
Keeps the last CA in the K8s API caBundle when generating a new CA. Should cover the case when follower replicas haven't received a new leaf cert from the new CA, and are still listening with a leaf cert from the previous CA.