Skip to content

Commit 409e245

Browse files
jake-kramerjake-kramer
authored
fix: Return modified labels after validation sanitization (#4662)
This fixes a bug where the logic in `handleSanitizedLabel` was not properly executed. The code would modify the slice itself, but this new modified slice was never returned up the call stack.
1 parent 6004579 commit 409e245

File tree

7 files changed

+107
-22
lines changed

7 files changed

+107
-22
lines changed

‎pkg/distributor/distributor.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1174,7 +1174,7 @@ type sampleSeriesVisitor struct {
11741174
discardedProfiles int
11751175
}
11761176

1177-
func (v *sampleSeriesVisitor) ValidateLabels(labels phlaremodel.Labels) error {
1177+
func (v *sampleSeriesVisitor) ValidateLabels(labels phlaremodel.Labels) (phlaremodel.Labels, error) {
11781178
return validation.ValidateLabels(v.limits, v.tenantID, labels, v.logger)
11791179
}
11801180

‎pkg/model/pprofsplit/pprof_split.go‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,9 @@ type SampleSeriesVisitor interface {
1717
VisitProfile(phlaremodel.Labels)
1818
VisitSampleSeries(phlaremodel.Labels, []*profilev1.Sample)
1919
// ValidateLabels is called to validate the labels before
20-
// they are passed to the visitor.
21-
ValidateLabels(phlaremodel.Labels) error
20+
// they are passed to the visitor. Returns the potentially modified
21+
// labels slice (e.g., after sanitization) and any validation error.
22+
ValidateLabels(phlaremodel.Labels) (phlaremodel.Labels, error)
2223
Discarded(profiles, bytes int)
2324
}
2425

@@ -52,7 +53,9 @@ func VisitSampleSeries(
5253
}
5354
if len(profile.Sample) > 0 {
5455
labels = builder.Labels()
55-
if err := visitor.ValidateLabels(labels); err != nil {
56+
var err error
57+
labels, err = visitor.ValidateLabels(labels)
58+
if err != nil {
5659
return err
5760
}
5861
visitor.VisitProfile(labels)
@@ -85,10 +88,11 @@ func VisitSampleSeries(
8588
for _, idx := range groupsKept.order {
8689
for _, group := range groupsKept.m[idx] {
8790
if len(group.sampleGroup.Samples) > 0 {
88-
if err := visitor.ValidateLabels(group.labels); err != nil {
91+
validatedLabels, err := visitor.ValidateLabels(group.labels)
92+
if err != nil {
8993
return err
9094
}
91-
visitor.VisitSampleSeries(group.labels, group.sampleGroup.Samples)
95+
visitor.VisitSampleSeries(validatedLabels, group.sampleGroup.Samples)
9296
}
9397
}
9498
}

‎pkg/model/pprofsplit/pprof_split_by.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ type sampleGroup struct {
7373
samples []*profilev1.Sample
7474
}
7575

76-
func (m *sampleSeriesMerger) ValidateLabels(labels phlaremodel.Labels) error {
76+
func (m *sampleSeriesMerger) ValidateLabels(labels phlaremodel.Labels) (phlaremodel.Labels, error) {
7777
return m.visitor.ValidateLabels(labels)
7878
}
7979

‎pkg/model/pprofsplit/pprof_split_test.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,9 @@ func (m *mockVisitor) VisitSampleSeries(labels phlaremodel.Labels, samples []*pr
4040
})
4141
}
4242

43-
func (m *mockVisitor) ValidateLabels(phlaremodel.Labels) error { return m.err }
43+
func (m *mockVisitor) ValidateLabels(labels phlaremodel.Labels) (phlaremodel.Labels, error) {
44+
return labels, m.err
45+
}
4446

4547
func (m *mockVisitor) Discarded(profiles, bytes int) {
4648
m.discardedBytes += bytes

‎pkg/segmentwriter/segment.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -564,7 +564,9 @@ func (v *sampleAppender) VisitSampleSeries(labels model.Labels, samples []*profi
564564
v.dataset.Ingest(&n, v.id, labels, v.annotations)
565565
}
566566

567-
func (v *sampleAppender) ValidateLabels(model.Labels) error { return nil }
567+
func (v *sampleAppender) ValidateLabels(labels model.Labels) (model.Labels, error) {
568+
return labels, nil
569+
}
568570

569571
func (v *sampleAppender) Discarded(_, _ int) {}
570572

‎pkg/validation/validate.go‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -126,23 +126,24 @@ type LabelValidationLimits interface {
126126
}
127127

128128
// ValidateLabels validates the labels of a profile.
129-
func ValidateLabels(limits LabelValidationLimits, tenantID string, ls []*typesv1.LabelPair, logger log.Logger) error {
129+
// Returns the potentially modified labels slice (e.g., after sanitization) and any validation error.
130+
func ValidateLabels(limits LabelValidationLimits, tenantID string, ls []*typesv1.LabelPair, logger log.Logger) ([]*typesv1.LabelPair, error) {
130131
if len(ls) == 0 {
131-
return NewErrorf(MissingLabels, MissingLabelsErrorMsg)
132+
return nil, NewErrorf(MissingLabels, MissingLabelsErrorMsg)
132133
}
133134
sort.Sort(phlaremodel.Labels(ls))
134135
numLabelNames := len(ls)
135136
maxLabels := limits.MaxLabelNamesPerSeries(tenantID)
136137
if numLabelNames > maxLabels {
137-
return NewErrorf(MaxLabelNamesPerSeries, MaxLabelNamesPerSeriesErrorMsg, phlaremodel.LabelPairsString(ls), numLabelNames, maxLabels)
138+
return nil, NewErrorf(MaxLabelNamesPerSeries, MaxLabelNamesPerSeriesErrorMsg, phlaremodel.LabelPairsString(ls), numLabelNames, maxLabels)
138139
}
139140
metricNameValue := phlaremodel.Labels(ls).Get(model.MetricNameLabel)
140141
if !model.UTF8Validation.IsValidMetricName(metricNameValue) {
141-
return NewErrorf(InvalidLabels, InvalidLabelsErrorMsg, phlaremodel.LabelPairsString(ls), "invalid metric name")
142+
return nil, NewErrorf(InvalidLabels, InvalidLabelsErrorMsg, phlaremodel.LabelPairsString(ls), "invalid metric name")
142143
}
143144
serviceNameValue := phlaremodel.Labels(ls).Get(phlaremodel.LabelNameServiceName)
144145
if !isValidServiceName(serviceNameValue) {
145-
return NewErrorf(MissingLabels, InvalidLabelsErrorMsg, phlaremodel.LabelPairsString(ls), "service name is not provided")
146+
return nil, NewErrorf(MissingLabels, InvalidLabelsErrorMsg, phlaremodel.LabelPairsString(ls), "service name is not provided")
146147
}
147148

148149
var (
@@ -152,16 +153,16 @@ func ValidateLabels(limits LabelValidationLimits, tenantID string, ls []*typesv1
152153
for idx < len(ls) {
153154
l := ls[idx]
154155
if len(l.Name) > limits.MaxLabelNameLength(tenantID) {
155-
return NewErrorf(LabelNameTooLong, LabelNameTooLongErrorMsg, phlaremodel.LabelPairsString(ls), l.Name)
156+
return nil, NewErrorf(LabelNameTooLong, LabelNameTooLongErrorMsg, phlaremodel.LabelPairsString(ls), l.Name)
156157
}
157158
if len(l.Value) > limits.MaxLabelValueLength(tenantID) {
158-
return NewErrorf(LabelValueTooLong, LabelValueTooLongErrorMsg, phlaremodel.LabelPairsString(ls), l.Value)
159+
return nil, NewErrorf(LabelValueTooLong, LabelValueTooLongErrorMsg, phlaremodel.LabelPairsString(ls), l.Value)
159160
}
160161
if origName, newName, ok := SanitizeLegacyLabelName(l.Name); ok && origName != newName {
161162
var err error
162163
ls, idx, err = handleSanitizedLabel(ls, idx, origName, newName)
163164
if err != nil {
164-
return err
165+
return nil, err
165166
}
166167
level.Debug(logger).Log(
167168
"msg", "label name sanitized",
@@ -175,19 +176,19 @@ func ValidateLabels(limits LabelValidationLimits, tenantID string, ls []*typesv1
175176
}
176177
continue
177178
} else if !ok {
178-
return NewErrorf(InvalidLabels, InvalidLabelsErrorMsg, phlaremodel.LabelPairsString(ls), "invalid label name '"+origName+"'")
179+
return nil, NewErrorf(InvalidLabels, InvalidLabelsErrorMsg, phlaremodel.LabelPairsString(ls), "invalid label name '"+origName+"'")
179180
}
180181
if !model.LabelValue(l.Value).IsValid() {
181-
return NewErrorf(InvalidLabels, InvalidLabelsErrorMsg, phlaremodel.LabelPairsString(ls), "invalid label value '"+l.Value+"'")
182+
return nil, NewErrorf(InvalidLabels, InvalidLabelsErrorMsg, phlaremodel.LabelPairsString(ls), "invalid label value '"+l.Value+"'")
182183
}
183184
if cmp := strings.Compare(lastLabelName, l.Name); cmp == 0 {
184-
return NewErrorf(DuplicateLabelNames, DuplicateLabelNamesErrorMsg, phlaremodel.LabelPairsString(ls), l.Name)
185+
return nil, NewErrorf(DuplicateLabelNames, DuplicateLabelNamesErrorMsg, phlaremodel.LabelPairsString(ls), l.Name)
185186
}
186187
lastLabelName = l.Name
187188
idx += 1
188189
}
189190

190-
return nil
191+
return ls, nil
191192
}
192193

193194
// handleSanitizedLabel handles the case where a label name is sanitized. It ensures that the label name is unique and fails if the value is distinct.

‎pkg/validation/validate_test.go‎

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ func TestValidateLabels(t *testing.T) {
137137
},
138138
} {
139139
t.Run(tt.name, func(t *testing.T) {
140-
err := ValidateLabels(MockLimits{
140+
_, err := ValidateLabels(MockLimits{
141141
MaxLabelNamesPerSeriesValue: 4,
142142
MaxLabelNameLengthValue: 12,
143143
MaxLabelValueLengthValue: 10,
@@ -153,6 +153,82 @@ func TestValidateLabels(t *testing.T) {
153153
}
154154
}
155155

156+
func TestValidateLabels_SanitizedLabelsReturned(t *testing.T) {
157+
for _, tt := range []struct {
158+
name string
159+
inputLabels []*typesv1.LabelPair
160+
expectedLabels []*typesv1.LabelPair
161+
}{
162+
{
163+
name: "single dotted label is sanitized",
164+
inputLabels: []*typesv1.LabelPair{
165+
{Name: model.MetricNameLabel, Value: "cpu"},
166+
{Name: "service_name", Value: "my-svc"},
167+
{Name: "label.dot", Value: "val"},
168+
},
169+
expectedLabels: []*typesv1.LabelPair{
170+
{Name: model.MetricNameLabel, Value: "cpu"},
171+
{Name: "label_dot", Value: "val"},
172+
{Name: "service_name", Value: "my-svc"},
173+
},
174+
},
175+
{
176+
name: "dotted label merged with existing underscore label",
177+
inputLabels: []*typesv1.LabelPair{
178+
{Name: model.MetricNameLabel, Value: "cpu"},
179+
{Name: "service.name", Value: "my-svc"},
180+
{Name: "service_name", Value: "my-svc"},
181+
},
182+
expectedLabels: []*typesv1.LabelPair{
183+
{Name: model.MetricNameLabel, Value: "cpu"},
184+
{Name: "service_name", Value: "my-svc"},
185+
},
186+
},
187+
{
188+
name: "multiple dotted labels sanitized",
189+
inputLabels: []*typesv1.LabelPair{
190+
{Name: model.MetricNameLabel, Value: "cpu"},
191+
{Name: "foo.bar", Value: "val1"},
192+
{Name: "label.dot", Value: "val2"},
193+
{Name: "service_name", Value: "my-svc"},
194+
},
195+
expectedLabels: []*typesv1.LabelPair{
196+
{Name: model.MetricNameLabel, Value: "cpu"},
197+
{Name: "foo_bar", Value: "val1"},
198+
{Name: "label_dot", Value: "val2"},
199+
{Name: "service_name", Value: "my-svc"},
200+
},
201+
},
202+
{
203+
name: "labels without dots unchanged",
204+
inputLabels: []*typesv1.LabelPair{
205+
{Name: model.MetricNameLabel, Value: "cpu"},
206+
{Name: "service_name", Value: "my-svc"},
207+
},
208+
expectedLabels: []*typesv1.LabelPair{
209+
{Name: model.MetricNameLabel, Value: "cpu"},
210+
{Name: "service_name", Value: "my-svc"},
211+
},
212+
},
213+
} {
214+
t.Run(tt.name, func(t *testing.T) {
215+
result, err := ValidateLabels(MockLimits{
216+
MaxLabelNamesPerSeriesValue: 10,
217+
MaxLabelNameLengthValue: 50,
218+
MaxLabelValueLengthValue: 50,
219+
}, "test-tenant", tt.inputLabels, log.NewNopLogger())
220+
221+
require.NoError(t, err)
222+
require.Equal(t, len(tt.expectedLabels), len(result), "unexpected number of labels")
223+
224+
for i, expected := range tt.expectedLabels {
225+
require.Equal(t, expected.Name, result[i].Name, "label name mismatch at index %d", i)
226+
require.Equal(t, expected.Value, result[i].Value, "label value mismatch at index %d", i)
227+
}
228+
})
229+
}
230+
}
231+
156232
func Test_ValidateRangeRequest(t *testing.T) {
157233
now := model.Now()
158234
for _, tt := range []struct {

0 commit comments

Comments
 (0)