fix: add validation for rejecting operator owned keys

[deepakpunjabi]

This PR closes #393.

Summary

Previously, setting options in ValkeyCluster.spec.config that the operator manages internally caused a silent mismatch. Because Valkey uses the last entry in its configuration file, the operator's default settings silently overwrote user-defined settings without showing an error.

When TLS was disabled, the operator didn't override port or tls- settings. A user could accidentally change or close the port the operator needed to connect, breaking the deployment.

Features / Behaviour Changes

spec.config now rejects below operator-owned keys(case-insensitive):

• aclfile
• cluster-allow-replica-migration
• cluster-config-file
• cluster-enabled
• cluster-node-timeout
• cluster-replica-validity-factor
• dir
• port
• protected-mode
• shutdown-on-sigterm
• tls-auth-clients
• tls-ca-cert-file
• tls-cert-file
• tls-cluster
• tls-key-file
• tls-port
• tls-replication

Cluster directives the operator does not set(cluster-require-full-coverage, cluster-migration-barrier and cluster-allow-reads-when-down) stay available.

Rule is applied on create and update both.

Implementation

• api/v1alpha1/valkeycluster_types.go adds ReservedConfigKeys and a field-level CEL rule on Config.

• Two new test files.

• CRD is regenerated.

• spec.config sets maxProperties: 1000 to accept the CRD. Valkey has ~200 directives as of now.

• config_reserved_keys_test.go adds test for config drift as config is mentioned in 3 places - getBaseConfig, ReservedConfigKeys and the CEL rule.

• TestReservedConfigKeysMatchCELRule extracts the CEL list out of valkeycluster_types.go to compare it.

Limitations

• This is a breaking change for clusters that already set one of these keys. This should be fine as users were never intended to use this keys and we are on v1alpha1.
• ValkeyNode.spec.config is not covered in this PR.

Testing

config_reserved_keys_test.go, 3 plain Go tests without cluster:

• TestReservedConfigKeysMatchBaseConfig derives the real key set by calling getBaseConfig with TLS both off and on, and asserts it equals ReservedConfigKeys.
• TestReservedConfigKeysMatchCELRule parses the CEL literal from source and compares it to the same list.
• TestReservedConfigKeysAreLowercase guards the lowercased comparison, since an uppercase entry could never match.

valkeycluster_config_validation_test.go, 26 envtest specs against a Kubernetes API server:

• 17 specs, one per reserved key, each asserting rejection. Generated from ReservedConfigKeys so the coverage cannot fall behind the list.

• 3 specs for case-insensitivity: Cluster-Enabled, PROTECTED-MODE, TLS-Port.

• 3 specs for ordinary tunables, the cluster directives the operator does not own, and an absent config.

• Rejection when a reserved key is mixed in with valid ones.

• Rejection when a reserved key is added by update rather than create.

• The message names the offending keys, since there is no webhook to elaborate.

• make test, make lint, pre-commit run --all-files passes.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one issue.
• Commit message explains what changed and why
• Tests are added or updated.
• Documentation files are updated.
• I have run pre-commit locally (pre-commit run --all-files or hooks on commit)

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fef06afc-6dd5-46e0-9b12-56b3501f3f60

📥 Commits

Reviewing files that changed from the base of the PR and between c33988f and e6c07e6.

📒 Files selected for processing (4)

• api/v1alpha1/valkeycluster_types.go
• config/crd/bases/valkey.io_valkeyclusters.yaml
• internal/controller/config_reserved_keys_test.go
• internal/controller/valkeycluster_config_validation_test.go

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

---

📝 Walkthrough

Walkthrough

The API and CRD now reject operator-managed Valkey configuration keys, including case variants, and limit spec.config to 1,000 properties. Tests verify key-list consistency and create/update validation behavior.

Changes

Reserved configuration validation

Layer / File(s)	Summary
Reserved configuration contract api/v1alpha1/valkeycluster_types.go, config/crd/bases/valkey.io_valkeyclusters.yaml	Defines exported operator-owned keys. Adds documentation, a 1,000-property limit, and case-insensitive CEL validation.
Configuration contract consistency tests internal/controller/config_reserved_keys_test.go	Checks that reserved keys match generated base configuration directives, the CEL rule, and lowercase requirements.
Configuration admission tests internal/controller/valkeycluster_config_validation_test.go	Tests accepted keys, reserved-key rejection, case variants, mixed configurations, create and update validation, and error messages.

Suggested reviewers: jdheyburn

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address issue #393 by rejecting operator-owned keys in ValkeyCluster.spec.config with case-insensitive create and update validation.
Out of Scope Changes check	✅ Passed	The changes remain within scope for issue #393 and focus on ValkeyCluster.spec.config validation, CRD updates, and related tests.
Docstring Coverage	✅ Passed	Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Title check	✅ Passed	The title clearly and concisely states that the pull request adds validation to reject operator-owned keys.
Description check	✅ Passed	The description covers the issue, behavior changes, implementation, limitations, testing, and checklist, with only documentation left unchecked.

---

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.}

[greptile-apps]

Greptile Summary

This PR adds API-server validation that rejects case-insensitive operator-owned directives in ValkeyCluster.spec.config, preventing silent configuration conflicts and connectivity failures.

• Defines and documents the reserved configuration-key set in the Go API.
• Regenerates the ValkeyCluster CRD with CEL validation and a 1,000-property bound.
• Adds drift tests and envtest coverage for create, update, case-insensitive, mixed-key, and valid configurations.

Confidence Score: 5/5

The PR appears safe to merge with no actionable defects identified in the changed validation path.

The source API and generated CRD remain aligned, and the tests cover reserved-key drift plus admission behavior for creation, updates, case variants, mixed configurations, and allowed directives.

Important Files Changed

Filename	Overview
api/v1alpha1/valkeycluster_types.go	Adds a synchronized reserved-key list and case-insensitive field-level CEL validation for cluster configuration.
config/crd/bases/valkey.io_valkeyclusters.yaml	Publishes the generated validation rule and bounded map schema in the installed CRD.
internal/controller/config_reserved_keys_test.go	Verifies consistency among the operator-generated base configuration, Go reserved-key list, and CEL literal.
internal/controller/valkeycluster_config_validation_test.go	Exercises the new admission contract through API-server create and update requests, including valid and case-insensitive cases.

_{Reviews (1): Last reviewed commit: "fix: add validation for rejecting operat..." | Re-trigger Greptile}

[melancholictheory]

Ran this on a k3d cluster against a live API server rather than only through the suite, since a CEL rule is only as good as what the apiserver actually admits. Rejection works for aclfile, and the lowerAscii() handling holds for ACLFILE and for Cluster-Node-Timeout. Keys the operator does not set, cluster-require-full-coverage and cluster-migration-barrier, are still accepted, which is the carve-out the comment promises.

The part worth adding to the description is the upgrade path, because it is better than a reader would assume and nothing here says so. I created a cluster carrying dir under the current CRD, then installed this one on top:

• patching an unrelated field, replicas, succeeds and the apiserver attaches a warning quoting the rule. That is validation ratcheting: the unchanged offending field does not block the write, so nobody's existing cluster becomes unmanageable the moment the CRD lands.
• patching spec.config while keeping the reserved key is refused, which is the point.
• removing the key succeeds, so the migration is one edit.

Existing clusters therefore keep reconciling and get nudged on every unrelated edit rather than being wedged. Worth stating, since "operator-owned keys are now rejected" reads much scarier than that.

One observation that is not about this PR. shutdown-on-sigterm is on the list, and getBaseConfig emits it unconditionally with a comment saying it needs Valkey 9.0+. Valkey refuses to start on a directive it does not know, so the operator already cannot run against 8.x whatever the reserved list says. That is a data point for the support matrix in #375, and a second candidate for the gated set in #307, where tls-auto-reload-interval is currently the only entry.

On that: tls-auto-reload-interval is deliberately absent from ReservedConfigKeys, so #307 can still gate a user-set value rather than the API refusing it outright. That reads correct to me, and it is worth keeping in mind before anyone adds it here later.

[jdheyburn]

We could allow users to override this default. But I would say let's wait for the issue to come in first requesting the override so that we can understand the use case.

[jdheyburn]

Instead of listing these out twice, could we link to documentation where folks can go to find the blocked configs?

You would also need to add the documentation, which was going to be another comment I had :) Perhaps in docs/valkeycluster.md under config.

[jdheyburn]

Can you also add these, which are managed but applied directly on the pod.

cluster-announce-ip
primaryuser
primaryauth