add Tekton Chains type hints and osbuild manifest attestation#197
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 20 minutes and 23 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdds SecureBuild support with digest-pinned Tekton task-bundle resolution, computes and propagates artifact integrity and container digests, exports Tekton Tasks as a bundle in CI, wires IMAGE_* and ARTIFACT_INTEGRITY_DIGEST task results into Tekton/Chains outputs, and threads TaskBundleRef through CLI, API, controller, and pipeline generation. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CAIB CLI
participant API as Build API Server
participant Config as OperatorConfig
participant Controller as ImageBuild Controller
participant Tekton
User->>CLI: caib build --secure
CLI->>API: CreateBuild(SecureBuild=true)
API->>Config: Read OperatorConfig
Config-->>API: OSBuilds.TaskBundleRef
API->>API: validate digest-pinned TaskBundleRef
API->>Controller: submit ImageBuild (SecureBuild=true, TaskBundleRef)
Controller->>Config: Read OperatorConfig.TaskBundleRef
Controller->>Controller: set TaskResolver=bundle, embed PipelineSpec
Controller->>Tekton: create PipelineRun (inline PipelineSpec)
Tekton->>Tekton: run build-image (compute ARTIFACT_INTEGRITY_DIGEST, push container, record IMAGE_DIGEST/IMAGE_URL)
Tekton->>Tekton: run push-disk-artifact (verify expected digest, push disk, attach referrer)
Tekton->>Tekton: finally collect-images-result -> IMAGES (reads .chains container/disk url+digest)
Tekton-->>Controller: PipelineRun results (IMAGE_URL / IMAGE_DIGEST / ARTIFACT_INTEGRITY_DIGEST / IMAGES)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 424-454: The manifest is read with open(sys.argv[1]).read() which
doesn't use a context manager—change the manifest loading to use a with
open(...) as f: block and call json.load(f) to ensure the input file is properly
closed; keep the existing write using with open(sys.argv[2], "w") as f:
json.dump(...). Update the code around manifest =
json.loads(open(sys.argv[1]).read()) to use a context manager and leave the
redact_sensitive and redact_source_urls functions unchanged.
🪄 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: cd147fef-f894-4896-917f-3d7d0f973303
📒 Files selected for processing (3)
internal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.go
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)
internal/common/tasks/scripts/push_artifact.sh (1)
330-335:⚠️ Potential issue | 🔴 CriticalFix the push pipeline to preserve oras exit status.
The script avoids
/tmpforannotations_file(line 228) because ORAS containers may not have it available, but the newtee /tmp/oras-push-output.txtpipelines (lines 330–335, 391–396) reintroduce that dependency. Withoutpipefailenabled incommon.sh(which currently has onlyset -e), a failedoras pushwill exit with code 0 fromtee, allowing the script to continue and potentially report a stale digest from the fallback fetch.Add
set -o pipefailtocommon.sh, or capture the push output locally in the current directory (like annotations_file) and preserve the oras exit status explicitly:oras_output="./oras-push-output.txt" "$HOME/bin/oras" push ... 2>&1 | tee "$oras_output" oras_status=$? if [ $oras_status -ne 0 ]; then exit $oras_status fi DISK_DIGEST=$(sed -n 's/.*Digest: \(sha256:[a-f0-9]*\).*/\1/p' "$oras_output" 2>/dev/null | head -1)Also applies to: 391–396, 405–410.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/push_artifact.sh` around lines 330 - 335, The oras push pipeline currently pipes to tee writing to /tmp and loses the real exit status; update the push logic that invokes "$HOME/bin/oras" push (the block using ${artifact_type}, "$annotations_file", ${layer_args} and tee /tmp/oras-push-output.txt) to preserve failure codes by either enabling pipefail in the shared shell setup (add set -o pipefail to common.sh) or capturing output into a local file (e.g. oras_output="./oras-push-output.txt"), run "$HOME/bin/oras" push ... 2>&1 | tee "$oras_output", then immediately check the push exit status (oras_status=$?; if non-zero exit with that code) before parsing DISK_DIGEST from the output; apply the same change to the other tee usages referenced (the blocks around the other occurrences where tee writes to /tmp at the later push points).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 330-335: The oras push pipeline currently pipes to tee writing to
/tmp and loses the real exit status; update the push logic that invokes
"$HOME/bin/oras" push (the block using ${artifact_type}, "$annotations_file",
${layer_args} and tee /tmp/oras-push-output.txt) to preserve failure codes by
either enabling pipefail in the shared shell setup (add set -o pipefail to
common.sh) or capturing output into a local file (e.g.
oras_output="./oras-push-output.txt"), run "$HOME/bin/oras" push ... 2>&1 | tee
"$oras_output", then immediately check the push exit status (oras_status=$?; if
non-zero exit with that code) before parsing DISK_DIGEST from the output; apply
the same change to the other tee usages referenced (the blocks around the other
occurrences where tee writes to /tmp at the later push points).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dddd5af2-c1dd-4327-93f4-86272b4df526
📒 Files selected for processing (1)
internal/common/tasks/scripts/push_artifact.sh
6 tasks
bennyz
force-pushed
the
secure-build
branch
2 times, most recently
from
8b90176 to
62332a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.github/workflows/backport.yml (1)
8-10: Document thebackport/*label convention alongside release branch docs.The workflow relies on
backport/labels, but current release docs focus onrelease-X.Y.xbranches without label mapping guidance. Adding a short section indocs/releasing.mdwould make usage clearer for maintainers.Also applies to: 17-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/backport.yml around lines 8 - 10, Add a short documented mapping in docs/releasing.md that explains the backport/* label convention used by the .github/workflows/backport.yml workflow: describe that labels prefixed with backport/ (e.g., backport/release-1.2.x) trigger backports to corresponding release branches, show one or two concrete examples and the exact label format, and place it near the existing release branch guidance so maintainers can find how labels map to release-X.Y.x branches.api/v1alpha1/operatorconfig_types.go (1)
539-545: Consider enforcing digest-pinned bundle refs for secure builds.Allowing mutable tags in
TaskBundleRefweakens immutability guarantees. Prefer validating...@sha256:<digest>for secure-mode bundle references.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1alpha1/operatorconfig_types.go` around lines 539 - 545, TaskBundleRef allows mutable tags; enforce digest-pinned refs by adding validation that rejects refs not in the form "<name>@sha256:<64-hex>" when secure mode is requested. Implement this check inside the OperatorConfig validation path (e.g., the OperatorConfig.ValidateCreate / ValidateUpdate methods or the webhook handler that validates OperatorConfig) by matching TaskBundleRef against a regex like ^[^@]+@sha256:[0-9a-fA-F]{64}$ and return a clear field error if it fails; update any OpenAPI schema or unit tests for OperatorConfig validation accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/backport.yml:
- Around line 23-35: Replace the mutable action tags with immutable full
40-character commit SHAs: find the three uses entries referencing
actions/create-github-app-token@v2, actions/checkout@v4, and
korthout/backport-action@v4 and replace each @<tag> with the corresponding
repository commit SHA (e.g. actions/create-github-app-token@<40-char-sha>,
actions/checkout@<40-char-sha>, korthout/backport-action@<40-char-sha>); verify
the SHAs by checking the respective upstream repos/tags and update any dependent
references (e.g., steps.app-token.outputs.token) remain unchanged and the
workflow still runs with the pinned SHAs.
In @.github/workflows/build.yml:
- Around line 168-172: The current workflow builds BUNDLE_FILES as a
space-separated string and expands it unquoted in the tkn bundle push commands
which will break on paths with whitespace; replace that with an array (e.g.,
bundle_args) populated with each "-f <file>" element (iterate over
_output/tasks/*.yaml and append "-f" and the file to bundle_args) and then call
tkn bundle push "${{ env.TEKTON_BUNDLE_IMAGE }}:${{ env.VERSION }}"
"${bundle_args[@]}" and similarly for the :latest tag so the file arguments are
passed safely quoted; update both invocations that currently use BUNDLE_FILES to
use the new bundle_args array.
In `@internal/controller/imagebuild/controller.go`:
- Around line 1949-1954: The upload pod is only getting NodeSelector/Tolerations
copied from operatorConfig.Spec.OSBuilds (pod.Spec.NodeSelector /
pod.Spec.Tolerations) which allows it to be scheduled on a different
architecture than the build pod; modify the code that creates the upload pod to
also copy the same required architecture affinity used by createBuildTaskRun()
(the kubernetes.io/arch=<spec.architecture> affinity) so the upload pod has the
same nodeAffinity/requiredDuringSchedulingIgnoredDuringExecution constraint as
the build pod, ensuring both pods only schedule onto the same architecture;
locate the upload-pod creation path and add the same affinity construction or
copy from the build affinity logic so the labels/selector exactly match the
build pod’s arch requirement.
- Around line 546-549: When imageBuild.Spec.SecureBuild is true but
operatorConfig.Spec.OSBuilds.TaskBundleRef is empty, you must fail closed rather
than leaving buildConfig.TaskResolver unset; update the logic around
imageBuild.Spec.SecureBuild to check operatorConfig.Spec.OSBuilds.TaskBundleRef
and if empty return an error (or set the ImageBuild status condition) so
PipelineRun creation is aborted, otherwise set buildConfig.TaskResolver =
"bundle" and buildConfig.TaskBundleRef =
operatorConfig.Spec.OSBuilds.TaskBundleRef; reference
imageBuild.Spec.SecureBuild, operatorConfig.Spec.OSBuilds.TaskBundleRef,
buildConfig.TaskResolver, buildConfig.TaskBundleRef and the PipelineRun creation
path when making the change.
---
Nitpick comments:
In @.github/workflows/backport.yml:
- Around line 8-10: Add a short documented mapping in docs/releasing.md that
explains the backport/* label convention used by the
.github/workflows/backport.yml workflow: describe that labels prefixed with
backport/ (e.g., backport/release-1.2.x) trigger backports to corresponding
release branches, show one or two concrete examples and the exact label format,
and place it near the existing release branch guidance so maintainers can find
how labels map to release-X.Y.x branches.
In `@api/v1alpha1/operatorconfig_types.go`:
- Around line 539-545: TaskBundleRef allows mutable tags; enforce digest-pinned
refs by adding validation that rejects refs not in the form
"<name>@sha256:<64-hex>" when secure mode is requested. Implement this check
inside the OperatorConfig validation path (e.g., the
OperatorConfig.ValidateCreate / ValidateUpdate methods or the webhook handler
that validates OperatorConfig) by matching TaskBundleRef against a regex like
^[^@]+@sha256:[0-9a-fA-F]{64}$ and return a clear field error if it fails;
update any OpenAPI schema or unit tests for OperatorConfig validation
accordingly.
🪄 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: 1236361b-7115-4c0b-8a9e-29a17ce785c2
⛔ Files ignored due to path filters (2)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (20)
.github/workflows/backport.yml.github/workflows/build.yml.gitignoreMakefileapi/v1alpha1/imagebuild_types.goapi/v1alpha1/operatorconfig_types.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/image/image.gocmd/caib/main.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_test.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.gointernal/controller/operatorconfig/controller.go
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- internal/buildapi/types.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/common/tasks/scripts/build_image.sh
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
.github/workflows/build.yml (1)
168-172:⚠️ Potential issue | 🟡 MinorUse an argument array for
tkn bundle pushfile flags.Line 171 and Line 172 expand
$BUNDLE_FILESunquoted, which is fragile (SC2086). Build-farguments in an array and pass"${bundle_args[@]}".💡 Proposed fix
- BUNDLE_FILES=$(printf -- '-f %s ' _output/tasks/*.yaml) - tkn bundle push "${{ env.TEKTON_BUNDLE_IMAGE }}:${{ env.VERSION }}" $BUNDLE_FILES - tkn bundle push "${{ env.TEKTON_BUNDLE_IMAGE }}:latest" $BUNDLE_FILES + shopt -s nullglob + bundle_args=() + for f in _output/tasks/*.yaml; do + bundle_args+=(-f "$f") + done + if [ ${`#bundle_args`[@]} -eq 0 ]; then + echo "No exported task YAMLs found under _output/tasks" >&2 + exit 1 + fi + tkn bundle push "${{ env.TEKTON_BUNDLE_IMAGE }}:${{ env.VERSION }}" "${bundle_args[@]}" + tkn bundle push "${{ env.TEKTON_BUNDLE_IMAGE }}:latest" "${bundle_args[@]}"#!/bin/bash set -euo pipefail # Verify this step no longer uses unquoted $BUNDLE_FILES expansion. rg -n 'BUNDLE_FILES|bundle_args|tkn bundle push' .github/workflows/build.yml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 168 - 172, Replace the fragile unquoted expansion of BUNDLE_FILES used in the tkn bundle push steps by building an array of file-flag arguments (e.g. bundle_args) where each entry is "-f" and the corresponding path, then call tkn bundle push with the array expansion quoted as "${bundle_args[@]}"; update the steps that reference BUNDLE_FILES (the variable and the two tkn bundle push invocations) to populate bundle_args from _output/tasks/*.yaml and pass the array to tkn bundle push to avoid word-splitting issues (refer to BUNDLE_FILES, bundle_args, and the tkn bundle push invocations to locate the changes)..github/workflows/backport.yml (1)
23-23:⚠️ Potential issue | 🟠 MajorPin privileged workflow actions to immutable commit SHAs.
Line 23, Line 29, and Line 35 use mutable tags (
@v2/@v4). In apull_request_targetworkflow, this weakens supply-chain guarantees.🔒 Proposed hardening
- - uses: actions/create-github-app-token@v2 + - uses: actions/create-github-app-token@<40-char-commit-sha> # v2 - - uses: actions/checkout@v4 + - uses: actions/checkout@<40-char-commit-sha> # v4 - - name: Create backport PRs - uses: korthout/backport-action@v4 + - name: Create backport PRs + uses: korthout/backport-action@<40-char-commit-sha> # v4#!/bin/bash set -euo pipefail # Verify action references in this workflow are pinned to full SHAs. rg -nP '^\s*-\s*uses:\s*[^@]+@(?![0-9a-f]{40}\b)\S+' .github/workflows/backport.ymlAlso applies to: 29-29, 35-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/backport.yml at line 23, The workflow uses mutable tags for actions (e.g., actions/create-github-app-token@v2, peter-evans/create-pull-request@v4, actions/checkout@v4); replace each tag reference with the corresponding full 40-character commit SHA from the action's repository to pin the exact revision, updating the three "uses:" lines accordingly (find the latest recommended SHA for each action in its GitHub repo and substitute it for `@vX`), and re-run the verification regex provided in the review to ensure no action reference remains unpinned.internal/controller/imagebuild/controller.go (2)
1946-1954:⚠️ Potential issue | 🟠 MajorApply architecture affinity to upload pod, not only selector/tolerations.
Line 1949-1954 copies
nodeSelector/tolerations, but build pods also enforce architecture affinity at Line 970-984. The upload pod should carry the same arch constraint to avoid first-consumer PVC placement on incompatible nodes.🧭 Proposed fix
if operatorConfig.Spec.OSBuilds != nil && len(operatorConfig.Spec.OSBuilds.Tolerations) > 0 { pod.Spec.Tolerations = operatorConfig.Spec.OSBuilds.Tolerations } + pod.Spec.Affinity = &corev1.Affinity{ + NodeAffinity: &corev1.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{ + { + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: corev1.LabelArchStable, + Operator: corev1.NodeSelectorOpIn, + Values: []string{imageBuild.Spec.Architecture}, + }, + }, + }, + }, + }, + }, + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 1946 - 1954, The upload pod currently copies only NodeSelector and Tolerations from operatorConfig.Spec.OSBuilds; it must also carry the same architecture affinity used by the build pods. Locate the architecture affinity logic applied for build pods (the block that sets pod.Spec.Affinity based on operatorConfig.Spec.OSBuilds / the kubernetes.io/arch constraint) and apply the same affinity to the upload pod (set pod.Spec.Affinity accordingly) when operatorConfig.Spec.OSBuilds is present so the WaitForFirstConsumer PVC lands on nodes with the correct architecture.
546-549:⚠️ Potential issue | 🟠 MajorFail closed when secure build is requested without a bundle reference.
At Line 546-549,
SecureBuild=truewith emptyTaskBundleRefleaves resolver unset, and Line 1017-1020 then falls back to cluster tasks instead of rejecting the request.🛡️ Proposed fix
- if imageBuild.Spec.SecureBuild && operatorConfig.Spec.OSBuilds.TaskBundleRef != "" { - buildConfig.TaskResolver = "bundle" - buildConfig.TaskBundleRef = operatorConfig.Spec.OSBuilds.TaskBundleRef - } + if imageBuild.Spec.SecureBuild { + if operatorConfig.Spec.OSBuilds.TaskBundleRef == "" { + return fmt.Errorf("secureBuild is enabled but OperatorConfig.spec.osBuilds.taskBundleRef is not set") + } + buildConfig.TaskResolver = "bundle" + buildConfig.TaskBundleRef = operatorConfig.Spec.OSBuilds.TaskBundleRef + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 546 - 549, When imageBuild.Spec.SecureBuild is true but operatorConfig.Spec.OSBuilds.TaskBundleRef is empty, the code currently leaves buildConfig.TaskResolver unset and later falls back to cluster tasks; change the check around where buildConfig.TaskResolver and buildConfig.TaskBundleRef are set (the block referencing imageBuild, operatorConfig and buildConfig) to fail closed: if SecureBuild==true and TaskBundleRef=="" return an error (e.g., fmt.Errorf("secure build requested but no TaskBundleRef configured")) or set the request/Build status to rejected instead of leaving resolver empty, preventing the fallback to cluster tasks; ensure the early return/error happens before further processing that would use buildConfig (so callers of this code can surface the rejection).
🧹 Nitpick comments (1)
internal/common/tasks/pipeline_test.go (1)
132-142: Cover the digest params in this finally-task assertion too.This block verifies
container-urlanddisk-url, but the new Chains contract also depends oncontainer-digestanddisk-digestreachingcollect-images-result. Adding those assertions here would catch regressions in the provenance wiring instead of only testing the URL half of it.🧪 Suggested test additions
if paramNames["container-url"] != "$(tasks.build-image.results.IMAGE_URL)" { t.Errorf("container-url param = %q, want build-image result ref", paramNames["container-url"]) } + if paramNames["container-digest"] != "$(tasks.build-image.results.IMAGE_DIGEST)" { + t.Errorf("container-digest param = %q, want build-image result ref", paramNames["container-digest"]) + } if paramNames["disk-url"] != "$(tasks.push-disk-artifact.results.IMAGE_URL)" { t.Errorf("disk-url param = %q, want push-disk-artifact result ref", paramNames["disk-url"]) } + if paramNames["disk-digest"] != "$(tasks.push-disk-artifact.results.IMAGE_DIGEST)" { + t.Errorf("disk-digest param = %q, want push-disk-artifact result ref", paramNames["disk-digest"]) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/pipeline_test.go` around lines 132 - 142, The test currently checks only container-url and disk-url in the finally-task params; add equivalent assertions for the digest params so the provenance wiring is covered: after building paramNames from task.Params verify paramNames["container-digest"] == "$(tasks.build-image.results.IMAGE_DIGEST)" and paramNames["disk-digest"] == "$(tasks.push-disk-artifact.results.IMAGE_DIGEST)" and produce t.Errorf messages similar to the existing ones (e.g., "container-digest param = %q, want build-image result ref") to fail the test if the digest refs are wrong; update the block that builds paramNames from task.Params to include these two new checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/buildapi/server.go`:
- Line 2068: When SecureBuild is requested (req.SecureBuild) ensure the server
fails closed if no task bundle is configured: add a validation near where
SecureBuild is read (the code setting SecureBuild: req.SecureBuild) to check
OperatorConfig.Spec.OSBuilds.TaskBundleRef (or the equivalent field on
OperatorConfig) and return an error (or gRPC status invalid/failed precondition)
when req.SecureBuild is true but TaskBundleRef is nil/empty; update the request
handling path that constructs the build options so SecureBuild cannot be
propagated unless TaskBundleRef is present.
In `@internal/buildapi/types.go`:
- Around line 143-145: BuildRequest.SecureBuild is not propagated into the build
template, so retries from getBuildTemplate in internal/buildapi/server.go can
lose secure mode; update getBuildTemplate (and any code paths that create or
restore templates from BuildRequest) to copy request.SecureBuild into the
template representation and ensure any function that rehydrates a BuildRequest
from a template restores SecureBuild as well (look for getBuildTemplate and the
template->request conversion logic and set the SecureBuild field accordingly).
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 197-220: The integrity check computes ACTUAL_DIGEST before the
target-specific normalization (the boot_a → boot_b duplication for targets
matching ride4*|ridesx4*), so parts_dir may be validated but the pushed artifact
gets an extra layer and no longer matches EXPECTED_DIGEST; to fix, move the
ACTUAL_DIGEST calculation and the parts_dir/exportFile checks to after the
normalization step (or alternatively ensure the build task emits EXPECTED_DIGEST
for the post-normalization layout), updating the logic that sets ACTUAL_DIGEST
(the sha256 computation for parts_dir and exportFile) so it runs only after the
boot_a→boot_b duplication is applied.
---
Duplicate comments:
In @.github/workflows/backport.yml:
- Line 23: The workflow uses mutable tags for actions (e.g.,
actions/create-github-app-token@v2, peter-evans/create-pull-request@v4,
actions/checkout@v4); replace each tag reference with the corresponding full
40-character commit SHA from the action's repository to pin the exact revision,
updating the three "uses:" lines accordingly (find the latest recommended SHA
for each action in its GitHub repo and substitute it for `@vX`), and re-run the
verification regex provided in the review to ensure no action reference remains
unpinned.
In @.github/workflows/build.yml:
- Around line 168-172: Replace the fragile unquoted expansion of BUNDLE_FILES
used in the tkn bundle push steps by building an array of file-flag arguments
(e.g. bundle_args) where each entry is "-f" and the corresponding path, then
call tkn bundle push with the array expansion quoted as "${bundle_args[@]}";
update the steps that reference BUNDLE_FILES (the variable and the two tkn
bundle push invocations) to populate bundle_args from _output/tasks/*.yaml and
pass the array to tkn bundle push to avoid word-splitting issues (refer to
BUNDLE_FILES, bundle_args, and the tkn bundle push invocations to locate the
changes).
In `@internal/controller/imagebuild/controller.go`:
- Around line 1946-1954: The upload pod currently copies only NodeSelector and
Tolerations from operatorConfig.Spec.OSBuilds; it must also carry the same
architecture affinity used by the build pods. Locate the architecture affinity
logic applied for build pods (the block that sets pod.Spec.Affinity based on
operatorConfig.Spec.OSBuilds / the kubernetes.io/arch constraint) and apply the
same affinity to the upload pod (set pod.Spec.Affinity accordingly) when
operatorConfig.Spec.OSBuilds is present so the WaitForFirstConsumer PVC lands on
nodes with the correct architecture.
- Around line 546-549: When imageBuild.Spec.SecureBuild is true but
operatorConfig.Spec.OSBuilds.TaskBundleRef is empty, the code currently leaves
buildConfig.TaskResolver unset and later falls back to cluster tasks; change the
check around where buildConfig.TaskResolver and buildConfig.TaskBundleRef are
set (the block referencing imageBuild, operatorConfig and buildConfig) to fail
closed: if SecureBuild==true and TaskBundleRef=="" return an error (e.g.,
fmt.Errorf("secure build requested but no TaskBundleRef configured")) or set the
request/Build status to rejected instead of leaving resolver empty, preventing
the fallback to cluster tasks; ensure the early return/error happens before
further processing that would use buildConfig (so callers of this code can
surface the rejection).
---
Nitpick comments:
In `@internal/common/tasks/pipeline_test.go`:
- Around line 132-142: The test currently checks only container-url and disk-url
in the finally-task params; add equivalent assertions for the digest params so
the provenance wiring is covered: after building paramNames from task.Params
verify paramNames["container-digest"] ==
"$(tasks.build-image.results.IMAGE_DIGEST)" and paramNames["disk-digest"] ==
"$(tasks.push-disk-artifact.results.IMAGE_DIGEST)" and produce t.Errorf messages
similar to the existing ones (e.g., "container-digest param = %q, want
build-image result ref") to fail the test if the digest refs are wrong; update
the block that builds paramNames from task.Params to include these two new
checks.
🪄 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: e39b9c74-5969-4452-8a71-5801643b3c8b
⛔ Files ignored due to path filters (2)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (20)
.github/workflows/backport.yml.github/workflows/build.yml.gitignoreMakefileapi/v1alpha1/imagebuild_types.goapi/v1alpha1/operatorconfig_types.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/image/image.gocmd/caib/main.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_test.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.gointernal/controller/operatorconfig/controller.go
✅ Files skipped from review due to trivial changes (2)
- cmd/caib/main.go
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/common/tasks/scripts/build_image.sh
Comment on lines
+197
to
+220
| # Verify artifact integrity against the digest produced by the build task. | ||
| EXPECTED_DIGEST="$(params.expected-artifact-digest)" | ||
| if [ -n "$EXPECTED_DIGEST" ]; then | ||
| echo "=== Artifact Integrity Verification ===" | ||
| if [ -d "${parts_dir}" ] && [ -n "$(ls -A "${parts_dir}" 2>/dev/null)" ]; then | ||
| ACTUAL_DIGEST="sha256:$(cd "${parts_dir}" && find . -maxdepth 1 -type f ! -name '*.size' ! -name 'aib-manifest.yml' -printf '%f\n' | sort | xargs sha256sum | sha256sum | cut -d' ' -f1)" | ||
| elif [ -f "${exportFile}" ]; then | ||
| ACTUAL_DIGEST="sha256:$(sha256sum "${exportFile}" | cut -d' ' -f1)" | ||
| else | ||
| echo "WARNING: Cannot verify integrity — artifact not found yet" | ||
| ACTUAL_DIGEST="" | ||
| fi | ||
| if [ -n "$ACTUAL_DIGEST" ]; then | ||
| if [ "$EXPECTED_DIGEST" != "$ACTUAL_DIGEST" ]; then | ||
| echo "ERROR: Artifact integrity check failed!" >&2 | ||
| echo " Expected: $EXPECTED_DIGEST" >&2 | ||
| echo " Actual: $ACTUAL_DIGEST" >&2 | ||
| exit 1 | ||
| fi | ||
| echo " Integrity verified: $ACTUAL_DIGEST" | ||
| fi | ||
| else | ||
| echo "No artifact integrity digest provided, skipping verification" | ||
| fi |
There was a problem hiding this comment.
Verify the digest after target-specific part normalization.
Line 197 validates parts_dir before the later boot_a → boot_b duplication for ride4*|ridesx4*. In that case, this check can pass even though the multi-layer artifact that gets pushed contains an extra layer not covered by EXPECTED_DIGEST, which breaks the build→push integrity chain for those targets. Move the digest calculation after that normalization step, or have the build task hash the post-normalization layout instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/common/tasks/scripts/push_artifact.sh` around lines 197 - 220, The
integrity check computes ACTUAL_DIGEST before the target-specific normalization
(the boot_a → boot_b duplication for targets matching ride4*|ridesx4*), so
parts_dir may be validated but the pushed artifact gets an extra layer and no
longer matches EXPECTED_DIGEST; to fix, move the ACTUAL_DIGEST calculation and
the parts_dir/exportFile checks to after the normalization step (or
alternatively ensure the build task emits EXPECTED_DIGEST for the
post-normalization layout), updating the logic that sets ACTUAL_DIGEST (the
sha256 computation for parts_dir and exportFile) so it runs only after the
boot_a→boot_b duplication is applied.
0dcdfbb to
feccc96
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/common/tasks/scripts/push_artifact.sh (1)
197-220:⚠️ Potential issue | 🟡 MinorDigest verification occurs before target-specific normalization.
The integrity check computes
ACTUAL_DIGESTat lines 201-204, but theboot_a→boot_bduplication forride4*|ridesx4*targets happens later at lines 234-246. This meansparts_dircan pass verification, but the final pushed artifact contains an extra duplicated layer not covered byEXPECTED_DIGEST.Consider moving the digest calculation after the normalization step, or ensure the build task computes
ARTIFACT_INTEGRITY_DIGESTfor the post-normalization layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/push_artifact.sh` around lines 197 - 220, The digest is currently computed using EXPECTED_DIGEST/ACTUAL_DIGEST before the target-specific normalization that duplicates boot_a→boot_b for ride4*|ridesx4* targets, so the pushed artifact can differ from what was verified; to fix, move the ACTUAL_DIGEST calculation block (the code that sets ACTUAL_DIGEST using parts_dir/exportFile and compares to EXPECTED_DIGEST) to after the normalization logic that performs the boot_a→boot_b duplication, or alternatively ensure the build task exposes a post-normalization ARTIFACT_INTEGRITY_DIGEST that you compare instead of computing ACTUAL_DIGEST early; update references to EXPECTED_DIGEST, ACTUAL_DIGEST, parts_dir, exportFile and the normalization code handling boot_a/boot_b (ride4*|ridesx4*) so verification reflects the final layout.
🧹 Nitpick comments (2)
internal/common/tasks/pipeline_test.go (1)
283-294: Consider usingstrings.Containsfrom stdlib.The
contains()andsearchString()helper functions reimplementstrings.Contains(). Using the standard library would be cleaner:🔧 Suggested simplification
+import "strings" + func TestCollectImagesScript_JSONOutput(t *testing.T) { // ... - if !contains(script, "IMAGES=\"[\"") { + if !strings.Contains(script, "IMAGES=\"[\"") { t.Error("script should initialize JSON array") } - if !contains(script, "IMAGES=\"${IMAGES}]\"") { + if !strings.Contains(script, "IMAGES=\"${IMAGES}]\"") { t.Error("script should close JSON array") } // ... } - -func contains(s, substr string) bool { - return len(s) >= len(substr) && searchString(s, substr) -} - -func searchString(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/pipeline_test.go` around lines 283 - 294, The helper functions contains and searchString reimplement standard behavior—replace all uses of contains(...) with strings.Contains(...) and remove the searchString function (and contains) from internal/common/tasks/pipeline_test.go; also add or ensure the "strings" import is present in the test file and run `go vet`/`go test` to verify no references remain..github/workflows/build.yml (1)
141-176: Tekton bundle build job is well-structured.The job correctly:
- Runs on
mainandrelease-*branches- Uses Go 1.24 consistent with other jobs
- Properly handles file arguments with an array (addressing past review)
- Pushes both versioned and
latesttagsMinor robustness concern: If
_output/tasks/*.yamlmatches no files, bash will iterate once with the literal string_output/tasks/*.yaml. Consider enablingnullglobor adding a file existence check:🔧 Suggested improvement
- name: Push Tekton Bundle run: | + shopt -s nullglob bundle_args=() for f in _output/tasks/*.yaml; do bundle_args+=(-f "$f") done + if [ ${`#bundle_args`[@]} -eq 0 ]; then + echo "ERROR: No task YAMLs found in _output/tasks/" >&2 + exit 1 + fi tkn bundle push "${{ env.TEKTON_BUNDLE_IMAGE }}:${{ env.VERSION }}" "${bundle_args[@]}" tkn bundle push "${{ env.TEKTON_BUNDLE_IMAGE }}:latest" "${bundle_args[@]}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 141 - 176, The build-tekton-bundle step can iterate a literal glob when no files exist; modify the step that builds bundle_args (the for loop and bundle_args array) to guard against missing files by either enabling bash nullglob (e.g., shopt -s nullglob before the loop) or by expanding the glob into an array first and checking its length (e.g., files=( _output/tasks/*.yaml ); if files is empty then skip/fail gracefully), then iterate over that array to populate bundle_args and perform the tkn bundle push only when files are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 152-156: The CI step "Install tkn CLI" currently downloads and
installs tkn without integrity checks; update that step to fetch the official
checksum or signature for tkn_0.40.0_Linux_x86_64.tar.gz (or the corresponding
release artifact), verify the downloaded archive using sha256 (or GPG) before
extracting and moving it, and fail the job if verification does not match;
specifically modify the "Install tkn CLI" step to curl the release
checksum/signature, validate the tarball (e.g., via sha256sum -c or gpg
--verify), then only run tar and sudo mv when verification succeeds.
---
Duplicate comments:
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 197-220: The digest is currently computed using
EXPECTED_DIGEST/ACTUAL_DIGEST before the target-specific normalization that
duplicates boot_a→boot_b for ride4*|ridesx4* targets, so the pushed artifact can
differ from what was verified; to fix, move the ACTUAL_DIGEST calculation block
(the code that sets ACTUAL_DIGEST using parts_dir/exportFile and compares to
EXPECTED_DIGEST) to after the normalization logic that performs the
boot_a→boot_b duplication, or alternatively ensure the build task exposes a
post-normalization ARTIFACT_INTEGRITY_DIGEST that you compare instead of
computing ACTUAL_DIGEST early; update references to EXPECTED_DIGEST,
ACTUAL_DIGEST, parts_dir, exportFile and the normalization code handling
boot_a/boot_b (ride4*|ridesx4*) so verification reflects the final layout.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 141-176: The build-tekton-bundle step can iterate a literal glob
when no files exist; modify the step that builds bundle_args (the for loop and
bundle_args array) to guard against missing files by either enabling bash
nullglob (e.g., shopt -s nullglob before the loop) or by expanding the glob into
an array first and checking its length (e.g., files=( _output/tasks/*.yaml ); if
files is empty then skip/fail gracefully), then iterate over that array to
populate bundle_args and perform the tkn bundle push only when files are
present.
In `@internal/common/tasks/pipeline_test.go`:
- Around line 283-294: The helper functions contains and searchString
reimplement standard behavior—replace all uses of contains(...) with
strings.Contains(...) and remove the searchString function (and contains) from
internal/common/tasks/pipeline_test.go; also add or ensure the "strings" import
is present in the test file and run `go vet`/`go test` to verify no references
remain.
🪄 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: 4745b289-8fbe-4216-b33a-b83cd0cadb29
⛔ Files ignored due to path filters (2)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (19)
.github/workflows/build.yml.gitignoreMakefileapi/v1alpha1/imagebuild_types.goapi/v1alpha1/operatorconfig_types.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/image/image.gocmd/caib/main.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_test.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.gointernal/controller/operatorconfig/controller.go
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- cmd/caib/main.go
- internal/controller/operatorconfig/controller.go
- cmd/caib/runtime_wiring.go
- internal/common/tasks/tasks.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/caib/buildcmd/build_disk_test.go
- api/v1alpha1/operatorconfig_types.go
- api/v1alpha1/imagebuild_types.go
- internal/buildapi/server.go
- internal/controller/imagebuild/controller.go
- internal/common/tasks/scripts/build_image.sh
bkhizgiy
approved these changes
Apr 12, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
internal/common/tasks/scripts/push_artifact.sh (1)
197-216:⚠️ Potential issue | 🟠 MajorMove integrity verification after target-specific normalization.
The artifact integrity check runs at line 201 using
parts_dir, but theboot_a→boot_bduplication forride4*|ridesx4*targets happens later (lines 231-242). This means:
- Build task computes digest of
parts_dir(containsboot_aonly)- Push task verifies against
parts_dir→ passes- Push task duplicates
boot_a→boot_b- Push task pushes artifact (now includes
boot_b)The pushed artifact differs from what was integrity-verified.
🔧 Proposed fix: move verification after normalization
cd /workspace/shared -# Verify artifact integrity against the digest produced by the build task. -EXPECTED_DIGEST="$(params.expected-artifact-digest)" -if [ -n "$EXPECTED_DIGEST" ]; then - echo "=== Artifact Integrity Verification ===" - ACTUAL_DIGEST=$(compute_artifact_digest "${parts_dir}" "${exportFile}") - if [ -z "$ACTUAL_DIGEST" ]; then - echo "WARNING: Cannot verify integrity — artifact not found yet" - fi - if [ -n "$ACTUAL_DIGEST" ]; then - if [ "$EXPECTED_DIGEST" != "$ACTUAL_DIGEST" ]; then - echo "ERROR: Artifact integrity check failed!" >&2 - echo " Expected: $EXPECTED_DIGEST" >&2 - echo " Actual: $ACTUAL_DIGEST" >&2 - exit 1 - fi - echo " Integrity verified: $ACTUAL_DIGEST" - fi -else - echo "No artifact integrity digest provided, skipping verification" -fi +EXPECTED_DIGEST="$(params.expected-artifact-digest)" echo "=== Artifact Push Configuration ==="Then add the verification block after the
boot_a→boot_bduplication (after line 242):;; esac + # Verify artifact integrity AFTER target-specific normalization + if [ -n "$EXPECTED_DIGEST" ]; then + echo "=== Artifact Integrity Verification ===" + ACTUAL_DIGEST=$(compute_artifact_digest "${parts_dir}" "${exportFile}") + if [ -z "$ACTUAL_DIGEST" ]; then + echo "WARNING: Cannot verify integrity — artifact not found" + elif [ "$EXPECTED_DIGEST" != "$ACTUAL_DIGEST" ]; then + echo "ERROR: Artifact integrity check failed!" >&2 + echo " Expected: $EXPECTED_DIGEST" >&2 + echo " Actual: $ACTUAL_DIGEST" >&2 + exit 1 + else + echo " Integrity verified: $ACTUAL_DIGEST" + fi + fi ls -la "${parts_dir}/"Note: Alternatively, the build task could emit the digest for the post-normalization layout by also performing the duplication before hashing, but that would require duplicating the target-specific logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/push_artifact.sh` around lines 197 - 216, The integrity check runs too early against parts_dir before the target-specific normalization that duplicates boot_a→boot_b for ride4*/ridesx4* targets; move the EXPECTED_DIGEST / compute_artifact_digest verification block so it executes after the boot_a→boot_b duplication (i.e., after the case branch that performs the duplication and before the final ls -la "${parts_dir}/"), and use the same logic but ensure you call compute_artifact_digest "${parts_dir}" "${exportFile}" there, handle empty ACTUAL_DIGEST with a warning, compare EXPECTED_DIGEST vs ACTUAL_DIGEST and exit 1 on mismatch, otherwise print the verified digest..github/workflows/build.yml (1)
169-176:⚠️ Potential issue | 🟡 MinorHandle empty task glob explicitly before bundle push.
If
_output/tasks/*.yamlmatches nothing, the loop can pass a literal glob path totkn, causing a confusing failure path.Suggested hardening
- name: Push Tekton Bundle run: | + shopt -s nullglob bundle_args=() for f in _output/tasks/*.yaml; do bundle_args+=(-f "$f") done + if [ ${`#bundle_args`[@]} -eq 0 ]; then + echo "No exported task YAMLs found under _output/tasks" >&2 + exit 1 + fi tkn bundle push "${{ env.TEKTON_BUNDLE_IMAGE }}:${{ env.VERSION }}" "${bundle_args[@]}" tkn bundle push "${{ env.TEKTON_BUNDLE_IMAGE }}:latest" "${bundle_args[@]}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 169 - 176, The loop that builds bundle_args from the glob `_output/tasks/*.yaml` can pass the literal glob when there are no matches; update the step that populates bundle_args (the for f in _output/tasks/*.yaml loop) to first detect whether the glob yields any files (e.g., use a test like checking if any file exists with _output/tasks/*.yaml or use a shell glob test/array capture) and only run the tkn bundle push commands if bundle_args is non-empty; ensure the tkn bundle push calls (tkn bundle push "${{ env.TEKTON_BUNDLE_IMAGE }}:${{ env.VERSION }}" "${bundle_args[@]}" and the :latest call) are guarded so they never receive the unexpanded glob.internal/controller/imagebuild/controller.go (1)
523-552:⚠️ Potential issue | 🟠 MajorFail closed when
secureBuildis requested butOperatorConfigis absent.The new guard only runs inside
err == nil && operatorConfig.Spec.OSBuilds != nil. If theconfigresource is missing, orspec.osBuildsis nil,buildConfigstays unset and Lines 1015-1021 fall back to the cluster-installed pipeline. That silently turnssecureBuild: trueinto a non-secure build instead of rejecting it.Suggested direction
operatorConfig := &automotivev1alpha1.OperatorConfig{} err := r.Get(ctx, types.NamespacedName{Name: "config", Namespace: OperatorNamespace}, operatorConfig) if err != nil && !errors.IsNotFound(err) { return fmt.Errorf("failed to get OperatorConfig configuration: %w", err) } + + if imageBuild.Spec.SecureBuild { + if err != nil || operatorConfig.Spec.OSBuilds == nil || operatorConfig.Spec.OSBuilds.TaskBundleRef == "" { + return fmt.Errorf("secureBuild requested but OperatorConfig.spec.osBuilds.taskBundleRef is not set") + } + } var buildConfig *tasks.BuildConfig if err == nil && operatorConfig.Spec.OSBuilds != nil { buildConfig = &tasks.BuildConfig{ ... } controllerutils.ApplyTrustedCABundleFromOSBuilds(buildConfig, operatorConfig.Spec.OSBuilds) - if imageBuild.Spec.SecureBuild { - if operatorConfig.Spec.OSBuilds.TaskBundleRef == "" { - return fmt.Errorf("secureBuild requested but OperatorConfig.spec.osBuilds.taskBundleRef is not set") - } - buildConfig.TaskResolver = tasks.TaskResolverBundle - buildConfig.TaskBundleRef = operatorConfig.Spec.OSBuilds.TaskBundleRef - } + if imageBuild.Spec.SecureBuild { + buildConfig.TaskResolver = tasks.TaskResolverBundle + buildConfig.TaskBundleRef = operatorConfig.Spec.OSBuilds.TaskBundleRef + } }Also applies to: 1015-1021
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 523 - 552, The code currently only enforces secureBuild checks inside the block guarded by err == nil && operatorConfig.Spec.OSBuilds != nil, which allows imageBuild.Spec.SecureBuild to silently fall back when the OperatorConfig is missing; to fix, immediately after attempting to r.Get the OperatorConfig (and before using buildConfig) add an explicit check for imageBuild.Spec.SecureBuild: if SecureBuild is true and (err != nil || operatorConfig.Spec.OSBuilds == nil || operatorConfig.Spec.OSBuilds.TaskBundleRef == ""), return an error indicating secure builds require a present OperatorConfig with osBuilds and taskBundleRef; reference operatorConfig, imageBuild.Spec.SecureBuild, r.Get, OperatorNamespace, and operatorConfig.Spec.OSBuilds when adding this guard so secureBuild cannot be turned into a non-secure fallback.
🧹 Nitpick comments (1)
internal/controller/imagebuild/controller.go (1)
1947-1974: Extract the arch-affinity builder into a shared helper.This now duplicates the
kubernetes.io/archaffinity construction increateBuildTaskRun. A small helper would keep upload-pod and PipelineRun scheduling rules in lockstep the next time one side changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 1947 - 1974, The arch-affinity construction for kubernetes.io/arch is duplicated here and in createBuildTaskRun; extract a shared helper (e.g., function buildArchAffinity(arch string) *corev1.Affinity) that builds the corev1.Affinity using imageBuild.Spec.Architecture and corev1.LabelArchStable, then replace the inline pod.Spec.Affinity assignment in this file and the matching code in createBuildTaskRun with calls to that helper so both upload-pod and PipelineRun use the same scheduling rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/ISSUE_TEMPLATE/bug_report.md:
- Around line 22-24: The unlabeled fenced code block containing "Paste relevant
logs here" in the bug report template triggers markdownlint MD040; update that
fence to include a language identifier (e.g., change the opening ``` to ```text
or ```log) so the block becomes ```text Paste relevant logs here ```; ensure
only the opening fence is modified to include the identifier in the
.github/ISSUE_TEMPLATE/bug_report.md snippet.
In @.github/workflows/build.yml:
- Around line 187-190: The "Install crane" step downloads and installs the crane
binary without verifying integrity; update the step that runs under the "Install
crane" job to fetch the corresponding checksum (or signature) from the GitHub
release and verify it before moving the binary to /usr/local/bin: download both
the artifact and its checksum file (or signature), validate the artifact with
sha256sum -c or gpg --verify, fail the job on mismatch, and only on successful
verification perform the sudo mv; reference the step name "Install crane" and
the current curl/tar/mv sequence to locate where to add the checksum/signature
download and verification logic.
In `@api/v1alpha1/operatorconfig_types.go`:
- Around line 539-545: TaskBundleRef must be pinned to a digest for secure
builds; add validation to reject mutable tags by (1) adding a kubebuilder
validation pattern on the TaskBundleRef field (e.g.
+kubebuilder:validation:Pattern="^.+@sha256:[a-fA-F0-9]{64}$") so digest-pinned
refs are required, and (2) implement/extend the OperatorConfig webhook
validation methods (ValidateCreate/ValidateUpdate on the OperatorConfig type) to
conditionally reject objects where SecureBuild (or SecureBuild=true) is set but
TaskBundleRef does not match the `@sha256`: pattern, returning a clear admission
error referencing TaskBundleRef.
---
Duplicate comments:
In @.github/workflows/build.yml:
- Around line 169-176: The loop that builds bundle_args from the glob
`_output/tasks/*.yaml` can pass the literal glob when there are no matches;
update the step that populates bundle_args (the for f in _output/tasks/*.yaml
loop) to first detect whether the glob yields any files (e.g., use a test like
checking if any file exists with _output/tasks/*.yaml or use a shell glob
test/array capture) and only run the tkn bundle push commands if bundle_args is
non-empty; ensure the tkn bundle push calls (tkn bundle push "${{
env.TEKTON_BUNDLE_IMAGE }}:${{ env.VERSION }}" "${bundle_args[@]}" and the
:latest call) are guarded so they never receive the unexpanded glob.
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 197-216: The integrity check runs too early against parts_dir
before the target-specific normalization that duplicates boot_a→boot_b for
ride4*/ridesx4* targets; move the EXPECTED_DIGEST / compute_artifact_digest
verification block so it executes after the boot_a→boot_b duplication (i.e.,
after the case branch that performs the duplication and before the final ls -la
"${parts_dir}/"), and use the same logic but ensure you call
compute_artifact_digest "${parts_dir}" "${exportFile}" there, handle empty
ACTUAL_DIGEST with a warning, compare EXPECTED_DIGEST vs ACTUAL_DIGEST and exit
1 on mismatch, otherwise print the verified digest.
In `@internal/controller/imagebuild/controller.go`:
- Around line 523-552: The code currently only enforces secureBuild checks
inside the block guarded by err == nil && operatorConfig.Spec.OSBuilds != nil,
which allows imageBuild.Spec.SecureBuild to silently fall back when the
OperatorConfig is missing; to fix, immediately after attempting to r.Get the
OperatorConfig (and before using buildConfig) add an explicit check for
imageBuild.Spec.SecureBuild: if SecureBuild is true and (err != nil ||
operatorConfig.Spec.OSBuilds == nil ||
operatorConfig.Spec.OSBuilds.TaskBundleRef == ""), return an error indicating
secure builds require a present OperatorConfig with osBuilds and taskBundleRef;
reference operatorConfig, imageBuild.Spec.SecureBuild, r.Get, OperatorNamespace,
and operatorConfig.Spec.OSBuilds when adding this guard so secureBuild cannot be
turned into a non-secure fallback.
---
Nitpick comments:
In `@internal/controller/imagebuild/controller.go`:
- Around line 1947-1974: The arch-affinity construction for kubernetes.io/arch
is duplicated here and in createBuildTaskRun; extract a shared helper (e.g.,
function buildArchAffinity(arch string) *corev1.Affinity) that builds the
corev1.Affinity using imageBuild.Spec.Architecture and corev1.LabelArchStable,
then replace the inline pod.Spec.Affinity assignment in this file and the
matching code in createBuildTaskRun with calls to that helper so both upload-pod
and PipelineRun use the same scheduling rule.
🪄 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: 02443c2b-e228-43c4-8ba5-a8fd00114e16
⛔ Files ignored due to path filters (2)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (28)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/PULL_REQUEST_TEMPLATE.md.github/dependabot.yml.github/workflows/backport.yml.github/workflows/build.yml.github/workflows/lint.yml.gitignore.pre-commit-config.yamlMakefileapi/v1alpha1/imagebuild_types.goapi/v1alpha1/operatorconfig_types.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/image/image.gocmd/caib/main.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.godocs/adr/0001-use-architecture-decision-records.mdinternal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_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.go
✅ Files skipped from review due to trivial changes (11)
- .gitignore
- cmd/caib/main.go
- .github/PULL_REQUEST_TEMPLATE.md
- .github/workflows/lint.yml
- .github/ISSUE_TEMPLATE/feature_request.md
- .github/dependabot.yml
- docs/adr/0001-use-architecture-decision-records.md
- internal/buildapi/types.go
- .pre-commit-config.yaml
- cmd/caib/buildcmd/build_disk_test.go
- Makefile
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/controller/operatorconfig/controller.go
- cmd/caib/runtime_wiring.go
- internal/buildapi/server.go
- internal/common/tasks/tasks.go
- internal/common/tasks/pipeline_test.go
Comment on lines
+539
to
+545
| // TaskBundleRef is the OCI reference to a signed Tekton Bundle containing task definitions. | ||
| // When set, builds created with SecureBuild=true will resolve tasks from this bundle | ||
| // instead of the cluster-installed tasks. The bundle should be signed with cosign | ||
| // and contain the same tasks as the operator deploys. | ||
| // Example: "quay.io/rh-sdv-cloud/automotive-dev-tekton-tasks:v0.1.0@sha256:abc123..." | ||
| // +optional | ||
| TaskBundleRef string `json:"taskBundleRef,omitempty"` |
There was a problem hiding this comment.
Require digest-pinned bundle refs for secure builds.
TaskBundleRef currently accepts mutable tags, so a retag can change the resolved task payload without any secureBuild spec change. That weakens the immutability/auditability guarantee this feature is trying to provide. Please validate or reject refs that are not pinned with @sha256:.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v1alpha1/operatorconfig_types.go` around lines 539 - 545, TaskBundleRef
must be pinned to a digest for secure builds; add validation to reject mutable
tags by (1) adding a kubebuilder validation pattern on the TaskBundleRef field
(e.g. +kubebuilder:validation:Pattern="^.+@sha256:[a-fA-F0-9]{64}$") so
digest-pinned refs are required, and (2) implement/extend the OperatorConfig
webhook validation methods (ValidateCreate/ValidateUpdate on the OperatorConfig
type) to conditionally reject objects where SecureBuild (or SecureBuild=true) is
set but TaskBundleRef does not match the `@sha256`: pattern, returning a clear
admission error referencing TaskBundleRef.
dd3d11d to
a8f2e7f
Compare
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)
internal/controller/imagebuild/controller.go (1)
530-552:⚠️ Potential issue | 🟠 MajorReturn error when
SecureBuild=truebutOperatorConfigorOSBuildsis missing.The
SecureBuildvalidation at line 546 is nested inside theOSBuildsguard. IfOperatorConfigis missing orspec.osBuildsis nil, the validation never runs andbuildConfigstays nil. This causes lines 1015–1021 to silently fall back to the cluster-installed pipeline, which is unsigned and violates the caller's intent to use secure build. Add a check at the top level: ifimageBuild.Spec.SecureBuildis true butbuildConfigis nil after the config fetch, return an error instead of proceeding to the fallback path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 530 - 552, Add a top-level validation after operatorConfig/OSBuilds are processed: if imageBuild.Spec.SecureBuild is true but buildConfig is still nil (meaning operatorConfig or operatorConfig.Spec.OSBuilds was missing), return an error indicating secure build requested but no OSBuilds/operator config found. In other words, after the block that may set buildConfig (and before any fallback to cluster pipelines), check imageBuild.Spec.SecureBuild && buildConfig == nil and return a clear fmt.Errorf referencing the missing OperatorConfig/OSBuilds so callers don’t silently fall back to an unsigned pipeline; keep existing in-block secureBuild checks (TaskBundleRef) intact.
♻️ Duplicate comments (3)
internal/common/tasks/scripts/push_artifact.sh (1)
197-216:⚠️ Potential issue | 🟠 MajorCompute the integrity digest after target-specific layout normalization.
Lines 197-213 hash the artifact before lines 230-242 may add
boot_bforride4*|ridesx4*. On those targets, the pushed multi-layer artifact is no longer the artifact that was verified, so the build→push integrity chain is incomplete. Move the digest check after theboot_aduplication, or have the build task hash the post-normalization layout instead.Also applies to: 230-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/push_artifact.sh` around lines 197 - 216, The integrity digest is being computed with compute_artifact_digest using EXPECTED_DIGEST, parts_dir and exportFile before target-specific layout normalization (the boot_a/boot_b duplication for ride4*/ridesx4*), so the pushed multi-layer artifact no longer matches the verified digest; either move the EXPECTED_DIGEST/compute_artifact_digest check to after the layout normalization/boot_a duplication (the code that adds boot_b from boot_a for ride4*/ridesx4*), or change the build task to produce and store the digest of the post-normalization layout (i.e., compute the digest after boot_a duplication), ensuring compute_artifact_digest is called on the final artifact layout that will be pushed..github/workflows/build.yml (1)
187-190:⚠️ Potential issue | 🟠 MajorStill verify the
cranetarball before extracting it.This step still pipes a GitHub release artifact straight into
tar, so a compromised download becomes code execution in CI. Please download the archive and release checksums separately, verify the SHA-256, then extract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 187 - 190, In the "Install crane" CI step replace the direct curl | tar pipeline with a download-and-verify flow: use the same release URL for the go-containerregistry tarball and the corresponding checksum file, compute the downloaded archive's SHA-256 and compare it to the expected checksum, only after the verification succeed run tar to extract "crane" and then sudo mv crane /usr/local/bin/; update the step that currently uses curl -sSL "https://github.com/google/go-containerregistry/releases/download/..." | tar xz crane so that it downloads both artifact and checksum, verifies the SHA-256 match, and aborts the job if verification fails.api/v1alpha1/operatorconfig_types.go (1)
539-545:⚠️ Potential issue | 🟠 MajorRequire digest-pinned bundle refs for secure builds.
taskBundleRefstill accepts mutable tags, so the resolved task payload can change without anyOperatorConfigorImageBuildspec change. That undermines the immutability this feature is trying to provide. Please reject non-empty refs that are not pinned with@sha256:. As per coding guidelines, "After modifying types in api/v1alpha1/, runmake generate manifeststo regenerate DeepCopy methods and CRDs".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1alpha1/operatorconfig_types.go` around lines 539 - 545, TaskBundleRef currently allows mutable tags; update validation so any non-empty TaskBundleRef on the OperatorConfig (field TaskBundleRef in api/v1alpha1/operatorconfig_types.go) must include an immutable digest (contain "@sha256:") and reject otherwise, returning a validation error for the CR (implement in the relevant ValidateCreate/ValidateUpdate or webhook validation logic that checks the TaskBundleRef string); after changing the type/validation run "make generate manifests" to regenerate deepcopy methods and CRDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/controller/imagebuild/controller.go`:
- Around line 530-552: Add a top-level validation after operatorConfig/OSBuilds
are processed: if imageBuild.Spec.SecureBuild is true but buildConfig is still
nil (meaning operatorConfig or operatorConfig.Spec.OSBuilds was missing), return
an error indicating secure build requested but no OSBuilds/operator config
found. In other words, after the block that may set buildConfig (and before any
fallback to cluster pipelines), check imageBuild.Spec.SecureBuild && buildConfig
== nil and return a clear fmt.Errorf referencing the missing
OperatorConfig/OSBuilds so callers don’t silently fall back to an unsigned
pipeline; keep existing in-block secureBuild checks (TaskBundleRef) intact.
---
Duplicate comments:
In @.github/workflows/build.yml:
- Around line 187-190: In the "Install crane" CI step replace the direct curl |
tar pipeline with a download-and-verify flow: use the same release URL for the
go-containerregistry tarball and the corresponding checksum file, compute the
downloaded archive's SHA-256 and compare it to the expected checksum, only after
the verification succeed run tar to extract "crane" and then sudo mv crane
/usr/local/bin/; update the step that currently uses curl -sSL
"https://github.com/google/go-containerregistry/releases/download/..." | tar xz
crane so that it downloads both artifact and checksum, verifies the SHA-256
match, and aborts the job if verification fails.
In `@api/v1alpha1/operatorconfig_types.go`:
- Around line 539-545: TaskBundleRef currently allows mutable tags; update
validation so any non-empty TaskBundleRef on the OperatorConfig (field
TaskBundleRef in api/v1alpha1/operatorconfig_types.go) must include an immutable
digest (contain "@sha256:") and reject otherwise, returning a validation error
for the CR (implement in the relevant ValidateCreate/ValidateUpdate or webhook
validation logic that checks the TaskBundleRef string); after changing the
type/validation run "make generate manifests" to regenerate deepcopy methods and
CRDs.
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 197-216: The integrity digest is being computed with
compute_artifact_digest using EXPECTED_DIGEST, parts_dir and exportFile before
target-specific layout normalization (the boot_a/boot_b duplication for
ride4*/ridesx4*), so the pushed multi-layer artifact no longer matches the
verified digest; either move the EXPECTED_DIGEST/compute_artifact_digest check
to after the layout normalization/boot_a duplication (the code that adds boot_b
from boot_a for ride4*/ridesx4*), or change the build task to produce and store
the digest of the post-normalization layout (i.e., compute the digest after
boot_a duplication), ensuring compute_artifact_digest is called on the final
artifact layout that will be pushed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: beda813c-b5c8-48d4-aced-ded86f6d9395
⛔ Files ignored due to path filters (2)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (20)
.github/workflows/build.yml.gitignoreMakefileapi/v1alpha1/imagebuild_types.goapi/v1alpha1/operatorconfig_types.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/image/image.gocmd/caib/main.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_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.go
✅ Files skipped from review due to trivial changes (6)
- .gitignore
- cmd/caib/main.go
- cmd/caib/buildcmd/build_disk_test.go
- internal/common/tasks/scripts/build_image.sh
- cmd/export-tasks/main.go
- internal/common/tasks/tasks.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/buildapi/types.go
- internal/controller/operatorconfig/controller.go
- internal/buildapi/server.go
- internal/common/tasks/scripts/common.sh
- api/v1alpha1/imagebuild_types.go
- Makefile
- cmd/caib/runtime_wiring.go
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/common/tasks/scripts/push_artifact.sh (1)
197-216:⚠️ Potential issue | 🟠 MajorVerify the normalized parts layout, not the pre-duplication one.
EXPECTED_DIGESTis checked before theride4*|ridesx4*boot_a→boot_bduplication later in the file, so those targets can pass integrity verification and still push a different artifact set. Please move thecompute_artifact_digestcall until after that normalization step so the build→push digest chain covers the exact bytes ORAS uploads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/push_artifact.sh` around lines 197 - 216, The integrity check currently computes ACTUAL_DIGEST early using compute_artifact_digest with parts_dir and exportFile while EXPECTED_DIGEST is set, but this occurs before the normalization/duplication step (the ride4*|ridesx4* boot_a → boot_b duplication), so move the compute_artifact_digest invocation and the ACTUAL_DIGEST/verification block to after that normalization/duplication code path; ensure you still gate the check on EXPECTED_DIGEST and use the same variables (EXPECTED_DIGEST, ACTUAL_DIGEST, parts_dir, exportFile) so the digest is computed against the final normalized parts layout that ORAS will upload.internal/controller/imagebuild/controller.go (1)
546-555:⚠️ Potential issue | 🟠 MajorKeep
SecureBuildfail-closed whenOperatorConfig/OSBuildsis absent.This validation only runs inside the
err == nil && operatorConfig.Spec.OSBuilds != nilbranch. If anImageBuildwithSpec.SecureBuild=trueis created directly and theOperatorConfigis missing orspec.osBuildsis nil, Lines 1018-1024 still fall back toPipelineRef, so the build silently uses cluster-installed tasks instead of the signed bundle. The secure-build guard needs to reject that path too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 546 - 555, The SecureBuild path must fail-closed when OperatorConfig or OperatorConfig.Spec.OSBuilds is missing: inside the code handling imageBuild.Spec.SecureBuild, first check that operatorConfig is non-nil and operatorConfig.Spec.OSBuilds is non-nil and return an error if either is absent, then preserve the existing checks that TaskBundleRef is set and contains "@sha256:" before setting buildConfig.TaskResolver and buildConfig.TaskBundleRef; this ensures ImageBuild.Spec.SecureBuild cannot silently fall back to PipelineRef when OperatorConfig/OSBuilds is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/buildapi/server.go`:
- Around line 2049-2064: The secureBuild validation block that checks
req.SecureBuild and reads OperatorConfig (including
operatorConfig.Spec.OSBuilds.TaskBundleRef and the "@sha256:" check) must run
before any side-effecting calls like resolveRegistryForBuild() and the
flash-secret/ImageStream creation; move that entire validation earlier in the
handler (before resolveRegistryForBuild() and before any
createFlashSecret/createImageStream logic) so a rejected request returns 400
without creating transient resources, or alternatively add deterministic cleanup
on the error path that deletes any created secrets/ImageStreams and aborts
ImageBuild creation; ensure you reference the existing req.SecureBuild check and
use k8sClient.Get(..., operatorConfig) and the TaskBundleRef digest check when
relocating the logic.
In `@internal/common/tasks/tasks.go`:
- Around line 48-49: In buildTaskRef, don’t silently fall through to cluster
tasks when TaskResolver == TaskResolverBundle but TaskBundleRef is empty;
instead validate and return an explicit error (or a clearly typed sentinel)
immediately when buildConfig.TaskResolver == TaskResolverBundle &&
buildConfig.TaskBundleRef == "" so you fail fast; update the branch that
currently returns a tektonv1.TaskRef to instead surface an error and apply the
same validation to the other bundle-related branch around the 69-87 region so
any missing TaskBundleRef is rejected rather than downgraded to cluster tasks.
- Around line 1326-1413: The finally task collect-images-result is referencing
optional task results from push-disk-artifact (disk-url/disk-digest) which
causes the finally task to be skipped when that task is skipped; replace those
Param.Value references to avoid direct
$(tasks.push-disk-artifact.results.IMAGE_URL) and
$(tasks.push-disk-artifact.results.IMAGE_DIGEST) in collect-images-result and
instead use pipeline Params (e.g., params.disk-url and params.disk-digest) with
sensible defaults (empty string) so the finally step can always resolve its
inputs; keep the push-disk-artifact task responsible for populating those
pipeline params (or add a tiny setter task that runs only when
push-disk-artifact runs) so collect-images-result's Params (disk-url,
disk-digest) no longer directly reference push-disk-artifact and will not cause
the finally task to be skipped.
---
Duplicate comments:
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 197-216: The integrity check currently computes ACTUAL_DIGEST
early using compute_artifact_digest with parts_dir and exportFile while
EXPECTED_DIGEST is set, but this occurs before the normalization/duplication
step (the ride4*|ridesx4* boot_a → boot_b duplication), so move the
compute_artifact_digest invocation and the ACTUAL_DIGEST/verification block to
after that normalization/duplication code path; ensure you still gate the check
on EXPECTED_DIGEST and use the same variables (EXPECTED_DIGEST, ACTUAL_DIGEST,
parts_dir, exportFile) so the digest is computed against the final normalized
parts layout that ORAS will upload.
In `@internal/controller/imagebuild/controller.go`:
- Around line 546-555: The SecureBuild path must fail-closed when OperatorConfig
or OperatorConfig.Spec.OSBuilds is missing: inside the code handling
imageBuild.Spec.SecureBuild, first check that operatorConfig is non-nil and
operatorConfig.Spec.OSBuilds is non-nil and return an error if either is absent,
then preserve the existing checks that TaskBundleRef is set and contains
"@sha256:" before setting buildConfig.TaskResolver and
buildConfig.TaskBundleRef; this ensures ImageBuild.Spec.SecureBuild cannot
silently fall back to PipelineRef when OperatorConfig/OSBuilds is missing.
🪄 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: 60edd0cb-c364-4631-aa97-2ca12fd6beff
⛔ Files ignored due to path filters (2)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (20)
.github/workflows/build.yml.gitignoreMakefileapi/v1alpha1/imagebuild_types.goapi/v1alpha1/operatorconfig_types.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/image/image.gocmd/caib/main.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_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.go
✅ Files skipped from review due to trivial changes (7)
- .gitignore
- cmd/caib/main.go
- cmd/caib/runtime_wiring.go
- cmd/caib/buildcmd/build_disk_test.go
- internal/common/tasks/scripts/common.sh
- internal/controller/operatorconfig/controller.go
- internal/common/tasks/pipeline_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- internal/common/tasks/scripts/build_image.sh
Comment on lines
+48
to
+49
| if buildConfig != nil && buildConfig.TaskResolver == TaskResolverBundle && buildConfig.TaskBundleRef != "" { | ||
| return &tektonv1.TaskRef{ |
There was a problem hiding this comment.
Don't silently downgrade bundle mode to cluster tasks.
If TaskResolver is TaskResolverBundle but TaskBundleRef is empty, this falls through to the cluster resolver. That turns a secure-build misconfiguration into an unsigned cluster-task execution instead of an explicit failure. Please fail fast here or validate this state before calling buildTaskRef.
Also applies to: 69-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/common/tasks/tasks.go` around lines 48 - 49, In buildTaskRef, don’t
silently fall through to cluster tasks when TaskResolver == TaskResolverBundle
but TaskBundleRef is empty; instead validate and return an explicit error (or a
clearly typed sentinel) immediately when buildConfig.TaskResolver ==
TaskResolverBundle && buildConfig.TaskBundleRef == "" so you fail fast; update
the branch that currently returns a tektonv1.TaskRef to instead surface an error
and apply the same validation to the other bundle-related branch around the
69-87 region so any missing TaskBundleRef is rejected rather than downgraded to
cluster tasks.
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)
internal/controller/imagebuild/controller.go (1)
531-556:⚠️ Potential issue | 🟠 MajorFail closed when
secureBuildcannot resolve bundle config.The
secureBuildchecks only run insideerr == nil && operatorConfig.Spec.OSBuilds != nil. IfOperatorConfigis missing orspec.osBuildsis nil, this falls through to the clusterPipelineRefpath and executes unsigned cluster tasks even thoughsecureBuildwas requested.Proposed fix
var buildConfig *tasks.BuildConfig + if imageBuild.Spec.SecureBuild && (err != nil || operatorConfig.Spec.OSBuilds == nil) { + return fmt.Errorf("secureBuild requested but OperatorConfig.spec.osBuilds.taskBundleRef is unavailable") + } if err == nil && operatorConfig.Spec.OSBuilds != nil { // Convert OSBuildsConfig to BuildConfig buildConfig = &tasks.BuildConfig{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 531 - 556, The secureBuild validation currently only executes inside the branch guarded by err == nil && operatorConfig.Spec.OSBuilds != nil, so if OperatorConfig or spec.osBuilds is missing we fall back to cluster PipelineRef and may run unsigned tasks despite imageBuild.Spec.SecureBuild being true; move or duplicate the secureBuild checks to run unconditionally whenever imageBuild.Spec.SecureBuild is true (before choosing between OSBuilds-derived buildConfig vs cluster PipelineRef), validating that operatorConfig and operatorConfig.Spec.OSBuilds exist, that operatorConfig.Spec.OSBuilds.TaskBundleRef is set and contains "@sha256:", and if validation fails return an error; ensure when valid you still set buildConfig.TaskResolver = tasks.TaskResolverBundle and buildConfig.TaskBundleRef = operatorConfig.Spec.OSBuilds.TaskBundleRef so secure builds never fall through to cluster PipelineRef.
♻️ Duplicate comments (3)
internal/common/tasks/scripts/push_artifact.sh (1)
197-216:⚠️ Potential issue | 🟠 MajorVerify after the target-specific layer normalization.
For
ride4*|ridesx4*, Lines 230-242 addboot_bafter the digest check at Lines 197-216. That meansEXPECTED_DIGESTcan match even though the artifact that actually gets pushed contains extra layers/bytes not covered by the verified digest.Also applies to: 230-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/push_artifact.sh` around lines 197 - 216, The integrity check currently computes EXPECTED_DIGEST and calls compute_artifact_digest before the target-specific layer normalization (e.g., before adding boot_b for ride4*/ridesx4*), so the pushed artifact can differ from the verified content; either move the EXPECTED_DIGEST/ACTUAL_DIGEST verification block to run after the target-specific normalization code (the code that appends boot_b or performs layer normalization), or recompute ACTUAL_DIGEST to include those added layers by calling compute_artifact_digest after the normalization step; update references to EXPECTED_DIGEST and ACTUAL_DIGEST around the functions/sections that perform the normalization/append (e.g., the code that appends boot_b) so the final artifact content is what gets verified.internal/common/tasks/tasks.go (1)
1245-1256:⚠️ Potential issue | 🟠 Major
collect-images-resultstill depends on results from an optional task.
push-disk-artifactis conditional, but the finally task still dereferences$(tasks.push-disk-artifact.results.IMAGE_URL)andIMAGE_DIGESTdirectly. In container-only runs that task is skipped, so Tekton skipscollect-images-resultand the pipeline-levelIMAGESresult never gets produced.Also applies to: 1393-1408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/tasks.go` around lines 1245 - 1256, collect-images-result is dereferencing $(tasks.push-disk-artifact.results.IMAGE_URL) and IMAGE_DIGEST even when the conditional task push-disk-artifact is skipped; add the same WhenExpressions used by push-disk-artifact (check $(params.export-oci) and $(params.secret-ref) notin "", "null") to the collect-images-result finally step so it only runs when push-disk-artifact ran, and ensure any other finally steps that reference push-disk-artifact results are similarly guarded (also apply the same change for the similar block around lines 1393-1408).internal/controller/imagebuild/controller.go (1)
1950-1977:⚠️ Potential issue | 🟠 MajorPropagate
RuntimeClassNameto the upload pod as well.
createBuildTaskRun()applies a runtime class to build pods, but the upload pod still only mirrors node selectors, tolerations, and arch affinity. If the runtime class carries scheduling constraints, the PVC can still be first-bound on a node the build pod cannot use.Proposed fix
if operatorConfig.Spec.OSBuilds != nil && len(operatorConfig.Spec.OSBuilds.Tolerations) > 0 { pod.Spec.Tolerations = operatorConfig.Spec.OSBuilds.Tolerations } +if imageBuild.Spec.RuntimeClassName != "" { + pod.Spec.RuntimeClassName = &imageBuild.Spec.RuntimeClassName +} else if operatorConfig.Spec.OSBuilds != nil && operatorConfig.Spec.OSBuilds.RuntimeClassName != "" { + pod.Spec.RuntimeClassName = &operatorConfig.Spec.OSBuilds.RuntimeClassName +} if imageBuild.Spec.Architecture != "" { pod.Spec.Affinity = &corev1.Affinity{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 1950 - 1977, The upload pod creation currently mirrors NodeSelector, Tolerations, and Affinity but does not propagate the RuntimeClassName used by build pods; update the upload pod spec (same place where NodeSelector/Tolerations/Affinity are set in controller.go) to set pod.Spec.RuntimeClassName to the same runtime class used for builds (e.g., propagate imageBuild.Spec.RuntimeClassName or the value set by createBuildTaskRun()), ensuring the upload pod uses the same runtime class scheduling constraints as the build pod.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/controller/imagebuild/controller.go`:
- Around line 531-556: The secureBuild validation currently only executes inside
the branch guarded by err == nil && operatorConfig.Spec.OSBuilds != nil, so if
OperatorConfig or spec.osBuilds is missing we fall back to cluster PipelineRef
and may run unsigned tasks despite imageBuild.Spec.SecureBuild being true; move
or duplicate the secureBuild checks to run unconditionally whenever
imageBuild.Spec.SecureBuild is true (before choosing between OSBuilds-derived
buildConfig vs cluster PipelineRef), validating that operatorConfig and
operatorConfig.Spec.OSBuilds exist, that
operatorConfig.Spec.OSBuilds.TaskBundleRef is set and contains "@sha256:", and
if validation fails return an error; ensure when valid you still set
buildConfig.TaskResolver = tasks.TaskResolverBundle and
buildConfig.TaskBundleRef = operatorConfig.Spec.OSBuilds.TaskBundleRef so secure
builds never fall through to cluster PipelineRef.
---
Duplicate comments:
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 197-216: The integrity check currently computes EXPECTED_DIGEST
and calls compute_artifact_digest before the target-specific layer normalization
(e.g., before adding boot_b for ride4*/ridesx4*), so the pushed artifact can
differ from the verified content; either move the EXPECTED_DIGEST/ACTUAL_DIGEST
verification block to run after the target-specific normalization code (the code
that appends boot_b or performs layer normalization), or recompute ACTUAL_DIGEST
to include those added layers by calling compute_artifact_digest after the
normalization step; update references to EXPECTED_DIGEST and ACTUAL_DIGEST
around the functions/sections that perform the normalization/append (e.g., the
code that appends boot_b) so the final artifact content is what gets verified.
In `@internal/common/tasks/tasks.go`:
- Around line 1245-1256: collect-images-result is dereferencing
$(tasks.push-disk-artifact.results.IMAGE_URL) and IMAGE_DIGEST even when the
conditional task push-disk-artifact is skipped; add the same WhenExpressions
used by push-disk-artifact (check $(params.export-oci) and $(params.secret-ref)
notin "", "null") to the collect-images-result finally step so it only runs when
push-disk-artifact ran, and ensure any other finally steps that reference
push-disk-artifact results are similarly guarded (also apply the same change for
the similar block around lines 1393-1408).
In `@internal/controller/imagebuild/controller.go`:
- Around line 1950-1977: The upload pod creation currently mirrors NodeSelector,
Tolerations, and Affinity but does not propagate the RuntimeClassName used by
build pods; update the upload pod spec (same place where
NodeSelector/Tolerations/Affinity are set in controller.go) to set
pod.Spec.RuntimeClassName to the same runtime class used for builds (e.g.,
propagate imageBuild.Spec.RuntimeClassName or the value set by
createBuildTaskRun()), ensuring the upload pod uses the same runtime class
scheduling constraints as the build pod.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 337dbb21-b623-4bef-a5bd-6d53b37d795d
⛔ Files ignored due to path filters (2)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (20)
.github/workflows/build.yml.gitignoreMakefileapi/v1alpha1/imagebuild_types.goapi/v1alpha1/operatorconfig_types.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/image/image.gocmd/caib/main.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_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.go
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- internal/controller/operatorconfig/controller.go
- cmd/caib/buildcmd/build_disk_test.go
- cmd/caib/runtime_wiring.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/caib/main.go
- Makefile
👮 Files not reviewed due to content moderation or server errors (7)
- internal/buildapi/types.go
- cmd/caib/buildcmd/build.go
- cmd/caib/image/image.go
- api/v1alpha1/imagebuild_types.go
- api/v1alpha1/operatorconfig_types.go
- internal/buildapi/server.go
- internal/common/tasks/scripts/common.sh
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
api/v1alpha1/operatorconfig_types.go (1)
539-545:⚠️ Potential issue | 🟠 MajorRequire strict digest-pinned
taskBundleRefat schema level.
TaskBundleRefstill accepts mutable/invalid refs. Enforce@sha256:<64-hex>in the CRD so invalid config is rejected at admission time, not later at build request time.🔒 Suggested schema guard
// Example: "quay.io/rh-sdv-cloud/automotive-dev-tekton-tasks:v0.1.0@sha256:abc123..." + // +kubebuilder:validation:Pattern=`^.+@sha256:[a-fA-F0-9]{64}$` // +optional TaskBundleRef string `json:"taskBundleRef,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1alpha1/operatorconfig_types.go` around lines 539 - 545, The TaskBundleRef field currently allows mutable refs; add a CRD validation pattern to require a digest-pinned OCI reference by annotating TaskBundleRef with a kubebuilder validation marker that enforces the suffix `@sha256`:<64-hex> (e.g. +kubebuilder:validation:Pattern="^.+@sha256:[a-fA-F0-9]{64}$"), and update the TaskBundleRef field's comment to mention the strict digest requirement; target the TaskBundleRef string struct tag in operatorconfig_types.go to implement this change.internal/buildapi/server.go (1)
2064-2070:⚠️ Potential issue | 🟠 MajorRun secure-build validation before any side effects.
This check still executes after workspace/registry/flash setup paths. A rejected secure build can leave transient resources behind. Move validation earlier in
createBuild(before workspace/registry/secret creation).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/buildapi/server.go` around lines 2064 - 2070, Move the secure-build validation to run before any side effects in createBuild: call validateSecureBuild(ctx, k8sClient, namespace) (when req.SecureBuild is true) at the very start of createBuild, before creating workspace/registry/secret/flash resources so a failure returns immediately (c.JSON and return) and prevents transient resources from being created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/buildapi/server.go`:
- Around line 1948-1950: The current substring check for
operatorConfig.Spec.OSBuilds.TaskBundleRef is too weak; replace it with a strict
digest validation that trims whitespace and verifies the format matches the full
sha256 digest pattern (e.g. .+@sha256:<64 hex chars>). In the function/method
where this occurs (look for the code referencing
operatorConfig.Spec.OSBuilds.TaskBundleRef in server.go), use strings.TrimSpace
on TaskBundleRef and validate with a regular expression like
^.+@sha256:[0-9a-fA-F]{64}$ (or equivalent
regexp.MatchString/regexp.MustCompile) and return the existing fmt.Errorf when
it does not match. Ensure you reference the trimmed value in the error message
so the reported ref is the normalized input.
In `@internal/common/tasks/pipeline_test.go`:
- Around line 266-271: The test currently does strings.SplitN(line, "@", 2) and
then accesses parts[1], which panics when there is no '@'; change the check to
validate exactly one separator by using strings.Count(line, "@") != 1 (or
equivalent), call t.Errorf with the corrected message, and then return early
from that loop iteration (add a continue) to avoid accessing parts[1]; only
after that success check perform strings.SplitN(line, "@", 2) and the subsequent
prefix assertion on parts[1].
- Around line 230-234: Test dereferences task.TaskSpec.Steps without checking
task.TaskSpec for nil; add a defensive nil check like in TestCollectImagesParams
so the test fails cleanly instead of panicking. Update the block that handles
task.Name == "collect-images-result" to first verify task.TaskSpec != nil and
fail with t.Fatal or t.Fatalf if it is nil, then check len(task.TaskSpec.Steps)
and access task.TaskSpec.Steps[0].Script as before (this mirrors the nil-check
pattern used elsewhere and protects against switching to TaskRef/resolver-based
tasks and changes from GenerateTektonPipeline).
---
Duplicate comments:
In `@api/v1alpha1/operatorconfig_types.go`:
- Around line 539-545: The TaskBundleRef field currently allows mutable refs;
add a CRD validation pattern to require a digest-pinned OCI reference by
annotating TaskBundleRef with a kubebuilder validation marker that enforces the
suffix `@sha256`:<64-hex> (e.g.
+kubebuilder:validation:Pattern="^.+@sha256:[a-fA-F0-9]{64}$"), and update the
TaskBundleRef field's comment to mention the strict digest requirement; target
the TaskBundleRef string struct tag in operatorconfig_types.go to implement this
change.
In `@internal/buildapi/server.go`:
- Around line 2064-2070: Move the secure-build validation to run before any side
effects in createBuild: call validateSecureBuild(ctx, k8sClient, namespace)
(when req.SecureBuild is true) at the very start of createBuild, before creating
workspace/registry/secret/flash resources so a failure returns immediately
(c.JSON and return) and prevents transient resources from being created.
🪄 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: c613e753-2756-4088-b5ac-c5d8940a86ee
⛔ Files ignored due to path filters (2)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (12)
api/v1alpha1/imagebuild_types.goapi/v1alpha1/operatorconfig_types.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/image/image.gocmd/caib/main.gocmd/caib/runtime_wiring.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_test.gointernal/controller/imagebuild/controller.gointernal/controller/operatorconfig/controller.go
✅ Files skipped from review due to trivial changes (2)
- cmd/caib/buildcmd/build_disk_test.go
- internal/controller/operatorconfig/controller.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/caib/main.go
- internal/buildapi/types.go
- cmd/caib/runtime_wiring.go
- internal/controller/imagebuild/controller.go
- api/v1alpha1/imagebuild_types.go
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
.github/workflows/build.yml (1)
169-176:⚠️ Potential issue | 🟡 MinorMake the exported-task glob null-safe again.
If
_output/tasksis empty, this loop adds a literal-f _output/tasks/*.yamlargument andtkn bundle pushfails with a confusing error instead of surfacing the missing export.Suggested fix
- bundle_args=() - for f in _output/tasks/*.yaml; do + shopt -s nullglob + bundle_args=() + for f in _output/tasks/*.yaml; do bundle_args+=(-f "$f") done + if [ ${`#bundle_args`[@]} -eq 0 ]; then + echo "No exported task YAMLs found under _output/tasks" >&2 + exit 1 + fi tkn bundle push "${{ env.TEKTON_BUNDLE_IMAGE }}:${{ env.VERSION }}" "${bundle_args[@]}" tkn bundle push "${{ env.TEKTON_BUNDLE_IMAGE }}:latest" "${bundle_args[@]}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 169 - 176, The loop that builds bundle_args (for f in _output/tasks/*.yaml) can pass the literal pattern when there are no matches; make the glob null-safe by enabling nullglob (shopt -s nullglob) before the loop (and restore it afterward) or explicitly check for existence (e.g., if compgen -G "_output/tasks/*.yaml" >/dev/null) and fail early with a clear message; ensure this change affects the "Push Tekton Bundle" step and the bundle_args accumulation so tkn bundle push is only called when real files were found.internal/common/tasks/pipeline_test.go (1)
264-270:⚠️ Potential issue | 🟡 MinorGuard the malformed-line case before indexing
parts[1].If a future fixture lacks
@, Line 269 still dereferencesparts[1]after thet.Errorf, so the test panics instead of failing cleanly. Add acontinueafter the separator check;strings.Count(line, "@") != 1would also match the assertion message.Suggested change
- parts := strings.SplitN(line, "@", 2) - if len(parts) != 2 { + if strings.Count(line, "@") != 1 { t.Errorf("line %q should contain exactly one '@' separator", line) + continue } + parts := strings.SplitN(line, "@", 2) if !strings.HasPrefix(parts[1], "sha256:") { t.Errorf("digest %q should start with sha256:", parts[1]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/pipeline_test.go` around lines 264 - 270, The test loop over lines uses strings.SplitN(line, "@", 2) and then indexes parts[1] even when the separator check fails; modify the malformed-line branch in the loop (the t.Errorf that checks len(parts) != 2) to either pre-check with strings.Count(line, "@") != 1 or, more simply, add a continue immediately after the t.Errorf so the code does not dereference parts[1]; update the loop that references parts, parts[1], strings.SplitN, and t.Errorf accordingly.
🧹 Nitpick comments (3)
internal/common/tasks/scripts/build_image.sh (1)
764-764: Consider using Bash parameter expansion instead ofsed.Per Shellcheck SC2001, the
sedsubstitution can be replaced with a simpler Bash construct.♻️ Suggested change
- boot_b_file=$(echo "$boot_a_file" | sed 's/boot_a/boot_b/') + boot_b_file="${boot_a_file/boot_a/boot_b}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/build_image.sh` at line 764, Replace the sed-based substitution used when building boot_b_file from boot_a_file with Bash parameter expansion: instead of invoking sed on boot_a_file, assign boot_b_file using Bash's string-replacement expansion to substitute the substring "boot_a" with "boot_b" directly on the variable; update the assignment where boot_b_file is set so it uses parameter expansion on boot_a_file (referencing the boot_b_file and boot_a_file variables and the existing substitution intent).internal/buildapi/server.go (1)
1948-1950: HardentaskBundleRefvalidation with stricter digest matching.The current
strings.Contains(operatorConfig.Spec.OSBuilds.TaskBundleRef, "@sha256:")check is permissive and accepts malformed refs likefoo@sha256:(missing actual digest) or@sha256:abc(short digest). For security-critical secure build functionality, enforcing the full 64-character hex digest format would be more robust.♻️ Suggested stricter validation
+import "regexp" + +var digestPinnedRefRegex = regexp.MustCompile(`^.+@sha256:[a-fA-F0-9]{64}$`) + func validateSecureBuild(ctx context.Context, k8sClient client.Client, namespace string) error { operatorConfig := &automotivev1alpha1.OperatorConfig{} if err := k8sClient.Get(ctx, types.NamespacedName{Name: "config", Namespace: namespace}, operatorConfig); err != nil { return fmt.Errorf("secureBuild requested but OperatorConfig could not be read") } if operatorConfig.Spec.OSBuilds == nil || operatorConfig.Spec.OSBuilds.TaskBundleRef == "" { return fmt.Errorf("secureBuild requested but OperatorConfig.spec.osBuilds.taskBundleRef is not set") } - if !strings.Contains(operatorConfig.Spec.OSBuilds.TaskBundleRef, "@sha256:") { + ref := strings.TrimSpace(operatorConfig.Spec.OSBuilds.TaskBundleRef) + if !digestPinnedRefRegex.MatchString(ref) { return fmt.Errorf("secureBuild requires a digest-pinned taskBundleRef (must contain `@sha256`:), got %q", operatorConfig.Spec.OSBuilds.TaskBundleRef) } return nil }Note: The same pattern exists in
internal/controller/imagebuild/controller.go:555, so consider updating both locations for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/buildapi/server.go` around lines 1948 - 1950, Replace the permissive contains check on operatorConfig.Spec.OSBuilds.TaskBundleRef with a strict regex match that enforces a full 64‑character hex digest (e.g. require "@sha256:" followed by exactly 64 hex chars); update the validation where the snippet appears (checking operatorConfig.Spec.OSBuilds.TaskBundleRef) to use regexp.MatchString or compiled regexp like ^.*@sha256:[A-Fa-f0-9]{64}$ and return the same formatted error if the match fails; apply the same change in the corresponding check in internal/controller/imagebuild/controller.go for consistency.internal/controller/imagebuild/controller.go (1)
1955-1982: Upload pod still misses the build pod'sRuntimeClassName.This copies node selector, tolerations, and arch affinity, but
createBuildTaskRun()also appliesRuntimeClassNamefromOperatorConfig.spec.osBuilds/ImageBuild.spec.runtimeClassName. If that runtime class adds scheduling constraints, the upload pod can still become the PVC's first consumer on nodes the build pod cannot use.Suggested change
if operatorConfig.Spec.OSBuilds != nil && len(operatorConfig.Spec.OSBuilds.Tolerations) > 0 { pod.Spec.Tolerations = operatorConfig.Spec.OSBuilds.Tolerations } +if operatorConfig.Spec.OSBuilds != nil && operatorConfig.Spec.OSBuilds.RuntimeClassName != "" { + pod.Spec.RuntimeClassName = &operatorConfig.Spec.OSBuilds.RuntimeClassName +} +if imageBuild.Spec.RuntimeClassName != "" { + pod.Spec.RuntimeClassName = &imageBuild.Spec.RuntimeClassName +} if imageBuild.Spec.Architecture != "" { pod.Spec.Affinity = &corev1.Affinity{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 1955 - 1982, The upload pod setup omits copying the RuntimeClassName causing a mismatch with createBuildTaskRun(); update the same block to set pod.Spec.RuntimeClassName (a *string) by applying ImageBuild.spec.runtimeClassName if present, otherwise falling back to OperatorConfig.spec.osBuilds.runtimeClassName (the same precedence used in createBuildTaskRun()), ensuring the upload pod uses the same RuntimeClassName as the build task.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/export-tasks/main.go`:
- Around line 38-46: The exported bundle currently constructs tasks with
nil/empty BuildConfig (calls to GenerateBuildAutomotiveImageTask("", nil, ""),
GeneratePushArtifactRegistryTask("", nil), GeneratePrepareBuilderTask("", nil),
GenerateFlashTask("", nil), and GenerateSealedTasks("")) which drops
TrustedCABundleKind/TrustedCABundleName from sealedTaskSpec; change this to pass
the resolved BuildConfig used by the operator into those task generators
(replace nil/"" with the actual BuildConfig variable) and ensure the call to
GenerateSealedTasks forwards that same BuildConfig so sealedTaskSpec preserves
TrustedCABundleKind/TrustedCABundleName.
In `@internal/common/tasks/tasks.go`:
- Around line 1326-1373: The collect-images-result task can read stale files
from a reused shared workspace (workspaceNameShared) and emit previous
image@digest; modify the embedded TaskSpec for "collect-images-result" (the Step
named "collect") to clear the chains directory at start (e.g., run rm -rf
"$CHAINS_DIR"/* or remove the directory and recreate it) before reading files,
or instead scope the chains path per PipelineRun (e.g., include a
PipelineRun-unique env var in CHAINS_DIR such as TEKTON_PIPELINE_RUN_NAME) so
createBuildTaskRun that may bind BuildCachePVC cannot leak prior runs'
/workspace/shared/.chains into IMAGES. Ensure the change is made in the Script
of the collect step in the collect-images-result TaskSpec.
In `@internal/controller/imagebuild/controller.go`:
- Around line 530-533: When aborting PipelineRun creation for secure-build
misconfigurations (e.g., the branch checking imageBuild.Spec.SecureBuild && (err
!= nil || operatorConfig.Spec.OSBuilds == nil) and the similar branch around
taskBundleRef), set a terminal status on the ImageBuild before returning instead
of only returning an error: populate the ImageBuild.Status (e.g., State/Phase =
"Failed" and Status.Message with the specific error like missing operatorConfig,
missing spec.osBuilds or invalid taskBundleRef), persist it via the controller's
status update method (use the existing Status().Update/patch helper used
elsewhere in this controller) and then return the error; do this for both the
block referencing imageBuild.Spec.SecureBuild/operatorConfig.Spec.OSBuilds and
the block referencing taskBundleRef so the CR clearly reflects the failure.
---
Duplicate comments:
In @.github/workflows/build.yml:
- Around line 169-176: The loop that builds bundle_args (for f in
_output/tasks/*.yaml) can pass the literal pattern when there are no matches;
make the glob null-safe by enabling nullglob (shopt -s nullglob) before the loop
(and restore it afterward) or explicitly check for existence (e.g., if compgen
-G "_output/tasks/*.yaml" >/dev/null) and fail early with a clear message;
ensure this change affects the "Push Tekton Bundle" step and the bundle_args
accumulation so tkn bundle push is only called when real files were found.
In `@internal/common/tasks/pipeline_test.go`:
- Around line 264-270: The test loop over lines uses strings.SplitN(line, "@",
2) and then indexes parts[1] even when the separator check fails; modify the
malformed-line branch in the loop (the t.Errorf that checks len(parts) != 2) to
either pre-check with strings.Count(line, "@") != 1 or, more simply, add a
continue immediately after the t.Errorf so the code does not dereference
parts[1]; update the loop that references parts, parts[1], strings.SplitN, and
t.Errorf accordingly.
---
Nitpick comments:
In `@internal/buildapi/server.go`:
- Around line 1948-1950: Replace the permissive contains check on
operatorConfig.Spec.OSBuilds.TaskBundleRef with a strict regex match that
enforces a full 64‑character hex digest (e.g. require "@sha256:" followed by
exactly 64 hex chars); update the validation where the snippet appears (checking
operatorConfig.Spec.OSBuilds.TaskBundleRef) to use regexp.MatchString or
compiled regexp like ^.*@sha256:[A-Fa-f0-9]{64}$ and return the same formatted
error if the match fails; apply the same change in the corresponding check in
internal/controller/imagebuild/controller.go for consistency.
In `@internal/common/tasks/scripts/build_image.sh`:
- Line 764: Replace the sed-based substitution used when building boot_b_file
from boot_a_file with Bash parameter expansion: instead of invoking sed on
boot_a_file, assign boot_b_file using Bash's string-replacement expansion to
substitute the substring "boot_a" with "boot_b" directly on the variable; update
the assignment where boot_b_file is set so it uses parameter expansion on
boot_a_file (referencing the boot_b_file and boot_a_file variables and the
existing substitution intent).
In `@internal/controller/imagebuild/controller.go`:
- Around line 1955-1982: The upload pod setup omits copying the RuntimeClassName
causing a mismatch with createBuildTaskRun(); update the same block to set
pod.Spec.RuntimeClassName (a *string) by applying
ImageBuild.spec.runtimeClassName if present, otherwise falling back to
OperatorConfig.spec.osBuilds.runtimeClassName (the same precedence used in
createBuildTaskRun()), ensuring the upload pod uses the same RuntimeClassName as
the build task.
🪄 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: 2e4c601f-5848-4f33-a5f1-f8c620135454
⛔ Files ignored due to path filters (2)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (20)
.github/workflows/build.yml.gitignoreMakefileapi/v1alpha1/imagebuild_types.goapi/v1alpha1/operatorconfig_types.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/image/image.gocmd/caib/main.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_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.go
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- internal/controller/operatorconfig/controller.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/caib/buildcmd/build_disk_test.go
- api/v1alpha1/imagebuild_types.go
- cmd/caib/main.go
- internal/common/tasks/scripts/common.sh
- Makefile
- cmd/caib/runtime_wiring.go
Comment on lines
+38
to
+46
| // Use nil buildConfig for defaults — bundle tasks should not bake in | ||
| // cluster-specific settings like memory volumes or custom timeouts. | ||
| taskList := []*tektonv1.Task{ | ||
| tasks.GenerateBuildAutomotiveImageTask("", nil, ""), | ||
| tasks.GeneratePushArtifactRegistryTask("", nil), | ||
| tasks.GeneratePrepareBuilderTask("", nil), | ||
| tasks.GenerateFlashTask("", nil), | ||
| } | ||
| taskList = append(taskList, tasks.GenerateSealedTasks("")...) |
There was a problem hiding this comment.
Bundle export diverges from the operator’s actual task specs.
These tasks are exported with nil buildConfig, but the operator deploy path in internal/controller/operatorconfig/controller.go:656-662 generates the same tasks with the resolved buildConfig. That means secure builds can resolve a different task spec than the cluster is configured to run whenever OperatorConfig customizes task generation. At minimum, the exported sealed tasks lose the trusted CA bundle wiring here.
Based on learnings, sealedTaskSpec only uses TrustedCABundleKind and TrustedCABundleName from BuildConfig, so omitting buildConfig here drops that configuration from exported sealed tasks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/export-tasks/main.go` around lines 38 - 46, The exported bundle currently
constructs tasks with nil/empty BuildConfig (calls to
GenerateBuildAutomotiveImageTask("", nil, ""),
GeneratePushArtifactRegistryTask("", nil), GeneratePrepareBuilderTask("", nil),
GenerateFlashTask("", nil), and GenerateSealedTasks("")) which drops
TrustedCABundleKind/TrustedCABundleName from sealedTaskSpec; change this to pass
the resolved BuildConfig used by the operator into those task generators
(replace nil/"" with the actual BuildConfig variable) and ensure the call to
GenerateSealedTasks forwards that same BuildConfig so sealedTaskSpec preserves
TrustedCABundleKind/TrustedCABundleName.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/common/tasks/scripts/push_artifact.sh (1)
197-216:⚠️ Potential issue | 🟠 MajorHash the normalized artifact, not the pre-normalization layout.
This verification runs before Lines 230-243 add
boot_bforride4*|ridesx4*. That lets the integrity check pass even though the artifact that actually gets pushed contains an extra layer and no longer matchesEXPECTED_DIGEST.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/push_artifact.sh` around lines 197 - 216, The integrity check currently computes ACTUAL_DIGEST via compute_artifact_digest("${parts_dir}" "${exportFile}") before the script appends the boot_b layer for ride4*|ridesx4*, so the digest can differ from the final pushed artifact; move or redo the digest calculation so it runs after the artifact normalization step (i.e., after the code that adds boot_b for ride4*/ridesx4*) or change compute_artifact_digest to operate on the normalized/exported artifact state instead of the pre-normalization layout; ensure ACTUAL_DIGEST is obtained from the exact artifact that will be pushed (using the same parts_dir/exportFile contents post-normalization) before comparing to EXPECTED_DIGEST.internal/common/tasks/tasks.go (2)
1341-1364:⚠️ Potential issue | 🔴 CriticalUse a run-scoped Chains directory instead of a fixed
.chainspath.This script reads fixed files under
/workspace/shared/.chainsand emits them if present. On any reused shared workspace, a run that skips one of the pushes can inherit oldurl/digestfiles and publish a previous build’simage@digestinIMAGES. ScopeCHAINS_DIRby PipelineRun identity and update the writer scripts to use the same path, or clear the current run’s Chains directory before any writer task executes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/tasks.go` around lines 1341 - 1364, The script uses a fixed CHAINS_DIR="/workspace/shared/.chains" which can cause stale url/digest files to leak between runs; change CHAINS_DIR to be run-scoped (e.g., include the PipelineRun ID or run UID) and ensure all writer scripts that create container/disk url and digest use the same run-scoped path (or alternately add an initialization step that atomically clears/creates the run-scoped chains directory before any writer task runs); update the CHAINS_DIR variable in the reader script and the corresponding writer scripts/functions that write container/url, container/digest, disk/url, disk/digest so they reference the identical run-scoped directory (use the same pipeline/run identifier symbol your pipeline exposes).
47-87:⚠️ Potential issue | 🟠 MajorFail fast when bundle mode has no bundle ref.
If
TaskResolverisbundlebutTaskBundleRefis empty, this helper silently falls back to the cluster resolver. That turns a secure-build misconfiguration into execution of cluster-installed tasks instead of the signed bundle you intended. Please reject that state explicitly instead of downgrading it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/tasks.go` around lines 47 - 87, The helper buildTaskRef currently falls back to the cluster resolver when BuildConfig.TaskResolver == TaskResolverBundle but BuildConfig.TaskBundleRef is empty; change buildTaskRef to explicitly reject that misconfiguration by returning an error instead of silently downgrading. Update the signature of buildTaskRef to return (*tektonv1.TaskRef, error), check if buildConfig != nil && buildConfig.TaskResolver == TaskResolverBundle && buildConfig.TaskBundleRef == "" and return a descriptive error (mentioning BuildConfig.TaskBundleRef missing) in that branch; otherwise construct and return the appropriate *tektonv1.TaskRef and nil error. Propagate the signature change to all callers so they handle the error (e.g., bubble up or fail the request).
🧹 Nitpick comments (2)
internal/common/tasks/scripts/build_image.sh (1)
767-767: Consider using parameter expansion instead ofsedfor simple substitution.Per shellcheck (SC2001), you can use bash parameter expansion for this substitution:
- boot_b_file=$(echo "$boot_a_file" | sed 's/boot_a/boot_b/') + boot_b_file="${boot_a_file/boot_a/boot_b}"This is a minor style improvement that avoids spawning a subprocess.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/build_image.sh` at line 767, Replace the sed substitution that computes boot_b_file from boot_a_file with Bash parameter expansion: where the script currently uses boot_b_file=$(echo "$boot_a_file" | sed 's/boot_a/boot_b/'), change it to use the shell's string replacement on the variable (operate on boot_a_file to produce boot_b_file using ${boot_a_file/.../...}) so the code avoids spawning sed and satisfies SC2001; update any references to boot_b_file accordingly.internal/controller/imagebuild/controller.go (1)
511-520: Stop keying terminal-state handling off the error text.
strings.Contains(err.Error(), "secureBuild")is brittle here. A wording change increateBuildTaskRun()can turn the same configuration error back into a retry loop. Prefer a sentinel/wrapped error anderrors.Is(...)for the terminal secure-build path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 511 - 520, Replace the brittle string check with a sentinel/wrapped error: add a package-level sentinel error (e.g., ErrSecureBuildValidation) and make createBuildTaskRun return that error (or wrap it) when the secure-build validation fails; then in the reconciler use errors.Is(err, ErrSecureBuildValidation) instead of strings.Contains(err.Error(), "secureBuild"); ensure the createBuildTaskRun call sites wrap with fmt.Errorf("%w: details", ErrSecureBuildValidation) and import "errors" where you perform the Is check so terminal handling reliably detects the secure-build validation path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/buildapi/server.go`:
- Around line 1944-1947: The validateSecureBuild function currently returns a
generic client-side error when k8sClient.Get fails reading OperatorConfig;
change the Get error handling in validateSecureBuild to return a wrapped error
that includes the underlying k8s error (e.g., fmt.Errorf("could not read
OperatorConfig: %w", err)) so callers can detect it as a server/cluster read
failure and surface a 5xx (not a 400); do the same adjustment for the similar
block referenced around lines 2010-2013 so only missing/invalid taskBundleRef
remains a 400 while unreadable cluster-state errors include the original err and
propagate as 5xx.
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 431-436: The fallback that re-fetches the manifest by mutable tag
is unsafe: if DISK_DIGEST (parsed from /tmp/oras-push-output.txt) is empty,
remove the block that runs "$HOME/bin/oras" manifest fetch --descriptor
"${repo_url}" and instead treat a missing DISK_DIGEST as a hard failure (exit
non‑zero with an error) or, better, change the push step to produce/capture the
immutable digest directly (ensure the oras push invocation writes a stable
digest to /tmp/oras-push-output.txt or use oras’s machine-readable/json output)
so DISK_DIGEST is never recovered by re-reading the mutable tag.
---
Duplicate comments:
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 197-216: The integrity check currently computes ACTUAL_DIGEST via
compute_artifact_digest("${parts_dir}" "${exportFile}") before the script
appends the boot_b layer for ride4*|ridesx4*, so the digest can differ from the
final pushed artifact; move or redo the digest calculation so it runs after the
artifact normalization step (i.e., after the code that adds boot_b for
ride4*/ridesx4*) or change compute_artifact_digest to operate on the
normalized/exported artifact state instead of the pre-normalization layout;
ensure ACTUAL_DIGEST is obtained from the exact artifact that will be pushed
(using the same parts_dir/exportFile contents post-normalization) before
comparing to EXPECTED_DIGEST.
In `@internal/common/tasks/tasks.go`:
- Around line 1341-1364: The script uses a fixed
CHAINS_DIR="/workspace/shared/.chains" which can cause stale url/digest files to
leak between runs; change CHAINS_DIR to be run-scoped (e.g., include the
PipelineRun ID or run UID) and ensure all writer scripts that create
container/disk url and digest use the same run-scoped path (or alternately add
an initialization step that atomically clears/creates the run-scoped chains
directory before any writer task runs); update the CHAINS_DIR variable in the
reader script and the corresponding writer scripts/functions that write
container/url, container/digest, disk/url, disk/digest so they reference the
identical run-scoped directory (use the same pipeline/run identifier symbol your
pipeline exposes).
- Around line 47-87: The helper buildTaskRef currently falls back to the cluster
resolver when BuildConfig.TaskResolver == TaskResolverBundle but
BuildConfig.TaskBundleRef is empty; change buildTaskRef to explicitly reject
that misconfiguration by returning an error instead of silently downgrading.
Update the signature of buildTaskRef to return (*tektonv1.TaskRef, error), check
if buildConfig != nil && buildConfig.TaskResolver == TaskResolverBundle &&
buildConfig.TaskBundleRef == "" and return a descriptive error (mentioning
BuildConfig.TaskBundleRef missing) in that branch; otherwise construct and
return the appropriate *tektonv1.TaskRef and nil error. Propagate the signature
change to all callers so they handle the error (e.g., bubble up or fail the
request).
---
Nitpick comments:
In `@internal/common/tasks/scripts/build_image.sh`:
- Line 767: Replace the sed substitution that computes boot_b_file from
boot_a_file with Bash parameter expansion: where the script currently uses
boot_b_file=$(echo "$boot_a_file" | sed 's/boot_a/boot_b/'), change it to use
the shell's string replacement on the variable (operate on boot_a_file to
produce boot_b_file using ${boot_a_file/.../...}) so the code avoids spawning
sed and satisfies SC2001; update any references to boot_b_file accordingly.
In `@internal/controller/imagebuild/controller.go`:
- Around line 511-520: Replace the brittle string check with a sentinel/wrapped
error: add a package-level sentinel error (e.g., ErrSecureBuildValidation) and
make createBuildTaskRun return that error (or wrap it) when the secure-build
validation fails; then in the reconciler use errors.Is(err,
ErrSecureBuildValidation) instead of strings.Contains(err.Error(),
"secureBuild"); ensure the createBuildTaskRun call sites wrap with
fmt.Errorf("%w: details", ErrSecureBuildValidation) and import "errors" where
you perform the Is check so terminal handling reliably detects the secure-build
validation path.
🪄 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: 4d504300-0050-40c9-9335-76f5396f144f
⛔ Files ignored due to path filters (2)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (20)
.github/workflows/build.yml.gitignoreMakefileapi/v1alpha1/imagebuild_types.goapi/v1alpha1/operatorconfig_types.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/image/image.gocmd/caib/main.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_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.go
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- internal/buildapi/types.go
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/caib/main.go
- api/v1alpha1/imagebuild_types.go
- cmd/caib/runtime_wiring.go
- cmd/export-tasks/main.go
- Makefile
- internal/common/tasks/scripts/common.sh
- .github/workflows/build.yml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
api/v1alpha1/operatorconfig_types.go (1)
539-545:⚠️ Potential issue | 🟠 MajorRequire digest-pinned bundle refs in the API.
TaskBundleRefstill accepts mutable tags, so a later retag can change the resolved task payload without anysecureBuildspec change. Please add CRD/webhook validation that only allows@sha256:-pinned refs for this path, or otherwise reject mutable refs when secure builds depend on them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1alpha1/operatorconfig_types.go` around lines 539 - 545, TaskBundleRef currently allows mutable image tags; enforce digest-pinned refs by adding validation that only accepts OCI references containing an `@sha256`: digest. Update the TaskBundleRef field on the OperatorConfig type (TaskBundleRef string) to include either a kubebuilder CRD validation pattern (e.g. a regex matching ".*@sha256:[a-fA-F0-9]{64}$") or implement the corresponding webhook validation in the OperatorConfig admission logic to reject values that do not match that digest pattern, returning a clear validation error when SecureBuild depends on the field.internal/common/tasks/scripts/push_artifact.sh (1)
197-243:⚠️ Potential issue | 🟠 MajorVerify the post-normalization artifact and fail closed.
ACTUAL_DIGESTis still computed before theboot_a→boot_bfallback, and an empty digest only logs a warning. Forride4*|ridesx4*, that means this task can validate one layout and then push a different one; and if hashing fails entirely, the integrity check is silently bypassed even thoughEXPECTED_DIGESTwas provided. Move the digest calculation after the target-specific normalization and treat a missingACTUAL_DIGESTas an error when verification is requested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/push_artifact.sh` around lines 197 - 243, The script currently computes ACTUAL_DIGEST (via compute_artifact_digest "${parts_dir}" "${exportFile}") before the target-specific normalization that duplicates boot_a→boot_b for ride4*/ridesx4*, and it only warns when ACTUAL_DIGEST is empty; move the EXPECTED_DIGEST/ACTUAL_DIGEST verification block so ACTUAL_DIGEST is computed after the case block that handles boot_a/boot_b normalization (referencing variables parts_dir, exportFile, target and function compute_artifact_digest), and when EXPECTED_DIGEST is provided treat a missing/empty ACTUAL_DIGEST as a fatal error (exit non-zero) rather than a warning so the task fails closed if hashing cannot be performed.internal/common/tasks/tasks.go (1)
47-88:⚠️ Potential issue | 🟠 MajorDon’t silently downgrade bundle mode to cluster tasks.
If
TaskResolveris"bundle"butTaskBundleRefis empty, this helper falls back to the cluster resolver. That turns a secure-build misconfiguration into unsigned cluster-task execution instead of an explicit failure. Please validate this state before callingbuildTaskRef, or change the helper to surface an error rather than defaulting here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/tasks.go` around lines 47 - 88, The helper buildTaskRef currently silently falls back to cluster resolver when BuildConfig.TaskResolver == TaskResolverBundle but BuildConfig.TaskBundleRef is empty; change buildTaskRef to validate that case and surface an explicit error instead of defaulting: update the signature to return (*tektonv1.TaskRef, error), check if buildConfig != nil && buildConfig.TaskResolver == TaskResolverBundle && buildConfig.TaskBundleRef == "" and return a descriptive error, otherwise construct and return the TaskRef as before; update all callers of buildTaskRef to handle the error accordingly. Include references to buildTaskRef, BuildConfig.TaskResolver, BuildConfig.TaskBundleRef and TaskResolverBundle in your changes.
🧹 Nitpick comments (1)
internal/controller/imagebuild/controller.go (1)
511-520: Use a sentinel error instead of checking error message text for terminal behavior.
strings.Contains(err.Error(), "secureBuild")at line 514 makes the Failed-vs-retry decision fragile: any future secure-build config error that omits the exact "secureBuild" token will silently regress to the retry path. All three current secure-build validation errors (lines 545, 567, 570) include the token, but the pattern depends on exact wording rather than error type.Define a sentinel error and wrap secure-build validation failures with it:
♻️ Suggested direction
+var errSecureBuildConfig = errors.New("secureBuild configuration error") + func (r *ImageBuildReconciler) startNewBuild( ctx context.Context, imageBuild *automotivev1alpha1.ImageBuild, ) (ctrl.Result, error) { if err := r.createBuildTaskRun(ctx, imageBuild); err != nil { - if strings.Contains(err.Error(), "secureBuild") { + if errors.Is(err, errSecureBuildConfig) { msg := fmt.Sprintf("Build configuration error: %v", err) if statusErr := r.updateStatus(ctx, imageBuild, phaseFailed, msg); statusErr != nil { return ctrl.Result{}, statusErr } return ctrl.Result{}, nil }Then wrap terminal secure-build validation at lines 545, 567, 570:
- return fmt.Errorf("secureBuild requested but OperatorConfig or spec.osBuilds is not available") + return fmt.Errorf("%w: OperatorConfig or spec.osBuilds is not available", errSecureBuildConfig)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 511 - 520, Replace the fragile text-match check for secure-build errors with a sentinel error: declare a package-level sentinel (e.g. var errSecureBuildValidation = errors.New("secureBuildValidation")) and import the errors package; when the secure-build validation fails (the validation sites inside createBuildTaskRun / the secure-build validation helpers currently emitting errors that contain "secureBuild"), wrap those original errors with fmt.Errorf("%w: %v", errSecureBuildValidation, originalErr) so the terminal nature is preserved; then change the reconcile branch that currently does strings.Contains(err.Error(), "secureBuild") to use errors.Is(err, errSecureBuildValidation) to detect terminal secure-build validation failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/buildapi/server.go`:
- Around line 2010-2016: The preflight currently validates
OperatorConfig.spec.osBuilds.taskBundleRef but only sets
ImageBuild.SecureBuild=true, which allows a race where OperatorConfig changes
before reconciliation; modify the request handling so that when req.SecureBuild
is true you call validateSecureBuild(ctx, k8sClient, namespace) and capture the
resolved taskBundleRef/digest it returns, then persist that resolved immutable
value on the ImageBuild object (e.g., add/set ImageBuild.Status or Spec field
like ResolvedTaskBundleRef or TaskBundleDigest) instead of just SecureBuild;
ensure the controller that creates the PipelineRun reads that persisted
ImageBuild field (not OperatorConfig) so the reconciled PipelineRun uses the
snapshot digest.
---
Duplicate comments:
In `@api/v1alpha1/operatorconfig_types.go`:
- Around line 539-545: TaskBundleRef currently allows mutable image tags;
enforce digest-pinned refs by adding validation that only accepts OCI references
containing an `@sha256`: digest. Update the TaskBundleRef field on the
OperatorConfig type (TaskBundleRef string) to include either a kubebuilder CRD
validation pattern (e.g. a regex matching ".*@sha256:[a-fA-F0-9]{64}$") or
implement the corresponding webhook validation in the OperatorConfig admission
logic to reject values that do not match that digest pattern, returning a clear
validation error when SecureBuild depends on the field.
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 197-243: The script currently computes ACTUAL_DIGEST (via
compute_artifact_digest "${parts_dir}" "${exportFile}") before the
target-specific normalization that duplicates boot_a→boot_b for ride4*/ridesx4*,
and it only warns when ACTUAL_DIGEST is empty; move the
EXPECTED_DIGEST/ACTUAL_DIGEST verification block so ACTUAL_DIGEST is computed
after the case block that handles boot_a/boot_b normalization (referencing
variables parts_dir, exportFile, target and function compute_artifact_digest),
and when EXPECTED_DIGEST is provided treat a missing/empty ACTUAL_DIGEST as a
fatal error (exit non-zero) rather than a warning so the task fails closed if
hashing cannot be performed.
In `@internal/common/tasks/tasks.go`:
- Around line 47-88: The helper buildTaskRef currently silently falls back to
cluster resolver when BuildConfig.TaskResolver == TaskResolverBundle but
BuildConfig.TaskBundleRef is empty; change buildTaskRef to validate that case
and surface an explicit error instead of defaulting: update the signature to
return (*tektonv1.TaskRef, error), check if buildConfig != nil &&
buildConfig.TaskResolver == TaskResolverBundle && buildConfig.TaskBundleRef ==
"" and return a descriptive error, otherwise construct and return the TaskRef as
before; update all callers of buildTaskRef to handle the error accordingly.
Include references to buildTaskRef, BuildConfig.TaskResolver,
BuildConfig.TaskBundleRef and TaskResolverBundle in your changes.
---
Nitpick comments:
In `@internal/controller/imagebuild/controller.go`:
- Around line 511-520: Replace the fragile text-match check for secure-build
errors with a sentinel error: declare a package-level sentinel (e.g. var
errSecureBuildValidation = errors.New("secureBuildValidation")) and import the
errors package; when the secure-build validation fails (the validation sites
inside createBuildTaskRun / the secure-build validation helpers currently
emitting errors that contain "secureBuild"), wrap those original errors with
fmt.Errorf("%w: %v", errSecureBuildValidation, originalErr) so the terminal
nature is preserved; then change the reconcile branch that currently does
strings.Contains(err.Error(), "secureBuild") to use errors.Is(err,
errSecureBuildValidation) to detect terminal secure-build validation failures.
🪄 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: 6862f016-189d-4103-96ca-b3fdc00c4dc0
⛔ Files ignored due to path filters (2)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (20)
.github/workflows/build.yml.gitignoreMakefileapi/v1alpha1/imagebuild_types.goapi/v1alpha1/operatorconfig_types.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/image/image.gocmd/caib/main.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_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.go
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- cmd/caib/main.go
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/caib/buildcmd/build_disk_test.go
- cmd/caib/runtime_wiring.go
- cmd/caib/image/image.go
- internal/common/tasks/scripts/common.sh
- cmd/export-tasks/main.go
- Makefile
- internal/common/tasks/pipeline_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/common/tasks/scripts/push_artifact.sh (1)
197-216:⚠️ Potential issue | 🟠 MajorVerify the artifact after the
ride4*/ridesx4*fallback mutatesparts_dir.
ACTUAL_DIGESTis computed before the laterboot_a→boot_bduplication. If that fallback runs, the uploaded artifact layout no longer matches what was verified, which breaks the build→push integrity chain for those targets. Move the digest calculation to after the normalization block.Suggested fix
-# Verify artifact integrity against the digest produced by the build task. -EXPECTED_DIGEST="$(params.expected-artifact-digest)" -if [ -n "$EXPECTED_DIGEST" ]; then - echo "=== Artifact Integrity Verification ===" - ACTUAL_DIGEST=$(compute_artifact_digest "${parts_dir}" "${exportFile}") - if [ -z "$ACTUAL_DIGEST" ]; then - echo "WARNING: Cannot verify integrity — artifact not found yet" - fi - if [ -n "$ACTUAL_DIGEST" ]; then - if [ "$EXPECTED_DIGEST" != "$ACTUAL_DIGEST" ]; then - echo "ERROR: Artifact integrity check failed!" >&2 - echo " Expected: $EXPECTED_DIGEST" >&2 - echo " Actual: $ACTUAL_DIGEST" >&2 - exit 1 - fi - echo " Integrity verified: $ACTUAL_DIGEST" - fi -else - echo "No artifact integrity digest provided, skipping verification" -fi - echo "=== Artifact Push Configuration ===" @@ case "$target" in ride4*|ridesx4*) for boot_a_file in "${parts_dir}"/boot_a.*; do [ -f "$boot_a_file" ] || continue boot_b_file=$(echo "$boot_a_file" | sed 's/boot_a/boot_b/') if [ ! -f "$boot_b_file" ]; then echo "Duplicating $(basename "$boot_a_file") as $(basename "$boot_b_file") for target $target" cp "$boot_a_file" "$boot_b_file" fi done ;; esac + +EXPECTED_DIGEST="$(params.expected-artifact-digest)" +if [ -n "$EXPECTED_DIGEST" ]; then + echo "=== Artifact Integrity Verification ===" + ACTUAL_DIGEST=$(compute_artifact_digest "${parts_dir}" "${exportFile}") + if [ -z "$ACTUAL_DIGEST" ]; then + echo "WARNING: Cannot verify integrity — artifact not found yet" + elif [ "$EXPECTED_DIGEST" != "$ACTUAL_DIGEST" ]; then + echo "ERROR: Artifact integrity check failed!" >&2 + echo " Expected: $EXPECTED_DIGEST" >&2 + echo " Actual: $ACTUAL_DIGEST" >&2 + exit 1 + else + echo " Integrity verified: $ACTUAL_DIGEST" + fi +else + echo "No artifact integrity digest provided, skipping verification" +fiAlso applies to: 230-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/push_artifact.sh` around lines 197 - 216, The script computes ACTUAL_DIGEST with compute_artifact_digest using parts_dir and exportFile before the fallback that normalizes ride4*/ridesx4* by duplicating boot_a → boot_b; if that fallback runs the layout changes and the prior digest is invalid. Move the ACTUAL_DIGEST calculation (the compute_artifact_digest call and any checks that reference ACTUAL_DIGEST) to after the normalization/boot_a→boot_b duplication block so the digest is computed against the final parts_dir layout; apply the same change for the duplicate section around the other occurrence noted (the block covering the later lines).
🧹 Nitpick comments (2)
.github/workflows/build.yml (1)
169-176: Add guard for missing task files.If
make export-tasksfails or produces no output, the glob_output/tasks/*.yamlwon't match, leavingbundle_argsempty. Thetkn bundle pushwould then fail cryptically or push an empty bundle.🛡️ Suggested guard
- name: Push Tekton Bundle run: | + shopt -s nullglob bundle_args=() for f in _output/tasks/*.yaml; do bundle_args+=(-f "$f") done + if [ ${`#bundle_args`[@]} -eq 0 ]; then + echo "No task YAMLs found in _output/tasks" >&2 + exit 1 + fi tkn bundle push "${{ env.TEKTON_BUNDLE_IMAGE }}:${{ env.VERSION }}" "${bundle_args[@]}" tkn bundle push "${{ env.TEKTON_BUNDLE_IMAGE }}:latest" "${bundle_args[@]}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 169 - 176, The push step currently builds bundle_args from the glob _output/tasks/*.yaml and calls tkn bundle push even when no files match; add a guard that verifies there is at least one matching file (e.g. check bundle_args array length or test for any _output/tasks/*.yaml) before running tkn bundle push, and if none exist emit a clear message and skip or exit non-zero to avoid attempting to push an empty bundle; update the block around bundle_args and the two tkn bundle push calls to only run when the guard passes.Makefile (1)
389-391: Consider guarding against empty task directory.If
export-tasksfails silently or produces no output,$(wildcard $(TEKTON_TASKS_DIR)/*.yaml)returns empty, andtkn bundle pushruns with no-farguments. This would either fail cryptically or push an empty bundle.🛡️ Suggested guard
.PHONY: bundle-tasks bundle-tasks: export-tasks ## Build and push a Tekton Bundle OCI image from exported tasks + `@test` -n "$$(ls -A $(TEKTON_TASKS_DIR)/*.yaml 2>/dev/null)" || { echo "No task YAMLs found in $(TEKTON_TASKS_DIR)"; exit 1; } tkn bundle push $(TEKTON_BUNDLE_IMG) $(addprefix -f ,$(wildcard $(TEKTON_TASKS_DIR)/*.yaml))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 389 - 391, The bundle-tasks target can call tkn bundle push with no -f args if $(wildcard $(TEKTON_TASKS_DIR)/*.yaml) is empty; update the bundle-tasks recipe so it first collects the task files (using the same wildcard), tests that the list is non-empty and prints a clear error and exits non-zero if empty (or re-run/depends on export-tasks), and only then invokes tkn bundle push with the generated -f arguments; reference the Makefile target name bundle-tasks, the variable TEKTON_TASKS_DIR, the export-tasks dependency, and the tkn bundle push invocation when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 178-180: The retag-on-release job runs on tag events but depends
on an image pushed by build-tekton-bundle, so add an explicit dependency or
document the coupling: update the retag-on-release job (job name
"retag-on-release") to include needs: build-tekton-bundle so it waits for the
build that pushes ${ env.TEKTON_BUNDLE_IMAGE }:${ env.VERSION }, or
alternatively add an inline comment and restructure the workflow so the bundle
is built/pushed as part of the release flow that creates the tag.
- Line 145: The workflow has an inconsistent actions/checkout version: change
the lone usage of actions/checkout@v4 to actions/checkout@v6 so it matches the
other jobs; locate the checkout step (the line containing "uses:
actions/checkout@v4") and update the version tag to "@v6" to ensure consistency
across the workflow.
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 790-799: The current block writes Tekton Chains hints whenever
CONTAINER_PUSH is set even if no digest was captured; read PUSHED_DIGEST from
/tmp/container-push-digest.txt (as already done) and only write
/tekton/results/IMAGE_URL, /tekton/results/IMAGE_DIGEST, the workspace files
"$WORKSPACE_PATH/.chains/container/url" and ".../digest", and print the "Tekton
Chains: IMAGE_URL=... IMAGE_DIGEST=..." line when PUSHED_DIGEST is non-empty —
change the outer conditional to require both CONTAINER_PUSH and a non-empty
PUSHED_DIGEST (i.e. check [ -n "$CONTAINER_PUSH" ] && [ -n "$PUSHED_DIGEST" ])
so Chains hints are published only when a digest was actually captured.
---
Duplicate comments:
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 197-216: The script computes ACTUAL_DIGEST with
compute_artifact_digest using parts_dir and exportFile before the fallback that
normalizes ride4*/ridesx4* by duplicating boot_a → boot_b; if that fallback runs
the layout changes and the prior digest is invalid. Move the ACTUAL_DIGEST
calculation (the compute_artifact_digest call and any checks that reference
ACTUAL_DIGEST) to after the normalization/boot_a→boot_b duplication block so the
digest is computed against the final parts_dir layout; apply the same change for
the duplicate section around the other occurrence noted (the block covering the
later lines).
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 169-176: The push step currently builds bundle_args from the glob
_output/tasks/*.yaml and calls tkn bundle push even when no files match; add a
guard that verifies there is at least one matching file (e.g. check bundle_args
array length or test for any _output/tasks/*.yaml) before running tkn bundle
push, and if none exist emit a clear message and skip or exit non-zero to avoid
attempting to push an empty bundle; update the block around bundle_args and the
two tkn bundle push calls to only run when the guard passes.
In `@Makefile`:
- Around line 389-391: The bundle-tasks target can call tkn bundle push with no
-f args if $(wildcard $(TEKTON_TASKS_DIR)/*.yaml) is empty; update the
bundle-tasks recipe so it first collects the task files (using the same
wildcard), tests that the list is non-empty and prints a clear error and exits
non-zero if empty (or re-run/depends on export-tasks), and only then invokes tkn
bundle push with the generated -f arguments; reference the Makefile target name
bundle-tasks, the variable TEKTON_TASKS_DIR, the export-tasks dependency, and
the tkn bundle push invocation when making this change.
🪄 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: 238aea02-dc5e-4994-9d4d-a0146615e29f
⛔ Files ignored due to path filters (2)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (20)
.github/workflows/build.yml.gitignoreMakefileapi/v1alpha1/imagebuild_types.goapi/v1alpha1/operatorconfig_types.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/image/image.gocmd/caib/main.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_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.go
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- cmd/caib/main.go
- internal/controller/operatorconfig/controller.go
🚧 Files skipped from review as they are similar to previous changes (8)
- cmd/caib/buildcmd/build_disk_test.go
- cmd/caib/buildcmd/build.go
- cmd/caib/image/image.go
- api/v1alpha1/operatorconfig_types.go
- internal/buildapi/types.go
- internal/common/tasks/scripts/common.sh
- internal/controller/imagebuild/controller.go
- internal/common/tasks/tasks.go
Comment on lines
+790
to
+799
| # Write Tekton Chains type hint results for bootc container | ||
| if [ -n "${CONTAINER_PUSH:-}" ]; then | ||
| PUSHED_DIGEST=$(cat /tmp/container-push-digest.txt 2>/dev/null || echo "") | ||
| echo -n "$CONTAINER_PUSH" > /tekton/results/IMAGE_URL | ||
| echo -n "$PUSHED_DIGEST" > /tekton/results/IMAGE_DIGEST | ||
| echo "Tekton Chains: IMAGE_URL=$CONTAINER_PUSH IMAGE_DIGEST=$PUSHED_DIGEST" | ||
| # Write to workspace for cross-task access (avoids Tekton result-ref issues with skipped tasks) | ||
| mkdir -p "$WORKSPACE_PATH/.chains/container" | ||
| echo -n "$CONTAINER_PUSH" > "$WORKSPACE_PATH/.chains/container/url" | ||
| echo -n "$PUSHED_DIGEST" > "$WORKSPACE_PATH/.chains/container/digest" |
There was a problem hiding this comment.
Only publish container Chains hints after a digest is actually captured.
This block keys off CONTAINER_PUSH alone, so it can write IMAGE_URL with an empty IMAGE_DIGEST when no push ran for the selected mode, or when skopeo --digestfile did not produce output. That leaves Tekton Chains with a false artifact hint.
Suggested fix
-# Write Tekton Chains type hint results for bootc container
-if [ -n "${CONTAINER_PUSH:-}" ]; then
- PUSHED_DIGEST=$(cat /tmp/container-push-digest.txt 2>/dev/null || echo "")
- echo -n "$CONTAINER_PUSH" > /tekton/results/IMAGE_URL
- echo -n "$PUSHED_DIGEST" > /tekton/results/IMAGE_DIGEST
- echo "Tekton Chains: IMAGE_URL=$CONTAINER_PUSH IMAGE_DIGEST=$PUSHED_DIGEST"
- # Write to workspace for cross-task access (avoids Tekton result-ref issues with skipped tasks)
- mkdir -p "$WORKSPACE_PATH/.chains/container"
- echo -n "$CONTAINER_PUSH" > "$WORKSPACE_PATH/.chains/container/url"
- echo -n "$PUSHED_DIGEST" > "$WORKSPACE_PATH/.chains/container/digest"
+# Write Tekton Chains type hint results for bootc container
+if [ -n "${CONTAINER_PUSH_PID:-}" ]; then
+ PUSHED_DIGEST=$(cat /tmp/container-push-digest.txt 2>/dev/null || echo "")
+ if [ -z "$PUSHED_DIGEST" ]; then
+ echo "ERROR: container push completed but no digest was captured" >&2
+ exit 1
+ fi
+ echo -n "$CONTAINER_PUSH" > /tekton/results/IMAGE_URL
+ echo -n "$PUSHED_DIGEST" > /tekton/results/IMAGE_DIGEST
+ echo "Tekton Chains: IMAGE_URL=$CONTAINER_PUSH IMAGE_DIGEST=$PUSHED_DIGEST"
+ mkdir -p "$WORKSPACE_PATH/.chains/container"
+ echo -n "$CONTAINER_PUSH" > "$WORKSPACE_PATH/.chains/container/url"
+ echo -n "$PUSHED_DIGEST" > "$WORKSPACE_PATH/.chains/container/digest"
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/common/tasks/scripts/build_image.sh` around lines 790 - 799, The
current block writes Tekton Chains hints whenever CONTAINER_PUSH is set even if
no digest was captured; read PUSHED_DIGEST from /tmp/container-push-digest.txt
(as already done) and only write /tekton/results/IMAGE_URL,
/tekton/results/IMAGE_DIGEST, the workspace files
"$WORKSPACE_PATH/.chains/container/url" and ".../digest", and print the "Tekton
Chains: IMAGE_URL=... IMAGE_DIGEST=..." line when PUSHED_DIGEST is non-empty —
change the outer conditional to require both CONTAINER_PUSH and a non-empty
PUSHED_DIGEST (i.e. check [ -n "$CONTAINER_PUSH" ] && [ -n "$PUSHED_DIGEST" ])
so Chains hints are published only when a digest was actually captured.
7b8f0d5 to
327b4eb
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
internal/common/tasks/scripts/build_image.sh (1)
790-803:⚠️ Potential issue | 🟠 MajorOnly publish container Chains hints after a digest is captured.
Line 791 still keys off
CONTAINER_PUSHalone. After the warning on Line 794, the script keeps writingIMAGE_URLand an emptyIMAGE_DIGEST, which leaves Tekton Chains with a false artifact hint.Suggested fix
-# Write Tekton Chains type hint results for bootc container -if [ -n "${CONTAINER_PUSH:-}" ]; then - PUSHED_DIGEST=$(cat /tmp/container-push-digest.txt 2>/dev/null || echo "") - if [ -z "$PUSHED_DIGEST" ]; then - echo "WARNING: container push completed but no digest was captured, skipping Chains hints" - fi - echo -n "$CONTAINER_PUSH" > /tekton/results/IMAGE_URL - echo -n "$PUSHED_DIGEST" > /tekton/results/IMAGE_DIGEST - echo "Tekton Chains: IMAGE_URL=$CONTAINER_PUSH IMAGE_DIGEST=$PUSHED_DIGEST" - # Write to workspace for cross-task access (avoids Tekton result-ref issues with skipped tasks) - mkdir -p "$WORKSPACE_PATH/.chains/container" - echo -n "$CONTAINER_PUSH" > "$WORKSPACE_PATH/.chains/container/url" - echo -n "$PUSHED_DIGEST" > "$WORKSPACE_PATH/.chains/container/digest" +# Write Tekton Chains type hint results for bootc container +if [ -n "${CONTAINER_PUSH_PID:-}" ]; then + PUSHED_DIGEST=$(cat /tmp/container-push-digest.txt 2>/dev/null || echo "") + if [ -z "$PUSHED_DIGEST" ]; then + echo "ERROR: container push completed but no digest was captured" >&2 + exit 1 + fi + echo -n "$CONTAINER_PUSH" > /tekton/results/IMAGE_URL + echo -n "$PUSHED_DIGEST" > /tekton/results/IMAGE_DIGEST + echo "Tekton Chains: IMAGE_URL=$CONTAINER_PUSH IMAGE_DIGEST=$PUSHED_DIGEST" + mkdir -p "$WORKSPACE_PATH/.chains/container" + echo -n "$CONTAINER_PUSH" > "$WORKSPACE_PATH/.chains/container/url" + echo -n "$PUSHED_DIGEST" > "$WORKSPACE_PATH/.chains/container/digest" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/build_image.sh` around lines 790 - 803, The script currently writes Tekton Chains hints whenever CONTAINER_PUSH is set even if PUSHED_DIGEST is empty, causing false artifact hints; update the conditional around the write operations so they only run when both CONTAINER_PUSH and a non-empty PUSHED_DIGEST are present (i.e., check PUSHED_DIGEST after reading /tmp/container-push-digest.txt), and only then write to /tekton/results/IMAGE_URL, /tekton/results/IMAGE_DIGEST and WORKSPACE_PATH/.chains/container/{url,digest}; if PUSHED_DIGEST is empty keep the warning and skip the result/workspace writes.internal/common/tasks/scripts/push_artifact.sh (1)
198-244:⚠️ Potential issue | 🟠 MajorVerify the normalized artifact, and fail closed if it cannot be hashed.
Line 202 computes
ACTUAL_DIGESTbefore theboot_a→boot_bfallback at Lines 231-244, and Lines 203-205 only warn when hashing returns empty. That means this task can mutate the artifact after “verification” or skip verification entirely even thoughexpected-artifact-digestwas supplied.Suggested fix
EXPECTED_DIGEST="$(params.expected-artifact-digest)" if [ -n "$EXPECTED_DIGEST" ]; then echo "=== Artifact Integrity Verification ===" - ACTUAL_DIGEST=$(compute_artifact_digest "${parts_dir}" "${exportFile}") - if [ -z "$ACTUAL_DIGEST" ]; then - echo "WARNING: Cannot verify integrity — artifact not found yet" - fi - if [ -n "$ACTUAL_DIGEST" ]; then - if [ "$EXPECTED_DIGEST" != "$ACTUAL_DIGEST" ]; then - echo "ERROR: Artifact integrity check failed!" >&2 - echo " Expected: $EXPECTED_DIGEST" >&2 - echo " Actual: $ACTUAL_DIGEST" >&2 - exit 1 - fi - echo " Integrity verified: $ACTUAL_DIGEST" - fi + VERIFY_INTEGRITY=true else echo "No artifact integrity digest provided, skipping verification" + VERIFY_INTEGRITY=false fi @@ case "$target" in ride4*|ridesx4*) for boot_a_file in "${parts_dir}"/boot_a.*; do [ -f "$boot_a_file" ] || continue boot_b_file=$(echo "$boot_a_file" | sed 's/boot_a/boot_b/') @@ ;; esac + + if [ "$VERIFY_INTEGRITY" = "true" ]; then + ACTUAL_DIGEST=$(compute_artifact_digest "${parts_dir}" "${exportFile}") + if [ -z "$ACTUAL_DIGEST" ]; then + echo "ERROR: Cannot verify integrity — artifact digest could not be computed" >&2 + exit 1 + fi + if [ "$EXPECTED_DIGEST" != "$ACTUAL_DIGEST" ]; then + echo "ERROR: Artifact integrity check failed!" >&2 + echo " Expected: $EXPECTED_DIGEST" >&2 + echo " Actual: $ACTUAL_DIGEST" >&2 + exit 1 + fi + echo " Integrity verified: $ACTUAL_DIGEST" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/push_artifact.sh` around lines 198 - 244, The script computes ACTUAL_DIGEST (via compute_artifact_digest) before the post-build normalization step that may duplicate boot_b from boot_a (the case block handling target patterns), and currently only warns if ACTUAL_DIGEST is empty; move the digest computation to after the parts_dir handling (after the case ... esac) so the digest covers the final, normalized artifact, and if EXPECTED_DIGEST is set but ACTUAL_DIGEST is empty or missing, exit non‑zero (fail closed) instead of just warning; update references to EXPECTED_DIGEST, ACTUAL_DIGEST, compute_artifact_digest, parts_dir, exportFile, and target accordingly.internal/common/tasks/tasks.go (1)
47-68:⚠️ Potential issue | 🟠 MajorDon’t silently downgrade bundle mode to cluster tasks.
If
TaskResolverisTaskResolverBundlebutTaskBundleRefis empty, Line 69 still returns the cluster resolver. That turns a secure-build misconfiguration into unsigned task resolution instead of a hard failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/tasks.go` around lines 47 - 68, buildTaskRef currently falls through to returning a cluster TaskRef when buildConfig.TaskResolver == TaskResolverBundle but buildConfig.TaskBundleRef is empty; change buildTaskRef to explicitly detect the case where TaskResolver == TaskResolverBundle and TaskBundleRef == "" and return nil (or otherwise surface an error) instead of returning the cluster resolver, by adding a guard like: if buildConfig != nil && buildConfig.TaskResolver == TaskResolverBundle && buildConfig.TaskBundleRef == "" { return nil } so callers of buildTaskRef can handle the misconfiguration rather than silently downgrading to cluster tasks.
🧹 Nitpick comments (1)
Makefile (1)
390-391: Add an explicit guard when no exported task YAMLs are present.This improves failure clarity before invoking
tkn bundle push.🔧 Proposed guard
bundle-tasks: export-tasks ## Build and push a Tekton Bundle OCI image from exported tasks + `@test` -n "$(wildcard $(TEKTON_TASKS_DIR)/*.yaml)" || (echo "No task YAML files found in $(TEKTON_TASKS_DIR)"; exit 1) tkn bundle push $(TEKTON_BUNDLE_IMG) $(addprefix -f ,$(wildcard $(TEKTON_TASKS_DIR)/*.yaml))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 390 - 391, The bundle-tasks target invokes "tkn bundle push $(TEKTON_BUNDLE_IMG) $(addprefix -f ,$(wildcard $(TEKTON_TASKS_DIR)/*.yaml))" without guarding against an empty file list; add a guard that checks whether $(wildcard $(TEKTON_TASKS_DIR)/*.yaml) is non-empty and fails early with a clear message (referencing TEKTON_TASKS_DIR and TEKTON_BUNDLE_IMG) before running tkn bundle push to avoid invoking tkn with no input files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/imagebuild_types.go`:
- Around line 68-79: The deepcopy methods for ImageBuildSpec are out of
date—regenerate the generated code so the new SecureBuild and TaskBundleRef
fields are included; run the repository codegen (e.g., run "make generate
manifests" or the project's equivalent) to update
api/v1alpha1/zz_generated.deepcopy.go, verify the generated
DeepCopyInto/DeepCopy methods for the ImageBuildSpec type include SecureBuild
and TaskBundleRef, and commit the updated generated files.
In `@internal/common/tasks/scripts/common.sh`:
- Around line 173-176: The current parts_dir branch computes a digest after
filtering but doesn't verify that any eligible files remain and uses plain xargs
which breaks on whitespace; update the logic around parts_dir and the echo
"sha256:..." command to first collect the filtered file list (using find with
the same predicates but outputting null-separated entries), check that this
filtered list is non-empty before computing the combined sha256, and only then
run a null-safe sha256 pipeline (e.g., find -print0 + xargs -0 or a while-read
-d '' loop) to produce the hash; if the filtered list is empty, fall back to the
single_file branch behavior. Ensure you modify the block that references
parts_dir and the echo "sha256:$(...)" invocation so filenames with whitespace
are handled safely and no digest is emitted for an effectively
empty/fully-excluded parts set.
In `@internal/controller/imagebuild/controller.go`:
- Around line 577-594: Add a rejection for custom YQ helper image drift similar
to the CA and scratch-volume checks: if buildConfig.getYQHelperImage() is
non-empty and not equal to the default helper image baked into bundle tasks
(e.g., tasks.DefaultYQHelperImage), return an error message referencing
OperatorConfig.spec.images.yqHelperImage and the bundle default explaining to
build a custom bundle or remove the override; place this check alongside the
existing TrustedCABundleName/Kind and UseMemoryVolumes/UsePVCScratchVolumes
checks in the same block (look for buildConfig.TrustedCABundleName,
TrustedCABundleKind, UseMemoryVolumes, UsePVCScratchVolumes) so secure builds
fail fast when the configured YQ helper image would silently diverge.
---
Duplicate comments:
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 790-803: The script currently writes Tekton Chains hints whenever
CONTAINER_PUSH is set even if PUSHED_DIGEST is empty, causing false artifact
hints; update the conditional around the write operations so they only run when
both CONTAINER_PUSH and a non-empty PUSHED_DIGEST are present (i.e., check
PUSHED_DIGEST after reading /tmp/container-push-digest.txt), and only then write
to /tekton/results/IMAGE_URL, /tekton/results/IMAGE_DIGEST and
WORKSPACE_PATH/.chains/container/{url,digest}; if PUSHED_DIGEST is empty keep
the warning and skip the result/workspace writes.
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 198-244: The script computes ACTUAL_DIGEST (via
compute_artifact_digest) before the post-build normalization step that may
duplicate boot_b from boot_a (the case block handling target patterns), and
currently only warns if ACTUAL_DIGEST is empty; move the digest computation to
after the parts_dir handling (after the case ... esac) so the digest covers the
final, normalized artifact, and if EXPECTED_DIGEST is set but ACTUAL_DIGEST is
empty or missing, exit non‑zero (fail closed) instead of just warning; update
references to EXPECTED_DIGEST, ACTUAL_DIGEST, compute_artifact_digest,
parts_dir, exportFile, and target accordingly.
In `@internal/common/tasks/tasks.go`:
- Around line 47-68: buildTaskRef currently falls through to returning a cluster
TaskRef when buildConfig.TaskResolver == TaskResolverBundle but
buildConfig.TaskBundleRef is empty; change buildTaskRef to explicitly detect the
case where TaskResolver == TaskResolverBundle and TaskBundleRef == "" and return
nil (or otherwise surface an error) instead of returning the cluster resolver,
by adding a guard like: if buildConfig != nil && buildConfig.TaskResolver ==
TaskResolverBundle && buildConfig.TaskBundleRef == "" { return nil } so callers
of buildTaskRef can handle the misconfiguration rather than silently downgrading
to cluster tasks.
---
Nitpick comments:
In `@Makefile`:
- Around line 390-391: The bundle-tasks target invokes "tkn bundle push
$(TEKTON_BUNDLE_IMG) $(addprefix -f ,$(wildcard $(TEKTON_TASKS_DIR)/*.yaml))"
without guarding against an empty file list; add a guard that checks whether
$(wildcard $(TEKTON_TASKS_DIR)/*.yaml) is non-empty and fails early with a clear
message (referencing TEKTON_TASKS_DIR and TEKTON_BUNDLE_IMG) before running tkn
bundle push to avoid invoking tkn with no input files.
🪄 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: 60f800d2-2964-4de9-a07a-6d013b8b2d69
⛔ Files ignored due to path filters (2)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (21)
.github/workflows/build.yml.gitignoreMakefileapi/v1alpha1/imagebuild_types.goapi/v1alpha1/operatorconfig_types.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/image/image.gocmd/caib/main.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_test.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/common.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/common/tasks/trusted_ca_test.gointernal/controller/imagebuild/controller.gointernal/controller/operatorconfig/controller.go
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- internal/controller/operatorconfig/controller.go
- cmd/caib/main.go
- internal/common/tasks/pipeline_test.go
- cmd/caib/image/image.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/caib/buildcmd/build_disk_test.go
- cmd/caib/runtime_wiring.go
- .github/workflows/build.yml
Comment on lines
+68
to
79
| // SecureBuild enables supply chain security for this build. | ||
| // When true, pipeline tasks are resolved from the signed Tekton Bundle | ||
| // specified in TaskBundleRef instead of cluster-installed tasks. | ||
| // +optional | ||
| SecureBuild bool `json:"secureBuild,omitempty"` | ||
|
|
||
| // TaskBundleRef is the digest-pinned OCI reference to the Tekton Bundle | ||
| // used for this build. Set automatically by the Build API from the | ||
| // OperatorConfig at request time to prevent TOCTOU races. | ||
| // +optional | ||
| TaskBundleRef string `json:"taskBundleRef,omitempty"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check api type fields =="
rg -n --type go 'SecureBuild|TaskBundleRef' api/v1alpha1
echo "== Check generated deepcopy code =="
fd 'zz_generated.deepcopy.go' api/v1alpha1 --exec rg -n 'SecureBuild|TaskBundleRef' {}
echo "== Check CRD schemas include new fields =="
fd '.+\.ya?ml$' config/crd/bases --exec rg -n 'secureBuild|taskBundleRef' {}Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 1000
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check .gitignore for generated files patterns =="
rg -n 'zz_generated\.deepcopy|config/crd/bases' .gitignore
echo "== Verify if zz_generated.deepcopy.go exists =="
find api/v1alpha1 -name 'zz_generated.deepcopy.go' -type f 2>/dev/null | head -5
echo "== Verify if config/crd/bases exists =="
find config/crd/bases -type f 2>/dev/null | head -5
echo "== Check if config/crd/bases directory exists =="
test -d config/crd/bases && echo "Directory exists" || echo "Directory does not exist"Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== View .gitignore content =="
cat .gitignore
echo ""
echo "== Check git status for generated files =="
git ls-files api/v1alpha1/zz_generated.deepcopy.go 2>/dev/null || echo "File not tracked in git"
echo ""
echo "== List files in config/crd/bases if it exists =="
ls -la config/crd/bases/ 2>/dev/null || echo "Directory does not exist"
echo ""
echo "== Check if files exist in filesystem =="
test -f api/v1alpha1/zz_generated.deepcopy.go && echo "zz_generated.deepcopy.go EXISTS" || echo "zz_generated.deepcopy.go DOES NOT EXIST"Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 1691
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check if SecureBuild/TaskBundleRef in zz_generated.deepcopy.go =="
rg -n 'SecureBuild|TaskBundleRef' api/v1alpha1/zz_generated.deepcopy.go 2>/dev/null || echo "Fields NOT found in deepcopy file"
echo ""
echo "== Check ImageBuild CRD for secureBuild/taskBundleRef =="
rg -n 'secureBuild|taskBundleRef' config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yaml 2>/dev/null || echo "Fields NOT found in CRD"
echo ""
echo "== Show context around ImageBuildSpec in CRD =="
grep -A 200 'ImageBuildSpec' config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yaml | head -100Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 5074
DeepCopy generation is missing the new fields—run make generate manifests before merge.
The CRD includes secureBuild and taskBundleRef, but the generated DeepCopy file (api/v1alpha1/zz_generated.deepcopy.go) does not contain these fields. The deepcopy methods must be regenerated to include the new fields in the ImageBuildSpec type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v1alpha1/imagebuild_types.go` around lines 68 - 79, The deepcopy methods
for ImageBuildSpec are out of date—regenerate the generated code so the new
SecureBuild and TaskBundleRef fields are included; run the repository codegen
(e.g., run "make generate manifests" or the project's equivalent) to update
api/v1alpha1/zz_generated.deepcopy.go, verify the generated
DeepCopyInto/DeepCopy methods for the ImageBuildSpec type include SecureBuild
and TaskBundleRef, and commit the updated generated files.
There was a problem hiding this comment.
@bennyz, you're right — I apologize for the noise. SecureBuild is a bool and TaskBundleRef is a string, both of which are value types in Go. The generated DeepCopyInto method copies all value-typed fields automatically via the *out = *in struct assignment at the top. Only reference types (pointers, slices, maps, interfaces) require explicit per-field handling in the generated deepcopy code. The concern I raised was invalid.
✏️ Learnings added
Learnt from: bennyz
Repo: centos-automotive-suite/automotive-dev-operator PR: 197
File: api/v1alpha1/imagebuild_types.go:68-79
Timestamp: 2026-04-16T08:44:03.524Z
Learning: In centos-automotive-suite/automotive-dev-operator (Go / controller-gen), generated `DeepCopyInto` methods copy all value-typed fields (bool, string, int, etc.) automatically through the `*out = *in` struct assignment. Do NOT flag missing deepcopy entries for plain bool or string fields in api/v1alpha1 types — only pointer, slice, map, and interface fields require explicit handling in the generated deepcopy code.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: bkhizgiy
Repo: centos-automotive-suite/automotive-dev-operator PR: 144
File: internal/controller/imagereseal/controller.go:713-719
Timestamp: 2026-03-05T14:31:11.693Z
Learning: In automotive-dev-operator, the `sealedTaskSpec` function in `internal/common/tasks/tasks.go` only uses `TrustedCABundleKind` and `TrustedCABundleName` from `BuildConfig`. Fields like `UseMemoryVolumes`, `MemoryVolumeSize`, `DefaultLeaseDuration`, and `FlashTimeoutMinutes` are not used by sealed task specs; they are only used by image build and flash task generation functions. Therefore, `resolveBuildConfig` in `internal/controller/imagereseal/controller.go` does not need to populate those fields.
Learnt from: bkhizgiy
Repo: centos-automotive-suite/automotive-dev-operator PR: 144
File: api/v1alpha1/operatorconfig_types.go:392-399
Timestamp: 2026-03-09T17:42:40.911Z
Learning: In automotive-dev-operator, all Tekton build tasks (imagebuild, imagereseal) currently run in the operator namespace (automotive-dev-operator-system). Therefore, `CertificateSourceRef` in `api/v1alpha1/operatorconfig_types.go` only needs `Kind` and `Name` — no `Namespace` field is required because Secret/ConfigMap volumes are resolved within the same operator namespace. Cross-namespace CA bundle replication is deferred to a future PR.
Learnt from: bennyz
Repo: centos-automotive-suite/automotive-dev-operator PR: 150
File: api/v1alpha1/imagebuild_types.go:74-77
Timestamp: 2026-03-12T09:28:25.830Z
Learning: In centos-automotive-suite/automotive-dev-operator, the files `api/v1alpha1/zz_generated.deepcopy.go` and all `config/crd/bases/*.yaml` CRD manifests are explicitly listed in `.gitignore` and are NOT committed to the repository. They are regenerated at deploy/build time via `make generate manifests`. PRs should NOT include these generated files.
Learnt from: CR
Repo: centos-automotive-suite/automotive-dev-operator PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-08T15:28:37.446Z
Learning: Applies to api/v1alpha1/**/*.go : After modifying types in api/v1alpha1/, run `make generate manifests` to regenerate DeepCopy methods and CRDs
Learnt from: bennyz
Repo: centos-automotive-suite/automotive-dev-operator PR: 128
File: internal/buildapi/container_builds.go:197-211
Timestamp: 2026-02-25T15:25:41.519Z
Learning: In the automotive-dev-operator repository's container build flow, the registry secrets managed by `setContainerBuildSecretOwnerRef` in internal/buildapi/container_builds.go are newly created and don't have pre-existing owner references, so directly setting the OwnerReferences slice is safe.
Learnt from: evakhoni
Repo: centos-automotive-suite/automotive-dev-operator PR: 136
File: cmd/caib/image/image.go:83-87
Timestamp: 2026-03-05T08:02:58.618Z
Learning: In the automotive-dev-operator repository, the sealed image commands (prepare-reseal, reseal, extract-for-signing, inject-signed) in cmd/caib/sealedcmd/sealed.go always execute via the API (calling sealedRunViaAPI). There is no local execution path — all handlers unconditionally require a server URL. Despite PR `#91` description mentioning local mode, this is not reflected in the actual code.
Learnt from: bennyz
Repo: centos-automotive-suite/automotive-dev-operator PR: 190
File: internal/controller/imagebuild/controller.go:203-212
Timestamp: 2026-03-30T15:44:51.734Z
Learning: When using controller-runtime’s `controllerutil.SetOwnerReference` / `SetControllerReference`, note that `referSameObject` de-duplicates existing owner references by matching Group + Version + Kind + Name only (not by UID). Although the UID from the object is written into the owner reference, it is not used for deduplication/lookup. In reviews, don’t expect owner reference matching to prevent duplicates based on UID—ensure the GVK+name are what you intend to be unique.
Comment on lines
+173
to
+176
| if [ -d "$parts_dir" ] && [ -n "$(ls -A "$parts_dir" 2>/dev/null)" ]; then | ||
| echo "sha256:$(cd "$parts_dir" && find . -maxdepth 1 -type f ! -name '*.size' ! -name 'aib-manifest.yml' -printf '%f\n' | sort | xargs sha256sum | sha256sum | cut -d' ' -f1)" | ||
| elif [ -f "$single_file" ]; then | ||
| echo "sha256:$(sha256sum "$single_file" | cut -d' ' -f1)" |
There was a problem hiding this comment.
Avoid hashing an empty/non-eligible parts set.
Line 173 checks only whether the directory is non-empty, but Line 174 hashes after filtering. If only excluded files exist, this still emits a digest (from an empty stream) instead of returning empty/falling back to single_file. Also, default xargs splits on whitespace in filenames.
🔧 Proposed fix
compute_artifact_digest() {
local parts_dir="$1" single_file="$2"
- if [ -d "$parts_dir" ] && [ -n "$(ls -A "$parts_dir" 2>/dev/null)" ]; then
- echo "sha256:$(cd "$parts_dir" && find . -maxdepth 1 -type f ! -name '*.size' ! -name 'aib-manifest.yml' -printf '%f\n' | sort | xargs sha256sum | sha256sum | cut -d' ' -f1)"
+ local -a files=()
+ if [ -d "$parts_dir" ]; then
+ mapfile -d '' -t files < <(
+ find "$parts_dir" -maxdepth 1 -type f ! -name '*.size' ! -name 'aib-manifest.yml' -printf '%f\0' | sort -z
+ )
+ if [ "${`#files`[@]}" -gt 0 ]; then
+ echo "sha256:$(
+ cd "$parts_dir" &&
+ printf '%s\0' "${files[@]}" |
+ xargs -0 sha256sum |
+ sha256sum |
+ cut -d' ' -f1
+ )"
+ return
+ fi
elif [ -f "$single_file" ]; then
echo "sha256:$(sha256sum "$single_file" | cut -d' ' -f1)"
fi
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/common/tasks/scripts/common.sh` around lines 173 - 176, The current
parts_dir branch computes a digest after filtering but doesn't verify that any
eligible files remain and uses plain xargs which breaks on whitespace; update
the logic around parts_dir and the echo "sha256:..." command to first collect
the filtered file list (using find with the same predicates but outputting
null-separated entries), check that this filtered list is non-empty before
computing the combined sha256, and only then run a null-safe sha256 pipeline
(e.g., find -print0 + xargs -0 or a while-read -d '' loop) to produce the hash;
if the filtered list is empty, fall back to the single_file branch behavior.
Ensure you modify the block that references parts_dir and the echo
"sha256:$(...)" invocation so filenames with whitespace are handled safely and
no digest is emitted for an effectively empty/fully-excluded parts set.
Emit IMAGE_URL and IMAGE_DIGEST task results from build and push tasks, enabling Tekton Chains to automatically discover artifacts and generate signed SLSA provenance attestations. Attach the sanitized osbuild manifest (image.json) as an OCI referrer on the disk artifact for supply chain verification. - build_image.sh: capture bootc container digest via --digestfile, write to /tekton/results/ (empty defaults prevent TaskRun failure when no container push occurs) - push_artifact.sh: capture ORAS push digest via tee + sed fallback, write to /tekton/results/; sanitize and attach osbuild manifest as OCI referrer (strips password hashes, secrets, embedded credentials) - tasks.go: declare IMAGE_URL/IMAGE_DIGEST results on both tasks; surface as pipeline results (named differently to avoid Chains double-counting) All changes are passive - when Chains is not installed or the registry does not support OCI referrers, builds are unaffected. Assisted-by: claude-opus-4.6 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
…pport Add multi-artifact Tekton Chains SLSA provenance via IMAGES pipeline result and collect-images-result finally task. Both bootc container and disk image artifacts now get signed attestations. Add sha256 artifact integrity verification between build and push tasks to detect tampering on shared PVC volumes. Refactor inline TaskRef blocks to use buildTaskRef() helper that supports both cluster and bundle resolvers, controlled via BuildConfig.TaskResolver. Assisted-by: claude-opus-4.6 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Add cmd/export-tasks to generate task YAML from Go code, ensuring bundles contain the exact same definitions as cluster-installed tasks. Add Makefile targets (export-tasks, bundle-tasks) for local bundle building. Add build-tekton-bundle CI job that pushes bundles on every commit to main/release-*, with cosign signing on release tags. Assisted-by: claude-opus-4.6 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Add TaskBundleRef to OperatorConfig.OSBuilds to configure the signed bundle reference cluster-wide. Add SecureBuild to ImageBuild spec. When true, the controller reads TaskBundleRef from OperatorConfig and sets BuildConfig to resolve tasks from the bundle instead of cluster-installed tasks. Wire --secure flag through caib CLI (build, disk, build-dev commands), Build API, and ImageBuild controller. Assisted-by: claude-opus-4.6 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Cover buildTaskRef cluster/bundle switching, IMAGES pipeline result, collect-images-result finally task, artifact integrity digest params, and bundle resolver propagation through GenerateTektonPipeline. Assisted-by: claude-opus-4.6 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
The upload pod is the first consumer of the WaitForFirstConsumer PVC, so it determines which topology zone the PVC binds to. Without the kubernetes.io/arch affinity, on multi-arch clusters the upload pod could schedule on a different architecture node than the build pod, making the build unschedulable. Assisted-by: claude-opus-4.6 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
10 tasks
This was referenced May 4, 2026
14 tasks
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
2 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.
Emit IMAGE_URL and IMAGE_DIGEST task results from build and push tasks, enabling Tekton Chains to automatically discover artifacts and generate signed SLSA provenance attestations. Attach the sanitized osbuild manifest (image.json) as an OCI referrer on the disk artifact for supply chain verification.
Summary by CodeRabbit
New Features
Build & Infrastructure
Features
Bug Fixes
Tests