SRVOCF-1052: Reload serving TLS certificate at runtime by dsimansk · Pull Request #173 · openshift/faas-console-plugin

dsimansk · 2026-09-01T10:07:12Z

Summary

🔧 Replace the one-time tls.LoadX509KeyPair call at startup with a tlsreload.Reloader that polls the mounted cert/key pair every 5 minutes and atomically swaps the cached certificate served via tls.Config.GetCertificate, so operator-rotated serving certs are picked up without restarting the pod.
🔧 Skip re-parsing when the on-disk pair is unchanged (content hash) and retain the last valid certificate when an update is incomplete or invalid.
🧪 Add Ginkgo unit tests for the reloader (a rotated cert is served, an invalid replacement keeps the current cert, an unchanged pair skips reparsing).
📚 Document the runtime TLS reload design in docs/ARCHITECTURE.md.

Checklist

Updated docs/ARCHITECTURE.md (if there are relevant changes to our layered architecture)

The backend previously loaded its serving certificate only during startup, so a rotated Secret was not reflected until the pod restarted. Reload the mounted pair periodically and retain the last valid pair when an update is incomplete or invalid, allowing new TLS connections to use the rotated certificate without interrupting existing ones.

The reloader polled every 30s, re-reading and re-parsing the cert/key on every tick and swapping the cached certificate even when nothing changed. It logged only failures, so a real rotation was invisible, and kept an unreachable nil guard in GetCertificate that New already makes impossible. Poll every 5 minutes and skip the parse and atomic swap when a content hash shows the on-disk pair is unchanged, so steady state costs only two small reads. Log successful loads with the cert expiry so rotations are observable, and drop the dead nil guard. Polling is kept (rather than inotify) because Kubernetes rotates mounted secrets via an atomic ..data symlink swap that file watches miss, and it avoids the k8s.io/apiserver dependency tree. The decision is recorded in docs/ARCHITECTURE.md. Issue SRVOCF-1052 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Assert that reload() returns the same cached *tls.Certificate pointer when the on-disk cert/key pair is unchanged, guarding the skip-reparse optimization against a future regression. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

openshift-merge-bot · 2026-09-01T10:07:15Z

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

openshift-ci-robot · 2026-09-01T10:07:17Z

@dsimansk: This pull request references SRVOCF-1052 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.1.0" version, but no target version was set.

Details

In response to this:

Summary

🔧 Replace the one-time tls.LoadX509KeyPair call at startup with a tlsreload.Reloader that polls the mounted cert/key pair every 5 minutes and atomically swaps the cached certificate served via tls.Config.GetCertificate, so operator-rotated serving certs are picked up without restarting the pod.

🔧 Skip re-parsing when the on-disk pair is unchanged (content hash) and retain the last valid certificate when an update is incomplete or invalid.

🧪 Add Ginkgo unit tests for the reloader (a rotated cert is served, an invalid replacement keeps the current cert, an unchanged pair skips reparsing).

📚 Document the runtime TLS reload design in docs/ARCHITECTURE.md.

Fixes SRVOCF-1052

Checklist

Updated docs/ARCHITECTURE.md (if there are relevant changes to our layered architecture)

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 · 2026-09-01T10:18:45Z

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

twoGiants

Thank you for this PR @dsimansk, security improvements are all so important and valued!

Writing it up in a testable packge is the way to go, great call!

I found a few things and left my review below.

Besides the review below I have an idea for an improvement on how you could refactor it towards a solution without a timer.

Reimplement the watch using fsnotify which is already in go.mod as an indirect dependency, so promoting it is alright. Here is a draft from Claude but there is probably an even simpler way:

type Reloader struct {
	certFile     string
	keyFile      string
	current      atomic.Pointer[tls.Certificate]
	lastHash     [sha256.Size]byte
	pollInterval time.Duration // 0 uses defaultPollInterval; tests can override
}

const defaultPollInterval = 1 * time.Minute

// ...

// Run watches the cert/key files for changes and reloads on change.
// It blocks until ctx is cancelled.
func (r *Reloader) Run(ctx context.Context) {
	for {
		if err := r.watch(ctx); err != nil {
			slog.Error("certificate watch failed, restarting", "err", err)
		}
		select {
		case <-ctx.Done():
			return
		case <-time.After(time.Second):
		}
	}
}

func (r *Reloader) watch(ctx context.Context) error {
	w, err := fsnotify.NewWatcher()
	if err != nil {
		return fmt.Errorf("create fsnotify watcher: %w", err)
	}
	defer w.Close()

	for _, f := range []string{r.certFile, r.keyFile} {
		if err := w.Add(f); err != nil {
			return fmt.Errorf("watch %s: %w", f, err)
		}
	}

	interval := r.pollInterval
	if interval == 0 {
		interval = defaultPollInterval
	}
	ticker := time.NewTicker(interval)
	defer ticker.Stop()

	for {
		select {
		case <-ctx.Done():
			return nil

		case event, ok := <-w.Events:
			if !ok {
				return fmt.Errorf("fsnotify events channel closed")
			}
			if event.Has(fsnotify.Remove) || event.Has(fsnotify.Rename) {
				_ = w.Remove(event.Name)
				if err := w.Add(event.Name); err != nil {
					return fmt.Errorf("re-watch %s: %w", event.Name, err)
				}
			}
			if err := r.reload(); err != nil {
				slog.Error("failed to reload TLS certificate", "err", err)
			}

		case err, ok := <-w.Errors:
			if !ok {
				return fmt.Errorf("fsnotify errors channel closed")
			}
			return fmt.Errorf("fsnotify error: %w", err)

		case <-ticker.C:
			if err := r.reload(); err != nil {
				slog.Error("failed to reload TLS certificate (poll)", "err", err)
			}
		}
	}
}

twoGiants · 2026-09-02T12:59:02Z

+	"time"
+)
+
+const certificateReloadInterval = 5 * time.Minute


This should be configurable, so that we don't need a new commit to change it.

twoGiants · 2026-09-02T13:02:17Z

+	}
+}
+
+func (r *Reloader) GetCertificate(_ *tls.ClientHelloInfo) (*tls.Certificate, error) {


This is a public method and Reloader can be initialized without using New -> so this can break.

I'd add correct error handling here.

And when this methods gets some logic it'll need test coverage.

twoGiants · 2026-09-02T13:05:17Z

+	if err != nil {
+		return fmt.Errorf("load TLS certificate: %w", err)
+	}
+	r.current.Store(&cert)


Shouldn't CompareAndSwap be used here? Or Swap? That would probably be "safer".

twoGiants · 2026-09-02T13:09:59Z

+	return nil
+}
+
+func (r *Reloader) Run() {


We probably should have graceful shutdown here (and in main.go!). The change here is small, can you add it?

func (r *Reloader) Run(ctx context.Context) { ticker := time.NewTicker(certificateReloadInterval) defer ticker.Stop() for { select { case <-ctx.Done(): return case <-ticker.C: if err := r.reload(); err != nil { slog.Error("failed to reload TLS certificate", "err", err) } } } }

And for main.go -> pls create a ticket.

twoGiants · 2026-09-02T13:14:20Z

+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("Reloader", func() {


Pls test the package using it's public api, not internal methods.

twoGiants · 2026-09-02T13:15:27Z

+
+func (r *Reloader) reload() error {
+	certPEM, err := os.ReadFile(r.certFile)
+	if err != nil {


This condition has no test.

twoGiants · 2026-09-02T13:15:33Z

+	if err != nil {
+		return fmt.Errorf("read TLS certificate: %w", err)
+	}
+	keyPEM, err := os.ReadFile(r.keyFile)


This condition has no test.

twoGiants · 2026-09-02T13:16:25Z

+)
+
+var _ = Describe("Reloader", func() {
+	It("uses the certificate loaded after a rotation", func() {


You can add a test case for when the certs were deleted and are missing.

openshift-ci · 2026-09-02T13:38:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

Cragsmann · 2026-09-03T08:33:16Z

Will this solve https://redhat.atlassian.net/browse/SRVOCF-1081 ?

Cragsmann · 2026-09-03T08:49:48Z

/retest

pmeida · 2026-09-03T09:14:49Z

Will this solve https://redhat.atlassian.net/browse/SRVOCF-1081 ?

No, see https://redhat.atlassian.net/browse/SRVOCF-1081?focusedCommentId=18281972

pmeida

Great work!
Agreeing with Stanislav review I would only add one nit

pmeida · 2026-09-03T13:57:01Z

+
+	cert, err := tls.X509KeyPair(certPEM, keyPEM)
+	if err != nil {
+		return fmt.Errorf("load TLS certificate: %w", err)


Suggested change

return fmt.Errorf("load TLS certificate: %w", err)

return fmt.Errorf("parse TLS key pair %w", err)

Otherwise the log from main.go

reloader, err := tlsreload.New(*certFile, *keyFile) if err != nil { log.Fatalf("Failed to load TLS certificate: %v", err) }

will result in an akward error message like Failed to load TLS certificate: load TLS certificate: ...

dsimansk and others added 3 commits September 1, 2026 11:53

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

openshift-ci Bot requested review from matejvasek and pmeida September 1, 2026 10:07

twoGiants self-assigned this Sep 2, 2026

twoGiants self-requested a review September 2, 2026 13:34

twoGiants requested changes Sep 2, 2026

View reviewed changes

pmeida reviewed Sep 3, 2026

View reviewed changes

	return fmt.Errorf("load TLS certificate: %w", err)
	return fmt.Errorf("parse TLS key pair %w", err)

Uh oh!

Conversation

dsimansk commented Sep 1, 2026

Summary

Checklist

Uh oh!

openshift-merge-bot Bot commented Sep 1, 2026

Uh oh!

openshift-ci-robot commented Sep 1, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

openshift-ci Bot commented Sep 1, 2026

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Sep 2, 2026

Uh oh!

Cragsmann commented Sep 3, 2026

Uh oh!

Cragsmann commented Sep 3, 2026

Uh oh!

pmeida commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmeida left a comment

Choose a reason for hiding this comment

Uh oh!

pmeida Sep 3, 2026 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

openshift-ci-robot commented Sep 1, 2026 •

edited by openshift-ci Bot

Loading

pmeida commented Sep 3, 2026 •

edited

Loading

pmeida Sep 3, 2026 •

edited

Loading