Sync images from one image stream to another registry #14471

smarterclayton · 2017-06-05T01:33:27Z

Creates a command that can be used to bulk promote images from one registry to another, and accepts injected pull secrets.

mfojtik · 2017-06-05T07:33:07Z

pkg/image/api/types.go

 	// Conditions is an array of conditions that apply to the tag event list.
 	Conditions []TagEventCondition
+	// PendingCopy is set if there is a copy waiting for a layer upload on this image stream tag.
+	PendingCopy *ImageStreamTagCopy


can't be this condition?

For two step copy (definition then binary layer upload) we need a place to store the pending image metadata. Since the API is tag focused, it's ok to not allow lots of pending images. I don't want to put too much metadata in condition

mfojtik · 2017-06-05T07:33:41Z

pkg/image/api/types.go

+	// Standard object's metadata.
+	metav1.ObjectMeta
+
+	// from is an optional reference to an existing image stream tag or image to copy.


godoc: uppercase (also the other)

mfojtik · 2017-06-05T07:34:14Z

pkg/image/api/types.go

+	From *kapi.ObjectReference
+	// image is metadata that will replace the existing metadata of from, or if from
+	// is empty, will be used to create a new scratch image.
+	Image *ImageCopyMetadata


Isn't To better here? (or at least I would expect from->to ;-)

Not sure - this is more declarative than destination. so if you specify it it's "this is what I want".

mfojtik · 2017-06-05T07:35:47Z

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

 	InfraTemplateServiceBrokerServiceAccountName = "template-service-broker"
 	TemplateServiceBrokerControllerRoleName      = "system:openshift:template-service-broker"
+
+	InfraRegistryManagementControllerServiceAccountName = "registry-management-controller"


this will make my life in #14317 harder :-) is it possible to base this on top of the refactoring?

This isn't for 3.6, just putting it up while i iterate.

mfojtik · 2017-06-05T07:37:31Z

pkg/cmd/server/origin/master.go

+
+// lazyServiceAccountSecretBasicAuthStore reads the service account token from a service account
+// and uses it as basic auth credentials.
+type lazyServiceAccountSecretBasicAuthStore struct {


is this master.go good place for this to live?

it's the only place it's used right now, I don't think of this as final because it's a really inelegant way for the master to prove auth to the registry. Unfortunately, we have no other way to auth to the registry.

mfojtik · 2017-06-05T07:40:28Z

pkg/image/registry/imagestreamtag/rest.go

+		imageStreamRegistry: imageStreamRegistry,
+		sarRegistry:         sarRegistry,
+		defaultRegistry:     defaultRegistry,
+		gr:                  schema.GroupResource{Resource: "imagestreamtags", Group: "image.openshift.io"},


imageapi.Resource("imagestreamtags") ?

mfojtik · 2017-06-05T07:41:09Z

pkg/image/registry/imagestreamtag/rest.go

 	event := imageapi.LatestTaggedImage(imageStream, tag)
 	if event == nil || len(event.Image) == 0 {
 		if !allowEmptyEvent {
+			glog.V(4).Infof("did not find tag %s in image stream status tags: %#v", tag, imageStream.Status.Tags)


wish we have some pretty-printer for tags (we print the tags in several places and the Status.Tags is pretty big arr)

mfojtik · 2017-06-05T07:41:39Z

images/dockerregistry/config.yml

    - name: openshift
      options:
-        acceptschema2: false
+        acceptschema2: true


already done in #13428

i needed it to test because this only supports pushing as v2. That's temporary.

mfojtik · 2017-06-05T07:42:20Z

pkg/cmd/infra/pusher/pusher.go

+
+var (
+	longDesc = templates.LongDesc(`
+		Push an image to a new location


can you provide some examples?

This is still draft, but the use case is going to be a controller that runs a pod in your namespace that syncs your images to other repositories. So this is just the first part, still need to add some other bits:

make it easy to inject pull secrets that can be used

the controller itself

figure out how to batch this - I kind of think we want to batch all of the "syncs" in a namespace all at once into one job.

mfojtik · 2017-06-05T07:43:06Z

pkg/image/importer/client.go

 		t = r.context.InsecureTransport
 	}
 	src := *registry
+	if len(src.Scheme) == 0 {


@miminar check

mfojtik · 2017-06-05T07:43:55Z

@smarterclayton would be nice to provide some examples of this and extended test.

@miminar @dmage for the review.

dmage · 2017-06-05T08:19:58Z

pkg/image/importer/credentials.go

+		// try removing the canonical ports
+		if strings.HasSuffix(target.Host, ":443") || strings.HasSuffix(target.Host, ":80") {
+			host := strings.SplitN(target.Host, ":", 2)[0]
+			glog.V(5).Infof("Being asked for %s, trying %s without port", target, host)


Should :80 port be stripped for https?

A secret that is only served over http doesn't make much sense, but in this case it's checking whether you defined a more generic secret (i.e. you have "foo.com" but not "foo.com:443" - we should try "foo.com").

smarterclayton · 2017-06-05T12:32:34Z

It's more the declarative "I want this image". From could be "base for layers" or "base for metadata" On Jun 5, 2017, at 3:34 AM, Michal Fojtik <[email protected]> wrote: *@mfojtik* commented on this pull request. ------------------------------ In pkg/image/api/types.go <#14471 (comment)>:

@@ -406,6 +409,40 @@ type ImageStreamImage struct {

Image Image } +// ImageStreamTagCopy allows a client to create a copy of an existing image that changes +// metadata or adds a new layer. It also allows the client to create a new image (i.e. +// FROM scratch). The resulting image is stored as a tag on the stream. +type ImageStreamTagCopy struct { + metav1.TypeMeta + // Standard object's metadata. + metav1.ObjectMeta + + // from is an optional reference to an existing image stream tag or image to copy. + // If from is not set, this is assumed to create a new scratch image. + From *kapi.ObjectReference + // image is metadata that will replace the existing metadata of from, or if from + // is empty, will be used to create a new scratch image. + Image *ImageCopyMetadata Isn't To better here? (or at least I would expect from->to ;-) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14471 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pxKvALZjSrSa6uiIlx9qNkcyBu0Xks5sA695gaJpZM4Nvjzj> .

openshift-bot · 2017-06-18T15:47:08Z

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

openshift-merge-robot · 2017-07-25T19:01:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: smarterclayton

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-07-28T22:08:13Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: smarterclayton

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-07-28T22:36:13Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: smarterclayton

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

soltysh · 2017-08-04T12:20:23Z

vendor/github.com/docker/distribution/registry/client/auth/session.go

 	}
 	var addedScopes bool
 	for _, scope := range additionalScopes {
+		if hasScope(scopes, scope) {


Why you not upstreaming this?

It's now upstream and merged.

miminar · 2017-10-04T11:38:11Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+			// load the manifest
+			srcDigest := digest.Digest(srcDigestString)
+			// var contentDigest digest.Digest / client.ReturnContentDigest(&contentDigest),
+			srcManifest, err := manifests.Get(ctx, digest.Digest(srcDigest), schema2ManifestOnly)


Schema1 is served regardless of accept header or WithManifestMediaTypes option if the desired tag points to it.

miminar · 2017-10-04T11:49:44Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+						glog.V(4).Infof("Manifest exists in %s, no need to copy layers without --force", dst.ref)
+					}
+				}
+				if mustCopyLayers {


Can this go to a function/method?

Also a good candidate for parallelization.

I've actually prefer the reusing of moby's blob upload code.
But that doesn't handle s3.

I definitely don't want to take any more references to moby code. If the distribution client is insufficient containers/image should take it.

miminar · 2017-10-04T11:56:17Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+						for _, blob := range srcManifest.References() {
+							// tagging within the same registry is a no-op
+							if src.ref.Registry == dst.ref.Registry && canonicalFrom.String() == canonicalTo.String() {
+								continue


Could this be moved before the blobs loop?

Yeah, will change and make clearer

miminar · 2017-10-04T11:57:32Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+				}
+
+				// if the destination tag already has this manifest, do nothing
+				var mustCopyLayers bool


Can be set to true if source and destination repository are the same.

No, mustCopyLayers should only be true when the user requests it.

I'm sorry, I actually meant the opposite. If the source and destination repository are the same, no copy is necessary.

miminar · 2017-10-04T12:01:30Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+					// upload the each manifest
+					for _, srcManifest := range srcManifests {
+						switch srcManifest.(type) {
+						case *schema2.DeserializedManifest:


I don't really see a difference between handling schema2 and schema1.

forcing function to stop propagation of schema1. It's easy enough to add later.

miminar · 2017-10-04T12:25:22Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+
+							var options []distribution.BlobCreateOption
+							if !o.SkipMount {
+								options = append(options, client.WithMountFrom(blobSource), WithDescriptor(blob))


This MountFrom makes sense only when src and dst registries are the same.

As an optimization, pushed/skipped blobs to the same registry could be remembered and then used for MountFrom.

Registries must ignore this field when it's not valid, so it doesn't cost us anything to always send it.

Please put there at least a comment that this isn't supposed to work in most cases.

miminar · 2017-10-04T12:36:38Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+							// mount successful
+							if ebm, ok := err.(distribution.ErrBlobMounted); ok {
+								glog.V(5).Infof("Blob mounted %#v", blob)
+								if ebm.From.Digest() != blob.Digest {


Not sure if it's worth it. Docker daemon doesn't check this.

We are checking it because in a mirroring case we may have incorrect data, thus an admin may need to run --force.

miminar · 2017-10-04T12:54:03Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+			}
+			return srcManifests, srcDigest.Algorithm().FromBytes(body), nil
+		default:
+			return append(srcManifests, srcManifest), srcDigest, nil


The manifest itself needs to be modified to omit all the references filtered out. Otherwise the manifest Put will fail due to not existing references in the destination repository.

The manifest is modified.

The manifest list is modified.

Sorry, I was confused by the name srcManifest which is the input parameter. Having different name for the processed manifest list wouldn't hurt.

miminar · 2017-10-04T12:58:11Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+
+				// upload and tag the manifest
+				for _, tag := range dst.tags {
+					toDigest, err := toManifests.Put(ctx, srcManifest, distribution.WithTag(tag))


That won't help. Manifest must not include references that don't exist in the destination repository. Otherwise, verification will fail.

smarterclayton · 2017-10-05T03:26:32Z

Schema1 is served regardless of accept header or WithManifestMediaTypes option if the desired tag points to it.

Not on a registry that does dynamic transformation.

smarterclayton · 2017-10-05T03:29:34Z

That won't help. Manifest must not include references that don't exist in the destination repository. Otherwise, verification will fail.

Not sure what you are referring to?

smarterclayton · 2017-10-05T03:39:26Z

Schema1 is served regardless of accept header or WithManifestMediaTypes option if the desired tag points to it.

Ideally we'd have code for schema1 -> 2 that we would use here, then admins could use this as a migration function. Followup

smarterclayton · 2017-10-05T20:23:06Z

More comments?

miminar · 2017-10-06T08:59:03Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+%[1]s myregistry.com/myimage:latest s3://s3.amazonaws.com/<region>/<bucket>/image
+
+# Copy image to multiple locations
+%[1]s myregistry.com/myimage:latest docker.io/myrepository/myimage:{stable,dev}


👎 for the bash syntax. It's not obvious even to regular bash users.

miminar · 2017-10-06T09:03:07Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+				}
+
+				// if the destination tag already has this manifest, do nothing
+				var mustCopyLayers bool


I'm sorry, I actually meant the opposite. If the source and destination repository are the same, no copy is necessary.

miminar · 2017-10-06T09:09:07Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+
+							var options []distribution.BlobCreateOption
+							if !o.SkipMount {
+								options = append(options, client.WithMountFrom(blobSource), WithDescriptor(blob))


Please put there at least a comment that this isn't supposed to work in most cases.

miminar · 2017-10-06T09:19:10Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+			}
+			return srcManifests, srcDigest.Algorithm().FromBytes(body), nil
+		default:
+			return append(srcManifests, srcManifest), srcDigest, nil


Sorry, I was confused by the name srcManifest which is the input parameter. Having different name for the processed manifest list wouldn't hurt.

miminar · 2017-10-06T09:25:47Z

pkg/oc/cli/cmd/image/mirror/s3.go

+func (r *s3Repository) Named() reference.Named {
+	name, err := reference.ParseNamed(r.repoName)
+	if err != nil {
+		panic(err)


Can s3Driver.Repository() attempt to parse the repoName and error out to prevent this panic?

Only valid repo names are allowed. I could pass a named reference I suppose.

miminar · 2017-10-06T10:03:33Z

Not on a registry that does dynamic transformation.

Didn't know there was such a registry. Do you have a doc at hand?

Not sure what you are referring to?

Nevermind, you've already answered.

dmage · 2017-10-06T10:13:57Z

Schema1 is served regardless of accept header or WithManifestMediaTypes option if the desired tag points to it.

Not on a registry that does dynamic transformation.

Schema1 can't be upgraded to schema2. Distribution can dynamically transform schema2 into schema1 for backward compatibility: old clients (which support only schema1 manifests) are able to pull images with schema2 manifests.

smarterclayton · 2017-10-06T15:33:34Z

Schema1 can't be upgraded to schema2. Distribution can dynamically transform schema2 into schema1 for backward compatibility: old clients (which support only schema1 manifests) are able to pull images with schema2 manifests.

What is missing from schema1 that can't be recreated?

smarterclayton · 2017-10-06T20:34:53Z

Updated with latest comments, I also fixed issues with s3 and manifest lists (the wrong file was being pushed).

smarterclayton · 2017-10-07T18:14:29Z

/retest

miminar

nits

miminar · 2017-10-10T18:54:45Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+			pattern = "^" + pattern
+		}
+		if !strings.HasSuffix(pattern, "$") {
+			pattern = pattern + "$"


I don't find this intuitive. I'd expect --filter-by-os=/amd64/ to match. I can always anchor it if I want to match whole string.

OK. Will change.

miminar · 2017-10-10T19:19:31Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+
+							// if we aren't forcing upload, skip the blob copy
+							if !o.Force {
+								_, err := toBlobs.Stat(ctx, blob.Digest)


To save a few queries, a set of blobs uploaded to particular repo could be created. I'd expect some shared blobs among different manifests of a single manifest list.

Yes. I think when we parallelize this we'll want to do it.

Allows tokens that cover the scope to be reused

When Docker-Content-Digest is not specified on the response, fall back from HEAD to GET so that we can calculate the digest from the manifest content.

A new command `oc image mirror` uses the Docker registry API to read from one or more images and copy them to remote registries (without locally storing those images). The command can also copy to an S3 bucket, creating the necessary structure to form a read-only registry.

smarterclayton · 2017-10-12T23:46:00Z

Applied labels, will be adding extended tests after I cut a beta release

openshift-bot · 2017-10-14T05:19:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-10-14T23:27:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2017-10-15T02:51:49Z

Automatic merge from submit-queue.

smarterclayton force-pushed the pusher branch 2 times, most recently from 0bd3ca6 to 48a1d5b Compare June 5, 2017 04:09

mfojtik reviewed Jun 5, 2017

View reviewed changes

pkg/image/importer/client.go

t = r.context.InsecureTransport

}

src := *registry

if len(src.Scheme) == 0 {

Copy link

Contributor

mfojtik Jun 5, 2017

Choose a reason for hiding this comment

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

@miminar check

dmage reviewed Jun 5, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2017

openshift-merge-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 24, 2017

openshift-merge-robot assigned mfojtik Jul 25, 2017

openshift-merge-robot assigned soltysh Jul 25, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2017

openshift-merge-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2017

soltysh reviewed Aug 4, 2017

View reviewed changes

openshift-merge-robot added the needs-api-review label Aug 4, 2017

smarterclayton force-pushed the pusher branch from 48a1d5b to debf8ac Compare August 23, 2017 23:30

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2017

miminar suggested changes Oct 4, 2017

View reviewed changes

smarterclayton force-pushed the pusher branch from 6b24e82 to c6117fc Compare October 5, 2017 03:53

miminar reviewed Oct 6, 2017

View reviewed changes

smarterclayton force-pushed the pusher branch from c6117fc to de91b17 Compare October 6, 2017 20:34

miminar approved these changes Oct 10, 2017

View reviewed changes

miminar mentioned this pull request Oct 12, 2017

registry: Calculate schema 1 layer sizes in the registry #16776

Closed

smarterclayton added 5 commits October 12, 2017 11:40

UPSTREAM: docker/distribution: 2382: Don't double add scopes

3abce45

Allows tokens that cover the scope to be reused

UPSTREAM: docker/distribution: 2402: Allow manifest specification

46b6eb9

UPSTREAM: docker/distribution: 2384: Fallback to GET for manifest

00b30f3

When Docker-Content-Digest is not specified on the response, fall back from HEAD to GET so that we can calculate the digest from the manifest content.

bump(github.com/aws/aws-sdk-go/service/s3): add two packages

176a7a1

smarterclayton force-pushed the pusher branch from de91b17 to e04b165 Compare October 12, 2017 15:40

smarterclayton added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2017

openshift-merge-robot added the vendor-update Touching vendor dir or related files label Oct 14, 2017

openshift-merge-robot merged commit ac23330 into openshift:master Oct 15, 2017

Sync images from one image stream to another registry #14471

Sync images from one image stream to another registry #14471

Uh oh!

Conversation

smarterclayton commented Jun 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Jun 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Jun 5, 2017 via email

Uh oh!

openshift-bot commented Jun 18, 2017

Uh oh!

openshift-merge-robot commented Jul 25, 2017

Uh oh!

openshift-merge-robot commented Jul 28, 2017

Uh oh!

openshift-merge-robot commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!