Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

really skip label check by ignoring incoming labels #3

Merged
merged 2 commits into from
Jul 8, 2022

Conversation

replay
Copy link

@replay replay commented Jul 7, 2022

currently the flag SkipInboundLabelCheck doesn't really make memberlist skip the label check, because if this flag is enabled and the memberlist client is configured with no label and then it receives a message that has a label then prints a warning and drops the received message. I think if SkipInboundLabelCheck is enabled then it shouldn't matter whether incoming messages have a label, at all.

@replay replay marked this pull request as draft July 7, 2022 19:43
@replay replay changed the base branch from gem-version-update to gem-version July 7, 2022 19:59
@56quarters
Copy link

Am I correct that this change is only required during the migration of a cluster that doesn't use to labels to start using them? If so should we plan to upstream this at all or just add it to our fork temporarily? I suppose this is useful for more than just Mimir so perhaps upstreaming makes sense.

@replay
Copy link
Author

replay commented Jul 8, 2022

Am I correct that this change is only required during the migration of a cluster that doesn't use to labels to start using them?

Correct

If so should we plan to upstream this at all or just add it to our fork temporarily? I suppose this is useful for more than just Mimir so perhaps upstreaming makes sense.

I think upstreaming it makes sense, because as you said it could be useful to more people. I want to test it first in a real-world scenario before upstreaming though

@replay replay marked this pull request as ready for review July 8, 2022 00:01
@pracucci pracucci self-requested a review July 8, 2022 09:11
@pracucci
Copy link

pracucci commented Jul 8, 2022

I checked the original PR hashicorp#246 to see if there was any discussion about SkipInboundLabelCheck, cause it looks weird the way it was implemented. I'm wondering if it's us missing something.

Copy link

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I keep not understanding why it was implemented that way in the original PR, but after double checking the code I think this change is good.

@slim-bean
Copy link

slim-bean commented Jul 8, 2022

I keep not understanding why it was implemented that way in the original PR, but after double checking the code I think this change is good.

I also looked at the initial PR and I'm not sure why it was implemented the way it is, I suspect it was an oversight because the way it's implemented in main doesn't seem like it really serves any useful purpose:

If the flag is true: any packet with a label will fail
If the flag is false: any packet with a label will fail

¯\_(ツ)_/¯

@pracucci
Copy link

pracucci commented Jul 8, 2022

4 people looking at it, and looks good, so let's merge it.

@pracucci pracucci merged commit bd88e10 into gem-version Jul 8, 2022
@pracucci pracucci deleted the really_skip_label_check branch July 8, 2022 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants