Skip to content

Add support for MatchExpressions #5201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
arkodg merged 11 commits into from
Feb 12, 2025
Merged

Add support for MatchExpressions #5201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e97d9eb
Add MatchExpression to TargetSelector type
dprotaso Feb 4, 2025
19b2fe7
add matchExpressions support to helpers
dprotaso Feb 4, 2025
a87138c
include note in release notes
dprotaso Feb 4, 2025
6e4dab1
update CRD manifests
dprotaso Feb 4, 2025
37a8d2d
add e2e test
dprotaso Feb 5, 2025
604548f
fix indentation
dprotaso Feb 11, 2025
a52eb93
fix httproute rules in e2e test
dprotaso Feb 11, 2025
c2fa492
drop atomic type
dprotaso Feb 11, 2025
76e5731
include path '/' in expected request
dprotaso Feb 12, 2025
863905c
add namespace to expected response
dprotaso Feb 12, 2025
21f3d00
Merge branch 'main' into match-expression
arkodg Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion api/v1alpha1/policy_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package v1alpha1

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"
gwapiv1a2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
)
Expand Down Expand Up @@ -37,7 +38,14 @@ type TargetSelector struct {
Kind gwapiv1.Kind `json:"kind"`

// MatchLabels are the set of label selectors for identifying the targeted resource
MatchLabels map[string]string `json:"matchLabels"`
// +optional

@dprotaso dprotaso Feb 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note: empty MatchLabels and and empty MatchExpressions will match everything

@guydc guydc Feb 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 dprotaso Feb 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

MatchLabels map[string]string `json:"matchLabels,omitempty"`

@sanposhiho sanposhiho Feb 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 sanposhiho Feb 11, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cc @arkodg

@dprotaso dprotaso Feb 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

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

@sanposhiho sanposhiho Feb 11, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@dprotaso dprotaso Feb 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@sanposhiho sanposhiho Feb 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 arkodg Feb 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 sanposhiho Feb 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.


// MatchExpressions is a list of label selector requirements. The requirements are ANDed.
//
// +optional
// +listType=atomic
MatchExpressions []metav1.LabelSelectorRequirement `json:"matchExpressions,omitempty"`
}

func (p PolicyTargetReferences) GetTargetRefs() []gwapiv1a2.LocalPolicyTargetReferenceWithSectionName {
Expand Down
7 changes: 7 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 33 additions & 1 deletion charts/gateway-helm/crds/generated/gateway.envoyproxy.io_backendtrafficpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1508,6 +1508,39 @@ spec:
minLength: 1
pattern: ^[a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?$
type: string
matchExpressions:
description: MatchExpressions is a list of label selector requirements.
The requirements are ANDed.
items:
description: |-
A label selector requirement is a selector that contains values, a key, and an operator that
relates the key and values.
properties:
key:
description: key is the label key that the selector applies
to.
type: string
operator:
description: |-
operator represents a key's relationship to a set of values.
Valid operators are In, NotIn, Exists and DoesNotExist.
type: string
values:
description: |-
values is an array of string values. If the operator is In or NotIn,
the values array must be non-empty. If the operator is Exists or DoesNotExist,
the values array must be empty. This array is replaced during a strategic
merge patch.
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand All @@ -1516,7 +1549,6 @@ spec:
type: object
required:
- kind
- matchLabels
type: object
x-kubernetes-validations:
- message: group must be gateway.networking.k8s.io
Expand Down
34 changes: 33 additions & 1 deletion charts/gateway-helm/crds/generated/gateway.envoyproxy.io_clienttrafficpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,39 @@ spec:
minLength: 1
pattern: ^[a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?$
type: string
matchExpressions:
description: MatchExpressions is a list of label selector requirements.
The requirements are ANDed.
items:
description: |-
A label selector requirement is a selector that contains values, a key, and an operator that
relates the key and values.
properties:
key:
description: key is the label key that the selector applies
to.
type: string
operator:
description: |-
operator represents a key's relationship to a set of values.
Valid operators are In, NotIn, Exists and DoesNotExist.
type: string
values:
description: |-
values is an array of string values. If the operator is In or NotIn,
the values array must be non-empty. If the operator is Exists or DoesNotExist,
the values array must be empty. This array is replaced during a strategic
merge patch.
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand All @@ -616,7 +649,6 @@ spec:
type: object
required:
- kind
- matchLabels
type: object
x-kubernetes-validations:
- message: group must be gateway.networking.k8s.io
Expand Down
34 changes: 33 additions & 1 deletion charts/gateway-helm/crds/generated/gateway.envoyproxy.io_envoyextensionpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1201,6 +1201,39 @@ spec:
minLength: 1
pattern: ^[a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?$
type: string
matchExpressions:
description: MatchExpressions is a list of label selector requirements.
The requirements are ANDed.
items:
description: |-
A label selector requirement is a selector that contains values, a key, and an operator that
relates the key and values.
properties:
key:
description: key is the label key that the selector applies
to.
type: string
operator:
description: |-
operator represents a key's relationship to a set of values.
Valid operators are In, NotIn, Exists and DoesNotExist.
type: string
values:
description: |-
values is an array of string values. If the operator is In or NotIn,
the values array must be non-empty. If the operator is Exists or DoesNotExist,
the values array must be empty. This array is replaced during a strategic
merge patch.
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand All @@ -1209,7 +1242,6 @@ spec:
type: object
required:
- kind
- matchLabels
type: object
x-kubernetes-validations:
- message: group must be gateway.networking.k8s.io
Expand Down
34 changes: 33 additions & 1 deletion charts/gateway-helm/crds/generated/gateway.envoyproxy.io_securitypolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4447,6 +4447,39 @@ spec:
minLength: 1
pattern: ^[a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?$
type: string
matchExpressions:
description: MatchExpressions is a list of label selector requirements.
The requirements are ANDed.
items:
description: |-
A label selector requirement is a selector that contains values, a key, and an operator that
relates the key and values.
properties:
key:
description: key is the label key that the selector applies
to.
type: string
operator:
description: |-
operator represents a key's relationship to a set of values.
Valid operators are In, NotIn, Exists and DoesNotExist.
type: string
values:
description: |-
values is an array of string values. If the operator is In or NotIn,
the values array must be non-empty. If the operator is Exists or DoesNotExist,
the values array must be empty. This array is replaced during a strategic
merge patch.
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand All @@ -4455,7 +4488,6 @@ spec:
type: object
required:
- kind
- matchLabels
type: object
x-kubernetes-validations:
- message: group must be gateway.networking.k8s.io
Expand Down
14 changes: 13 additions & 1 deletion internal/gatewayapi/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,10 +533,22 @@ type targetRefWithTimestamp struct {
CreationTimestamp metav1.Time
}

func selectorFromTargetSelector(selector egv1a1.TargetSelector) labels.Selector {
l, err := metav1.LabelSelectorAsSelector(&metav1.LabelSelector{

@dprotaso dprotaso Feb 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

MatchLabels: selector.MatchLabels,
MatchExpressions: selector.MatchExpressions,
})
if err != nil {
// TODO - how do we we bubble this up

@dprotaso dprotaso Feb 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 sanposhiho Feb 12, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

return labels.Nothing()
}
return l
}

func getPolicyTargetRefs[T client.Object](policy egv1a1.PolicyTargetReferences, potentialTargets []T) []gwapiv1a2.LocalPolicyTargetReferenceWithSectionName {
dedup := sets.New[targetRefWithTimestamp]()
for _, currSelector := range policy.TargetSelectors {
labelSelector := labels.SelectorFromSet(currSelector.MatchLabels)
labelSelector := selectorFromTargetSelector(currSelector)
for _, obj := range potentialTargets {
gvk := obj.GetObjectKind().GroupVersionKind()
if gvk.Kind != string(currSelector.Kind) ||
Expand Down
101 changes: 101 additions & 0 deletions internal/gatewayapi/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (

"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -475,6 +476,106 @@ func TestGetPolicyTargetRefs(t *testing.T) {
},
results: []gwapiv1a2.LocalPolicyTargetReferenceWithSectionName{},
},
{
name: "match expression",
policy: egv1a1.PolicyTargetReferences{
TargetSelectors: []egv1a1.TargetSelector{
{
Kind: "Gateway",
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "environment",
Operator: "In",
Values: []string{"prod", "staging"},
},
},
},
},
},
targets: []*unstructured.Unstructured{
{
Object: map[string]any{
"apiVersion": "gateway.networking.k8s.io/v1",
"kind": "Gateway",
"metadata": map[string]any{
"name": "first",
"namespace": "default",
"labels": map[string]any{
"environment": "prod",
},
},
},
},
{
Object: map[string]any{
"apiVersion": "gateway.networking.k8s.io/v1",
"kind": "Gateway",
"metadata": map[string]any{
"name": "second",
"namespace": "default",
"labels": map[string]any{
"environment": "dev",
},
},
},
},
},
results: []gwapiv1a2.LocalPolicyTargetReferenceWithSectionName{
{
LocalPolicyTargetReference: gwapiv1a2.LocalPolicyTargetReference{
Group: "gateway.networking.k8s.io",
Kind: "Gateway",
Name: "first",
},
},
},
},
{
name: "match expression - bad expression matches nothing",
policy: egv1a1.PolicyTargetReferences{
TargetSelectors: []egv1a1.TargetSelector{
{
Kind: "Gateway",
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "environment",
Operator: "Foo",
Values: []string{"prod", "staging"},
},
},
},
},
},
targets: []*unstructured.Unstructured{
{
Object: map[string]any{
"apiVersion": "gateway.networking.k8s.io/v1",
"kind": "Gateway",
"metadata": map[string]any{
"name": "first",
"namespace": "default",
"labels": map[string]any{
"environment": "prod",
},
},
},
},
{
Object: map[string]any{
"apiVersion": "gateway.networking.k8s.io/v1",
"kind": "Gateway",
"metadata": map[string]any{
"name": "second",
"namespace": "default",
"labels": map[string]any{
"environment": "dev",
},
},
},
},
},
results: []gwapiv1a2.LocalPolicyTargetReferenceWithSectionName{},
},
}

for _, tc := range testCases {
Expand Down
1 change: 1 addition & 0 deletions release-notes/current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ security updates: |
new features: |
Added support for configuring maxUnavailable in KubernetesPodDisruptionBudgetSpec
Added support for percentage-based request mirroring
Allow matchExpressions in TargetSelector
Add defaulter for gateway-api resources loading from file to be able to set default values.

bug fixes: |
Expand Down
3 changes: 2 additions & 1 deletion site/content/en/latest/api/extension_types.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4297,7 +4297,8 @@ _Appears in:_
| --- | --- | --- | --- | --- |
| `group` | _[Group](#group)_ | true | gateway.networking.k8s.io | Group is the group that this selector targets. Defaults to gateway.networking.k8s.io |
| `kind` | _[Kind](#kind)_ | true | | Kind is the resource kind that this selector targets. |
| `matchLabels` | _object (keys:string, values:string)_ | true | | MatchLabels are the set of label selectors for identifying the targeted resource |
| `matchLabels` | _object (keys:string, values:string)_ | false | | MatchLabels are the set of label selectors for identifying the targeted resource |
| `matchExpressions` | _[LabelSelectorRequirement](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.29/#labelselectorrequirement-v1-meta) array_ | false | | MatchExpressions is a list of label selector requirements. The requirements are ANDed. |


#### Timeout
Expand Down
3 changes: 2 additions & 1 deletion site/content/zh/latest/api/extension_types.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4297,7 +4297,8 @@ _Appears in:_
| --- | --- | --- | --- | --- |
| `group` | _[Group](#group)_ | true | gateway.networking.k8s.io | Group is the group that this selector targets. Defaults to gateway.networking.k8s.io |
| `kind` | _[Kind](#kind)_ | true | | Kind is the resource kind that this selector targets. |
| `matchLabels` | _object (keys:string, values:string)_ | true | | MatchLabels are the set of label selectors for identifying the targeted resource |
| `matchLabels` | _object (keys:string, values:string)_ | false | | MatchLabels are the set of label selectors for identifying the targeted resource |
| `matchExpressions` | _[LabelSelectorRequirement](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.29/#labelselectorrequirement-v1-meta) array_ | false | | MatchExpressions is a list of label selector requirements. The requirements are ANDed. |


#### Timeout
Expand Down
Loading