Bump dependencies. Changes to get latest c/r library working. #138

irbekrm · 2023-06-01T13:35:19Z

This PR:

updates go module dependencies
bumps Go 1.19 -> 1.20
makes changes required to use latest c/r library:
- updates controller watch setup
- changes to use the new webhook setup mechanism as the one we currently use has been removed
- changes how the fake cache gets loaded with objects
- ⚠️ BREAKING ⚠️ changes validating webhook path /validate -> /validate-trust-cert-manager-io-v1alpha1-bundle as this is hardcoded in the new c/r webhook setup. This has a potential of causing webhook downtime during upgrade to the version of trust-manager that adds this change (if the ValidatingWebhookConfiguration has been updated, but the webhook is still running the old version that does not yet serve to the new path)

Look at https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0 to understand the changes

This PR is also very similar to cert-manager/approver-policy#233

Signed-off-by: irbekrm <irbekrm@gmail.com>

This should be a test rather than something that's in users request path Signed-off-by: irbekrm <irbekrm@gmail.com>

This commit does not actually change any of the logic. All the changes are in response to c/r v0.15 changes for Watch setup. See c/r release notes https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0 Signed-off-by: irbekrm <irbekrm@gmail.com>

…parately See kubernetes-sigs/controller-runtime#2259 Signed-off-by: irbekrm <irbekrm@gmail.com>

As the mechanism that we have been using so far that depends on injector decoder injection has been removed Signed-off-by: irbekrm <irbekrm@gmail.com>

The path is hardcoded in c/r, so we can't keep the old path Signed-off-by: irbekrm <irbekrm@gmail.com>

irbekrm · 2023-06-01T15:16:32Z

pkg/bundle/controller.go

-		// These transforms are used as a safety check to ensure that only
-		// resources of the expected types are cached.
-		TransformByObject: map[client.Object]toolscache.TransformFunc{
-			new(corev1.Namespace): func(obj any) (any, error) {
-				return obj, nil
-			},
-			new(trustapi.Bundle): func(obj any) (any, error) {
-				return obj, nil
-			},
-			new(corev1.Secret): func(obj any) (any, error) {
-				return obj, nil
-			},
-			new(corev1.ConfigMap): func(obj any) (any, error) {
-				return obj, nil
-			},
-		},
-		DefaultTransform: func(obj any) (any, error) {
-			return nil, fmt.Errorf("object %T not supported by target cache", obj)
-		},


I understand that it's hard or potentially not possible to devise a test that could test that the cache only has watches for these object types. But I don't think that we should put a check like this that should run as a test in every user request's path. Ultimately, if this would actually catch anything, it would most likely be first noticed by a user when it started spamming their logs.

irbekrm · 2023-06-01T15:21:25Z

pkg/bundle/controller.go


 		// Reconcile trust.cert-manager.io Bundles
-		Watches(source.NewKindWithCache(new(trustapi.Bundle), sourceCache), &handler.EnqueueRequestForObject{}).
+		WatchesRawSource(&source.Informer{Informer: bundleInformer}, &handler.EnqueueRequestForObject{}).


This is the new set up of how to use external cache

irbekrm · 2023-06-01T15:24:08Z

pkg/webhook/webhook.go

 	validator := &validator{log: opts.Log.WithName("validation")}
-	mgr.GetWebhookServer().Register("/validate", &webhook.Admission{Handler: validator})
-	mgr.AddReadyzCheck("validator", validator.check)
+	err := builder.WebhookManagedBy(mgr).


See cert-manager/approver-policy#233 (comment)

irbekrm · 2023-06-01T15:26:43Z

pkg/webhook/validation_test.go

