Skip to content
This repository has been archived by the owner on May 3, 2022. It is now read-only.

Add push and pull support using cnab-to-oci #681

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

radu-matei
Copy link
Member

This PR is the very first attempt to make things build. It is a sanity check and a reference for the all dependency versions we need in order to have Duffle and cnab-to-oci compile, and it uses my forks for cnab-to-oci, as well as a CNAB Go package extracted from Duffle in order to avoid circular dependencies.

It also implements naive push and pull commands, to make sure everything works together -- see below.

Obviously, this should not be merged in its current state.

radu:deislabs$ duffle push bundle.json -t docker.io/radumatei/duffle-push-test3
Starting to copy image cnab/helloworld:0.1.1...
Completed image cnab/helloworld:0.1.1 copy
WARN[0007] reference for unknown type: application/vnd.cnab.config.v1+json 
Pushed successfully, with digest "sha256:0ad49bc2506ed60a4f8de90703b84fe3c356b1ca0e526fe7200c3d95c4457826"

radu:deislabs$ duffle pull docker.io/radumatei/duffle-push-test3@sha256:0ad49bc2506ed60a4f8de90703b84fe3c356b1ca0e526fe7200c3d95c4457826 -o test3.json
radu:deislabs$ cat test3.json 
{
	"name": "duffle-puhs-test",
	"version": "0.1.1",
	"description": "please, no more go deps shenanigans",
	"keywords": [
		"helloworld",
		"cnab",
		"tutorial"
	],
	"maintainers": [
		{
			"name": "Jane Doe",
			"email": "[email protected]",
			"url": "https://example.com"
		}
	],
	"invocationImages": [
		{
			"imageType": "docker",
			"image": "radumatei/duffle-push-test3@sha256:a59a4e74d9cc89e4e75dfb2cc7ea5c108e4236ba6231b53081a9e2506d1197b6",
			"size": 942,
			"mediaType": "application/vnd.docker.distribution.manifest.v2+json"
		}
	],
	"images": null,
	"parameters": null,
	"credentials": null
}

cc @michelleN, @silvin-lubecki

@ghost ghost assigned radu-matei Mar 27, 2019
@ghost ghost added the review label Mar 27, 2019
@glyn
Copy link
Contributor

glyn commented Mar 27, 2019

This PR does not appear to satisfy the requirements of #668:

  • It implements a many-one mapping of repositories; issue 668 requires a one-one mapping.
  • It moves a bundle and its images around as an atomic unit; issue 668 relocates just the images of a bundle and does not require either the original or relocated bundle to be stored in a repository.

I've added an agenda item to today's CNAB community call to discuss these points.

@radu-matei
Copy link
Member Author

@glyn -- I do have one question about this discussion -- and keep in mind that I haven't followed the registry and image relocation discussions closely, so I might be asking something obvious here.

Have we decided that the requirements from #668 about image relocation (which as I recall, is a continuation of the rewrite command) are the also the requirements for the simple bundle push / pull to OCI registries?

At least at first, they look like two separate use cases, both with valid scenarios.

@glyn
Copy link
Contributor

glyn commented Mar 27, 2019

@glyn -- I do have one question about this discussion -- and keep in mind that I haven't followed the registry and image relocation discussions closely, so I might be asking something obvious here.

Have we decided that the requirements from #668 about image relocation (which as I recall, is a continuation of the rewrite command) are the also the requirements for the simple bundle push / pull to OCI registries?

At least at first, they look like two separate use cases, both with valid scenarios.

No, we certainly haven't decided that, but I wanted to point out the potential conflict and have the discussion so we are all "on the same page".

@radu-matei
Copy link
Member Author

Thanks for pointing this out, @glyn!

To sum up the discussion from the CNAB meeting earlier today, we decided that both approaches are valid, and we want to eventually support relocating the images for duffle push, perhaps under a flag.

As we are working on the two implementations and we have a clear view of how they work, we will decide if / how we want to integrate relocating images either directly in Duffle, or as part of the cnab-to-oci library used in Duffle for registry support.

Gopkg.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good, excepting the new dependency on Kubernetes 😕

@silvin-lubecki
Copy link
Contributor

@radu-matei : the PR moving to github.com/deislabs/cnab-go/bundle has already been merged on cnab-to-oci, but the move in duffle is on this PR, still in draft, and we have some vendoring issues. Could you split this PR, and extract the renaming to another PR we can merge quicker?

@silvin-lubecki
Copy link
Contributor

Done with #704

jcsirot pushed a commit to jcsirot/docker-app that referenced this pull request Apr 10, 2019
- Use a custom duffle branch using github.com/deislabs/cnab-go/bundle (waiting for cnabio/duffle#681 to be merged)
jcsirot pushed a commit to jcsirot/docker-app that referenced this pull request Apr 10, 2019
- Use a custom duffle branch using github.com/deislabs/cnab-go/bundle (waiting for cnabio/duffle#681 to be merged)

Signed-off-by: Jean-Christophe Sirot <[email protected]>
simonferquel pushed a commit to jcsirot/docker-app that referenced this pull request Apr 10, 2019
- Use a custom duffle branch using github.com/deislabs/cnab-go/bundle (waiting for cnabio/duffle#681 to be merged)

Signed-off-by: Jean-Christophe Sirot <[email protected]>
@radu-matei
Copy link
Member Author

@silvin-lubecki - absolutely, we shouldn't wait on that PR.
Thanks!

@silvin-lubecki silvin-lubecki mentioned this pull request Apr 16, 2019
chris-crone pushed a commit to jcsirot/docker-app that referenced this pull request Apr 17, 2019
- Use a custom duffle branch using github.com/deislabs/cnab-go/bundle (waiting for cnabio/duffle#681 to be merged)

Signed-off-by: Jean-Christophe Sirot <[email protected]>
@radu-matei
Copy link
Member Author

radu-matei commented Apr 21, 2019

This PR introduces an initial implementation for working with OCI registries, provided by github.com/docker/cnab-to-oci.

  • duffle push - if you logged in to an OCI registry using the Docker CLI, this command will reuse the same credentials, so you should be able to push.
$ duffle push test.json --target cnabregistry.azurecr.io/duffle-push-target:latest --bundle-is-file
Starting to copy image ubuntu:latest...
Completed image ubuntu:latest copy
WARN[0002] reference for unknown type: application/vnd.cnab.config.v1+json
Pushed successfully, with digest "sha256:8a6482e6e5778354cbd95cac49ce3cb5fd78ded0df744c889ca3b29cb131f310"

As the current implementation uses cnab-to-oci, you can read this WIP document describing the rationale behind cnab-to-oci.

Note that you can push a bundle from the bundle store, or a bundle from a file, by passing the --bundle-is-file flag.

  • duffle pull - again, this will reuse the Docker CLI credentials
$ rm -rf ~/.duffle/
$ duffle pull cnabregistry.azurecr.io/duffle-push-target:latest
$ duffle bundle list
NAME                    VERSION DIGEST
helloworld-test-push    0.1.2   8d100741027eac95a658d354d73bf090f080a318

Note that you can either pull a tag, for example cnabregistry.azurecr.io/duffle-push-target:latest, or directly pull the SHA256 - for example cnabregistry.azurecr.io/duffle-push-target@sha256:8a6482e6e5778354cbd95cac49ce3cb5fd78ded0df744c889ca3b29cb131f310

This has been tested using Azure Container Registries and Docker Hub.

Feedback needed:

  • how the repository is passed for push(the --target flag) - this PR does not follow the same tag -> push strategy the Docker CLI follows - meaning the name of the bundle (as defined by the spec) does not dictate the repository that will be used to push to (as in - you need to first tag an image and include the repository name with the Docker CLI before you can push). In its current form, the bundle will be pushed to whatever --target points to, provided you have push access.

  • what should Duffle do if the images referenced in the bundle aren't already pushed and available to copy? (@glyn might have some insight here, as he just implemented the duffle relocate command`)

Thanks!

@radu-matei radu-matei marked this pull request as ready for review April 21, 2019 20:37
@radu-matei radu-matei changed the title [WIP] Add push and pull support using cnab-to-oci Add push and pull support using cnab-to-oci Apr 21, 2019
var pull pullCmd
cmd := &cobra.Command{
Use: "pull <ref> [options]",
Short: "Pulls an image reference",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this command really pull an image reference?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be... pulls a remote bundle from an OCI registry?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thanks for catching this. Updating.

@glyn
Copy link
Contributor

glyn commented Apr 23, 2019

  • what should Duffle do if the images referenced in the bundle aren't already pushed and available to copy? (@glyn might have some insight here, as he just implemented the duffle relocate command`)

I'm not sure what the intended spec of duffle push is or should be. This needs to be made clear, preferably in the help text. Specifically:

  • Does this command operate on just the bundle itself or on referenced images too?
  • If it operates on the reference images, what assumptions does it make about their location and what does it do to them?

(duffle relocate only deals with images, but it will read them from their current repositories and write them to their relocated repositories.)

Copy link
Contributor

@glyn glyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The help text needs beefing up so that the spec of duffle push and pull is clear.

@glyn
Copy link
Contributor

glyn commented Apr 23, 2019

Obviously, this should not be merged in its current state.

Nit: have you heard of draft pull requests? :-)

@michelleN
Copy link
Contributor

@glyn Radu had in fact made this a draft pull request and changed it as ready for review in the past few days.

@glyn
Copy link
Contributor

glyn commented Apr 23, 2019

@glyn Radu had in fact made this a draft pull request and changed it as ready for review in the past few days.

Ah, I missed that. Thanks.

@michelleN michelleN self-requested a review April 24, 2019 16:09
Copy link
Contributor

@michelleN michelleN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm understanding this correctly, --target is something required. If so, I think it should be a required argument and not a flag unless we plan on using some sort of defaulting mechanism later on. This is a non-blocking comment.

@radu-matei
Copy link
Member Author

@michelleN - yes, the target flag is required. Adding this as the second argument would look like:

duffle push <bundle> <repository>:<optional-tag>

That indeed seems a better, updating.

Thanks for the feedback.

Copy link
Contributor

@michelleN michelleN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lmk when you're ready for another review @radu-matei. Heads up, I also saw the following error with $ duffle bundle list --> Error: cannot load bundle: invalid character '-' in numeric literal

Don't think that's an issue in master but I'll double check. Cheers.

@radu-matei
Copy link
Member Author

@michelleN - updated the UX with your suggestions, and improved the help messages for both commands.

That error message seems to be tied to some signed bundles (from before pulling out the signing part), and also rebased this, meaning there are no more references to those in this branch, so everything should be fixed now.

Copy link
Contributor

@glyn glyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying the spec of push. I would like a little more clarity on the handling of images by push and maybe pull.

const pushDesc = `
Pushes a CNAB bundle to an OCI registry by pushing all container
images referenced in the bundle to the target repository (all images are
pushed to the same repository, and are referenceable through their digest).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are images "pushed" from? Are they copied direct from their remote repositories or do they have to be present in a docker daemon?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they are directly copied from the remote (and actually, as #734 tracks, if the images are only present in the local daemon, this will fail)..

Pulls a CNAB bundle from an OCI repository.
The only argument for this command is the repository where
the bundle can be found, and by default, this command pulls the
bundle and stores it in the local bundle store.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this command do anything with images referenced by the bundle?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, by default it does not pull the referenced images.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m revisiting this PR after the whole OCI discussion - do you think it pulling the bundle should pull the referenced images?
Most likely they will not be used on the same machine that pulls the bundle, so not sure about this - would an optional flag for pulling the images be helpful?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense for a local development use case to pre-populate the local docker image store (just pull the bundle and everything in it and it "works"), so I agree with the optional flag.

By the way I don't think it should be a cnab-to-oci feature, as it has no knowledge of the docker engine, only duffle has. I guess it will then parse the bundle.json and call docker pull on every docker image using the docker client ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree - I wasn't thinking this should be in cnab-to-oci.

}

func newPullCmd(w io.Writer) *cobra.Command {
const usage = `Pulls a bundle from an OCI repository`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use const block

@radu-matei
Copy link
Member Author

radu-matei commented Aug 7, 2019

Rebased off #827 - haven't addressed all comments yet.
Using WIP cnabio/cnab-to-oci#61

Signed-off-by: Radu M <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants