Memberlist: prevent accidental cross-cluster joins with cluster labels#7385
Open
sandy2008 wants to merge 4 commits intocortexproject:masterfrom
Open
Memberlist: prevent accidental cross-cluster joins with cluster labels#7385sandy2008 wants to merge 4 commits intocortexproject:masterfrom
sandy2008 wants to merge 4 commits intocortexproject:masterfrom
Conversation
Signed-off-by: Sandy Chen <Yuxuan.Chen@morganstanley.com>
Signed-off-by: Sandy Chen <Yuxuan.Chen@morganstanley.com>
CharlieTLe
reviewed
Mar 30, 2026
Member
CharlieTLe
left a comment
There was a problem hiding this comment.
Overall Review
This is a solid, well-tested feature PR. The core implementation is minimal and correct — it properly wires Cortex config to the already-supported memberlist library fields. The test coverage (unit + integration) and documentation updates are thorough.
Main actionable items:
- Fix CHANGELOG category —
[BUGFIX]→[FEATURE]or[ENHANCEMENT](see inline comment) - Reconsider the log line change — the original per-
RandomizeNodeNamelog was removed and replaced with an unconditional one; keep original behavior or document the change (see inline comment) - Clean up the polling loop in the test helper (see inline comment)
- Minor nits on struct field ordering, test matrix coverage, and migration guide doc change noted inline
Security note: Cluster labels are transmitted in plaintext over the gossip protocol — they are not a security boundary, only an operational safety net. Consider adding a note in the docs that this is not a security mechanism; an attacker on the network could still join by matching the label.
…, clean up polling loop, add test cases, and strengthen integration assertion Signed-off-by: Sandy Chen <Yuxuan.Chen@morganstanley.com>
Contributor
Author
|
Thanks for the thorough review @CharlieTLe! All feedback addressed in the latest push:
|
Signed-off-by: Sandy Chen <Yuxuan.Chen@morganstanley.com>
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.
Summary
This fixes a Cortex gap in the memberlist integration: the vendored memberlist library already supports cluster labels, but Cortex did not expose that functionality in its own config. As a result, separate deployments that could reach the same gossip seed addresses could accidentally join the same memberlist cluster.
This change:
memberlist.cluster-labelandmemberlist.cluster-label-verification-disabledin Cortex and wires them into memberlistNote: The doc change in
docs/guides/migration-kv-store-to-memberlist.mdalso fixes a pre-existing typo (abort_if_join_failstoabort_if_cluster_join_fails), which is unrelated to the cluster label feature itself.Security Note
Cluster labels are transmitted in plaintext over the gossip protocol. They are not a security boundary, only an operational safety net. An attacker on the network could still join by matching the label. This is documented in the migration guide.
Why
Without this, Cortex operators have no built-in way to prevent accidental cross-cluster memberlist merges caused by shared seed addresses or cross-namespace discovery. This is the same class of problem described in Mimir issue #6187.
Verification
go test ./pkg/ring/kv/memberlist -run "Test(BuildMemberlistConfigClusterLabelOptions|MultipleClients)"go test -v -tags=integration,integration_memberlist -run "TestSingleBinaryWithMemberlistClusterLabel(Isolation|MigrationPhases)$" -count=1 ./integration/...