Skip to content

ROX-26719: Decouple ServiceCertificatesRepo from localscanner package#12825

Merged
vladbologa merged 4 commits intomasterfrom
vb/sensor-extract-certrepo
Oct 6, 2024
Merged

ROX-26719: Decouple ServiceCertificatesRepo from localscanner package#12825
vladbologa merged 4 commits intomasterfrom
vb/sensor-extract-certrepo

Conversation

@vladbologa
Copy link
Contributor

@vladbologa vladbologa commented Sep 26, 2024 •
edited
Loading

Description

#12740 introduces a new Central API to refresh all Secured Cluster certificates.

On the Sensor side, I plan to reuse parts of the existing certificate refresh logic from the localscanner package, which is:

  • currently tightly coupled with the local scanner refresh API
  • required for backward compatibility with older Central versions, even after the new refresh mechanism is introduced

To prepare for this, I'm extracting the ServiceCertificatesRepo from the localscanner package, so that it can be used to handle also other sets of certificates, not just local scanner.

Changes in this PR:

  • move the sensor/kubernetes/localscanner package to sensor/kubernetes/certrefresh/localscanner
  • extract ServiceCertificatesRepo code into ``sensor/kubernetes/certrefresh/certrepo`
  • export the interface and implementation so that it can be used by the future cert refresh code
  • split the existing ServiceCertificatesRepo tests between the two packages (localscanner and certrepo)

There are no functionality changes in this PR - only refactoring, renames, and the necessary updates to fix compilation and tests. It's probably easiest to review 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

Existing tests should verify regressions. There is no new functionality added in this PR.

@openshift-ci
Copy link

openshift-ci bot commented Sep 26, 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-25948: Prefactor - decouple ServiceCertificatesRepo from localscanner package ROX-25948: Decouple ServiceCertificatesRepo from localscanner package Sep 26, 2024
@vladbologa vladbologa force-pushed the vb/sensor-extract-certrepo branch from e482df9 to 1a80aef Compare September 26, 2024 20:11
@vladbologa
Copy link
Contributor Author

vladbologa commented Sep 26, 2024

/test all

@rhacs-bot
Copy link
Contributor

rhacs-bot commented Sep 26, 2024 •
edited
Loading

Images are ready for the commit at abb17fd.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.6.x-595-gabb17fd176.

@codecov
Copy link

codecov bot commented Sep 26, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 78.84615% with 11 lines in your changes missing coverage. Please review.

Project coverage is 48.14%. Comparing base (32860be) to head (abb17fd).
Report is 67 commits behind head on master.

Files with missing lines Patch % Lines
...efresh/certrepo/service_certificates_repository.go 57.69% 11 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #12825      +/-   ##
==========================================
- Coverage   48.16%   48.14%   -0.02%     
==========================================
  Files        2439     2440       +1     
  Lines      175036   175036              
==========================================
- Hits        84304    84270      -34     
- Misses      83934    83964      +30     
- Partials     6798     6802       +4
Flag Coverage Δ
go-unit-tests 48.14% <78.84%> (-0.02%) ⬇️

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
Copy link
Contributor Author

vladbologa commented Sep 27, 2024

/test gke-nongroovy-e2e-tests

@vladbologa vladbologa force-pushed the vb/sensor-extract-certrepo branch from 1a80aef to abb17fd Compare September 27, 2024 13:53
@vladbologa vladbologa marked this pull request as ready for review September 27, 2024 13:55
@vladbologa vladbologa requested a review from a team as a code owner September 27, 2024 13:55
vikin91
vikin91 approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@vikin91 vikin91 left a comment

Choose a reason for hiding this comment

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

All looks good! I left one comment to consider.
Please make sure that CI is happy before merging.

@vladbologa
Copy link
Contributor Author

vladbologa commented Sep 30, 2024

/test gke-qa-e2e-tests

porridge
porridge approved these changes Oct 1, 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.

LGTM modulo the checkboxes in PR description.

@vladbologa vladbologa merged commit d2d04f7 into master Oct 6, 2024
@vladbologa vladbologa deleted the vb/sensor-extract-certrepo branch October 6, 2024 19:06
@vladbologa vladbologa mentioned this pull request Oct 18, 2024
Merged
9 tasks
@vladbologa vladbologa changed the title ROX-25948: Decouple ServiceCertificatesRepo from localscanner package ROX-26719: Decouple ServiceCertificatesRepo from localscanner package Oct 22, 2024
@vladbologa vladbologa mentioned this pull request Oct 24, 2024
Merged
9 tasks
@vladbologa vladbologa mentioned this pull request Nov 5, 2024
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vikin91 vikin91 vikin91 approved these changes

@porridge porridge porridge approved these changes

Assignees

No one assigned

Labels

area/sensor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vladbologa @rhacs-bot @vikin91 @porridge