replace dynamic client with controller-runtime client #4153

skriss · 2021-11-03T18:12:31Z

Replaces the dynamic Kubernetes client with
the controller-runtime client, which is easier
to use and can natively perform reads from
the cache.

Closes #3983.

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

Replaces the dynamic Kubernetes client with the controller-runtime client, which is easier to use and can natively perform reads from the cache. Closes projectcontour#3983. Signed-off-by: Steve Kriss <krisss@vmware.com>

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

codecov · 2021-11-03T18:23:26Z

Codecov Report

Merging #4153 (8676e2d) into main (7fa2a53) will increase coverage by 0.61%.
The diff coverage is 25.41%.

❗ Current head 8676e2d differs from pull request most recent head a6aee60. Consider uploading reports for the commit a6aee60 to get more accurate results

@@            Coverage Diff             @@
##             main    #4153      +/-   ##
==========================================
+ Coverage   72.56%   73.17%   +0.61%     
==========================================
  Files         115      113       -2     
  Lines       10032     9884     -148     
==========================================
- Hits         7280     7233      -47     
+ Misses       2597     2497     -100     
+ Partials      155      154       -1

Impacted Files	Coverage Δ
cmd/contour/certgen.go	`26.41% <0.00%> (ø)`
cmd/contour/ingressstatus.go	`34.37% <ø> (ø)`
cmd/contour/leadership.go	`0.00% <0.00%> (ø)`
cmd/contour/serve.go	`12.91% <0.00%> (+0.31%)`	⬆️
internal/contour/handler.go	`92.00% <ø> (ø)`
internal/controller/gateway.go	`0.00% <0.00%> (ø)`
internal/controller/gatewayclass.go	`0.00% <0.00%> (ø)`
internal/controller/tcproute.go	`0.00% <0.00%> (ø)`
internal/controller/udproute.go	`0.00% <0.00%> (ø)`
internal/k8s/clients.go	`0.00% <0.00%> (-20.69%)`	⬇️
... and 11 more

skriss · 2021-11-03T18:17:55Z

cmd/contour/serve.go

+	for name, r := range map[string]client.Object{
+		"httpproxies":               &contour_api_v1.HTTPProxy{},
+		"tlscertificatedelegations": &contour_api_v1.TLSCertificateDelegation{},
+		"extensionservices":         &contour_api_v1alpha1.ExtensionService{},
+		"contourconfigurations":     &contour_api_v1alpha1.ContourConfiguration{},
+		"services":                  &corev1.Service{},
+		"ingresses":                 &networking_v1.Ingress{},
+		"ingressclasses":            &networking_v1.IngressClass{},
+	} {


it seemed easier to just inline these now that they're one-liners each, but if you all prefer this being kept in informers.go / in a DefaultResources() function I can go back to that.

Agree inline is cleaner.

skriss · 2021-11-03T18:19:22Z

cmd/contour/serve.go

-		// Only inform on GatewayAPI if found in the cluster.
-		if s.clients.ResourcesExist(k8s.GatewayAPIResources()...) {


I dropped the explicit resources-exist check since it seemed redundant now: startup will fail anyway if you try to enable Gateway API but the resources don't exist. Had the benefit of being able to entirely get rid of the RESTMapper.

skriss · 2021-11-03T18:21:04Z

examples/contour/02-role-contour.yaml

@@ -21,29 +21,8 @@ rules:
  - ""
  resources:
  - endpoints
-  verbs:


this change was just rearranging things to group them by API group, shouldn't be any actual changes to the rules.

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

skriss · 2021-11-03T18:35:09Z

Ready for review but I'd like to do some additional manual testing on this one before merging.

stevesloka

lgtm pending the smoke tests! There are spots where we could probably just move to controller manager setups to standardize a bit on how we get data, but not for this PR.

stevesloka · 2021-11-04T15:37:00Z

cmd/contour/serve.go

+	for name, r := range map[string]client.Object{
+		"httpproxies":               &contour_api_v1.HTTPProxy{},
+		"tlscertificatedelegations": &contour_api_v1.TLSCertificateDelegation{},
+		"extensionservices":         &contour_api_v1alpha1.ExtensionService{},
+		"contourconfigurations":     &contour_api_v1alpha1.ContourConfiguration{},
+		"services":                  &corev1.Service{},
+		"ingresses":                 &networking_v1.Ingress{},
+		"ingressclasses":            &networking_v1.IngressClass{},
+	} {


Agree inline is cleaner.

cmd/contour/serve.go

skriss · 2021-11-04T18:20:02Z

I deployed it to a cloud cluster, ensured ingress address was updated properly, did some manual smoke-testing and things seemed fine so I think we're good here.

internal/k8s/rbac.go

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

skriss added 3 commits November 3, 2021 12:10

replace dynamic client with controller-runtime client

b065ca4

Replaces the dynamic Kubernetes client with the controller-runtime client, which is easier to use and can natively perform reads from the cache. Closes projectcontour#3983. Signed-off-by: Steve Kriss <krisss@vmware.com>

simplify informer creation

7dcd02f

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

simplify client creation

abe801d

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

skriss commented Nov 3, 2021

View reviewed changes

changelog

b5f447d

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

skriss added the release-note/small A small change that needs one line of explanation in the release notes. label Nov 3, 2021

skriss marked this pull request as ready for review November 3, 2021 18:34

skriss requested a review from a team as a code owner November 3, 2021 18:34

skriss requested review from tsaarni and youngnick and removed request for a team November 3, 2021 18:34

skriss added the do not merge Don't merge this PR until this label is removed. label Nov 3, 2021

stevesloka approved these changes Nov 4, 2021

View reviewed changes

skriss mentioned this pull request Nov 4, 2021

enable Contour to process RLS even if it's created after Contour startup #4155

Open

skriss removed the do not merge Don't merge this PR until this label is removed. label Nov 4, 2021

skriss added this to the 1.20.0 milestone Nov 4, 2021

sunjayBhatia reviewed Nov 4, 2021

View reviewed changes

internal/k8s/rbac.go Show resolved Hide resolved

sunjayBhatia approved these changes Nov 4, 2021

View reviewed changes

move configmap rbac into rbac.go

a6aee60

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

skriss merged commit cb3ec9a into projectcontour:main Nov 4, 2021

skriss deleted the rm-dynamic-client branch November 8, 2021 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace dynamic client with controller-runtime client #4153

replace dynamic client with controller-runtime client #4153

skriss commented Nov 3, 2021

codecov bot commented Nov 3, 2021 •

edited

Loading

skriss Nov 3, 2021 •

edited

Loading

stevesloka Nov 4, 2021

skriss Nov 3, 2021

skriss Nov 3, 2021

skriss commented Nov 3, 2021 •

edited

Loading

stevesloka left a comment

stevesloka Nov 4, 2021

skriss commented Nov 4, 2021

		// Only inform on GatewayAPI if found in the cluster.
		if s.clients.ResourcesExist(k8s.GatewayAPIResources()...) {

replace dynamic client with controller-runtime client #4153

replace dynamic client with controller-runtime client #4153

Conversation

skriss commented Nov 3, 2021

codecov bot commented Nov 3, 2021 • edited Loading

Codecov Report

skriss Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

stevesloka Nov 4, 2021

Choose a reason for hiding this comment

skriss Nov 3, 2021

Choose a reason for hiding this comment

skriss Nov 3, 2021

Choose a reason for hiding this comment

skriss commented Nov 3, 2021 • edited Loading

stevesloka left a comment

Choose a reason for hiding this comment

stevesloka Nov 4, 2021

Choose a reason for hiding this comment

skriss commented Nov 4, 2021

codecov bot commented Nov 3, 2021 •

edited

Loading

skriss Nov 3, 2021 •

edited

Loading

skriss commented Nov 3, 2021 •

edited

Loading