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

querier: Add validation for querier address flags #4897

Merged
merged 4 commits into from
Nov 25, 2021

Conversation

saswatamcode
Copy link
Member

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

This PR adds validation for querier address flags.

Adds the validateAddrs() function which checks for duplicates, empty or invalid addresses provided using --endpoint, --store, --rule, --metadata, --exemplar or --target flags.

Fixes #4894.

Verification

Tested locally.

Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
}

set[s] = struct{}{}
Copy link
Member

@GiedriusS GiedriusS Nov 24, 2021

Choose a reason for hiding this comment

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

Maybe we can also call https://pkg.go.dev/net#ParseIP here to check hostAndPort[0]? scratch that, it is probably tricky to check whether we were given a hostname or if we were given an IP address

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

I was thinking that maybe instead of using Strings() on these flags, we could define a custom parser (https://github.com/alecthomas/kingpin#custom-parsers) that automatically calls validateAddrs? That way we wouldn't miss calling these functions on the provided values. What do you think? 🤔

@saswatamcode
Copy link
Member Author

This would be better! Let me try it out and push a commit. 🙂

Signed-off-by: Saswata Mukherjee <[email protected]>
@saswatamcode
Copy link
Member Author

I've changed this to custom kingpin parser which is defined in extkingpin/flags.go now. 🙂

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

👍

@GiedriusS GiedriusS enabled auto-merge (squash) November 25, 2021 13:16
@GiedriusS GiedriusS merged commit 562c2a5 into thanos-io:main Nov 25, 2021
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.

Querier: Endpoint (store) flag should ignore empty or obviously incorrect addresses
2 participants