fix(image): make OCI version and revision labels state the truth - #199
Conversation
The profiling image pinned org.opencontainers.image.version to the literal
0.1.0-SNAPSHOT, so the label started lying the moment the project version
moved. Replace it with the distroless sibling's treatment: an ARG
APP_VERSION=dev declared between FROM and the LABEL block, emitted as
${APP_VERSION} in the position the old literal occupied.
The jfr Compose overlay re-specifies only context and dockerfile, so it
inherits the base service's build.args.APP_VERSION — no pom or Compose edit
is required.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CWMcYKQuv3uo5RiNZEin5w
The published image carried no org.opencontainers.image.revision, so an operator holding a digest could recover the source commit only through the mutable sha- tag. Add ARG APP_REVISION=dev beside APP_VERSION on the distroless Dockerfile and emit the revision label from it. release.yml performs no docker build of its own — the harness compose build is the one image build in the lane — so the Compose service's build.args is the only channel reaching the ARG. Plumb APP_REVISION through it and feed it the release-tag commit already resolved by the `commit` step. The smoke step gains a revision leg symmetric with the landed version leg: it reads the label off the pulled digest and fails the release when it differs from the release-tag commit, so a released image whose labels lie does not ship. The operator guide documents the label under "Which tag to use", stating plainly that it is a convenience and not a security control — provenance authority remains the Cosign certificate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CWMcYKQuv3uo5RiNZEin5w
Nothing read the version or revision label back off the image, so the whole
arg chain — Compose build.args to Dockerfile ARG to LABEL — could break with
every step still succeeding and the release lane still going green.
ImageMetadataIT reads both labels off the built api-sheriff:distroless image
with the same `docker image inspect` the release smoke step performs, never
off Dockerfile text: a declaration that reads correctly proves nothing about
what arrived. It resolves the expected values exactly as the Compose
${VAR:-dev} passthrough does, and carries a negative control — a label the
image does not carry must read back empty — so the positive legs cannot pass
on a helper that returns a constant.
ImageLabelActivationWiringTest pins the one hop the image cannot show: a fast
no-Docker surefire guard that the api-sheriff service declares both args in
the ${VAR:-dev} form. That hop is load-bearing because release.yml runs no
docker build of its own. It carries the sibling wiring tests' vacuity guard.
The jfr profile excludes ImageMetadataIT: that lane tags api-sheriff:jfr, so
the IT would inspect an image the profile never builds.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CWMcYKQuv3uo5RiNZEin5w
…ests The pre-commit gate rewrites sources while exiting 0, so the two files added by the previous commit came back modified: Process.waitFor now takes the Duration overload, the SnakeYAML import moves to the sibling wiring tests' ordering, and the wiring test's throws clause widens. Committing the rewrite is what makes the next gate run idempotent instead of dirtying the tree again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CWMcYKQuv3uo5RiNZEin5w
… tests Collapse the three-deep single-caller helper ladder in ImageLabelActivationWiringTest into one no-arg helper, narrow the test's throws clause to IOException, and make ImageMetadataIT's IMAGE constant private. The negative-control leg and the vacuity guard are deliberately kept — they are the anti-vacuity controls the deliverable exists to provide. Co-Authored-By: Claude <noreply@anthropic.com>
…g test The pre-push quality gate's OpenRewrite pass reverted two of the simplify sweep's edits on ImageLabelActivationWiringTest: it restored the `throws Exception` signature and moved the SnakeYAML import back above the JUnit block. The gate is the authority on both, so its output is taken rather than re-applied against it. Re-verified: the gate is idempotent on the resulting tree. Co-Authored-By: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cuioss/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change adds OCI version and revision labels to Docker images, forwards release metadata through Compose and the release workflow, validates labels with integration and smoke tests, prepares JFR output directories, and documents image metadata inspection. ChangesOCI image metadata
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Reviewer Guide 🔍(Review updated until commit f312bf4)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In ImageMetadataIT the image name "api-sheriff:distroless" is hard-coded; consider sourcing this from a shared configuration or an environment variable to avoid divergence if the compose service or tag name changes.
- The docker inspection logic in ImageMetadataIT is implemented via ad‑hoc ProcessBuilder invocation; extracting this into a small reusable utility (e.g., a helper class or method shared by tests and release scripts) would reduce duplication and make future changes to the label read format simpler.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In ImageMetadataIT the image name "api-sheriff:distroless" is hard-coded; consider sourcing this from a shared configuration or an environment variable to avoid divergence if the compose service or tag name changes.
- The docker inspection logic in ImageMetadataIT is implemented via ad‑hoc ProcessBuilder invocation; extracting this into a small reusable utility (e.g., a helper class or method shared by tests and release scripts) would reduce duplication and make future changes to the label read format simpler.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d10f5b5c-3de2-49dd-9788-dfb2ce8e53f8
📒 Files selected for processing (8)
.github/workflows/release.ymlapi-sheriff/src/main/docker/Dockerfile.nativeapi-sheriff/src/main/docker/Dockerfile.native.jfrdoc/user/container-image.adocintegration-tests/docker-compose.ymlintegration-tests/pom.xmlintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/ImageLabelActivationWiringTest.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/ImageMetadataIT.java
|
@sourcery-ai — replying to your review here because the comment carries no resolvable thread id. 1. 2. Extract the Thanks for the review — both points were assessed on their merits, not filtered out by Sourcery being configured as an optional reviewer. |
Three CodeRabbit findings on #199, plus the lane fix the third one exposed. The timeout guard in the label reader was dead code: readAllBytes() ran before waitFor(), so it blocked until EOF and the timeout could only be evaluated once the process had already exited. Reordered to wait -> destroyForcibly on expiry -> read, and moved into a shared ImageLabelInspector so the ordering is stated once rather than duplicated into a copy that could silently regress. ImageLabelActivationWiringTest no longer hardcodes the arg set it checks; it derives it from Dockerfile.native at run time (LABEL-interpolated variables intersected with declared ARGs), which is what makes it catch growth. Verified in both directions: adding an APP_CREATED arg+label to the Dockerfile failed the test with the intended message, and the control was reverted. The pom comment justifying the JFR exclusion claimed no automated run builds that image. That is false about the very profile it sat in, so the justification is replaced and ImageMetadataJfrIT now asserts the JFR image's version label plus an absent-label control. It records why there is no revision leg: Dockerfile.native.jfr declares no APP_REVISION. Running that IT required fixing the JFR lane, which could not start at all. docker-compose.jfr.yml bind-mounts ./target/jfr-recordings at /tmp/jfr-output; whichever compose command touched the service first created that directory as root, and the uid-1001 container then could not write the recording, so the gateway died at startup. The new prepare-jfr-output-dir.sh pre-creates it 1777 -- sticky, so the world write is not also a world delete of the recording -- and is called from both entry points that compose the overlay. ImageMetadataJfrIT passes (2/2). MtlsHandshakeIT fails 2/3 in this lane, which the fix unmasks rather than causes: the same tests pass 3/3 under -Pintegration-tests on the identical tree. Filed as #201. Co-Authored-By: Claude <noreply@anthropic.com>
|
/review |
|
@sourcery-ai — correcting my earlier reply. What landed in On the hard-coded tag: I said it would be parameterized. It was not. On the extraction: I declined it, and then it happened. The Net: suggestion 2 is implemented; suggestion 1 is not. I would rather leave that asymmetry visible than let the earlier comment stand as an accurate description of the branch. |
1 participant
Summary
Makes the OCI provenance metadata on both container images state the truth. Two labels are in scope,
answering the same question from opposite ends: what is this image — the version label, which was a
hard-coded literal on the JFR variant — and which commit produced it — the revision label, which
existed on neither image.
The release-tag commit SHA is plumbed all the way to the published image, and both labels are
asserted on a built image rather than on Dockerfile text, so a broken arg chain fails at PR time
instead of shipping a silently-
devlabel on a release.Changes
Version label — retire the hard-coded literal (
api-sheriff/src/main/docker/Dockerfile.native.jfr)LABEL org.opencontainers.image.version="0.1.0-SNAPSHOT"is replaced by the sibling Dockerfile'sproven treatment:
ARG APP_VERSION=devplusLABEL … ="${APP_VERSION}". The JFR image is built bythe
jfrCompose overlay, which re-specifies onlycontextanddockerfileand therefore inheritsthe base service's
build.args; nothing publishes it, sodevis the honest resolved value. No pomand no compose change was needed.
Revision label — plumb the release-tag commit to the published image
api-sheriff/src/main/docker/Dockerfile.native— addsARG APP_REVISION=devandLABEL org.opencontainers.image.revision="${APP_REVISION}".integration-tests/docker-compose.yml— addsAPP_REVISION: ${APP_REVISION:-dev}to theapi-sheriffservice'sbuild.args. This hop is mandatory, not incidental:release.ymlperformsno
docker buildof its own, so the harness Compose build is the single image build andbuild.argsis the only channel reaching the DockerfileARG..github/workflows/release.yml— exportsAPP_REVISION: ${{ steps.commit.outputs.sha }}into theintegration-test step that builds the image (the same value published as the
sha-registry tag),and extends the published-image smoke check to assert the revision label matches the release-tag
commit.
doc/user/container-image.adoc— documents the label and states explicitly that it is aconvenience for a reader holding the image, not a security control; Cosign's certificate
remains the provenance authority.
Assertions on the built image
integration-tests/src/test/java/.../ImageMetadataIT.java— reads both labels off the builtapi-sheriff:distrolessviadocker image inspect, resolving expected values exactly as theCompose
${VAR:-dev}passthrough does. Carries a negative control asserting that an absent labelreads back empty, so the two positive legs cannot pass against a helper that returns a constant.
integration-tests/src/test/java/.../ImageLabelActivationWiringTest.java— a fast, no-Dockersurefire guard that the committed Compose descriptor actually declares each provenance build arg in
the
${VAR:-dev}form. This pins the one link the built image cannot show; drop an entry and theDockerfile default silently wins while every step still succeeds. Includes a vacuity guard against
an empty
build.argsmap.integration-tests/pom.xml— excludesImageMetadataITfrom thejfrfailsafe execution, whichbuilds and tags
api-sheriff:jfrand would otherwise inspect an image that lane never produces.Test Plan
verify -Ppre-commit) — every deliverable touches abuild.map-pinnedpath class (
Dockerfile*,docker-compose*.yml,.github/workflows/*), so the gate isunavoidable here.
verify)verify -Pintegration-tests -pl integration-tests -am) —ImageMetadataITandImageLabelActivationWiringTestboth green, labels read off the builtimage.
Related Issues
None.
Intent
Problem. Both images misstated their provenance.
Dockerfile.native.jfrhard-codedimage.version="0.1.0-SNAPSHOT"— false the moment the version moved, with no way to become true.Neither image carried
image.revision, so an image in hand could not name its commit.Approach. Make both labels build-supplied (
ARG …=dev+LABEL … ="${ARG}"),devbeing thehonest default for anything never published. The revision value's route is narrower than it looks:
release.ymlruns nodocker buildof its own — the IT harness's Compose build is the project'ssingle image build — so the SHA travels env → compose
build.args→ARG→LABEL, and droppingthe compose hop would ship an empty label while every step still exits zero. That is why the
assertions target the built image, not Dockerfile text, plus one surefire test pinning the
Compose hop an image cannot show.
Non-goals. Cosign's signing block and keyless identity are untouched (settled by #195). No second
provenance mechanism: Cosign's certificate stays the authority; the label is a convenience, and the
user doc says so, so no one mistakes a mutable label for a security control. Release trigger,
version-changed guard and Trivy threshold unchanged. The JFR image's label is deliberately not
machine-asserted — no automated run builds it.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation