cmd-import: pull disk images from OCI#4591
Conversation
|
Skipping CI for Draft Pull Request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support to cosa import for automatically discovering and downloading disk image artifacts from an OCI image index, introducing a new --download command-line option. The review feedback focuses on improving the robustness of the implementation, specifically by ensuring that the oras CLI tool is validated before use, handling cases where requested downloads are unavailable, and adding defensive checks to prevent crashes when parsing potentially malformed JSON payloads or missing manifest keys.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/retest |
|
/gemini-review |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
In Konflux we are building the disk images using image-builder and
storing them as intermediary artifacts in OCI.
Abusing the OCI manifest spec a little bit we describe the disk images
types under `platform.os` with the artifact name.
This allows to create a big manifest list referencing the regular OCI
container images from the base build alongside all the disk images.
Here is an example:
```json
{
"schemaVersion": 2,
"mediaType": "application/vnd.oci.image.index.v1+json",
"manifests": [
{
"mediaType": "application/vnd.oci.image.manifest.v1+json",
"digest": "sha256:1e247...",
"size": 771,
"platform": {
"architecture": "amd64",
"os": "qemu"
},
"artifactType": "application/vnd.diskimage.qcow2"
},
...
]
}
```
With this change, `cosa import` knows how to find a disk image from such
an index and download them the `--download` flag.
e.g.
`cosa import docker://quay.io/konflux-fedora/coreos-tenant/disk-images-rawhide:on-pr-2d54f6ac04172313b4a3eec102d55bbd9def3097 --download qemu`
will result in a build dir populated with the OCI artifact and the
qemu artifact:
```
cosa list
45.20260701.91.1
Timestamp: 2026-07-01T06:09:39Z (8:15:24 ago)
Artifacts: ostree oci-manifest qemu
Config: c632a672e1b70cc3de53843db2bee58a4edaa367
```
Downloaded blobs are integrity-checked against the layer digest from
the manifest.
The --arch flag allows downloading disk images for a different
architecture than the host (defaults to host basearch).
Non-registry sources (oci-archive:, containers-storage:) are unaffected
and continue to use the standard skopeo-based flow.
Assisted-by: Opencode.ai <Opus 4.6>
fixup
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/cmd-import`:
- Around line 236-258: Validate registry-controlled artifact fields before they
are used to build filenames or populate build_meta['images']: in the cmd-import
flow where platform = entry.get('platform', {}), oci_goarch_to_basearch(), and
artifact_type_to_extension() are used, reject empty or unsafe platform.os,
platform.architecture, and artifactType-derived suffixes before appending to
artifacts or downloading. Add explicit checks in the manifest-processing path to
fail fast on unsupported platform names and only allow values that are valid for
the build metadata schema, so downstream path/key construction never sees
untrusted components.
- Around line 68-75: The `--download` path in `cmd-import` compares `args.arch`
directly against `disk_image_artifacts` entries, but those entries are
normalized with `oci_goarch_to_basearch()`, so mismatched aliases like `arm64`
vs `aarch64` can fail to match. Update the `download_arch` selection and the
artifact filtering in the `--download` branch to apply the same architecture
normalization used for OCI index entries, likely by normalizing `args.arch`
before comparing in the list comprehension and error message.
- Around line 60-71: The current try_fetch_oci_index()/--download flow is
masking real registry fetch failures as “not an OCI image index.” Update the
src/cmd-import logic around try_fetch_oci_index, registry_base_from_ref, and the
--download check so only known non-registry transports are treated as a missing
index, while actual oras manifest fetch/auth/transport errors are allowed to
surface when args.download is set. Keep the early
disk_image_artifacts/index_manifest handling, but distinguish between “not
supported transport” and genuine fetch failure before printing the OCI index
error.
- Around line 201-207: The ref normalization in registry_base_from_ref() still
leaves tags on refs like registry/repo:tag@sha256:..., so update the stripping
logic to remove the digest first and then always strip any tag from the final
path segment. Keep the fix localized to registry_base_from_ref() and ensure the
returned base is tag-free before later ORAS fetches use it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 680e01e9-32b8-4731-99f0-fab7de0db016
📒 Files selected for processing (2)
src/cmd-importsrc/deps.txt
| index_manifest = try_fetch_oci_index(args.srcimg) | ||
| disk_image_artifacts = [] | ||
| registry_base = None | ||
| if index_manifest is not None: | ||
| registry_base = registry_base_from_ref(args.srcimg) | ||
| disk_image_artifacts = discover_disk_image_artifacts(index_manifest) | ||
|
|
||
| # fail early if --download was requested but we can't fulfill it | ||
| download_arch = args.arch if args.arch else arch | ||
| if args.download: | ||
| if index_manifest is None: | ||
| print("ERROR: --download was specified, but the source image is not an OCI image index") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file structure first.
git ls-files src | sed -n '1,120p'
# Find the relevant symbols and callers.
rg -n "try_fetch_oci_index|registry_from_arg|oci-archive:|containers-storage:|--download|download_arch" src
# Read the file around the reported lines, with line numbers.
wc -l src/cmd-import
sed -n '1,260p' src/cmd-importRepository: coreos/coreos-assembler
Length of output: 17664
Don't mask oras manifest fetch failures as “not an OCI image index” src/cmd-import:60-71
try_fetch_oci_index() returns None for any fetch error, so auth/transport failures are reported as the wrong --download error. Skip the known non-registry transports explicitly, but let registry fetch failures surface when --download is set.
🤖 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 `@src/cmd-import` around lines 60 - 71, The current
try_fetch_oci_index()/--download flow is masking real registry fetch failures as
“not an OCI image index.” Update the src/cmd-import logic around
try_fetch_oci_index, registry_base_from_ref, and the --download check so only
known non-registry transports are treated as a missing index, while actual oras
manifest fetch/auth/transport errors are allowed to surface when args.download
is set. Keep the early disk_image_artifacts/index_manifest handling, but
distinguish between “not supported transport” and genuine fetch failure before
printing the OCI index error.
| download_arch = args.arch if args.arch else arch | ||
| if args.download: | ||
| if index_manifest is None: | ||
| print("ERROR: --download was specified, but the source image is not an OCI image index") | ||
| sys.exit(1) | ||
| arch_artifacts = [a for a in disk_image_artifacts if a['arch'] == download_arch] | ||
| if not arch_artifacts: | ||
| print(f"ERROR: --download was specified, but no disk image artifacts were found for {download_arch}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize --arch with the same mapping used for index entries.
Line 68 compares args.arch verbatim against artifacts normalized through oci_goarch_to_basearch(), so --arch arm64 will not match an OCI arm64 descriptor stored as aarch64.
Proposed fix
- download_arch = args.arch if args.arch else arch
+ download_arch = oci_goarch_to_basearch(args.arch) if args.arch else arch📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| download_arch = args.arch if args.arch else arch | |
| if args.download: | |
| if index_manifest is None: | |
| print("ERROR: --download was specified, but the source image is not an OCI image index") | |
| sys.exit(1) | |
| arch_artifacts = [a for a in disk_image_artifacts if a['arch'] == download_arch] | |
| if not arch_artifacts: | |
| print(f"ERROR: --download was specified, but no disk image artifacts were found for {download_arch}") | |
| download_arch = oci_goarch_to_basearch(args.arch) if args.arch else arch | |
| if args.download: | |
| if index_manifest is None: | |
| print("ERROR: --download was specified, but the source image is not an OCI image index") | |
| sys.exit(1) | |
| arch_artifacts = [a for a in disk_image_artifacts if a['arch'] == download_arch] | |
| if not arch_artifacts: | |
| print(f"ERROR: --download was specified, but no disk image artifacts were found for {download_arch}") |
🤖 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 `@src/cmd-import` around lines 68 - 75, The `--download` path in `cmd-import`
compares `args.arch` directly against `disk_image_artifacts` entries, but those
entries are normalized with `oci_goarch_to_basearch()`, so mismatched aliases
like `arm64` vs `aarch64` can fail to match. Update the `download_arch`
selection and the artifact filtering in the `--download` branch to apply the
same architecture normalization used for OCI index entries, likely by
normalizing `args.arch` before comparing in the list comprehension and error
message.
| # strip digest | ||
| if '@' in ref: | ||
| ref = ref.split('@')[0] | ||
| # strip tag | ||
| elif ':' in ref.split('/')[-1]: | ||
| ref = ref.rsplit(':', 1)[0] | ||
| return ref |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the surrounding implementation.
git ls-files | rg '^src/cmd-import$'
wc -l src/cmd-import
sed -n '160,240p' src/cmd-import
# Find call sites that use the parsed ref value in ORAS-related operations.
rg -n "strip digest|strip tag|oras|artifact-digest|@.*sha256|rsplit\\(':', 1\\)|split\\('@'" src/cmd-import src || trueRepository: coreos/coreos-assembler
Length of output: 4179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the functions that use registry_base_from_ref and how refs are reconstructed.
sed -n '240,330p' src/cmd-import
sed -n '330,420p' src/cmd-import
# Show all references to registry_base_from_ref in the file.
rg -n "registry_base_from_ref\\(" src/cmd-importRepository: coreos/coreos-assembler
Length of output: 7263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect argument parsing and the main import flow around registry_base_from_ref.
sed -n '1,120p' src/cmd-import
sed -n '120,180p' src/cmd-import
# Check whether the code constructs or expects refs containing both tag and digest.
rg -n ":[^/@]+@sha256:|`@sha256`:|registry_base_from_ref|srcimg" src/cmd-importRepository: coreos/coreos-assembler
Length of output: 8791
Strip the tag after removing the digest. registry_base_from_ref() leaves registry/repo:tag for refs like registry/repo:tag@sha256:..., so the later ORAS fetches use a tagged base. Strip the digest first, then always remove any tag from the last path segment.
🤖 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 `@src/cmd-import` around lines 201 - 207, The ref normalization in
registry_base_from_ref() still leaves tags on refs like
registry/repo:tag@sha256:..., so update the stripping logic to remove the digest
first and then always strip any tag from the final path segment. Keep the fix
localized to registry_base_from_ref() and ensure the returned base is tag-free
before later ORAS fetches use it.
| platform = entry.get('platform', {}) | ||
| goarch = platform.get('architecture', '') | ||
| arch = oci_goarch_to_basearch(goarch) | ||
|
|
||
| platform_name = platform.get('os', '') | ||
| digest = entry.get('digest') | ||
| if not platform_name: | ||
| raise ValueError( | ||
| f"Malformed manifest entry: missing platform.os " | ||
| f"(digest: {digest or 'unknown'})") | ||
|
|
||
| if not digest: | ||
| raise ValueError( | ||
| f"Malformed manifest entry for {platform_name}: missing digest") | ||
|
|
||
| extension = artifact_type_to_extension(artifact_type) | ||
|
|
||
| artifacts.append({ | ||
| 'platform': platform_name, | ||
| 'arch': arch, | ||
| 'extension': extension, | ||
| 'manifest_digest': digest, | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate registry-controlled artifact names before using them as paths and meta keys.
platform.os, platform.architecture, and the artifactType suffix flow into filenames and build_meta['images'][platform]. Reject empty or unsafe components before download; preferably also reject platform names not supported by the build metadata schema.
Proposed fix sketch
+SAFE_ARTIFACT_COMPONENT_RE = re.compile(r'^[A-Za-z0-9][A-Za-z0-9_.+-]*$')
+
+
+def validate_artifact_component(field, value):
+ if not value or not SAFE_ARTIFACT_COMPONENT_RE.fullmatch(value):
+ raise ValueError(f"Unsafe disk image {field}: {value!r}")
+
+
def discover_disk_image_artifacts(index_manifest):
@@
platform = entry.get('platform', {})
goarch = platform.get('architecture', '')
+ validate_artifact_component('architecture', goarch)
arch = oci_goarch_to_basearch(goarch)
@@
platform_name = platform.get('os', '')
+ validate_artifact_component('platform', platform_name)
@@
extension = artifact_type_to_extension(artifact_type)
+ validate_artifact_component('extension', extension)Also applies to: 112-118, 463-466
🤖 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 `@src/cmd-import` around lines 236 - 258, Validate registry-controlled artifact
fields before they are used to build filenames or populate build_meta['images']:
in the cmd-import flow where platform = entry.get('platform', {}),
oci_goarch_to_basearch(), and artifact_type_to_extension() are used, reject
empty or unsafe platform.os, platform.architecture, and artifactType-derived
suffixes before appending to artifacts or downloading. Add explicit checks in
the manifest-processing path to fail fast on unsupported platform names and only
allow values that are valid for the build metadata schema, so downstream
path/key construction never sees untrusted components.
Support using coreos/coreos-assembler#4591 to download the pre-built QEMU image from the konflux builds instead of building it manually.
joelcapitao
left a comment
joelcapitao
left a comment
There was a problem hiding this comment.
LGTM
I left an in-line comment that can be addressed in a follow-up patch
There is also a fixup typo in the commit message but I would not trigger pipelines again only for its removal (just in case there is another edit before the merge)
| stress-ng | ||
|
|
||
| # To create/download OCI artifacts | ||
| golang-oras |
There was a problem hiding this comment.
Note that the RPM is unmaintained c.f. https://src.fedoraproject.org/rpms/golang-oras and is shipping an old release i.e. 1.2.2 Dec 19 2024 https://github.com/oras-project/oras/releases/tag/v1.2.2
Pulling it from their container image could be a solution:
FROM ghcr.io/oras-project/oras:v1.3.2 as oras-bin
FROM quay.io/fedora/fedora:43
COPY --from=oras-bin /bin/oras /usr/local/bin/oras
2 participants
In Konflux we are building the disk images using image-builder and storing them as intermediary artifacts in OCI.
Abusing the OCI manifest spec a little bit we describe the disk images types under
platform.oswith the artifact name.This allows to create a big manifest list referencing the regular OCI container images from the base build alongside all the disk images. Here is an example:
{ "schemaVersion": 2, "mediaType": "application/vnd.oci.image.index.v1+json", "manifests": [ { "mediaType": "application/vnd.oci.image.manifest.v1+json", "digest": "sha256:1e24720d2ba7aba2a1cb8d82eb40e4c3fdde109c40d9a0eb118889ce5bedf6e3", "size": 771, "platform": { "architecture": "amd64", "os": "qemu" }, "artifactType": "application/vnd.diskimage.qcow2" }, { "mediaType": "application/vnd.oci.image.manifest.v1+json", "digest": "sha256:c7e32f00573f696796dec43df101a00db23e0b4702271fa6c7b8f00dbacca6ea", "size": 773, "platform": { "architecture": "amd64", "os": "metal" }, "artifactType": "application/vnd.diskimage.raw.gzip" }, { "mediaType": "application/vnd.oci.image.manifest.v1+json", "digest": "sha256:2f568edad2c832b24a67e362c4056eec49a658228f3f00ac597c9ce206c80261", "size": 11457, "platform": { "architecture": "amd64", "os": "linux" } } ] }With this change,
cosa importknows how to pull a disk image from such a manifest with the--downloadflag, passing the artifact name. e.g.cosa import docker://quay.io/konflux-fedora/coreos-tenant/disk-images-rawhide:on-pr-2d54f6ac04172313b4a3eec102d55bbd9def3097 --download qemuwill result in a build dir populated with the OCI artifact and the qemu artifact:Assisted-by: Opencode.ai <Opus 4.6>
Summary by CodeRabbit
New Features
cosa importcan now import disk image artifacts from OCI image indexes.Bug Fixes
Chores