OCPBUGS-38335: certrotation: avoid using applySecret to catch racy cert updates #1772

vrutkovs · 2024-08-06T12:46:03Z

applySecret/applyConfigMap does Get initially, so instead of using the version we got from informer it refreshes it and may overwrite if other changes were done in parallel.

Instead of using applySecret/applyConfigMap we should be invoking Create/Update here directly. If the informer has outdated version its update would be rejected by etcd and sync function would be restarted

openshift-ci-robot · 2024-08-06T12:46:06Z

@vrutkovs: This pull request references API-1802 which is a valid jira issue.

In response to this:

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.

openshift-ci-robot · 2024-08-06T12:54:00Z

@vrutkovs: This pull request references API-1802 which is a valid jira issue.

In response to this:

applySecret/applyConfigMap does Get initially, so instead of using the version we got from informer it refreshes it and may overwrite if other changes were done in parallel.

Instead of using applySecret/applyConfigMap we should be invoking Create/Update here directly. If the informer has outdated version its update would be rejected by etcd and sync function would be restarted

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.

bertinatto · 2024-08-08T18:24:02Z

pkg/operator/certrotation/cabundle.go

+		if len(caBundleConfigMap.ResourceVersion) == 0 {
+			actualCABundleConfigMap, err = c.Client.ConfigMaps(c.Namespace).Create(ctx, caBundleConfigMap, metav1.CreateOptions{})
+		} else {
+			actualCABundleConfigMap, err = c.Client.ConfigMaps(c.Namespace).Update(ctx, caBundleConfigMap.DeepCopy(), metav1.UpdateOptions{})


This won't record an event anymore, like ApplyConfigMap() does. Would that be something useful for users?

caBundleConfigMap is already a copy, and it may have been modified already. Do we really need another DeepCopy() here?

Reporting an event on successful update is a good idea.

We don't need to call DeepCopy() here, please remove it.

Done, removed

bertinatto · 2024-08-08T18:34:40Z

pkg/operator/certrotation/cabundle.go

 		if err != nil {
 			return nil, err
 		}
-		if updated {
+		if equality.Semantic.DeepEqual(actualCABundleConfigMap.Data, caBundleConfigMap.Data) {


Technically other parts of caBundleConfigMap may have been updated (not only .Data). Should we log the message below in those cases?

Why do we need this additional check anyway ? (line 89)
We already know that the update was required. So, if the call succeeded then we can simply report success.

Good idea, we already have modified, so lets use it to emit the event

bertinatto · 2024-08-08T18:48:30Z

pkg/operator/certrotation/signer.go

-	// and it works in tandem with a particular carry patch applied to the openshift kube-apiserver.
-	// we will remove this when we migrate all of the affected secret
-	// objects to their intended type: https://issues.redhat.com/browse/API-1800
-	UseSecretUpdateOnly bool


It seems like this change here won't help with the race (?).

Since this was added here and you don't need it anymore, could we revert that PR instead? This would also get rid of the resourceapply changes, which we aren't doing here.

+1, we should cleanup and remove unused code like ApplySecretDoNotUse.

p0lyn0mial · 2024-08-09T06:18:13Z

pkg/operator/certrotation/cabundle.go

-		actualCABundleConfigMap, updated, err := resourceapply.ApplyConfigMap(ctx, c.Client, c.EventRecorder, caBundleConfigMap)
+		var actualCABundleConfigMap *corev1.ConfigMap
+		if len(caBundleConfigMap.ResourceVersion) == 0 {
+			actualCABundleConfigMap, err = c.Client.ConfigMaps(c.Namespace).Create(ctx, caBundleConfigMap, metav1.CreateOptions{})


Given that we would like to report successful creation as well why not change the code to be more explicit.
For example:

we would introduce a new var creationRequired bool or var creationNeeded bool variable.

we would set the new variable at line 60

we would rename modified to updateRequired or updateNeeded

we would introduce a branch before line 89, for example:

if creationRequired { create report assign to `caBundleConfigMap` } else if updateRequired { update report assign to `caBundleConfigMap` }

Alternatively we could move the creation to line 53.

The creation is a rare operation and immediately after creation the sync loop will be triggered and we can issue an update.

Good idea, now all functions should have exists and modified

we don't want to report https://github.com/openshift/library-go/blob/master/pkg/operator/resource/resourceapply/event_helpers.go#L25 anymore ?

bertinatto · 2024-08-09T12:14:42Z

pkg/operator/certrotation/signer.go

+		if !exists {
+			actualSigningCertKeyPairSecret, err = c.Client.Secrets(c.Namespace).Create(ctx, signingCertKeyPairSecret, metav1.CreateOptions{})
+		} else {
+			actualSigningCertKeyPairSecret, err = c.Client.Secrets(c.Namespace).Update(ctx, signingCertKeyPairSecret.DeepCopy(), metav1.UpdateOptions{})


signingCertKeyPairSecret is either new or a deep copy of originalSigningCertKeyPairSecret, so I think you don't need another deep copy here?

good catch, removed

openshift-ci-robot · 2024-08-12T08:43:58Z

@vrutkovs: This pull request references Jira Issue OCPBUGS-38335, 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.18.0) matches configured target version for branch (4.18.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.

In response to this:

applySecret/applyConfigMap does Get initially, so instead of using the version we got from informer it refreshes it and may overwrite if other changes were done in parallel.

Instead of using applySecret/applyConfigMap we should be invoking Create/Update here directly. If the informer has outdated version its update would be rejected by etcd and sync function would be restarted

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.

p0lyn0mial · 2024-08-12T08:55:50Z

pkg/operator/resource/resourceapply/core.go

-// Deprecated: DO NOT USE, it is intended as a short term hack for a very specific use case,
-// and it works in tandem with a particular carry patch applied to the openshift kube-apiserver.
-// Use ApplySecret instead.
-func ApplySecretDoNotUse(ctx context.Context, client coreclientv1.SecretsGetter, recorder events.Recorder, required *corev1.Secret) (*corev1.Secret, bool, error) {


The original PR introduced more changes. Not all were remove. Please open a new PR that reverts #1705.

p0lyn0mial · 2024-08-12T09:06:04Z

pkg/operator/certrotation/cabundle.go

+		if !exists {
+			actualCABundleConfigMap, err = c.Client.ConfigMaps(c.Namespace).Create(ctx, caBundleConfigMap, metav1.CreateOptions{})
+		} else {
+			actualCABundleConfigMap, err = c.Client.ConfigMaps(c.Namespace).Update(ctx, caBundleConfigMap, metav1.UpdateOptions{})


why don't want to report https://github.com/openshift/library-go/blob/master/pkg/operator/resource/resourceapply/event_helpers.go#L34 anymore ?

Good point, we get an event that its about to be updated, but not the update result. Added this to the latest commit

bertinatto · 2024-08-12T19:43:39Z

pkg/operator/certrotation/target.go

-		actualTargetCertKeyPairSecret, _, err := resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, targetCertKeyPairSecret)
+		var actualTargetCertKeyPairSecret *corev1.Secret
+		var action string
+		if len(targetCertKeyPairSecret.ResourceVersion) == 0 {


if !exists instead?

bertinatto · 2024-08-12T19:57:19Z

pkg/operator/certrotation/event.go

+	"github.com/openshift/library-go/pkg/operator/events"
+)
+
+func emitEvent(recorder events.Recorder, objType string, action string, obj string, err error) {


I think this function could just receive a obj runtime.Object and you could figure out objType and obj using resourcehelper.GuessObjectGroupVersionKind(obj); gvk.Kind and resourcehelper.FormatResourceForCLIWithNamespace(obj) respectively.

So you could do:

emitEvent(c.EventRecorder, actualCABundleConfigMap, "Create", err)

Good idea. Also using FormatResourceForCLI instead of namespaced version as events are namespaced anyway

bertinatto · 2024-08-12T20:09:14Z

pkg/operator/certrotation/signer.go

+			actualSigningCertKeyPairSecret, err = c.Client.Secrets(c.Namespace).Update(ctx, signingCertKeyPairSecret, metav1.UpdateOptions{})
+			action = "Update"
+		}
+		emitEvent(c.EventRecorder, "Secret", action, resourcehelper.FormatResourceForCLIWithNamespace(actualSigningCertKeyPairSecret), err)


If you do https://github.com/openshift/library-go/pull/1772/files#r1714300480, this would get shorter:

var actualSigningCertKeyPairSecret *corev1.Secret if !exists { actualSigningCertKeyPairSecret, err = c.Client.Secrets(c.Namespace).Create(ctx, signingCertKeyPairSecret, metav1.CreateOptions{}) emitEvent(c.EventRecorder, "Create", actualSigningCertKeyPairSecret, err) } else { actualSigningCertKeyPairSecret, err = c.Client.Secrets(c.Namespace).Update(ctx, signingCertKeyPairSecret, metav1.UpdateOptions{}) emitEvent(c.EventRecorder, "Update", actualSigningCertKeyPairSecret, err) }

to minimize copying important params I introduced "action" which is the only variable there

bertinatto

While I'm not familiar with these controllers, I did my best to review this, and to the best of my knowledge, these changes appear to be correct.

/lgtm

p0lyn0mial · 2024-08-14T06:07:22Z

pkg/operator/certrotation/event.go

+	"k8s.io/apimachinery/pkg/runtime"
+)
+
+func emitEvent(recorder events.Recorder, action string, obj runtime.Object, err error) {


Instead of this new function I would prefer to reuse reportCreateEvent and reportUpdateEvent.

I think we could open a new PR in which we would:

crate a new helpers pkg in library-go/pkg/operator/resource/helpers

we would move operator/resource/resourceapply/event_helpers.go to the new pkg (with tests)

we would export all report methods.

Then we could remove this file and use the new exported methods directly. WDYT?

p0lyn0mial · 2024-08-14T06:25:37Z

pkg/operator/certrotation/cabundle.go

@@ -59,6 +59,7 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC
 			c.AdditionalAnnotations,
 		)}
 		modified = true
+		exists = false
 	}

 	needsOwnerUpdate := false


and we could modify line 70 to:

updateRequired = needsOwnerUpdate || needsMetadataUpdate

p0lyn0mial · 2024-08-14T07:01:20Z

pkg/operator/certrotation/signer.go

@@ -95,15 +96,25 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*

 		LabelAsManagedSecret(signingCertKeyPairSecret, CertificateTypeSigner)

-		modified = true
+		updateRequired = true
 		signerUpdated = true


do we really need this var ?

I think it could be replaced by creationRequired || updateRequired, right ?

Yes. updateRequired means "any part of the secret has changed", but signerUpdated is specific to certificate part (we need to pass it on to trigger CA bundle / target cert updates)

sure, I think that the following code:

// at this point, the secret has the correct signer, so we should read that signer to be able to sign signingCertKeyPair, err := crypto.GetCAFromBytes(signingCertKeyPairSecret.Data["tls.crt"], signingCertKeyPairSecret.Data["tls.key"]) if err != nil { return nil, signerUpdated, err } return signingCertKeyPair, signerUpdated, nil

could be replaced be:

// at this point, the secret has the correct signer, so we should read that signer to be able to sign signingCertKeyPair, err := crypto.GetCAFromBytes(signingCertKeyPairSecret.Data["tls.crt"], signingCertKeyPairSecret.Data["tls.key"]) if err != nil { return nil, creationRequired || updateRequired, err } return signingCertKeyPair, creationRequired || updateRequired, nil

and we could remove signerUpdated, am I right ?

No, updateRequired || creationRequired is not equivalent to signerUpdated. We set updateRequired = true when annotations change too.

This PR is already growing too large - can we optimize this in a followup?

ok, do you also want to move the refactor commit to a new PR ?

Lets leave this version as is and experiment if signerUpdated can be removed in a separate PR.

I think we want more specific messages for signer/cabundle/target too, i.e. Updated contents and Updated metadata probably

p0lyn0mial · 2024-08-14T07:03:09Z

pkg/operator/certrotation/target.go

@@ -108,8 +110,8 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont
 			),
 			Type: corev1.SecretTypeTLS,
 		}
-		modified = true
-		secretDoesntExist = true
+		updateRequired = true


is this needed here ?

Oh, right, no longer needed

p0lyn0mial · 2024-08-14T07:54:04Z

@vrutkovs could you also open a proof PR in kas-o so that we get a CI signal?

vrutkovs · 2024-08-14T08:16:18Z

Opened openshift/cluster-kube-apiserver-operator#1721

p0lyn0mial · 2024-08-14T08:18:09Z

Opened openshift/cluster-kube-apiserver-operator#1721

the pr was created one week ago and e2e tests haven't been executed. Could you address #1772 (comment) and then pull this pr into 1721 and kick off the tests?

p0lyn0mial · 2024-08-14T08:44:45Z

/lgtm

let's wait for openshift/cluster-kube-apiserver-operator#1721 before merging.

vrutkovs · 2024-08-14T13:01:41Z

/test unit

p0lyn0mial · 2024-08-14T13:02:30Z

pkg/operator/certrotation/target.go


-	if reason := c.CertCreator.NeedNewTargetCertKeyPair(targetCertKeyPairSecret, signingCertKeyPair, caBundleCerts, c.Refresh, c.RefreshOnlyWhenExpired, secretDoesntExist); len(reason) > 0 {
+	if reason := c.CertCreator.NeedNewTargetCertKeyPair(targetCertKeyPairSecret, signingCertKeyPair, caBundleCerts, c.Refresh, c.RefreshOnlyWhenExpired, creationRequired); len(reason) > 0 {


does c.CertCreator.NeedNewTargetCertKeyPair persist any resource in etcd (does it use Apply* method?)

No, we don't even pass client there. It decides on the contents of the secret whether it needs to be regenerated

ok, thanks.

p0lyn0mial · 2024-08-14T13:03:57Z

pkg/operator/certrotation/signer.go

@@ -74,18 +75,18 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*
 			),
 			Type: corev1.SecretTypeTLS,
 		}
-		signerExists = false
+		creationRequired = true
 	}

 	// apply necessary metadata (possibly via delete+recreate) if secret exists


this comment is outdated.

p0lyn0mial · 2024-08-14T13:24:23Z

/lgtm

let's wait for openshift/cluster-kube-apiserver-operator#1721 before merging.

openshift-ci · 2024-08-14T13:40:48Z

@vrutkovs: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

soltysh

/approve

openshift-ci · 2024-08-16T09:24:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, p0lyn0mial, soltysh, vrutkovs

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/operator/OWNERS~~ [soltysh]

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-08-16T09:27:57Z

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

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

In response to this:

applySecret/applyConfigMap does Get initially, so instead of using the version we got from informer it refreshes it and may overwrite if other changes were done in parallel.

Instead of using applySecret/applyConfigMap we should be invoking Create/Update here directly. If the informer has outdated version its update would be rejected by etcd and sync function would be restarted

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.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 6, 2024

openshift-ci bot requested review from deads2k and hexfusion August 6, 2024 12:46

vrutkovs force-pushed the cert-rotation-skip-get-in-apply branch from 8592119 to d3df677 Compare August 6, 2024 13:29

vrutkovs mentioned this pull request Aug 7, 2024

OCPBUGS-38335: Bump library-go openshift/cluster-kube-apiserver-operator#1721

Merged

bertinatto reviewed Aug 8, 2024

View reviewed changes

p0lyn0mial reviewed Aug 9, 2024

