migrate to controller-runtime controllers for k8s watch events #3510

stevesloka · 2021-03-23T15:39:53Z

This replaces the internal informers and moves to the controller-runtime controllers
setting up Contour to migrate over to the controller-runtime caches.

Note: This only currently applies to gateway-api resources. Once a GA version of controller-runtime is released, we can migrate the rest of the resources over.

Updates #2683

Signed-off-by: Steve Sloka slokas@vmware.com

codecov · 2021-03-23T15:45:13Z

Codecov Report

Merging #3510 (041e9c7) into main (7ed38de) will decrease coverage by 0.84%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #3510      +/-   ##
==========================================
- Coverage   77.06%   76.21%   -0.85%     
==========================================
  Files         100      103       +3     
  Lines        7116     7195      +79     
==========================================
  Hits         5484     5484              
- Misses       1511     1590      +79     
  Partials      121      121

Impacted Files	Coverage Δ
cmd/contour/serve.go	`10.92% <0.00%> (-0.35%)`	⬇️
internal/controller/gateway.go	`0.00% <0.00%> (ø)`
internal/controller/httproute.go	`0.00% <0.00%> (ø)`
internal/controller/tlsroute.go	`0.00% <0.00%> (ø)`

youngnick · 2021-03-30T04:21:46Z

Thanks for doing this Steve. It looks much more straightforward than I feared.

How does this fit into the overall "migrate everything to controller-runtime" plan? What's left to do after this?

stevesloka · 2021-04-01T21:34:29Z

How does this fit into the overall "migrate everything to controller-runtime" plan? What's left to do after this?

This essentially just replaces the informers with controller-runtime. To use the runtime caches, we'd need to totally swap out cache.go and figure out how to do the new filtering bits since there's a lot of logic there now that would need to be done somewhere else in some way. That's the more difficult part and would need a bunch of thought as to how to approach that problem.

danehans · 2021-04-01T21:37:42Z

@stevesloka needs a rebase.

danehans · 2021-04-01T21:44:47Z

cmd/contour/serve.go

+	// Setup a Manager
+	mgr, err := manager.New(controller_config.GetConfigOrDie(), manager.Options{})
+	if err != nil {
+		log.Fatal(err, "unable to set up overall controller manager")


I suggest dropping"overall", maybe something like:
"unable to create contour manager" or "unable to create contour server manager".

I dropped "overall".

internal/dag/cache.go

danehans · 2021-04-01T22:03:14Z

internal/k8s/cache/backendpolicy.go

+func (r *backendPolicyReconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) {
+
+	// Fetch the BackendPolicy from the cache.
+	gateway := &gatewayapi_v1alpha1.BackendPolicy{}


I find this var name misleading, maybe something like policy or be.

I'm the best at copy/paste. =)

internal/k8s/cache/endpoint.go

danehans · 2021-04-01T22:14:13Z

internal/k8s/cache/gateway.go

+	if err != nil {
+		return nil, err
+	}
+	if err := c.Watch(&source.Kind{Type: &gatewayapi_v1alpha1.Gateway{}}, &handler.EnqueueRequestForObject{}); err != nil {


Should gateway resources be filtered using a predicate? For example, only enqueue gateways that match Contour's gateway config and reference a GatewayClass owned by contour or contour-operator?

I'd say "and/or" since the GatewayClass is not required, but aside from that I agree.

internal/k8s/cache/namespace.go

internal/k8s/cache/tlsroute.go

danehans

I provided a few bits of feedback throughout.

youngnick

I'm not a fan of this much boilerplate, but it seems like it's a necessary step on getting us closer to using controller-runtime fully, including caches.

A small non-blocking suggestion about keeping doServe small as well.

Thanks @stevesloka for getting this done.

cmd/contour/serve.go

youngnick · 2021-04-06T07:03:51Z

internal/k8s/cache/endpoint.go

+
+// NewEndpointController creates the endpoint controller from mgr. The controller will be pre-configured
+// to watch for Endpoint objects across all namespaces.
+func NewEndpointController(mgr manager.Manager, eventHandler cache.ResourceEventHandler, log logrus.FieldLogger) (controller.Controller, error) {


Tangentially related: We're also going to have to figure out what to do about EndpointSlice at some point, since Endpoints will go away (I think) in the future.

Yup I agree: #2696

youngnick · 2021-04-06T07:05:31Z

internal/k8s/cache/gateway.go

+	if err != nil {
+		return nil, err
+	}
+	if err := c.Watch(&source.Kind{Type: &gatewayapi_v1alpha1.Gateway{}}, &handler.EnqueueRequestForObject{}); err != nil {


I'd say "and/or" since the GatewayClass is not required, but aside from that I agree.

youngnick · 2021-04-06T07:08:48Z

internal/k8s/converter.go

@@ -37,34 +37,14 @@ type DynamicClientHandler struct {
 }

 func (d *DynamicClientHandler) OnAdd(obj interface{}) {
-	obj, err := d.Converter.FromUnstructured(obj)


Is it guaranteed that we'll never get Unstructured? If so, neat.

Yeah we shouldn't since the controller-runtime is doing all of this for us now.

sunjayBhatia

I think the cmd/contour/ingressstatus.go file also sets up some informers so it should probably be updated as well to use the same pattern

I guess I would echo the sentiments about trying to deduplicate some of the repeated bits in the controller constructors/reconcile methods, if I need to make a change it would be great to do it one place rather than many since its pretty much the same code

maybe a constructor that takes a type and for any slight differences between the controllers for different types there could be an optional mutator function or something? not sure how much would be different between different controllers other than different predicates/filters

sunjayBhatia · 2021-04-06T21:06:01Z

cmd/contour/serve.go

-		if err != nil {
-			log.WithError(err).WithField("resource", r).Fatal("failed to create informer")
-		}
+	//err = contour_api_v1.AddToScheme(mgr.GetScheme())


Some extra content here?

danehans · 2021-04-06T21:35:07Z

cmd/contour/serve.go

@@ -231,7 +243,7 @@ func doServe(log logrus.FieldLogger, ctx *serveContext) error {

 	// Before we can build the event handler, we need to initialize the converter we'll
 	// use to convert from Unstructured.
-	converter, err := k8s.NewUnstructuredConverter()
+	converter, err := k8s.NewUnstructuredConverter(mgr.GetScheme())


Does it make more sense to create the scheme with the gvk's that we care about and pass the scheme in as a manager.Option?

Yeah that's sort of what was happening here:

contour/internal/k8s/scheme.go

Line 27 in 4885db6

func NewContourScheme(s *runtime.Scheme) error {

skriss

@stevesloka could you provide some additional context on what the end state for this looks like? I'm having a little trouble giving feedback on approach since I'm not sure where we're trying to get to. My understanding is that we're already using some controller-runtime libraries for the informers (see https://github.com/projectcontour/contour/blob/main/internal/k8s/clients.go#L36 and https://github.com/projectcontour/contour/blob/main/internal/k8s/clients.go#L95). What is the role of these reconcilers going to be in the target state? Is that going to change how the DAG is processed?

stevesloka · 2021-04-07T16:17:36Z

The goal of this change was to allow us to manage separate controllers for different objects, mainly the gatewayapi work. We have cleaner hooks now to work on changes to gatewayclasses, gateways, etc which may not be DAG relevant (i.e. a new gateway might cause an envoy service to be created, etc).

There has been discussion of removing the internal k8s cache and moving to a controller-runtime cache, but that needs some more thoughts as to how to approach since we filter on the k8s cache so much today.

stevesloka · 2021-04-07T16:24:11Z

I guess I would echo the sentiments about trying to deduplicate some of the repeated bits in the controller constructors/reconcile methods, if I need to make a change it would be great to do it one place rather than many since its pretty much the same code

@sunjayBhatia yup I agree that there are duplicated bits at the moment, but I think when @danehans starts to add some of the "operator" logic that might make more sense. We've overloaded the cahce.go file to do so much, splitting out more I think would be helpful.

Things we could do would be to move the secret validation out, the logic to know if a service is referenced from an ingress object, etc. That way we'd have a cleaner approach to look at when making changes and the k8s cache turns into just a cache, not a cache w/logic.

skriss · 2021-04-07T17:38:28Z

The goal of this change was to allow us to manage separate controllers for different objects, mainly the gatewayapi work. We have cleaner hooks now to work on changes to gatewayclasses, gateways, etc which may not be DAG relevant (i.e. a new gateway might cause an envoy service to be created, etc).

So is this primarily useful only if we decide to move the gatewayclass/gateway controllers into Contour? If so, I'm wondering if it'd make sense to keep this in the experimental branch that you/@danehans are going to be working on for now until we make a decision on path forward there. Otherwise, this seems like a fair amount of overhead to add to Contour if all the controllers are doing is passing events through to the event handler since the informers are capable of doing that today. Maybe we can spend some time talking through this one in the maintainers meeting.

stevesloka · 2021-04-08T14:53:19Z

Aside from GatewayAPI work, this allows us to further clean up cache.go file to not do so much, splitting out into different places which would also be helpful.

We do need to discuss if we do want to move to controller-runtime, doing that would be best done in steps vs a drastic change.

I think fine to keep experimental until we decide the above steps.

github-actions · 2021-04-23T00:19:32Z

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 90 days.

stevesloka · 2021-05-07T20:34:15Z

rebased @danehans 🚀

danehans · 2021-05-07T20:37:29Z

@stevesloka the updated PR no longer starts the controller-runtime manager. See this for the fix.

stevesloka · 2021-05-07T20:55:01Z

@danehans ahh good catch! Interesting that the events still worked? The integration tests all passed fine.

youngnick

This still LGTM, I'd like to get this one in so we can unblock further Gateway API work.

cmd/contour/serve.go

internal/k8s/cache/gateway.go

skriss · 2021-05-11T14:11:48Z

Looks like you hit #3660 in the E2E run (fixed by #3662). We can ignore since it's not a required check yet.

internal/reconciler/gateway.go

sunjayBhatia · 2021-05-11T15:15:05Z

internal/reconciler/gateway.go

+
+// NewGatewayController creates the gateway controller from mgr. The controller will be pre-configured
+// to watch for Gateway objects across all namespaces.
+func NewGatewayController(mgr manager.Manager, eventHandler cache.ResourceEventHandler, log logrus.FieldLogger) (controller.Controller, error) {


looks like 90% of code between the controller files is the same, seems like a future refactor could be to have a controller factory that creates a resource specific controller given a type and a few other fields so we don't have to duplicate?

do we foresee needing different logic for different resources?

Yup I think we will for sure need different logic. We'll need to manage finalizers as well as Envoy instances, etc.

cmd/contour/serve.go

danehans · 2021-05-11T17:11:36Z

Yeah I think reconciler makes sense.

@stevesloka kubebuilder uses "controller" to name the pkg. You may want to consider following suit.

stevesloka · 2021-05-11T17:39:52Z

Ok all fixed up I think and also renamed to controller. @danehans / @skriss

skriss

LGTM pending CI.

skriss · 2021-05-11T18:00:55Z

sorry, merged another PR and caused a merge conflict.

Signed-off-by: Steve Sloka <slokas@vmware.com>

Moves from informers to controller-runtime for gateway-api processing. Signed-off-by: Steve Sloka <slokas@vmware.com>

Signed-off-by: Steve Sloka <slokas@vmware.com>

stevesloka requested a review from a team as a code owner March 23, 2021 15:39

stevesloka requested review from danehans and skriss and removed request for a team March 23, 2021 15:39

stevesloka force-pushed the controllerRuntime branch from 5c18ee2 to f42ae95 Compare March 23, 2021 16:29

danehans reviewed Apr 1, 2021

View reviewed changes

internal/dag/cache.go Outdated Show resolved Hide resolved

danehans reviewed Apr 1, 2021

View reviewed changes

internal/k8s/cache/endpoint.go Outdated Show resolved Hide resolved

danehans reviewed Apr 1, 2021

View reviewed changes

internal/k8s/cache/namespace.go Outdated Show resolved Hide resolved

danehans reviewed Apr 1, 2021

View reviewed changes

internal/k8s/cache/tlsroute.go Outdated Show resolved Hide resolved

danehans suggested changes Apr 1, 2021

View reviewed changes

stevesloka force-pushed the controllerRuntime branch from f42ae95 to 0a3a8f9 Compare April 5, 2021 16:21

youngnick approved these changes Apr 6, 2021

View reviewed changes

stevesloka requested review from youngnick and danehans April 6, 2021 20:10

sunjayBhatia reviewed Apr 6, 2021

View reviewed changes

danehans reviewed Apr 6, 2021

View reviewed changes

skriss reviewed Apr 6, 2021

View reviewed changes

stevesloka force-pushed the controllerRuntime branch from 4885db6 to 210c825 Compare April 7, 2021 16:29

stevesloka force-pushed the controllerRuntime branch from 52336c3 to e3a073b Compare May 7, 2021 20:30

This was referenced May 10, 2021

Implements GatewayClass Controller #3659

Merged

Updates Examples for Gateway API Support #3664

Merged

youngnick approved these changes May 11, 2021

View reviewed changes

skriss requested changes May 11, 2021

View reviewed changes

stevesloka force-pushed the controllerRuntime branch from 8008743 to 5017dda Compare May 11, 2021 13:58

skriss reviewed May 11, 2021

View reviewed changes

internal/reconciler/gateway.go Outdated Show resolved Hide resolved

skriss mentioned this pull request May 11, 2021

Update k8s deps for 1.21.0 release #3581

Merged

sunjayBhatia reviewed May 11, 2021

View reviewed changes

skriss reviewed May 11, 2021

View reviewed changes

cmd/contour/serve.go Show resolved Hide resolved

skriss approved these changes May 11, 2021

View reviewed changes

stevesloka added 8 commits May 11, 2021 14:07

fix merge conflict

8e3c66b

Signed-off-by: Steve Sloka <slokas@vmware.com>

Move to controller-runtime for gateway-api types.

e7f0c93

Moves from informers to controller-runtime for gateway-api processing. Signed-off-by: Steve Sloka <slokas@vmware.com>

start manager

323fa63

Signed-off-by: Steve Sloka <slokas@vmware.com>

review comments from @skriss

bd76479

Signed-off-by: Steve Sloka <slokas@vmware.com>

add delete to the 'IsNotFound' case

ecdd483

Signed-off-by: Steve Sloka <slokas@vmware.com>

remove finalizers check since not in use at the moment

6d515e7

Signed-off-by: Steve Sloka <slokas@vmware.com>

handle delete with name/namespace

07a7f63

Signed-off-by: Steve Sloka <slokas@vmware.com>

review comments

041e9c7

Signed-off-by: Steve Sloka <slokas@vmware.com>

stevesloka force-pushed the controllerRuntime branch from 63caf0f to 041e9c7 Compare May 11, 2021 18:09

stevesloka merged commit db368dc into projectcontour:main May 11, 2021

stevesloka deleted the controllerRuntime branch May 11, 2021 19:19

danehans mentioned this pull request May 24, 2021

Add Controller Runtime Reconcile Tests #3705

Closed

migrate to controller-runtime controllers for k8s watch events #3510

migrate to controller-runtime controllers for k8s watch events #3510

Conversation

stevesloka commented Mar 23, 2021 • edited

codecov bot commented Mar 23, 2021 • edited

Codecov Report

youngnick commented Mar 30, 2021

stevesloka commented Apr 1, 2021

danehans commented Apr 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans left a comment

Choose a reason for hiding this comment

youngnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunjayBhatia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

stevesloka commented Apr 7, 2021

stevesloka commented Apr 7, 2021

skriss commented Apr 7, 2021

stevesloka commented Apr 8, 2021

github-actions bot commented Apr 23, 2021

stevesloka commented May 7, 2021

danehans commented May 7, 2021

stevesloka commented May 7, 2021

youngnick left a comment

Choose a reason for hiding this comment

skriss commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans commented May 11, 2021

stevesloka commented May 11, 2021

skriss left a comment

Choose a reason for hiding this comment

skriss commented May 11, 2021

stevesloka commented Mar 23, 2021 •

edited

codecov bot commented Mar 23, 2021 •

edited