internal/provisioner: set GatewayClass conditions correctly based on parametersRef

[skriss]

Sets the GatewayClass's "Accepted" condition to true or
false based on whether the GatewayClass has a parametersRef
and if it is valid. Also wires up all of the reconciles
based on changes in related objects.

Signed-off-by: Steve Kriss krisss@vmware.com

(This doesn't actually do anything with the parameters yet, that'll come later, this was big enough as-is just to get the conditions and reconcile loops right).

I don't love only testing this controller logic via E2E's, but it does work pretty well and the alternatives are either a bunch of fakes/mocks for the Client which can get messy, or using envtest (which ends up looking a lot like E2E's, but runs locally so faster, but also doesn't always work like a real cluster which can cause some weirdness). Any thoughts?

[codecov]

Codecov Report

Merging #4440 (79df8ab) into main (105f6e5) will decrease coverage by 0.45%.
The diff coverage is 18.40%.

@@            Coverage Diff             @@
##             main    #4440      +/-   ##
==========================================
- Coverage   74.12%   73.67%   -0.46%     
==========================================
  Files         137      137              
  Lines       12219    12332     +113     
==========================================
+ Hits         9057     9085      +28     
- Misses       2964     3049      +85     
  Partials      198      198              

Impacted Files	Coverage Δ
cmd/contour/gatewayprovisioner.go	0.00% <0.00%> (ø)
internal/provisioner/controller/gatewayclass.go	12.82% <12.50%> (+12.82%)	⬆️
internal/provisioner/controller/gateway.go	8.07% <30.95%> (+8.07%)	⬆️

[sunjayBhatia]

I don't love only testing this controller logic via E2E's, but it does work pretty well and the alternatives are either a bunch of fakes/mocks for the Client which can get messy, or using envtest (which ends up looking a lot like E2E's, but runs locally so faster, but also doesn't always work like a real cluster which can cause some weirdness). Any thoughts?

having this same debate w/ the work to add a new runnable that preheats the cache/dag on contour startup, some combo of envtest and mocks will probably be what i end up doing since i just want to make sure at a lower level objects are sent to the event handler etc. and there isn't something easily observable via an e2e test

slightly different than this case since you do have the status element to observe as a behavioral output of the controller running, though if we end up pulling status updating etc. into another async component, that might change so setting up the controller with fakes might be nice then

this maybe goes back to some thoughts we had around sticking with Controllers vs. setting up informers and event handlers which are a much simpler abstraction to test, if we stick with the former we probably need to invest a little time into thinking about a better way to test things more granularly (envtest or otherwise) and/or create better abstractions that the main controller logic uses that we can then unit test (hard to justify sometimes bc it feels like an abstraction for abstraction's sake)

[skriss]

I don't love only testing this controller logic via E2E's, but it does work pretty well and the alternatives are either a bunch of fakes/mocks for the Client which can get messy, or using envtest (which ends up looking a lot like E2E's, but runs locally so faster, but also doesn't always work like a real cluster which can cause some weirdness). Any thoughts?

having this same debate w/ the work to add a new runnable that preheats the cache/dag on contour startup, some combo of envtest and mocks will probably be what i end up doing since i just want to make sure at a lower level objects are sent to the event handler etc. and there isn't something easily observable via an e2e test

slightly different than this case since you do have the status element to observe as a behavioral output of the controller running, though if we end up pulling status updating etc. into another async component, that might change so setting up the controller with fakes might be nice then

this maybe goes back to some thoughts we had around sticking with Controllers vs. setting up informers and event handlers which are a much simpler abstraction to test, if we stick with the former we probably need to invest a little time into thinking about a better way to test things more granularly (envtest or otherwise) and/or create better abstractions that the main controller logic uses that we can then unit test (hard to justify sometimes bc it feels like an abstraction for abstraction's sake)

Yeah, in this case the E2E's are effective, just slower.

I'm not opposed to introducing envtest for some of this stuff, we just need to be aware of the limitations of each approach to testing and try not to get too much duplication across all of them.

For now, I'm going to do some experimenting with tests using the fake controller-runtime client (which is implemented as a simple in-mem store of objects) -- I've had success with that approach on other projects and I think we might be able to get decent mileage out of the fake client at least for the provisioner, where our "output" is really just creating Kubernetes resources. Will look to do that in separate PRs.

[sunjayBhatia]

for the future, will we need to implement this differently (custom handler that ignores the update event?) so that updates to ContourDeployment resources don't trigger a reconcile? (and updates to GatewayClasses do not cause a re-reconfiguration of the Contour installation for an existing Gateway)

[skriss]

Yeah good question - I think there are a couple different ways we could go about implementing, but my plan had been to revise the logic that's in the EnsureObject functions, to not do full updates if the object already exists, but instead to only update those fields that should be updated. So things coming from GatewayClass would not flow through to updates to the existing objects, but e.g. updates to Listener definitions would flow through as updates to the existing objects. I think we probably need some logic like that regardless, because if you take that example of Listeners being updated, we need to ensure when we do push those updates through, that things derived from GatewayClass don't also get updated to reflect the current state of the GatewayClass.

[sunjayBhatia]

ah i see, do the logic further down the line of processing an update 👍🏽