[provider] refactoring kubernetes provider to single reconciler by chauhanshubham · Pull Request #702 · envoyproxy/gateway

chauhanshubham · 2022-11-06T09:36:34Z

This commit removes separate controllers for gateway API objects, and
works on only a single controller that now watches for all related objects.
Prior to this we had n controllers each watching >=1 resources and pushing
according to the resource map. These pushes happened from these n controllers.
For instance;

HTTPRoute controller pushing httproute, service and namespace (for refgrant processing) to the resource map
every route controller duplicating this process
Gateway controller pushing gateways, referencegrants, namespace, secret to the resource map
essentially every controller pushing it's namespace to the resource map

Now:
We have a single gatewayAPIController that has a single gatewayAPIReconciler, which reconciles
the gatewayClass object while watching for ALL gateway API objects. Any CRUD in these related
objects, triggers the same reconciliation logic, where the resource structure is created from scratch,
and pushed as is to the resource map.
Any error during reconciliation - particularly around any object not being found (non-existant/deleted resource),
skips the reconciliation around that particular update, and therefore does not lead to a push to
the resource map. Therefore, the push ONLY happens when all related objects exist - once per reconciliation.

A few tests are not required so those have been removed - we could work towards adding more
meaningful ones subsequently. Conformance tests remain as is, with two new additions as part of
this PR (commented out reference grant ones)

Signed-off-by: Shubham Chauhan shubham@tetrate.io

Resolves #413
Resolves #539

codecov-commenter · 2022-11-06T09:56:01Z

Codecov Report

Merging #702 (3bb2362) into main (923c4b0) will decrease coverage by 2.19%.
The diff coverage is 47.02%.

@@            Coverage Diff             @@
##             main     #702      +/-   ##
==========================================
- Coverage   63.67%   61.47%   -2.20%     
==========================================
  Files          47       46       -1     
  Lines        5949     5649     -300     
==========================================
- Hits         3788     3473     -315     
- Misses       1921     1949      +28     
+ Partials      240      227      -13

Impacted Files	Coverage Δ
internal/cmd/server.go	`8.10% <0.00%> (+0.48%)`	⬆️
internal/gatewayapi/helpers_v1alpha2.go	`20.63% <0.00%> (-8.26%)`	⬇️
internal/gatewayapi/translator.go	`85.34% <ø> (ø)`
internal/gatewayapi/zz_generated.deepcopy.go	`0.00% <0.00%> (ø)`
internal/message/types.go	`0.00% <0.00%> (-75.00%)`	⬇️
internal/provider/kubernetes/kubernetes.go	`58.53% <0.00%> (+7.59%)`	⬆️
internal/status/status.go	`0.00% <0.00%> (ø)`
internal/gatewayapi/runner/runner.go	`32.91% <11.11%> (-23.34%)`	⬇️
internal/provider/kubernetes/predicates.go	`49.65% <49.65%> (ø)`
internal/provider/kubernetes/controller.go	`52.13% <52.13%> (ø)`
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

internal/provider/kubernetes/controller.go

internal/provider/kubernetes/route.go

chauhanshubham · 2022-11-12T15:16:42Z

oh god all the tests passed :')

chauhanshubham · 2022-11-12T15:18:51Z

so brute force single controller is working (all based on existing tests)
On to a few more manual tests and @arkodg 's review comments and a few enhancements here and there if possible.
Manual tests would mostly cover experimenting with obj creation ordering.

arkodg · 2022-11-17T18:02:49Z

hey @chauhanshubham did you get a chance to review the comments ?

internal/gatewayapi/translator.go

internal/status/status.go

danehans

@chauhanshubham thanks for all your work on this PR. I have a few notes and questions that I would like your feedback on.

api/config/v1alpha1/groupversion_info.go

internal/provider/kubernetes/controller.go

internal/status/status.go

internal/provider/kubernetes/helpers.go