ROX-26719: Repository for Secured Cluster certificates#13023
ROX-26719: Repository for Secured Cluster certificates#13023vladbologa merged 6 commits intomasterfrom
Conversation
|
Images are ready for the commit at 18c4edb. To use with deploy scripts, first |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13023 +/- ##
==========================================
+ Coverage 48.29% 48.32% +0.02%
==========================================
Files 2455 2457 +2
Lines 176836 176855 +19
==========================================
+ Hits 85408 85459 +51
+ Misses 84556 84530 -26
+ Partials 6872 6866 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
5126995 to
fc8cf84
Compare
9798dcb to
f70d284
Compare
vikin91
left a comment
There was a problem hiding this comment.
Review summary
The change looks good, but I would love to see those tests and test helpers organized differently - changing that would be sufficient to make it mergable for me.
PR description
However, I would like to highlight something else. When looking at that PR (and the other one from the past), I asked myself why are we introducing such changes? I navigated to Jira ticket and it looks like its description does not fit the contents of this PR. I checked the design doc and also found nothing. I am missing here a WHY section and the PR description does not offer it.
- "This PR adds the securedcluster.NewServiceCertificatesRepo function" - I think it was already there
- "It uses a new naming scheme for the certificates, so that it does not conflict with the legacy certificates" - then the old certs stop being fetchable after merging this? Pleas provide some more on the compatibility part.
- "It also moves the FetchSensorDeploymentOwnerRef function from the localscanner package and exports it, because it is useful for calling NewServiceCertificatesRepo (which requires an owner reference parameter)." - that paragraph is clear.
Validation
The paragraph
Tested manually on a local deployment that the certs are fetched and stored. (needs additional code that is not part of this PR)
makes me a bit worried, as theNewServiceCertificatesRepowas already there (even before #12825) so we should not need a new code to make sure that it works. Was the old function unused? How do we make sure that all code paths that used this function are still okay?
The |
9e20f7a to
0708ae8
Compare
vikin91
left a comment
There was a problem hiding this comment.
Vlad, that is a great PR description! Many thanks for making this PR so much better.
I currently do not see any issues with the code, so I think it can be merged like this.
When new code that uses this contribution appears in the future I may look at it again with some additional context, but for now all looks good!
porridge
left a comment
There was a problem hiding this comment.
Nice! A few nitpicks/questions inline.
sensor/kubernetes/certrefresh/securedcluster/cert_repo_init_test.go
Outdated
Show resolved
Hide resolved
0708ae8 to
18c4edb
Compare
|
/retest |
Description
Follow-up of #12825
Why this change is needed
#12740 introduced a new gRPC endpoint to retrieve secured cluster certificates. What I'm trying to achieve with the current set of Sensor PRs is to reuse the old Sensor mechanism to refresh local certificates, so that it can retrieve and refresh full secured cluster certificates, while keeping the previous functionality intact.
It's important to note that both mechanisms will exist in parallel (for a while), because Sensor needs to be compatible with old versions of Centrals that don't have the new
IssueSecuredClusterCertsRequestAPI. There will be a newSensorComponentthat uses theIssueSecuredClusterCertsRequestAPI, and my intention is to reuse as much code between them as possible, without changing the behaviour of the existing component (that only refreshes local scanner certs).What this change does
This PR is partly a prefactor that doesn't change any existing functionality at all.
It adds the
securedcluster.NewServiceCertificatesRepofunction (similar tolocalscanner.NewServiceCertificatesRepo) that creates a certificate repo to store and retrieve Secured Cluster certificates. It uses a new naming scheme for the certificates, so that it does not conflict with the legacy certificates.After this PR is merged, two different functions will exist:
localscanner.NewServiceCertificatesRepo(the one that was already there)securedcluster.NewServiceCertificatesRepo(the new one that isn't used yet)It also moves the
FetchSensorDeploymentOwnerReffunction from thelocalscannerpackage and exports it, because it is useful for callingNewServiceCertificatesRepo(which requires an owner reference parameter).To create secured clusters secrets from a
*storage.TypedServiceCertificateSet, you'd have to:certrefresh.FetchSensorDeploymentOwnerRefsecuredcluster.NewServiceCertificatesRepocertrepo.EnsureServiceCertificatesto store the certificates as secretsNew certificate naming scheme
securedcluster.NewServiceCertificatesRepostores certificates with a new naming scheme, but currently there is no code that creates these new certificates, nor loads them. Loading will be handled by https://issues.redhat.com/browse/ROX-26055Either way the old certificates are still there (e.g. the init bundle certificates + the local scanner ones) and will be used as before.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Added tests for
FetchSensorDeploymentOwnerRef, which already existed but didn't have coverage.Added test for
securedcluster.NewServiceCertificatesRepoTested manually on a local deployment that the certs are fetched and stored. (needs additional code that is not part of this PR)