WIP certrotation: don't let multiple controllers rewrite metadata of the shared resource #1693

vrutkovs · 2024-03-13T15:48:55Z

This prevents http 409 errors when both labels and content need updating in one sync. See this test failure when new library-go is being pulled in cluster-kube-apiserver-operator

vrutkovs · 2024-03-13T15:58:29Z

/hold

Testing this in openshift/cluster-kube-apiserver-operator#1655

vrutkovs · 2024-03-14T09:12:11Z

/hold cancel

Seems to work as expected

p0lyn0mial · 2024-04-12T08:01:45Z

pkg/operator/certrotation/cabundle.go

@@ -64,10 +64,11 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC
 	}
 	needsMetadataUpdate = c.AdditionalAnnotations.EnsureTLSMetadataUpdate(&caBundleConfigMap.ObjectMeta) || needsMetadataUpdate
 	if needsMetadataUpdate && len(caBundleConfigMap.ResourceVersion) > 0 {
-		_, _, err := resourceapply.ApplyConfigMap(ctx, c.Client, c.EventRecorder, caBundleConfigMap)
+		actualCABundleConfigMap, _, err := resourceapply.ApplyConfigMap(ctx, c.Client, c.EventRecorder, caBundleConfigMap)


do you mind adding a unit test ?

Done, not sure if it really ensures hotlooping is not happening (imo fuzzing would confirm it)

This PR doesn't solve the hotloop issue. The hotloop issue was resolved by openshift/cluster-kube-apiserver-operator#1661.

There were 4 controllers fighting over kube-control-plane-signer-ca

Fixed this by allowing metadata change to the first controller only (whoever has set the first ownership ref).

I'll run this a few time to make sure this won't make TLS metadata jitter from run to run

p0lyn0mial · 2024-04-12T08:12:40Z

In the future we could refactor the controller to only issue a single update when meta and content require updating.

p0lyn0mial · 2024-04-12T14:17:46Z

@vrutkovs a wrong update ? I don't see a unit test.

This prevents http 409 errors when both labels and content need updating in one sync

p0lyn0mial · 2024-04-15T07:05:48Z

See this test failure when new library-go is being pulled in cluster-kube-apiserver-operator

For the record, the above failure was due to openshift/cluster-kube-apiserver-operator#1661 not due to the issue addressed by this PR.

p0lyn0mial · 2024-04-15T07:06:41Z

pkg/operator/certrotation/cabundle_test.go

+				if !actions[2].Matches("get", "configmaps") {
+					t.Error(actions[2])
+				}
+				if !actions[3].Matches("update", "configmaps") {


the second call is unnecessary, we should refactor the controller to issue only a single API call.

in addition it is worth to also examine the payload of the request (updated configmap), please add it to your todo list.

p0lyn0mial · 2024-04-15T07:08:00Z

/lgtm
/approve

p0lyn0mial · 2024-04-15T07:10:08Z

/assign @stlaz

for approval

openshift-ci · 2024-04-16T11:28:37Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2024-04-16T11:29:09Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: p0lyn0mial, vrutkovs
Once this PR has been reviewed and has the lgtm label, please ask for approval from stlaz. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/operator/certrotation/OWNERS

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

Ensure that multiple controllers managing single resource don't overwrite each other metadata changes. This is archived by checking ownership references - if current controller is set as first owner other metadata changes are being ignored. This ensures that multiple cert rotation controllers can handle single CA bundle configmap - but only one of them will be updating configmap metadata - whichever is started first

…y metadata In this case cert-rotation won't convert secret type as `ensureSecretTLSTypeSet` would never run

openshift-ci · 2024-04-16T13:37:13Z

@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/test-infra repository. I understand the commands that are listed here.

p0lyn0mial · 2024-04-17T08:08:12Z

pkg/operator/certrotation/cabundle.go

 	needsMetadataUpdate := false
 	if c.Owner != nil {
 		needsMetadataUpdate = ensureOwnerReference(&caBundleConfigMap.ObjectMeta, c.Owner)
+		if caBundleConfigMap.ObjectMeta.OwnerReferences[0] == *c.Owner {


Why can't we just have a single owner?

CA bundle can contain multiple signers, controlled by different controllers. We could have a single ownerRef, but that wouldn't reflect reality. It also makes easier to find such CA bundles in the must-gathers

CA bundle can contain multiple signers, controlled by different controllers

This is wrong and should be fixed. Why can't we have a single signer ?
Could you point me to code that does this ?

Two controllers:

InternalLoadBalancerServing

ExternalLoadBalancerServing
are managing single signer - loadbalancer-serving-signer and CA bundle loadbalancer-serving-ca (in order to produce two target certs). As a result, both are added as owners to the signer secret and configmap.

I'm planning to rewrite this as "NewCertRotationController should accept an array of RotatedSelfSignedCertKeySecrets" later to avoid it, but in general it may happen accidentally and we'd need a way to prevent that.

Still, there is a single signer and a CA bundle.
Also as far as I can tell kas-o doesn't make use of the Owner field for both signer and CA resources.

Owner denotes a controller, not the signer or CA. So yes, these are derived from single signer - but controlled by different controllers.

Do you think a better option would be implementing a controller which controls multiple target certs instead?

UPD: created #1722 to try this

I don't understand why we have to have multiple controllers controlling a single resource. I would prefer to have a single controller for that.

It seems to me that we would like to mark which components are using the resource, which could be done with a single controller.

openshift-ci bot requested review from mfojtik and stlaz March 13, 2024 15:51

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 13, 2024

nrb mentioned this pull request Mar 13, 2024

OCPCLOUD-2513: Remove cloud-provider and cloud-config flags #1692

Merged

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 14, 2024

p0lyn0mial reviewed Apr 12, 2024

View reviewed changes

vrutkovs force-pushed the EnsureConfigMapCABundle-apply-fix branch from 7b0793e to 6df5e95 Compare April 12, 2024 14:08

vrutkovs force-pushed the EnsureConfigMapCABundle-apply-fix branch from 6df5e95 to 9f8acda Compare April 12, 2024 14:23

certrotation: update caBundleConfigMap after labels have been applied

079ddac

This prevents http 409 errors when both labels and content need updating in one sync

vrutkovs force-pushed the EnsureConfigMapCABundle-apply-fix branch from 9f8acda to 079ddac Compare April 12, 2024 14:33

p0lyn0mial reviewed Apr 15, 2024

View reviewed changes

openshift-ci bot assigned p0lyn0mial Apr 15, 2024

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

openshift-ci bot assigned stlaz Apr 15, 2024

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

vrutkovs changed the title ~~certrotation: update caBundleConfigMap after labels have been applied~~ certrotation: don't let multiple controllers rewrite metadata of the shared resource Apr 16, 2024

vrutkovs force-pushed the EnsureConfigMapCABundle-apply-fix branch from db12f27 to 21aeec1 Compare April 16, 2024 12:50

certrotation: cover corner case when invalid secret type has necessar…

fbd9afd

…y metadata In this case cert-rotation won't convert secret type as `ensureSecretTLSTypeSet` would never run

vrutkovs mentioned this pull request Apr 16, 2024

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

Closed

p0lyn0mial reviewed Apr 17, 2024

View reviewed changes

vrutkovs changed the title ~~certrotation: don't let multiple controllers rewrite metadata of the shared resource~~ WIP certrotation: don't let multiple controllers rewrite metadata of the shared resource Apr 24, 2024

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP certrotation: don't let multiple controllers rewrite metadata of the shared resource #1693

WIP certrotation: don't let multiple controllers rewrite metadata of the shared resource #1693

vrutkovs commented Mar 13, 2024

vrutkovs commented Mar 13, 2024

vrutkovs commented Mar 14, 2024

p0lyn0mial Apr 12, 2024

vrutkovs Apr 12, 2024

p0lyn0mial Apr 15, 2024

vrutkovs Apr 16, 2024

p0lyn0mial commented Apr 12, 2024

p0lyn0mial commented Apr 12, 2024

p0lyn0mial commented Apr 15, 2024

p0lyn0mial Apr 15, 2024

p0lyn0mial Apr 15, 2024

p0lyn0mial commented Apr 15, 2024

p0lyn0mial commented Apr 15, 2024

openshift-ci bot commented Apr 16, 2024

openshift-ci bot commented Apr 16, 2024

openshift-ci bot commented Apr 16, 2024

p0lyn0mial Apr 17, 2024

vrutkovs Apr 17, 2024

p0lyn0mial Apr 17, 2024

vrutkovs Apr 17, 2024

p0lyn0mial Apr 17, 2024

vrutkovs Apr 19, 2024 •

edited

p0lyn0mial Apr 22, 2024

WIP certrotation: don't let multiple controllers rewrite metadata of the shared resource #1693

Are you sure you want to change the base?

WIP certrotation: don't let multiple controllers rewrite metadata of the shared resource #1693

Conversation

vrutkovs commented Mar 13, 2024

vrutkovs commented Mar 13, 2024

vrutkovs commented Mar 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

p0lyn0mial commented Apr 12, 2024

p0lyn0mial commented Apr 12, 2024

p0lyn0mial commented Apr 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Apr 15, 2024

p0lyn0mial commented Apr 15, 2024

openshift-ci bot commented Apr 16, 2024

openshift-ci bot commented Apr 16, 2024

openshift-ci bot commented Apr 16, 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

vrutkovs Apr 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrutkovs Apr 19, 2024 •

edited