fix(metrics-generator): prevent WAL deletion when tenant is empty - #5586
Conversation
|
|
| // TODO move this to the generator.go | ||
|
|
||
| // Validate empty tenant to prevent WAL directory deletion | ||
| if tenant == "" { |
There was a problem hiding this comment.
i like this fix b/c of how catastrophic the result is if it occurs, but technically this scenario should never happen.
wdyt about just returning an error instead of using "single-tenant"?
There was a problem hiding this comment.
@joe-elliott
I agree thx
Finally, I used RemoteWriteAddOrgIDHeader as the decision flag because it already maps directly to the deployment mode:
- true (multi-tenant) → tenant ID is required for isolation, so we throw an error if it’s missing
- false (single-tenant) → no isolation needed, so we allow empty tenant IDs to pass through
This way we don’t introduce new config knobs or break existing single-tenant flows.
The validation logic stays aligned with the original flag’s semantics, making the behavior both clear and consistent.
There was a problem hiding this comment.
RemoteWriteOrgIDHeader doesn't indicate whether or not Tempo is in multitenant mode. Honestly an empty tenant here is always an error whether it's running as single or multi tenant. I'd just change the code to always return an error if tenant is an empty string.
There was a problem hiding this comment.
@joe-elliott I understood I changed my code always return error when tenant comes empty string
joe-elliott
left a comment
joe-elliott
left a comment
There was a problem hiding this comment.
nice find and thx for the contribution!
…afana#5586) * fix(metrics-generator): prevent WAL deletion when tenant is empty * fix(metrics-generator): updateCHANGELOG.md * fix : empty tenant should return error in multi-tenant * fix : empty tenant should return error in multi-tenant * fix : change condition from test --------- Co-authored-by: backend-sungyoon <kim.sungyoon@cashwalk.io>
4 participants
What this PR does:
Prevents metrics-generator from deleting the entire WAL root directory when an empty tenant is encountered. This issue could cause data loss for all tenants
Root Cause:
• Empty tenant ("") causes filepath.Join(cfg.Path, tenant) to resolve to the WAL root directory
• os.RemoveAll(walDir) then deletes the entire root, affecting all tenant WAL data
• This occurs when X-Scope-OrgID header is missing or internal metrics lack tenant labels
Solution:
• Throw Error when tenant is empty when multi tenancy environment enabled
Testing:
• Added TestInstance_emptyTenantValidation with 2 test cases
• All existing tests pass
• Verified WAL directory structure remains intact for existing tenants
Which issue(s) this PR fixes:
Fixes potential WAL data loss in multi-tenant environments (no existing issue number)
Checklist