Skip to content

Upgrade makefile-modules and move to Go 1.27.1 - #831

Merged
wallrj-cyberark merged 1 commit into
masterfrom
upgrade-klone-go-1.27
Sep 8, 2026
Merged

Upgrade makefile-modules and move to Go 1.27.1#831
wallrj-cyberark merged 1 commit into
masterfrom
upgrade-klone-go-1.27

Conversation

@wallrj-cyberark

@wallrj-cyberark wallrj-cyberark commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Why now?

govulncheck has failed on master every night since at least 4 September, and it blocks the v1.12.0-alpha.0 pre-release we want to cut for end-to-end testing. make verify-govulncheck reported 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:

ID Package
GO-2026-6218 net/url
GO-2026-6091 html/template
GO-2026-6090 crypto/tls
GO-2026-6089 net/http
GO-2026-5972 encoding/asn1
GO-2026-5856 crypto/tls
GO-2026-5026 net/http

The 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-klone followed by make generate. Everything under make/_shared/ and the two deleted workflow files is generated output, not hand-written.

I first tried the smaller change of editing VENDORED_GO_VERSION to 1.26.6 by hand. That does not work: generate-klone is a generate target, so make verify re-vendors the file from the pinned repo_hash and the edit disappears. Upstream main is on 1.27.1, so make upgrade-klone is the only route.

Three things needed fixing to stay green

1. encoding/json error text changed. Go 1.27 re-implemented encoding/json on top of encoding/json/v2. A failing MarshalJSON on a json.RawMessage now names *jsontext.Value:

- expected: "...for type json.RawMessage: invalid character 'x'..."
+ actual:   "...for type *jsontext.Value: invalid character 'x'..."

TestFileClient_PostDataReadingsWithOptions asserted that whole string with assert.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 nolintlint linter 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-upgrade workflow is deleted. This is deliberate upstream, not collateral damage. cert-manager/makefile-modules@36c614a8 replaced it with a Renovate JSONata manager, and generate-base now explicitly removes the old files. Our .github/renovate.json5 already extends github>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-upgrade has 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.
  • Renovate has never opened a PR on this repository. If it is not actually onboarded here, deleting the self-upgrade workflow leaves nothing upgrading 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 new nolintlint, helm-unittest (82 tests, 6 suites) and the Kyverno pod security standards checks.

make test-unit:

tests=429 failures=0 errors=0

make verify-govulncheck — reduced from 8 called vulnerabilities to 1, the expected x/text one:

Vulnerability #1: GO-2026-5970
    Infinite loop on invalid input in golang.org/x/text
  Module: golang.org/x/text
    Found in: golang.org/x/text@v0.37.0
    Fixed in: golang.org/x/text@v0.39.0

Your code is affected by 1 vulnerability from 1 module.
Other generated changes: kind images drop 1.33 and add 1.37, base image digests, a new helm-diff target
  • kind bumped 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 references kind_image_kube_1.33 or a K8S_VERSION, so there is nothing to update.
  • quay.io/jetstack/base-static and base-static-csi digests moved to current latest.
  • A new make helm-diff target arrived from upstream, for diffing the locally built chart against a released version. Unused here, but handy for release checks.
  • The vendored govulncheck.yaml template picked up newer actions/checkout and actions/setup-go pins. Note our real .github/workflows/govulncheck.yaml is manually maintained, as its header says, so it keeps its existing pins and is unaffected.

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 FelixPhipps left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.yamlnolintlint added 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 — the rm -f of the two make-self-upgrade files is genuinely upstream, carries its own TODO: Remove when all downstream repos are updated, and .github/renovate.json5 does extend github>cert-manager/makefile-modules:renovate-config.json5. The mechanism is in place as described.
  • GO_TOOLCHAIN_VERSION in tools/00_mod.mk — I initially read the GO_TOOLCHAIN_VERSION := go$(VENDORED_GO_VERSION) at line 368 as clobbering the go env GOVERSION probe above it; it doesn't, it's the else branch of the ifeq ($(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_latest moving 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-tool v0.5.3 → v0.6.0, image-tool v0.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"

  1. The self-upgrade gap. Confirmed your suspicion: gh search prs --repo jetstack/jetstack-secure --app renovate returns zero results ever. So after this merges, nothing in-repo bumps klone.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.

  2. .github/workflows/govulncheck.yaml pin drift. Your note that it's manually maintained and unaffected is accurate — but it's on actions/checkout v5.0.0 and actions/setup-go v6.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 vendors pinact v4.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 both encoding/json implementations 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 / expectedErrorContains fields with two early-return branches is a bit more machinery than needed — a single expectedErrorContains []string used by both error cases would collapse it. Also expectedJSON: "[]" 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

@wallrj-cyberark wallrj-cyberark added test-ark test-e2e To signal e2e test job to be run test-ngts labels Sep 8, 2026
@wallrj-cyberark

Copy link
Copy Markdown
Contributor Author

Thanks, that is a genuinely useful review — particularly catching that GO_TOOLCHAIN_VERSION is the else branch rather than a clobber, and then saying so rather than leaving it as a question.

The e2e suites are running now. I have added test-e2e, test-ark and test-ngts. Agreed on the reasoning: the Go bump is the safe part, and the release toolchain the jump dragged along (ko, syft, goreleaser, cosign, helm-tool v0.6.0, image-tool v0.2.0) is exactly what an alpha would exercise first.

One trap worth recording for the next person. Adding the labels on its own does nothing. tests.yaml uses on: pull_request: {}, which is the default event types of opened, synchronize and reopenedlabeled is not among them. A re-run does not help either, because it replays the original event payload, which had no labels. I had to close and reopen the PR to get a fresh payload. Anyone following the "just add the label" advice will otherwise sit watching a run that never starts.

On go.mod still saying go 1.26.4: deliberate, and it matches prior art. I checked rather than assumed. The last comparable change here, 961ed0c ("make upgrade-klone to bump go to 1.26.1"), touched only klone.yaml and make/_shared/tools/00_mod.mk, leaving go.mod alone. So the two values are not coupled by convention, and the fact that both read 1.26.4 before this PR was drift rather than enforcement. Your reasoning for keeping the directive low on an importable module is the same conclusion I came to, so I would rather not raise it without a reason.

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 klone.yaml. The e2e label gating is tracked separately, including the question of whether the ark-test-e2e 400 "conflicting tagging values" flake is still real, since that gate was explicitly marked TEMPORARY. There is a third one for the nightly govulncheck failing silently for weeks, which is how this whole thing went unnoticed.

The pinact observation is a good catch and new to me. You are right that .github/workflows/govulncheck.yaml was already two majors behind the old template, not just the new one, and that this PR vendors pinact v4.1.1 for the first time. I have added that to the dependency-automation item rather than let it drift again.

On the test changes, I agree and will do them in a follow-up rather than hold this PR. Adding "error calling MarshalJSON" as a third fragment is the substantive one — you are right that as written the assertion would pass if the error arrived from an unrelated path that happened to mention the invalid character, which weakens exactly the thing the case exists to check. Collapsing the two fields into a single expectedErrorContains and dropping the now-dead expectedJSON on the error cases is the obvious tidy-up alongside it.

And agreed on Kubernetes 1.33: the only consequence is that an explicit K8S_VERSION=1.33 now hits fatal_if_undefined, and I could not find a support claim for it either.

@wallrj-cyberark
wallrj-cyberark merged commit 0c29720 into master Sep 8, 2026
10 checks passed
@wallrj-cyberark
wallrj-cyberark deleted the upgrade-klone-go-1.27 branch September 8, 2026 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test-ark test-e2e To signal e2e test job to be run test-ngts

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants