receive: fix relabel config NameValidationScheme panic#8840
Open
Chaithanya5gif wants to merge 6 commits into
Open
receive: fix relabel config NameValidationScheme panic#8840Chaithanya5gif wants to merge 6 commits into
Chaithanya5gif wants to merge 6 commits into
Conversation
58f42c3 to
2675a8e
Compare
When Thanos parses --receive.relabel-config from YAML, the NameValidationScheme field defaults to UnsetValidation (0) because it has the yaml:"-" tag. If a metric matches the relabel rules, relabel.Process() will panic on this unset scheme. This calls Validate(model.LegacyValidation) on each config after unmarshalling to properly initialize NameValidationScheme. Added e2e test TestReceiveWithRelabelConfigSmoke to trigger relabeling with a metric. Signed-off-by: chaithanya <chaithanyyal55@gmail.com>
59d0dd2 to
405ff6a
Compare
Member
|
@Chaithanya5gif why is this a draft? |
Docker network names must be <= 16 characters. receive-relabel-smoke (21 chars) -> recv-relabel (12 chars) Signed-off-by: chaithanya <chaithanyyal55@gmail.com>
f60234d to
5d4a695
Compare
Author
@GiedriusS I marked it as draft because the e2e test was failing due to a Docker network name constraint (the test name was too long). I've fixed that now shortened receive-relabel-smoke to recv-relabel. All checks are passing. Should I mark it ready for review? |
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
2 participants
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.
Description
This PR fixes a critical bug where the
receivecomponent panics when processing metrics that match rules in a provided--receive.relabel-configYAML file.Motivation / Root Cause
In Prometheus's
relabel.Configstruct, theNameValidationSchemefield is tagged withyaml:"-". When Thanos parses the relabeling configuration from a YAML file, this field defaults to0(model.UnsetValidation).Later in the pipeline, if a metric actually matches the
source_labelsdefined in the config,relabel.Process()attempts to process the replacement. Because theNameValidationSchemeis left unset, it hits thedefaultcase in the validation switch statement inside the Prometheus relabel package, causing an immediate panic.(Note: Prior to this PR, existing tests for this functionality passed vacuously because they did not send metrics that successfully matched the regex to trigger the replacement path).
Changes
cmd/thanos/receive.go: After unmarshalling the relabel configs, we now explicitly iterate through them and invokecfg.Validate(model.LegacyValidation). This mirrors standard Prometheus behavior and safely initializes theNameValidationSchemebefore the config is ever passed to the router/TSDB.Testing
TestReceiveWithRelabelConfigSmokeintest/e2e/receive_test.go. This test injects a relabel config containingsource_labelsand actively pushes a metric that matches those labels. This forces the receiver to exercise therelabel.Process()replacement path, ensuring the validation panic is caught and prevented in the future.