Implements GatewayClass Controller #3659
Conversation
danehans
requested review from
skriss and
sunjayBhatia
and removed request for
a team
|
When reviewing, I suggest filtering the commits to separate this PR from #3510. |
Codecov Report
@@ Coverage Diff @@
## main #3659 +/- ##
==========================================
- Coverage 76.20% 75.69% -0.51%
==========================================
Files 103 107 +4
Lines 7171 7288 +117
==========================================
+ Hits 5465 5517 +52
- Misses 1587 1650 +63
- Partials 119 121 +2
|
|
xref #3664 that updates deployment manifests/tooling. |
There was a problem hiding this comment.
This is a quick first pass, needs a rebase now that I just merged #3510. Also, would be super nice to pick this out into smaller PRs, one for the controller bits, one for status, one for validation, etc.
Also seems like many of the bits aren't tested (the Codecov is complaining all over the place which is making the review difficult).
internal/k8s/cache/gateway.go
Outdated
|
|
||
| // enqueueRequestForOwnedGateway returns an event handler that maps events to | ||
| // Gateway objects that reference a GatewayClass owned by Contour. | ||
| func (r *gatewayReconciler) enqueueRequestForOwnedGateway() handler.EventHandler { |
There was a problem hiding this comment.
Right now cache.go filters on the configured gateway (https://github.com/projectcontour/contour/blob/main/internal/dag/cache.go#L144). Do we need to have the additional checks if we're only ever going to be hard coded to one single gateway?
There was a problem hiding this comment.
@stevesloka enqueueRequestForOwnedGateway() filters events before they are given to the controller's EventHandler. This follows controller-runtime best practices for event filtering. Maybe the dag filter you reference should be removed in favor of this watch predicate, thoughts?
There was a problem hiding this comment.
It's not a problem to move the logic from where it's happening, just need to pick one and remove the other. Also, need to make sure we're filtering against the configured name/namespace of the gateway.
There was a problem hiding this comment.
@stevesloka ack. I think it makes sense to filter as close to the source as possible, so this PR sticks with the predicate approach and does not filter gatewayclasses in the dag pkg.
danehans
changed the title
|
@stevesloka to reduce the size of this PR, I only implemented the gatewayclass controller in commit |
There was a problem hiding this comment.
Overall looks good to me @danehans! Just a few comments/questions/thoughts.
The smaller bits make it much easier to review, thanks for doing that. =)
examples/gateway/00-crds.yaml
Outdated
| @@ -0,0 +1,205 @@ | |||
| apiVersion: apiextensions.k8s.io/v1 | |||
There was a problem hiding this comment.
Right now I've been just applying the CRDs from the upstream repo. Do we want to replicate them here?
There was a problem hiding this comment.
@stevesloka I think it makes sense add the upstream CRDs here so we can provide a similar users experience as kubectl apply -f https://projectcontour.io/quickstart/contour.yaml. I automated the process in #3676.
There was a problem hiding this comment.
Commented on linked issue, would be good to get some discussion as to why this is desirable there
internal/status/gatewayclass.go
Outdated
|
|
||
| updated.Status.Conditions = mergeConditions(updated.Status.Conditions, computeGatewayClassAdmittedCondition(valid)) | ||
|
|
||
| if equality.GatewayClassStatusChanged(gc.Status, updated.Status) { |
There was a problem hiding this comment.
This is diverting from how we manage conditions today by creating a ConditionsAccessor (https://github.com/projectcontour/contour/blob/main/internal/status/conditions.go#L74). I think this is ok, but need to make sure we don't duplicate logic.
There was a problem hiding this comment.
Also it doesn't make sense, but what if two controllers start updating status for the Class? Can a class be shared across controllers for multiple gateways?
There was a problem hiding this comment.
@stevesloka hmm... I wonder if this is a gap in the gc status spec. Maybe gc status should be similar to route status, where conditions are specified per gateway. Otherwise we need to perform a dance when multiple contours in gateway-mode are running in the cluster. cc: @youngnick @jpeach
There was a problem hiding this comment.
I can open an upstream issue in the meantime.
There was a problem hiding this comment.
xref upstream discussion: https://kubernetes.slack.com/archives/CR0H13KGA/p1620933776270200
There was a problem hiding this comment.
@stevesloka commit 61b743d introduces a gateway config param to enable/disable the gatewayclass controller. I think this is a simple and explicit approach that we can iterate on if needed. I was considering using gateway ns/name ownership labels but that's a bit tricky and has it's own disadvantages.
There was a problem hiding this comment.
What does this config option do? Why would I want to disable the GatewayClass controller?
There was a problem hiding this comment.
@stevesloka I initially thought that the 1st Contour would have the gc controller enabled and subsequent Contours would disable the gc controller so only 1 Contour manages gc's (since the controller string is the same for all Contours). I updated the approach in this PR so each instance of Contour will run the gc controller with a unique gc.spec.controller that is based on the namespace Contour is running in. This will ensure multiple instances of Contour do not fight over managing a gc.
There was a problem hiding this comment.
Feels like Contour should just not support gatewayclass. I don't think it buys us anything. Couldn't we just map to the config file or something? It's going to be a lot of work to have leader election across controllers & test/validate that.
There was a problem hiding this comment.
Let's have a chat about this, feels like we need to better map out how these pieces are going to fit together.
pkg/config/parameters.go
Outdated
| @@ -32,6 +32,7 @@ type ServerType string | |||
|
|
|||
| const ContourServerType ServerType = "contour" | |||
| const EnvoyServerType ServerType = "envoy" | |||
| const ContourGatewayClass = "projectcontour.io/contour" | |||
There was a problem hiding this comment.
I wonder if we can use the class to know if we should dynamically provision Envoy or not? This isn't part of this review per-say just thinking about how to best manage that.
There was a problem hiding this comment.
In an effort to minimize the size of this PR, I'm adding Envoy support in a future PR. IMO the gc.controller string should simply differentiate implementations from one another. From my understanding, that's the intent of the field. By default, Contour should manage the Envoy infra when Gateway CRDs/CRs exist and the gc controller is "projectcontour.io/contour". If a user does not want Contour to manage the Envoy infra, we can add a config knob to the resource referenced by the gc paramRef. I prefer that we avoid adding this knob until someone makes a strong case for it.
There was a problem hiding this comment.
I prefer that we avoid adding this knob until someone makes a strong case for it.
I've got a strong case for it, which was part of the reason moving this into Contour. I think we absolutely need to have a solution for folks who want a managed Envoy and for those who don't.
The controller name is just a string, just thinking of using that to tell us which controller should manage stuff, meaning should Contour provision or not. It can go in the spec somewhere as well, just trying to reduce the number of fields that need configuration.
There was a problem hiding this comment.
The other thing to think about is how do we determine if a GatewayClass should be operator managed or not? Right now it will act on all instances, but do all users want that? The class could then be projectcontour.io/operator telling us that it's an operator managed instance vs a self-managed.
There was a problem hiding this comment.
@stevesloka IMO the proper way to provide the "--enableEnvoyMgt" config knob is through gc.spec.parametersRef. At first, the referent can be the contour.yaml configmap that includes the knob. I think it's beneficial in the future to transition the configmap to a structured object similar to what's being done with the operator.
There was a problem hiding this comment.
I was just following the docs:
Controller is a domain/path string that indicates the controller that is managing Gateways of this class.
So we're defining which controller is the one in charge. We're introducing three different controller who are responsible. Is it the operator? Is it contour? Is it the cluster admin? Etc, etc.
There was a problem hiding this comment.
@stevesloka IMO the upstream docs need add'l details. I'm trying to define a gc.spec.controller naming schema for the gatewayclass contrller implemented in Contour. We have to consider that multiple Contour's may be running in the cluster, each with a gc controller. Therefore, gc.spec.controller should take the form of "CONTOUR_NAMESPACE/contour". I was originally thinking "CONTOUR_API_GROUP/CONTOUR_NAMESPACE/contour" but I reduced the verbosity based on #3659 (comment).
We're introducing three different controller who are responsible. Is it the operator? Is it contour? Is it the cluster admin?
The number is based on the number of Contours in the cluster. Each instance of Contour will have its own gc.spec.controller. The operator will continue to support its existing controller string until we can deprecate Gateway API support in the operator.
There was a problem hiding this comment.
So is the goal @danehans to try and make the gatewayclass unique? So regardless of what the string looks like, we only have a problem if there are two gateways which reference the same gatewayclass (regardless of the controller)? Is that correct?
There was a problem hiding this comment.
@stevesloka that is correct. When Contour supports managing multiple Envoy data-planes, we can drop the existing Gateway ns/name from the Contour config. For every gateway that references the expected gatewayclass name, the matching Contour will deploy the envoy infra (daemonset, service, etc.).
There was a problem hiding this comment.
Ok I guess we can do that, just doesn't feel right with the API to reference against a gateway class. It feels like an instance of Contour/Envoy should be driven against a Gateway. You should be able to have two Gateways referencing the same Class it seems from the API.
I'd be curious what @youngnick thinks on this for another perspective. Also, curious about the discussion coming up that you're leading @danehans on a GatewayClass-less cluster to see how that might fit in.
To keep things moving along this I'd suggest that we hold on the gateway class config item, chat about it a bit and add that logic back in when we've come up with the plan. I think for the implementation state as it stands now, a single Class with a single Gateway is the likely environment.
|
@stevesloka commit |
danehans
force-pushed
the
gw_api_support
branch
3 times, most recently
from
40d66b1
to
4254151
Compare
There was a problem hiding this comment.
Some more comments/questions and still a few open from last review. Thanks @danehans!
pkg/config/parameters.go
Outdated
| @@ -491,6 +493,10 @@ type Parameters struct { | |||
| // Server contains parameters for the xDS server. | |||
| Server ServerParameters `yaml:"server,omitempty"` | |||
|
|
|||
| // GatewayClassController is the identifier used by Contour's gatewayclass | |||
| // controller to determine if a GatewayClass object should be managed. | |||
| GatewayClassController string `yaml:"gatewayclass-controller,omitempty"` | |||
There was a problem hiding this comment.
Doesn't the gatewayclass reference give you this value automatically? We've got to have another set of config items now in addition to the gateway to configure?
There was a problem hiding this comment.
This tells the controller how to construct the gatewayclass.spec.controller string used for reconciling gatewayclasses. If a cluster has multiple Contours, the gatewayclass-controller for each needs to differentiate its self. For example:
kind: GatewayClass
apiVersion: networking.x-k8s.io/v1alpha1
metadata:
name: public
spec:
controller: projectcontour.io/public/contour
---
kind: Gateway
apiVersion: networking.x-k8s.io/v1alpha1
metadata:
name: contour
namespace: public
spec:
gatewayClassName: public
<SNIP>
---
kind: GatewayClass
apiVersion: networking.x-k8s.io/v1alpha1
metadata:
name: private
spec:
controller: projectcontour.io/private/contour
---
kind: Gateway
apiVersion: networking.x-k8s.io/v1alpha1
metadata:
name: contour
namespace: private
spec:
gatewayClassName: private
<SNIP>
---
Note: This is internal config and not exposed to users.
There was a problem hiding this comment.
This is internal config and not exposed to users.
It's in the public config package and specifies yaml parsers, so from the PR it seems like users are supposed to configure this field.
Should we just look at the configured gateway.spec.controller and verify the class matches that? If not then skip, otherwise process? Seems like we just need a gateway.spec.parametersRef and skip the whole gatewayclass object all together.
There was a problem hiding this comment.
@stevesloka OK, I'm still trying to connect the dots with the Contour code. I'm trying to generate the class name for Contour based on the namespace that Contour is running, i.e. <CONTOUR_NAMESPACE>/contour or even better <CONTOUR_NAMESPACE>/<CONTOUR_DEPLOYMENT_NAME>.
examples/gateway/00-crds.yaml
Outdated
| @@ -0,0 +1,205 @@ | |||
| apiVersion: apiextensions.k8s.io/v1 | |||
There was a problem hiding this comment.
Commented on linked issue, would be good to get some discussion as to why this is desirable there
| @@ -0,0 +1,130 @@ | |||
| // Copyright Project Contour Authors | |||
There was a problem hiding this comment.
I think there is starting to be enough logic in these controllers we need a way to ensure some more direct test coverage, or have some more detailed integration tests that we can be confident all the logic here is covered
There was a problem hiding this comment.
@sunjayBhatia the event handler is making things difficult to mock the controller's Reconcile() method. Looking at cmd/contour/serve.go a lot needs to happen to mock the k8s.DynamicClientHandler. Maybe it's better to use e2e for testing. Thoughts? cc: @stevesloka
There was a problem hiding this comment.
I can pick up the testing bit next, was going to add some since now we're adding logic into these controllers. Happy to take the initial framework bits for that.
There was a problem hiding this comment.
Stepping back a bit from the changes around a GatewayClass controller reference, it seems we need to define what we're solving with that and what other issues exist once we start reconciling GatewayClasses
- An installation of Contour needs to know what GatewayClass to reconcile/admit
- And by extension, which Gateways it should admit
- Making the controller name configurable on a Contour instance make sense for this
- However we also have a configurable Gateway name field, which probably makes sense to remove so we don't have an unwieldy UX and two fields to configure
- What happens when there are multiple GatewayClasses with the matching controller name Contour is configured to reconcile?
- Assuming you can create multiple separate GatewayClasses with different names but the same controller name field, what happens? Right now it looks like this change does not do any filtering so if you were to add multiple we would process both and admit both
Just quickly thinking out loud, this is how I could see things working:
- contour config changes:
- we remove support for the gateway name specification in contour
- we add some config that tells a contour instance what its "controller string" to look for is (like in this PR)
- user flow:
- contour controller installed, with a specific controller string to look for on gatewayclasses
- cluster operator creates a gatewayclass with controller name set to contour's, contour takes note and sets status
- error case: cluster operator creates a second gatewayclass with controller name set to contour's, contour takes note and says it is not admitted, since it is younger than the first gatewayclass
- this could change in the future if we want to refactor contour to support multiple gatewayclasses somehow, though seems improbable
- Below points are maybe out of scope of this PR, but related
- cluster operator creates a gateway referencing admitted gatewayclass above, contour takes note, starts creating data plane, sets status, etc.
- error case: cluster operator creates a second gateway referencing admitted gatewayclass above, contour takes note, sets status saying it is not admitted since it is younger than first gateway
- this could change if we want to merge the gateways, or support a single contour running multiple data planes etc.
- app devs/operators create routes as expected, contour reconciles, etc.
- contour controller installed, with a specific controller string to look for on gatewayclasses
@sunjayBhatia I provided a response to your comment in the linked PR. |
@sunjayBhatia as I mentioned to @stevesloka, I don't think the value Contour uses for
In our refactored model, Contour represents a gc and envoy represents a gw. Until Contour supports managing multiple data-planes, i.e. gw's, Contour will support a 1:1 of gc-to-gw. When Contour does support managing multiple data-planes, we can remove the gateway config and Contour will watch all gw's and filter based on the
This PR does not prevent this scenario (trying to keep the PR small). I like your idea of using
If we remove, then Contour should only manage the 1st gateway of a matching gc. Otherwise, we should keep this config until Contour supports managing multiple data-planes. Either way, this issue will be addressed in a separate PR that focuses on the gw controller.
Instead of a config knob (I think users may misconfigure and cause problems), the string should be unique per Contour using a defined naming scheme
Yes, I plan to address these steps in a follow-on PR. |
|
We discussed this in our maintainer's meeting today. Here's what I remember:
That's all I can remember, if anyone else has further updates, please add them. I'll rereview once @danehans has made an update. |
danehans
added
the
release-note
There was a problem hiding this comment.
Overall I think it looks good @danehans.
One question is where does the magic string of the controller name get filtered? I couldn't find that bit which I think is important for this or a future PR depending on your order. Thanks!
cmd/contour/serve.go
Outdated
| @@ -155,6 +155,9 @@ func registerServe(app *kingpin.Application) (*kingpin.CmdClause, *serveContext) | |||
|
|
|||
| serve.Flag("debug", "Enable debug logging.").Short('d').BoolVar(&ctx.Config.Debug) | |||
| serve.Flag("kubernetes-debug", "Enable Kubernetes client debug logging with log level.").PlaceHolder("<log level>").UintVar(&ctx.KubernetesDebug) | |||
|
|
|||
| serve.Flag("gateway-class-name", "Contour GatewayClass name.").PlaceHolder("<api_group>/<namespace>/contour").StringVar(&ctx.gatewayClassName) | |||
There was a problem hiding this comment.
From the last meeting, was this intended to be convention? The GatewayClass name would match how Contour is deployed? So in this example a GatewayClass of projectcontour.io/projectcontour/contour should map to an instance of Contour in the projectcontour namespace?
Also should replace <api_group> with projectcontour.io since that's what's expected.
There was a problem hiding this comment.
If we do find we need this then it should come from the config file, not a flag. We've been adding all those bits to the config file that needs configuring.
There was a problem hiding this comment.
From the last meeting, was this intended to be convention?
@stevesloka from my understanding <api_group>/<namespace>/contour was the agreed upon convention.
The GatewayClass name would match how Contour is deployed?
Yes, the namespace that Contour is deployed.
Also should replace <api_group> with projectcontour.io since that's what's expected.
I used <api_group> since this portion of the string is derived from the API group. I'll update to the group string.
If we do find we need this then it should come from the config file, not a flag.
Per feedback from the meeting, I was following how the ingress class name is configured. I'll update to use the config file.
| func validateGatewayClassObjMeta(ctx context.Context, cli client.Client, meta *metav1.ObjectMeta, path *field.Path) field.ErrorList { | ||
| errs := apivalidation.ValidateObjectMeta(meta, false, ValidateGatewayClassName, path.Child("name")) | ||
|
|
||
| classes := &gatewayapi_v1alpha1.GatewayClassList{} |
There was a problem hiding this comment.
Why do we need to check for other gatewayclasses? I'd expect "Validate" would just validate the one that we care about and we'd ignore all others. In this example now, we'd only care about classes that match projectcontour.io/projectcontour/contour? If there are more classes, then we'd need to validate against that controller but I don't see where the controller is pulled in?
There was a problem hiding this comment.
@stevesloka a gatewayclass is invalid if one is already admitted with the same spec.controller. I did update the func as the one you reviewed did not check the controller and status conditions of existing gatewayclasses.
I think this comes in here: contour/internal/controller/gatewayclass.go Lines 68 to 90 in 1bbbcbb |
Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>
Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>
Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>
@stevesloka the string is passed from the serveContext in cmd/serve.go to |
Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>
There was a problem hiding this comment.
/lgtm 🎉
I think we can follow up with more but would be nice to get this in and expand from there. Thanks, @danehans!!
There was a problem hiding this comment.
I think before we move too far forward with more of the controller pattern, it would be good to get some unit/functional testing and some more testing on the order of the featuretests package or something
by moving to the controller/reconcile pattern we've sort of skirted around the coverage the featuretests package provides it seems and should rectify that
having tests in this style makes it really easy to show a more end to end flow and what should happen when certain resources are added and to do positive and negative cases
also would be good to collect/list (in the PR description maybe) and have issues for all the follow ups we intend to do since this PR is big already but we want to address some things and not block this too much
|
@sunjayBhatia I created an issue regarding the controller testing, added a link to the PR description and assigned @stevesloka since offered to take on the task in a follow-on PR (xref). |
|
@sunjayBhatia thanks for the review. Commit |
Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>
|
@stevesloka commit |
|
Nice work @danehans! 🎉 |
youngnick
moved this from 1.14 release completed
to 1.16 release
in Contour Project Board
Updates Gateway API support to use controller-runtime added by #3510:
cmd/contour/serve.go: AddsNewGatewayClassController()to create a new gatewayclass controller.examples/contour/02-role-contour.yamlandexamples/render/contour.yaml: Updates Contour's RBAC to support reconciling gatewayclasses and managing the Envoy service.internal/dag/cache.go: Adds gatewayclasses that contain the contour controller string to the kubernetes cache.internal/equality/equality.go: Adds an equality pkg used to compare current/desired state of resources, conditions, etc..internal/controller/gatewayclass.go: Adds gatewayclass controller and reconciler functionality.internal/k8s/informers.go: Updates kubebuilder RBAC tags and the list of Gateway API resources to include gatewayclasses.internal/status/conditions.go: Adds support for managing gateway and gatewayclass status conditions.internal/status/gatewayclass.go: Adds support for managing gatewayclass status.internal/validation/gatewayclass.go: Adds support for validating gatewayclass resources.pkg/config/parameters.go: AddsGatewayClassControlleridentifier used by Contour's gatewayclass controller to determine if a GatewayClass object should be managed.Requires #3510
TODOs: