Add support for MatchExpressions

[dprotaso]

• Add MatchExpression to TargetSelector type
• add matchExpressions support to helpers

What type of PR is this?

This features extends TargetSelector to support match expressions

This affects the following types that embed TargetSelector

BackendTrafficPolicySpec
ClientTrafficPolicySpec
EnvoyExtensionPolicySpec
PolicyTargetReferences
SecurityPolicySpec

What this PR does / why we need it:

This provides further flexibility in matching resources

Which issue(s) this PR fixes:

N/A

Release Notes: Yes

[dprotaso]

Note: empty MatchLabels and and empty MatchExpressions will match everything

[guydc]

Was this previous behavior with MatchLabels? if not, let's add a release note for a breaking change.

Also, in general, let's document these semantics in the top-level struct.

What's the behavior if both are set? If this is not allowed, maybe we should:

• Enforce it with CEL.
• Move to a new GW-API-like structure where we have a Match type and then the match value

type: LabelSelector
matchLabels:
  - foo: bar
  - bar:foo 

[dprotaso]

Was this previous behavior with MatchLabels? if not, let's add a release note for a breaking change.

Yeah - nil/empty set matches everything - see ref

What's the behavior if both are set? If this is not allowed, maybe we should:

They can be used in conjunction with each other - like a regular metav1.LabelSelector. This is similar behaviour to the namespaceSelector on various k8s types.

You can see it in the parsing logic here - https://github.com/kubernetes/apimachinery/blob/a19f1f813776bd4a90401b63a96e07ed847f88d0/pkg/apis/meta/v1/helpers.go#L36

[dprotaso]

I didn't see a validation phase after cel but before translation. So for now if we can't parse the selector we match nothing.

[sanposhiho]

Can you open a follow-up issue so that we don't forget this? If users get trapped in this, it's going to be hard for them to figure out why their change isn't working. So, we should report it somewhere, probably at the resource's status ideally.

[dprotaso]

I also didn't see an opportunity to 'cache' the parsing here - if there's a way to do that with envoy gateway code (like some in memory temporal state) let me know

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.99%. Comparing base (7146fe8) to head (21f3d00).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5201   +/-   ##
=======================================
  Coverage   67.98%   67.99%           
=======================================
  Files         212      212           
  Lines       33328    33338   +10     
=======================================
+ Hits        22659    22667    +8     
  Misses       9298     9298           
- Partials     1371     1373    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[arkodg]

thanks @dprotaso, the API looks good, can you also add an e2e to make sure this works
cc @liorokman @guydc

[guydc]

thanks @dprotaso, the API looks good, can you also add an e2e to make sure this works cc @liorokman @guydc

or at least a translator test. Besides that - looks good.

[dprotaso]

can you also add an e2e to make sure this works

Do you have a preference for which resource to do an e2e test with?

Any thoughts on this comment?
#5201 (comment)

[dprotaso]

Weird make generate doesn't update the CRDs

[dprotaso]

I'll add a bad selector test as well

[dprotaso]

So one thing I realized - if the matchExpressions is not valid then there's no way to collect a list of ancestors.

Thus to report an error to the user we'd need a top-level condition on all the policies.

Let me know what you want me to do.

[dprotaso]

An alternative would be to create cel expressions and hope they never let in any bad selectors to the parse call (still and edge case)

[arkodg]

LGTM thanks !

[arkodg]

hey @dprotaso can you fix the lint issues

test/e2e/testdata/backend-traffic-policy-match-expression.yaml
  Error: 12:7 [indentation] wrong indentation: expected 4 but found 6
  Error: 28:7 [indentation] wrong indentation: expected 4 but found 6

[dprotaso]

rebased and fixed indentation

[sanposhiho]

I'm not sure if it's safe to add omitempty here. like what if a user upgrade -> create something with empty MatchLabels -> downgrade?
I don't know if EG team usually thinks about such scenarios, but at k/k, we do.

[sanposhiho]

cc @arkodg

[dprotaso]

Wouldn't this scenario be fine? eg. a nil map is still a valid map

https://go.dev/play/p/bnc8MFRh2Y6

[sanposhiho]

I'm not sure how openapi's required would behave with nil map.

[dprotaso]

ah good point - also these APIs are marked alpha - so I wonder if there even is a guarantee that downgrades work

[sanposhiho]

Yeah, so I see it as more about the rule on this project; whether/how much we consider downgrade scenarios.

But, I believe, at least ideally, we should care the downgrading scenario for alpha APIs as well. Especially this change's blast radius could (i.e., we need to check) be terrible since we're not sure how kube-apiserver behaves with CRD update (in our case, version downgrade) that puts required on the existing field, while there're some existing resources that have empty on the field.

The obvious safer way here is to introduce this change at v1alpha2. (but, again, I'd defer to how other maintainers think and how we define the policy within the project)

[arkodg]

thanks for raising this concern @sanposhiho
loosening validation is fine here, would be great to guarantee the downgrade case, and deal with CRD and Image version mismatch, since the API is alpha its fine if we cannot support that

[sanposhiho]

Sure, then let's go with this current change and future changes for alpha APIs as well.

But, we need to watch out, generally speaking, some changes at CRD could break kube-apiserver layer (like you wouldn't be able to operate resources) when reviewing this kind of API changes.

[arkodg]

e2e is failing, and so is DCO @dprotaso

[dprotaso]

fixed DCO

e2e test failure seems unrelated now