fix: VSA generation issues in verify-conforma-konflux-ta task#3118
fix: VSA generation issues in verify-conforma-konflux-ta task#3118simonbaird merged 1 commit intoconforma:mainfrom
Conversation
Review Summary by QodoAdd missing sourceDataArtifact parameter to trusted artifact task
WalkthroughsDescription• Add missing sourceDataArtifact parameter to trusted artifact task • Fixes basename error when pushing trusted artifacts • Properly sets sourceDataArtifact from previous task results Diagramflowchart LR
A["Task Parameters"] -->|missing sourceDataArtifact| B["basename Error"]
C["Previous Task Results"] -->|sourceDataArtifact.path| D["Task Execution"]
B -->|fix applied| D
File Changes1. tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewⓘ The new review experience is currently in Beta. Learn more |
arewm
force-pushed
the
fix/ta-missing-basename
branch
from
8a8b30b to
728022f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Patch looks reasonable. I don't think I understand the motivation though. What/where is /usr/local/bin/create-archive ? What is a sourceDataArtifact in this context, and what are you doing with the VSA content? |
|
/ok-to-test |
|
I found we do have some test coverage for this task in |
I did some rftm, and I know what this is now. 😅 Is your plan to consume the VSA as a "trusted artifact" input in another task? |
|
I am still working on some additional changes when testing locally. I moved this PR to draft until I finish working on it. |
|
I did a sanity check on the bash, seems good: |
tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml
Outdated
Show resolved
Hide resolved
arewm
changed the title
The upstream verify-conforma-konflux-ta task had several issues when VSA generation is enabled. This local copy fixes them while an upstream PR (conforma/cli#3118) is pending. The ec CLI requires --attestation-output-dir to be under /tmp or cwd, so VSA output is written to /tmp/vsa-output then copied to the workdir for trusted artifact archival. Results use echo -n to avoid trailing newlines that broke Tekton When Expression matching downstream. The --vsa-upload flag is only passed for dsse format since the local backend requires a signing key. The conforma JSON report is included in the trusted artifact for downstream SLSA VSA generation. The taskRef is hardcoded to this repo to bypass the ec-defaults ConfigMap injection (see konflux-ci/konflux-ci#5375). Assisted-by: Claude Code (Opus 4.6)
arewm
force-pushed
the
fix/ta-missing-basename
branch
from
17339a9 to
c4378d5
Compare
arewm
force-pushed
the
fix/ta-missing-basename
branch
3 times, most recently
from
a9b567a to
63d3d42
Compare
Review Summary by QodoFix VSA generation and archival in verify-conforma-konflux-ta task
WalkthroughsDescription• Fixed VSA generation bugs preventing file production and archival - ec CLI output path must be under /tmp or cwd, not /var/workdir - --vsa-upload only passed for dsse format (predicate doesn't need signing) - echo without -n flag breaks Tekton When Expression matching • Fixed trusted artifact archival using correct extract directory parameter - workDir now uses $(params.TRUSTED_ARTIFACTS_EXTRACT_DIR) instead of hardcoded path - VSA files properly captured in archive root, not nested under conforma/ prefix • Added report-json.json to VSA directory for downstream SLSA generation • Added acceptance test scenario for ENABLE_VSA with predicate format • Introduced taskResultShouldEqual step definition for targeted assertions Diagramflowchart LR
A["VSA Generation Issues"] --> B["Fix ec CLI Path Constraints"]
A --> C["Fix Format-Specific Logic"]
A --> D["Fix Echo Newline Issue"]
B --> E["Use /tmp for Output"]
C --> F["--vsa-upload Only for DSSE"]
D --> G["Use echo -n Flag"]
E --> H["Copy to workDir After"]
A --> I["Fix Archival Path"]
I --> J["Use Extract Dir Parameter"]
I --> K["Include report-json.json"]
J --> L["Correct Archive Structure"]
A --> M["Add Test Coverage"]
M --> N["Predicate Format Scenario"]
M --> O["Result Assertion Helper"]
File Changes1. acceptance/kubernetes/kubernetes.go
|
Code Review by Qodo
1. Non-local VSA_UPLOAD fails step
|
|
@simonbaird, I have moved this out of draft. The content of this PR is ready for review again. I tested this task out in my own release pipeline on KinD. The content of that task should match the one in this PR. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml
Outdated
Show resolved
Hide resolved
arewm
force-pushed
the
fix/ta-missing-basename
branch
2 times, most recently
from
86bcb21 to
15a0584
Compare
arewm
force-pushed
the
fix/ta-missing-basename
branch
from
15a0584 to
37bdfdc
Compare
arewm
force-pushed
the
fix/ta-missing-basename
branch
from
37bdfdc to
6c9427d
Compare
|
@simonbaird, acceptance tests are now passing for this PR. |
|
/ok-to-test |
|
Lgtm. It's gonna conflict with #3158 PR, but I'll sort it out. |
|
Do you have a Jira open for this? |
|
I don't, no. It was an issue that I was encountering from before. We previously enabled functionality to produce the VSA subject without the envelope. We never provided a way to get that content out. |
|
I know I approved already, but I was just reading one of Qodo's review comments and it seems like maybe a blocker? Wdyt? |
…test coverage The ENABLE_VSA code path had several bugs that together prevented VSA files from being produced and archived correctly. The ec CLI rejects output paths outside /tmp or cwd, --vsa-upload must only be passed for dsse format since the local@ backend requires a signing key, and echo without -n writes a trailing newline that breaks Tekton When Expression matching. The trusted artifact archival was also broken: workDir was hardcoded to /var/workdir rather than $(params.TRUSTED_ARTIFACTS_EXTRACT_DIR), so downstream tasks would find the snapshot nested under a conforma/ prefix instead of at the archive root. The VSA_UPLOAD default was similarly misaligned, pointing outside the extract dir so VSA files were never captured in the archive. The report-json.json is now also copied into the VSA directory so downstream tasks can generate SLSA VSAs from the Conforma evaluation report. Adds an acceptance test scenario for ENABLE_VSA=true with predicate format, a new taskResultShouldEqual step definition for targeted result assertions, and updates the existing snapshot for the echo -n fix. Assisted-by: Claude Code (Sonnet 4.6) Signed-off-by: arewm <arewm@users.noreply.github.com>
arewm
force-pushed
the
fix/ta-missing-basename
branch
from
6c9427d to
0057798
Compare
📝 WalkthroughWalkthroughChanges introduce new test infrastructure for validating task results, add a test scenario for VSA generation with predicate format attestation, update documentation and task configuration for VSA handling with refined directory paths, and modify result value typing in TaskInfo processing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
🧹 Nitpick comments (1)
tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml (1)
325-327: Consider extractingVSA_LOCAL_PATHonce.
VSA_LOCAL_PATHis computed identically at lines 327 and 355. Since both usages are within the same script, you could compute it once after theENABLE_VSAcheck and reuse it.♻️ Suggested simplification
# Add VSA arguments if enabled if [[ "$(params.ENABLE_VSA)" == "true" ]]; then EC_ARGS+=(--vsa --attestation-format=$(params.ATTESTATION_FORMAT)) # Extract local path from VSA_UPLOAD for output directory # VSA_UPLOAD format is "local@/path/to/dir" VSA_LOCAL_PATH=$(echo "$(params.VSA_UPLOAD)" | grep -oE '^local@[^ ]+' | sed 's/^local@//' | head -n1 || true) if [[ "$(params.ATTESTATION_FORMAT)" == "dsse" ]]; then ... fi # ec requires --attestation-output-dir to be under /tmp or cwd. ... echo -n "true" > $(results.VSA_GENERATED.path) else echo -n "false" > $(results.VSA_GENERATED.path) fi # Execute EC with constructed arguments ec "${EC_ARGS[@]}" # Copy VSA output from /tmp to workdir for trusted artifact archival if [[ "$(params.ENABLE_VSA)" == "true" ]]; then - VSA_LOCAL_PATH=$(echo "$(params.VSA_UPLOAD)" | grep -oE '^local@[^ ]+' | sed 's/^local@//' | head -n1 || true) + # VSA_LOCAL_PATH already computed above if [[ -n "$VSA_LOCAL_PATH" && -d "/tmp/vsa-output" ]]; thenAlso applies to: 354-361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml` around lines 325 - 327, The script computes VSA_LOCAL_PATH twice; extract and assign VSA_LOCAL_PATH a single time after the ENABLE_VSA check and before any uses so both later blocks reuse the same variable. Locate the two identical extraction commands that set VSA_LOCAL_PATH and replace them with one assignment (using the existing grep/sed pipeline) placed immediately after the ENABLE_VSA condition is evaluated, then remove the duplicate extraction and reference the single VSA_LOCAL_PATH variable in the sections that previously recomputed it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml`:
- Around line 325-327: The script computes VSA_LOCAL_PATH twice; extract and
assign VSA_LOCAL_PATH a single time after the ENABLE_VSA check and before any
uses so both later blocks reuse the same variable. Locate the two identical
extraction commands that set VSA_LOCAL_PATH and replace them with one assignment
(using the existing grep/sed pipeline) placed immediately after the ENABLE_VSA
condition is evaluated, then remove the duplicate extraction and reference the
single VSA_LOCAL_PATH variable in the sections that previously recomputed it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c60d232-c646-44bd-ab61-f77e7ed0edff
⛔ Files ignored due to path filters (1)
features/__snapshots__/ta_task_validate_image.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
acceptance/kubernetes/kind/kubernetes.goacceptance/kubernetes/kubernetes.godocs/modules/ROOT/pages/verify-conforma-konflux-ta.adocfeatures/ta_task_validate_image.featuretasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml
|
/ok-to-test |
|
Let's merge. |
A fixup for conforma#3118. Looks like we don't need it until later in the task, so remove the initial place where it was being calculated but not used. Also add a fixme descibing the problem what was mentioned by Qodo in the conforma#3118 PR.
A fixup for conforma#3118. Looks like we don't need it until later in the task, so remove the initial place where it was being calculated but not used. Also add a fixme descibing the problem what was mentioned by Qodo in the conforma#3118 PR.
2 participants
The ENABLE_VSA code path had several bugs that together prevented VSA files
from being produced and archived correctly. The ec CLI rejects output paths
outside /tmp or cwd, --vsa-upload must only be passed for dsse format since
the local@ backend requires a signing key, and echo without -n writes a
trailing newline that breaks Tekton When Expression matching.
The trusted artifact archival was also broken: workDir was hardcoded to
/var/workdir rather than $(params.TRUSTED_ARTIFACTS_EXTRACT_DIR), so
downstream tasks would find the snapshot nested under a conforma/ prefix
instead of at the archive root. The VSA_UPLOAD default was similarly
misaligned, pointing outside the extract dir so VSA files were never
captured in the archive. The report-json.json is now also copied into the
VSA directory so downstream tasks can generate SLSA VSAs from the
Conforma evaluation report.
Adds an acceptance test scenario for ENABLE_VSA=true with predicate format,
a new taskResultShouldEqual step definition for targeted result assertions,
and updates the existing snapshot for the echo -n fix.
Test plan
🤖 Generated with Claude Code