From 3cdaf383810f249d2993758edc9a40b4c0a05a1e Mon Sep 17 00:00:00 2001 From: mohityadav8 Date: Sat, 17 Oct 2026 05:39:51 +0530 Subject: [PATCH 1/2] fix(operator): propagate webhook cert disk write errors Signed-off-by: mohityadav8 --- .../internal/controller/webhook_controller.go | 15 ++++++++--- .../controller/webhook_controller_test.go | 27 +++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/operator/internal/controller/webhook_controller.go b/operator/internal/controller/webhook_controller.go index 8d457eb46..ae93052c4 100644 --- a/operator/internal/controller/webhook_controller.go +++ b/operator/internal/controller/webhook_controller.go @@ -385,9 +385,12 @@ func (r *WebhookController) GetOrCreateWebhookCertSecret(ctx context.Context, se return nil, err } - // Write cert and key to disk if CertDir is set + // Write cert and key to disk if CertDir is set. Fail before touching the Secret so + // a disk that cannot be written does not leave disk and Secret out of sync. if r.certDir != "" { - _ = writeCertAndKey([]byte(webhookCert.TLSCert), []byte(webhookCert.TLSKey), r.certDir) + if err := writeCertAndKey([]byte(webhookCert.TLSCert), []byte(webhookCert.TLSKey), r.certDir); err != nil { + return nil, fmt.Errorf("writing webhook cert to %s: %w", r.certDir, err) + } } secret = webhookCert.ToSecret(secretName, namespace, r.opts.ServiceName) @@ -435,9 +438,13 @@ func (r *WebhookController) CheckOrUpdateWebhookCertSecret(ctx context.Context, return false, err } - // Write cert and key to disk if CertDir is set + // Write cert and key to disk if CertDir is set. Returwebhook_controller_test.gon the error before the Secret is + // updated: otherwise a failing disk leaves disk != Secret, so every reconcile re-mints + // a cert and rewrites the Secret. Returning here lets controller-runtime back off. if r.certDir != "" { - _ = writeCertAndKey([]byte(webhookCert.TLSCert), []byte(webhookCert.TLSKey), r.certDir) + if err := writeCertAndKey([]byte(webhookCert.TLSCert), []byte(webhookCert.TLSKey), r.certDir); err != nil { + return false, fmt.Errorf("writing rotated webhook cert to %s: %w", r.certDir, err) + } } secret.Data["ca.crt"] = webhookCert.CABytes diff --git a/operator/internal/controller/webhook_controller_test.go b/operator/internal/controller/webhook_controller_test.go index 5663862d6..88268dab0 100644 --- a/operator/internal/controller/webhook_controller_test.go +++ b/operator/internal/controller/webhook_controller_test.go @@ -933,6 +933,33 @@ var _ = Describe("WebhookController", Ordered, func() { Expect(err).NotTo(HaveOccurred()) Expect(updated).To(BeFalse()) }) + + It("should return an error and leave the Secret untouched when the cert cannot be written to disk", func() { + diskCert, err := generateCert(serviceName, namespace, certValidityDurationYear) + Expect(err).NotTo(HaveOccurred()) + secretCert, err := generateCert(serviceName, namespace, certValidityDurationYear) + Expect(err).NotTo(HaveOccurred()) + + secret := secretCert.ToSecret(secretName, namespace, serviceName) + controller.Client = fake.NewClientBuilder().WithObjects(secret).Build() + + // Disk holds a different cert than the Secret, so rotation is triggered. + err = writeCertAndKey([]byte(diskCert.TLSCert), []byte(diskCert.TLSKey), tmpDir) + Expect(err).NotTo(HaveOccurred()) + + // Turn tls.key into a directory so the write fails even when tests run as root. + keyPath := filepath.Join(tmpDir, "tls.key") + Expect(os.Remove(keyPath)).To(Succeed()) + Expect(os.Mkdir(keyPath, 0o700)).To(Succeed()) + + origCert := append([]byte(nil), secret.Data["tls.crt"]...) + + updated, err := controller.CheckOrUpdateWebhookCertSecret(context.Background(), secret) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("writing rotated webhook cert")) + Expect(updated).To(BeFalse()) + Expect(secret.Data["tls.crt"]).To(Equal(origCert)) + }) }) }) From 66faae5bb1a8b9596646cbf5b548f3f2a4a5c90e Mon Sep 17 00:00:00 2001 From: mohityadav8 Date: Fri, 23 Oct 2026 00:31:19 +0530 Subject: [PATCH 2/2] fix: make SecretCertWatcher the sole disk writer Signed-off-by: mohityadav8 --- operator/internal/controller/cert_utils.go | 55 ++++++++++++++++++- .../internal/controller/webhook_controller.go | 15 ++--- 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/operator/internal/controller/cert_utils.go b/operator/internal/controller/cert_utils.go index abe50762b..ad7f29fb4 100644 --- a/operator/internal/controller/cert_utils.go +++ b/operator/internal/controller/cert_utils.go @@ -30,6 +30,7 @@ import ( "math/big" "os" "path/filepath" + "sync" "time" "github.com/NVIDIA/nodewright/operator/api/v1alpha1" @@ -243,7 +244,13 @@ func ensureDummyCert(certDir string) error { return nil } +// certWriteMu serializes writeCertAndKey. It is package-level because the writers +// (webhook bootstrap reconcile and SecretCertWatcher) share no struct, only the +// process and the cert directory. +var certWriteMu sync.Mutex + // Write cert and key to disk if CertDir is set, make sure the directory exists, and the files are 600. Will also sync from the secret to the disk. +// Writes are serialized across goroutines and each file is replaced atomically (temp file + rename). func writeCertAndKey(certPEM, keyPEM []byte, certDir string) error { if certDir == "" { return fmt.Errorf("certDir is required") @@ -254,15 +261,59 @@ func writeCertAndKey(certPEM, keyPEM []byte, certDir string) error { certPath := filepath.Join(certDir, "tls.crt") keyPath := filepath.Join(certDir, "tls.key") - if err := os.WriteFile(certPath, certPEM, 0o600); err != nil { + + // Serialize writers so a cert write and a key write from two goroutines + // (bootstrap reconcile and SecretCertWatcher) can never interleave and + // leave tls.crt from one cert next to tls.key from another. + certWriteMu.Lock() + defer certWriteMu.Unlock() + + if err := writeFileAtomic(certPath, certPEM, 0o600); err != nil { return fmt.Errorf("failed to write cert: %w", err) } - if err := os.WriteFile(keyPath, keyPEM, 0o600); err != nil { + if err := writeFileAtomic(keyPath, keyPEM, 0o600); err != nil { return fmt.Errorf("failed to write key: %w", err) } return nil } +// writeFileAtomic writes data to a temp file in the same directory as path and +// renames it into place, so readers see either the old file or the new one in +// full, never a partially written file. The temp file lives next to path so the +// rename stays on one filesystem (rename is only atomic within a filesystem). +func writeFileAtomic(path string, data []byte, perm os.FileMode) error { + tmp, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp-*") + if err != nil { + return err + } + tmpName := tmp.Name() + + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpName) + return err + } + if err := tmp.Chmod(perm); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpName) + return err + } + if err := tmp.Sync(); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpName) + return err + } + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpName) + return err + } + if err := os.Rename(tmpName, path); err != nil { + _ = os.Remove(tmpName) + return err + } + return nil +} + // certExistsOnDisk returns an error if the cert or key is not present on disk func certExistsOnDisk(certDir string) error { certPath := filepath.Join(certDir, "tls.crt") diff --git a/operator/internal/controller/webhook_controller.go b/operator/internal/controller/webhook_controller.go index ae93052c4..8d457eb46 100644 --- a/operator/internal/controller/webhook_controller.go +++ b/operator/internal/controller/webhook_controller.go @@ -385,12 +385,9 @@ func (r *WebhookController) GetOrCreateWebhookCertSecret(ctx context.Context, se return nil, err } - // Write cert and key to disk if CertDir is set. Fail before touching the Secret so - // a disk that cannot be written does not leave disk and Secret out of sync. + // Write cert and key to disk if CertDir is set if r.certDir != "" { - if err := writeCertAndKey([]byte(webhookCert.TLSCert), []byte(webhookCert.TLSKey), r.certDir); err != nil { - return nil, fmt.Errorf("writing webhook cert to %s: %w", r.certDir, err) - } + _ = writeCertAndKey([]byte(webhookCert.TLSCert), []byte(webhookCert.TLSKey), r.certDir) } secret = webhookCert.ToSecret(secretName, namespace, r.opts.ServiceName) @@ -438,13 +435,9 @@ func (r *WebhookController) CheckOrUpdateWebhookCertSecret(ctx context.Context, return false, err } - // Write cert and key to disk if CertDir is set. Returwebhook_controller_test.gon the error before the Secret is - // updated: otherwise a failing disk leaves disk != Secret, so every reconcile re-mints - // a cert and rewrites the Secret. Returning here lets controller-runtime back off. + // Write cert and key to disk if CertDir is set if r.certDir != "" { - if err := writeCertAndKey([]byte(webhookCert.TLSCert), []byte(webhookCert.TLSKey), r.certDir); err != nil { - return false, fmt.Errorf("writing rotated webhook cert to %s: %w", r.certDir, err) - } + _ = writeCertAndKey([]byte(webhookCert.TLSCert), []byte(webhookCert.TLSKey), r.certDir) } secret.Data["ca.crt"] = webhookCert.CABytes