@@ -20,11 +20,10 @@ import (
 	"context"
 	"testing"

-	admissionv1 "k8s.io/api/admission/v1"


The test change is purely in response to the new webhook setup mechanism. A couple test cases that were dealing with populating admission response/retrieving object from admission request have been removed as we no longer need to do that- c/r gives our webhook validator the decoded object from the request and only expects back an error and warnings.

irbekrm · 2023-06-01T15:27:34Z

deploy/charts/trust-manager/templates/webhook.yaml

@@ -49,4 +49,4 @@ webhooks:
      service:
        name: {{ include "trust-manager.name" . }}
        namespace: {{ .Release.Namespace | quote }}
-        path: /validate
+        path: /validate-trust-cert-manager-io-v1alpha1-bundle


See cert-manager/approver-policy#233 (comment)

SgtCoDFish

Couple of questions / suggestions on this - thanks for doing it!

pkg/bundle/controller.go

irbekrm · 2023-06-01T17:01:22Z

Thanks for taking a look and spotting the issues @SgtCoDFish ! Think I've now fixed those

pkg/bundle/controller.go

Signed-off-by: irbekrm <irbekrm@gmail.com>

irbekrm · 2023-06-02T09:20:43Z

weird docker flake

/retest

SgtCoDFish

/lgtm
/approve

This looks great and this is a piece of work that would've sucked to do so I really appreciate you doing it! 🚀

jetstack-bot · 2023-06-02T09:52:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irbekrm, SgtCoDFish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [SgtCoDFish,irbekrm]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

irbekrm added 2 commits June 1, 2023 14:37

go get -u -t ./...

3e09712

Signed-off-by: irbekrm <irbekrm@gmail.com>

Go 1.19 -> 1.20

1921b84

Signed-off-by: irbekrm <irbekrm@gmail.com>

irbekrm force-pushed the bump_deps branch from e7ddaf5 to 03fe443 Compare June 1, 2023 13:39

irbekrm added 6 commits June 1, 2023 15:50

Adds a comment about why we have a second cache

991c2d6

Signed-off-by: irbekrm <irbekrm@gmail.com>

Removes transforms that were used as a cache test

3db11e5

This should be a test rather than something that's in users request path Signed-off-by: irbekrm <irbekrm@gmail.com>

c/r changes: fake objects with status subresource requires loading se…

6c430da

…parately See kubernetes-sigs/controller-runtime#2259 Signed-off-by: irbekrm <irbekrm@gmail.com>

c/r changes: adapt the new webhook setup mechanism

59a8ba3

As the mechanism that we have been using so far that depends on injector decoder injection has been removed Signed-off-by: irbekrm <irbekrm@gmail.com>

c/r changes: BREAKING: use the new validating webhook path

bc2b2bd

The path is hardcoded in c/r, so we can't keep the old path Signed-off-by: irbekrm <irbekrm@gmail.com>

irbekrm force-pushed the bump_deps branch from 03fe443 to bc2b2bd Compare June 1, 2023 15:00

irbekrm changed the title ~~Bump deps~~ Bump dependencies. Changes to get latest c/r library working. Jun 1, 2023

irbekrm commented Jun 1, 2023

View reviewed changes

SgtCoDFish mentioned this pull request Jun 1, 2023

Bump golang/x libs to fix CVE-2022-41723 #139

Closed

SgtCoDFish reviewed Jun 1, 2023

View reviewed changes

pkg/bundle/controller.go Outdated Show resolved Hide resolved

pkg/bundle/controller.go Outdated Show resolved Hide resolved

SgtCoDFish requested changes Jun 1, 2023

View reviewed changes

pkg/bundle/controller.go Outdated Show resolved Hide resolved

pkg/bundle/controller.go Outdated Show resolved Hide resolved

Code review feedback

6d339ca

Signed-off-by: irbekrm <irbekrm@gmail.com>

irbekrm force-pushed the bump_deps branch from 93c31f0 to 6d339ca Compare June 2, 2023 09:16

SgtCoDFish approved these changes Jun 2, 2023

View reviewed changes

jetstack-bot assigned SgtCoDFish Jun 2, 2023

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2023

SgtCoDFish merged commit 99cd18a into cert-manager:main Jun 2, 2023
4 checks passed

irbekrm deleted the bump_deps branch June 2, 2023 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump dependencies. Changes to get latest c/r library working. #138

Bump dependencies. Changes to get latest c/r library working. #138

irbekrm commented Jun 1, 2023 •

edited

irbekrm Jun 1, 2023 •

edited

irbekrm Jun 1, 2023

irbekrm Jun 1, 2023

irbekrm Jun 1, 2023

irbekrm Jun 1, 2023

SgtCoDFish left a comment

irbekrm commented Jun 1, 2023

irbekrm commented Jun 2, 2023

SgtCoDFish left a comment

jetstack-bot commented Jun 2, 2023

Bump dependencies. Changes to get latest c/r library working. #138

Bump dependencies. Changes to get latest c/r library working. #138

Conversation

irbekrm commented Jun 1, 2023 • edited

irbekrm Jun 1, 2023 • edited

Choose a reason for hiding this comment

irbekrm Jun 1, 2023

Choose a reason for hiding this comment

irbekrm Jun 1, 2023

Choose a reason for hiding this comment

irbekrm Jun 1, 2023

Choose a reason for hiding this comment

irbekrm Jun 1, 2023

Choose a reason for hiding this comment

SgtCoDFish left a comment

Choose a reason for hiding this comment

irbekrm commented Jun 1, 2023

irbekrm commented Jun 2, 2023

SgtCoDFish left a comment

Choose a reason for hiding this comment

jetstack-bot commented Jun 2, 2023

irbekrm commented Jun 1, 2023 •

edited

irbekrm Jun 1, 2023 •

edited