refactor(api)!: move ValkeyCluster TLS under spec.networking - #339
Conversation
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // 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"` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Agree, we can remove them. I did the same for the scheduling changes I made.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok let's go with democracy. Updating PR :)
| @@ -287,6 +304,27 @@ type TLSConfig struct { | |||
| Certificate CertificateRef `json:"certificate,omitempty"` | |||
There was a problem hiding this comment.
Do you mind adding some validation here, until we have additional TLS fields added in future?
| Certificate CertificateRef `json:"certificate,omitempty"` | |
| // +kubebuilder:validation:Required | |
| Certificate CertificateRef `json:"certificate,omitempty"` |
There was a problem hiding this comment.
Similar to a previous comment, we want this to be mandatory if its defined:
| // +kubebuilder:validation:Required | |
| // +kubebuilder:validation:MinLength=1 | |
| // +kubebuilder:validation:MaxLength=253 | |
| SecretName string `json:"secretName,omitempty"` |
|
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. |
|
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 ;)). |
|
See thread - moving forward with cleanup of old spec in this PR. |
| func (c *ValkeyCluster) GetTLS() *TLSConfig { | ||
| if c == nil || c.Spec.Networking == nil { | ||
| return nil | ||
| } | ||
| return c.Spec.Networking.TLS | ||
| } |
There was a problem hiding this comment.
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
- Evidence file captured while the check ran.
- 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.
There was a problem hiding this comment.
@greptile-apps If the user switches spec.tls to spec.networking.tls, things would continue as normal - right?
There was a problem hiding this comment.
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.TLSThe 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.
There was a problem hiding this comment.
Resolved on the description: dual-read was dropped by design after review.
GetTLS only reads networking.tls. Protection is migrate-before-CRD + release notes (stating TLS turns off if you don't migrate), not a fallback path.
There was a problem hiding this comment.
Got it — thanks for the clarification. The PR description does state this explicitly, and the GetTLS() implementation matches the stated design. My concern was valid as a flag (silent plaintext on missed migration), and it sounds like the release note wording — explicitly calling out the TLS-off consequence rather than a soft "field moved" message — is the intended mitigation.
No code change needed here. The implementation is consistent with the design decision.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
|
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 |
|
@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 |
|
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 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 |
| | `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. |
There was a problem hiding this comment.
I think we can put this in the release notes instead, or we will remove this in 0.6?
There was a problem hiding this comment.
Yeah I'm on board with stripping at 0.6.
Introduce NetworkingSpec and move TLS to networking.tls. Drop top-level spec.tls (alpha hard break). Require certificate.secretName when TLS is set. Migrate every ValkeyCluster to networking.tls before rolling the new CRD, or the cluster comes back up with TLS off. Refs: valkey-io#318 Testing - go test ./api/v1alpha1/ ./internal/controller/ - make generate manifests Signed-off-by: daanvinken <daanvinken@tythus.com>
19b9a0b to
c6163cc
Compare
📝 WalkthroughWalkthroughChangesValkeyCluster TLS nesting
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
5 participants
Summary
Phase 1a of #318: introduce
spec.networkingand move TLS tospec.networking.tls.Hard break (no dual-read): top-level
spec.tlsis removed from the API.GetTLS()only readsspec.networking.tls. There is no fallback and no deprecation warning path. When TLS is set,certificate.secretNameis required.Breaking upgrade / release note
Same approach as the scheduling move (v0.4.0): document in release notes + docs. For TLS the failure mode is worse than scheduling (security downgrade), so the wording must state the consequence, not only that the field moved.
Migrate every ValkeyCluster to
spec.networking.tlsbefore rolling the new CRD/operator.After the CRD drops top-level
tls, the API server prunes that field from objects already in etcd. The operator then sees no TLS config and reconciles the cluster without TLS (plaintext). Clients that expected TLS fail. That is intentional 0.x behaviour; the release note is the protection, not a dual-read.Draft for the next operator GitHub release
Copy into the release body (do not use a soft “fields moved” line alone):