Skip to content

receive: handle empty tenant_matcher_type as exact match in shuffle sharding - #8974

Open
bisshwajitsamanta wants to merge 8 commits into
thanos-io:mainfrom
bisshwajitsamanta:fix/shuffle-sharding-empty-matcher
Open

receive: handle empty tenant_matcher_type as exact match in shuffle sharding#8974
bisshwajitsamanta wants to merge 8 commits into
thanos-io:mainfrom
bisshwajitsamanta:fix/shuffle-sharding-empty-matcher

Conversation

@bisshwajitsamanta

Copy link
Copy Markdown

Description ⚙️

When tenant_matcher_type is omitted in a shuffle sharding override configuration, Go's YAML parser initializes the struct field as "" (empty string) due to the omitempty tag. Previously, getShardSize() only checked for TenantMatcherTypeExact ("exact"), causing overrides with an omitted matcher type to be silently skipped and fall back to the default shard size.

This PR updates the switch evaluation in getShardSize() to match both TenantMatcherTypeExact and "", restoring the expected default fallback behavior.

Special Notes for Reviewer 🧪

  • Added "" to the exact match case statement in receive/hashring.go.
  • Updated test cases in receive/hashring_test.go to verify that overrides apply correctly when tenant_matcher_type is omitted.

Signed-off-by: Bisshwajit Samanta <bisshwajitsamanta@gmail.com>
@bisshwajitsamanta
bisshwajitsamanta force-pushed the fix/shuffle-sharding-empty-matcher branch from 7555390 to 924b772 Compare August 11, 2026 10:47
@bisshwajitsamanta

Copy link
Copy Markdown
Author

"Note for reviewers: The CI failure in TestReceive (panic: sync: negative WaitGroup counter in handler.go) appears to be a pre-existing race condition on main related to the recent non-blocking fast-path additions. It is unrelated to the hashring.go configuration fix in this PR. Let me know if you'd like me to rebase once main is stabilized!"

@GiedriusS GiedriusS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When can this be empty? I think instead the focus should be on receive.ParseConfig. Perhaps tenantMatcher needs a custom UnmarshalJSON that errors on all values except the ones allowed. You can try writing a test for that.

@bisshwajitsamanta

bisshwajitsamanta commented Aug 12, 2026

Copy link
Copy Markdown
Author

Thanks for the feedback!

To answer your question about when it can be empty: tenant_matcher_type is an optional field in the hashring configuration (json:"tenant_matcher_type,omitempty"). When users leave it out of their configuration file, Go unmarshals it as an empty string "", which Thanos defaults to "exact" matching behavior.

That said, validating early in ParseConfig via a custom UnmarshalJSON on tenantMatcher makes total sense—it's a much cleaner way to fail fast on invalid matchers while still accepting "exact", "glob", and "".

I'm working on implementing the custom unmarshaler and adding the corresponding unit tests soon!

- Add custom UnmarshalJSON for tenantMatcher to fail fast on invalid types and explicit empty strings
- Hydrate omitted tenant_matcher_type fields to 'exact' in ParseConfig
- Add table-driven tests for tenantMatcher unmarshaling

Signed-off-by: Bisshwajit Samanta <bisshwajitsamanta@gmail.com>
@bisshwajitsamanta
bisshwajitsamanta force-pushed the fix/shuffle-sharding-empty-matcher branch from 293e58f to 92b53a3 Compare August 13, 2026 05:40
@bisshwajitsamanta

bisshwajitsamanta commented Aug 13, 2026

Copy link
Copy Markdown
Author

Summary of Changes

  1. Parse-Time Governance (UnmarshalJSON): Added custom JSON unmarshaling for tenantMatcher to act as a strict firewall. It explicitly accepts "exact" and "glob". Any invalid string or explicit empty string ("") fails fast during configuration loading rather than causing subtle runtime errors or silent fallbacks later.
  2. Post-Parsing Normalization (ParseConfig): Added a hydration pass after unmarshaling to handle completely omitted fields. If tenant_matcher_type is missing from the configuration payload, Go leaves it as a zero-value (""); our normalization pass automatically hydrates it to TenantMatcherTypeExact ("exact").
  3. Runtime Clean-Up (getShardSize): Removed the redundant case "" check from getShardSize. Since the configuration layer now guarantees all inputs are normalized upon loading, the routing layer relies strictly on explicitly defined types (TenantMatcherTypeExact and TenantMatcherGlob).
  4. Unit Tests: Added table-driven tests for tenantMatcher.UnmarshalJSON covering valid matchers, invalid inputs, and explicitly rejected empty strings. (Note: Dedicated unit tests for the ParseConfig normalization loop will be added in a follow-up commit shortly).
  5. Documentation: Updated configuration documentation to clarify permitted values (exact, glob) and default omitted behavior.

Verification Output

go test -v -run "^(TestTenantMatcher_UnmarshalJSON|TestShuffleShardHashring)$" ./pkg/receive
=== RUN   TestShuffleShardHashring
=== PAUSE TestShuffleShardHashring
=== RUN   TestTenantMatcher_UnmarshalJSON
=== PAUSE TestTenantMatcher_UnmarshalJSON
=== CONT  TestShuffleShardHashring
=== CONT  TestTenantMatcher_UnmarshalJSON
=== RUN   TestShuffleShardHashring/ketama_with_shuffle_sharding
=== RUN   TestTenantMatcher_UnmarshalJSON/Invalid_garbage_matcher
=== PAUSE TestTenantMatcher_UnmarshalJSON/Invalid_garbage_matcher
=== RUN   TestTenantMatcher_UnmarshalJSON/Valid_exact_matcher
=== PAUSE TestTenantMatcher_UnmarshalJSON/Valid_exact_matcher
=== RUN   TestTenantMatcher_UnmarshalJSON/Valid_glob_matcher
=== PAUSE TestTenantMatcher_UnmarshalJSON/Valid_glob_matcher
=== RUN   TestTenantMatcher_UnmarshalJSON/Invalid_empty_matcher_(explicitly_rejected)
=== PAUSE TestTenantMatcher_UnmarshalJSON/Invalid_empty_matcher_(explicitly_rejected)
=== CONT  TestTenantMatcher_UnmarshalJSON/Invalid_garbage_matcher
=== CONT  TestTenantMatcher_UnmarshalJSON/Valid_glob_matcher
=== CONT  TestTenantMatcher_UnmarshalJSON/Valid_exact_matcher
=== CONT  TestTenantMatcher_UnmarshalJSON/Invalid_empty_matcher_(explicitly_rejected)
--- PASS: TestTenantMatcher_UnmarshalJSON (0.00s)
    --- PASS: TestTenantMatcher_UnmarshalJSON/Invalid_garbage_matcher (0.00s)
    --- PASS: TestTenantMatcher_UnmarshalJSON/Valid_glob_matcher (0.00s)
    --- PASS: TestTenantMatcher_UnmarshalJSON/Valid_exact_matcher (0.00s)
    --- PASS: TestTenantMatcher_UnmarshalJSON/Invalid_empty_matcher_(explicitly_rejected) (0.00s)
=== RUN   TestShuffleShardHashring/ketama_with_glob_tenant_override
=== RUN   TestShuffleShardHashring/big_shard_size
=== RUN   TestShuffleShardHashring/zone_awareness_disabled
--- PASS: TestShuffleShardHashring (0.02s)
    --- PASS: TestShuffleShardHashring/ketama_with_shuffle_sharding (0.01s)
    --- PASS: TestShuffleShardHashring/ketama_with_glob_tenant_override (0.00s)
    --- PASS: TestShuffleShardHashring/big_shard_size (0.00s)
    --- PASS: TestShuffleShardHashring/zone_awareness_disabled (0.01s)
PASS
ok      github.com/thanos-io/thanos/pkg/receive 1.100s

@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.53%. Comparing base (d567cbc) to head (2cc33df).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/receive/config.go 90.47% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8974      +/-   ##
==========================================
+ Coverage   64.50%   64.53%   +0.02%     
==========================================
  Files         289      289              
  Lines       37352    37372      +20     
==========================================
+ Hits        24094    24117      +23     
+ Misses      11160    11156       -4     
- Partials     2098     2099       +1     

☔ View full report in Codecov by Harness.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pull-request-size pull-request-size Bot added size/L and removed size/M labels Aug 13, 2026
@bisshwajitsamanta
bisshwajitsamanta force-pushed the fix/shuffle-sharding-empty-matcher branch 2 times, most recently from dc180cc to c68e281 Compare August 13, 2026 13:53
@bisshwajitsamanta
bisshwajitsamanta force-pushed the fix/shuffle-sharding-empty-matcher branch from 97975ab to 9b79b8e Compare August 13, 2026 14:35
- Remove redundant empty string case from getShardSize since ParseConfig hydrates omitted fields to exact
- Rely strictly on TenantMatcherTypeExact and TenantMatcherGlob at runtime

Signed-off-by: Bisshwajit Samanta <bisshwajitsamanta@gmail.com>
Signed-off-by: Bisshwajit Samanta <bisshwajitsamanta@gmail.com>
Signed-off-by: Bisshwajit Samanta <bisshwajitsamanta@gmail.com>
Signed-off-by: Bisshwajit Samanta <bisshwajitsamanta@gmail.com>
@bisshwajitsamanta
bisshwajitsamanta force-pushed the fix/shuffle-sharding-empty-matcher branch from 9b79b8e to 3d7a99f Compare August 13, 2026 14:55
@bisshwajitsamanta

Copy link
Copy Markdown
Author

Hi @GiedriusS ,

I've completed the implementation and updated the test suite. All CI checks (including DCO sign-off) are now green.

📌 Summary of Changes:

  1. Parse-Time Normalization (ParseConfig): Omitted tenant_matcher_type fields (at both top-level hashring and nested override scopes) now hydrate to "exact" during configuration loading. This enforces a single source of truth (TenantMatcherTypeExact) across the system lifecycle.
  2. Strict Ingestion Validation (UnmarshalJSON): Custom unmarshaling strictly validates input matcher strings. Explicitly provided empty strings ("") or unrecognized matchers are rejected immediately at parse time to fail fast against configuration errors or template bugs.
  3. Test Coverage & Docs: Updated TestParseConfig and TestTenantMatcher_UnmarshalJSON to thoroughly cover default hydration vs. strict validation boundaries, complete with inline doc comments describing the invariants.

The PR is ready for final review and approval! 🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants