Fix duplicate .att/.sig OCI layers for same digest type hints#1601
Fix duplicate .att/.sig OCI layers for same digest type hints#1601ab-ghosh wants to merge 1 commit intotektoncd:mainfrom
Conversation
tekton-robot
added
the
size/L
ab-ghosh
force-pushed
the
fix-duplicate-oci-layers-1596
branch
from
84b9ba9 to
0151950
Compare
|
/approve |
tekton-robot
added
the
approved
anithapriyanatarajan
removed
the
approved
|
/kind bug |
|
/cc @jkhelil |
| continue | ||
| } | ||
| if seen[dgst.DigestStr()] { | ||
| continue |
There was a problem hiding this comment.
NIT - Adding a log line for eg. logger.Debugf("Skipping duplicate digest %s", dgst.DigestStr()) would help people understand why an image wasn't processed.
There was a problem hiding this comment.
Thanks for the suggestion, addressed.
| continue | ||
| } | ||
| if seen[dgst.DigestStr()] { | ||
| continue |
tekton-robot
added
the
approved
ab-ghosh
force-pushed
the
fix-duplicate-oci-layers-1596
branch
from
0151950 to
7d970c4
Compare
|
/lgtm |
|
@infernus01: changing LGTM is restricted to collaborators 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 kubernetes/test-infra repository. |
pkg/artifacts/signable.go
Outdated
| logger.Errorf("error getting digest: %v", err) | ||
| continue | ||
| } | ||
| if seen[dgst.DigestStr()] { |
There was a problem hiding this comment.
@ab-ghosh dgst.DigestStr() returns only sha256:xxx, stripping the repository name entirely. This means two legitimately different images that happen to produce the same content digest (e.g., a mirrored image gcr.io/my/img@sha256:abc and quay.io/my/img@sha256:abc) will be silently deduplicated, dropping one of them from the signing set. so could you consider dgst.Name() that uses the full repo@SHA256:XYZ. Also please include test case addressing this.
There was a problem hiding this comment.
Yeah, that's right, thanks for the suggestion. I have updated it to use dgst.Name() and also added a test case to cover this scenario.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: infernus01 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 |
tekton-robot
removed
the
approved
When a Task declares the same OCI image through multiple type-hint
formats (IMAGE_URL/IMAGE_DIGEST and IMAGES), Chains produces duplicate
layers in cosign .att/.sig manifests. This adds dedup at two levels:
1. Extraction: track seen digests in ExtractOCIImagesFromResults to
avoid returning the same digest from different type-hint formats
2. Storage: check existing layers in AttestationStorer and SimpleStorer
before appending, preventing duplicates from independent signable
types (TaskRunArtifact + OCIArtifact) storing to the same tag
Also adds debug logging when existing layer checks fail instead of
silently swallowing errors.
Fixes tektoncd#1596
ab-ghosh
force-pushed
the
fix-duplicate-oci-layers-1596
branch
from
7d970c4 to
6a97a81
Compare
4 participants
Changes
When a Task declares the same OCI image through multiple type-hint formats, produces duplicate layers in the
.attand/or.sigmanifests.This fixes the issue at two levels:
seenmap inExtractOCIImagesFromResultsto skip already-extracted digests across all type-hint parsing loops.AttestationStorer.StoreandSimpleStorer.Store, check if a layer with the same content digest already exists. This handles the case where independent signable types (TaskRunArtifactandOCIArtifact) both store to the same.att/.sigtag.Fixes #1596
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes