Upgrade makefile-modules and move to Go 1.27.1 - #831
Conversation
Ran `make upgrade-klone` followed by `make generate`. The motivation is govulncheck, which has failed on master every night since at least 4 September. Go 1.27.1 clears seven of the eight vulnerabilities our code calls, all of them in the standard library: GO-2026-6218, GO-2026-6091, GO-2026-6090, GO-2026-6089, GO-2026-5972, GO-2026-5856 and GO-2026-5026. The remaining one is in golang.org/x/text and is handled separately. Three changes were needed to keep the build green: - Go 1.27 re-implemented encoding/json on top of encoding/json/v2, so a failing MarshalJSON on a json.RawMessage now names *jsontext.Value rather than json.RawMessage. One test asserted that whole message, so it now asserts the fragments that are not a standard library type name. - The upgrade enables the nolintlint linter, which rejects a space between the comment marker and the directive. Three existing directives needed the space removing. - Upstream replaced the make-self-upgrade workflow with a Renovate JSONata manager, so `make generate` deletes it. Our .github/renovate.json5 already extends the makefile-modules config that carries the replacement. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Richard Wall <richard.wall@cyberark.com>
FelixPhipps
left a comment
There was a problem hiding this comment.
Claude review
Read the full diff, separated generated from hand-written changes, and checked the claims against the repo.
Verdict: approve, with one pre-merge ask (run the e2e labels) and two follow-ups.
The description is excellent — the generated/hand-written split is accurate, and documenting the failed VENDORED_GO_VERSION := 1.26.6 shortcut saves the next person the same dead end. verify and test are both green.
The three hand-written fixes are correct
All verified against the branch:
.golangci.yaml+make/_shared/go/.golangci.override.yaml—nolintlintadded in both, so the generated file and its source stay consistent. The three// nolint:→//nolint:fixes (pkg/agent/config.go:687,:740,pkg/client/client_venconn_test.go:361) are the complete set; lint passes.make/_shared/repository-base/01_mod.mk— therm -fof the twomake-self-upgradefiles is genuinely upstream, carries its ownTODO: Remove when all downstream repos are updated, and.github/renovate.json5does extendgithub>cert-manager/makefile-modules:renovate-config.json5. The mechanism is in place as described.GO_TOOLCHAIN_VERSIONintools/00_mod.mk— I initially read theGO_TOOLCHAIN_VERSION := go$(VENDORED_GO_VERSION)at line 368 as clobbering thego env GOVERSIONprobe above it; it doesn't, it's theelsebranch of theifeq ($(VENDOR_GO),)at line 340. Correct as generated.
Please run the label-gated e2e suites before merging
test-e2e, ark-test-e2e and ngts-test-e2e all skipped — they're gated on the test-e2e / test-ark / test-ngts labels (tests.yaml:83,120,154). The Go bump is the safe part of this PR; the unexercised part is everything else the toolchain jump dragged along:
- kind v0.32.0 → v0.33.0, with
kind_image_latestmoving Kubernetes 1.36 → 1.37, and kubectl to v1.37.0 - helm 4.2.0 → 4.2.4, kyverno 1.18.1 → 1.19.0, istioctl 1.30.0 → 1.31.0
- the whole release toolchain: ko 0.18.1 → 0.19.1, syft 1.45.0 → 1.51.1, goreleaser 2.16.0 → 2.18.0, cosign, oras, crane,
helm-toolv0.5.3 → v0.6.0,image-toolv0.1.0 → v0.2.0
Since the stated goal is cutting v1.12.0-alpha.0, shipping untested release tooling is the main residual risk here. Add the three labels and let them run.
Two follow-ups worth filing now, not "someone should pick up"
-
The self-upgrade gap. Confirmed your suspicion:
gh search prs --repo jetstack/jetstack-secure --app renovatereturns zero results ever. So after this merges, nothing in-repo bumpsklone.yaml. That's not hypothetical — it's precisely what produced this PR (19 Aug → 8 Sep of silent failure, turning a patch bump into a minor). Worth confirming the Renovate app is actually installed on the repo and that the JSONata manager fires, ideally before merge; if that can't be checked quickly, file and assign the issue rather than leaving it in the description. -
.github/workflows/govulncheck.yamlpin drift. Your note that it's manually maintained and unaffected is accurate — but it's onactions/checkoutv5.0.0 andactions/setup-gov6.0.0, i.e. it was already two majors behind the old template, not just the new one. Pre-existing and out of scope, but this PR vendorspinactv4.1.1 for the first time, which is the tool for exactly this. Natural follow-up.
Minor, non-blocking
pkg/client/client_file_test.go — asserting fragments over the full string is the right call. Two small things:
- Consider adding
"error calling MarshalJSON"as a third fragment. It's stable across bothencoding/jsonimplementations and it's the actual behaviour under test; as written, the two fragments would still pass if the error arrived from an unrelated path that happened to mention the invalid character. - Two mutually exclusive
expectedError/expectedErrorContainsfields with two early-return branches is a bit more machinery than needed — a singleexpectedErrorContains []stringused by both error cases would collapse it. AlsoexpectedJSON: "[]"on the two error cases is dead now that both branches return early (pre-existing, tidy-up only).
go.mod still says go 1.26.4. Not a govulncheck problem — CI resolves the toolchain through make print-go-version → 1.27.1, and stdlib findings key off the build toolchain — and keeping the directive low is a defensible minimum-version choice for an importable module. Just confirm it's deliberate rather than missed.
Kubernetes 1.33 dropped from the kind matrix. Nothing sets K8S_VERSION today, so your "nothing to update" is right. The only consequence is that make ... K8S_VERSION=1.33 now hits fatal_if_undefined (kind/00_mod.mk:28). Fine unless a support policy still claims 1.33 — I found no such claim in the README.
🤖 Generated with Claude Code
|
Thanks, that is a genuinely useful review — particularly catching that The e2e suites are running now. I have added One trap worth recording for the next person. Adding the labels on its own does nothing. On Both follow-ups are already filed and assigned internally, not left in the description. Your Renovate finding matches mine exactly: zero Renovate PRs on this repository, ever. That is tracked, along with the point that once this merges there is nothing in-repo bumping The On the test changes, I agree and will do them in a follow-up rather than hold this PR. Adding And agreed on Kubernetes 1.33: the only consequence is that an explicit |
Why now?
govulncheckhas failed on master every night since at least 4 September, and it blocks thev1.12.0-alpha.0pre-release we want to cut for end-to-end testing.make verify-govulncheckreported 8 vulnerabilities that our code actually calls.This PR fixes 7 of the 8, all in the standard library, by moving the vendored Go toolchain from 1.26.4 to 1.27.1:
net/urlhtml/templatecrypto/tlsnet/httpencoding/asn1crypto/tlsnet/httpThe eighth, GO-2026-5970 in
golang.org/x/text, is a dependency bump rather than a toolchain bump, so it is #832. The two are independent and can merge in either order; together they take govulncheck to zero.What I did
Ran
make upgrade-klonefollowed bymake generate. Everything undermake/_shared/and the two deleted workflow files is generated output, not hand-written.I first tried the smaller change of editing
VENDORED_GO_VERSIONto 1.26.6 by hand. That does not work:generate-kloneis a generate target, somake verifyre-vendors the file from the pinnedrepo_hashand the edit disappears. Upstreammainis on 1.27.1, somake upgrade-kloneis the only route.Three things needed fixing to stay green
1.
encoding/jsonerror text changed. Go 1.27 re-implementedencoding/jsonon top ofencoding/json/v2. A failingMarshalJSONon ajson.RawMessagenow names*jsontext.Value:TestFileClient_PostDataReadingsWithOptionsasserted that whole string withassert.EqualError. Rather than swap in the new string, which would break again on the next toolchain bump, it now asserts the two fragments that are not a standard library type name.2. The
nolintlintlinter is now enabled. It rejects a space between the comment marker and the directive. Three pre-existing directives needed the space removing (// nolint:to//nolint:).3. The
make-self-upgradeworkflow is deleted. This is deliberate upstream, not collateral damage. cert-manager/makefile-modules@36c614a8 replaced it with a Renovate JSONata manager, andgenerate-basenow explicitly removes the old files. Our.github/renovate.json5already extendsgithub>cert-manager/makefile-modules:renovate-config.json5, which is where the replacement manager lives, so the mechanism is in place.Please look at this bit
Two related observations that this PR does not fix, and that someone should pick up:
make-self-upgradehas been failing every night since at least 19 August, so we have been getting no automatic klone upgrades for about three weeks. That is why this jump is a whole minor version rather than a patch.klone.yaml, and we will be back in the same position in a few months.Neither is a regression introduced here, and the deleted workflow was broken anyway, but the gap is worth closing.
Testing
All green locally on Go 1.27.1: verify, 429 unit tests, and govulncheck down to the single expected finding
make verify— passes, including golangci-lint with the newnolintlint, helm-unittest (82 tests, 6 suites) and the Kyverno pod security standards checks.make test-unit:make verify-govulncheck— reduced from 8 called vulnerabilities to 1, the expectedx/textone:Other generated changes: kind images drop 1.33 and add 1.37, base image digests, a new helm-diff target
kindbumped v0.32.0 to v0.33.0. The image matrix drops Kubernetes 1.33 and adds 1.37. Nothing in this repo's workflows or makefiles referenceskind_image_kube_1.33or aK8S_VERSION, so there is nothing to update.quay.io/jetstack/base-staticandbase-static-csidigests moved to currentlatest.make helm-difftarget arrived from upstream, for diffing the locally built chart against a released version. Unused here, but handy for release checks.govulncheck.yamltemplate picked up neweractions/checkoutandactions/setup-gopins. Note our real.github/workflows/govulncheck.yamlis manually maintained, as its header says, so it keeps its existing pins and is unaffected.