OCPBUGS-33013: certsyncpod+installerpod: Swap secret/cm directories atomically #2009

tchap · 2025-09-09T15:41:28Z

Currently it can happen that cert-syncer replaces some of the secret/configmap files successfully and then fails. This can lead to problems when these are e.g. TLS cert/key files and the directory gets inconsistent. This may seem transient, but when cert-syncer is terminated in the middle, it can later fail to start as the whole kube-apiserver gets into a crash loop.

This introduces a new staticpod.SwapDirectoriesAtomic, which uses unix.Renameat2 with RENAME_EXCHANGE flag set.

openshift-ci-robot · 2025-09-10T12:56:08Z

@tchap: This pull request references Jira Issue OCPBUGS-33013, 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.21.0) matches configured target version for branch (4.21.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:

Currently it can happen that cert-syncer replaces some of the secret/configmap files successfully and then fails. This can lead to problems when these are e.g. TLS cert/key files and the directory gets inconsistent. This may seem transient, but when cert-syncer is terminated in the middle, it can later fail to start as the whole kube-apiserver gets into a crash loop.

This introduces a new staticpod.SwapDirectoriesAtomic, which uses unix.Renameat2 with RENAME_EXCHANGE flag set.

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 · 2025-09-10T12:57:22Z

@tchap: This pull request references Jira Issue OCPBUGS-33013, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @wangke19

In response to this:

Currently it can happen that cert-syncer replaces some of the secret/configmap files successfully and then fails. This can lead to problems when these are e.g. TLS cert/key files and the directory gets inconsistent. This may seem transient, but when cert-syncer is terminated in the middle, it can later fail to start as the whole kube-apiserver gets into a crash loop.

This introduces a new staticpod.SwapDirectoriesAtomic, which uses unix.Renameat2 with RENAME_EXCHANGE flag set. This should not be a problem as this call is supported since Linux 3.15 on all modern file systems.

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 · 2025-09-10T13:11:53Z

@tchap: This pull request references Jira Issue OCPBUGS-33013, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.21.0) matches configured target version for branch (4.21.0)
bug is in the state POST, 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:

Currently it can happen that cert-syncer replaces some of the secret/configmap files successfully and then fails. This can lead to problems when these are e.g. TLS cert/key files and the directory gets inconsistent. This may seem transient, but when cert-syncer is terminated in the middle, it can later fail to start as the whole kube-apiserver gets into a crash loop.

This introduces a new staticpod.SwapDirectoriesAtomic, which uses unix.Renameat2 with RENAME_EXCHANGE flag set.

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.

tchap · 2025-09-10T13:12:25Z

I actually have to make sure this can be merged as this is only supported on Linux 3.15 or later.

/hold

tchap · 2025-09-10T13:47:04Z

This patch should be OK for RHEL 8 or later based on https://access.redhat.com/articles/3078

The latest CI for OCP 4.21 actually uses RHEL 9.6.

tchap · 2025-09-11T12:35:32Z

The PR using this change in cluster-kube-apiserver-operator seems to be passing on CI, I deem this ready.

/unhold

p0lyn0mial · 2025-09-15T07:51:42Z

@tchap is there a must-gather from an incident i could take a look at ?

tchap · 2025-09-15T07:57:19Z

@p0lyn0mial I think that I consulted this one: https://access.redhat.com/support/cases/#/case/03849958/discussion?attachmentId=a096R00003JpGgMQAV

p0lyn0mial · 2025-09-15T08:08:24Z

@vrutkovs do you have time to take a look at this issue ?

I think that the issue might be real. I think the issue is when a two file cert is replaced. It can happen that the server picks up the update and notices the public/private key mismatch and crashes. Is there a way to repo this issue ?

p0lyn0mial · 2025-10-13T06:22:19Z

pkg/operator/staticpod/certsyncpod/certsync_controller.go

+}
+
 func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncContext) error {
+	if err := dirutils.RemoveContent(getStagingDir(c.destinationDir)); err != nil {


mhm, maybe this could be done in the Sync method, after creating the staging dir. wdyt?

It would make sense, but here we just call it once for the whole staging area while Sync works per object/directory. So actually it makes sense to call it here to prune old staging directories, not just the directory for the object being staged.

p0lyn0mial · 2025-10-13T06:26:58Z

pkg/operator/staticpod/installerpod/cmd.go

+		contentDir := getSecretDir(resourceDir, secretBaseName)
+		stagingDir := getSecretStagingDir(resourceDir, secretBaseName)
+
+		if err := atomicdir.Sync(contentDir, 0700, stagingDir, secret.Data, 0600); err != nil {


the sync method expects that the filenames don't hold a path, right ?

Yeah, it's actually being checked.

tchap · 2025-10-14T09:57:59Z

@p0lyn0mial I pushed a complete change now with some unit tests added. They are not complete, particularly FS operations failing are not tested, but also some combinations of sync/get errors are also not tested, because mocking them is annoying. Let me know whether you require more tests. There were none before, so...

tchap · 2025-10-14T09:58:36Z

pkg/operator/staticpod/certsyncpod/certsync_controller.go

+			files[k] = []byte(v)
 		}
-		c.eventRecorder.Eventf("CertificateUpdated", "Wrote updated configmap: %s/%s", configMap.Namespace, configMap.Name)
+		// XXX: Are these permissions correct?


Not sure about this actually...

tchap · 2025-10-14T09:59:35Z

pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go

 		})
 	}
 }
-


I just moved this into a separate package to be able to import it.

tchap · 2025-10-14T10:01:24Z

installerpod BWT does contain some tests regarding the files written, so I haven't touched or improved that.

tchap · 2025-10-14T10:38:22Z

Updated openshift/cluster-kube-apiserver-operator#1917 with the current changes.

p0lyn0mial · 2025-10-15T09:06:02Z

pkg/operator/staticpod/certsyncpod/certsync_controller.go

+}
+
 func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncContext) error {
+	if err := dirutils.RemoveContent(getStagingDir(c.destinationDir)); err != nil {


I think that we should clean per resource cm/secrets otherwise the old data might be left from the previous runs, right ?

I think that we should do that inside the Sync func, wdyt ?

we could add cleaning somewhere here and fail if the cleaning fn returns an err, wdyt?

atomicdir.Sync works with a particular object, right? So you would tell it to sync cm into configmaps/cm and use staging/cert-sync/configmaps/cm for staging. So what should it prune exactly? When there is a leftover in staging/cert-sync, Sync cannot really remove it as it work with a subdir of that path. So my idea was to prune staging/cert-sync at the beginning of sync so that we are clean and that's it.

Having said that, we can also extend Sync to remove everything from staging/cert-sync/configmaps/cm just to be sure there are no leftovers and it's encapsulated, but I wanted to ensure that on a higher level with a single rm call.

pkg/operator/staticpod/installerpod/cmd.go

pkg/operator/staticpod/certsyncpod/certsync_controller.go

tchap · 2025-10-15T11:19:31Z

pkg/operator/staticpod/installerpod/cmd.go

-	filePerms := os.FileMode(0600)
-	if strings.HasSuffix(fullFilename, ".sh") {
-		filePerms = 0755
-	}


Hmm, didn't notice this check for custom permission setting...

tchap · 2025-10-16T15:27:38Z

pkg/operator/staticpod/certsyncpod/certsync_controller_sync_linux_test.go

+					strings.HasSuffix(path, "/staging/cert-sync/secrets") ||
+					strings.HasSuffix(path, "/staging/cert-sync/configmaps") ||
+					path == filepath.Join(controller.destinationDir, "configmaps") ||
+					path == filepath.Join(controller.destinationDir, "secrets") {


This is not too pretty, but meh.

p0lyn0mial

added a few more comments. overall lgtm.
please also test this pr with some operator e.g. kas-o

pkg/operator/staticpod/certsyncpod/certsync_controller.go

pkg/operator/staticpod/certsyncpod/certsync_controller_linux_test.go

pkg/operator/staticpod/installerpod/cmd.go

Use atomicdir.Sync to write target secret/configmap directories to be synchronized with the relevant objects. Added unit tests, but the coverage is not complete. Particularly filesystem operations failing are not being tested.

openshift-ci · 2025-10-24T13:07:44Z

@tchap: 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.

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 Sep 9, 2025

openshift-ci bot requested review from p0lyn0mial and tkashem September 9, 2025 15:44

tchap force-pushed the atomic-certsync branch from b3eca57 to a389cbd Compare September 10, 2025 09:12

tchap mentioned this pull request Sep 10, 2025

WIP: Update library-go to improve cert-syncer openshift/cluster-kube-apiserver-operator#1917

Open

tchap force-pushed the atomic-certsync branch 4 times, most recently from 8dec327 to 60f05a8 Compare September 10, 2025 12:45

tchap changed the title ~~WIP: certsyncpod: Swap secret/cm directories atomically~~ OCPBUGS-33013: certsyncpod: Swap secret/cm directories atomically Sep 10, 2025

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 10, 2025

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

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

openshift-ci bot requested a review from wangke19 September 10, 2025 12:56

tchap changed the title ~~OCPBUGS-33013: certsyncpod: Swap secret/cm directories atomically~~ WIP: OCPBUGS-33013: certsyncpod: Swap secret/cm directories atomically Sep 10, 2025

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 Sep 10, 2025

tchap changed the title ~~WIP: OCPBUGS-33013: certsyncpod: Swap secret/cm directories atomically~~ OCPBUGS-33013: certsyncpod: Swap secret/cm directories atomically Sep 10, 2025

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 10, 2025

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

tchap force-pushed the atomic-certsync branch from 60f05a8 to f6df27a Compare September 10, 2025 14:03

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

p0lyn0mial reviewed Oct 13, 2025

View reviewed changes

tchap force-pushed the atomic-certsync branch from a835118 to 6b52148 Compare October 14, 2025 09:54

tchap commented Oct 14, 2025

View reviewed changes