client.FromLabelAdaptersToLabels breaks contract of labels.Labels

[pstibrany]

cortex/pkg/ingester/client/compat.go

Line 193 in fab5ced

func FromLabelAdaptersToLabels(ls []LabelAdapter) labels.Labels {

labels.Labels is supposed to be sorted set of labels. Conversion from []LabelAdapter is unsafe unless order is checked first.

[bboreham]

There's various other ways to resolve this - could comment that it doesn't check that contact, or comment that it must only be called with sorted LabelAdapter.

Could you say a bit about where this caused a problem?

[pstibrany]

Yes, documenting break of contract or required preconditions is one way of fixing it.

I'm not sure if it is a problem in Cortex (on quick check, it doesn't seem to be), but I've found several unsafe usages in Loki.

[pstibrany]

Same problem in

cortex/pkg/storage/tsdb/labels.go

Line 14 in 5bd771f

func FromLabelAdaptersToLabels(input []client.LabelAdapter) labels.Labels {

(with slightly different types) (@pracucci)

[pracucci]

Same problem in

cortex/pkg/storage/tsdb/labels.go

Line 14 in 5bd771f

func FromLabelAdaptersToLabels(input []client.LabelAdapter) labels.Labels {

(with slightly different types) (@pracucci)

@pstibrany May you elaborate a bit more on the issue, please? labels.Labels is []Label and the tsdb.FromLabelAdaptersToLabels() iterates over the input list of LabelAdapter keeping the same ordering in the output list of Labels so, as far as the input is correctly sorted, the output is as well. What am I missing?

[pstibrany]

as far as the input is correctly sorted, the output is as well.

This is exactly the problem. labels.Labels is documented as: "Labels is a sorted set of labels.", and there may be code relying on this property. However, FromLabelAdaptersToLabels doesn't check for it, and will happily generate unsorted labels.Labels instance, thus breaking contract of labels.Labels. (Personally, I prefer code to be correct first, fast second. In this instance, it's easy to fix. In compact.go file, code tries to be smart, at the expense of being incorrect).

[stale]

This issue has been automatically marked as stale because it has not had any activity in the past 30 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[pstibrany]

There is now a comment on the function warning about this behavior.