View reviewed changes

vrutkovs force-pushed the cert-rotation-skip-get-in-apply branch 2 times, most recently from 287516d to 73337c3 Compare August 9, 2024 09:54

bertinatto reviewed Aug 9, 2024

View reviewed changes

vrutkovs force-pushed the cert-rotation-skip-get-in-apply branch from 73337c3 to 2a150ed Compare August 9, 2024 13:03

vrutkovs changed the title ~~API-1802: certrotation: avoid using applySecret to catch racy cert updates~~ OCPBUGS-38335: certrotation: avoid using applySecret to catch racy cert updates Aug 12, 2024

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Aug 12, 2024

openshift-ci bot requested a review from wangke19 August 12, 2024 08:43

p0lyn0mial reviewed Aug 12, 2024

View reviewed changes

vrutkovs mentioned this pull request Aug 12, 2024

OCPBUGS-38335: Revert "use update only secret-apply for cert rotation logic" #1773

Merged

vrutkovs force-pushed the cert-rotation-skip-get-in-apply branch 3 times, most recently from f468f49 to c702d4d Compare August 12, 2024 11:48

bertinatto reviewed Aug 12, 2024

View reviewed changes

vrutkovs force-pushed the cert-rotation-skip-get-in-apply branch from c702d4d to ce677d9 Compare August 13, 2024 06:27

bertinatto reviewed Aug 13, 2024

View reviewed changes

openshift-ci bot assigned bertinatto Aug 13, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2024

p0lyn0mial reviewed Aug 14, 2024

View reviewed changes

resourcehelper: expose ReportCreateEvent/ReportUpdateEvent

a042827

vrutkovs force-pushed the cert-rotation-skip-get-in-apply branch from ce677d9 to 34d2c9d Compare August 14, 2024 06:47

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2024

p0lyn0mial reviewed Aug 14, 2024

View reviewed changes

vrutkovs force-pushed the cert-rotation-skip-get-in-apply branch from 34d2c9d to 0399b35 Compare August 14, 2024 08:38

openshift-ci bot assigned p0lyn0mial Aug 14, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2024

vrutkovs force-pushed the cert-rotation-skip-get-in-apply branch from 0399b35 to 3ac47e0 Compare August 14, 2024 10:52

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2024

vrutkovs force-pushed the cert-rotation-skip-get-in-apply branch from 3ac47e0 to a8b7daa Compare August 14, 2024 12:19

p0lyn0mial reviewed Aug 14, 2024

View reviewed changes

certrotation: avoid using applySecret to catch racy cert updates

3a8d8c5

vrutkovs force-pushed the cert-rotation-skip-get-in-apply branch from a8b7daa to 3a8d8c5 Compare August 14, 2024 13:12

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2024

soltysh approved these changes Aug 16, 2024

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2024

openshift-merge-bot bot merged commit e21e788 into openshift:master Aug 16, 2024
4 checks passed

vrutkovs mentioned this pull request Aug 16, 2024

OCPBUGS-38335: Bump library-go openshift/cluster-kube-controller-manager-operator#818

Closed


		if reason := c.CertCreator.NeedNewTargetCertKeyPair(targetCertKeyPairSecret, signingCertKeyPair, caBundleCerts, c.Refresh, c.RefreshOnlyWhenExpired, secretDoesntExist); len(reason) > 0 {
		if reason := c.CertCreator.NeedNewTargetCertKeyPair(targetCertKeyPairSecret, signingCertKeyPair, caBundleCerts, c.Refresh, c.RefreshOnlyWhenExpired, creationRequired); len(reason) > 0 {

OCPBUGS-38335: certrotation: avoid using applySecret to catch racy cert updates #1772

OCPBUGS-38335: certrotation: avoid using applySecret to catch racy cert updates #1772

Conversation

vrutkovs commented Aug 6, 2024 • edited Loading

openshift-ci-robot commented Aug 6, 2024 • edited by openshift-ci bot Loading

openshift-ci-robot commented Aug 6, 2024 • edited by openshift-ci bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertinatto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Aug 14, 2024

vrutkovs commented Aug 14, 2024

p0lyn0mial commented Aug 14, 2024

p0lyn0mial commented Aug 14, 2024

vrutkovs commented Aug 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Aug 14, 2024

openshift-ci bot commented Aug 14, 2024

soltysh left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 16, 2024

openshift-ci-robot commented Aug 16, 2024

vrutkovs commented Aug 6, 2024 •

edited

Loading

openshift-ci-robot commented Aug 6, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 6, 2024 •

edited by openshift-ci bot

Loading