ROX-27394: Sensor offline mode support in TLS issuer by vladbologa · Pull Request #13596 · stackrox/stackrox

vladbologa · 2024-12-12T15:57:40Z

Description

This PR adds explicit handling for Sensor offline mode in the TLS issuer components. As these components cannot do anything useful in offline mode, the certificate refresh logic is now only active when Sensor is online.
(note: the TLS issuer was already resilient to Sensor being offline due to its retry mechanism)

Main changes:

start the certificate refresher only when the component is both started and online
changed behavior of Start / Stop so that they can be called in the wrong order (e.g. calling Start several times in a row is now fine)
Perform the Central capability check when Sensor enters online mode
integration tests that cycle through online and offline modes
unit test for Central capability check
some testing code clean-up

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

How I validated my change

Manual smoke test that shows that the Secured Cluster cert refresher works:

# Disable CRS to trigger a cert refresh on startup (otherwise the CRS logic retrieves the certs)
ROX_DEPLOY_SENSOR_WITH_CRS=false SENSOR_HELM_MANAGED=true ./deploy/k8s/deploy-local.sh

# Wait for sensor to be in Ready state

❯ kubectl -n stackrox get secret | grep tls-
tls-cert-scanner-v4-db                                    Opaque               3      5m31s
tls-cert-sensor                                           Opaque               3      5m31s
tls-cert-collector                                        Opaque               3      5m30s
tls-cert-admission-control                                Opaque               3      5m30s
tls-cert-scanner                                          Opaque               3      5m29s
tls-cert-scanner-db                                       Opaque               3      5m29s
tls-cert-scanner-v4-indexer                               Opaque               3      5m29s

Sensor logs:

kubernetes/certrefresh: 2025/02/20 15:59:19.365620 tls_issuer_common.go:139: Info: Component runs now in Online mode
...
kubernetes/certrefresh: 2025/02/20 15:59:21.641724 cert_refresher.go:110: Warn: secured cluster not found (this is expected on a new deployment), will refresh certificates immediately: 7 errors occurred:
	* secrets "tls-cert-scanner-db" not found
	* secrets "tls-cert-scanner-v4-indexer" not found
	* secrets "tls-cert-scanner-v4-db" not found
	* secrets "tls-cert-sensor" not found
	* secrets "tls-cert-collector" not found
	* secrets "tls-cert-admission-control" not found
	* secrets "tls-cert-scanner" not found

kubernetes/certrefresh: 2025/02/20 15:59:24.442826 cert_refresher.go:85: Info: successfully refreshed secured cluster for: SENSOR_SERVICE, COLLECTOR_SERVICE, ADMISSION_CONTROL_SERVICE, SCANNER_V4_DB_SERVICE, SCANNER_DB_SERVICE, SCANNER_SERVICE, SCANNER_V4_INDEXER_SERVICE
kubernetes/certrefresh: 2025/02/20 15:59:24.442884 cert_refresher.go:46: Info: secured cluster scheduled to be refreshed in 3604h37m14.557118633s

openshift-ci · 2024-12-12T15:57:44Z

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

rhacs-bot · 2024-12-12T16:22:31Z

Images are ready for the commit at 949d12a.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-17-g949d12a184.

vikin91

Vlad, I just quickly browsed the code and have two impressions:

The changes required to handle offline mode look pretty complex. This is not typical. Are you sure that we cannot simplify somewhere?
I read the PR description (it is great and all points make sense) and I think you still need to add the integration tests. They are pretty important for testing offline mode as they typically uncover many problems. Please make sure to add them.

I will go over the code properly soon (I hope).

vladbologa · 2024-12-19T10:52:48Z

Vlad, I just quickly browsed the code and have two impressions:

The changes required to handle offline mode look pretty complex. This is not typical. Are you sure that we cannot simplify somewhere?

Most changes are in the tests (and I also extracted some common test functions).
I'm also applying the same changes to 2 Sensor components (LocalScannerTLSIssuer the legacy component, and SecuredClusterTLSIssuer the new component), which makes the change look bigger.

But otherwise I wouldn't say that the changes are complex, it's mostly moving some code from the Start/Stop methods to goOnline/goOffline.

I read the PR description (it is great and all points make sense) and I think you still need to add the integration tests. They are pretty important for testing offline mode as they typically uncover many problems. Please make sure to add them.

I did add tests for cycling between offline and online for both components here:

stackrox/sensor/kubernetes/certrefresh/securedcluster_tls_issuer_test.go

Line 326 in e88e47f

    
           func (s *securedClusterTLSIssuerIntegrationTests) TestSensorOnlineOfflineModes() {

and here:

stackrox/sensor/kubernetes/certrefresh/localscanner_tls_issuer_test.go

Line 292 in e88e47f

    
           func (s *localScannerTLSIssuerIntegrationTests) TestSensorOnlineOfflineModes() {

EDIT: The above refers to an older version of this PR. The code has changed significantly since then, and now there are no distinct LocalScannerTLSIssuer and SecuredClusterTLSIssuer, they are now both the same type, with different configuration.

codecov · 2025-02-03T10:40:51Z

Codecov Report

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

Project coverage is 49.04%. Comparing base (d4e1ed2) to head (949d12a).
Report is 85 commits behind head on master.

Files with missing lines	Patch %	Lines
sensor/kubernetes/certrefresh/tls_issuer_common.go	94.33%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13596      +/-   ##
==========================================
- Coverage   49.07%   49.04%   -0.04%     
==========================================
  Files        2514     2524      +10     
  Lines      182769   183446     +677     
==========================================
+ Hits        89691    89963     +272     
- Misses      85956    86363     +407     
+ Partials     7122     7120       -2

Flag	Coverage Δ
go-unit-tests	`49.04% <95.08%> (-0.04%)`	⬇️

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.

vikin91 · 2025-02-12T14:25:57Z

Sorry @vladbologa , I didn't make it with the review before my PTO. Do you maybe want to ping others from Maple for a review?

lvalerom

I need to have a closer look a the tests but here's some early feedback to the production logic.

sensor/kubernetes/certrefresh/tls_issuer_common.go

vladbologa · 2025-02-20T16:03:56Z

/retest

lvalerom

I think the handling of the cancel function should be protected with a mutex but other than that looks really good.

pkg/concurrency/retry_ticker.go

sensor/kubernetes/certrefresh/tls_issuer_common.go

lvalerom

LGTM! Thank you for making the changes!

openshift-ci bot added the do-not-merge/work-in-progress label Dec 12, 2024

github-actions bot added the area/sensor label Dec 12, 2024

vladbologa mentioned this pull request Dec 12, 2024

ROX-27014: Cert refresh concurrency clean-up #13369

Merged

9 tasks

vladbologa changed the title ~~WIP: Support Sensor offline mode in TLS issuer~~ ROX-27394: Support Sensor offline mode in TLS issuer Dec 12, 2024

vladbologa force-pushed the vb/sensor-offline-cert-refresh branch 5 times, most recently from dbece3b to d2a3e4c Compare December 17, 2024 12:55

vladbologa changed the title ~~ROX-27394: Support Sensor offline mode in TLS issuer~~ ROX-27394: Sensor offline mode support in TLS issuer Dec 17, 2024

vladbologa force-pushed the vb/sensor-offline-cert-refresh branch from 0c5f579 to 74476fc Compare December 17, 2024 14:11

vladbologa marked this pull request as ready for review December 17, 2024 14:58

vladbologa requested a review from a team as a code owner December 17, 2024 14:58

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 17, 2024

vladbologa requested a review from vikin91 December 17, 2024 14:58

vikin91 reviewed Dec 19, 2024

View reviewed changes

Base automatically changed from vb/cert-refresh-channels to master January 7, 2025 11:08

vladbologa marked this pull request as draft January 9, 2025 13:52

openshift-ci bot added the do-not-merge/work-in-progress label Jan 9, 2025

vladbologa mentioned this pull request Jan 9, 2025

ROX-27014: Cert refresh testing cleanup - extract verifySecrets #13650

Merged

9 tasks

vladbologa force-pushed the vb/sensor-offline-cert-refresh branch from e88e47f to f7d07e3 Compare January 31, 2025 13:36

vladbologa marked this pull request as ready for review January 31, 2025 14:52

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 31, 2025

vladbologa requested a review from vikin91 January 31, 2025 14:58

vladbologa force-pushed the vb/sensor-offline-cert-refresh branch from 3539f93 to 82d798b Compare February 3, 2025 09:34

vladbologa force-pushed the vb/sensor-offline-cert-refresh branch from 82d798b to d25f9c3 Compare February 5, 2025 09:13

vladbologa requested a review from lvalerom February 13, 2025 09:32

lvalerom reviewed Feb 13, 2025

View reviewed changes

sensor/kubernetes/certrefresh/tls_issuer_common.go Outdated Show resolved Hide resolved

sensor/kubernetes/certrefresh/tls_issuer_common.go Outdated Show resolved Hide resolved

vladbologa added 4 commits February 13, 2025 19:58

ROX-27394: Sensor offline mode support in TLS issuerwq

483d68e

Add more tests

1df83f8

Error check

829f3a7

Make online / started atomic

656a37f

vladbologa force-pushed the vb/sensor-offline-cert-refresh branch from 8331dd0 to 656a37f Compare February 13, 2025 18:58

Add context to the Start method of RetryTicker

5349f6e

vladbologa requested a review from a team as a code owner February 20, 2025 15:03

vladbologa requested a review from lvalerom February 20, 2025 15:05

lvalerom reviewed Feb 24, 2025

View reviewed changes

pkg/concurrency/retry_ticker.go Show resolved Hide resolved

sensor/kubernetes/certrefresh/tls_issuer_common.go Show resolved Hide resolved

sensor/kubernetes/certrefresh/tls_issuer_common.go Show resolved Hide resolved

vladbologa added 2 commits February 24, 2025 16:14

Add mutex for activate/deactivate

ea50bc8

Add tests for RetryTicker ctx cancelation

949d12a

vladbologa requested a review from lvalerom February 24, 2025 15:34

lvalerom approved these changes Feb 25, 2025

View reviewed changes

vladbologa merged commit b3acb5d into master Feb 25, 2025
92 checks passed

vladbologa deleted the vb/sensor-offline-cert-refresh branch February 25, 2025 09:55

vladbologa mentioned this pull request Feb 27, 2025

ROX-28258: Fix segfault when calling deactivate on stopped TLS issuer #14429

Merged

9 tasks

vladbologa mentioned this pull request May 28, 2025

ROX-29452: Fix confusing certrefresh logs #15464

Merged

9 tasks

Conversation

vladbologa commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

User-facing documentation

Testing and quality

Automated testing

How I validated my change

Uh oh!

openshift-ci bot commented Dec 12, 2024

Uh oh!

rhacs-bot commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vikin91 left a comment

Choose a reason for hiding this comment

Uh oh!

vladbologa commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vikin91 commented Feb 12, 2025

Uh oh!

lvalerom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vladbologa commented Feb 20, 2025

Uh oh!

lvalerom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lvalerom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vladbologa commented Dec 12, 2024 •

edited

Loading

rhacs-bot commented Dec 12, 2024 •

edited

Loading

vladbologa commented Dec 19, 2024 •

edited

Loading

codecov bot commented Feb 3, 2025 •

edited

Loading