feat: watch resource with selectors by den3tsou · Pull Request #1661 · envoyproxy/gateway

den3tsou · 2023-07-16T00:03:42Z

What this PR does / why we need it:
This PR is to provides a handy way for users to watch namespaces with certain labels. Envoy Gateway currently only supports specifying namespaces to watch which is not convenient for users.

Add namespaceSelectors watch mode for Kubernetes provider. Users will need to specify EnvoyGateway.Provider.Type and precisely one of EnvoyGateway.Provider.Kubernetes.Wach.Namespaces and EnvoyGateway.Provider.Kubernetes.Wach.NamespaceSelectors to set the KuberNtes wach mode.

The namespaceSelectors doesn't change the namespace informers watch. The informer still watches all namespaces. The events which have Objects that are not under namesapces with the labels set by NamespaceSelectors are filtered out.

Which issue(s) this PR fixes:
Fixes #1622

Add namespaceSelectors watch mode for Kubernetes provider. Users will need to specify `EnvoyGateway.Provider.Type` and precisely one of `EnvoyGateway.Provider.Kubernetes.Wach.Namespaces` and `EnvoyGateway.Provider.Kubernetes.Wach.NamespaceSelectors` to set the KuberNtes wach mode. The namespaceSelectors doesn't change the namespace informers watch. The informer still watches all namespaces. The events which have Objects that are not under namesapces with the labels set by `NamespaceSelectors` are filtered out. Signed-off-by: Den Tsou <den3tsou@gmail.com>

den3tsou · 2023-07-16T00:08:11Z

internal/provider/kubernetes/kubernetes.go

 	}

-	if svr.EnvoyGateway.Provider != nil &&
+	// TODO: implment config validatoin on the watch mode config


There doesn't seem to be any validation in place. Do I need to add it in this PR?

den3tsou · 2023-07-16T00:09:03Z

internal/provider/kubernetes/predicates.go

+			},
+			ns,
+		); err != nil {
+			// Question: what's the common practice to handle error in predicate?


Could anyone help me to understand the common practices in Enovy Gateway to handle errors in predicate?

logging an error and returning false is fine

den3tsou · 2023-07-16T00:09:30Z

internal/provider/kubernetes/predicates_test.go


+// TestGatewayClassHasMatchingNamespaceLabels tests the hasMatchingNamespaceLabels
+// predicate function.
+func TestGatewayClassHasMatchingNamespaceLabels(t *testing.T) {


Is this the only place to add tests? Do I need to add tests anywhere else?

a good end to end test to add would be to implement this example https://gateway-api.sigs.k8s.io/guides/multiple-ns/ in our e2e https://github.com/envoyproxy/gateway/tree/main/test/e2e/tests
and ensure above traffic path works

Hi @arkodg, would you please help me understand how to test the predicate behaviour with e2e tests? Is there any API I could hit in order to know that certain events are filtered out?

based on https://gateway-api.sigs.k8s.io/guides/multiple-ns/#shared-gateway you could ensure e2e routing works when default ns has label shared-gateway-access: "true" set and fails when it is not set

Hi @arkodg, this is doable but difficult. There is no easy way to reconfig Envoy Gateway in the current testing structure inlcuding 1) apply new ConfigMap 2) restart Envoy Gateway which is needed for this test. Adding those changes will make this PR too big. What's your thoughts on this?

Hi @arkodg This is a bit tricky. I will share what I've found, so you could help me to brainstorm some ideas.

There are two approach available at this moment

Initialise the k8s client to obtain all namespaces with certain labels

Pros:

No complex change for reconciler because configuring Manager is enough

Cons:

Users need to restart Envoy Gateway controller every time they add update the namespaces unless there is another goroutine or anything similar to check the namespace regularly and update the Manger config accordingly

Use predicate and update all of logic inside reconciler to check if the objects' namespaces have certain labels

Pros:

Users don't need to redeploy Envoy Gateway controller every time they update namespaces

Cons:

All logic inside reconciler uses manager.client needs to have additional logic to check if the objects are under namepsace with certain labels. This may cause performance issue because every object will need to be checked.

These two solutions are what I've found so far. I've checked the controller-runtime to see if it has any features that I could workaround. Unfortunately, I coudln't find any solutions yet. Do you have any ideas?

Hi @arkodg , any update on this. I am fine to decline this PR and work on other tasks. I've spent fair bit of time finding other solution. But this is more related to how reconciler works, there is no work around for it. All of code related to pull data from api server will need to be updated.

I vote for Use predicate and update all of logic inside reconciler to check if the objects' namespaces have certain labels . The con here is the extra Get of the Namespace which is hopefully cached and shouldnt be expensive, another con is logic complexity, but counting on you to max code reuse :)

Thanks 👍 . I will start to implement it :-D

Hi @arkodg , do you mind having a look and giving some early feedback? I am still working out how to add tests to make sure all places with the namespace labels check are covered.

Signed-off-by: Den Tsou <den3tsou@gmail.com>

api/config/v1alpha1/envoygateway_types.go

codecov · 2023-07-19T03:08:50Z

Codecov Report

Merging #1661 (4e27b7c) into main (31ea531) will increase coverage by 0.69%.
The diff coverage is 70.48%.

@@            Coverage Diff             @@
##             main    #1661      +/-   ##
==========================================
+ Coverage   65.36%   66.06%   +0.69%     
==========================================
  Files          86       86              
  Lines       12529    12829     +300     
==========================================
+ Hits         8190     8475     +285     
+ Misses       3822     3821       -1     
- Partials      517      533      +16

Files Changed	Coverage Δ
internal/provider/kubernetes/filters.go	`52.30% <54.16%> (-4.22%)`	⬇️
internal/provider/kubernetes/predicates.go	`54.78% <59.37%> (+0.84%)`	⬆️
internal/provider/kubernetes/routes.go	`56.00% <73.33%> (+12.27%)`	⬆️
internal/provider/kubernetes/controller.go	`55.75% <78.10%> (+6.61%)`	⬆️
internal/provider/kubernetes/kubernetes.go	`69.64% <100.00%> (+2.33%)`	⬆️

... and 1 file with indirect coverage changes

Signed-off-by: Den Tsou <den3tsou@gmail.com>

…elector

* This is a protytpe. Refactoring is required to make the code more readable because the code is getting more complex after adding this logic * Update to check all labels of namespaces of objects returned by client * Fix a bug that wrong type was checked * Don't apply predicate to filter out the event related to GatewayClass because GatewayClass is cluster scoped object * Simple test to check if only certain number of gateway is returned. More test logic is indeed needed to be added Signed-off-by: Den Tsou <den3tsou@gmail.com>

Signed-off-by: Den Tsou <den3tsou@gmail.com>

Signed-off-by: Den <6628668+den3tsou@users.noreply.github.com>

Signed-off-by: Den Tsou <den3tsou@gmail.com>

arkodg · 2023-08-24T00:33:01Z

api/config/v1alpha1/envoygateway_types.go

+	// Type indicates what watch mode to use. KubernetesWatchModeTypeNamespaces and
+	// KubernetesWatchModeTypeNamespaceSelectors are currently supported
+	// By default, when this field is unset or empty, Envoy Gateway will watch for input namespaced resources
+	// from all namespaces.


needs a default tag here similar to

gateway/api/config/v1alpha1/envoyproxy_types.go

Line 169 in 91c0888

// +kubebuilder:default=Replace

Do I miss anything here? This is an optional field. It has to be defined with either Namespaces or NamespaceSelectors

Hi @arkodg, do you mind helping me understand this?

this should be // +kubebuilder:default=Namespaces

arkodg · 2023-08-24T00:37:04Z

internal/provider/kubernetes/routes.go

 	}

-	for _, udpRoute := range udpRouteList.Items {
+	udpRoutes := udpRouteList.Items


can all this code be consolidated/reused across all resource types by using a helper func with a client.Object signature ?

ahhh. Good catch. I didn't realise that client.Object could be used!

Unfortunately, AuthenticaionFilter, RateLimitFilterandunstructured.Unstructured` don't implment the common interface. The alternative I could do is to add a NamespaceGetter interface to simplify the code.

you could use existing code for them, and add a TODO

arkodg · 2023-08-24T00:38:07Z

internal/provider/kubernetes/predicates.go

+		ok, err := r.checkNamespaceLabels(ns)
+		if err != nil {
+			r.log.Error(
+				err, "fail to get Namespace",


Suggested change

err, "fail to get Namespace",

err, "failed to get Namespace labels",

arkodg · 2023-08-24T00:42:59Z

internal/provider/kubernetes/controller.go

+		predicate.ResourceVersionChangedPredicate{},
+		predicate.NewPredicateFuncs(r.hasManagedClass),
+	}
+	if len(ls) != 0 {


why is ls needed here, can't you just use r.namespaceLabels as a way to check

good catch. updated

* Update hasMatchingNamespaceLabels signature because labels is part of the struct field now * Remove the logic to check namespace of certificate ref because it is not necessary * Refactor the checkNamespaceLabels with new interface, so the code is more readable now Signed-off-by: Den Tsou <den3tsou@gmail.com>

Signed-off-by: Den Tsou <den3tsou@gmail.com>

Signed-off-by: Den <6628668+den3tsou@users.noreply.github.com>

Signed-off-by: Den Tsou <den3tsou@gmail.com>

den3tsou · 2023-09-03T07:04:50Z

Hi @arkodg , I finally complete the integration tests. Would you please have a look again?

arkodg

one nit, else looks good, thanks @den3tsou !

arkodg · 2023-09-12T20:56:24Z

lets merge this one after #1534 has merged to reduce conflicts

Alice-Lilith

lgtm. @arkodg I see no conflicts after merging #1534 so I'm gonna go ahead and merge this now to prevent additional churn on this PR. Any outstanding nits can be resolved via a follow-up PR.

den3tsou requested a review from a team as a code owner July 16, 2023 00:03

den3tsou commented Jul 16, 2023

View reviewed changes

den3tsou added 2 commits July 17, 2023 21:35

fix: fix typos

84f7aa5

Signed-off-by: Den Tsou <den3tsou@gmail.com>

fix: fix a test and fix a bug that pointer wasn't checked

ea8fd66

Signed-off-by: Den Tsou <den3tsou@gmail.com>

arkodg reviewed Jul 18, 2023

View reviewed changes

api/config/v1alpha1/envoygateway_types.go Outdated Show resolved Hide resolved

arkodg reviewed Jul 18, 2023

View reviewed changes

api/config/v1alpha1/envoygateway_types.go Outdated Show resolved Hide resolved

arkodg reviewed Jul 18, 2023

View reviewed changes

api/config/v1alpha1/envoygateway_types.go Outdated Show resolved Hide resolved

Merge branch 'main' into watch-resource-with-selector

42f4d45

den3tsou and others added 7 commits July 19, 2023 19:04

chore: remove an unneeded comment

77098b9

Signed-off-by: Den Tsou <den3tsou@gmail.com>

chore: replace string type with KubernetesWatchModeType

4b13b00

Signed-off-by: Den Tsou <den3tsou@gmail.com>

fix: use right values for KubernetesWatchModeType

0ad39f0

Signed-off-by: Den Tsou <den3tsou@gmail.com>

Merge branch 'main' into watch-resource-with-selector

631fb62

chore: run make generate

862d22a

Signed-off-by: Den Tsou <den3tsou@gmail.com>

Merge remote-tracking branch 'origin/main' into watch-resource-with-s…

4b6abc9

…elector

Merge branch 'main' into watch-resource-with-selector

6b88978

arkodg added this to the 0.6.0-rc1 milestone Jul 27, 2023

den3tsou and others added 11 commits July 31, 2023 21:36

Merge branch 'main' into watch-resource-with-selector

bfeb418

Merge branch 'main' into watch-resource-with-selector

c78ccf3

Merge remote-tracking branch 'origin/main' into watch-resource-with-s…

02e1998

…elector

Merge branch 'main' into watch-resource-with-selector

bd9b27e

refactor: move namespace labels to struct field

4ccc2bd

Signed-off-by: Den Tsou <den3tsou@gmail.com>

Merge branch 'main' into watch-resource-with-selector

b0c8407

Signed-off-by: Den <6628668+den3tsou@users.noreply.github.com>

resolve the issue casued by resolving conflicts

2360d73

Signed-off-by: Den Tsou <den3tsou@gmail.com>

chore: add test for gateway

5fd511a

Signed-off-by: Den Tsou <den3tsou@gmail.com>

Merge branch 'main' into watch-resource-with-selector

2db4634

chore: clean up some todo and add a comment

d401440

Signed-off-by: Den Tsou <den3tsou@gmail.com>

arkodg reviewed Aug 24, 2023

View reviewed changes

den3tsou and others added 8 commits August 28, 2023 16:55

fix: fix typos

fdef578

Signed-off-by: Den Tsou <den3tsou@gmail.com>

Merge branch 'main' into watch-resource-with-selector

faa6e24

fix: fix the lint errors

8e5e421

Signed-off-by: Den Tsou <den3tsou@gmail.com>

Merge branch 'main' into watch-resource-with-selector

06f7de0

Signed-off-by: Den <6628668+den3tsou@users.noreply.github.com>

refactor: revert back to get namespace from caller

f5bf23a

Signed-off-by: Den Tsou <den3tsou@gmail.com>

test: add a integration test for the change

9897b46

Signed-off-by: Den Tsou <den3tsou@gmail.com>

Merge branch 'main' into watch-resource-with-selector

7ecfe13

den3tsou added 3 commits September 5, 2023 19:58

Merge branch 'main' into watch-resource-with-selector

716ccdc

Merge branch 'main' into watch-resource-with-selector

f6655b3

Merge branch 'main' into watch-resource-with-selector

4e27b7c

arkodg approved these changes Sep 12, 2023

View reviewed changes

arkodg requested review from a team, LanceEa, chauhanshubham and kflynn and removed request for a team September 12, 2023 20:56

Alice-Lilith approved these changes Sep 12, 2023

View reviewed changes

Alice-Lilith merged commit 373ef32 into envoyproxy:main Sep 12, 2023

den3tsou deleted the watch-resource-with-selector branch September 13, 2023 08:09

den3tsou mentioned this pull request Sep 13, 2023

chore: Add kubebuilder directive to setup default value #1886

Closed

	err, "fail to get Namespace",
	err, "failed to get Namespace labels",

Conversation

den3tsou commented Jul 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

den3tsou Aug 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

den3tsou commented Sep 3, 2023

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

arkodg commented Sep 12, 2023

Uh oh!

Alice-Lilith left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

arkodg Jul 18, 2023 •

edited

Loading

arkodg Jul 26, 2023 •

edited

Loading

codecov bot commented Jul 19, 2023 •

edited

Loading

arkodg Aug 24, 2023 •

edited

Loading

den3tsou Aug 26, 2023 •

edited

Loading