Skip to content

feat(operator-wandb): add session key rotation - #661

Merged
zacharyblasczyk merged 7 commits into
mainfrom
agent/operator-session-key-rotation
Aug 5, 2026
Merged

feat(operator-wandb): add session key rotation#661
zacharyblasczyk merged 7 commits into
mainfrom
agent/operator-session-key-rotation

Conversation

@zacharyblasczyk

@zacharyblasczyk zacharyblasczyk commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Why

wandb/core#48640 adds support for verifying sessions with a previous Gorilla
session key. The operator chart already generates the current key, but has no
safe way to rotate it across a rolling deployment.

What changed

  • Add global.auth.sessionKey and global.auth.sessionKeyPrevious. Each value
    accepts either a literal or a Kubernetes valueFrom map.
  • Add a chart-managed prepare -> activate -> clear rotation workflow.
  • Store rotation state on the existing release Secret. Repeated Helm
    reconciliations keep the same keys.
  • Add an API/app rollout marker for every rotation phase.
  • Document the chart-managed and external Secret workflows.
  • Run the new Helm unit tests in the operator-wandb workflow.

The managed workflow generates a 32-character candidate during prepare.
activate promotes it and retains the outgoing key for verification. clear
removes verification-only keys after the overlap window. A prepared rotation
can also be cleared without activating it.

This pairs with wandb/core#48640.

Validation

  • helm unittest ./charts/operator-wandb/
  • helm lint ./charts/operator-wandb/
  • ct lint --charts charts/operator-wandb --validate-maintainers=false
  • Default and user-defined-secrets chartsnap cases

cc @cvp — could you take a look at the operator-facing rotation workflow and
values API?

Summary by CodeRabbit

  • New Features
    • Added support for managed session key rotation, including prepare, activate, clear, and emergency cutover flows.
    • Added configurable external session key overrides and rollout identifiers for safer updates.
  • Bug Fixes
    • Improved handling of session key values so both current and previous keys can be preserved during rotation.
    • Updated test coverage for session key setup, rotation behavior, and fractional session length formatting.
  • Documentation
    • Expanded guidance for session key management and rotation in the chart docs.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c79b7047e6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread charts/operator-wandb/templates/session-key.yaml Outdated
@zacharyblasczyk

Copy link
Copy Markdown
Contributor Author

@vanpelt @j7m4 The final commit (c2d665af) is ready for re-review. Required lint-test, snapshot tests, and the session-rotation test coverage are green; auto-merge is enabled. The three optional OLAP fixture failures reproduce the existing ClickHouse dirty-migration issue and are unrelated to this change.

@zacharyblasczyk

Copy link
Copy Markdown
Contributor Author

QA Google prerelease validation is complete for chart 0.43.10-PR661-6b0e1089 and Core 0.84.0-pre-rotate-session-keys.0.

The live matrix covered managed prepare/activate/clear, cancellation, accelerated clear, hard cutover, external Secret recovery, API-key continuity, current/previous internal JWT verification, real browser timeout, and all three supported app/local-service topologies. QA is restored and healthy with no temporary Secrets remaining.

One live finding: the installed Stakater Reloader restarted API immediately when the referenced external Secret changed, before sessionKeyRolloutId changed. Commit 6e686d20 corrects the runbook to describe this behavior while retaining the marker as the deterministic rollout guarantee. The follow-up is docs-only; Helm 4.2.2 lint and all 24 unit tests pass locally. CI is rerunning.

@vanpelt @j7m4 this clears the prerelease validation gate for review once required checks finish.

Copy link
Copy Markdown
Contributor Author

QA Google follow-up / correction — July 30, 2026

An external wandb-spec-user overwrite temporarily replaced the intended Core prerelease with an unrelated 0.84.0-jennwandb... image during the final cookie test. I detected the mismatch from the live pods, discarded that result, restored the intended Core tag, upgraded QA to the latest PR chart, and reran the affected path through operator reconciliation.

Final artifacts:

  • operator-wandb: 0.43.10-PR661-23a6f2f6
  • Core: 0.84.0-pre-rotate-session-keys.0

Fresh operator/chart results:

  • activate: the generated Secret and both API replicas carried current + previous keys; cookies signed by either key authenticated on both replicas.
  • clear: the generated Secret dropped GORILLA_SESSION_KEY_PREVIOUS; after reconciliation and rollout, both API replicas accepted the current-key cookie and rejected a cookie signed by the exact previous key.
  • Removing the completed rotation stanza reconciled back to normal managed mode with no previous key or stale rotation annotations.
  • sessionLengthHours: 0.1 produced the expected natural expirations; removing it restored API and Glue to 720h.
  • Existing API keys were unaffected (final 10/10), health passed 10/10, and the W&B CR finished Completed.
  • All W&B deployments are fully ready/updated on the intended Core prerelease; there are no Jenn/:latest W&B images and no remaining test Secrets, pods, cookie jars, DB sessions, or port-forwards.

The previously exercised supported topology fixtures and live modes remain passing: local-bypass without App, local-bypass with App, and App without local-bypass. The Reloader observation is now documented: a referenced Secret update may trigger an automatic rollout, while sessionKeyRolloutId remains the deterministic rollout control.

Copy link
Copy Markdown
Contributor Author

CI triage update: the remaining red checks are the three olap-features-enabled install jobs for Kubernetes 1.34.8, 1.35.5, and 1.36.1. This is repository-wide rather than specific to PR 661.

The same three jobs are currently failing on unrelated branches/runs, including:

In PR 661's run, the OLAP fixture pulls floating megabinary:latest and other :latest images; the API migration init container enters CrashLoopBackOff and ct install reaches its 600-second deadline. There is no session-key-rotation error in that failure path. Lint, prerelease packaging, snapshot tests, every non-OLAP install job, local Helm 4 lint, Helm unit tests, and the QA Google deployment all pass.

I have not added an unrelated fixture/image-pinning change to this PR.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
charts/operator-wandb/tests/session_key_rotation_test.yaml (1)

300-340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add tests for the remaining rotation validation branches.

Add failed-template cases for an ID without a phase, a phase without an ID, and an unsupported phase. These branches protect invalid rotation state before the Secret is rendered.

As per coding guidelines, “Exercise every meaningful branch in Helm templates with render or snapshot tests”.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/operator-wandb/tests/session_key_rotation_test.yaml` around lines 300
- 340, Add failed-template cases alongside the existing session key rotation
validation tests for an ID without a phase, a phase without an ID, and an
unsupported phase. Set the corresponding global.auth.sessionKeyRotation fields
and assert each branch’s exact validation error before Secret rendering.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@charts/operator-wandb/tests/session_key_rotation_test.yaml`:
- Around line 300-340: Add failed-template cases alongside the existing session
key rotation validation tests for an ID without a phase, a phase without an ID,
and an unsupported phase. Set the corresponding global.auth.sessionKeyRotation
fields and assert each branch’s exact validation error before Secret rendering.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: aa76077a-40dd-4b15-a65b-64e4c24f6ceb

📥 Commits

Reviewing files that changed from the base of the PR and between 651c688 and 79dbf06.

📒 Files selected for processing (10)
  • .github/workflows/test-operator-wandb.yaml
  • charts/operator-wandb/Chart.yaml
  • charts/operator-wandb/README.md
  • charts/operator-wandb/docs/session-key-rotation.md
  • charts/operator-wandb/templates/_session-key.tpl
  • charts/operator-wandb/templates/session-key.yaml
  • charts/operator-wandb/tests/session_key_env_test.yaml
  • charts/operator-wandb/tests/session_key_rotation_test.yaml
  • charts/operator-wandb/tests/session_length_test.yaml
  • charts/operator-wandb/values.yaml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

notify:delivery-tooling-team Notifies delivery-tooling-team of PR review request in their team channel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants