Allow limiting Contour to watch certain namespaces #5214

nsimons · 2023-03-22T09:21:17Z

This PR is a continuation of #5002 with added checks, test cases and documentation.

Signed-off-by: Niklas Simons [email protected]

codecov · 2023-03-22T09:45:30Z

Codecov Report

Merging #5214 (14adedc) into main (180ebd3) will decrease coverage by 0.12%.
The diff coverage is 5.12%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5214      +/-   ##
==========================================
- Coverage   78.30%   78.18%   -0.12%     
==========================================
  Files         138      138              
  Lines       18450    18480      +30     
==========================================
+ Hits        14447    14449       +2     
- Misses       3727     3755      +28     
  Partials      276      276

Impacted Files	Coverage Δ
cmd/contour/servecontext.go	`81.85% <0.00%> (-1.71%)`	⬇️
cmd/contour/serve.go	`19.97% <6.66%> (-0.26%)`	⬇️

tsaarni

Looks very good to me 👍 Added few questions inline.

tsaarni · 2023-03-24T12:09:22Z

test/e2e/httpproxy/httpproxy_test.go

@@ -444,7 +444,7 @@ descriptors:
 				}
 			})

-			f.NamespacedTest("root-ns-crd", testRootNamespaces(rootNamespaces))
+			f.NamespacedTest("nonroot-ns-crd", testRootNamespaces(rootNamespaces))


I was first bit confused about why the root to nonroot change 😅 At first sight, it seemed to go against the naming convention used in the test cases. But I see it: The namespace argument of NamespacedTest() has always been used to test the negative case i.e. root proxy is in non-root namespace.

I know this is old existing test case, not part of this PR, but maybe a tiny comment could be added to clarify?

Yeah it was a bit confusing for me at first as well, hence the renaming. I'll refactor it further so that it's no longer a magic string.

tsaarni · 2023-03-24T12:22:03Z

test/e2e/httpproxy/namespaces_test.go

+			// Proxy in non-watched namespace should fail
+			deployEchoServer(f.T(), f.Client, nonWatchedNS, "echo")
+			p := newEchoProxy("proxy", nonWatchedNS)
+			_, ok := f.CreateHTTPProxyAndWaitFor(p, e2e.HTTPProxyNotReconciled)


I wonder if this would catch a failure? Or will it always likely return ok=true immediately, since newly created resource would have NotReconciled by default, before being updated by the controller? It might be a race between test and the controller.

When the default state is also the expected state, I guess test should poll until value is considered stable. I do not see such variant of wait.PollNnn() in k8s.io/apimachinery/pkg/util/wait and I'm quite uncertain if it is worth the effort to even look for, since the "real test" would be anyways to deploy with Role/RoleBinding RBAC. The lack of status updates is of course a good secondary indication that ClusterRoles were not used 😄

Good point, the resource would ideally always be NotReconciled because nobody would update it...

I suggest that we still have the negative test because otherwise the happy case is not that trustworthy. We can use wait.Poll with a sufficient interval parameter, since the function will automatically wait one interval before doing the first poll. But we also don't want the test case to run too long. I feel like the controller is fairly quick, so if we set the interval to let's say 10 seconds, we can be fairly certain that the resource should have updated by then (if the controller code was misbehaving). Of course, this is not fool proof but better than nothing (?).

examples/namespaced/kustomization.yaml

skriss

Thanks @nsimons, this looks pretty good to me, couple comments below

changelogs/unreleased/5214-nsimons-minor.md

site/content/docs/main/deploy-options.md

examples/namespaced/kustomization.yaml

test/e2e/httpproxy/namespaces_test.go

skriss · 2023-04-05T18:32:49Z

Thanks for the updates @nsimons - I think this looks good, would just like to poke around with testing a little bit more, but we should be able to get this into the upcoming release

skriss · 2023-04-07T15:38:36Z

One note: if --watch-namespaces is specified and does not include the namespace in which Contour is installed, then Contour does not watch the Envoy service for LoadBalancerStatus and thus does not populate it on HTTPProxies/Ingresses/Gateways. This is a little bit tricky, since it's easy to imagine a scenario where a user wants to only watch e.g. namespace foo for config, but also wants loadBalancerIP to be populated on their routing resources. Any thoughts on the best way to handle this scenario?

nsimons · 2023-04-11T12:45:53Z

Hmm, right.
I don't know if there is any clean way to deal with this. The cache needs to cover the namespace where Envoy's service resides (same as Contour, or given by --envoy-service-namespace), otherwise the status updater will not get any events.

We could force the user to give the namespace in --watch-namespaces (similar as envoy client certificate and fallback cert), but if the user does not really care about it then it's a superfluous restriction. Or we can add a warning that LoadBalancerStatus is not populated if the namespace is not watched. I'm leaning towards the latter.

It depends which use cases we want to support. From the related issues I think the most common scenario will be that everything is located within the same namespace (Contour, Envoy, application). Another one that we can foresee is that Contour+Envoy is in an infra namespace and the app is somewhere else. In that case it's probably okay that Contour watches both infra and app namespaces, even if we don't expect any resources in infra ns.

skriss · 2023-04-11T20:47:21Z

Yeah, thinking some more I think it's fair to say that if you want the Envoy service to be watched, you need to include it. Let's just see if we can find a place to explicitly document this -- I'm thinking maybe deploy-options.md is the most logical place to cover it.

skriss · 2023-04-11T20:49:00Z

xref #5099 (comment) -- so looks like:

currently, this PR (Allow limiting Contour to watch certain namespaces #5214) and Reduce the memory footprint by optimizing Kubernetes informer cache #5099 are incompatible with each other
there are upcoming changes in controller-runtime that will break both, but not sure when the new version that includes those changes will be out

tsaarni · 2023-04-12T17:00:31Z

xref #5099 (comment) -- so looks like:

currently, this PR (Allow limiting Contour to watch certain namespaces #5214) and Reduce the memory footprint by optimizing Kubernetes informer cache #5099 are incompatible with each other

there are upcoming changes in controller-runtime that will break both, but not sure when the new version that includes those changes will be out

I did not find a way to achieve both goals with the current version. I submitted question kubernetes-sigs/controller-runtime#2273 asking for help.

I tested the main branch of controller-runtime and I can confirm it solves the problem. I stored a diff of my experiment here https://gist.github.com/tsaarni/94184af509664334e0b3350c9c0ac86a. The project is going through API refactoring and few changes were required around the code base.

tsaarni · 2023-04-13T16:22:25Z

I did not find a way to achieve both goals with the current version. I submitted question kubernetes-sigs/controller-runtime#2273 asking for help.

I got confirmation that it is not possible as of today. Though I'm not absolutely 100% certain if one could hack around the limitation, maybe one could reproduce some larger part of the builder logic on Contour side...

The API in main has not yet been settled and there will be another round of changes, but it looks like we will be able to have namespace restricted watches + transform functions in the final API.

skriss · 2023-04-13T16:26:54Z

So sounds like we may have to pick/choose between #5214 and #5099 for the upcoming Contour 1.25, and then plan to refactor later to support both. @tsaarni @nsimons what are your thoughts on priority between the two? From my perspective #5099 would be higher priority, but would like your take as well.

sunjayBhatia · 2023-04-13T23:26:49Z

So sounds like we may have to pick/choose between #5214 and #5099 for the upcoming Contour 1.25, and then plan to refactor later to support both. @tsaarni @nsimons what are your thoughts on priority between the two? From my perspective #5099 would be higher priority, but would like your take as well.

personally +1 on #5099 since that can have a wider benefit for users, but can be swayed as well

tsaarni · 2023-04-14T06:50:19Z

We discussed this with @nsimons. Maybe there could be one option that would allow doing both, with one limitation:

Background

Currently controller-runtime allows setting single namespace manager.Options{Namespace: "foo"} while still using the BuilderWithOptions() which is needed to implement the memory footprint optimization.

Proposal

In the 1st phase we could limit #5214 to single namespace. When controller-runtime releases the new API, we could have 2nd phase that allows multiple namespaces to be watched. Multiple namespaces can be useful since it allows "cross-namespace" features to work, even when Contour is not granted cluster roles.

Alternative 1: We could keep --watch-namespaces (plural) but have it print an error "currently only single namespace is allowed" if more than one namespace is given. In future, we can allow multiple namespaces without breaking the API.

Alternative 2: We could change it to --watch-namespace and introduce new command line option later, when (or if) watching multiple namespaces will be added.

For our own use case the limitation of single namespace would be completely OK. If we agree on the approach, @nsimons promised to make the changes to this PR on top of #5099, so we could work on the memory footprint issue first.

sunjayBhatia · 2023-04-17T16:42:14Z

Proposal

In the 1st phase we could limit #5214 to single namespace. When controller-runtime releases the new API, we could have 2nd phase that allows multiple namespaces to be watched. Multiple namespaces can be useful since it allows "cross-namespace" features to work, even when Contour is not granted cluster roles.

Alternative 1: We could keep --watch-namespaces (plural) but have it print an error "currently only single namespace is allowed" if more than one namespace is given. In future, we can allow multiple namespaces without breaking the API.

Alternative 2: We could change it to --watch-namespace and introduce new command line option later, when (or if) watching multiple namespaces will be added.

For our own use case the limitation of single namespace would be completely OK. If we agree on the approach, @nsimons promised to make the changes to this PR on top of #5099, so we could work on the memory footprint issue first.

I'm down with this proposal, and Alternative 1 (if we want to do 2 we could make the flag like --disable-feature flag which can be specified multiple times)

sunjayBhatia

relevant to this PR, this issue has some comments and links with details on the next controller-runtime release date and release notes: kubernetes-sigs/controller-runtime#2295

nsimons · 2023-05-15T08:49:38Z

Thanks Sunjay. I think it's better that we wait for controller-runtime 0.15.0 which contains the full multi-namespace support. Then we can merge this PR almost as-is.

It should be released quite soon as far as I understood. I started to bump and test the alpha version in a related PR.

tsaarni

LGTM 👍

skriss

LGTM, thanks @nsimons!

cmd/contour/serve.go

Signed-off-by: Niklas Simons <[email protected]>

Signed-off-by: Niklas Simons <[email protected]> Signed-off-by: shadi-altarsha <[email protected]>

Signed-off-by: Niklas Simons <[email protected]>

nsimons requested a review from a team as a code owner March 22, 2023 09:21

nsimons requested review from stevesloka and skriss and removed request for a team March 22, 2023 09:21

nsimons force-pushed the watch-namespaces branch from 666138d to 9efccd6 Compare March 22, 2023 09:39

tsaarni mentioned this pull request Mar 22, 2023

Allow limiting K8s watches on specific namespaces. #5002

Closed

tsaarni added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Mar 22, 2023

tsaarni reviewed Mar 24, 2023

View reviewed changes

nsimons force-pushed the watch-namespaces branch from 9efccd6 to 7309959 Compare March 27, 2023 14:06

skriss requested changes Apr 4, 2023

View reviewed changes

nsimons force-pushed the watch-namespaces branch 2 times, most recently from 5d7d132 to c3b749e Compare April 5, 2023 13:49

skriss requested a review from sunjayBhatia April 5, 2023 18:30

skriss mentioned this pull request Apr 5, 2023

Support namespace-scoped Contour instances via Gateway provisioner #5256

Closed

skriss mentioned this pull request Apr 7, 2023

Question about "installing multiple Contour instances" #4899

Closed

skriss mentioned this pull request Apr 11, 2023

Reduce the memory footprint by optimizing Kubernetes informer cache #5099

Merged

pnbrown mentioned this pull request Apr 20, 2023

Add an option to GatewayProvisioner CRD to watch subset of namespaces #5302

Closed

skriss added this to the 1.26.0 milestone Apr 26, 2023

sunjayBhatia reviewed May 5, 2023

View reviewed changes

nsimons mentioned this pull request May 15, 2023

Bump client-go, controller-runtime and related dependencies #5362

Merged

nsimons force-pushed the watch-namespaces branch from c3b749e to c12eb68 Compare May 26, 2023 06:45

nsimons requested a review from skriss May 26, 2023 07:10

tsaarni approved these changes May 26, 2023

View reviewed changes

sunjayBhatia self-requested a review May 26, 2023 17:16

skriss approved these changes May 26, 2023

View reviewed changes

sunjayBhatia reviewed May 31, 2023

View reviewed changes

cmd/contour/serve.go Outdated Show resolved Hide resolved

Allow limiting Contour to watch certain namespaces

14adedc

Signed-off-by: Niklas Simons <[email protected]>

nsimons force-pushed the watch-namespaces branch from c12eb68 to 14adedc Compare June 1, 2023 10:59

sunjayBhatia approved these changes Jun 1, 2023

View reviewed changes

sunjayBhatia merged commit dcc12d9 into projectcontour:main Jun 1, 2023

shadialtarsha pushed a commit to shadialtarsha/contour that referenced this pull request Jun 6, 2023

Allow limiting Contour to watch certain namespaces (projectcontour#5214)

283906d

Signed-off-by: Niklas Simons <[email protected]> Signed-off-by: shadi-altarsha <[email protected]>

shadialtarsha pushed a commit to shadialtarsha/contour that referenced this pull request Jun 6, 2023

Allow limiting Contour to watch certain namespaces (projectcontour#5214)

8bc1681

Signed-off-by: Niklas Simons <[email protected]>

lubronzhan mentioned this pull request Jan 12, 2024

Allow gatewayProvisioner to create contour that only watch limited na… #6073

Merged

Allow limiting Contour to watch certain namespaces #5214

Allow limiting Contour to watch certain namespaces #5214

Uh oh!

Conversation

nsimons commented Mar 22, 2023 • edited by skriss Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

tsaarni left a comment • 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!

tsaarni Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

nsimons Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

tsaarni Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

nsimons Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skriss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skriss commented Apr 5, 2023

Uh oh!

skriss commented Apr 7, 2023

Uh oh!

nsimons commented Apr 11, 2023

Uh oh!

skriss commented Apr 11, 2023

Uh oh!

skriss commented Apr 11, 2023

Uh oh!

tsaarni commented Apr 12, 2023

Uh oh!

tsaarni commented Apr 13, 2023

Uh oh!

skriss commented Apr 13, 2023

Uh oh!

sunjayBhatia commented Apr 13, 2023

Uh oh!

tsaarni commented Apr 14, 2023

Uh oh!

sunjayBhatia commented Apr 17, 2023

Uh oh!

sunjayBhatia left a comment

Choose a reason for hiding this comment

Uh oh!

nsimons commented May 15, 2023

Uh oh!

tsaarni left a comment

Choose a reason for hiding this comment

Uh oh!

skriss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nsimons commented Mar 22, 2023 •

edited by skriss

Loading

codecov bot commented Mar 22, 2023 •

edited

Loading

tsaarni left a comment •

edited

Loading