[wip] Add option to push all tags #1099

byarbrough · 2022-07-21T19:57:02Z

If the libimage.PushOptions AllTags=true then libimage.Push() will search the local image storage for all images matching the source repository name. It will then push each tag for each of those images to the matching repository.

This seeks to mirror behavior behind docker push --all-tags IMAGE.

If AllTags=true:

the user must provide the name of the image only; providing a tag will crash
libimage.Push() will always return nil for the manifest byte array
only the docker transport is supported due to how tags are appended to names with other transports.

Requirement for containers/podman#14949
@rhatdan

Signed-off-by: Brian Yarbrough bcynmelk+git@gmail.com

openshift-ci · 2022-07-21T19:57:09Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: byarbrough
Once this PR has been reviewed and has the lgtm label, please assign baude for approval by writing /assign @baude in a comment. For more information see:The Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2022-07-22T14:57:36Z

libimage/push.go

+	if options.AllTags {
+
+		// Do not allow : for tags, other than specifying transport
+		d := strings.TrimPrefix(destination, "docker://")


This should be done with containers/image functions not via parsing of the image. @vrothberg @mtrmac PTAL

Yes, see an example here https://github.com/containers/common/blob/main/libimage/pull.go#L146

We should also error out if the destination is something else than this transport.

Assuming this intends to reuse the Push semantics:

Don’t push parsing of destination down to pushImage; do it above this code, using the existing logic (fallback to docker, and the like, getting a destRef).

Then, here, enforce destRef.Transport().Name() == dockerTransport.Transport().Name()

Afterwards, use reference.WithTag(destRef().DockerReference(), …)

But that makes it hard to reject things like repo:latest as output, because a destRef always has a tag.

Really I think PushAllTags should not even accept the docker:// syntax — after all, the c/image docker:// syntax always specifies an image, not a repo. Strictly parse the destination as a reference.ParseNormalizedNamed(), fail on !reference.IsNameOnly, and then use reference.WithTag.

(But really this is the least problematic part of the code.)

libimage/push.go

vrothberg · 2022-07-22T15:24:41Z

libimage/push.go

+	if options.AllTags {
+
+		// Do not allow : for tags, other than specifying transport
+		d := strings.TrimPrefix(destination, "docker://")


Yes, see an example here https://github.com/containers/common/blob/main/libimage/pull.go#L146

We should also error out if the destination is something else than this transport.

vrothberg · 2022-07-22T15:57:10Z

libimage/push.go

+
+		// Do not allow : for tags, other than specifying transport
+		d := strings.TrimPrefix(destination, "docker://")
+		if strings.ContainsAny(d, ":") {


This won't work in all cases, for instance, when pushing to docker://localhost:5000/....

What we need to do is to first parse the destination as done in pushImage and then call DockerReference(). The return value is a named reference which we can check for a tag as done here: https://github.com/containers/common/blob/main/libimage/normalize.go#L43

docker.ParseReference("busybox").DockerReference().String() == "docker.io/library/busybox:latest", so it’s harder than that.

vrothberg · 2022-07-22T15:58:45Z

libimage/push.go

 	if r.eventChannel != nil {
 		defer r.writeEvent(&Event{ID: image.ID(), Name: destination, Time: time.Now(), Type: EventTypeImagePush})
 	}

 	// Buildah compat: Make sure to tag the destination image if it's a
 	// Docker archive. This way, we preserve the image name.
 	if destRef.Transport().Name() == dockerArchiveTransport.Transport.Name() {
-		if named, err := reference.ParseNamed(resolvedSource); err == nil {
+		if named, err := reference.ParseNamed(destination); err == nil {


That could break things as destination may not be equal to resolvedSource.

It's probably better to do the AllTags branch below in line 121 or above in line 106. This way, we make sure to preserve the exact logic.

The writeEvent had to be moved further down.

libimage/push.go

mtrmac · 2022-07-22T15:54:15Z

libimage/push.go

+	if options.AllTags {
+
+		// Do not allow : for tags, other than specifying transport
+		d := strings.TrimPrefix(destination, "docker://")


Assuming this intends to reuse the Push semantics:

Don’t push parsing of destination down to pushImage; do it above this code, using the existing logic (fallback to docker, and the like, getting a destRef).

Then, here, enforce destRef.Transport().Name() == dockerTransport.Transport().Name()

Afterwards, use reference.WithTag(destRef().DockerReference(), …)

But that makes it hard to reject things like repo:latest as output, because a destRef always has a tag.

Really I think PushAllTags should not even accept the docker:// syntax — after all, the c/image docker:// syntax always specifies an image, not a repo. Strictly parse the destination as a reference.ParseNormalizedNamed(), fail on !reference.IsNameOnly, and then use reference.WithTag.

(But really this is the least problematic part of the code.)

libimage/push.go

mtrmac · 2022-07-22T16:06:05Z

libimage/push.go

+			return nil, fmt.Errorf("tag can't be used with --all-tags/-a")
+		}
+
+		namedRepoTags, err := image.NamedTaggedRepoTags()


Wait. What is actually the semantics of the AllTags option? Don’t say “like docker push”, actually write it out (and document for the PushOptions field), please.

AFAICS Docker’s implementation finds all local images that match the provided repository (even if there are multiple different images), and pushes all of them.

This one finds a single image, looks up the tags within that provided repository that point at that single image, and pushes only those.

That’s a completely different behavior. Which one is this intended to implement?

The former: push all images for the matched repository.

I'll take the examples provided to shore up the logic and integrate the comments within the existing Push runtime.

libimage/push.go

byarbrough · 2022-07-25T17:49:28Z

@mtrmac I really appreciate the time you put into the review. Just to make sure I'm understanding:
You recommend making a new Runtime, PushAllTags?

If so, I do think that would simplify some of the logic.

mtrmac · 2022-07-25T18:09:23Z

Just to make sure I'm understanding:
You recommend making a new Runtime, PushAllTags?

I think that would make sense, but it doesn’t resolve any of the name semantics issues in itself. WRT this API decision, I’ll defer to @vrothberg .

vrothberg · 2022-07-26T08:56:10Z

Just to make sure I'm understanding:
You recommend making a new Runtime, PushAllTags?

I think that would make sense, but it doesn’t resolve any of the name semantics issues in itself. WRT this API decision, I’ll defer to @vrothberg .

I prefer to have only Push and add an AllTags bool field as done in this PR. The source/destination arguments are intentionally left as string such that we have the flexibility.

No objections to any of the comments from @mtrmac. Especially #1099 (comment) will determine how the code should look like.

This seeks to mirror `docker push --all-tags IMAGE`. Because tags are appended to the destination, this will only work with the docker transport. Otherwise you'd get directories overwriting or with weird names. Also note that if AllTags is true then the user must provide the name of the image only; providing a tag will crash. Docker has this behavior: ``` tag can't be used with --all-tags/-a ``` Requirement for containers/podman#14949 Signed-off-by: Brian Yarbrough <bcynmelk+git@gmail.com>

1. Inverts test of AllTags bool, so that false is handled first the single image is pushed. 2. Makes use of library functions to test source formats instead of flaky string parsing. 3. Ensures that all tags across mulitple images for a given repo are pushed, instead of just for the single image. 4. Make the AllTags bool and a specified destination mutually exclusive. Note that Podman currently assumes the destination is the source if no destination is provided, rather than leaving it as an empty string. This will cause an error, and I think should be fixed in a PR to that repo since libimage.PushImage already fills in resolvedSource for an empty destination. https://github.com/containers/podman/blob/v4.2/cmd/podman/images/push.go#L131 Also note that the unit test is not able to push images to a real docker registry, so I would consider them incomplete, but don't know how to improve. Signed-off-by: Brian Yarbrough <bcynmelk+git@gmail.com>

mtrmac · 2022-07-28T19:13:10Z

libimage/push.go

+	//
+	// Start by making sure a destination was not specified, since we'll get that from the source
+	if len(destination) != 0 {
+		return nil, fmt.Errorf("`destination` should not be specified if using AllTags")


Is that a fundamental design decision, or a missing feature to be possibly added later?

(I’m perfectly fine with not implementing all possible options at first. I just want future maintainers to know what the intent was.)

I think it could be a new feature to be added later. But for right now, I didn't have anything to base that feature off of, so it was simpler to be strict about it.

libimage/push.go

mtrmac · 2022-07-28T19:19:12Z

libimage/push.go

+			return nil, err
+		}
+		for _, n := range namedTagged {
+			destWithTag := fmt.Sprintf("%s:%s", source, n.Tag())


reference.WithTag(srcNamed, n.Tag().

But I suspect the whole tag listing thing might need reworking and then this becomes unnecessary.

(Hypothetically: List all images, obtain their .Names(), parse them using ParseNormalizedNamed, use those where .Name() matches the user’s input. It’s quite possible that libimage already implements something like that, @vrothberg would know.

Alternatively, it’s of course possible to use ListImages and then do an extra filtering step, but that seems conceptually redundant.)

I looked at the WithTags option, but that returns a different type, so didn't use it.

I looked through most of the libimage functions and nothing caught my eye as a better option. I've been having a difficult time juggling the different ways to refer to a single image and its tag.

libimage/push.go

vrothberg · 2022-07-29T11:56:42Z

I won't find time today to review the PR, sorry. I'll take a detailed look on Monday.

vrothberg · 2022-08-01T09:32:19Z

libimage/push.go

 	"github.com/containers/image/v5/transports/alltransports"
 	"github.com/sirupsen/logrus"
 )

 // PushOptions allows for custommizing image pushes.
 type PushOptions struct {
 	CopyOptions
+	AllTags bool


Please add a comment to the new field.

libimage/push.go

Now AllTags will filter again on the source, so you only push to the repository specified in the original source. Push() with AllTags=true returns a nil []byte Also addresses some minor comments and adds descriptions to new fields. Signed-off-by: Brian Yarbrough <bcynmelk+git@gmail.com>

mtrmac · 2022-08-08T14:01:53Z

libimage/push.go

+			if err != nil {
+				return nil, err
+			}
+			if reference.Path(currentNamed) == reference.Path(srcNamed) {


This should be currentNamed.Name() == srcNamed.Name(), because Path does not include the registry (example.com:5000/foo and quay.io/foo would match).

mtrmac · 2022-08-08T14:05:22Z

libimage/push.go

+				// Have to use Sprintf because pushImage expects a string
+				destWithTag := fmt.Sprintf("%s:%s", source, n.Tag())
+				_, err := pushImage(ctx, img, destWithTag, options, "", r)


Conceptually I’d prefer to move the destRef := ParseImageName etc. logic from pushImage into the !AllTags case above, and have pushImage accept a types.ImageReference; then this can use docker.NewReference instead of making another round-trip via an untyped string.

But I haven’t really examined how that would work with the writeEvent use of destination, so I’m not completely sure it would be viable.

Why is this combining source and n at all? We know the repo of currentNamed and srcNamed matches, the two only differ in the tag; so even if this had to use a string, we could use n.String(), couldn’t we?

mtrmac · 2022-08-08T14:06:14Z

libimage/push.go

+}
+
+// pushImage sends a single image to be copied to the destination
+func pushImage(ctx context.Context, image *Image, destination string, options *PushOptions, resolvedSource string, r *Runtime) ([]byte, error) {


I’d prefer this to be a method on Runtime (func (r *Runtime) pushImage), but I’ll defer to @vrothberg .

mtrmac · 2022-08-08T14:08:04Z

libimage/push.go

-	}
-
-	logrus.Debugf("Pushing image %s to %s", source, destination)
+	logrus.Debugf("Pushing image %s to %s", transports.ImageName(srcRef), destination)


This would now print a fairly large c/storage reference. I’m honestly completely unsure about this — on one hand the output will be less readable, OTOH actually listing the precise image we are pushing is quite valuable.

So I’m fine with this change, just highlighting this for @vrothberg .

mtrmac · 2022-08-08T14:11:12Z

libimage/push_test.go

+		{"alpine", "docker.io/library/alpine", true},    // fail for destination
+		{"docker://docker.io/library/alpine", "", true}, // fail for transport
+		{"docker.io/library/alpine:latest", "", true},   // fail for tag
+		{"alpine:latest", "", true},                     // fail for tag


Non-blocking? The comments are better than nothing but not really recording the intent — I hand to refer back to the code to see what conditions are imposed on the destination. Something like “non-empty destination is currently rejected” / “transport:image-name references are rejected because they refer to an image, not a repo” / “source must not contain a tag” … would be nice.

mtrmac · 2022-08-08T14:13:54Z

libimage/push.go

+		return nil, fmt.Errorf("`destination` should not be specified if using AllTags")
+	}
+
+	// Make sure the source repository does not have a tag


Suggested change

// Make sure the source repository does not have a tag

// Make sure the source repository does not have a tag

// This intentionally does not use `alltransports.ParseImageName`, because the outcome of that

// refers to a single image, not a repo (e.g. it defaults to …:latest for docker:// references), which

// is not the semantics we want.

mtrmac · 2022-08-08T14:21:36Z

libimage/push.go

+
+	logrus.Debugf("Finding all images for source %s", srcNamed.Name())
+	listOptions := &ListImagesOptions{}
+	srcImages, _ := r.ListImages(ctx, []string{srcNamed.Name()}, listOptions)


Does this actually work? AFAICS this is equivalent to

srcImages = []*libimage.Image{r.LookupImage(srcNamed.Name()}

i.e. it always only finds one image. Am I missing something?

I increasingly think that the logic that chooses which images to push should have fairly robust unit tests (single image; single image with multiple tags; multiple tags pointing to different images; images with tags both in the specified repo and outside …). That’s hard to do with the current Push API, because it now doesn’t report anything about the returned images — so I guess the code to turn source into a list of (*libimage.Image, reference.Named), or some similar output data, should be turned into a separate helper function with unit tests.

(Probably irrelevant: why is it OK to ignore errors here?)

mtrmac · 2022-08-09T14:09:25Z

Just to link things together:

Podman RFE: podman push --all-tags podman#2369
Previous Buildah PR: Add --all-tags to push buildah#1380 . Note also Add --all-tags to push buildah#1380 (review) — who is actually using --all-tags and who should be using it? It feels like a bandaid.

byarbrough · 2022-08-10T17:14:38Z

Just to link things together:

* Podman RFE: [podman push --all-tags podman#2369](https://github.com/containers/podman/issues/2369)

* Previous Buildah PR: [Add --all-tags to push buildah#1380](https://github.com/containers/buildah/pull/1380) . Note also [Add --all-tags to push buildah#1380 (review)](https://github.com/containers/buildah/pull/1380#pullrequestreview-210936302) — who is actually using `--all-tags` and who _should_ be using it? It feels like a bandaid.

@mtrmac It does feel like a bandaid. Clearly, I've been struggling with the logic (partly because of newness to the codebase; I really appreciate everyone's patience and comments). But partly because it is a weird usage.

When I first started this, I thought Push() would just be a subset of Copy() functionality; the only difference being that with Push source is always and implicitly local image storage, compared to Skopeo being able to copy between two remotes. It turns out that is sort of the case, but it actually requires extensive logic.

Skopeo does have the ability to copy with --all but it actually uses containers/image/copy.go and it requires a "list" of images... I'm not even sure how to give it a list.

I recognize they are all different tools and will have different behaviors; I don't understand the difference between c/common and c/images. I can keep iterating on the comments above... or I can wait for more design discussion.

@rhatdan

mtrmac · 2022-08-10T17:25:03Z

skopeo copy --all copies a multi-arch image; that would be “a single image” with “a single tag” compared to this push --all-tags, which pushes multiple images with multiple different tags.

c/image/libimage is intended to a simpler layer on top of c/storage+c/image that is closer to the Podman feature set, and more opinionated about use of c/storage and typical use cases (e.g. it has a concept of “push”, where various aspects of the destination are heuristically defaulted from the source, or the users’ input is heuristically interpreted) whereas c/image is designed to be lower-level and unopinionated (c/image/copy.Image intentionally requires unambiguous input specification, and doesn’t inherit properties that way).

WRT my old objections to --all-tags: That primarily meant I didn’t intend to spend much time working on that feature. That doesn’t necessarily directly apply to you: you presumably started this work for some specific reason. Sure, it might be worth discussing whether --all-tags is really the best way to accomplish your primary objective.

rhatdan · 2022-08-15T18:52:30Z

@byarbrough Any update on this?

byarbrough · 2022-08-15T20:40:55Z

Apologies; last week got away from me. I'll have the fixes up tomorrow!

rhatdan · 2022-08-23T21:04:59Z

@vrothberg @mtrmac PTAL

mtrmac · 2022-08-23T21:08:49Z

@rhatdan GitHub shows no changes to the PR since my last review.

rhatdan · 2022-08-23T21:11:04Z

Oops I read it backwards.

rhatdan · 2022-09-09T10:00:34Z

@byarbrough are you still working on this PR?

byarbrough · 2022-09-22T21:53:04Z

I agree that a unit test to compare the wanted and got list of images is required. But I have not been able to find a good way to do that, so I am going to close this PR.
Thank you for everyone's time - apologies for the waste!

rhatdan · 2022-09-23T12:53:12Z

@vrothberg @mtrmac I think we still want this capability. Do you agree?

mtrmac · 2022-09-23T14:27:42Z

Per containers/buildah#1380 (review) , I don’t think this is a feature that should be necessary.

It only “works” if everything in the build/publish pipeline implements an equivalent of --all-tags. Something in the pipeline did know what was being built (there is, and can be, no buildah build --all-tags) and that information was not provided to the push operation. IMHO, fixing that build automation is going to be much easier than us building the feature correctly and reliably — and it is going to make that build automation more predictable and more reliable.

jakecorrenti · 2022-09-23T18:43:09Z

@rhatdan Is this not something I should pick up?

openshift-ci bot added the do-not-merge/work-in-progress label Jul 21, 2022

byarbrough force-pushed the push-all-tags branch from 365d41f to 951cdd6 Compare July 21, 2022 20:02

rhatdan reviewed Jul 22, 2022

View reviewed changes

vrothberg reviewed Jul 22, 2022

View reviewed changes

mtrmac requested changes Jul 22, 2022

View reviewed changes

byarbrough force-pushed the push-all-tags branch from 8a37162 to 56a7865 Compare July 28, 2022 15:30

byarbrough added 2 commits July 28, 2022 11:09

byarbrough force-pushed the push-all-tags branch from 56a7865 to ab5bc69 Compare July 28, 2022 17:09

mtrmac reviewed Jul 28, 2022

View reviewed changes

vrothberg reviewed Aug 1, 2022

View reviewed changes

mtrmac reviewed Aug 8, 2022

View reviewed changes

mtrmac mentioned this pull request Aug 9, 2022

podman push --all-tags containers/podman#2369

Closed

byarbrough mentioned this pull request Aug 23, 2022

[wip] Podman push --all-tags option containers/podman#14949

Closed

byarbrough closed this Sep 22, 2022

byarbrough mentioned this pull request Sep 22, 2022

man pages: docker differences containers/podman#14917

Closed

69 tasks

rhatdan reopened this Sep 23, 2022

rhatdan closed this Sep 23, 2022

byarbrough deleted the push-all-tags branch November 4, 2022 17:24

-	// Make sure the source repository does not have a tag
+	// Make sure the source repository does not have a tag
+	// This intentionally does not use `alltransports.ParseImageName`, because the outcome of that
+	// refers to a single image, not a repo (e.g. it defaults to …:latest for docker:// references), which
+	// is not the semantics we want.

[wip] Add option to push all tags #1099

[wip] Add option to push all tags #1099

Conversation

byarbrough commented Jul 21, 2022 • edited

openshift-ci bot commented Jul 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac Jul 22, 2022 • edited

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 Jul 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byarbrough commented Jul 25, 2022

mtrmac commented Jul 25, 2022

vrothberg commented Jul 26, 2022

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

vrothberg commented Jul 29, 2022

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 commented Aug 9, 2022

byarbrough commented Aug 10, 2022 • edited

mtrmac commented Aug 10, 2022

rhatdan commented Aug 15, 2022

byarbrough commented Aug 15, 2022

rhatdan commented Aug 23, 2022

mtrmac commented Aug 23, 2022

rhatdan commented Aug 23, 2022

rhatdan commented Sep 9, 2022

byarbrough commented Sep 22, 2022

rhatdan commented Sep 23, 2022

mtrmac commented Sep 23, 2022

jakecorrenti commented Sep 23, 2022

byarbrough commented Jul 21, 2022 •

edited

mtrmac Jul 22, 2022 •

edited

mtrmac Jul 22, 2022 •

edited

byarbrough commented Aug 10, 2022 •

edited