feat(seaweedfs): expose JWT expires_after_seconds via values - #2911
feat(seaweedfs): expose JWT expires_after_seconds via values#2911IvanHunters wants to merge 1 commit into
expires_after_seconds via values#2911Conversation
SeaweedFS scaffold sets a default JWT TTL of 10 seconds, hardcoded in
the security.toml template — no values override available. Under heavy
load (e.g. large image blob uploads via Harbor's S3 backend) volume
servers report `token has invalid claims: token is expired` and the
write fails.
Add an optional knob:
global:
seaweedfs:
securityConfig:
jwtSigning:
expiresAfterSeconds:
volumeWrite: 3600
volumeRead: 3600
filerWrite: 3600
filerRead: 3600
When the value is 0 (default) the field is omitted, preserving the
existing SeaweedFS default of 10 seconds. Otherwise the value is
emitted as `expires_after_seconds = N` under the matching jwt section.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
📝 WalkthroughWalkthroughThis pull request adds configurable JWT expiration times to the SeaweedFS Helm chart. The chart values define per-operation lifetime settings (volume write/read, filer write/read), and the security ConfigMap template conditionally renders expiration parameters in JWT sections based on these values, falling back to defaults when set to 0. ChangesJWT Expiration Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/system/seaweedfs/charts/seaweedfs/templates/shared/security-configmap.yaml (1)
1-119:⚠️ Potential issue | 🟠 MajorAlign SeaweedFS vendoring workflow for
security-configmapchanges
packages/system/seaweedfs/Makefile’supdatetarget wipescharts/, re-downloads the upstream SeaweedFS chart, then re-applies onlypatches/resize-api-server-annotation.diffandpatches/disable-ca-key-rotation.patch. The change inpackages/system/seaweedfs/charts/seaweedfs/templates/shared/security-configmap.yamlwill be lost on the nextmake updateunless it’s already present upstream or added to thepatches/set and applied inupdate.
- Should this be upstreamed to
github.com/seaweedfs/seaweedfsfirst, then re-vendored?- If not upstreamed, add a new patch for this template/config and apply it in
make update.🤖 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 `@packages/system/seaweedfs/charts/seaweedfs/templates/shared/security-configmap.yaml` around lines 1 - 119, The change to templates/shared/security-configmap.yaml will be lost by the packages/system/seaweedfs Makefile's update target because it re-downloads charts and reapplies only existing patches (patches/resize-api-server-annotation.diff and patches/disable-ca-key-rotation.patch); fix by either (A) upstreaming the template change to github.com/seaweedfs/seaweedfs and then re-running the vendoring to pick it up, or (B) add a new patch file (e.g., patches/security-configmap.patch) that contains the diff for templates/shared/security-configmap.yaml and update the Makefile's update flow to apply that patch alongside the existing ones so the change survives make update; locate references via security-configmap.yaml, the Makefile update target, and the patches/ directory to implement the chosen approach.Source: Learnings
🤖 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.
Outside diff comments:
In
`@packages/system/seaweedfs/charts/seaweedfs/templates/shared/security-configmap.yaml`:
- Around line 1-119: The change to templates/shared/security-configmap.yaml will
be lost by the packages/system/seaweedfs Makefile's update target because it
re-downloads charts and reapplies only existing patches
(patches/resize-api-server-annotation.diff and
patches/disable-ca-key-rotation.patch); fix by either (A) upstreaming the
template change to github.com/seaweedfs/seaweedfs and then re-running the
vendoring to pick it up, or (B) add a new patch file (e.g.,
patches/security-configmap.patch) that contains the diff for
templates/shared/security-configmap.yaml and update the Makefile's update flow
to apply that patch alongside the existing ones so the change survives make
update; locate references via security-configmap.yaml, the Makefile update
target, and the patches/ directory to implement the chosen approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0c0dc09-82fb-4f05-9756-3b77e768e763
📒 Files selected for processing (2)
packages/system/seaweedfs/charts/seaweedfs/templates/shared/security-configmap.yamlpackages/system/seaweedfs/charts/seaweedfs/values.yaml
|
Closing in favour of upstreaming this. The patch as written modifies files under Filed upstream as seaweedfs/seaweedfs#9937 asking for the values-driven If the upstream change stalls, a follow-up PR here will reintroduce this as a proper entry under |
Pulls in upstream SeaweedFS 4.34, which includes: - SIGHUP-based hot-reload of JWT signing keys (seaweedfs/seaweedfs#9826, fixing seaweedfs/seaweedfs#9823). Operators recovering from key rotation or mismatch no longer need to restart 'weed' processes — kill -HUP is enough. - Native 'expires_after_seconds' values for jwt.signing / jwt.signing.read / jwt.filer_signing / jwt.filer_signing.read, exposed under global.seaweedfs.securityConfig.jwtSigning.expiresAfterSeconds. This supersedes the patch in the now-closed cozystack#2911. 'make update' driven; patches in 'patches/' reapplied cleanly. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Pulls in upstream SeaweedFS 4.34, which includes: - SIGHUP-based hot-reload of JWT signing keys (seaweedfs/seaweedfs#9826, fixing seaweedfs/seaweedfs#9823). Operators recovering from key rotation or mismatch no longer need to restart 'weed' processes — kill -HUP is enough. - Native 'expires_after_seconds' values for jwt.signing / jwt.signing.read / jwt.filer_signing / jwt.filer_signing.read, exposed under global.seaweedfs.securityConfig.jwtSigning.expiresAfterSeconds. This supersedes the patch in the now-closed cozystack#2911. 'make update' driven; patches in 'patches/' reapplied cleanly. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Summary
The SeaweedFS upstream default JWT TTL is 10 seconds. The current Cozystack chart hardcodes the absence of
expires_after_secondsinsecurity.toml, so operators get the 10-second behaviour with no way to override.For larger backends — most visibly Harbor's S3 backend (the
s3awsregistry driver against SeaweedFS' S3 frontend) — this default is too short. Blob uploads that take longer than ~10 seconds, or volume → master heartbeat / inter-process traffic on a slow link, end up seeingtoken has invalid claims: token is expiredand the write fails. Symptom from Harbor's perspective:The corresponding SeaweedFS-side error in volume / s3 logs is
wrong jwtwith reasontoken is expired. Restarting the volume server forces re-issuing of tokens (with the same 10-second TTL), so the situation recurs.Change
Add an optional
expiresAfterSecondsblock underglobal.seaweedfs.securityConfig.jwtSigning:When the value is
0(the default) theexpires_after_secondsline is omitted fromsecurity.toml, preserving SeaweedFS' built-in 10-second default for backwards compatibility. When set to a positive integer, the value is emitted under the matching[jwt.*]section.Test plan
helm templatewith default values — noexpires_after_secondsline emitted (current behaviour preserved).helm templatewithexpiresAfterSeconds.volumeWrite: 3600—expires_after_seconds = 3600appears under[jwt.signing], nothing under other sections.3600, restart master + volume pods, confirm large Harbor uploads succeed andtoken is expirederrors stop appearing in volume logs.Reference
SeaweedFS source:
weed/security/jwt.goreadsexpiresAfterSecand only sets theexpclaim when value is positive. Scaffold (security.toml) documents the 10-second default but doesn't include a way to override via Helm-style config — that's what this PR adds at the Cozystack chart level.Summary by CodeRabbit