Make controller-gen/kustomize/golangci-lint/envtest tool-binary caching reliable - #2367
Open
kaovilai wants to merge 5 commits into
Open
Make controller-gen/kustomize/golangci-lint/envtest tool-binary caching reliable#2367kaovilai wants to merge 5 commits into
kaovilai wants to merge 5 commits into
Conversation
go-install-tool-branch only installs when the binary is missing, never verifying the pinned version against what's already on disk. Once a binary lands in bin/<branch>/, it's reused forever across branch switches and version bumps since bin/ is gitignored and nothing else resets it. kustomize and controller-gen's targets also weren't fully .PHONY (only the wrapper name was, not the binary path), so Make's own mtime-based staleness check could skip their recipe entirely before any version check ran. Introduce go-install-tool-versioned, which compares a sidecar <binary>.version marker file against the pinned version instead of introspecting the binary's own --version output. Binary introspection isn't reliable for every tool installed this way: kustomize's `version` command depends on ldflags its own release process sets, which `go install` doesn't set, so identically-installed kustomize binaries were observed reporting "(devel)", an unexpanded `$Format:%H$` git-archive placeholder, or a correct version string depending on unrelated build-time factors. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Makefile validates pinned tool versions and host architecture from Go binary metadata. It reinstalls mismatched binaries, preserves applicable unversioned symlinks, and removes sidecar marker handling. ChangesTool installation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 530-550: Update the versions target to read the .version sidecar
for marker-managed tools such as KUSTOMIZE instead of invoking the binary’s
version command. Preserve existing binary introspection for tools without marker
files, and reuse the marker path established by go-install-tool-versioned.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e93a7bca-24c0-4d5d-82da-82c240a84ce2
📒 Files selected for processing (1)
Makefile
Folds in the fix from openshift#2152 (same author, same theme: harden Makefile tool-binary caching under bin/). A containerized Make target (e.g. podman/docker build with a different GOARCH) can write a linux binary into the shared bin/ directory, replacing the native host binary `make test` needs — and since setup-envtest isn't branch-scoped like the other three tools, that binary is shared across every branch checkout too. openshift#2152's own check used `$(ENVTEST) --help`'s exit code as the "is this binary compatible" signal, but that's unreliable the same way relying on kustomize's --version output was: setup-envtest's own --help exits 2 by its own convention even on a perfectly good binary, so that check would have triggered a reinstall on every single invocation, permanently defeating the cache. Verified by cross-compiling a real linux/amd64 setup-envtest and running it on this darwin/arm64 host: the shell reports exit code 126 specifically (POSIX "found but cannot execute" / exec format error) — check that instead of any nonzero exit. Also added $(ENVTEST) to the .PHONY line, matching the fix already applied to controller-gen/kustomize in the previous commit: without it, Make's own mtime-based staleness check can skip the recipe (and therefore this check) entirely once the binary file exists. Closes openshift#2152 Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
544-566: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake marker-managed caches platform-specific.
go-install-tool-versionedchecks only the version marker. The cache paths for Controller-Gen, Kustomize, and GolangCI-Lint omitGOOSandGOARCH. A same-version binary from another architecture can be reused and fail at execution. Include the platform in the cache key or validate the binary before reuse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 544 - 566, The go-install-tool-versioned cache currently reuses markers based only on tool version, allowing binaries from another platform to be selected. Update go-install-tool-versioned and the Controller-Gen, Kustomize, and GolangCI-Lint cache paths or marker validation to include GOOS and GOARCH, ensuring reuse only occurs for the current platform while preserving version checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Makefile`:
- Around line 544-566: The go-install-tool-versioned cache currently reuses
markers based only on tool version, allowing binaries from another platform to
be selected. Update go-install-tool-versioned and the Controller-Gen, Kustomize,
and GolangCI-Lint cache paths or marker validation to include GOOS and GOARCH,
ensuring reuse only occurs for the current platform while preserving version
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5cedc214-40c1-438d-a936-5286a91d2bf1
📒 Files selected for processing (1)
Makefile
Member
Author
|
/test 5.0-e2e-test-aws |
The version-marker check alone isn't enough: a binary can have a correct .version marker but still be the wrong architecture, e.g. if a containerized build with a different GOARCH bind-mounts the host's bin/ directory (this Makefile documents exactly that workflow for `make test`: `docker run --platform linux/amd64 -v $PWD:$PWD ...`). Anything that does `go install` in there writes onto the host's real bin/ tree since it's the same mounted path, not a copy. That's not envtest-specific — it can happen to any of these four cached tool binaries. go-install-tool-versioned now also probes `$(1) --version` and checks specifically for exit code 126 (POSIX "found but cannot execute" / exec format error), same technique as the envtest fix. Verified all three tools (controller-gen, kustomize, golangci-lint) correctly detect and repair a wrong-arch binary even when its .version marker already matches the pinned version: cross-compiled a real linux/amd64 binary for each, copied it over the working native binary, confirmed each was detected and replaced. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
An independent second-opinion review caught a severe bug in the
previous commit: the exit-126 probe (`$(1) --version`, then a
separate `if [ $? -eq 126 ]`) is a bare command outside any &&/||/if
guard. Under `set -e` — which this Makefile's `.SHELLFLAGS = -ec`
enables, honored by GNU Make 3.82+ — a bare failing command aborts
the whole recipe immediately, before the exit-code check ever runs.
My local macOS Make (3.81) silently ignores .SHELLFLAGS, so none of
my testing could have caught this.
Confirmed for real: installed GNU Make 4.4.1 via Homebrew and
reproduced the crash directly — `make kustomize` died with
"Error 1" every time the binary already existed, because kustomize's
own `--version` exits 1 by its own convention even on a perfectly
healthy binary (same root cause class as the setup-envtest --help
issue already fixed: a tool's own nonzero-on-success exit code was
being misread as "broken").
Replaced the whole exit-code-probe approach with `go version -m`,
which reads a binary's embedded module version and GOOS/GOARCH
directly from its build info, without executing it at all. This is
strictly better, not just a patch:
- No execution means no exit-code heuristic to get wrong, and no
-e hazard, for any of these four tools.
- No execution also closes a gap the review surfaced: exit-126
detection cannot work at all inside a container with
qemu-user-static/binfmt_misc registered (standard in multi-arch
CI/build images), since a wrong-arch binary just runs under
emulation and returns its own exit code instead of an
exec-format-error — defeating the check silently in exactly the
environments it was meant to protect.
- Drops the sidecar `.version` marker file entirely — go version -m
reads the real, authoritative module version already embedded in
the binary, so there's nothing separate left to go stale or
desync from a copied/moved binary.
envtest's bespoke arch-only check (added in the previous commit) is
replaced outright by a plain call to the same go-install-tool-versioned
macro used by the other three tools, rather than patched in place —
one verified mechanism instead of two.
Verified end-to-end with GNU Make 4.4.1 specifically (not just the
macOS-default 3.81, which cannot exercise this class of bug): fresh
install, idempotent re-run (previously the exact crash case for
kustomize/envtest), and wrong-arch detection+repair, for all four
tools. `make generate manifests bundle` zero diff, `make test` fully
green.
Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
sseago
previously approved these changes
Aug 12, 2026
Both go-install-tool-versioned's doc comment and the envtest section's note narrated the debugging process that led here (earlier attempts, "confirmed with real GNU Make 4.4.1", "the previous exit-code-126 version... reliably crashed", a specific past PR reference) rather than just stating why the current code is shaped this way. Keeps the durable WHY (unreliable tool self-report, the set -e hazard as a standing fact about this Makefile, the qemu/binfmt_misc blind spot) as present-tense design rationale instead. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
shubham-pampattiwar
approved these changes
Aug 12, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, shubham-pampattiwar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@kaovilai: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Folds in #2152 (same author, same theme, reviewer bandwidth is thin — consolidating into one PR rather than two small ones).
Summary
go-install-tool-branchonly installs a build tool (controller-gen, kustomize, golangci-lint) when the binary is missing, never verifying the pinned version against what's already on disk. Once a binary lands atbin/<branch>/<tool>, it's reused forever — even across branch switches, even afterCONTROLLER_TOOLS_VERSION/KUSTOMIZE_VERSION/GOLANGCI_LINT_VERSIONchange — becausebin/is gitignored and nothing else resets it.kustomize/controller-gentargets also weren't fully.PHONY, so Make's own mtime-based staleness check could skip the recipe entirely before any check even ran.None of the four tools (
controller-gen/kustomize/golangci-lint/setup-envtest, the last folded in from #2152) were checked for architecture compatibility either. This repo's own Makefile documents runningmake testinsidedocker run --platform linux/amd64 -v $PWD:$PWD ...to reproduce Prow's CI environment — that bind-mounts the real host directory into a forced-arch container, so anygo installin there writes a foreign-arch binary straight onto the host'sbin/tree (same path, not a copy). Not envtest-specific — any of these four cached tool binaries can be clobbered this way.Design
go-install-tool-versionedreads a binary's embedded build info viago version -m— the module version andGOOS/GOARCHa binary was built with — instead of executing it or trusting a sidecar marker file:modline's version, not the tool's own--versionoutput. That output isn't a reliable version or health signal on its own: it can depend on ldflags a tool's own release process sets (whichgo installdoesn't set — verified in practice: threebin/*/kustomizebinaries on this machine, installed identically, reported(devel), an unexpanded$Format:%H$placeholder, and a correctv5.2.1), and some tools exit nonzero on--versioneven when perfectly healthy (kustomizeexits1,setup-envtest's--helpexits2). A bare probe command that exits nonzero is also dangerous under this Makefile's.SHELLFLAGS = -ec(enablesset -e, honored by GNU Make 3.82+): it aborts the whole recipe unless wrapped in anif/||guard.build GOOS=.../GOARCH=...lines against this host's own, instead of executing the binary and interpreting its exit code — which can't detect a wrong-arch binary at all inside a container withqemu-user-static/binfmt_miscregistered (standard for multi-arch CI/build images), since the foreign-arch binary just runs under emulation and returns its own exit code rather than an exec-format-error.Reading embedded build info sidesteps both problems at once: no execution means no exit-code heuristic to get wrong and no qemu blind spot.
setup-envtest's arch check is folded into this same shared macro rather than kept as a separate mechanism.Known limitations
ENVTESTPATH's arch selection has a separate, pre-existing bug — tracked as ENVTESTPATH arch-selection is decided at Makefile parse time, picks amd64 on any cold bin/ regardless of host arch #2377, not fixed here. Itsifeq-based amd64 fallback is decided at Makefile-parse time and picks the amd64-forced variant on any coldbin/, regardless of actual host arch — confirmed via direct repro (see ENVTESTPATH arch-selection is decided at Makefile parse time, picks amd64 on any cold bin/ regardless of host arch #2377). It's invisible on amd64 CI (no CI/release-branch risk) and only affects arm64 local dev with a cold checkout, so it's out of scope here rather than a reason to expand this PR's diff a third time.bin/<tool>→ last-branch-wins, pre-existing behavior) are a separate, lower-priority observation, also worth a follow-up.Testing
Verified against GNU Make 4.4.1 specifically (installed via Homebrew), not just the macOS-default Make 3.81 — the latter silently ignores
.SHELLFLAGS, so it can't exerciseset -erecipe behavior at all.rm -rf bin/oadp-devthengmake controller-gen/gmake kustomize/gmake golangci-lint/gmake envtestindividually — each installs fresh.linux/amd64binary, copied it over the working native binary) → each correctly detected viago version -m'sGOARCH/GOOSfields, removed, reinstalled with a valid native binary.gmake generate manifests bundle→ zero diff against a clean checkout.gmake test→ exit 0, all packages pass,api is up to date,bundle is up to date.coderabbit review --agent→ 0 findings.Cherry-picked to
oadp-1.4as #2368.Note
Responses generated with Claude
Summary by CodeRabbit