ROX-9148: Create cert refresher for business logic of local scanner certificate refresh by juanrh · Pull Request #474 · stackrox/stackrox

juanrh · 2022-01-28T17:11:31Z

Description

This is a fragment of #400 that has the business logic for the client side of keeping local scanner certificates updated, that is not related to a sensor component.

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
Evaluated and added CHANGELOG entry if required
Determined and documented upgrade steps

No need to add a CHANGELOG entry or update steps.

Testing Performed

New unit tests

go test -count=100 -race -v -p=1 github.com/stackrox/rox/sensor/kubernetes/localscanner -run TestCertRefresher
go test -count=100 -race -v -p=1 github.com/stackrox/rox/pkg/concurrency -run TestRetryTicker

ghost · 2022-01-28T17:29:19Z

Tag for build #216678 is 3.68.x-235-g3f4e8b1ec4.

💻 For deploying this image using the dev scripts, run the following first:

export MAIN_IMAGE_TAG='3.68.x-235-g3f4e8b1ec4'

📦 You can also generate an installation bundle with:

docker run -i --rm stackrox/main:3.68.x-235-g3f4e8b1ec4 central generate interactive > bundle.zip

🕹️ A roxctl binary artifact can be downloaded from CircleCI.

porridge

Looks nice so far, looking forward to a version rebased on all the dependent PRs.

porridge · 2022-01-31T10:51:00Z

sensor/kubernetes/localscanner/cert_refresher.go

Well, this is a lie in case timeToRefresh was > 0, right?

timeToRefresh is the time for the next refresh, we just refreshed the certificates in line 85 with r.refreshCertificates(ctx). I'll rename timeToRefresh to timeToNextRefresh to make this more clear

What I mean is that if we landed here because refreshCertificates returned early, on line 108 then nothing got refreshed this time round, and this message is a lie. I think we should log this around line 123 instead, and even potentially rename refreshCertificates to maybeRefreshCertificates.

you are right, done

porridge · 2022-02-08T08:12:50Z

sensor/kubernetes/localscanner/cert_refresher.go

What I mean is that if we landed here because refreshCertificates returned early, on line 108 then nothing got refreshed this time round, and this message is a lie. I think we should log this around line 123 instead, and even potentially rename refreshCertificates to maybeRefreshCertificates.

porridge · 2022-02-08T08:14:45Z

sensor/kubernetes/localscanner/cert_refresher.go

This is not an error right after deployment, when it's expected, right?

As discussed in #457 the repository should already populate the secret data, so this would be unexpected. Although I wonder if this could also happen even after the repository creates the secrets initialising the secret data with the certificates, due to k8s eventual consistency. If that is the case we'd have the certificates emitted twice in a row, do you think that could happen too?

For the same, I think scanner should be able to wait without crashing until the secrets are created and populated. Is that correct or am I missing something?

I think we should make it such that when a secret is present, it contains expected data. In other words we should never create an empty secret.

I understand you mean that in that case we should not treat ErrEmptyCertificate differently to other errors. The current proposal for #457 creates secrets with data populated with the certificates. So I think we can go two paths here:

Treat errors in getTimeToRefresh as unrecoverable: so getTimeToRefresh just returns 0, err for any error returned by r.getCertsRenewalTime(certificates). This implies an infinite loop of errors when the secret data is corrupted. The current proposal for ROX-9128: Create certSecretsRepo type to store certificates in k8s secrets #457 only creates a secret when it doesn't exist, so restarting Sensor wouldn't fix that

Treat errors in getTimeToRefresh as recoverable, and recover by returning 0, nil for any error returned by r.getCertsRenewalTime(certificates). This can also leads to an infinite loop of requesting certificates, if the certificate renewal process always fails.

I tend to prefer option 2, what do you think?

the repository should already populate the secret data, so this would be unexpected

I understand you mean that in that case we should not treat ErrEmptyCertificate differently to other errors.

Yes, if we never create an empty secret then we should not be having a special error instance for an empty secret. Just treat it as invalid cert ripe for a refresh.

I first modified getTimeToRefresh to never fail and always recover using 0 as timeToNextRefresh.
But then I realised that if we get an error in the second call to r.getTimeToRefresh in the body of ensureCertificatesAreFresh, for the new certificates returned by r.requestCertificates(ctx), then we should not return 0, nil to the ticker that is calling RefreshCertificates, because that would imply an immediate retry, thus hammering central with certificate requests just because the refresher doesn't understand the certificates. So I have added a recoverFromErr parameter to getTimeToRefresh to optionally ignore errors, so in ensureCertificatesAreFresh for the the first call we ignore errors and refresh the certificate immediately if we cannot parse the certificates stored in the secrets, but for the second call where we parse the new certificates we don't recover from an error so the backoff mechanism in the ticker can work.

What do you think?

porridge · 2022-02-08T08:15:32Z

sensor/kubernetes/localscanner/cert_refresher.go

Since there are no other errs in this function:

Suggested change

renewalTime, renewalTimeErr := r.getCertsRenewalTime(certificates)

renewalTime, err := r.getCertsRenewalTime(certificates)

porridge · 2022-02-08T08:20:39Z

sensor/kubernetes/localscanner/cert_refresher.go

I'm not sure this interface is all that useful...

this would be useful for testing the sensor component that launches the refresher, as it would allow to mock the refresher easily

Right, but an interface should be defined in the user's package, not in the implementer's one.
https://blog.chewxy.com/2018/03/18/golang-interfaces/

Here it's the same package, but arguably the same principles should apply when we split things across files.

What I'm understanding here is that the suggestion is moving the certRefresher interface to the sensor component that uses it, e.g. to file tls_issuer.go in https://github.com/stackrox/stackrox/pull/565/files#diff-81bd3808339e34da64a1fa5e66c27f0ec9d75a8a51956b1773c4e0c8b6295c08. I'm thus removing the interface from this PR

porridge · 2022-02-08T08:23:01Z

sensor/kubernetes/localscanner/cert_refresher.go

Suggested change

ticker := concurrency.NewRetryTicker(refresher.RefreshCertificates, timeout, backoff)

return ticker

return concurrency.NewRetryTicker(refresher.RefreshCertificates, timeout, backoff)

porridge · 2022-02-08T08:25:36Z

sensor/kubernetes/localscanner/cert_refresher.go

Suggested change

// PutServiceCertificates TODO replace by serviceCertificatesRepo from ROX-9128

type PutServiceCertificates interface {

// certificatesRepo TODO replace by serviceCertificatesRepo from ROX-9128

type certificatesRepo interface {

porridge · 2022-02-09T07:35:53Z

sensor/kubernetes/localscanner/cert_refresher.go

I have to admit that because the last line of parameter list is indented the same as the first line of code, I find it hard to tell them apart visually. WDYT about inserting a blank line as the first line of function body whenever there is a line break inside the parameter list?

done, I agree that is more readable

porridge · 2022-02-09T07:43:49Z

sensor/kubernetes/localscanner/cert_refresher.go

This is only passed as the function to the ticker and not accessed anywhere else (apart from unit tests), right?
In that case I guess it does not need to be exported? 🤔

renamed to lower case

porridge · 2022-02-09T07:48:48Z

sensor/kubernetes/localscanner/cert_refresher.go

If the entire API is just creating a ticker and delegating Stop and Start to it, then perhaps the refresher should not be wrapping it in the first place? Perhaps we can simplify the whole thing by having newCertRefresher return an appropriately configured ticker instead? WDYT?

that makes sense, and it simplifies the code significantly, done

porridge · 2022-02-09T07:54:10Z

sensor/kubernetes/localscanner/cert_refresher.go

the repository should already populate the secret data, so this would be unexpected

I understand you mean that in that case we should not treat ErrEmptyCertificate differently to other errors.

Yes, if we never create an empty secret then we should not be having a special error instance for an empty secret. Just treat it as invalid cert ripe for a refresh.

porridge

LGTM, apart from a few nitpicks.

porridge · 2022-02-11T08:52:01Z

sensor/kubernetes/localscanner/cert_refresher.go

Nit: corrupted might be too strong a word for the case where CA certs differ. Perhaps just inconsistent state?

changed this to

log.Errorf("local scanner certificates are in an inconsistent state, will refresh certificates immediately: %s", getCertsErr)

porridge · 2022-02-11T08:52:25Z

sensor/kubernetes/localscanner/cert_refresher.go

Suggested change

timeout time.Duration, backoff wait.Backoff) concurrency.RetryTicker {

return concurrency.NewRetryTicker(func(ctx context.Context) (timeToNextTick time.Duration, err error) {

timeout time.Duration, backoff wait.Backoff) concurrency.RetryTicker {

return concurrency.NewRetryTicker(func(ctx context.Context) (timeToNextTick time.Duration, err error) {

porridge · 2022-02-11T08:57:58Z

sensor/kubernetes/localscanner/cert_refresher.go

Please use a consistent wrapping style for the same function prototypes (i.e. here vs refreshCertificates). For 4 parameters I'd prefer the style used here, but up to you as long as it's consistent.

now using longer lines for refreshCertificates too

porridge · 2022-02-11T09:03:27Z

sensor/kubernetes/localscanner/cert_refresher.go

Suggested change

// recoverFromErr to so the ticker knows this is an error, and it retries with backoff.

// recoverFromEr=false so the ticker knows this is an error, and it retries with backoff.

porridge · 2022-02-11T09:04:46Z

sensor/kubernetes/localscanner/cert_refresher.go

TBH my preference would be to inline this function. The extra recoverFromErr parameter seems to introduce more complexity than is solved by having this standalone function. At least I had a hard time figuring out what is going on :-)

You are right, I like how your comments keep making the code smaller!

SimonBaeumer · 2022-02-12T11:08:07Z

sensor/kubernetes/localscanner/cert_refresher.go

Suggested change

// ErrDifferentCAForDifferentServiceTypes TODO replace by ROX-9128

// ErrDifferentCAForDifferentServiceTypes TODO(ROX-9128): <description why>

The plan is not merging this even with a TODO, because that would cause a conflict with #457 because both would define the same package variable and break the build. Merging this PR is blocked by merging #457

references to ROX-9128 removed after rebase

SimonBaeumer · 2022-02-12T11:08:44Z

sensor/kubernetes/localscanner/cert_refresher.go

Suggested change

ErrDifferentCAForDifferentServiceTypes = errors.New("found different CA PEM in secret Data for different service types")

ErrDifferentCAForDifferentServiceTypes = errors.New("found different CA PEM in secret Data field for different service types")

SimonBaeumer · 2022-02-12T11:09:09Z

sensor/kubernetes/localscanner/cert_refresher.go

Suggested change

// ErrMissingSecretData TODO replace by ROX-9128

// ErrMissingSecretData TODO(ROX-9128): <description>

references to ROX-9128 removed after rebase

SimonBaeumer · 2022-02-12T11:11:21Z

sensor/kubernetes/localscanner/cert_refresher.go

This is a mock right? Can this be removed?
If you add temporary code try to mark it as TODO(do-not-merge): ... to prevent accidental merges.

That is not a mock, per another review comment I moved the interface definition to the place where they are used, see #474 (comment) for context

Does it mean we add the interfaces where they are needed?
Tbh I've never seen this before it and seems to not make sense anymore if we have another package using the same interface as a dependency (in structs or function signatures).
cc: @porridge

I.e. the Reader is also defined in io.go where the implementation lives

For this particular case, another option is putting all interfaces in a new file sensor/kubernetes/localscanner/types.go, what do you think?

@porridge Do you have any production examples? Preferably in a well-known project?
Also took a look at the uber style guide where it is not mentioned.

@SimonBaeumer here's the most authoritative source I could find: https://github.com/golang/go/wiki/CodeReviewComments#interfaces

This is also alluded to by people like Rob Pike in places like https://groups.google.com/g/golang-nuts/c/mg-8_jMyasY/m/lo-kDuEd540J which explains the philosophical background of why this rule was created in the first place.
I'm also reasonably sure I've seen a talk by one of the Go creators that mentioned this, but I cannot find it :-(

Some more places where it's mentioned by other people:

https://blog.logrocket.com/exploring-structs-interfaces-go/#other-interface-types-considerations
https://commandercoriander.net/blog/2018/03/30/go-interfaces/

Of course there are cases where this is hard to apply, but I don't think this is one of them.

Not the talk I had in mind, but fun to watch: https://www.youtube.com/watch?v=PAAkCSZUG1c&t=318s

Very interesting discussion. Some golang concepts like interfaces or methods are sneaky to me because they sound similar to Java or C++ concepts, but are actually quite different, so this material is really useful

Interesting, thanks for sharing @porridge. I was not aware of this, need to dig deeper later 👍

SimonBaeumer · 2022-02-12T11:13:03Z

sensor/kubernetes/localscanner/cert_refresher.go

Suggested change

// refreshCertificates determines refreshes the certificate secrets if needed, and returns the time

// refreshCertificates refreshes the certificate secrets if needed, and returns the time

SimonBaeumer · 2022-02-12T11:15:03Z

sensor/kubernetes/localscanner/cert_refresher.go

I think this needs to be replaced with ensureSecrets after the repo secrets PR is merged

sensor/kubernetes/localscanner/cert_refresher.go

SimonBaeumer · 2022-02-12T11:38:30Z

I did not review tests as waiting for updating the interface for ensure secrets.

… refresh

…reFresh

it should be declared instead in the client code that is the sensor component that uses it

SimonBaeumer

Already looks very good.
Small remarks on the interface discussion and mocking (using generated mocks instead).
Good job here!

SimonBaeumer · 2022-02-15T16:01:36Z

pkg/concurrency/retry_ticker_test.go

+	time.Sleep(capTime)
+	assert.Nil(t, ticker.getTickTimer())


Do we have something like an assert.Until function?
Or retry this until either ticker.getTickerTimer==null or timeout exceeded?
I looks like a general pattern here. Would be great to have something like that here. (Must not be part of this PR, instead adding it as a separate ticket.)

I haven't seen that in testify, but we can implement something like that with PollWithTimeout

func assertTickerEventuallyStops(t *testing.T, ticker *retryTickerImpl) { ok := PollWithTimeout(func() bool { return ticker.getTickTimer() == nil }, 10 * time.Millisecond, capTime) assert.True(t, ok, "ticker should eventually stop") }

I have checked that this catches the error for TestRetryTickerStop and TestRetryTickerStopsOnNonRecoverableErrors when changing scheduleTick to work wrong, so I'm using this instead.

Nice catch, this should make our tests slightly faster

SimonBaeumer · 2022-02-15T16:15:38Z

sensor/kubernetes/localscanner/cert_refresher_test.go

+}
+
+func (s *certRefresherSuite) TestRefreshCertificatesRequestCertificatesFailure() {
+	certificates := (*storage.TypedServiceCertificateSet)(nil)


Suggested change

certificates := (*storage.TypedServiceCertificateSet)(nil)

var certificates *storage.TypedServiceCertificateSet

SimonBaeumer · 2022-02-15T16:16:07Z

sensor/kubernetes/localscanner/cert_refresher_test.go

+}
+
+func (s *certRefresherSuite) TestRefreshCertificatesRequestCertificatesResponseFailure() {
+	certificates := (*storage.TypedServiceCertificateSet)(nil)


Suggested change

certificates := (*storage.TypedServiceCertificateSet)(nil)

var certificates *storage.TypedServiceCertificateSet

SimonBaeumer · 2022-02-15T16:41:07Z

sensor/kubernetes/localscanner/cert_refresher.go

Does it mean we add the interfaces where they are needed?
Tbh I've never seen this before it and seems to not make sense anymore if we have another package using the same interface as a dependency (in structs or function signatures).
cc: @porridge

SimonBaeumer · 2022-02-15T16:42:45Z

sensor/kubernetes/localscanner/cert_refresher.go

I.e. the Reader is also defined in io.go where the implementation lives

SimonBaeumer · 2022-02-15T16:43:45Z

sensor/kubernetes/localscanner/cert_refresher.go

+		return 0, nil
+	}
+	if k8sErrors.IsNotFound(getCertsErr) {
+		log.Warnf("local scanner certificates are missing, "+


Suggested change

log.Warnf("local scanner certificates are missing, "+

log.Warnf("local scanner certificates not found, " +

sensor/kubernetes/localscanner/cert_refresher_test.go

SimonBaeumer

Besides the interface location this looks good. Imho if we don't have evidence to put in the cert_refresher let's put it back to implementation side.

…ertificate refresh (#474)

juanrh requested review from SimonBaeumer and porridge January 28, 2022 17:11

github-actions bot added the area/sensor label Jan 28, 2022

juanrh marked this pull request as draft January 28, 2022 17:13

juanrh mentioned this pull request Jan 28, 2022

ROX-9014: Draft TLSIssuer as a SensorComponent and CertificateIssuer #400

Closed

4 tasks

porridge reviewed Jan 31, 2022

View reviewed changes

juanrh force-pushed the juanrh/ROX-9129 branch from 6c8635f to 0943901 Compare February 2, 2022 08:11

This was referenced Feb 2, 2022

ROX-9128: Create certSecretsRepo type to store certificates in k8s secrets #457

Merged

ROX-9015: new SensorComponent to keep local scanner secrets up to date #565

Merged

juanrh force-pushed the juanrh/ROX-9148 branch from 5d5b3f4 to 16707ef Compare February 7, 2022 18:51

juanrh changed the base branch from juanrh/ROX-9129 to master February 7, 2022 18:51

juanrh requested a review from porridge February 7, 2022 18:52

juanrh marked this pull request as ready for review February 7, 2022 18:52

porridge requested changes Feb 8, 2022

View reviewed changes

juanrh force-pushed the juanrh/ROX-9148 branch from cb3c49e to b651192 Compare February 8, 2022 13:05

juanrh requested a review from porridge February 8, 2022 16:49

porridge reviewed Feb 9, 2022

View reviewed changes

juanrh requested a review from porridge February 10, 2022 15:39

porridge approved these changes Feb 11, 2022

View reviewed changes

SimonBaeumer suggested changes Feb 12, 2022

View reviewed changes

Juan Rodriguez Hortala added 8 commits February 14, 2022 15:04

Create cert refresher for business logic of local scanner certificate…

bc601ac

… refresh

fix style

c99945f

improve logging and rename refreshCertificates to ensureCertificatesA…

ea91e38

…reFresh

fix typo and code readability improvements

7eae6fd

remove definition of certRefresher interface

86db20b

it should be declared instead in the client code that is the sensor component that uses it

fix style

d12ad14

adapt to ROX-9129 push to master

380e30f

define certificates repo in client code

5e1b2f2

Juan Rodriguez Hortala added 17 commits February 14, 2022 15:04

getTimeToRefresh doesn't fail and always recovers using 0

3450316

return errors found on getTimeToRefresh for the new certificate

7b9dd39

add new line to make code more readable

af8e914

do not export refreshCertificates

05e77ab

remove new type and return a retry ticker instead

caf2699

only log a successful refresh if successfully parse the new certs

69441db

recover from corrupted certificates

56a10a7

make repo interface private

4cd78f8

rename log message when certificates cannot be parsed

ba44699

improve indenting

3c04f2d

remove getTimeToRefreshFromCertificates to simplify code

fe4703f

complete tests

c1cd100

adjustments after rebase

aaede95

assert more specific errors

8e95f72

stop ticker on non recoverable errors

6401143

create missing secrets, and give up on wrong owners

44c67b5

check repository interface is implemented

f8c139b

juanrh force-pushed the juanrh/ROX-9148 branch from c1ce53b to f8c139b Compare February 14, 2022 17:52

Juan Rodriguez Hortala added 2 commits February 15, 2022 13:09

complete tests

1e1f1f1

fix typo in comment

1cb403b

juanrh requested a review from SimonBaeumer February 15, 2022 12:14

SimonBaeumer suggested changes Feb 15, 2022

View reviewed changes

Juan Rodriguez Hortala added 3 commits February 16, 2022 09:55

poll instead of sleep to wait for ticker stop

b8cfd0a

fix style

db609ad

minor fixes

3f4e8b1

SimonBaeumer approved these changes Feb 16, 2022

View reviewed changes

juanrh merged commit 5f295b0 into master Feb 17, 2022

juanrh deleted the juanrh/ROX-9148 branch February 17, 2022 08:34

RTann pushed a commit that referenced this pull request Apr 6, 2022

ROX-9148: Create cert refresher for business logic of local scanner c…

3462cdb

…ertificate refresh (#474)

	renewalTime, renewalTimeErr := r.getCertsRenewalTime(certificates)
	renewalTime, err := r.getCertsRenewalTime(certificates)

	ticker := concurrency.NewRetryTicker(refresher.RefreshCertificates, timeout, backoff)
	return ticker
	return concurrency.NewRetryTicker(refresher.RefreshCertificates, timeout, backoff)

-// PutServiceCertificates TODO replace by serviceCertificatesRepo from ROX-9128
-type PutServiceCertificates interface {
+// certificatesRepo TODO replace by serviceCertificatesRepo from ROX-9128
+type certificatesRepo interface {

-	timeout time.Duration, backoff wait.Backoff) concurrency.RetryTicker {
-	return concurrency.NewRetryTicker(func(ctx context.Context) (timeToNextTick time.Duration, err error) {
+	timeout time.Duration, backoff wait.Backoff) concurrency.RetryTicker {
+	return concurrency.NewRetryTicker(func(ctx context.Context) (timeToNextTick time.Duration, err error) {

	// recoverFromErr to so the ticker knows this is an error, and it retries with backoff.
	// recoverFromEr=false so the ticker knows this is an error, and it retries with backoff.

	// ErrDifferentCAForDifferentServiceTypes TODO replace by ROX-9128
	// ErrDifferentCAForDifferentServiceTypes TODO(ROX-9128): <description why>

	ErrDifferentCAForDifferentServiceTypes = errors.New("found different CA PEM in secret Data for different service types")
	ErrDifferentCAForDifferentServiceTypes = errors.New("found different CA PEM in secret Data field for different service types")

	// ErrMissingSecretData TODO replace by ROX-9128
	// ErrMissingSecretData TODO(ROX-9128): <description>

	// refreshCertificates determines refreshes the certificate secrets if needed, and returns the time
	// refreshCertificates refreshes the certificate secrets if needed, and returns the time

	certificates := (*storage.TypedServiceCertificateSet)(nil)
	var certificates *storage.TypedServiceCertificateSet

	log.Warnf("local scanner certificates are missing, "+
	log.Warnf("local scanner certificates not found, " +

Conversation

juanrh commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing Performed

Uh oh!

ghost commented Jan 28, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

porridge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juanrh Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

porridge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juanrh commented Jan 28, 2022 •

edited

Loading

ghost commented Jan 28, 2022 •

edited by ghost

Loading

juanrh Feb 10, 2022 •

edited

Loading

juanrh Feb 11, 2022 •

edited

Loading