fea(#1619): skopeo sync support use src or dest use oci format #1620

tanguofu · 2022-04-11T13:00:17Z

No description provided.

Signed-off-by: root <root@guofutan-NB6.tencent.com>

mtrmac

Thanks!

For now, just a quick look:

You have already identified the downside: OCI can’t represent the original format of all images. Still, that might not be an issue for some.
I feel rather strongly that the code to parse/list images from an OCI repository should exist as a public API in c/image, not here.
Is there a specific reason to use this layout (one OCI repo per registry repo)? It seems to unnecessary limit data sharing, which was the motivation for this feature in the first place.

cmd/skopeo/sync.go

mtrmac · 2022-04-11T13:32:16Z

cmd/skopeo/sync.go

+			defer indexJSON.Close()
+
+			index := &imgspecv1.Index{}
+			if err := json.NewDecoder(indexJSON).Decode(index); err != nil {


This deep knowledge of image formats really isn’t Skopeo’s business.

Instead, this should look like something very similar to containers/image#1381 ’s oci/archive.Reader: maybe an oci/layout.Reader that can just return oci/layout references (but much simpler than containers/image#1381, because it doesn’t need to maintain state for a decompressed temporary directory), and Skopeo should just consume that API to list the images.

cmd/skopeo/sync.go

mtrmac · 2022-04-11T13:35:01Z

cmd/skopeo/sync.go

+	}
+
+	if opts.preserveDigests && opts.destination == oci.Transport.Name() {
+		return errors.New("oci DESTINATION transports not support --preserve-digests options")


Suggested change

return errors.New("oci DESTINATION transports not support --preserve-digests options")

return errors.New("using the oci transport as a destination does not support --preserve-digests")

This was removed entirely?!

mtrmac · 2022-04-11T13:35:37Z

cmd/skopeo/sync.go

 	}
-	if !contains(opts.source, []string{docker.Transport.Name(), directory.Transport.Name(), "yaml"}) {
+	if !contains(opts.source, []string{docker.Transport.Name(), directory.Transport.Name(), "yaml", oci.Transport.Name()}) {
 		return errors.Errorf("%q is not a valid source transport", opts.source)
 	}

 	if len(opts.destination) == 0 {
 		return errors.New("A destination transport must be specified")
 	}
-	if !contains(opts.destination, []string{docker.Transport.Name(), directory.Transport.Name()}) {
+	if !contains(opts.destination, []string{docker.Transport.Name(), directory.Transport.Name(), oci.Transport.Name()}) {
 		return errors.Errorf("%q is not a valid destination transport", opts.destination)
 	}

-	if opts.source == opts.destination && opts.source == directory.Transport.Name() {
-		return errors.New("sync from 'dir' to 'dir' not implemented, consider using rsync instead")
+	if opts.source == opts.destination && contains(opts.source, []string{directory.Transport.Name(), oci.Transport.Name()}) {
+		return errors.Errorf("sync from '%s' to '%s' not implemented, consider using rsync instead", opts.source, opts.destination)
+	}
+
+	if opts.preserveDigests && opts.destination == oci.Transport.Name() {
+		return errors.New("oci DESTINATION transports not support --preserve-digests options")


Note to self: All of this looks increasingly clumsy; take a deeper look.

(To clarify, I meant the existing code structure — it feels like a collection of special cases disconnected from the actual implementation, and can get out of sync.)

Yeah, let’s split this up, to remove the transport list duplication, and so that imagesToCopy and destinationReference don’t grow very large.

Turn imagesToCopy into a

var sourceImageListers map[string]func(…)… = { docker.Transport.Name(): listDockerSourceImages, … }

(imagesToCopy has almost no shared code, so that can be just carved up)

and similarly destinationReference should turn into

var destinationReferenceConstructors map[string]func(…)… = { docker.Transport.Name(): createDockerDestinationReference, … }

Then these contains checks turn into

sourceImageLister, ok := sourceImageListers[opts.source] if !ok { fail }

That feels like a necessary but arguably unrelated cleanup — I can work on that in a separate PR, if you’d like.

cmd/skopeo/sync.go

tanguofu · 2022-04-12T03:21:26Z

use library/mysql library/registry as dir name, then it will put the offline registry 127.0.0.1:5000 with same docker image namespace. keep the images name not change also can avoid same image;tag name conflict.

I’m not sure what conflict you mean. Note that the OCI image reference annotations are not limited to Docker-like tags; library/mysql:42.43.47 is a valid OCI image annotation, so oci:directory-path:library/mysql:42.43.47 is a valid way to refer to an image using the oci: transport. In particular, oci:directory:library/mysql:latest and oci:directory:library/registry:latest don’t conflict.

as your mention that OCI can’t represent the original format of all images, could u explain or give some case that oci
incompatible with docker image format.

Docker schema1/schema2 images (i. e. any images not already in the OCI format — most images in the wild, AFAIK) will be converted to OCI. That can preserve all features fine (except for health checks, I think) — but it inevitably changes the binary representation, and the image’s manifest digest. So a Docker schema2 image library/mysql@sha256:$digestvalue would, after transferring using the OCI format, have a different digest at the destination.

…ll the images within one directory Signed-off-by: root <root@guofutan-NB6.tencent.com>

Signed-off-by: root <root@guofutan-NB6.tencent.com>

mtrmac · 2022-04-21T16:56:22Z

cmd/skopeo/utils.go

@@ -100,6 +102,62 @@ type dockerImageOptions struct {
 	noCreds             bool                       // Access the registry anonymously
 }

+// ociCryptOptions collects CLI flags specific to the encryptLayer and keys of encryption or decryption,


Maybe

Suggested change

// ociCryptOptions collects CLI flags specific to the encryptLayer and keys of encryption or decryption,

// ociCryptOptions collects CLI flags related to OCI image encryption/decryption

encryptLayer is defined by this struct, so ociCryptOptions … related to encryptLayer is somewhat circular.

mtrmac · 2022-04-21T16:58:03Z

cmd/skopeo/utils.go

+// encryption-key and decryption-key cannot be specified together,
+// if set encryptLayer then encryptionKeys must be setted.


(Absolutely non-blocking: These might be better as comments on the individual fields. It would also be nice to be consistent about usage of --encryption-key (option name) vs encryptionKeys (struct field name) references.)

mtrmac · 2022-04-21T16:59:31Z

cmd/skopeo/utils.go

+	decryptionKeys []string // Keys needed to decrypt the image
+}
+
+func cryptFlags() (pflag.FlagSet, *ociCryptOptions) {


Suggested change

func cryptFlags() (pflag.FlagSet, *ociCryptOptions) {

func ociCryptFlags() (pflag.FlagSet, *ociCryptOptions) {

would be a tiny bit more consistent with the other CLI helpers.

mtrmac · 2022-04-21T17:04:50Z

cmd/skopeo/utils.go

+	}
+	// the directory holding the image must be created here
+	if err = os.MkdirAll(destination, 0755); err != nil {
+		return errors.Wrapf(err, "Error creating directory for image %s", destination)


Given the “Refusing to overwrite” behavior and this text saying “for image %s”, this seems unlikely to be reused for anything else — it is rather specific to the dir: transport; so I’m not sure that splitting this out is worthwhile.

I guess it depends on if we can find a good clear name for this behavior. checkExistAndMkdir is not that — that feels like two completely separate operations collected together, and checkExist doesn’t indicate anything about the consequences. So, for now it seems to me that leaving this in-line is much easier than finding a good name for a separate function, but there might well be a great name / way to express this that I’m missing.

mtrmac · 2022-04-21T17:05:16Z

docs/skopeo-sync.1.md

@@ -19,14 +19,15 @@ One of the problems of prefixing a destination with its transport is that, the r
 Available _source_ transports:
 - _docker_ (i.e. `--src docker`): _source_ is a repository hosted on a container registry (e.g.: `registry.example.com/busybox`).
 If no image tag is specified, skopeo sync copies all the tags found in that repository.
- - _dir_ (i.e. `--src dir`): _source_ is a local directory path (e.g.: `/media/usb/`). Refer to skopeo(1) **dir:**_path_ for the local image format.
+ - _dir_ (i.e. `--src dir`): _source_ is a local c path (e.g.: `/media/usb/`). Refer to skopeo(1) **dir:**_path_ for the local image format.


This change seems to be a typo.

mtrmac · 2022-04-21T17:17:37Z

cmd/skopeo/sync.go

+				// docker -> dir,docker,oci, trim domain
+				dockerRef := ref.DockerReference()
+				tagref, _ := reference.TagNameOnly(dockerRef).(reference.Tagged)
+				destSuffix = fmt.Sprintf("%s:%s", reference.Path(dockerRef), tagref.Tag())


Why is any of this changing at all? We can’t break existing scripts for docker -> dir and docker-> docker copies. Even excepting that, I don’t see why OCI would want to trim the registry domain: skopeo sync --src docker --dest oci quay.io/foo/bar:tag ./oci-archive can, AFAICS, use a quay.io/foo/bar:tag name as the annotation name.

(Also, the source could possibly be a digested reference, where the reference.Tagged cast would crash.)

There are some RFEs about more control of the source/destination name mapping, but that feels quite orthogonal to this PR about OCI support.

mtrmac · 2022-04-21T17:24:20Z

cmd/skopeo/sync.go

@@ -166,36 +176,32 @@ func parseRepositoryReference(input string) (reference.Named, error) {
 // destinationReference creates an image reference using the provided transport.


Something like

Suggested change

// destinationReference creates an image reference using the provided transport.

// destinationReference creates an image reference, combining the provided transport, the user-specified destination, and destSuffix containing a part of the source image’s name that should be used in the destination name.

otherwise the purpose of destSuffix is not obvious.

Possibly the order of arguments should then be changed to transport, destination, destSuffix so that it matches the usual order of the output.

mtrmac · 2022-04-21T17:26:17Z

cmd/skopeo/sync.go

-	if err != nil {
-		return nil, errors.Wrapf(err, "Cannot obtain a valid image reference for transport %q and reference %q", imageTransport.Name(), destination)
+	if destRefErr != nil {
+		return nil, errors.Wrapf(destRefErr, "Cannot obtain a valid image reference for transport %q and reference %q", transport, destString)


destString is not actually used on the OCI path … I guess this should list all of transport, destination, destSuffix individually..

mtrmac · 2022-04-21T17:29:24Z

cmd/skopeo/sync.go

 	}

+	logrus.Debugf("Destination for transport %q: %s", transport, destRef.StringWithinTransport())


This can now be just "Destination: %s", transports.ImageName(destRef) … or maybe this log line can be omitted entirely, because that value is already listed in fromToFields.

Originally, the log line was showing the input to ParseReference. The new equivalent would be showing (transport, destination, destSuffix), before the switch starts to interpret them.

mtrmac · 2022-04-21T17:31:15Z

cmd/skopeo/sync.go

+	}
+
+	if opts.preserveDigests && opts.destination == oci.Transport.Name() {
+		return errors.New("oci DESTINATION transports not support --preserve-digests options")


This was removed entirely?!

mtrmac

A full review now; #1620 (comment) is still the major structural blocker.

(Also note that tests are failing; see the logs, something like gofmt -s -w cmd/skopeo/copy.go might help.)

github-actions · 2022-05-22T00:00:49Z

A friendly reminder that this PR had no activity for 30 days.

github-actions · 2023-01-11T00:00:58Z

A friendly reminder that this PR had no activity for 30 days.

tanguofu force-pushed the main branch from 74def32 to 677a3a9 Compare April 11, 2022 13:01

fea(containers#1619): skopeo sync support use src or dest use oci format

677a3a9

Signed-off-by: root <root@guofutan-NB6.tencent.com>

mtrmac reviewed Apr 11, 2022

View reviewed changes

mtrmac linked an issue Apr 11, 2022 that may be closed by this pull request

fea: skopeo sync support use src or dest use oci format #1619

Closed

root added 2 commits April 14, 2022 17:27

fea(containers#1619): refactor the sync oci format implement, store a…

c42c292

…ll the images within one directory Signed-off-by: root <root@guofutan-NB6.tencent.com>

fea(containers#1619): fix destinationReference NewReference nil bugs

ea0e8a5

Signed-off-by: root <root@guofutan-NB6.tencent.com>

mtrmac reviewed Apr 21, 2022

View reviewed changes

mtrmac mentioned this pull request Apr 21, 2022

[Feature] make SplitDockerDomain public containers/image#1522

Closed

github-actions bot added the stale-pr label May 22, 2022

mtrmac added the kind/feature A request for, or a PR adding, new functionality label Dec 7, 2022

github-actions bot removed the stale-pr label Dec 8, 2022

github-actions bot added the stale-pr label Jan 11, 2023

github-actions bot closed this Jan 12, 2024

mtrmac mentioned this pull request Jan 13, 2024

fea: skopeo sync support use src or dest use oci format #1619

Closed

github-actions bot added the locked - please file new issue/PR label Apr 12, 2024

github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fea(#1619): skopeo sync support use src or dest use oci format #1620

fea(#1619): skopeo sync support use src or dest use oci format #1620

tanguofu commented Apr 11, 2022

mtrmac left a comment

mtrmac Apr 11, 2022

mtrmac Apr 11, 2022

mtrmac Apr 21, 2022

mtrmac Apr 11, 2022

mtrmac Apr 11, 2022

mtrmac Apr 21, 2022

tanguofu commented Apr 12, 2022 •

edited by mtrmac

Loading

mtrmac Apr 21, 2022

mtrmac Apr 21, 2022

mtrmac Apr 21, 2022

mtrmac Apr 21, 2022

mtrmac Apr 21, 2022

mtrmac Apr 21, 2022

mtrmac Apr 21, 2022

mtrmac Apr 21, 2022

mtrmac Apr 21, 2022

mtrmac Apr 21, 2022

mtrmac left a comment

github-actions bot commented May 22, 2022

github-actions bot commented Jan 11, 2023

	return errors.New("oci DESTINATION transports not support --preserve-digests options")
	return errors.New("using the oci transport as a destination does not support --preserve-digests")

	// ociCryptOptions collects CLI flags specific to the encryptLayer and keys of encryption or decryption,
	// ociCryptOptions collects CLI flags related to OCI image encryption/decryption

		// encryption-key and decryption-key cannot be specified together,
		// if set encryptLayer then encryptionKeys must be setted.

	func cryptFlags() (pflag.FlagSet, *ociCryptOptions) {
	func ociCryptFlags() (pflag.FlagSet, *ociCryptOptions) {

		@@ -166,36 +176,32 @@ func parseRepositoryReference(input string) (reference.Named, error) {
		// destinationReference creates an image reference using the provided transport.

		}

		logrus.Debugf("Destination for transport %q: %s", transport, destRef.StringWithinTransport())

fea(#1619): skopeo sync support use src or dest use oci format #1620

fea(#1619): skopeo sync support use src or dest use oci format #1620

Conversation

tanguofu commented Apr 11, 2022

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanguofu commented Apr 12, 2022 • edited by mtrmac Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

github-actions bot commented May 22, 2022

github-actions bot commented Jan 11, 2023

tanguofu commented Apr 12, 2022 •

edited by mtrmac

Loading