bumps Go version to 1.17, updates deps and kube libraries by irbekrm · Pull Request #4478 · cert-manager/cert-manager

irbekrm · 2021-09-27T13:35:14Z

What this PR does / why we need it:

Bumps Go 1.16 -> 1.17
Bumps kube libraries v0.22.0 -> v0.22.2
Bumps controller runtime v0.9.6 -> v0.10.1
Bumps some Bazel deps, nginx-ingress images for e2e tests and a few more things
Enforces minimum Bazel version of v4.0.0
Uses Bazel to load Kyverno images- this should allow for the images to be cached and hopefully for there to be less timeouts like this

Special notes for your reviewer:

Minimum required Bazel version is now v4.0.0 because this is the minimum version required by rules_go v0.28.0. I've updated the CI test images with Bazel in Bazel version bump testing#585 and website#708 updates supported Bazel versions in documentation. I've added some code borrowed from pixie to enforce the minimum version upfront as I think this is preferable as it'd be easier to debug than mystery errors because of version incompatibility
controller runtime v0.10.0 fixes a number of bugs that may or may not caused some of our test flakiness

Go 1.17:

I've regenerated the go.mod file with go mod tidy -go=1.17 and it is now in the new form required for module graph pruning in 1.17
go:build form of Go build tags has been automatically added to all files that previously only had // +build form of build tags, see https://go.googlesource.com/proposal/+/master/design/draft-gobuild.md
Binary size seems to have shrunk slightly due to compiler improvements:

irbe@cert-manager$ ls -lha bazel-out/k8-fastbuild/bin/cmd/controller/controller_/controller
-r-xr-xr-x 1 irbe irbe 60M Sep 27 15:53 bazel-out/k8-fastbuild/bin/cmd/controller/controller_/controller <- from current master

irbe@cert-manager$ ls -lha bazel-out/k8-fastbuild/bin/cmd/controller/controller_/controller
-r-xr-xr-x 1 irbe irbe 58M Sep 27 15:57 bazel-out/k8-fastbuild/bin/cmd/controller/controller_/controller <- this branch

Various improvements to core crypto libraries might make us faster and safer 🚀 https://golang.org/doc/go1.17#minor_library_changes, but don't appear breaking
e2e tests where run in CI for all kube versions + the upgrade test & all passed

cert-manager now uses Go 1.17.

irbekrm · 2021-09-27T13:35:49Z

/kind cleanup

irbekrm · 2021-09-27T17:20:30Z

hack/update-codegen.sh

+# This has been copied from https://github.com/bazelbuild/bazel/blob/master/tools/bash/runfiles/runfiles.bash
+# It exports rlocation function that can be used to access other bash scripts in this codebase.
+# set -e pipefail needs to happen after sourcing runfiles.bash, see https://github.com/bazelbuild/bazel/blob/master/tools/bash/runfiles/runfiles.bash#L21
+
+# --- begin runfiles.bash initialization v2 ---
+# Copy-pasted from the Bazel Bash runfiles library v2.
+set -uo pipefail; f=bazel_tools/tools/bash/runfiles/runfiles.bash
+source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \
+  source "$0.runfiles/$f" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
+  { echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e
+# --- end runfiles.bash initialization v2 ---
+
+


This is so that we can source the functions that remove/replace the autogen build tags from another file (using the rlocation function that is being sourced here- I wanted to do that, to make this file shorter as I had to add some extra functionality to replacing/removing.
Arguably, this adds complexity, but is also an example of how bash libraries can be imported if that was ever useful.

This appears to be a standard way of doing it, see i.e https://github.com/pixie-io/pixie/blob/main/src/e2e_test/px_cluster/test_px_on_gke.sh#L33-L45

It's pretty unreadable, but also quite interesting to look at.
It seems to be attempting to load bazel_tools/tools/bash/runfiles/runfiles.bash from various directories, which may be derived from the name of the current script $0, right?

Thanks for the explanatory comment.

Yes, it is interesting to look at!

It seems to be attempting to load bazel_tools/tools/bash/runfiles/runfiles.bash from various directories, which may be derived from the name of the current script $0, right?

Yes, my understanding of this is that since bazel_tools/tools/bash/runfiles is declared as a dependency of the update-codegen Bazel target (here), it will get added to the 'runfiles' when we bazel run this target- see a limited description of that mechanism here and here. These lines then attempt to source the runfiles.bash script from one of the known locations of these Bazel runfiles.

Perhaps I should add a comment about this.

hack/utils.sh

pkg/controller/certificaterequests/sync_test.go

irbekrm · 2021-09-28T07:53:56Z

devel/addon/ingressnginx/install.sh

    --set controller.config.no-tls-redirect-locations="" \
    --set admissionWebhooks.enabled=false \
    --set controller.admissionWebhooks.enabled=false \
+    --set controller.watchIngressWithoutClass="${INGRESS_WITHOUT_CLASS}" \


This is because this PR bumps Ingress-NGINX controller's version to v1.0.2 when running on kube 1.22. v1.X Ingress-NGINX requires for ingress class name to be defined on Ingress resources (docs).

cert-manager ingress-shim creates Ingresses with our without class name depending on how the users have configured the HTTP-01 solver on the issuer, see the code. Our e2e tests create some Ingresses without the class name (and we probably want to keep doing that, at least for as long as we support Ingress-NGINX versions where the class name is not mandatory) so the tests break with v1 Ingress-NGINX as the Ingresses without class name are ignored by the controller. Adding this flag makes the controller watch Ingresses without class.

I think we should eventually test with a standard v1 ingress controller configuration (that only wathces Ingresses with class name), but I think we don't want to deploy two ingress controllers, so might need to allow for different test configurations for v1 and pre-v1 ingress controllers.
We may also want to make the field descriptions on the solver spec more clear as to when the class name is required.

Ok. I think I get it. Thanks for the explanatory comment.
It might be worth summarizing that explanation as a comment in the code, for the benefit of others who might be tempted to change the versions etc.

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

Updates go.mod for Go 1.17 Signed-off-by: irbekrm <irbekrm@gmail.com>

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

New format of Go build tags gets added Signed-off-by: irbekrm <irbekrm@gmail.com>

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

In Go 1.17 x509.CreateCertificate fails if public key doesn't match private key https://golang.org/doc/go1.17 Signed-off-by: irbekrm <irbekrm@gmail.com>

Environment.CRDs is now a slice of apiextensionsv1.CustomResourceDefinitions instead of client.Object kubernetes-sigs/controller-runtime#1626 (comment) Signed-off-by: irbekrm <irbekrm@gmail.com>

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

So that we don't pull the same image for each test run. Also run helm install with --debug so that it outputs more information. Signed-off-by: irbekrm <irbekrm@gmail.com>

… controller Because we need to support different versions and configurations Signed-off-by: irbekrm <irbekrm@gmail.com>

wallrj

Thanks @irbekrm

That's a lot of work and some impressive hacking to work around the problems with the upgrade.

Some parts I didn't understand, and some things I'd like to see explained as comments in the code rather than as PR comments, but also happy to merge this as-is and improve later.

Unhold when you're happy with it.

/lgtm
/hold

WORKSPACE

devel/addon/ingressnginx/BUILD.bazel

wallrj · 2021-09-30T12:04:11Z

devel/addon/ingressnginx/install.sh

    --set controller.config.no-tls-redirect-locations="" \
    --set admissionWebhooks.enabled=false \
    --set controller.admissionWebhooks.enabled=false \
+    --set controller.watchIngressWithoutClass="${INGRESS_WITHOUT_CLASS}" \


Ok. I think I get it. Thanks for the explanatory comment.
It might be worth summarizing that explanation as a comment in the code, for the benefit of others who might be tempted to change the versions etc.

devel/addon/kyverno/BUILD.bazel

devel/addon/kyverno/install.sh

pkg/controller/certificaterequests/sync_test.go

wallrj · 2021-09-30T12:29:01Z

test/integration/framework/apiserver.go

 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
 	apiextensionsinstall "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/install"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"


We're importing this twice with different aliases.

wallrj · 2021-09-30T12:29:11Z

test/integration/framework/apiserver.go

 		t.Logf("Found CRD with name %q", crd.Name)
 	}
-	patchCRDConversion(crds, webhookOpts.URL, webhookOpts.CAPEM)
-	patchCRDServed(crds)


Wait a minute... I only added this yesterday!

Is it really not needed? If not, then remove the function definition too.

Apologies, I was rebasing this morning and did not realize this was recently added.

I think it may not be needed- I see that served is set to true for all versions in patchCRDConversion function (which was called on the line above) here. I will remove this now, but it would be good if you could double check?

OMG, that's my fault. I originally added it to patchCRDConversion then thought it would be clearer as a separate function and forgot to remove the original place. Sorry about that.

Why does patchCRDConversion now return a new slice of CRDs?
It seems superficial because inside the function we're still mutating the original CRD objects.

If these functions are called patchXYZ then I think it makes sense if they clearly modify the supplied slice of CRDs.

yeah it's a good point about the name and I didn't somehow didn't realize it was modifying those CRDs I think- I will change it back

wallrj · 2021-09-30T12:43:46Z

workspace.bzl

+
+# Check that a minimum version of bazel is being used.
+def check_min_bazel_version(bazel_version):
+    if "bazel_version" in dir(native) and native.bazel_version:


I guess native is builtin. I don't see it imported anywhere.

native is a built-in module and bazel_version appears to be an undocumented method within it :) bazelbuild/bazel#8305

wallrj

BTW I also confirmed that I was able to run all the E2E tests locally with Bazel 4

./devel/cluster/install.sh
./devel/setup-e2e-deps.sh
./devel/run-e2e.sh

Ran 468 of 789 Specs in 1059.924 seconds
SUCCESS! -- 468 Passed | 0 Failed | 0 Pending | 321 Skipped


Ginkgo ran 1 suite in 17m40.508821255s
Test Suite Passed

WORKSPACE

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

irbekrm · 2021-09-30T17:04:26Z

Thanks for the review, all the great comments and verifying that the setup works @wallrj !

I think I've addressed all the comments now.

wallrj

/lgtm

jetstack-bot · 2021-09-30T17:07:39Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [irbekrm,wallrj]

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

irbekrm · 2021-09-30T17:08:18Z

/hold cancel

jetstack-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 27, 2021

irbekrm mentioned this pull request Sep 27, 2021

Update supported Bazel versions cert-manager/website#708

Merged

irbekrm changed the title ~~WIP bumps Go version to 1.17, updates deps and kube libraries~~ bumps Go version to 1.17, updates deps and kube libraries Sep 27, 2021

jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 27, 2021

irbekrm force-pushed the bump_go_and_deps branch from 684bb15 to 4c09b81 Compare September 27, 2021 16:32

irbekrm commented Sep 27, 2021

View reviewed changes

hack/utils.sh Outdated Show resolved Hide resolved

irbekrm commented Sep 27, 2021

View reviewed changes

pkg/controller/certificaterequests/sync_test.go Show resolved Hide resolved

irbekrm force-pushed the bump_go_and_deps branch from 4c09b81 to 16756db Compare September 27, 2021 20:47

irbekrm commented Sep 28, 2021

View reviewed changes

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2021

irbekrm added 6 commits September 30, 2021 10:07

Bumps Go 1.16 -> 1.17

3d6a89e

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

Update kube libraries, regenerate go.mod/go.sum

0679c33

Updates go.mod for Go 1.17 Signed-off-by: irbekrm <irbekrm@gmail.com>

Ensures conversion code temp workaround works for Go 1.17

0889570

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

Runs ./hack/update-all.sh

cb6a746

New format of Go build tags gets added Signed-off-by: irbekrm <irbekrm@gmail.com>

Adds the new format of Go build tags to verify_boilerplate

577757f

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

Fix CertificateRequest test

7e9753c

In Go 1.17 x509.CreateCertificate fails if public key doesn't match private key https://golang.org/doc/go1.17 Signed-off-by: irbekrm <irbekrm@gmail.com>

irbekrm added 6 commits September 30, 2021 10:13

Changes required to use envtest from controller-runtime v0.10

5ff6e2d

Environment.CRDs is now a slice of apiextensionsv1.CustomResourceDefinitions instead of client.Object kubernetes-sigs/controller-runtime#1626 (comment) Signed-off-by: irbekrm <irbekrm@gmail.com>

Enforces a minimum Bazel version

4bab2f4

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

Bumps Bazel deps and ingress-nginx test image version

8370b08

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

e2e tests by default create a k8s 1.22 cluster

485629e

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

Load kyverno image with Bazel

7319d33

So that we don't pull the same image for each test run. Also run helm install with --debug so that it outputs more information. Signed-off-by: irbekrm <irbekrm@gmail.com>

Allows for Ingresses without class field set to be watched by Ingress…

3fa237c

… controller Because we need to support different versions and configurations Signed-off-by: irbekrm <irbekrm@gmail.com>

irbekrm force-pushed the bump_go_and_deps branch from 16756db to 3fa237c Compare September 30, 2021 09:16

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2021

wallrj approved these changes Sep 30, 2021

View reviewed changes

jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2021

jetstack-bot assigned wallrj Sep 30, 2021

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2021

wallrj mentioned this pull request Sep 30, 2021

Updates go.mod to v1.17 #4381

Closed

wallrj approved these changes Sep 30, 2021

View reviewed changes

WORKSPACE Show resolved Hide resolved

jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2021

irbekrm force-pushed the bump_go_and_deps branch 2 times, most recently from 3eb6a1e to 989c8d3 Compare September 30, 2021 15:59

Code review feedback

81bdabf

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

irbekrm force-pushed the bump_go_and_deps branch from 989c8d3 to 81bdabf Compare September 30, 2021 16:23

wallrj approved these changes Sep 30, 2021

View reviewed changes

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2021

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2021

jetstack-bot merged commit cfbb32d into cert-manager:master Sep 30, 2021

irbekrm deleted the bump_go_and_deps branch September 30, 2021 17:08

Conversation

irbekrm commented Sep 27, 2021 • edited by maelvls Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

irbekrm commented Sep 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

irbekrm Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wallrj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wallrj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

irbekrm commented Sep 30, 2021

Uh oh!

wallrj left a comment

Choose a reason for hiding this comment

Uh oh!

jetstack-bot commented Sep 30, 2021

Uh oh!

irbekrm commented Sep 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

irbekrm commented Sep 27, 2021 •

edited by maelvls

Loading

irbekrm Sep 28, 2021 •

edited

Loading