OCPBUGS-30119: certrotation: update all secret types to `kubernetes.io/tls` preserving existing content by vrutkovs · Pull Request #1681 · openshift/library-go

vrutkovs · 2024-03-01T11:27:59Z

Pre-4.7 clusters had SecretTypeTLS type set, so on 4.15 upgrade these clusters had the secret delete and recreated. This commit will ensure these are being created without cert and key being regenerated.

Fixes two issues wrt handling secrets:

existing secret type is converted to kubernetes.io/tls. If it cannot be done (i.e. tls.crt or tls.key is missing) it would be regenerated with SignerUpdateRequired event
if the type can be converted existing data is preserved, so born-before-4.7 clusters look identical to 4.15 clusters in regards to secret type

…ng existing content Pre-4.7 clusters had `SecretTypeTLS` type set, so on 4.15 upgrade these clusters had the secret delete and recreated. This commit will ensure these are being created without cert and key being regenerated

openshift-ci-robot · 2024-03-01T11:28:06Z

@vrutkovs: This pull request references Jira Issue OCPBUGS-30119, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.16.0) matches configured target version for branch (4.16.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @wangke19

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Pre-4.7 clusters had SecretTypeTLS type set, so on 4.15 upgrade these clusters had the secret delete and recreated. This commit will ensure these are being created without cert and key being regenerated

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

vrutkovs · 2024-03-01T11:46:00Z

/retest

benluddy

I'm still concerned about the recreate path of ApplySecretImproved. If control flow is interrupted between the delete and create (e.g. due to process crash), or the context being used for the client requests is canceled, what happens to the cert data? It seems to me like we would lose it in that case. Even if unlikely, the consequences could be painful. Could we temporarily stage the data in a second secret before deleting the target secret, or something similar?

benluddy · 2024-03-01T14:23:54Z

+		secret.Type = corev1.SecretTypeTLS
+		needsMetadataUpdate = true


Would it be worthwhile to check that the secret we're converting has tls.crt and tls.key before updating the type, since updating the type makes validation stricter (https://github.com/kubernetes/kubernetes/blob/d51e2da869350b2b20a33d060e17bd1d2165e02d/pkg/apis/core/validation/validation.go#L6392-L6398), or will it be enough to see UpdateSecretFailed event spam?

Right, we'd get UpdateSecretFailed event and have it re-created. If the existing secret doesn't have tls.crt and tls.key then its contents is unusable and it needs to be recreated

Recreated by whom? We don't react to events but the secret content, right?

Recreated by ApplySecret, which attempts apply first, but if it fails it does delete+create. If create with kubernetes.io/tls and existing content fails, it would throw an error, delete the content and generate a new cert.

it would throw an error, delete the content and generate a new cert

It would throw the error, but probably would loop

Updated ensureMetadataUpdate to update secret type (removing data if tls.crt/tls.key are missing) in this case, added unit tests

vrutkovs · 2024-03-01T17:02:39Z

the context being used for the client requests is canceled, what happens to the cert data? It seems to me like we would lose it in that case

Right, ApplySecretImproved is not atomic - imo we should first create a copy, then do delete + recreate. I'd however solve this separately, as it may affect a lot more cases - and doesn't need to be merged urgently. Tracking this in https://issues.redhat.com/browse/API-1716

benluddy · 2024-03-01T17:13:37Z

the context being used for the client requests is canceled, what happens to the cert data? It seems to me like we would lose it in that case

Right, ApplySecretImproved is not atomic - imo we should first create a copy, then do delete + recreate. I'd however solve this separately, as it may affect a lot more cases - and doesn't need to be merged urgently

Is it safe to solve separately? IIUC, merging this PR to migrate secret types will present a lot of chances to force an unnecessary rotation on older clusters.

vrutkovs · 2024-03-01T18:44:34Z

We already do "migration" in 4.15 with content replace anyway, so this PR helps in happy path. Corner case - cert sync is interrupted right during the migration - can be safely fixed outside of this PR

benluddy · 2024-03-01T18:48:53Z

"migration"

😂 Oh, of course. Thanks!

benluddy · 2024-03-01T18:53:33Z

/lgtm

openshift-ci-robot · 2024-03-04T12:48:01Z

@vrutkovs: This pull request references Jira Issue OCPBUGS-30119, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.16.0) matches configured target version for branch (4.16.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @wangke19

Details

In response to this:

Pre-4.7 clusters had SecretTypeTLS type set, so on 4.15 upgrade these clusters had the secret delete and recreated. This commit will ensure these are being created without cert and key being regenerated.

Fixes two issues wrt handling secrets:

existing secret type is converted to kubernetes.io/tls. If it cannot be done (i.e. tls.crt or tls.key is missing) it would be regenerated with SignerUpdateRequired event

if the type can be converted existing data is preserved, so born-before-4.7 clusters look identical to 4.15 clusters in regards to secret type

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

stlaz · 2024-03-04T12:47:54Z

+	// apply necessary metadata (possibly via delete+recreate) if secret exists
+	// this is done before content update to prevent unexpected rollouts
+	if ensureMetadataUpdate(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations) && len(signingCertKeyPairSecret.ResourceVersion) > 0 {
+		actualSigningCertKeyPairSecret, _, err := resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, signingCertKeyPairSecret)


nit: the following seems as a bit of a cleaner method to achieve the same

var err error signingCertKeyPairSecret, _, err = resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, signingCertKeyPairSecret)

Following the pattern as in if needed, reason := needNewSigningCertKeyPair

stlaz · 2024-03-04T16:08:52Z

+	// Existing secret not found - no need to update metadata (will be done by needNewSigningCertKeyPair / NeedNewTargetCertKeyPair)
+	if len(secret.ResourceVersion) == 0 {
+		return false
+	}


this probably belongs to the other function, otherwise nothing sets the ownerRef - or does it?

Yeah, good point, this also shows different function flows

stlaz · 2024-03-04T16:23:29Z

/approve

sdodson · 2024-03-04T21:11:03Z

Adding approved label based on #1681 (comment)

benluddy · 2024-03-04T22:30:47Z

+	}
+
+	// convert outdated secret type (created by pre 4.7 installer)
+	if secret.Type != corev1.SecretTypeTLS {


It should behave the same in practice, but since we're correcting for a specific mistake, how about:

Suggested change

if secret.Type != corev1.SecretTypeTLS {

if secret.Type == "SecretTypeTLS" {

And as an aside, can we have an issue to track removing this after N releases?

Confusingly SecretTypeTLS is kubernetes.io/tls and not "SecretTypeTLS". So if secret.Type != corev1.SecretTypeTLS would make sure any non-kubernetes.io/tls secret would converted to kubernetes.io/tls (clearing up data if necessary)

Right, I understand, but my point is that we expect any secret managed by this controller will either have type kubernetes.io/tls or, if it was created pre-4.7, SecretTypeTLS. If we discover that there is some instance of a managed secret with a completely unexpected type, I would rather understand why our expectation was wrong than stomp it.

Its perfectly possible for users to find a way to replace these certificates behind cert-syncer back and replace it with invalid contents. This code will ensure that we attempt to convert cases like pre-4.7 if possible, but also prioritize availability - invalid content would be stomped for cluster to continue functioning. Why the bad secret was stomped can be found out via audit logs (or etcd contents).
This change will also protects us from other similar cases like "old/misconfigured installer has generated certificates which we no longer expect"

benluddy · 2024-03-04T22:35:00Z

+				if len(actual.Annotations) == 0 {
+					t.Errorf("expected certificates to be annotated")
+				}


This looks redundant, any value for annotations that would fail here is also going to fail below.

Yes, but I'd prefer to a clear error message

Fine with me given that nothing out of the control of this test should be touching annotations.

benluddy · 2024-03-04T22:43:41Z

+				if len(actual.Annotations) == 0 {
+					t.Errorf("expected certificates to be annotated")
+				}
+				ownershipValue, found := actual.Annotations[annotations.OpenShiftComponent]


I prefer to avoid sharing values like this between tests and implementation, since it creates the opportunity to accidentally make a breaking change without seeing a test failure.

Feel free to ignore if you disagree or are comfortable with it in this instance because it is declared in openshift/api.

it creates the opportunity to accidentally make a breaking change without seeing a test failure

We explicitly set AdditionalAnnotations in https://github.com/openshift/library-go/pull/1681/files#diff-d7a4e30c6635930d8721bf060fe2803587326298e0eace9bada256f1b7377459R228-R230

That link didn't work for me. In this case I'm specifically referring to a change to the constant OpenShiftComponent, which would be a breaking change but would not cause this test to fail.

Ah, hmm, JiraComponent and OpenshiftComponent are linked in https://github.com/openshift/library-go/blob/master/pkg/operator/certrotation/annotations.go#L27-L30, not sure how to make sure unit tests don't depend on it though

benluddy · 2024-03-04T22:46:12Z

+					t.Error(actions[1])
+				}
+				if !actions[2].Matches("create", "secrets") {
+					t.Error(actions[0])


Suggested change

t.Error(actions[0])

t.Error(actions[2])

benluddy · 2024-03-04T22:47:27Z

+					t.Error(actions[1])
+				}
+				if !actions[2].Matches("create", "secrets") {
+					t.Error(actions[0])


Suggested change

t.Error(actions[0])

t.Error(actions[2])

benluddy · 2024-03-04T22:48:14Z

+					t.Error(actions[1])
+				}
+				if !actions[2].Matches("create", "secrets") {
+					t.Error(actions[0])


There are few more of these:

Suggested change

t.Error(actions[0])

t.Error(actions[2])

benluddy · 2024-03-04T22:52:40Z

-		_, _, err := resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, signingCertKeyPairSecret)
+	// apply necessary metadata (possibly via delete+recreate) if secret exists
+	// this is done before content update to prevent unexpected rollouts
+	if ensureMetadataUpdate(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations) && ensureSecretTLSTypeSet(signingCertKeyPairSecret) {


Why is this && and not ||?

We want to update labels before content changes only when:

secret type won't change (ensureSecretTLSTypeSet returns true)

metadata (annotations, ownership) needs updating (ensureMetadataUpdate returns true)

The function are also mutating signingCertKeyPairSecret, changing it type and clearing data, so the secret would be deleted + created during if needed, reason := needNewSigningCertKeyPair clause. It needs to be done once to avoid updating it twice for metadata and content ("update no annotations" and "update SecretTLSType secrets" unit test cases)

You'll want to make sure your secret always passes both functions, right?

Either operator might short-circuit in a different scenario.

You'll want to make sure your secret always passes both functions, right?

correct, if either doesn't pass possible updates will be applied by ApplySecret in needNewSigningCertKeyPair

so the secret would be deleted + created during if needed, reason := needNewSigningCertKeyPair clause

Thanks, this is the part that I was missing.

This would stuck on invalid secret type with valid metadata, #1687 fixes this case

…ecrets

openshift-ci · 2024-03-05T11:43:28Z

@vrutkovs: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

benluddy · 2024-03-05T14:31:08Z

/lgtm

openshift-ci · 2024-03-05T14:37:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benluddy, stlaz, vrutkovs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/operator/certrotation/OWNERS~~ [stlaz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2024-03-05T14:40:46Z

@vrutkovs: Jira Issue OCPBUGS-30119: All pull requests linked via external trackers have merged:

openshift/library-go#1681

Jira Issue OCPBUGS-30119 has been moved to the MODIFIED state.

Details

In response to this:

Pre-4.7 clusters had SecretTypeTLS type set, so on 4.15 upgrade these clusters had the secret delete and recreated. This commit will ensure these are being created without cert and key being regenerated.

Fixes two issues wrt handling secrets:

existing secret type is converted to kubernetes.io/tls. If it cannot be done (i.e. tls.crt or tls.key is missing) it would be regenerated with SignerUpdateRequired event

if the type can be converted existing data is preserved, so born-before-4.7 clusters look identical to 4.15 clusters in regards to secret type

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

sdodson · 2024-03-05T18:11:38Z

/cherry-pick release-4.14

openshift-cherrypick-robot · 2024-03-05T18:12:35Z

@sdodson: #1681 failed to apply on top of branch "release-4.14":

Applying: certrotation: update all secret types to `kubernetes.io/tls` preserving existing content
Using index info to reconstruct a base tree...
M	pkg/operator/certrotation/signer.go
M	pkg/operator/certrotation/target.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/operator/certrotation/target.go
CONFLICT (content): Merge conflict in pkg/operator/certrotation/target.go
Auto-merging pkg/operator/certrotation/signer.go
CONFLICT (content): Merge conflict in pkg/operator/certrotation/signer.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 certrotation: update all secret types to `kubernetes.io/tls` preserving existing content
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherry-pick release-4.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-merge-robot · 2024-03-06T14:50:01Z

Fix included in accepted release 4.16.0-0.nightly-2024-03-06-073110

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Mar 1, 2024

openshift-ci Bot requested review from hexfusion, stlaz and wangke19 March 1, 2024 11:28

vrutkovs force-pushed the signer-rotate-preserve-type branch from c296833 to 84116c5 Compare March 1, 2024 11:59

benluddy reviewed Mar 1, 2024

View reviewed changes

openshift-ci Bot assigned benluddy Mar 1, 2024

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Mar 1, 2024

stlaz reviewed Mar 4, 2024

View reviewed changes

vrutkovs force-pushed the signer-rotate-preserve-type branch from 84116c5 to 1ca53d1 Compare March 4, 2024 13:07

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2024

vrutkovs force-pushed the signer-rotate-preserve-type branch 3 times, most recently from b20d854 to dc9a1e5 Compare March 4, 2024 16:00

stlaz reviewed Mar 4, 2024

View reviewed changes

vrutkovs force-pushed the signer-rotate-preserve-type branch from dc9a1e5 to ed06b9e Compare March 4, 2024 16:18

sdodson added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2024

benluddy reviewed Mar 4, 2024

View reviewed changes

vrutkovs force-pushed the signer-rotate-preserve-type branch 2 times, most recently from 4bde05d to dfce6ae Compare March 5, 2024 10:56

certrotation: use shared metadata update code for signer and target s…

c1b3d39

…ecrets

vrutkovs force-pushed the signer-rotate-preserve-type branch from dfce6ae to c1b3d39 Compare March 5, 2024 11:34

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2024

openshift-merge-bot Bot merged commit 18ee827 into openshift:master Mar 5, 2024

vrutkovs mentioned this pull request Mar 5, 2024

OCPBUGS-30119: certrotation: Bump library-go to latest master openshift/cluster-kube-apiserver-operator#1651

Merged

This was referenced Mar 6, 2024

certrotation: cover corner case when invalid secret type has necessary metadata #1687

Closed

OCPBUGS-30304: [release-4.15] certrotation: update all secret types to kubernetes.io/tls preserving existing content #1688

Merged

wangke19 mentioned this pull request Dec 15, 2025

OCPBUGS-69758:CNTRLPLANE-2222:Migrate go test cert-rotation-tests.go to OTE openshift/cluster-kube-apiserver-operator#1983

Merged

		secret.Type = corev1.SecretTypeTLS
		needsMetadataUpdate = true

	if secret.Type != corev1.SecretTypeTLS {
	if secret.Type == "SecretTypeTLS" {

Conversation

vrutkovs commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 1, 2024

Uh oh!

vrutkovs commented Mar 1, 2024

Uh oh!

benluddy 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!

vrutkovs commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benluddy commented Mar 1, 2024

Uh oh!

vrutkovs commented Mar 1, 2024

Uh oh!

benluddy commented Mar 1, 2024

Uh oh!

benluddy commented Mar 1, 2024

Uh oh!

openshift-ci-robot commented Mar 4, 2024

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!

stlaz commented Mar 4, 2024

Uh oh!

sdodson commented Mar 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrutkovs Mar 5, 2024 • 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!

vrutkovs commented Mar 1, 2024 •

edited

Loading

vrutkovs commented Mar 1, 2024 •

edited

Loading

vrutkovs Mar 5, 2024 •

edited

Loading