[ANAMASGARD]

Overview

Fixes #441.

Certificate rotation currently shuts down and restarts the webhook HTTPS server, temporarily refusing connections. This change serves certificates through a synchronized tls.Config.GetCertificate cache while keeping the same server running.

• Validate stable file snapshots during startup and reload.
• Preserve the last valid certificate during missing, malformed, mismatched, or overlapping updates.
• Detect Kubernetes Secret symlink replacements using file identity, size, and modification time.
• Remove the polling watcher and certificate-triggered restart path.
• Add regression tests for existing connections, in-flight admission requests, concurrent handshakes, repeated rotations, snapshot consistency, and invalid-input recovery.

Exported interfaces, admission behavior, Helm configuration, and certificate-trust policy remain unchanged.

Signed Commits

• Yes, I signed my commits.

How to Test

• go test -count=1 ./admission/webhook
• CGO_ENABLED=1 go test -race -count=3 ./admission/webhook
• go vet ./...
• CGO_ENABLED=0 go build ./...

These checks pass locally, alongside formatting, changed-scope lint, and all three Helm fixture tests. The continuity regressions fail against the original restart implementation and pass with this change.

Full-repository race, CGO-disabled, and integration suites encounter one environment blocker: TestRegistryCommandWatch requires an available Docker-compatible runtime. The same failure reproduces on the unchanged baseline. Remaining watcher tests pass separately with race detection and with CGO disabled.

Summary by CodeRabbit

• New Features

  • TLS certificates now reload automatically when certificate files change, without interrupting active connections.
  • Certificate updates support symlink-based rotations and safely retain the last valid certificate when a replacement is invalid.
  • Server startup now reports invalid certificate or port-binding errors immediately.

• Reliability

  • Improved certificate rotation consistency and retry behavior during transient file or loading issues.
  • Expanded coverage for concurrent connections, certificate updates, startup failures, and connection continuity.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: d3f1700a-514c-4e35-a168-2be976b5d154

📥 Commits

Reviewing files that changed from the base of the PR and between 98eda6b and 02b27ec.

📒 Files selected for processing: 2

• admission/webhook/server.go
• admission/webhook/server_reload_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

The webhook now serves from one HTTP server and obtains TLS certificates through a synchronized cache. The cache detects certificate-file identity, size, and modification-time changes, validates stable snapshots, and retains the last valid certificate after reload failures. Tests cover rotations, ongoing and concurrent connections, cache reloads, startup errors, and listen errors.

Changes: Webhook TLS reload

Layer / File(s)	Summary
Certificate cache and reload rules admission/webhook/server.go, admission/webhook/server_reload_test.go	The cache tracks certificate-file metadata and stores the last valid certificate. It reloads changed files only when metadata remains stable. Tests cover unchanged files, unstable snapshots, retryable failures, concurrent reloads, and immutable published certificates.
TLS callback and server lifecycle admission/webhook/server.go, admission/webhook/server_reload_test.go	Run starts one server and uses tls.Config.GetCertificate to serve cached certificates. It closes the server on context cancellation. Tests cover rotations, connection continuity, symlink swaps, startup validation, and listen errors.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant TLSClient
  participant WebhookServer
  participant CertificateCache
  participant CertificateFiles
  TLSClient->>WebhookServer: Start TLS connection
  WebhookServer->>CertificateCache: Request certificate
  CertificateCache->>CertificateFiles: Check metadata and load changed files
  CertificateFiles-->>CertificateCache: Certificate files
  CertificateCache-->>WebhookServer: Current valid certificate
  WebhookServer-->>TLSClient: Complete TLS connection

Loading

Suggested reviewers: manoj-1407

Merge Risk: ⚪ Minimal · up to 02b27

The webhook now reloads rotated TLS certificates without restarting its HTTPS server, which removes the brief connection-refusal window during rotation. No actionable merge-blocking risk was found in the supplied changes, and the reported validation passes.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 31.82% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: reloading webhook TLS certificates without restarting the server.
Linked Issues check		Issue #441 requires certificate reload without HTTPS server restart, cached reload through tls.Config.GetCertificate, open-connection coverage, and removal of the restart path. server.go implements ce…
Out of Scope Changes check		The reviewed changes are limited to admission/webhook/server.go and its reload tests. The production changes implement #441. The added tests directly verify certificate continuity, reload consistency,…

Full details: Linked Issues check

Explanation

Issue #441 requires certificate reload without HTTPS server restart, cached reload through tls.Config.GetCertificate, open-connection coverage, and removal of the restart path. server.go implements certificateSnapshot and certificateCache, validates metadata before and after loading, retains the last valid certificate after reload errors, and configures tls.Config.GetCertificate. Run starts one server and no longer uses certificate-triggered shutdown or restart logic. server_reload_test.go covers existing connections, in-flight admission, concurrent handshakes, repeated rotations, invalid updates, and symlink swaps.

Full details: Out of Scope Changes check

Explanation

The reviewed changes are limited to admission/webhook/server.go and its reload tests. The production changes implement #441. The added tests directly verify certificate continuity, reload consistency, invalid-input recovery, startup errors, and listener errors. No unrelated behavior, configuration, interface, or trust-policy changes are evidenced.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[matthyx]

Approve 02b27ec59311454078aa0b03249618f393423a53 targeting main (98eda6b3029729b03bb0b8c94542cc19f52f14a5). No material blockers found; independent correctness and architecture reviews agree.

The fix is needed: current main shuts down the listener before replacement during certificate rotation, creating the availability gap documented in #441. A single server with a synchronized GetCertificate cache removes that cause. Stable metadata checks, immutable published certificates, preservation of the last valid pair, and retry after failed loads are sound. Existing public interfaces and admission handling are preserved. Tests cover held connections, in-flight admission, concurrent handshakes, repeated rotation, invalid replacements, symlink replacement, startup and listen errors.

History: #440 is the merged restart-based predecessor, not a duplicate of this improvement. Its earlier incomplete-pair and watcher-race objections were addressed; this PR also removes the dropped-notification/restart path. #372 was explicitly replaced by helm-charts#843 to avoid a separate certgen image, not rejected as a runtime-reload approach. #427 concerns admission alert identity. Searches across indexed open/closed/merged PRs and issues used certificate, TLS, rotation, webhook restart, notifyChanges and server.go/webhook (100-result limits per search); no superseding implementation found within those limits. This is not an exhaustive proof of absence.

Validation: both changed files inspected, caller lifecycle traced, description/reviews/inline comments and prior decisions read. gofmt -d is clean. In a credential-free, network-isolated component harness, the five supplied certificate-cache tests pass with go test -race -count=3 -v .; cache logic was extracted with only its logging call removed. This is component validation, not full-package or end-to-end continuity execution. The baseline restart problem is established by source tracing; I did not independently run a before/after continuity reproduction. Full-package tests, build, vet and lint were not rerun locally. GitHub reports successful cross-platform build, Basic-Test, integration-build, CodeQL and other listed checks for this head. PR450 currently has no existing inline findings or submitted reviews to resolve.

Nonblocking limits: metadata-preserving in-place rewrites remain intentionally undetectable; each callback performs serialized filesystem checks and retries invalid replacements. Neither establishes a material regression for the supported Secret-file rotation path. Immediately before submission, the PR remained open, non-draft, mergeable, with head and current main unchanged.