feat: add OCIVolumes feature gate with ORAS OCI volume support - #342
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR adds ORAS OCI volume support: a new ChangesORAS OCI Volumes Support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
6a426f0 to
188f07f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
internal/common/tasks/tasks.go (2)
906-946: 🧹 Nitpick | 🔵 TrivialCluster prerequisite:
ImageVolumefeature gate must be enabled.
OCIVolumes()relies oncorev1.ImageVolumeSource, which is gated by Kubernetes' ownImageVolumefeature (alpha in 1.31, beta since 1.33) and is still disabled by default, because not all container runtimes have full support for it. It also requires a compatible container runtime version (CRI-O ≥1.31, or containerd ≥2.1.0 for alpha support). If the target OpenShift cluster'sImageVolumegate isn't enabled, pods requesting this volume type will fail to schedule/validate even thoughOCIVolumesis Alpha-gated at the operator level.Since the PR objectives mention OpenShift cluster testing, worth confirming this cluster-side prerequisite is documented (e.g., install docs/README) so operators enabling
OCIVolumes: trueknow the underlying k8s feature gate dependency.🤖 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 `@internal/common/tasks/tasks.go` around lines 906 - 946, Document the cluster-side prerequisite for the ImageVolume dependency used by OCIVolumes(), since applyOCIVolumeMounts() and OCIVolumes() rely on corev1.ImageVolumeSource. Add a note in the install/docs/README explaining that enabling BuildConfig.UseOCIVolumes requires the Kubernetes ImageVolume feature gate and a compatible runtime, so operators know not to turn on the feature unless the OpenShift cluster supports it.
911-924: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueMount applied to all steps regardless of need.
applyOCIVolumeMountsunconditionally appends theoras-toolsmount to every step of every task it's applied to (e.g.find-manifest-file/build-imagesteps inGenerateBuildAutomotiveImageTasknever invokeoras). Restricting the mount to steps that actually need it would reduce unnecessary volume exposure across containers (least privilege), though the risk is low since the mount is read-only.♻️ Example: scope mounts to specific step names
-func applyOCIVolumeMounts(task *tektonv1.Task, buildConfig *BuildConfig) { +func applyOCIVolumeMounts(task *tektonv1.Task, buildConfig *BuildConfig, stepNames ...string) { if buildConfig == nil || !buildConfig.UseOCIVolumes || buildConfig.OrasImage == "" { return } + allowed := map[string]bool{} + for _, n := range stepNames { + allowed[n] = true + } + for i := range task.Spec.Steps { step := &task.Spec.Steps[i] + if len(allowed) > 0 && !allowed[step.Name] { + continue + } step.VolumeMounts = append(step.VolumeMounts, corev1.VolumeMount{ Name: ociVolumeNameOras, MountPath: ociMountPathOras, ReadOnly: true, }) } }🤖 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 `@internal/common/tasks/tasks.go` around lines 911 - 924, `applyOCIVolumeMounts` currently adds the `ociVolumeNameOras` mount to every step in a task, even for steps like `find-manifest-file` and `build-image` that do not use `oras`. Update the logic in `applyOCIVolumeMounts` to scope the mount only to the specific step names that actually need it, using `task.Spec.Steps` and the step `Name` field to identify them, so the mount is not appended to unrelated steps.internal/controller/imagebuild/controller.go (1)
905-911: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate OCI-volumes feature-gate wiring (3 sites).
This exact block — construct
featuregates.NewFromConfig(&spec), checkOCIVolumes, setUseOCIVolumes/OrasImage— is repeated verbatim here, again at Line 2598-2604 in this file, and a third time ininternal/controller/operatorconfig/controller.go(Lines 709-713). The codebase already has a precedent for factoring this kind of sharedBuildConfigpopulation out (seecontrollerutils.ApplyTrustedCABundleFromOSBuilds). Consider a similar helper, e.g.controllerutils.ApplyOCIVolumesConfig(bc *tasks.BuildConfig, spec *automotivev1alpha1.OperatorConfigSpec), to avoid the three copies drifting.♻️ Proposed helper
func ApplyOCIVolumesConfig(bc *tasks.BuildConfig, spec *automotivev1alpha1.OperatorConfigSpec) { gates := featuregates.NewFromConfig(spec) if gates.Enabled(featuregates.OCIVolumes) { bc.UseOCIVolumes = true bc.OrasImage = spec.GetImages().GetOrasImage() } }Also applies to: 2598-2604
🤖 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 `@internal/controller/imagebuild/controller.go` around lines 905 - 911, The OCI-volumes feature-gate wiring is duplicated in multiple places, so factor the repeated feature-gate check and BuildConfig mutation out of the image build controller into a shared helper. Add a helper such as controllerutils.ApplyOCIVolumesConfig that takes the BuildConfig and OperatorConfigSpec, performs the featuregates.NewFromConfig check for featuregates.OCIVolumes, and sets UseOCIVolumes and OrasImage; then replace the repeated inline blocks in imagebuild controller code and the operatorconfig controller with calls to that helper.internal/common/tasks/scripts/build_image.sh (1)
95-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winORAS fallback-download logic has diverged in robustness from
push_artifact.sh.Both scripts implement essentially the same "download + verify checksum + install oras" logic, but
push_artifact.shwas hardened in this PR (curl failure checks, missing-checksum check, extraction failure check,$HOMEedge-case guard for//bin//bin, cleanup trap) while this block keeps the older, unchecked version:curl -sLOfailures are silent,tar -zxfisn't checked, andmkdir -p "$HOME/bin"has no guard against$HOMEbeing unset/root. Since this logic is now duplicated in two places, further changes to one (as already happened here) risk leaving the other behind. Consider extracting a singleinstall_oras()function intocommon.shthat both scripts call.♻️ Sketch of a shared helper in common.sh
+install_oras() { + ORAS_VERSION="1.2.0" + case "$(uname -m)" in + x86_64) ORAS_ARCH="amd64" ;; + aarch64|arm64) ORAS_ARCH="arm64" ;; + *) echo "ERROR: Unsupported architecture: $(uname -m)" >&2; return 1 ;; + esac + # ... same hardened download/verify/install logic currently in push_artifact.sh ... +}Then both
build_image.shandpush_artifact.shcallinstall_oraswhenorasisn't already resolvable.🤖 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 `@internal/common/tasks/scripts/build_image.sh` around lines 95 - 121, The ORAS download/install fallback in build_image.sh is still using the older, less safe flow compared with push_artifact.sh. Refactor the duplicated inline block into a shared install_oras helper in common.sh and have both scripts call it when command -v oras fails. Make sure the shared helper includes the same hardened checks as push_artifact.sh: fail on curl/download errors, verify the checksum file and tar extraction, and guard the $HOME/bin install path edge case.internal/common/tasks/scripts/push_artifact.sh (1)
4-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded mount path duplicates the Go-side constant.
/oci-tools/oras/bin/orasis hardcoded here, mirroringOCIToolsMountBase + "/oras"(/oci-tools/oras) frominternal/common/tasks/tasks.go, plus an assumed/bin/oraslayout inside the ORAS image. If that Go constant ever changes, this script silently falls through tocommand -v oras/ the download fallback instead of failing loudly — a correctness regression that would go unnoticed until someone inspects logs. A brief comment noting this must stay in sync withociMountPathOrasintasks.gowould help.🤖 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 `@internal/common/tasks/scripts/push_artifact.sh` around lines 4 - 11, The ORAS mount path check in push_artifact.sh hardcodes the OCI volume location and can drift from the Go-side constant used by tasks.go. Update the logic around ORAS_BIN to keep it explicitly in sync with ociMountPathOras/OCIToolsMountBase, and add a brief comment near the /oci-tools/oras/bin/oras check documenting that this path must match the Go constant. Make the mismatch obvious so changes to the mount path do not silently fall back to command -v oras or the download path.
🤖 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 `@internal/controller/imagebuild/controller.go`:
- Line 1466: The OCI volume and task mount sources are being read from different
config snapshots, which can let `taskRef`/`pipelineRef` and
`podTemplate.Volumes` drift apart. Update the ImageBuild reconciliation flow
around `podTemplate.Volumes = append(... tasks.OCIVolumes(buildConfig)...)` and
the `taskRef`/`pipelineRef` selection so both come from the same reconciled
`OperatorConfig` snapshot, or defer creating the `PipelineRun` until the
regenerated Task/Pipeline has been applied.
---
Nitpick comments:
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 95-121: The ORAS download/install fallback in build_image.sh is
still using the older, less safe flow compared with push_artifact.sh. Refactor
the duplicated inline block into a shared install_oras helper in common.sh and
have both scripts call it when command -v oras fails. Make sure the shared
helper includes the same hardened checks as push_artifact.sh: fail on
curl/download errors, verify the checksum file and tar extraction, and guard the
$HOME/bin install path edge case.
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 4-11: The ORAS mount path check in push_artifact.sh hardcodes the
OCI volume location and can drift from the Go-side constant used by tasks.go.
Update the logic around ORAS_BIN to keep it explicitly in sync with
ociMountPathOras/OCIToolsMountBase, and add a brief comment near the
/oci-tools/oras/bin/oras check documenting that this path must match the Go
constant. Make the mismatch obvious so changes to the mount path do not silently
fall back to command -v oras or the download path.
In `@internal/common/tasks/tasks.go`:
- Around line 906-946: Document the cluster-side prerequisite for the
ImageVolume dependency used by OCIVolumes(), since applyOCIVolumeMounts() and
OCIVolumes() rely on corev1.ImageVolumeSource. Add a note in the
install/docs/README explaining that enabling BuildConfig.UseOCIVolumes requires
the Kubernetes ImageVolume feature gate and a compatible runtime, so operators
know not to turn on the feature unless the OpenShift cluster supports it.
- Around line 911-924: `applyOCIVolumeMounts` currently adds the
`ociVolumeNameOras` mount to every step in a task, even for steps like
`find-manifest-file` and `build-image` that do not use `oras`. Update the logic
in `applyOCIVolumeMounts` to scope the mount only to the specific step names
that actually need it, using `task.Spec.Steps` and the step `Name` field to
identify them, so the mount is not appended to unrelated steps.
In `@internal/controller/imagebuild/controller.go`:
- Around line 905-911: The OCI-volumes feature-gate wiring is duplicated in
multiple places, so factor the repeated feature-gate check and BuildConfig
mutation out of the image build controller into a shared helper. Add a helper
such as controllerutils.ApplyOCIVolumesConfig that takes the BuildConfig and
OperatorConfigSpec, performs the featuregates.NewFromConfig check for
featuregates.OCIVolumes, and sets UseOCIVolumes and OrasImage; then replace the
repeated inline blocks in imagebuild controller code and the operatorconfig
controller with calls to that helper.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0561b2b-1adb-4377-b395-8645ba1b992d
⛔ Files ignored due to path filters (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (10)
api/v1alpha1/operatorconfig_types.gointernal/common/tasks/oci_volumes_test.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/common.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.gointernal/controller/operatorconfig/controller.gointernal/featuregates/features.gointernal/featuregates/gates_test.go
188f07f to
f8467e2
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@ambient-code please review |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/common/tasks/scripts/push_artifact.sh (1)
10-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the ORAS download/verify/install logic into a shared helper.
This ~80-line download+checksum-verify+install flow appears to duplicate similar logic already present in
build_image.shper the PR summary. Centralizing it incommon.sh(which already gains an "OCI tools path hook" in this PR) would avoid version-pin drift (ORAS_VERSION="1.2.0") between the two call sites and reduce maintenance surface.🤖 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 `@internal/common/tasks/scripts/push_artifact.sh` around lines 10 - 91, The ORAS download, checksum verification, and install flow in push_artifact.sh is duplicated and should be centralized. Extract the logic used in the ORAS_BIN bootstrap block into a shared helper in common.sh, then have push_artifact.sh call that helper instead of embedding the full flow. Keep the version-pinned setup in one place so build_image.sh and the push_artifact.sh path both use the same helper and cannot drift.
🤖 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.
Nitpick comments:
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 10-91: The ORAS download, checksum verification, and install flow
in push_artifact.sh is duplicated and should be centralized. Extract the logic
used in the ORAS_BIN bootstrap block into a shared helper in common.sh, then
have push_artifact.sh call that helper instead of embedding the full flow. Keep
the version-pinned setup in one place so build_image.sh and the push_artifact.sh
path both use the same helper and cannot drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e22a4b68-402d-45f4-af02-c5ac06824bd5
⛔ Files ignored due to path filters (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (12)
api/v1alpha1/operatorconfig_types.gointernal/common/tasks/oci_volumes_test.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/common.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/controllerutils/oci_volumes.gointernal/controller/controllerutils/oci_volumes_test.gointernal/controller/imagebuild/controller.gointernal/controller/operatorconfig/controller.gointernal/featuregates/features.gointernal/featuregates/gates_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/common/tasks/scripts/common.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/common/tasks/scripts/build_image.sh
- internal/featuregates/gates_test.go
- internal/featuregates/features.go
- api/v1alpha1/operatorconfig_types.go
- internal/common/tasks/oci_volumes_test.go
- internal/common/tasks/tasks.go
![ambient-code[bot]](https://avatars.githubusercontent.com/u/231559327?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall this is a well-structured feature addition that follows the established patterns (feature gates, controllerutils helpers, BuildConfig extension). The approach of using image volumes via podTemplate to avoid Tekton's Task CRD pruning is sound. Good test coverage across the feature gate, volume mounting, and controller integration paths.
A few items worth considering below — nothing blocking, mostly around edge-case robustness.
f8467e2 to
257485b
Compare
|
@ambient-code please review |
There was a problem hiding this comment.
Re-review after force-push
Both items from the previous review have been addressed:
- pushPodTemplate guard — now correctly only creates the
PodTemplatewhen OCI volumes are present, avoiding a behavioral change for non-OCIVolumes push TaskRuns. - Feature registration pattern —
OCIVolumesis registered directly in thedefaultFeaturesmap literal, consistent with the intended pattern documented in the comment.
The overall implementation is clean and well-tested. The ORAS install consolidation into common.sh eliminates duplication between build_image.sh and push_artifact.sh, and the install_oras() function correctly short-circuits when oras is already on PATH (via OCI volume mount).
One minor observation (not blocking): the trap _cleanup_oras_files EXIT inside install_oras() replaces any existing EXIT trap. Currently this is fine since no other EXIT traps exist at that point, but it could be fragile if other cleanup traps are added in the future. A stacking approach (trap '...; _cleanup_oras_files' EXIT) or using a trap wrapper would be more robust — but that's a general shell scripting concern, not specific to this PR.
LGTM — good feature addition with solid test coverage.
| if command -v oras >/dev/null 2>&1; then | ||
| ORAS_BIN="$(command -v oras)" | ||
| echo "ORAS already available at $ORAS_BIN" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
If the oras binary location change, then command -v oras fails silently, install_oras falls back to downloading, and the feature appears to work but the OCI volume goes unused with no warning emitted.
I think adding a log line is much helpful when the PATH directory doesn't exist
echo "WARN: OCI volume mounted but ORAS binary not found at expected path, falling back to download" >&2
| return 0 | ||
| fi | ||
|
|
||
| ORAS_VERSION="1.2.0" |
There was a problem hiding this comment.
oras version is hardcoded in the fallback. If the operator configure spec.images.oras: ghcr.io/oras-project/oras:v1.3.0 then OCI volume has v1.3.0 but the fallback download still installs v1.2.0.
There was a problem hiding this comment.
we can introduce a variable to the task, but the plan is that over time (not too long hopefully) only ImageVolume will be used and the fallback will be removed entirely
257485b to
a3ed10a
Compare
Gate OCI image volumes behind a new Alpha feature gate (OCIVolumes). When enabled, mount the ORAS CLI container image as a read-only volume in build pods, eliminating the runtime download+verify of the ORAS binary. Enable via OperatorConfig: spec.featureGates.OCIVolumes: true Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
a3ed10a to
15aa241
Compare
3 participants
Gate OCI image volumes behind a new Alpha feature gate (OCIVolumes). When enabled, mount the ORAS CLI container image as a read-only volume in build pods, eliminating the runtime download+verify of the ORAS binary.
Enable via OperatorConfig:
spec.featureGates.OCIVolumes: true
Summary
Related Issues
Type of Change
Testing
make test)make lint)make manifests generate)Summary by CodeRabbit