Don't expand CIDR labels, match smartly in Labels instead

[squeed]

CIDR labels are expensive. Very expensive. For an IPv6 prefix, GetCidrLabels() can take over 2 KiB of memory.

This change refactors the label internals so that CIDR labels match when the cidr is larger as well. Then, we no longer have to expand CIDR labels at all; the matching logic handles it correctly.

Note to reviewers: Please review by commit; earlier commits add some test invariants. Another commit lightly refactors the label matching API to try and bring some sanity. Only the final two commits change any logic.

[joestringer]

In principle this seems like a really neat win. I can't currently think of any issues with encoding this into the selectors to identify labels with prefixes that are more specific than the selector.

I didn't review in deep detail beyond the second patch, primarily just for time and because I think you were interested in high level feedback at this time. Perhaps another @cilium/sig-policy member could do further indepth review or I can reschedule a subsequent follow-up review later on.

[squeed]

Probably the biggest issue this change "tickles" is the inconsistency around "directionality" in the Label / Labels / LabelArray APIs. This is because label equality, as written, is non-commutative.

a := ParseLabel("any:foo=bar")
b := ParseLabel("k8s:foo=bar")

// What is this, Javascript?
a.Equals(b) // == true
b.Equals(a) // == false
a == b // false

Oh, and Labels.Equals() doesn't use Label.Equals():

aa := labels.FromSlice({a}); bb := labels.FromSlice({b})
aa.Equals(bb) // == false
bb.Equals(aa) // == false

But LabelArray.Equals() does:

aa.LabelArray().Equals(bb.LabelArray()) // true
bb.LabelArray().Equals(aa.LabelArray()) // false

For more fun, LabelArray.Has() and Labels.Has() are also inverted in direction:

aa.LabelArray().Has(b) // false
aa.Has(b) // true

My eventual goal with all of this:

• .Equals() is always strict and commutative, for Label, Labels, and LabelArray. In fact, we should get rid of Label.Equals()
• ["1.1.1.1/32"].Has("1.1.1.0/24") == true for Labels and LabelArray
• ["k8s:foo=bar"].Has("any:foo=bar") == true for Labels and LabelArray
• Contains() and Lacks() are consistent with Has() (I believe they are, but this needs to be tested)
• Nothing breaks 😬

So, the first step is a consistent .Matches() function.

[squeed]

One thing this change relies on is the current limitation that CIDR labels don't have a value. We might fail the case for

["cidr:1.1.1.0/24=foo", "cidr:1.1.1.1/32=bar"].Get("cidr:1.1.1.1/32")
["cidr:1.1.1.0/24=foo", "cidr:1.1.1.1/32=bar"].Get("cidr:1.1.1.0/31")

[pippolo84]

I second what Joe said regarding this being a neat win. I remember thinking about something similar while working on #28788 but got overwhelmed by the potential impact of such change.

As you said, the CIDR labels are way too expensive, and in the IPv6 case, this also limits the effectiveness of the CIDR labels cache introduced in #28788, as it is not possible to get a meaningful hit ratio without a very large memory footprint.
Besides, this seems a lot easier to reason about, compared with the memoization approach of the CIDR cache.

My only concern is the current inconsistencies with the API you already highlighted, but the plan you depicted here seems sound and might also give us a chance to clean this up, besides improving performance. 👍

[squeed]

/test

[squeed]

/test

[nathanjsweet]

Very impressive! Thanks! Two questions.

[christarazi]

The changes LGTM and I can't think of any adverse effects from the PR. I am curious though if we have an understanding of what the actual performance impact is with this change. AFAIU, GetCIDRLabels consumed a lot of CPU and memory to generate CIDR labels constantly when the DNS proxy sees many DNS requests, but how much has this PR really changed? Have we done a pprof comparison, for example? I don't doubt that this PR improved things, but it's also useful to quantify how much has changed and worst case, potentially point out a flaw that we didn't detect previously.

[squeed]

/test