feat(sync): use regclient for sync extension #2524
Conversation
eusebiu-constantin-petu-dbk
force-pushed
the
sync_regclient
branch
2 times, most recently
from
7dcb55f to
ac000c0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2524 +/- ##
==========================================
- Coverage 92.82% 92.80% -0.02%
==========================================
Files 169 163 -6
Lines 22304 21646 -658
==========================================
- Hits 20703 20089 -614
+ Misses 998 965 -33
+ Partials 603 592 -11 ☔ View full report in Codecov by Sentry. |
eusebiu-constantin-petu-dbk
requested review from
rchincha and
andaaron
as code owners
eusebiu-constantin-petu-dbk
force-pushed
the
sync_regclient
branch
2 times, most recently
from
23a0ab2 to
4d75aa8
Compare
| } | ||
|
|
||
| // this function will check if tag is a cosign tag (signature or sbom). | ||
| func isCosignTag(tag string) bool { |
There was a problem hiding this comment.
I think we already have a function like this in the common package and another in the gc package. Could we consolidate?
There was a problem hiding this comment.
it doesn't check for sboms :D
| return nil | ||
| } | ||
| // we want referrers using sha-<digest>.*" tags | ||
| // service.copyOptions = append(service.copyOptions, regclient.ImageWithDigestTags()) |
There was a problem hiding this comment.
Why is this commented out?
There was a problem hiding this comment.
If I let both options enabled, regclient will overwrite some of the referrers.
Because regclient will create tags for all referrers it copies, even if there's no referrer tag on upstream.
I think this issue arises only if the upstream is older zot, or the upstream supports both digest referrers and subject referrers.
I'm not 100% percent sure what is the exact behaviour though.
I will try to fully grasp it.
There was a problem hiding this comment.
You mean zot manages the referrers tag internally, and updates it every time we copy something to the storage? In that case I think we should not copy it from upstream.
| case mediatype.Docker2ManifestList: | ||
| buf, desc, err = convertDockerListToOCI(ctx, man, imageReference, registry.client) | ||
| isConverted = true | ||
| case mediatype.OCI1Manifest, mediatype.OCI1ManifestList: |
There was a problem hiding this comment.
Off topic: let's make sure when we sync we don't unmarshall/marshal the manifest when writing to the destination.
Currently there's an open bug that sync reorders the fields in the image manifest, resulting in a different digest, and broken referrers.
There was a problem hiding this comment.
This is only for docker images we don't do any marshal unmarshal for oci.
For docker images I guess it's normal that referrers are lost in process of converting.
There was a problem hiding this comment.
I want to make sure that by switching to regclient this will also be resolved #2417
eusebiu-constantin-petu-dbk
force-pushed
the
sync_regclient
branch
2 times, most recently
from
0f45962 to
c13fbb9
Compare
replaced containers/image package with regclient/regclient package Signed-off-by: Petu Eusebiu <[email protected]> rebased Signed-off-by: Petu Eusebiu <[email protected]>
eusebiu-constantin-petu-dbk
force-pushed
the
sync_regclient
branch
from
c13fbb9 to
b4bab03
Compare
replaced containers/image package with regclient/regclient package
What type of PR is this?
Which issue does this PR fix:
What does this PR do / Why do we need it:
If an issue # is not available please add repro steps and logs showing the issue:
Testing done on this change:
Automation added to e2e:
Will this break upgrades or downgrades?
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.