Fix StringSliceCSV treating explicit empty string as a one-element list#7714
Open
pujitha24 wants to merge 1 commit into
Open
Fix StringSliceCSV treating explicit empty string as a one-element list#7714pujitha24 wants to merge 1 commit into
pujitha24 wants to merge 1 commit into
Conversation
Motivation: Issue cortexproject#6581 reports that setting `compactor.enabled_tenants: ""` explicitly in config causes the compactor to skip every tenant, instead of behaving like the unset/omitted case (all tenants allowed). The compactor logs "skipping user because it is not owned by this shard" for every real tenant. Approach: `StringSliceCSV.Set` (pkg/util/flagext/stringslicecsv.go), which backs `-compactor.enabled-tenants`/`-compactor.disabled-tenants` and several other CSV-list flags (alertmanager, ruler, store-gateway, ingester, querier, distributor, ring, limits, etc.), implemented `Set` as `strings.Split(s, ",")`. Go's `strings.Split("", ",")` returns `[""]`, a one-element slice containing an empty string, not an empty/nil slice. YAML unmarshaling for `enabled_tenants: ""` calls `Set("")`, so `EnabledTenants` ends up as `[""]` with `len() == 1`. `users.NewAllowedTenants` (pkg/util/users/allowed_tenants.go) treats `len(enabled) > 0` as "only tenants in this list are allowed", so with `enabled == [""]` only a tenant literally named `""` is allowed and every real tenant is rejected. This same `NewAllowedTenants` helper backs the identical enabled/disabled tenant list pattern in the compactor, alertmanager, ruler, and store-gateway, so all four are affected identically by explicitly empty tenant-list config, though issue cortexproject#6581 only reports it for the compactor. Fix `Set` to special-case an empty input string by setting the value to nil, matching the sibling `SecretStringSliceCSV.Set`, which already special-cases `s == ""` this same way. Validation: go build ./... go test ./pkg/util/flagext/... ./pkg/compactor/... ./pkg/util/validation/... ./pkg/util/users/... -tags "netgo slicelabels" All pass, including new tests in pkg/util/flagext/stringslicecsv_test.go covering direct `Set("")` and YAML unmarshaling of `csv: ""`. Before/after: with `enabled_tenants: ""`, no tenant was ever compacted by any compactor replica (every tenant matched "not owned by this shard"/was filtered out before ownership was even checked). After this fix, an explicitly empty `enabled_tenants` (or `disabled_tenants`) behaves identically to the field being omitted: all tenants are eligible, subject to normal ring sharding. Fixes cortexproject#6581 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request fixes a configuration edge case where CSV-backed string-slice flags/YAML fields (notably compactor.enabled_tenants / -compactor.enabled-tenants) treated an explicitly empty string ("") as a one-element slice containing "", which inadvertently enabled allow-list mode for a nonexistent empty tenant and caused all real tenants to be skipped.
Changes:
- Update
StringSliceCSV.Setto special-cases == ""by resetting the slice tonilinstead of usingstrings.Split, aligning behavior with “unset/omitted” semantics. - Add unit tests covering both direct
Set("")behavior and YAML unmarshalling ofcsv: "". - Add a CHANGELOG bugfix entry documenting the user-facing impact and resolution.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/util/flagext/stringslicecsv.go | Treat empty input string as an empty/unset list by setting the slice to nil before returning. |
| pkg/util/flagext/stringslicecsv_test.go | Add tests to ensure empty-string inputs don’t produce [""] and YAML "" unmarshals to an empty list. |
| CHANGELOG.md | Document the bugfix and its impact on enabled/disabled tenant CSV lists. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Motivation:
Issue #6581 reports that setting
compactor.enabled_tenants: ""explicitly in config causes the compactor to skip every tenant,
instead of behaving like the unset/omitted case (all tenants
allowed). The compactor logs "skipping user because it is not owned
by this shard" for every real tenant.
Approach:
StringSliceCSV.Set(pkg/util/flagext/stringslicecsv.go), whichbacks
-compactor.enabled-tenants/-compactor.disabled-tenantsandseveral other CSV-list flags (alertmanager, ruler, store-gateway,
ingester, querier, distributor, ring, limits, etc.), implemented
Setasstrings.Split(s, ","). Go'sstrings.Split("", ",")returns
[""], a one-element slice containing an empty string, notan empty/nil slice. YAML unmarshaling for
enabled_tenants: ""calls
Set(""), soEnabledTenantsends up as[""]withlen() == 1.users.NewAllowedTenants(pkg/util/users/allowed_tenants.go) treatslen(enabled) > 0as "only tenants in this list are allowed", sowith
enabled == [""]only a tenant literally named""isallowed and every real tenant is rejected. This same
NewAllowedTenantshelper backs the identical enabled/disabledtenant list pattern in the compactor, alertmanager, ruler, and
store-gateway, so all four are affected identically by explicitly
empty tenant-list config, though issue #6581 only reports it for the
compactor.
Fix
Setto special-case an empty input string by setting the valueto nil, matching the sibling
SecretStringSliceCSV.Set, whichalready special-cases
s == ""this same way.Validation:
go build ./...
go test ./pkg/util/flagext/... ./pkg/compactor/... ./pkg/util/validation/... ./pkg/util/users/... -tags "netgo slicelabels"
All pass, including new tests in pkg/util/flagext/stringslicecsv_test.go
covering direct
Set("")and YAML unmarshaling ofcsv: "".Before/after: with
enabled_tenants: "", no tenant was evercompacted by any compactor replica (every tenant matched
"not owned by this shard"/was filtered out before ownership was even
checked). After this fix, an explicitly empty
enabled_tenants(ordisabled_tenants) behaves identically to the field being omitted:all tenants are eligible, subject to normal ring sharding.
Fixes #6581
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com