refactor(api)!: move ValkeyCluster TLS under spec.networking by daanvinken · Pull Request #339 · valkey-io/valkey-operator

daanvinken · 2026-07-27T15:09:54Z

Summary

Phase 1a of #318: introduce spec.networking and prefer spec.networking.tls.

Top-level spec.tls stays for one-release dual-read so rolling the CRD does not prune TLS from objects already in etcd. CEL rejects setting both. Using only top-level tls emits a deprecation Warning event. Top-level field will be removed in a later release.

spec:
  networking:
    tls:
      certificate:
        secretName: valkey-tls

Behaviour

Spec	Result
`networking.tls` only	used (preferred)
top-level `tls` only	dual-read + deprecation warning
both set	admission reject (CEL)
neither	no TLS

ValkeyNode still uses top-level node.Spec.TLS (internal copy from the cluster).

Out of scope

Hostname discovery (networking.discovery) — feat: announce cluster nodes by hostname for TLS-friendly discovery #296 / Umbrella: spec.networking for in-cluster endpoint & connectivity configuration #297
External access (networking.external) — [Design] External cluster access for client connections outside Kubernetes #276
Removing top-level spec.tls (follow-up after migration window)

Testing

go test ./api/v1alpha1/ ./internal/controller/
make generate manifests
e2e TLS manifest updated to networking.tls

Umbrella #318 stays open for discovery/external.

Introduce NetworkingSpec and prefer networking.tls. Keep top-level spec.tls for one-release dual-read so CRD upgrade does not prune TLS from existing objects. CEL rejects setting both. Deprecation warning when only top-level tls is set. Top-level tls will be removed later. Refs: valkey-io#318 Testing - go test ./api/v1alpha1/ ./internal/controller/ - make generate manifests Signed-off-by: daanvinken <daanvinken@tythus.com>

greptile-apps · 2026-07-27T15:29:56Z

Greptile Summary

Refactors ValkeyCluster TLS configuration beneath spec.networking.

Adds the NetworkingSpec API type and nested TLS schema.
Updates controller configuration and ValkeyNode propagation to use GetTLS.
Regenerates CRD and deepcopy artifacts and updates TLS tests and documentation.

Confidence Score: 3/5

This PR is not safe to merge because upgrading an existing top-level-TLS cluster can roll it onto plaintext communication.

Removing the compatibility field and GetTLS fallback causes both rendered server configuration and managed ValkeyNode specs to lose TLS during reconciliation of legacy objects.

Files Needing Attention: api/v1alpha1/valkeycluster_types.go, internal/controller/config.go, internal/controller/valkeycluster_controller.go, config/crd/bases/valkey.io_valkeyclusters.yaml

Security Review

Existing clusters using legacy top-level spec.tls can be reconciled to plaintext because the compatibility field and dual-read fallback were removed together with the repeated warning.

T-Rex Logs

What T-Rex did

Executed a focused Go test to exercise the legacy TLS decoding path and verify how TLS information is rendered in the server configuration.
Observed that the GetTLS call returned nil, the rendered server configuration omitted TLS-related ports, and the desired ValkeyNode had a nil Spec.TLS, confirming the plaintext rollout path.
Reviewed CRD changes showing spec.networking.tls was introduced, requiring certificate.secretName and constraining its length to 1–253, and noted the head checkout remained at the requested SHA with no tracked modifications.
Confirmed that no legacy top-level TLS fallback behavior was exercised or reported.
Artifacts were collected to support both proofs, including a focused test reproduction, a controller-overlay execution artifact, verbose test output, and CRD validation logs.

_{Ran code and verified through T-Rex}

Important Files Changed

Filename	Overview
api/v1alpha1/valkeycluster_types.go	Introduces nested networking TLS but prematurely removes the compatibility field and fallback required for existing TLS clusters.
internal/controller/config.go	Routes server TLS rendering through GetTLS, making legacy clusters render plaintext configuration after the compatibility removal.
internal/controller/valkeycluster_controller.go	Propagates GetTLS into managed ValkeyNodes while removing the prior deprecation-event path.
config/crd/bases/valkey.io_valkeyclusters.yaml	Adds the nested networking TLS schema but removes the top-level TLS schema before the migration window is complete.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Existing cluster with spec.tls] --> B[Upgrade operator and CRD]
  B --> C[GetTLS returns nil]
  C --> D[Render configuration without TLS]
  C --> E[Set ValkeyNode TLS to nil]
  E --> F[Roll pods without TLS]
  D --> F

_{Reviews (2): Last reviewed commit: "refactor(api)!: drop top-level spec.tls;..." | Re-trigger Greptile}

sandeepkunusoth · 2026-07-27T16:51:54Z

+	// objects are dual-read and not silently pruned on CRD upgrade. Cannot be
+	// set together with networking.tls (CEL). Will be removed in a later release.
 	// +optional
 	TLS *TLSConfig `json:"tls,omitempty"`


i think we can remove these old fields within same PR. as 0.* version is still not mature and causes breaking changes between operator upgrades?

Agree, we can remove them. I did the same for the scheduling changes I made.

Also agree, it will simplify this PR a lot. We give a warning/heads up in the release notes instead of in the docs as previously done then.

Ok let's go with democracy. Updating PR :)

jdheyburn · 2026-07-27T21:50:39Z

@@ -287,6 +304,27 @@ type TLSConfig struct {
 	Certificate CertificateRef `json:"certificate,omitempty"`


Do you mind adding some validation here, until we have additional TLS fields added in future?

Suggested change

Certificate CertificateRef `json:"certificate,omitempty"`

// +kubebuilder:validation:Required

Certificate CertificateRef `json:"certificate,omitempty"`

jdheyburn · 2026-07-27T21:51:29Z

Similar to a previous comment, we want this to be mandatory if its defined:

Suggested change

// +kubebuilder:validation:Required

// +kubebuilder:validation:MinLength=1

// +kubebuilder:validation:MaxLength=253

SecretName string `json:"secretName,omitempty"`

melancholictheory · 2026-07-28T09:13:11Z

this is the migration i flagged on #318, done the careful way, and for TLS specifically i'd argue against the "just remove it, we're 0.x" that came up in the thread.

the reason is the failure mode. for the scheduling move, dropping the top-level field on an already-stored cluster meant pods reschedule, annoying but recoverable. for tls it means the cluster comes back up with tls off: an encrypted, authenticated cluster silently becomes plaintext on a CRD roll, and every client that expected tls fails. that's a security downgrade triggered by an operator upgrade, not a reschedule, so it's exactly the field where the silent prune is worth avoiding.

the dual-read + CEL-rejects-both + deprecation warning you've got is the right shape for that, and it also sets the pattern for the external-access move coming next, which has the same breaking-reshape problem. if the decision is still to remove top-level tls now, the mitigation has to be loud: a release note that says migrate the CR before rolling the CRD, not just the deprecation event, since the event only helps people who are still applying manifests, not the objects sitting in etcd.

daanvinken · 2026-07-28T12:18:42Z

Yeah I should have clarified but that's exactly the reason why I kept it, it's a strong security regression due to a simple operator upgrade.

I do understand the point of @jdheyburn and @sandeepkunusoth though that this operator is not meant for critical workloads yet.

I'd be also in favor of keeping backwards compatibility for at least one version and then clean up. This makes upgrades a lot easier and it's nice for us maintainers to gain some mileage with these kind of migrations (hoping we never need them ;)).
Happy to discuss further.

daanvinken · 2026-07-28T12:43:48Z

See thread - moving forward with cleanup of old spec in this PR.

Remove dual-read of legacy ValkeyCluster spec.tls per review consensus. TLS lives only under networking.tls. Require TLSConfig.certificate and CertificateRef.secretName when TLS is set. Docs call out hard migration before CRD roll. Testing - go test ./api/v1alpha1/ ./internal/controller/ - make generate manifests Signed-off-by: daanvinken <daanvinken@tythus.com>

greptile-apps · 2026-07-28T13:02:58Z

+func (c *ValkeyCluster) GetTLS() *TLSConfig {
+	if c == nil || c.Spec.Networking == nil {
+		return nil
+	}
+	return c.Spec.Networking.TLS
 }


Legacy TLS silently becomes plaintext

When an operator and CRD upgrade reconciles an existing cluster that uses only top-level spec.tls, GetTLS returns nil because the compatibility field and fallback have been removed. Configuration rendering then omits TLS and the desired ValkeyNode specs clear TLS, causing the pods to roll with plaintext communication.

Artifacts

Repro: focused Go test exercising legacy YAML decoding, GetTLS, configuration rendering, and desired ValkeyNode generation

Evidence file captured while the check ran.

Repro: Go overlay used to execute the focused test against the controller package without modifying tracked source

Evidence file captured while the check ran.

Repro: verbose failing test output showing nil TLS and omitted TLS directives

The full command output behind this check.

_{Ran code and verified through T-Rex}

@greptile-apps If the user switches spec.tls to spec.networking.tls, things would continue as normal - right?

Yes, exactly — if a user explicitly updates their manifest to spec.networking.tls, GetTLS() will return the config correctly and TLS continues to work.

The concern in my original comment is specifically about the upgrade window: objects already persisted in etcd with only spec.tls set (no user action taken yet) will have GetTLS() return nil after the operator upgrade, because there's no fallback path like:

if c.Spec.Networking != nil && c.Spec.Networking.TLS != nil { return c.Spec.Networking.TLS } // fallback to legacy field return c.Spec.TLS

The PR description says top-level spec.tls stays for "one-release dual-read" and emits a deprecation warning, but GetTLS() as written doesn't implement that fallback — it only reads from spec.networking.tls. So any cluster that hasn't been manually migrated yet would silently lose TLS on the next reconcile after upgrading the operator.

melancholictheory · 2026-07-28T13:22:34Z

makes sense for 0.x, no objection to going with the removal. the one thing i'd ask of the release note then: have it say plainly that a cluster with top-level spec.tls set comes back up with TLS off after the CRD rolls, so it reads as "this turns TLS off, migrate first" rather than "a field moved". the failure mode is silent and security-facing, so the wording is what does the protecting here once the dual-read is gone. and worth stating the ordering explicitly: migrate the CR to spec.networking.tls before rolling the new CRD, since after that the value is already gone from etcd's view.

jdheyburn · 2026-07-28T13:35:06Z

@melancholictheory Would a message in the release notes as we did for scheduling be sufficient? https://github.com/valkey-io/valkey-operator/releases/tag/v0.4.0

melancholictheory · 2026-07-28T13:59:58Z

release notes are the right place, yes, but the scheduling wording isn't quite enough as a template for this one. that note says "these fields moved to spec.scheduling", which is accurate for scheduling because the failure mode is cosmetic, pods reschedule. for tls the same "it moved" sentence hides the actual effect: an upgrade with top-level spec.tls still set doesn't move anything, it drops it, and the cluster comes back up serving plaintext with auth off.

so the note needs one extra sentence the scheduling one didn't: not just "tls moved to spec.networking.tls", but "if you upgrade with top-level spec.tls still set, TLS is disabled on the cluster, migrate the field before rolling the new CRD". same format as v0.4.0, just with the consequence spelled out and the ordering made explicit. that's the whole ask, the mechanism (release note) is fine, it's the wording that has to name the security effect rather than describe a field move.

and to your inline question (switching spec.tls -> spec.networking.tls continues as normal): yes, an explicit migration is seamless, the gap is only the upgrade-without-migrating path, which is exactly what the note covers.

jdheyburn · 2026-07-28T14:03:10Z

 | `tls.crt` | Server certificate (or chain) |
 | `tls.key` | Private key for the certificate |

+> **Breaking (alpha):** top-level `spec.tls` is removed. Use `spec.networking.tls`. Migrate every ValkeyCluster **before** rolling the CRD; the API server prunes unknown fields on existing objects, so an unmigrated CR quietly loses TLS after the CRD upgrade.


I think we can put this in the release notes instead, or we will remove this in 0.6?

daanvinken marked this pull request as ready for review July 27, 2026 15:17

greptile-apps Bot reviewed Jul 27, 2026

View reviewed changes

Comment thread internal/controller/valkeycluster_controller.go Outdated

sandeepkunusoth reviewed Jul 27, 2026

View reviewed changes

jdheyburn reviewed Jul 27, 2026

View reviewed changes

jdheyburn linked an issue Jul 27, 2026 that may be closed by this pull request

[DESIGN]: spec.networking API for ValkeyCluster connectivity #318

Open

5 tasks

jdheyburn removed a link to an issue Jul 27, 2026

[DESIGN]: spec.networking API for ValkeyCluster connectivity #318

Open

5 tasks

jdheyburn mentioned this pull request Jul 27, 2026

[DESIGN]: spec.networking API for ValkeyCluster connectivity #318

Open

5 tasks

greptile-apps Bot reviewed Jul 28, 2026

View reviewed changes

jdheyburn reviewed Jul 28, 2026

View reviewed changes

		@@ -287,6 +304,27 @@ type TLSConfig struct {
		Certificate CertificateRef `json:"certificate,omitempty"`

	Certificate CertificateRef `json:"certificate,omitempty"`
	// +kubebuilder:validation:Required
	Certificate CertificateRef `json:"certificate,omitempty"`

+    // +kubebuilder:validation:Required
+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=253
+	SecretName string `json:"secretName,omitempty"`

Uh oh!

Conversation

daanvinken commented Jul 27, 2026

Summary

Behaviour

Out of scope

Testing

Uh oh!

greptile-apps Bot commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 3/5

Security Review

T-Rex Logs

Important Files Changed

Flowchart

Uh oh!

Uh oh!

sandeepkunusoth Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

jdheyburn Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

bjosv Jul 28, 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!

daanvinken Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

jdheyburn Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

jdheyburn Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

melancholictheory commented Jul 28, 2026

Uh oh!

daanvinken commented Jul 28, 2026

Uh oh!

daanvinken commented Jul 28, 2026

Uh oh!

greptile-apps Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

jdheyburn Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

melancholictheory commented Jul 28, 2026

Uh oh!

jdheyburn commented Jul 28, 2026

Uh oh!

melancholictheory commented Jul 28, 2026

Uh oh!

jdheyburn Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

greptile-apps Bot commented Jul 27, 2026 •

edited

Loading

bjosv Jul 28, 2026 •

edited

Loading