Skip to content

Address Redis Cluster mode review follow-ups for #5153#5210

Open
reyortiz3 wants to merge 3 commits intomainfrom
worktree-followup-redis-cluster-mode
Open

Address Redis Cluster mode review follow-ups for #5153#5210
reyortiz3 wants to merge 3 commits intomainfrom
worktree-followup-redis-cluster-mode

Conversation

@reyortiz3
Copy link
Copy Markdown
Contributor

Summary

Three small follow-ups from the PR #5153 review that were deferred so the core cluster-mode support could land. None change runtime behaviour beyond the defensive filter in #2 (which only triggers on stray index members).

  • Document the cluster slot invariant for storeUpstreamTokensScript (pkg/authserver/storage/redis.go). The script reads oldUserID from KEYS[1] inside its body to keep the user-set bookkeeping atomic, so the user-set keys are constructed dynamically from ARGV[4] rather than declared as KEYS. The new comment makes the {ns:name} hash-tag requirement explicit, so a future refactor that rebuilds the prefix without the tag will be caught in review rather than silently regressing on standalone Redis and exploding with CROSSSLOT on a real cluster. (Reviewer offered two options — KEYS[3]/KEYS[4] or this comment; the KEYS approach would require resolving oldUserSetKey outside the script, which would break the atomic GET-of-old-userID the script exists to provide.)
  • Filter SMEMBERS results in GetAllUpstreamTokens, DeleteUpstreamTokens, and GetLatestUpstreamTokensForUser to entries that share the storage instance's keyPrefix, warn-logging anything dropped. A stray un-prefixed member (legacy data, an external admin op, a test fixture) would today surface as CROSSSLOT under cluster while passing on standalone; this turns it into a logged warning. The defensive filter is a pure helper with table-driven tests, plus a behaviour test that captures slog output to prove the wiring at all three call sites.
  • Align the operator CRD tls doc with the storage-layer comment so that crd-api.md and kubectl explain both describe TLS as applying to "Redis/Valkey master or cluster nodes" — previously cluster-mode users reading the CRD field would not realise this same field configured TLS for cluster-node connections. Regenerated CRD YAML, Helm templates, and crd-api.md to match.

Refs #5153.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)

New unit tests:

  • TestFilterIndexMembersByPrefix — pure helper, table-driven (parallel).
  • TestForeignMembersFilteredFromIndexOps — non-parallel, captures slog default and asserts each of the three operations emits a warn naming the operation, dropped member, and expected prefix.

API Compatibility

  • This PR does not break the v1beta1 API.

The CRD field RedisStorageConfig.TLS itself is unchanged — only the godoc string is updated. Generated CRD YAML differs only in the description: field of the same property.

Changes

File Change
pkg/authserver/storage/redis.go Add filterIndexMembersByPrefix + warnDroppedIndexMembers helpers; wire into GetAllUpstreamTokens, DeleteUpstreamTokens, GetLatestUpstreamTokensForUser; document cluster slot invariant on storeUpstreamTokensScript
pkg/authserver/storage/redis_test.go Add table-driven helper tests + slog-capturing behaviour test for the three filter call sites
cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go Update CRD TLS field doc to "master or cluster nodes"
Regenerated CRD YAML × 4, Helm templates, crd-api.md Doc string change propagated

Does this introduce a user-facing change?

Operators will see the updated TLS field description in kubectl explain and the published CRD reference. The defensive SMEMBERS filter is silent on a healthy keyspace; it only surfaces a WARN log if an index set somehow contains a foreign-prefix entry.

Generated with Claude Code

Three small follow-ups from the PR #5153 review that were deferred so the
core cluster-mode support could land:

- Document the cluster slot invariant for storeUpstreamTokensScript. The
  script reads oldUserID from KEYS[1] inside its body to keep the user-set
  bookkeeping atomic, so the user-set keys are constructed dynamically from
  ARGV[4] rather than declared as KEYS. The new comment makes the
  {ns:name} hash-tag requirement explicit, so a future refactor that
  rebuilds the prefix without the tag will be caught in review rather than
  silently regressing on standalone Redis and exploding with CROSSSLOT on
  a real cluster.

- Filter SMEMBERS results in GetAllUpstreamTokens, DeleteUpstreamTokens,
  and GetLatestUpstreamTokensForUser to entries that share the storage
  instance's keyPrefix, warn-logging anything dropped. A stray un-prefixed
  member (legacy data, an external admin op, a test fixture) would today
  surface as CROSSSLOT under cluster while passing on standalone; this
  turns it into a logged warning. The defensive filter is a pure helper
  with table-driven tests, plus a behavior test that captures slog output
  to prove the wiring at all three call sites.

- Align the operator CRD `tls` doc with the storage-layer comment so that
  crd-api.md and `kubectl explain` both describe TLS as applying to
  "Redis/Valkey master or cluster nodes" — previously cluster-mode users
  reading the CRD field would not realise this same field configured TLS
  for cluster-node connections. Regenerated CRD YAML, Helm templates, and
  crd-api.md to match.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label May 6, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.85%. Comparing base (6b39256) to head (cc54275).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5210      +/-   ##
==========================================
+ Coverage   67.83%   67.85%   +0.01%     
==========================================
  Files         610      610              
  Lines       62303    62321      +18     
==========================================
+ Hits        42262    42286      +24     
+ Misses      16875    16865      -10     
- Partials     3166     3170       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels May 7, 2026
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels May 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant