Skip to content

test: add long-label injection to TestValidateLabels#14156

Draft
aknuds1 wants to merge 1 commit intomainfrom
test/long-label-injection-validation
Draft

test: add long-label injection to TestValidateLabels#14156
aknuds1 wants to merge 1 commit intomainfrom
test/long-label-injection-validation

Conversation

@aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Jan 24, 2026

What this PR does

Following the review comment on PR #14131, this adds long-label injection variants to TestValidateLabels to ensure that long-label handling (truncate/drop strategies) doesn't mask ANY validation error, not just duplicate label detection.

For each eligible test case, the test now runs in three modes:

  • no_injection: original behavior
  • inject_long_label_truncate: adds a long-value label, uses truncate user
  • inject_long_label_drop: adds a long-value label, uses drop user

Test cases are skipped for injection when they would conflict with the injection behavior (already testing long-label handling, non-legacy scheme, would exceed label limits, or have exact label validation).

This provides regression protection: if a validation check is accidentally placed inside the long-label handling branch, the injection variants will catch it by showing the validation error is masked when a long label is present.

Which issue(s) this PR fixes or relates to

Related to #14131

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]. If changelog entry is not needed, please add the changelog-not-needed label to the PR.
  • about-versioning.md updated with experimental features.

@aknuds1 aknuds1 requested a review from a team as a code owner January 24, 2026 12:26
@aknuds1 aknuds1 added the changelog-not-needed PRs that don't need a CHANGELOG.md entry label Jan 24, 2026
@aknuds1 aknuds1 marked this pull request as draft January 24, 2026 12:28
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is ON. A Cloud Agent has been kicked off to fix the reported issue.

injectedLongLabelValue := strings.Repeat("L", 100)
// Pre-compute the expected hash for the injected long label.
injectedLabelTruncatedValue := injectedLongLabelValue[:4] + hashLabelValueInto(injectedLongLabelValue, injectedLongLabelValue)
injectedLabelDroppedValue := hashLabelValueInto(injectedLongLabelValue, injectedLongLabelValue)
Copy link

Choose a reason for hiding this comment

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

String mutation corrupts expected label values

Low Severity

The hashLabelValueInto function mutates the backing array of injectedLongLabelValue in place. On line 405, after the function call, injectedLongLabelValue[:4] returns "(has" (the first 4 bytes of the hash output) instead of "LLLL". On line 406, the second call computes a hash of the already-mutated string instead of the original. Both injectedLabelTruncatedValue and injectedLabelDroppedValue contain incorrect expected values. This bug is currently masked because these values are only used in dead code.

Fix in Cursor Fix in Web

@cursor
Copy link

cursor bot commented Jan 24, 2026

Bugbot Autofix prepared a fix for the bug found in the latest run.

  • ✅ Fixed: String mutation corrupts expected label values
    • Used separate string copies for each hashLabelValueInto call to prevent in-place mutation from corrupting the expected values and test input.

Create PR

Preview (63714cf)
diff --git a/pkg/distributor/validate_test.go b/pkg/distributor/validate_test.go
--- a/pkg/distributor/validate_test.go
+++ b/pkg/distributor/validate_test.go
@@ -402,8 +402,11 @@ func TestValidateLabels(t *testing.T) {
 	// injectedLongLabelValue is 100 chars, exceeding the 75 char limit for truncate/drop users.
 	injectedLongLabelValue := strings.Repeat("L", 100)
 	// Pre-compute the expected hash for the injected long label.
-	injectedLabelTruncatedValue := injectedLongLabelValue[:4] + hashLabelValueInto(injectedLongLabelValue, injectedLongLabelValue)
-	injectedLabelDroppedValue := hashLabelValueInto(injectedLongLabelValue, injectedLongLabelValue)
+	// Use separate copies because hashLabelValueInto mutates the backing array in place.
+	truncateSrc := strings.Repeat("L", 100)
+	injectedLabelTruncatedValue := "LLLL" + hashLabelValueInto(truncateSrc, truncateSrc)
+	dropSrc := strings.Repeat("L", 100)
+	injectedLabelDroppedValue := hashLabelValueInto(dropSrc, dropSrc)
 
 	for _, c := range testCases {
 		caseSchemes := validationSchemes

Following the review comment on PR #14131, this adds long-label
injection variants to TestValidateLabels to ensure that long-label
handling (truncate/drop strategies) doesn't mask ANY validation error,
not just duplicate label detection.

For each eligible test case, the test now runs in three modes:
- no_injection: original behavior
- inject_long_label_truncate: adds a long-value label, uses truncate user
- inject_long_label_drop: adds a long-value label, uses drop user

Test cases are skipped for injection when they:
- Already test long-label handling (customUserID is truncate/drop user)
- Use non-legacy validation scheme (injection users use legacy)
- Would exceed label count limits with the additional label
- Have wantLabels (exact label validation)

This provides regression protection: if a validation check is
accidentally placed inside the long-label handling branch (like the
bug fixed in #14131), the injection variants will catch it by showing
the validation error is masked when a long label is present.
@aknuds1 aknuds1 force-pushed the test/long-label-injection-validation branch from 183b33b to a5929d3 Compare January 28, 2026 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog-not-needed PRs that don't need a CHANGELOG.md entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments