OCPBUGS-65718: Fixes the test-integration when running on ppc64le and arm64#1431
OCPBUGS-65718: Fixes the test-integration when running on ppc64le and arm64#1431prb112 wants to merge 4 commits into
Conversation
Signed-off-by: Paul Bastide <pbastide@redhat.com>
Signed-off-by: Paul Bastide <pbastide@redhat.com>
openshift-ci-robot
added
jira/valid-reference
|
@prb112: This pull request references Jira Issue OCPBUGS-65718, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 manifest-digest discovery and per-manifest signature-image creation and pushes; integrates these into the test image generator to produce signatures for every manifest in component and release images; anchors integration test selection patterns in the Makefile. ChangesMulti-manifest signature generation
Sequence Diagram(s) sequenceDiagram
participant Generator as GenerateReleaseAndComponents
participant Resolver as GetAllManifestDigests
participant Signer as PushSignatureImage
participant Registry as remote.Write
Generator->>Resolver: resolve image ref -> list digests
Resolver-->>Generator: return digest list
loop per digest
Generator->>Signer: build & tag signature sha256-<digest>.sig
Signer->>Registry: remote.Write(signature image)
end
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/cc @KaushikOP He is double-checking |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: prb112 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/testutils/testutils.go`:
- Around line 184-191: The code currently swallows errors from desc.ImageIndex()
and idx.IndexManifest() by returning digests, nil which masks manifest-list
resolution failures; change both error paths to fail fast by returning nil and
the encountered error (propagate the error) instead of returning the partial
digests slice so callers see the failure (update the branches that handle err
after desc.ImageIndex() and after idx.IndexManifest() to return nil, err).
🪄 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: 447e6940-d2d4-4a72-9be8-15505f0fcfd2
📒 Files selected for processing (2)
Makefileinternal/testutils/testutils.go
| idx, err := desc.ImageIndex() | ||
| if err != nil { | ||
| return digests, nil // Return what we have | ||
| } | ||
|
|
||
| manifest, err := idx.IndexManifest() | ||
| if err != nil { | ||
| return digests, nil |
There was a problem hiding this comment.
Fail fast when index resolution breaks.
Returning digests, nil here masks manifest-list resolution failures and lets callers continue with only a partial signature set. For multi-arch images that means the top-level digest gets a .sig tag while instance digests are silently skipped.
Suggested fix
if desc.MediaType.IsIndex() {
idx, err := desc.ImageIndex()
if err != nil {
- return digests, nil // Return what we have
+ return nil, fmt.Errorf("resolve image index for %s: %w", imgRef, err)
}
manifest, err := idx.IndexManifest()
if err != nil {
- return digests, nil
+ return nil, fmt.Errorf("load index manifest for %s: %w", imgRef, err)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/testutils/testutils.go` around lines 184 - 191, The code currently
swallows errors from desc.ImageIndex() and idx.IndexManifest() by returning
digests, nil which masks manifest-list resolution failures; change both error
paths to fail fast by returning nil and the encountered error (propagate the
error) instead of returning the partial digests slice so callers see the failure
(update the branches that handle err after desc.ImageIndex() and after
idx.IndexManifest() to return nil, err).
prb112
force-pushed
the
OCPBUGS-65718-multi-arch
branch
from
7636b29 to
4f3dd32
Compare
openshift-ci
Bot
added
the
needs-rebase
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/pkg/archive/unarchive_test.go (1)
84-119: ⚖️ Poor tradeoffConsider using the same deterministic approach as TestUnArchiver_CacheDirError.
The current test attempts to create directories under
/which may behave differently across environments (containers, rootless podman, etc.). While the flexible error assertions with substring matching help, the test could be more reliable by following the pattern used inTestUnArchiver_CacheDirError(lines 136-145): create a read-only parent directory within the test's temp space instead of relying on system-level permission restrictions.For example:
- Create
readOnlyParent := filepath.Join(testFolder, "readonly")- Set permissions to 0555
- Try to create working dir inside:
filepath.Join(readOnlyParent, "working-dir")- Assert error contains "unable to create working dir"
This would make the test consistently fail in the expected way regardless of the execution environment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pkg/archive/unarchive_test.go` around lines 84 - 119, Replace the brittle root-level path usage in TestUnArchiver_WorkingDirError with the deterministic pattern used in TestUnArchiver_CacheDirError: create a read-only parent directory inside the test temp dir (e.g., readOnlyParent := filepath.Join(testFolder, "readonly")), chmod it to 0555, then pass filepath.Join(readOnlyParent, "working-dir") as the workingDir argument to NewArchiveExtractor (after calling prepareDummyTarWorkingDir as before); call o.Unarchive() and assert the returned error contains "unable to create working dir" to make the test reliably fail due to permission errors rather than environment-dependent root behavior.internal/pkg/archive/unarchive.go (1)
168-173: 💤 Low valueMinor documentation improvement for clarity.
The comment on line 169 states "unless it's the root directory which already ends with separator on Unix," but the code actually checks whether
absDirends with a path separator regardless of whether it's the root directory. While the behavior is correct (theHasSuffixcheck handles any directory that already has a trailing separator), the comment could be more accurate.Consider revising to: "Ensure absDir ends with a path separator for proper prefix checking, unless it already has one."
The path-sanitization logic itself is sound and correctly prevents path traversal attacks via canonicalization and prefix validation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pkg/archive/unarchive.go` around lines 168 - 173, Update the comment above the path-normalization block to more accurately describe behavior: replace the existing line referencing "unless it's the root directory which already ends with separator on Unix" with a concise note like "Ensure absDir ends with a path separator for proper prefix checking, unless it already has one." Keep the code using strings.HasSuffix(absDir, string(os.PathSeparator)), the subsequent append of os.PathSeparator, and the prefix check using strings.HasPrefix(absV, absDir) / absV == strings.TrimSuffix(absDir, string(os.PathSeparator)) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/pkg/archive/unarchive_test.go`:
- Around line 84-119: Replace the brittle root-level path usage in
TestUnArchiver_WorkingDirError with the deterministic pattern used in
TestUnArchiver_CacheDirError: create a read-only parent directory inside the
test temp dir (e.g., readOnlyParent := filepath.Join(testFolder, "readonly")),
chmod it to 0555, then pass filepath.Join(readOnlyParent, "working-dir") as the
workingDir argument to NewArchiveExtractor (after calling
prepareDummyTarWorkingDir as before); call o.Unarchive() and assert the returned
error contains "unable to create working dir" to make the test reliably fail due
to permission errors rather than environment-dependent root behavior.
In `@internal/pkg/archive/unarchive.go`:
- Around line 168-173: Update the comment above the path-normalization block to
more accurately describe behavior: replace the existing line referencing "unless
it's the root directory which already ends with separator on Unix" with a
concise note like "Ensure absDir ends with a path separator for proper prefix
checking, unless it already has one." Keep the code using
strings.HasSuffix(absDir, string(os.PathSeparator)), the subsequent append of
os.PathSeparator, and the prefix check using strings.HasPrefix(absV, absDir) /
absV == strings.TrimSuffix(absDir, string(os.PathSeparator)) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 86422770-4a5d-4766-bf44-a55bb75bf6aa
📒 Files selected for processing (3)
Makefileinternal/pkg/archive/unarchive.gointernal/pkg/archive/unarchive_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
Signed-off-by: Paul Bastide <pbastide@us.ibm.com>
prb112
force-pushed
the
OCPBUGS-65718-multi-arch
branch
from
4f3dd32 to
08496b1
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/testutils/testutils.go (1)
450-451: Use the shared digest-tag formatting in test signature generation (no hardcodedsha256:prefix).
internal/testutils/testutils.go’sPushSignatureImagebuilds the signature tag by hardcodingsha256:viastrings.TrimPrefix, while production usesinternal/pkg/signature.SigstoreAttachmentTagto validate the digest and format tags based on the digest algorithm. Switch the test helper to the shared logic (ordigest.Parse+ the same formatting) to avoid generating incorrect signature tags for non-sha256/malformed digests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/testutils/testutils.go` around lines 450 - 451, PushSignatureImage currently builds signature tags by trimming a hardcoded "sha256:" prefix and formatting "sha256-%s.sig", which breaks for non-sha256 or malformed digests; update PushSignatureImage to reuse the shared formatting logic instead: either call internal/pkg/signature.SigstoreAttachmentTag with the parsed digest or use digest.Parse to parse imgDigest and then format the tag using the digest algorithm and hex (same format SigstoreAttachmentTag produces). Replace the strings.TrimPrefix/fmt.Sprintf lines with that shared/parsed-digest approach so tests generate tags consistent with production validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/testutils/testutils.go`:
- Line 467: The call to crane.Image(sigContent) ignores the returned error
causing potential nil img panics; change img, _ := crane.Image(sigContent) to
capture the error (e.g., img, err := crane.Image(sigContent)) and handle it
appropriately in the surrounding function (return the error, propagate it, or
fail the test using t.Fatalf/t.Errorf), ensuring any failure to create the image
with sigContent is reported rather than swallowed.
---
Nitpick comments:
In `@internal/testutils/testutils.go`:
- Around line 450-451: PushSignatureImage currently builds signature tags by
trimming a hardcoded "sha256:" prefix and formatting "sha256-%s.sig", which
breaks for non-sha256 or malformed digests; update PushSignatureImage to reuse
the shared formatting logic instead: either call
internal/pkg/signature.SigstoreAttachmentTag with the parsed digest or use
digest.Parse to parse imgDigest and then format the tag using the digest
algorithm and hex (same format SigstoreAttachmentTag produces). Replace the
strings.TrimPrefix/fmt.Sprintf lines with that shared/parsed-digest approach so
tests generate tags consistent with production validation.
🪄 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: ad1237bc-1c9c-44cf-9f7e-08e91ff23c8f
📒 Files selected for processing (2)
Makefileinternal/testutils/testutils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@prb112: This pull request references Jira Issue OCPBUGS-65718, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
| $(GO) test $(GO_MOD_FLAGS) $(GO_BUILD_FLAGS) -coverprofile=tests/results-integration/cover-additional.out -race -count=1 ./internal/pkg/... -run TestIntegrationAdditional | ||
| $(GO) test $(GO_MOD_FLAGS) $(GO_BUILD_FLAGS) -coverprofile=tests/results-integration/cover-release.out -race -count=1 ./internal/pkg/... -run TestIntegrationRelease | ||
| $(GO) test $(GO_MOD_FLAGS) $(GO_BUILD_FLAGS) -coverprofile=tests/results-integration/cover-additional.out -race -count=1 ./internal/pkg/... -run 'TestIntegrationAdditional$$' | ||
| $(GO) test $(GO_MOD_FLAGS) $(GO_BUILD_FLAGS) -coverprofile=tests/results-integration/cover-release.out -race -count=1 ./internal/pkg/... -run 'TestIntegrationRelease$$' |
There was a problem hiding this comment.
Avoids running the M2M twice
|
@prb112: This pull request references Jira Issue OCPBUGS-65718, which is valid. 3 validation(s) were run on this bug
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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/testutils/testutils.go (1)
189-192: 💤 Low valueInconsistent error suppression: line 191 lacks the
nolintdirective present on line 186.Line 186 documents the intentional error-ignoring behavior with
//nolint:nilerr,errcheck, but line 191 does the samereturn digests, nilonIndexManifest()failure without the directive. For consistency and to satisfy linters, either add the same nolint annotation here or propagate the error as originally suggested.Option A: Add nolint for consistency
manifest, err := idx.IndexManifest() if err != nil { - return digests, nil + return digests, nil //nolint:nilerr,errcheck // Return what we have }Option B: Fail fast on both paths (as originally suggested)
if desc.MediaType.IsIndex() { idx, err := desc.ImageIndex() if err != nil { - return digests, nil //nolint:nilerr,errcheck // Return what we have + return nil, fmt.Errorf("resolve image index for %s: %w", imgRef, err) } manifest, err := idx.IndexManifest() if err != nil { - return digests, nil + return nil, fmt.Errorf("load index manifest for %s: %w", imgRef, err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/testutils/testutils.go` around lines 189 - 192, The error return from idx.IndexManifest() in the block using manifest and digests is suppressing the error by returning (digests, nil) but lacks the matching nolint directive present earlier; update the handler for IndexManifest() failure to match the existing behavior by adding the same comment directive (//nolint:nilerr,errcheck) on the return path or, alternatively, propagate the error instead of returning nil—locate the IndexManifest() call and its surrounding block (variables manifest, err, digests) and apply the same chosen fix consistently as was used on the earlier similar return.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/testutils/testutils.go`:
- Around line 189-192: The error return from idx.IndexManifest() in the block
using manifest and digests is suppressing the error by returning (digests, nil)
but lacks the matching nolint directive present earlier; update the handler for
IndexManifest() failure to match the existing behavior by adding the same
comment directive (//nolint:nilerr,errcheck) on the return path or,
alternatively, propagate the error instead of returning nil—locate the
IndexManifest() call and its surrounding block (variables manifest, err,
digests) and apply the same chosen fix consistently as was used on the earlier
similar return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 26d1c280-551b-4dd5-8f1a-1ffff05484e8
📒 Files selected for processing (1)
internal/testutils/testutils.go
prb112
force-pushed
the
OCPBUGS-65718-multi-arch
branch
3 times, most recently
from
d606358 to
93a51d5
Compare
Signed-off-by: Paul Bastide <pbastide@us.ibm.com>
prb112
force-pushed
the
OCPBUGS-65718-multi-arch
branch
from
93a51d5 to
2711e05
Compare
|
Hey @aguidirh Finally got to the root cause - signatures for the other arches were missing. I've fixed the code with the assistance of 'IBM Bob' Thanks Paul |
|
@prb112: 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. |
| if desc.MediaType.IsIndex() { | ||
| idx, err := desc.ImageIndex() | ||
| if err != nil { | ||
| return digests, nil //nolint:nilerr,errcheck // Return what we have |
There was a problem hiding this comment.
Is there any specific reason why the error is not returned here?
There was a problem hiding this comment.
will come back and answer tomorrow morning. Thanks Alex
|
|
||
| manifest, err := idx.IndexManifest() | ||
| if err != nil { | ||
| return digests, nil //nolint:nilerr,errcheck |
There was a problem hiding this comment.
Is there any specific reason why the error is not returned here?
|
/assign @prb112 |
|
@prb112 could you please have a look? It is also required to sign the commits, it's a new security requirement. |
3 participants
Description
Closed
OCPBUGS-65718: multi-arch tests were not passing on ppc64le#1325 in favor of this PR.This fixes the integration tests when running on non-x86 arches:
To test the multi-arch tests:
linux/ppc64le
linux/arm64
Github / Jira issue: OCPBUGS-65718
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Tested on multiple architectures
Expected Outcome
Expect test-integration to function correctly
Summary by CodeRabbit
Tests
Chores