feat(certificates) begin to abstract the cert manager patterns #4580
Conversation
steeling
requested review from
shashankram,
snehachhabria,
nojnhuh,
draychev and
jaellio
as code owners
steeling
force-pushed
the
feature/cert-manager-1
branch
2 times, most recently
from
a1ef803 to
6907a7e
Compare
Codecov Report
@@ Coverage Diff @@
## main #4580 +/- ##
==========================================
- Coverage 69.01% 68.78% -0.24%
==========================================
Files 217 217
Lines 14760 14951 +191
==========================================
+ Hits 10187 10284 +97
- Misses 4521 4615 +94
Partials 52 52
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Overall the refactor makes sense to me. Since this is refactoring sensitive parts of the code pertaining to certificates, could you kindly describe in your commit && PR description the testing that was done? Since we use 24h expiration for short-lived certs by default and we do not have an e2e tests to verify cert rotation, so manually testing cert rotation should suffice. You can do this by changing the serviceCertValidityDuration install option to small duration such as 2m.
pkg/certificate/manager.go
Outdated
| if cert.ShouldRotate() { | ||
| log.Trace().Msgf("Certificate found in cache but has expired SerialNumber=%s", cert.GetSerialNumber()) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I know this is how it was before, but why doesn't this check the cert's expiration directly? ShouldRotate() will return true for a period while the cert has not yet expired.
There was a problem hiding this comment.
getFromCache is called in IssueCertificate. I thought ShouldRotate() was used instead of the cert's expiration directly to avoid returning a certificate that would need to be rotated soon after being issued.
There was a problem hiding this comment.
I agree with Jon here, I think returning a soon-to-expired cert is better than no cert.
Particularly, it allows us to bump up the ShouldRotate time to say 1/3 of total lifetime of cert, without affecting this.
There was a problem hiding this comment.
Actually I spoke to soon. It may make sense to trigger a re-issue on certificates if it is expired, even on a GetCertificate call. Will dig through this a bit more, but since this PR isn't changing existing functionality I propose we don't block. Thoughts?
Will keep this open for further discussion
| m.msgBroker.GetCertPubSub().Pub(events.PubSubMessage{ | ||
| Kind: announcements.CertificateRotated, | ||
| NewObj: newCert, | ||
| OldObj: oldCert, | ||
| }, announcements.CertificateRotated.String()) |
There was a problem hiding this comment.
Not sure if you have a plan for this already, but I'd be interested in seeing if there's a better way we can handle cert rotation events. As-is, there are several mistakes that can be made here:
- You grab the wrong pubsub instance from the broker instead of
GetCertPubSub() - You forget or use the wrong
Kindin the message - You pass the wrong type to
NewObjorOldObjwhich could cause a panic if the subscribers aren't checking for that - You forget or use the wrong topic for the extra arguments to
.Pub()
I have a feeling something as simple as a plain channel or handler functions passed in (like for k8s informers) totally separate from the message broker would be a lot simpler to deal with while removing most or all of those concerns. At a minimum, much of that could be also be addressed by instead of exposing the pubsub instance directly from the broker, define our own .PublishRotatedCert(*Certificate, *Certificate) method on the broker that will handle those finer details.
There was a problem hiding this comment.
Since we're going down to a single place where we do this, I'm think we can be less concerned about introducing bugs than the current state where there was one instance per cert provider.
I think you highlight a good refactoring opportunity for the msgbroker though.
It may make more sense to provide more targeted methods, ie: PublishCertRotation, or provide generic methods that type switch based on the incoming type. Haven't looked at this code too much though
steeling
force-pushed
the
feature/cert-manager-1
branch
3 times, most recently
from
a70d7c0 to
95743fd
Compare
Good call! Done both by adding an automated test, and verifying manually through the demo with cert-manager deployed, and the settings as you suggested. Certs are successfully being rotated and present in envoy configs |
pkg/certificate/providers/certmanager/certificate_manager_test.go
Outdated
Show resolved
Hide resolved
This change pulls out the shared logic between the cert providers into a new manager struct. This is done in pieces, to avoid a single large PR with O(1000)'s of LOC changed. The goal is to reduce all of the existing "providers" to clients that only deal with certificates (no caching, msg broker, logging, or timing). Once all of these are gone, we will remove the certificate.Manager interface and promote the certificate.manager struct to the exported certificate.Manager (struct). Signed-off-by: Sean Teeling <[email protected]>
steeling
force-pushed
the
feature/cert-manager-1
branch
from
95743fd to
ff49723
Compare
…ervicemesh#4580) This change pulls out the shared logic between the cert providers into a new manager struct. This is done in pieces, to avoid a single large PR with O(1000)'s of LOC changed. The goal is to reduce all of the existing "providers" to clients that only deal with certificates (no caching, msg broker, logging, or timing). Once all of these are gone, we will remove the certificate.Manager interface and promote the certificate.manager struct to the exported certificate.Manager (struct). Signed-off-by: Sean Teeling <[email protected]>
…ervicemesh#4580) This change pulls out the shared logic between the cert providers into a new manager struct. This is done in pieces, to avoid a single large PR with O(1000)'s of LOC changed. The goal is to reduce all of the existing "providers" to clients that only deal with certificates (no caching, msg broker, logging, or timing). Once all of these are gone, we will remove the certificate.Manager interface and promote the certificate.manager struct to the exported certificate.Manager (struct). Signed-off-by: Sean Teeling <[email protected]> Signed-off-by: nshankar13 <[email protected]>
Builds on the work done in #4580 - this removes the old Manager interface, and exports the new manager interface, adds fake_manager.go to help with testing where needed - as the previous mockCertManager could not be used anymore. Rotor was moved into manager.go and is started whenever NewManager() is called. Signed-off-by: Sarah Christoff <[email protected]> Co-authored-by: Jon Huhn <[email protected]>
This change pulls out the shared logic between the cert providers into
a new manager struct. This is done in pieces, to avoid a single large PR
with O(1000)'s of LOC changed. The goal is to reduce all of the existing
"providers" to clients that only deal with certificates (no caching, msg broker,
logging, or timing).
Once all of these are gone, we will remove the certificate.Manager interface
and promote the certificate.manager struct to the exported certificate.Manager
(struct).
Eventually the rotor functionality will also be on the certificate.Manager. It looks like this
was done as an artifact of the current split. I had to move rotor.ShouldRotate to certificate
within this PR as opposed to later to avoid circular dependencies.
Signed-off-by: Sean Teeling [email protected]
Please answer the following questions with yes/no.
No
No
Part of #4533