pprof now installed in profiling-deps images - #5930
Conversation
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it introduces a new external supply-chain step (downloading and installing a pinned Go toolchain plus a pinned pprof commit, with hardcoded sha256 checksums) into every profiling-deps image, a human look would still be worthwhile.
What was reviewed: the new Go-toolchain-download-and-pprof-install RUN step added identically to profiling-deps.deb.m4 and profiling-deps.rpm.m4; confirmed the GO_SHA256_AMD64/ARM64 values and PPROF_COMMIT are correctly-sized hex strings (64/64/40 chars); confirmed the five generated per-OS Dockerfile.profiling-deps files stay byte-for-byte in sync with their m4 templates; and reviewed the comment-only doc updates in dockerfile.mk, the refresh workflow, and profiling.deb.m4/profiling.rpm.m4.
Extended reasoning...
Overview
This PR adds a new RUN layer to the .deb and .rpm profiling-deps m4 templates that downloads a pinned Go toolchain, verifies it via sha256, uses it to go install a pinned commit of github.com/google/pprof, and then removes the Go toolchain to keep the image slim. It also updates comment text (gperftools rationale, tool lists) in dockerfile.mk, the refresh workflow, and profiling.deb.m4/profiling.rpm.m4. All five per-OS generated Dockerfile.profiling-deps files were regenerated and verified byte-identical in intent to their m4 sources.
Security risks
The main supply-chain-relevant surface is the new curl-download-and-checksum-verify step for the Go toolchain (fetched over HTTPS with --proto '=https' --tlsv1.2, verified against a hardcoded sha256) and the go install of a pinned pprof commit (not sha-verified beyond Go's own module checksum database, which is standard practice for go install @ commit). I manually double-checked the length of the two hardcoded sha256 values and the pprof commit hash via regex matching; they are correctly formatted (64 hex chars for the sha256 values, 40 hex chars for the git commit). I did not independently verify these hashes/commit against Google's actual published values (no network access in this sandboxed check), but the pinning approach itself is sound and matches the existing pattern used for cargo/inferno installs earlier in the same file.
Level of scrutiny
This is a Docker build/tooling change confined to CI/profiling image generation — it does not touch FreeRADIUS server code, protocol handling, or auth/crypto paths. The change is mechanical and follows the existing pattern in the file (similar to the pre-existing rustup/cargo install block). That said, adding a new external binary download with hardcoded checksums to a build pipeline is exactly the kind of change where a second set of eyes checking the actual checksum values against upstream is valuable, since an incorrect checksum would only ever fail loudly (safe direction) but a checksum that was accidentally computed against the wrong artifact would not be caught by this review.
Other factors
The bug hunter's candidate list flagged the hard exit 1 on unsupported architectures and the fact that arm64 is never exercised by CI — both are minor, arguably intentional design choices (fail loudly rather than silently produce a broken image) rather than functional bugs, and were already left as inline candidates rather than being escalated as blocking. No CODEOWNERS/ANTOWNERS restrictions apply to these paths, and the PR author states they verified the change locally by running a full profiling-mode test.
|
@claude, there's no package manager that could install pprof? |
|
Claude finished @arr2036's task in 2m 13s —— View job Answer: is there a package manager that could install
|
A minor change made to the profiling images. pprof is now installed as part of the profiling-deps* images. All associated Dockerfiles also updated.
Update verified locally by running
make test.multi-server.ldap.short_ci MODE=profiling. Build and profiling test working as expected.