Skip to content

ROX-25949: gRPC endpoint to return Secured Cluster TLS certificates#12740

Merged
vladbologa merged 14 commits intomasterfrom
vb/renew-certs
Sep 27, 2024
Merged

ROX-25949: gRPC endpoint to return Secured Cluster TLS certificates#12740
vladbologa merged 14 commits intomasterfrom
vb/renew-certs

Conversation

@vladbologa
Copy link
Contributor

@vladbologa vladbologa commented Sep 18, 2024 •
edited
Loading

Description

This PR adds a new gRPC API that allows Sensor to request fresh Secured Cluster certificates from Central.
The implementation reuses the code that was generating local scanner certificates for Sensor (see #219)

Prior to this PR, we had a gRPC call IssueLocalScannerCertsRequest that returns all the certificates needed for scanner to run (scanner v2 certs + optionally scanner v4 certs, if it's enabled).

This introduces a similar API for the other Secured Cluster certs (sensor, collector, admission-controller), called IssueSecuredClusterCertsRequest that returns all these certs + the CA cert bundled together. To reuse the existing code, I did the following:

  • renamed central/localscanner/certificates.go to central/securedclustercertgen/certificates.go because now the functionality is more generic
  • extracted common code out of IssueLocalScannerCertsRequest
  • added IssueSecuredClusterCertsRequest that reuses the common code extracted above

So before we had:

  • IssueLocalScannerCertsRequest in central/localscanner/ (1)

Now we have:

  • IssueLocalScannerCertsRequest in central/securedclustercertgen (renamed from 1)
  • IssueSecuredClusterCertsRequest in central/securedclustercertgen (added in this PR)

Easier reviewed commit by commit.

User-facing documentation

  • CHANGELOG is updated OR update is not needed
  • documentation PR is created and is linked above OR is not needed

Testing and quality

  • the change is production ready: the change is GA or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • added unit tests
  • added e2e tests
  • added regression tests
  • added compatibility tests
  • modified existing tests

How I validated my change

Added unit & integration tests for the new API, CI should check regressions. The bulk of the testing will be done as part of ROX-25948.

@openshift-ci
Copy link

openshift-ci bot commented Sep 18, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@vladbologa vladbologa changed the title ROX-25949: Provide an API that returns fresh Secured Cluster TLS certificates ROX-25949: API that returns fresh Secured Cluster TLS certificates Sep 18, 2024
@rhacs-bot
Copy link
Contributor

rhacs-bot commented Sep 18, 2024 •
edited
Loading

Images are ready for the commit at 1b383d0.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.6.x-602-g1b383d009b.

@vladbologa vladbologa marked this pull request as ready for review September 18, 2024 12:36
@codecov
Copy link

codecov bot commented Sep 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 95.58824% with 3 lines in your changes missing coverage. Please review.

Project coverage is 48.15%. Comparing base (cec708a) to head (1b383d0).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...ntral/sensor/service/connection/connection_impl.go 92.85% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #12740      +/-   ##
==========================================
+ Coverage   48.14%   48.15%   +0.01%     
==========================================
  Files        2439     2439              
  Lines      174988   175033      +45     
==========================================
+ Hits        84252    84293      +41     
- Misses      83937    83941       +4     
  Partials     6799     6799
Flag Coverage Δ
go-unit-tests 48.15% <95.58%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vladbologa vladbologa changed the title ROX-25949: API that returns fresh Secured Cluster TLS certificates ROX-25949: gRPC endpoint to return Secured Cluster TLS certificates Sep 18, 2024
@vladbologa
Copy link
Contributor Author

vladbologa commented Sep 18, 2024

/retest

@vladbologa
Copy link
Contributor Author

vladbologa commented Sep 19, 2024

/test ocp-4-12-scanner-v4-tests

vladbologa
vladbologa commented Sep 19, 2024
View reviewed changes
SimonBaeumer
SimonBaeumer reviewed Sep 19, 2024
View reviewed changes
Copy link
Contributor

@SimonBaeumer SimonBaeumer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain how the cert generation extends the local scanner cert generation?
A description of the flow of the code would be good and which changes are most notably. Happy to jump on a call.

SimonBaeumer
SimonBaeumer approved these changes Sep 20, 2024
View reviewed changes
Copy link
Contributor

@SimonBaeumer SimonBaeumer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vladbologa
Copy link
Contributor Author

vladbologa commented Sep 20, 2024

/retest

mclasmeier
mclasmeier requested changes Sep 20, 2024
View reviewed changes
@vladbologa vladbologa force-pushed the vb/renew-certs branch 2 times, most recently from e2f3c77 to 68caa07 Compare September 23, 2024 15:08
@vladbologa
Copy link
Contributor Author

vladbologa commented Sep 24, 2024

/retest

@vladbologa vladbologa force-pushed the vb/renew-certs branch 2 times, most recently from 19ddf14 to d0955ba Compare September 25, 2024 09:31
mclasmeier
mclasmeier reviewed Sep 25, 2024
View reviewed changes
mclasmeier
mclasmeier reviewed Sep 26, 2024
View reviewed changes
porridge
porridge approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@porridge porridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@vladbologa
Copy link
Contributor Author

vladbologa commented Sep 27, 2024

/retest

mclasmeier
mclasmeier reviewed Sep 27, 2024
View reviewed changes
mclasmeier
mclasmeier approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@mclasmeier mclasmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, Vlad!

vladbologa and others added 14 commits September 27, 2024 10:09
@vladbologa
Copy link
Contributor Author

vladbologa commented Sep 27, 2024

/retest

@porridge porridge added the auto-retest PRs with this label will be automatically retested if prow checks fails label Sep 27, 2024
@rhacs-bot
Copy link
Contributor

rhacs-bot commented Sep 27, 2024

/retest

1 similar comment
@rhacs-bot
Copy link
Contributor

rhacs-bot commented Sep 27, 2024

/retest

@vladbologa vladbologa mentioned this pull request Sep 27, 2024
Merged
9 tasks
@vladbologa vladbologa merged commit 32860be into master Sep 27, 2024
@vladbologa vladbologa deleted the vb/renew-certs branch September 27, 2024 13:50
@vladbologa vladbologa mentioned this pull request Oct 21, 2024
Merged
9 tasks
@vladbologa vladbologa mentioned this pull request Oct 29, 2024
Merged
9 tasks
@vladbologa vladbologa mentioned this pull request Nov 5, 2024
Merged
9 tasks
@vladbologa vladbologa mentioned this pull request Nov 15, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@porridge porridge porridge approved these changes

@mclasmeier mclasmeier mclasmeier approved these changes

+1 more reviewer

@SimonBaeumer SimonBaeumer SimonBaeumer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/central area/sensor auto-retest PRs with this label will be automatically retested if prow checks fails

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vladbologa @rhacs-bot @porridge @SimonBaeumer @mclasmeier