Open
Conversation
Adds a Namespace controller that provisions isolated Valkey ACL users
for each site namespace annotated with deco.sites/valkey-acl: "true".
Each site gets a K8s Secret (valkey-acl) with its own credentials,
restricted to ~{site}:* key patterns. The mutating webhook injects
the secret into site pods via envFrom (optional=true, so pods degrade
gracefully to FILE_SYSTEM cache if not yet provisioned). A bootstrap
CLI annotates existing namespaces for migration.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/valkey/client.go">
<violation number="1" location="internal/valkey/client.go:68">
P1: Missing `resetpass` before setting the password causes old passwords to accumulate. Each `UpsertUser` call *adds* the new password but never removes previous ones, so all historical passwords remain valid. This breaks password rotation and undermines tenant isolation.
Add `"resetpass"` before `">" + password` to match the reset-then-set pattern already used for keys, channels, and commands.</violation>
</file>
<file name="internal/controller/namespace_controller.go">
<violation number="1" location="internal/controller/namespace_controller.go:204">
P1: Annotation is set on the Service's top-level metadata (`svc.Annotations`) instead of the template metadata (`svc.Spec.Template.Annotations`). Knative only creates a new Revision when `spec.template` changes, so this patch will never trigger the rollout needed for pods to pick up the new `valkey-acl` Secret.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| func (c *sentinelClient) UpsertUser(ctx context.Context, username, password string) error { | ||
| args := []interface{}{ | ||
| "ACL", "SETUSER", username, | ||
| "on", |
There was a problem hiding this comment.
P1: Missing resetpass before setting the password causes old passwords to accumulate. Each UpsertUser call adds the new password but never removes previous ones, so all historical passwords remain valid. This breaks password rotation and undermines tenant isolation.
Add "resetpass" before ">" + password to match the reset-then-set pattern already used for keys, channels, and commands.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/valkey/client.go, line 68:
<comment>Missing `resetpass` before setting the password causes old passwords to accumulate. Each `UpsertUser` call *adds* the new password but never removes previous ones, so all historical passwords remain valid. This breaks password rotation and undermines tenant isolation.
Add `"resetpass"` before `">" + password` to match the reset-then-set pattern already used for keys, channels, and commands.</comment>
<file context>
@@ -0,0 +1,121 @@
+func (c *sentinelClient) UpsertUser(ctx context.Context, username, password string) error {
+ args := []interface{}{
+ "ACL", "SETUSER", username,
+ "on",
+ ">" + password,
+ "resetkeys",
</file context>
| if svc.Annotations == nil { | ||
| svc.Annotations = make(map[string]string) | ||
| } | ||
| svc.Annotations[valkeyProvisionedAnnot] = now |
There was a problem hiding this comment.
P1: Annotation is set on the Service's top-level metadata (svc.Annotations) instead of the template metadata (svc.Spec.Template.Annotations). Knative only creates a new Revision when spec.template changes, so this patch will never trigger the rollout needed for pods to pick up the new valkey-acl Secret.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/controller/namespace_controller.go, line 204:
<comment>Annotation is set on the Service's top-level metadata (`svc.Annotations`) instead of the template metadata (`svc.Spec.Template.Annotations`). Knative only creates a new Revision when `spec.template` changes, so this patch will never trigger the rollout needed for pods to pick up the new `valkey-acl` Secret.</comment>
<file context>
@@ -0,0 +1,225 @@
+ if svc.Annotations == nil {
+ svc.Annotations = make(map[string]string)
+ }
+ svc.Annotations[valkeyProvisionedAnnot] = now
+ if err := r.Patch(ctx, svc, patch); err != nil {
+ log.Error(err, "Failed to patch Knative Service", "service", svc.Name)
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NamespaceReconcilerthat watches namespaces annotated withdeco.sites/valkey-acl: "true"and provisions a Valkey ACL user + K8s Secret (valkey-acl) per site~{site}:*and~lock:{site}:*key patterns — enforces tenant isolation at the Valkey protocol levelenvFrom: secretRef: valkey-acl optional: trueinto every site pod — pods degrade gracefully to FILE_SYSTEM cache if the secret isn't provisioned yetcmd/bootstrapCLI to annotate existingsites-*namespaces for migration (--dry-runsupported)NoopClientwhenVALKEY_SENTINEL_URLSis unset — safe to deploy before Valkey auth is enabledNew env vars (operator)
VALKEY_SENTINEL_URLSVALKEY_SENTINEL_MASTER_NAMEmymasterVALKEY_ADMIN_PASSWORDValidation required before enabling
VALKEY_SENTINEL_URLSconfiguredkubectl annotate ns sites-test deco.sites/valkey-acl=truekubectl get secret valkey-acl -n sites-testACL LISTshowsuser test ...other-site:keyastestuser →NOPERMgo run ./cmd/bootstrap --dry-runDependencies
infra_applications(separate step)🤖 Generated with Claude Code
Summary by cubic
Adds per-tenant Valkey ACLs for site namespaces to enforce protocol-level key isolation. A new controller provisions an ACL user and a
valkey-aclSecret per site, and the webhook injects it into pods.New Features
deco.sites/valkey-acl: "true"and creates a Valkey ACL user restricted to~{site}:*and~lock:{site}:*, plus avalkey-aclSecret withLOADER_CACHE_REDIS_USERNAME/LOADER_CACHE_REDIS_PASSWORD.envFromforvalkey-acl(optional=true) so pods start and fall back to FILE_SYSTEM cache if the Secret isn’t ready.cmd/bootstrapCLI to annotate existingsites-*namespaces (--dry-runsupported).NoopClientwhenVALKEY_SENTINEL_URLSis unset; periodic reconcile self-heals missing ACLs after Valkey restarts.Migration
VALKEY_SENTINEL_URLS,VALKEY_ADMIN_PASSWORD, and optionallyVALKEY_SENTINEL_MASTER_NAME(defaultmymaster) for the operator.deco.sites/valkey-acl=trueor rungo run ./cmd/bootstrap --namespace-pattern sites- [--dry-run].valkey-aclexists in the namespace andACL GETUSER {site}returns the user; cross-site writes should fail withNOPERM.Written for commit e0581ef. Summary will update on new commits.