Implements Gateway Controller #3805

danehans · 2021-06-13T19:03:21Z

Adds an initial Gateway controller implementation.

cmd/contour/serve.go: Updates NewGatewayController() to accept a gatewayclass controller string. ControllerName is now a required param, so the nil check is removed.
examples/*: Updates examples for Gateway support. Note that gateway/gatewayclass RBAC added the 'update' verb to support adding/removing the finalizer.
internal/controller/gateway.go: Adds a controller to reconcile Gateway objects owned by Contour. The controller validates, finalizes the referenced gatewayclass (if needed), and updates the status of managed Gateway objects.
internal/gateway/*: Adds functions for managing gateway objects.
internal/k8s/informers.go: Updates RBAC for managing status of Gateway objects.
internal/slice/*: Introduces a slice package for adding/removing a string from a []string.
internal/status/*: Adds support for managing Gateway conditions.
internal/validation/*: Adds initial support for validating Gateway objects.
internal/validation/gatewayclass_test.go Changes the scope of existing variables to support Gateway tests.
pkg/config/parameters_test.go Updates controllerName from optional to required.
test/scripts/install-contour-working.sh: Adds the controller name of the test gatewayclass to the contour config.

Signed-off-by: Daneyon Hansen daneyonhansen@gmail.com

codecov · 2021-06-13T19:07:47Z

Codecov Report

Merging #3805 (74caf31) into main (b2c7b00) will decrease coverage by 1.09%.
The diff coverage is 41.28%.

❗ Current head 74caf31 differs from pull request most recent head 14464ad. Consider uploading reports for the commit 14464ad to get more accurate results

@@            Coverage Diff             @@
##             main    #3805      +/-   ##
==========================================
- Coverage   75.81%   74.72%   -1.10%     
==========================================
  Files         107      110       +3     
  Lines        7340     7537     +197     
==========================================
+ Hits         5565     5632      +67     
- Misses       1654     1780     +126     
- Partials      121      125       +4

Impacted Files	Coverage Δ
cmd/contour/serve.go	`10.74% <0.00%> (ø)`
internal/controller/gateway.go	`0.00% <0.00%> (ø)`
internal/controller/gatewayclass.go	`0.00% <ø> (ø)`
internal/k8s/informers.go	`0.00% <ø> (ø)`
internal/status/gatewayconditions.go	`42.85% <42.85%> (ø)`
internal/status/routeconditions.go	`31.88% <66.66%> (ø)`
internal/status/cache.go	`89.13% <82.35%> (-4.99%)`	⬇️
internal/status/gatewayclassconditions.go	`89.28% <89.28%> (ø)`
internal/dag/gatewayapi_processor.go	`93.57% <90.24%> (-0.33%)`	⬇️
internal/slice/slice.go	`100.00% <100.00%> (ø)`
... and 3 more

internal/slice/slice.go

internal/slice/slice_test.go

internal/validation/gateway.go

danehans · 2021-06-16T17:17:07Z

#3812 created to track the need for gateway controller integration tests.

youngnick

This is mostly looking pretty good, but I think that we have to use the Scheduled Condition, not just remove it. Scheduled means that the resource is valid, has been accepted for processing, and infrastructure is being created. Ready means that infrastructure is created and ready to route traffic.

Obviously, until we are handling Envoy management, we can't guarantee Ready, but we should still be updating Scheduled. I guess the upstream docs could use some improvement to make this flow clearer?

youngnick · 2021-06-17T00:34:41Z

internal/controller/gateway.go

+	// The gateway is safe to process, so check if it's valid.
+	errs := validation.ValidateGateway(ctx, r.client, gw)
+	if errs != nil {
+		r.log.WithField("name", request.Name).WithField("namespace", request.Namespace).


We should also be setting the Scheduled Condition to status: false here, with a NotReconciled Reason.

The Gateway has been reconciled, but no infra is being provisioned since it's invalid. I will set Scheduled=false and use an InvalidGateway reason with the message indicating why the Gateway is invalid.

How does it work if not provisioning infra? Need to keep that use-case in mind.

If not provisioning infra, Scheduled is roughly equivalent to the Valid condition we use on HTTPProxy, it indicates that whether the object is syntactically valid. Should this be more than one Condition? Possibly, but I wasn't able to make that argument convincingly enough upstream.

youngnick · 2021-06-17T00:37:05Z

internal/controller/gateway.go

+		r.log.WithField("name", request.Name).WithField("namespace", request.Namespace).
+			Error("invalid gateway: ", int_errors.ParseFieldErrors(errs))
+	} else {
+		// TODO: Finalize the gateway when managed infrastructure is added.


We should set Scheduled to status: true at this point, with a Reason of Scheduled, I think? Reference is https://github.com/kubernetes-sigs/gateway-api/blob/16876c7362d58cae211e5599a6ef939ed1b859e9/apis/v1alpha2/gateway_types.go#L574-L614

Currently status.SyncGateway() sets Ready=true, but I'm updating that to Scheduled=true since the controller is not provisioning Envoy infra yet. I have also updated ensureClassFinalizer() to addClassFinalizer() so the func does not update the object. The object's status will now be sync'ed before being updated on the API server.

youngnick · 2021-06-17T00:43:09Z

internal/controller/gateway.go

+		// TODO: Ensure the gateway by creating manage infrastructure, i.e. the Envoy service.
+	}
+
+	if err := status.SyncGateway(ctx, r.client, gw, errs); err != nil {


Is this status Sync pattern common for controller-runtime controllers? In any case, we need to ensure that the Scheduled Condition is populated correctly (it signals that Contour has accepted the Gateway for processing and, eventually, started the process of creating infrastructure). We should add Ready with status: false when we add scheduled.

Then, Ready can be set to status: true when the infrastructure is provisioned.

I think we may need to change SyncGateway() to update Scheduled first maybe?

Is this status Sync pattern common for controller-runtime controllers?

It's a pattern I have seen with other controllers, essentially reconcile the object and calculate status before updating the object on the API server.

I think we may need to change SyncGateway() to update Scheduled first maybe?

Ack. I was still mentally operating in the operator world where the infra is provisioned. I have updated computeGatewayReadyCondition() to computeGatewayScheduledCondition() to manage the "Scheduled" condition type. When Envoy infra management is added, we can re-introduce computeGatewayReadyCondition() to calculate the "Ready" condition type.

internal/status/gateway.go

stevesloka

How do we manage setting status from the result of the DAG build vs the controller? Seems like we need to move the status setting to the DAG processor like it is today, and leave the gateway controller to manage the Envoy instances (if desired).

stevesloka · 2021-06-17T17:08:49Z

internal/validation/gateway.go

+}
+
+// validateGatewayNamespace validates whether other gateways exist in the namespace of gw.
+func validateGatewayNamespace(ctx context.Context, cli client.Client, gw *gatewayapi_v1alpha1.Gateway, path *field.Path) field.ErrorList {


Right now all validation is done in the DAG builder processor. How do we manage validation here on top of validation there? There are a bunch of bits that could be pulled out and pushed upstream, however, other things, like specific configuration items can't have generic validation and need to set status.

IMO validation is one of the first steps that a controller should perform on the object it reconciles. If utilizing the controller-runtime caches is the long-term direction, I think it's best to not add any more functionality to the DAG. As-is, the DAG will continue to perform validation, status mgt, etc. for xRoutes and Gateway listeners. The controller-runtime controllers will do the same for the objects they reconcile. When the xRoute controllers are implemented, we migrate the validation, status mgt, etc. from the Gateway API DAG processor.

I wouldn't go down that road right now, I'd let the DAG builder validate its stuff and keep everything together.

To me, this new controller should just manage Envoy instances, everything else should stay consistent as-is.

But don't let me distract, if other's don't agree then go with that. =)

I think this is a good chance to start pulling functionality out of the DAG, and decomposing it a bit. I think of Contour as having three stages: Ingestion of objects into the DAG, the DAG run, and then export to Envoy. I think that it's okay to have some validation at the ingestion stage, as long as we keep in mind what that means for the DAG. It's just a data model, and if we can split complexity out of it, it will be easier to code and maintain.

We'll need to either process a full dag to find some of the validation errors. One simple one is a missing/invalid service reference should set status on the Gateway. Probably for now we'll need to have the Gateway Controller get feedback from the result of a dag run and see if errors exist that it cares about to set proper status.

danehans · 2021-06-17T17:27:42Z

How do we manage setting status from the result of the DAG build vs the controller? Seems like we need to move the status setting to the DAG processor like it is today, and leave the gateway controller to manage the Envoy instances (if desired).

@stevesloka I thought we discussed moving away from the DAG and start utilizing the runtime controllers cache? @skriss did you mention that you were going to explore this in more detail? If the intent is to transition to the runtime cache, I thought it makes sense to have the controller manage GatewayClass/Gateway status and migrate the DAG's existing xRoute status management to xRoute controllers in the future.

stevesloka · 2021-06-17T17:34:44Z

I thought we discussed moving away from the DAG and start utilizing the runtime controllers cache?

The current DAG build is critical to how Contour works. Moving away from that is a core fundamental change to Contour. It's doable but needs a ton of work + overhaul to the core internals.

We talked I think about using the controller cache as opposed to our current custom caching, but still, the DAG needs to stay.

youngnick · 2021-06-18T00:42:15Z

I agree that the DAG needs to stay, but I think that filtering objects before they go in is pretty reasonable (which is what pre-DAG validation logic will do).

stevesloka · 2021-06-18T23:59:11Z

prefiltering

We also need to be careful to make sure if a cluster is running and something causes the Gateway to become invalid for one specific reason (say an invalid service), that we still keep the Gateway in the Contour cache so that we don't break all other routes that would be valid.

youngnick

Some small things, but overall I think this is okay. I'll leave to @stevesloka to give the final approval.

youngnick · 2021-06-24T00:51:24Z

internal/dag/gatewayapi_processor.go

+	}
+
+	// See if the referenced gatewayclass is admitted.
+	gcAdmitted := false


So, this needs to be here because we don't update status on admission? If so, could you put a comment to that effect, maybe with a TODO to do something about it?

youngnick · 2021-06-24T01:04:51Z

internal/dag/gatewayapi_processor.go

 	if p.source.gateway == nil {
-		p.Error("Gateway is not defined!")


Maybe the error should say something like "No gateway pointing to the GatewayClass whatever could be found"?

internal/featuretests/v3/gatewayapi_test.go

youngnick · 2021-06-24T01:09:40Z

internal/status/cache.go

-		entries:      make(map[string]map[types.NamespacedName]CacheEntry),
+		proxyUpdates:   make(map[types.NamespacedName]*ProxyUpdate),
+		gatewayRef:     gateway,
+		gatewayUpdates: make(map[types.NamespacedName]*GatewayConditionsUpdate),


The base Gateway status is actually pretty simple, but the Listener status is not. It's where all the Route stuff shows up, currently, so I think that having a way to distinguish between those is good.

youngnick · 2021-06-24T01:09:54Z

internal/status/cache.go

-		entries:      make(map[string]map[types.NamespacedName]CacheEntry),
+		proxyUpdates:   make(map[types.NamespacedName]*ProxyUpdate),
+		gatewayRef:     gateway,
+		gatewayUpdates: make(map[types.NamespacedName]*GatewayConditionsUpdate),


I think that we should get this working and come back for code duplication later.

danehans · 2021-06-24T16:12:35Z

So, this needs to be here because we don't update status on admission?

@youngnick that is the gatewayclass referenced by the gateway. If a gateway references a gatewayclass that is not admitted, then the gateway is considered invalid and the appropriate status condition is surfaced.

danehans · 2021-06-24T17:22:37Z

e2e (kubernetes:n-2 failure due to:

object is being deleted: gateways.networking.x-k8s.io "contour" already exists

e2e tests should consider appending a UID to gateway names for each test to avoid this issue. Gateways/GatewayClasses will use finalizers so deletion may take time, especially as Contour manages Envoy infra.

stevesloka

/lgtm to get this in and unblock a bunch of other issues once we get CI passing.

skriss · 2021-06-24T19:47:45Z

e2e (kubernetes:n-2 failure due to:
object is being deleted: gateways.networking.x-k8s.io "contour" already exists
e2e tests should consider appending a UID to gateway names for each test to avoid this issue. Gateways/GatewayClasses will use finalizers so deletion may take time, especially as Contour manages Envoy infra.

The plan is to move to having each test create a Gateway in its own namespace, to avoid test pollution like this. Requires #3803 to be able to easily update Contour config to point to the right gateway.

youngnick · 2021-06-25T05:32:27Z

Only failing CI is the codecov one, I think this is good to merge @danehans.

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

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

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

skriss · 2021-06-28T22:35:17Z

Looks like we've got some merge conflicts; @stevesloka were you going to take this one over?

stevesloka · 2021-06-28T23:34:48Z

Yup I can figure this one out @skriss.

youngnick · 2021-06-30T05:14:35Z

@stevesloka if you can clear the merge conflicts and CI is still passing, then I think this one is good to go for v1.17?

stevesloka · 2021-06-30T18:01:21Z

@youngnick nope there's an issue with finalizers. I'd like to pick this apart into pieces so we can make sure to test everything and find the flake. Personally, I'd delay the release, but need to chat about that. Otherwise would land in 1.18.

stevesloka · 2021-07-02T20:46:46Z

Implemented in #3858, closing and will follow the new PR for updates.

danehans requested a review from a team as a code owner June 13, 2021 19:03

danehans requested review from stevesloka and sunjayBhatia and removed request for a team June 13, 2021 19:03

danehans self-assigned this Jun 13, 2021

danehans added area/gateway-api Issues or PRs related to the Gateway (Gateway API working group) API. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 13, 2021

danehans added this to the 1.17.0 milestone Jun 13, 2021

danehans added this to 1.17 release in Contour Project Board Jun 13, 2021

sunjayBhatia reviewed Jun 14, 2021

View reviewed changes

internal/slice/slice.go Show resolved Hide resolved

sunjayBhatia reviewed Jun 14, 2021

View reviewed changes

internal/slice/slice_test.go Show resolved Hide resolved

sunjayBhatia reviewed Jun 14, 2021

View reviewed changes

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

danehans force-pushed the gw_impl branch 3 times, most recently from 6b45a4e to 83d4841 Compare June 15, 2021 18:42

danehans marked this pull request as draft June 16, 2021 00:19

danehans force-pushed the gw_impl branch from 83d4841 to f4f2e07 Compare June 16, 2021 16:49

danehans marked this pull request as ready for review June 16, 2021 17:04

danehans mentioned this pull request Jun 16, 2021

Adds Gateway Controller Integration Tests #3812

Closed

youngnick suggested changes Jun 17, 2021

View reviewed changes

stevesloka reviewed Jun 17, 2021

View reviewed changes

danehans force-pushed the gw_impl branch 2 times, most recently from fac6fbd to dcac033 Compare June 17, 2021 18:15

danehans marked this pull request as draft June 19, 2021 00:18

danehans force-pushed the gw_impl branch from 4bd950a to 004629c Compare June 23, 2021 01:28

youngnick approved these changes Jun 24, 2021

View reviewed changes

danehans force-pushed the gw_impl branch from bf67eca to 263d8e7 Compare June 24, 2021 16:50

stevesloka approved these changes Jun 24, 2021

View reviewed changes

danehans and others added 13 commits June 25, 2021 09:02

Implements Gateway Controller

1eb13e7

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

Adds Update Verb to Gateway RBAC

344a286

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

Adds Gateway Deletion and Expands Validation

4fedf59

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

Updates Status From Ready to Scheduled

22e95a3

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

Improves Code Test Coverage

d3c02ac

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

Refactors to Use GatewayAPIProcessor

74e5986

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

Simplifies GatewayAPIProcessor Refactor

0f5b842

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

Splits status condition files

e321691

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

Resolves youngnick Feedback

c8b3aa6

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

add ControllerName to Contour config when running GatewayAPI tests.

6302f51

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

update gateway e2e tests to pass GatewayClass and wait for deletion.

b8d9ca1

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

move gatewayclass creation to a seperate var that's not global

ee94d81

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

Remove global e2e gatewayclass and fixes stevesloka godocs

14464ad

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

danehans force-pushed the gw_impl branch from 263d8e7 to 14464ad Compare June 25, 2021 22:51

stevesloka mentioned this pull request Jul 1, 2021

GatewayAPI: Gateway Implementation #3858

Merged

stevesloka closed this Jul 2, 2021

stevesloka mentioned this pull request Jul 6, 2021

Gateway/GatewayClass Finalizers #3866

Closed

2 tasks

		if p.source.gateway == nil {
		p.Error("Gateway is not defined!")

Implements Gateway Controller #3805

Implements Gateway Controller #3805

Conversation

danehans commented Jun 13, 2021 • edited

codecov bot commented Jun 13, 2021 • edited

Codecov Report

danehans commented Jun 16, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevesloka 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

Choose a reason for hiding this comment

danehans commented Jun 17, 2021 • edited

stevesloka commented Jun 17, 2021 • edited

youngnick commented Jun 18, 2021

stevesloka commented Jun 18, 2021

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

danehans commented Jun 24, 2021

danehans commented Jun 24, 2021

stevesloka left a comment

Choose a reason for hiding this comment

skriss commented Jun 24, 2021

youngnick commented Jun 25, 2021

skriss commented Jun 28, 2021

stevesloka commented Jun 28, 2021

youngnick commented Jun 30, 2021

stevesloka commented Jun 30, 2021

stevesloka commented Jul 2, 2021

danehans commented Jun 13, 2021 •

edited

codecov bot commented Jun 13, 2021 •

edited

danehans commented Jun 17, 2021 •

edited

stevesloka commented Jun 17, 2021 •

edited