CLID-627: Add test to check signature mirroring and deletion#1449
CLID-627: Add test to check signature mirroring and deletion#1449adolfo-ab wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
|
@adolfo-ab: This pull request references CLID-627 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Cosign signing support to integration test image builders via new Makefiles ChangesSignature Preservation Testing Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant Disk as Local Disk Cache
participant Registry as Local Registry
rect rgba(70, 130, 180, 0.5)
Note over Test,Disk: Mirror to Disk
Test->>Disk: mirrorToDisk (isc-signatures.yaml)
Test->>Disk: verify cache and tar outputs
end
rect rgba(60, 179, 113, 0.5)
Note over Test,Registry: Disk to Mirror
Test->>Registry: diskToMirror --remove-signatures=false
Test->>Registry: verify mirrored images
end
rect rgba(205, 92, 92, 0.5)
Note over Test,Registry: Delete with Signature Validation
Test->>Registry: DeletePhaseOne (disc-signatures.yaml)
Test->>Registry: DeletePhaseTwo execution
Test->>Registry: expectOnlySignatureTagsRemain
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/helpers_test.go`:
- Around line 260-263: The `IsCatalog` check on line 260 only examines
`tags[0]`, causing non-deterministic test behavior when tag ordering changes.
Instead of checking only the first tag, iterate through all tags in the `tags`
slice and call `IsCatalog` for each tag. If any tag returns true for
`IsCatalog`, skip the repository with `continue`. Only process the repository if
none of the tags are identified as catalogs. This ensures the test correctly
handles repositories with multiple tags regardless of their order.
In `@tests/integration/image-builders/generate-cosign-keys.sh`:
- Around line 11-20: The early exit condition in the script only checks for the
existence of cosign.key and cosign.pub in the KEYS_DIR, but does not verify that
cosign-signing-config.json also exists. This causes the script to exit
prematurely when the signing config file is deleted while the key files remain,
leaving the repository in a broken state. Modify the conditional statement that
checks for existing keys to also include a check for the existence of
cosign-signing-config.json alongside cosign.key and cosign.pub in KEYS_DIR. Only
exit early if all three files are present; otherwise, allow the script to
continue and regenerate the missing file or files.
In `@tests/integration/image-builders/operator/Makefile`:
- Around line 57-62: In the sign-bundles-cosign target and all other similar
targets mentioned (lines 57-107) that contain for loops running critical
operations like cosign signing, image inspection, and artifact tagging, add
fail-fast semantics by including 'set -e' at the start of the shell command
block. This ensures that if any command in the loop fails, the entire loop and
target will exit immediately with a non-zero status instead of continuing
execution and silently masking failures. Apply this consistently across all
affected targets to prevent cryptographic signing errors and other critical
failures from being masked.
- Around line 84-87: The Python one-liner in the referrer variable assignment
(and similarly in the duplicate lines at 96 and 104) accesses the last element
of the manifests list without first validating that the list is not empty, which
will cause an IndexError if oras discover returns no referrers. Update the
Python code within the oras discover command to check if the manifests list
exists and contains at least one element before attempting to access it with the
index operator, and handle the case appropriately (either by returning an empty
value, exiting with an error, or using a default behavior depending on the
desired behavior). Apply this same fix to all three occurrences of this pattern
in the Makefile.
In `@tests/integration/image-builders/release/Makefile`:
- Around line 33-41: The sign-tag-legacy target contains a multi-step loop that
continues executing even when commands fail (skopeo inspect, JSON parsing, oras
discover, or oras tag), which silently hides failures. Add set -e at the
beginning of the shell command block (after the @ symbol) to enable fail-fast
behavior, ensuring that if any command in the loop returns a non-zero exit code,
the entire loop terminates immediately and the target fails as expected.
- Line 38: The referrer extraction in line 38 uses `manifests[-1]` without
validating that the manifests array is non-empty, which will cause a silent
failure or incorrect behavior if oras discover returns no referrers. Modify the
Python one-liner within the referrer variable assignment to add validation that
checks if the manifests array exists and contains elements before accessing the
last element, and handle the error case appropriately (such as exiting with an
error message or returning an empty value) to make the failure explicit rather
than silent.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 75ca94d5-8b86-45fc-87b0-79cb75e09474
⛔ Files ignored due to path filters (1)
tests/integration/testdata/keys/cosign.pubis excluded by!**/*.pub
📒 Files selected for processing (10)
tests/integration/helpers_test.gotests/integration/image-builders/generate-cosign-keys.shtests/integration/image-builders/operator/Makefiletests/integration/image-builders/release/Makefiletests/integration/image-builders/release/create-release.shtests/integration/m2d_d2m_test.gotests/integration/testdata/imagesetconfigs/signatures/disc-signatures.yamltests/integration/testdata/imagesetconfigs/signatures/isc-signatures.yamltests/integration/testdata/keys/cosign-signing-config.jsontests/integration/testdata/keys/cosign.key
💤 Files with no reviewable changes (1)
- tests/integration/image-builders/release/create-release.sh
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/integration/image-builders/release/Makefile (1)
28-41: 💤 Low valueConsider defining a variable for the release images list.
The image references
quay.io/oc-mirror/release/test-image:v0.0.1andquay.io/oc-mirror/release/test-release-index:v0.0.1are repeated insign-cosign(lines 29-30) andsign-tag-legacy(line 34). Extracting them to aRELEASE_IMAGESvariable would centralize the list and reduce duplication.RELEASE_IMAGES := quay.io/oc-mirror/release/test-image:v0.0.1 quay.io/oc-mirror/release/test-release-index:v0.0.1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/image-builders/release/Makefile` around lines 28 - 41, Define a RELEASE_IMAGES variable at the top of the Makefile containing the two image references (quay.io/oc-mirror/release/test-image:v0.0.1 and quay.io/oc-mirror/release/test-release-index:v0.0.1). Then replace the hardcoded image references in the sign-cosign target with references to this variable, and update the sign-tag-legacy target to use the RELEASE_IMAGES variable in the for loop instead of hardcoding the image list. This eliminates duplication and makes the images easier to maintain in a single location.tests/integration/image-builders/operator/Makefile (1)
80-107: 💤 Low valueConsider extracting the repeated legacy-tagging logic into a helper.
The
sign-tag-legacytarget contains three nearly identical loops (lines 80-89, 90-99, 100-107) that all perform the same sequence: inspect image → compute signature tag → discover referrer → tag referrer. This duplication increases maintenance burden and makes it easy for bugs to diverge across loops.Consider defining a reusable shell function or a separate target that accepts an image argument, then invoke it from a single loop over all images.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/image-builders/operator/Makefile` around lines 80 - 107, The sign-tag-legacy target contains three nearly identical loops that repeat the same sequence of operations: using skopeo inspect to get the image digest, computing a signature tag from the digest, discovering the referrer with oras discover, and finally tagging the referrer with oras tag. Extract this repeated logic into a reusable shell function that accepts an image name as a parameter and performs all these steps (the skopeo inspect, digest computation via sed substitution, oras discover, and final oras tag operations). Then replace each of the three separate for loops in sign-tag-legacy with a single loop that calls this helper function for each image, reducing duplication and maintenance burden.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/integration/image-builders/operator/Makefile`:
- Around line 80-107: The sign-tag-legacy target contains three nearly identical
loops that repeat the same sequence of operations: using skopeo inspect to get
the image digest, computing a signature tag from the digest, discovering the
referrer with oras discover, and finally tagging the referrer with oras tag.
Extract this repeated logic into a reusable shell function that accepts an image
name as a parameter and performs all these steps (the skopeo inspect, digest
computation via sed substitution, oras discover, and final oras tag operations).
Then replace each of the three separate for loops in sign-tag-legacy with a
single loop that calls this helper function for each image, reducing duplication
and maintenance burden.
In `@tests/integration/image-builders/release/Makefile`:
- Around line 28-41: Define a RELEASE_IMAGES variable at the top of the Makefile
containing the two image references (quay.io/oc-mirror/release/test-image:v0.0.1
and quay.io/oc-mirror/release/test-release-index:v0.0.1). Then replace the
hardcoded image references in the sign-cosign target with references to this
variable, and update the sign-tag-legacy target to use the RELEASE_IMAGES
variable in the for loop instead of hardcoding the image list. This eliminates
duplication and makes the images easier to maintain in a single location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 204bf8e0-8d64-495f-a743-21d55b2b35a0
⛔ Files ignored due to path filters (1)
tests/integration/testdata/keys/cosign.pubis excluded by!**/*.pub
📒 Files selected for processing (10)
tests/integration/helpers_test.gotests/integration/image-builders/generate-cosign-keys.shtests/integration/image-builders/operator/Makefiletests/integration/image-builders/release/Makefiletests/integration/image-builders/release/create-release.shtests/integration/m2d_d2m_test.gotests/integration/testdata/imagesetconfigs/signatures/disc-signatures.yamltests/integration/testdata/imagesetconfigs/signatures/isc-signatures.yamltests/integration/testdata/keys/cosign-signing-config.jsontests/integration/testdata/keys/cosign.key
💤 Files with no reviewable changes (1)
- tests/integration/image-builders/release/create-release.sh
✅ Files skipped from review due to trivial changes (1)
- tests/integration/testdata/keys/cosign-signing-config.json
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/integration/testdata/imagesetconfigs/signatures/disc-signatures.yaml
- tests/integration/testdata/imagesetconfigs/signatures/isc-signatures.yaml
- tests/integration/m2d_d2m_test.go
- tests/integration/image-builders/generate-cosign-keys.sh
- tests/integration/helpers_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/integration/image-builders/operator/Makefile (1)
53-68:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffMake signing loops fail-fast to prevent silent command failures.
Lines 54–58 and 62–67 run critical
cosign signoperations insideforloops without explicit fail-fast semantics. If anycosign signcommand fails mid-loop, subsequent iterations continue and the target reports success despite the failure—masking cryptographic signing errors.Proposed fix
sign-bundles: - `@for` dir in bundles/*/*-bundle-v*/; do \ + `@set` -e; \ + for dir in bundles/*/*-bundle-v*/; do \ tag=$$(basename "$$dir"); \ echo "Signing bundle $$tag"; \ COSIGN_PASSWORD="" cosign sign --key $(COSIGN_KEY) --tlog-upload=false "$(REGISTRY):$$tag"; \ done @@ sign-related: - `@for` dir in bundles/*/*-bundle-v*/; do \ + `@set` -e; \ + for dir in bundles/*/*-bundle-v*/; do \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/image-builders/operator/Makefile` around lines 53 - 68, The sign-bundles and sign-related targets run cosign sign commands inside for loops without fail-fast semantics, meaning if any cosign sign fails, the loop continues and the target still reports success, masking cryptographic signing errors. Add `set -e` as the first line inside each for loop (after the `@for dir in bundles/*/*-bundle-v*/;` part) to ensure that if any cosign sign command exits with a non-zero status, the entire loop and target will fail immediately rather than continuing to process remaining iterations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/integration/image-builders/operator/Makefile`:
- Around line 53-68: The sign-bundles and sign-related targets run cosign sign
commands inside for loops without fail-fast semantics, meaning if any cosign
sign fails, the loop continues and the target still reports success, masking
cryptographic signing errors. Add `set -e` as the first line inside each for
loop (after the `@for dir in bundles/*/*-bundle-v*/;` part) to ensure that if
any cosign sign command exits with a non-zero status, the entire loop and target
will fail immediately rather than continuing to process remaining iterations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fc1962cf-1fb2-40d5-b5c9-505b52b634a3
⛔ Files ignored due to path filters (1)
tests/integration/testdata/keys/cosign.pubis excluded by!**/*.pub
📒 Files selected for processing (9)
tests/integration/helpers_test.gotests/integration/image-builders/generate-cosign-keys.shtests/integration/image-builders/operator/Makefiletests/integration/image-builders/release/Makefiletests/integration/image-builders/release/create-release.shtests/integration/m2d_d2m_test.gotests/integration/testdata/imagesetconfigs/signatures/disc-signatures.yamltests/integration/testdata/imagesetconfigs/signatures/isc-signatures.yamltests/integration/testdata/keys/cosign.key
💤 Files with no reviewable changes (1)
- tests/integration/image-builders/release/create-release.sh
✅ Files skipped from review due to trivial changes (2)
- tests/integration/testdata/imagesetconfigs/signatures/disc-signatures.yaml
- tests/integration/testdata/imagesetconfigs/signatures/isc-signatures.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/m2d_d2m_test.go
- tests/integration/helpers_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/image-builders/release/Makefile`:
- Line 5: The `all` target in the Makefile lists `test` as a dependency, but no
corresponding `test:` target is defined in the file, which will cause make to
fail. You need to either add a new `test:` target that defines the necessary
test commands, or remove `test` from the dependency list of the `all` target if
testing is not required. Choose the appropriate solution based on whether tests
should be part of the build workflow.
In `@tests/integration/testdata/keys/cosign.key`:
- Around line 1-11: The file cosign.key contains an encrypted Sigstore private
key that has been committed to the repository. Remove this private key file from
the commit entirely. If test execution requires a key, either generate it
dynamically during test setup using the make target or CI environment, or
replace it with a public key file if only verification is needed. Ensure the
file is added to .gitignore if it needs to be generated locally but not
committed.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b26f1a1e-0a98-4527-913c-9088f34747e4
⛔ Files ignored due to path filters (1)
tests/integration/testdata/keys/cosign.pubis excluded by!**/*.pub
📒 Files selected for processing (9)
tests/integration/helpers_test.gotests/integration/image-builders/generate-cosign-keys.shtests/integration/image-builders/operator/Makefiletests/integration/image-builders/release/Makefiletests/integration/image-builders/release/create-release.shtests/integration/m2d_d2m_test.gotests/integration/testdata/imagesetconfigs/signatures/disc-signatures.yamltests/integration/testdata/imagesetconfigs/signatures/isc-signatures.yamltests/integration/testdata/keys/cosign.key
💤 Files with no reviewable changes (1)
- tests/integration/image-builders/release/create-release.sh
✅ Files skipped from review due to trivial changes (2)
- tests/integration/testdata/imagesetconfigs/signatures/isc-signatures.yaml
- tests/integration/testdata/imagesetconfigs/signatures/disc-signatures.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/integration/image-builders/generate-cosign-keys.sh
- tests/integration/helpers_test.go
- tests/integration/m2d_d2m_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/integration/image-builders/operator/Makefile (1)
53-68: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd fail-fast semantics in signing loops.
sign-bundlesandsign-relatedcan continue after a failedcosign sign, which may mask partial-signing failures.Suggested fix
sign-bundles: - `@for` dir in bundles/*/*-bundle-v*/; do \ + `@set` -e; \ + for dir in bundles/*/*-bundle-v*/; do \ tag=$$(basename "$$dir"); \ echo "Signing bundle $$tag"; \ COSIGN_PASSWORD="" cosign sign --key $(COSIGN_KEY) --tlog-upload=false "$(REGISTRY):$$tag"; \ done @@ sign-related: - `@for` dir in bundles/*/*-bundle-v*/; do \ + `@set` -e; \ + for dir in bundles/*/*-bundle-v*/; do \ bundle_tag=$$(basename "$$dir"); \ tag=$$(echo "$$bundle_tag" | sed 's/-bundle//'); \ echo "Signing related image $$tag"; \ COSIGN_PASSWORD="" cosign sign --key $(COSIGN_KEY) --tlog-upload=false "$(REGISTRY):$$tag"; \ done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/image-builders/operator/Makefile` around lines 53 - 68, The sign-bundles and sign-related targets do not have fail-fast semantics, allowing the loops to continue executing even when cosign sign fails for a bundle. Add error checking to both targets by including set -e at the beginning of the shell command block (after the @ symbol) to ensure the entire command fails immediately if any cosign sign invocation fails, preventing partial-signing failures from being masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/integration/image-builders/operator/Makefile`:
- Around line 53-68: The sign-bundles and sign-related targets do not have
fail-fast semantics, allowing the loops to continue executing even when cosign
sign fails for a bundle. Add error checking to both targets by including set -e
at the beginning of the shell command block (after the @ symbol) to ensure the
entire command fails immediately if any cosign sign invocation fails, preventing
partial-signing failures from being masked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2a3b1f35-020c-43b6-aa86-9a78f4af6559
⛔ Files ignored due to path filters (1)
tests/integration/testdata/keys/cosign.pubis excluded by!**/*.pub
📒 Files selected for processing (9)
tests/integration/helpers_test.gotests/integration/image-builders/generate-cosign-keys.shtests/integration/image-builders/operator/Makefiletests/integration/image-builders/release/Makefiletests/integration/image-builders/release/create-release.shtests/integration/m2d_d2m_test.gotests/integration/testdata/imagesetconfigs/signatures/disc-signatures.yamltests/integration/testdata/imagesetconfigs/signatures/isc-signatures.yamltests/integration/testdata/keys/cosign.key
💤 Files with no reviewable changes (1)
- tests/integration/image-builders/release/create-release.sh
✅ Files skipped from review due to trivial changes (1)
- tests/integration/testdata/imagesetconfigs/signatures/isc-signatures.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/integration/image-builders/generate-cosign-keys.sh
- tests/integration/m2d_d2m_test.go
- tests/integration/testdata/imagesetconfigs/signatures/disc-signatures.yaml
aguidirh
left a comment
There was a problem hiding this comment.
Thanks for this PR Adolfo, approving it.
I'm wondering if in a different PR we should cover the following:
-
Verification that signatures are actually valid after mirroring (currently we are only checking if they exist)
-
Signature verification using the public key
-
Behavior when signatures are corrupted
-
Mixed scenario: some images signed, some not
| } | ||
|
|
||
| // expectEmptyRegistry verifies that no non-catalog repos have tags remaining after a delete operation. | ||
| // expectOnlySignatureTagsRemain verifies that no non-catalog repos have tags remaining after a delete operation. |
There was a problem hiding this comment.
| // expectOnlySignatureTagsRemain verifies that no non-catalog repos have tags remaining after a delete operation. | |
| // expectOnlySignatureTagsRemain verifies that after delete, only signature tags (.sig) and catalog tags remain in the registry. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adolfo-ab, aguidirh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@adolfo-ab: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Description
Adds tests to verify that cosign signatures can be mirrored and deleted. It also adds the necessary scripts to attach the cosign signatures to test images.
Github / Jira issue:
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Expected Outcome
Please describe the outcome expected from the tests.
Summary by CodeRabbit
.sigtags may remain unconditionally; others must be catalog tags).COSIGN_KEYdefault.