cmd/build: add support for `--push` #4677

danishprakash · 2023-03-21T13:26:01Z

What type of PR is this?

/kind feature

Which issue(s) this PR fixes:

Special notes for your reviewer:

Extremely WIP draft.

Does this PR introduce a user-facing change?

- add support for `--push` flag to `bud` push image immediately after building

rhatdan · 2023-03-21T19:46:13Z

I was thinking this would just be added to the client and not the service side.

buildah build --push

Would internally do

buildah build followed by buildah push.

rhatdan · 2023-03-21T19:46:31Z

What do you think @nalind @flouthoc @vrothberg

nalind · 2023-03-21T20:18:24Z

I was thinking this would just be added to the client and not the service side.

buildah build --push

Would internally do

buildah build followed by buildah push.

That would be simpler; API callers already have a buildah.Push() that they can call directly, and it's less API churn if we add such a call to the tail end of cmd/buildah/buildCmd() if --push.

For the "output=registry" case, setting IsDir in a BuildOutputOption while also adding a Type field that overlaps its meaning is confusing. Making that a special case after we've extracted the output image's rootfs as an archive (an archive which doesn't get used if we're just going to push the image) looks wrong; pushing to a registry probably needs to be an entirely separate branch in generateBuildOutput().

define/build.go

TomSweeneyRedHat · 2023-03-21T22:10:23Z

And just in case, we'll need updates to the man page and additional test added for the final PR. From a quick flyby review, it LGTM other than other comments.

vrothberg · 2023-03-22T08:39:49Z

What do you think @nalind @flouthoc @vrothberg

I am no fan of mixing multiple commands as it breaks separation of concerns. buildah push has over a dozen command-line options. How would they be exposed in buildah build?

But I leave the decision up to the core maintainers of Buildah.

rhatdan · 2023-03-22T15:07:58Z

I see this for compatibility with buildx
https://docs.docker.com/engine/reference/commandline/buildx_build/
--push | | Shorthand for --output=type=registry

I don't think we have to add other options for push, and potentially could have just done this in podman build, but the --output=type calls are handled within buildah.

flouthoc · 2023-03-22T15:31:20Z

I concur should we just do this at podman, and only add --output=type=registry to buildah ?

danishprakash · 2023-03-23T11:29:13Z

I was thinking this would just be added to the client and not the service side.

buildah build --push

Would internally do

buildah build followed by buildah push.

I honestly didn't think that is how we'd want to push. On the outset, --push seems like it should be added serially to build but I felt it might not be extensible given there could be modifications/additions to the --output flag in future.

For the "output=registry" case, setting IsDir in a BuildOutputOption while also adding a Type field that overlaps its meaning is confusing. Making that a special case after we've extracted the output image's rootfs as an archive (an archive which doesn't get used if we're just going to push the image) looks wrong; pushing to a registry probably needs to be an entirely separate branch in generateBuildOutput().

You're right, that was an oversight.

danishprakash · 2023-03-23T11:30:53Z

I concur should we just do this at podman, and only add --output=type=registry to buildah ?

So adding the --push flag to podman which would be translated to --output within buildah? Sounds about right to me. That was the original plan as mentioned here.

pkg/parse/parse.go

pkg/cli/common.go

rhatdan · 2023-04-21T09:56:38Z

internal/util/util.go

+
+		image := opts.Attrs["name"]
+		destSpec := opts.Attrs["name"]
+		dest, err := alltransports.ParseImageName(destSpec)


I am not sure this code is correct, or necessary. I know that @mtrmac and @vrothberg frown on parsing specs outside of github.com/containers/image or github.com/containers/common/libimage

Yes; as a general principle I think the CLI layer, or something as close to it as possible, should be responsible for turning strings into a unique types.ImageReference.

Admittedly that’s not always possible, but it’s the thing to strive for.

Does it help if it's moved to util? #4677 (comment)

mtrmac · 2023-04-21T21:34:19Z

define/types.go

@@ -100,6 +100,8 @@ type Secret struct {

 // BuildOutputOptions contains the the outcome of parsing the value of a build --output flag
 type BuildOutputOption struct {
+	Type     string
+	Attrs    map[string]string


Why is this adding an untyped map instead simple Go fields that exactly represent what we need?

This was referenced from buildx but the idea remains the same, for every type, there are (and would be in the future) multiple arguments/options which are consolidated under attributes here. It's possible for us to have individual fields here but those would eventually proliferate into a heterogenous struct type and I'm not sure how ideal that would be.

This is ultimately up to Buildah maintainers, not me.

IMHO individual fields, or individual types, is much better than Attrs, because Attrs is equally heterogenous, just with much worse compiler / linter support.

We can do interfaces, type checks, wrappers, …

That's a good point. Perhaps we can have embedded structs here for individual Types as and when the situation arises? Looking for inputs

Up to Buildah maintainers.

(My personal style would be:

Type field first

Then separate sections for individual subtypes, separated by blank lines and with a comment “valid if Type == … ”

so that future maintainers can just look at the type definition and don’t have to chase individual field uses in an IDE.

I agree that separate structs, or even type checks, might be overkill for this small scope.)

define/types.go

pkg/parse/parse.go

pkg/cli/build.go

pkg/cli/common.go

internal/util/util.go

mtrmac · 2023-04-21T21:41:29Z

internal/util/util.go

+		libimageOptions.Writer = os.Stdout
+		runtime, err := libimage.RuntimeFromStore(store, &libimage.RuntimeOptions{SystemContext: &types.SystemContext{}})
+		destString := fmt.Sprintf("%s:%s", dest.Transport().Name(), dest.StringWithinTransport())
+		_, err = runtime.Push(context.Background(), image, destString, libimageOptions)


(Eventually, could libImage have an API that accepts type.ImageReference directly?)

internal/util/util.go

mtrmac

It’s quite likely I don’t have a full context of the goals / constraints of the Buildx feature set or compatibility; I’m looking basically only at this PR in isolation.

I’ll fully defer to actual Buildah maintainers.

define/types.go

internal/util/util.go

pkg/parse/parse.go

define/types.go

internal/util/util.go

mtrmac · 2023-04-25T17:47:34Z

util/util.go

@@ -50,6 +51,31 @@ func StringInSlice(s string, slice []string) bool {
 	return util.StringInSlice(s, slice)
 }

+func StringToImageReference(image string) (types.ImageReference, error) {


UIArgumentToImageReference? HeuristicUserInputToImageReference? Something along those lines? (Up to Buildah maintainers.)

I, fairly desperately, don’t want this code to be used to “round-trip” between ImageReference and string [there already are other functions for that], and this plain naming of the function suggests that it could be used for that purpose.

I, fairly desperately, don’t want this code to be used to “round-trip” between ImageReference and string

Sorry but would you mind explaining what you mean? Some sort of abuse this function would get by being used in unintended places?

Also, how about ImageStringToImageReference?

Some sort of abuse this function would get by being used in unintended places?

Yes. The function is a heuristic that can parse the input using several different format/semantics, depending on what happens to work.

A failure case I’ve seen fairly frequently in this space is finding some pre-existing function like that, similarly targeted at the UI and heuristically doing “what the user meant”, and embedding it somewhere deeper in the call stack, calling it with a string produced from a Go value, without accounting for the details of the heuristics, or possibly the CLI-level heuristics being expanded to add new user-level features. The outcome is that the Go value → string → Go value transformation (which might, sometimes, be unavoidable, e.g. for multi-machine operations) becomes inexact, hard to predict, and possibly even leads to security issues.

Hence all of the concern about not using strings as an internal representation if at all possible, as the easiest preventative measure.

Secondarily, using the right string representation / API. The types.ImageReference values already have an API for producing a string representation, and for creating one from a string representation. That one (alltransports.ParseImageName) should be used anywhere below the CLI heuristic layers, and the new function should be named so that it isn’t accidentally used instead.

So that’s why I would like some mention of UI, and preferably “heuristics” in the name.

“Image string” does not help because it does not disambiguate “a lossy heuristic subject to change” from “a stable round-trip-safe value conversion”, either would apply to images and strings.

internal/util/util.go

StarpTech · 2023-05-20T20:15:12Z

Hi, I just created containers/podman#18642 It looks related to this PR.

I am no fan of mixing multiple commands as it breaks separation of concerns. buildah push has over a dozen command-line options. How would they be exposed in buildah build?

I agree but a good reason to combine them is to enable parallelism.

rhatdan · 2023-06-27T21:16:20Z

@danishprakash Are you still working on this?

danishprakash · 2023-07-04T05:05:32Z

Yes, @rhatdan. Will try to address the remaining comments and the tests by early next week.

openshift-ci · 2023-07-18T07:53:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: danishprakash
Once this PR has been reviewed and has the lgtm label, please assign giuseppe for approval. 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

mtrmac

(FWIW the code right now fails a test.)

pkg/parse/parse.go

mtrmac · 2023-07-18T20:32:13Z

pkg/parse/parse.go

-			IsDir:    false,
-			IsStdout: true}, nil
-	}
-	if !strings.Contains(buildOutput, ",") {


This case is now completely removed? Isn’t that a breaking change for existing users?

It looks like a catch-call, I believe this is being handled by the switch case below, now that we expect extended attributes for types, we shouldn't be returning early here.

Isn‘t that confirming my impression that existing syntax stops working?

Why is that acceptable? (It might well be, I’m not that familiar with Buildah. But I definitely want this to be a conscious decision by Buildah maintainers.)

Fair enough, but now that we are handling extended attributes, it'd make little sense for us to still handle this case, and besides, it's going to be counterintuitive for the user to use this. But then again, i think it's better if someone who has more context on this piece comment once.

that existing syntax stops working

Not entirely though, you can still go ahead and use --output=type=image and it'll do the right thing instead of defaulting to a preset condition as before.

mtrmac · 2023-07-18T20:33:34Z

pkg/parse/parse.go

+		out.Push = true
+	}
+
+	if !typeSelected && !pathSelected {


This seems to need more nuance. Isn’t the path still required for local destinations, just not for the type …Image case?

And, for the type …Image case, isn’t specifying a path an error?

mtrmac · 2023-07-18T20:47:51Z

imagebuildah/stage_executor.go

@@ -2071,7 +2071,15 @@ func (s *StageExecutor) commit(ctx context.Context, createdBy string, emptyLayer
 	return imgID, ref, nil
 }

-func (s *StageExecutor) generateBuildOutput(buildOutputOpts define.BuildOutputOption) error {
+func (s *StageExecutor) generateBuildOutput(ops define.BuildOutputOption) error {
+	if ops.Push {


AFAICS type=image will go down the ExportFromReader path below, and fail because …Image is not handled there.

That’s just not implemented at all?

#4677 (comment)

internal/util/util.go

mtrmac · 2023-07-18T20:54:05Z

internal/util/util.go

+// ExportImage copies contents of the image to a new location
+func ExportImage(store storage.Store, opts define.BuildOutputOption) error {
+	if !opts.Push {
+		return nil


Why does this silently succeed? If the only caller only calls this with .Push, this function should either not check, or fail with an “internal inconsistency” error.

(Or, alternatively, is type=image implemented by doing nothing? If so, the caller would have to steer the control flow down this path. But it seems simpler to me to have a top=level dispatcher based on opts.Type, calling various functions, and not calling a function only for that function to do nothing as a special case.)

You're right, type=image on its own doesn't require anything additional since the image is built locally before we get to exports. So now, I'm handling a situation where we have type=image,push=true and if not, then proceed further down.

mtrmac · 2023-07-18T20:58:01Z

internal/util/util.go

+	libimageOptions := &libimage.PushOptions{}
+	libimageOptions.Writer = os.Stdout


Up to buildah maintainers:

I rather suspect quite a few other options could be set here, but I don’t know to what extent the infrastructure for that 1) already exists, or 2) should be built.

Implementation-wise, at a first glance it seems to me cleaner to delegate this to the existing buildah.Push code, even if with possibly fewer options; that would minimize drift.

Implementation-wise, at a first glance it seems to me cleaner to delegate this to the existing buildah.Push

That'd be great, right now the contract is a little too much for anyone else to make use of it. On the other hand, if we can abstract out the "push" functionality out of both for better extensibility, that might be ideal.

rhatdan · 2023-08-22T14:13:31Z

@flouthoc @nalind PTAL

flouthoc

This needs few tests in bud.bats, we already have some tests for pushing there ( you can take help from there )

github-actions · 2023-12-17T00:11:22Z

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

rhatdan · 2023-12-17T11:46:38Z

@danishprakash still working on this?

danishprakash · 2023-12-20T06:06:24Z

Yes, going to get back to this end of this week.

packit-as-a-service · 2024-02-15T12:39:35Z

Ephemeral COPR build failed. @containers/packit-build please check.

* pass systemCtx to push util * fix test Signed-off-by: danishprakash <danish.prakash@suse.com>

openshift-ci bot added do-not-merge/work-in-progress kind/feature Categorizes issue or PR as related to a new feature. labels Mar 21, 2023

TomSweeneyRedHat reviewed Mar 21, 2023

View reviewed changes

define/build.go Outdated Show resolved Hide resolved

danishprakash force-pushed the bud-push-output branch from 07d26e5 to f934001 Compare March 23, 2023 11:25

danishprakash mentioned this pull request Apr 21, 2023

[Feature]: Add support for "buildx build --push" and/or "buildx build --output=type=registry" #4671

Open

rhatdan reviewed Apr 21, 2023

View reviewed changes

pkg/parse/parse.go Outdated Show resolved Hide resolved

rhatdan reviewed Apr 21, 2023

View reviewed changes

pkg/cli/common.go Show resolved Hide resolved

rhatdan reviewed Apr 21, 2023

View reviewed changes

mtrmac reviewed Apr 21, 2023

View reviewed changes

danishprakash force-pushed the bud-push-output branch 4 times, most recently from e22f351 to d6925cd Compare April 25, 2023 06:18

mtrmac reviewed Apr 25, 2023

View reviewed changes

danishprakash force-pushed the bud-push-output branch from d6925cd to bd7e5cf Compare April 26, 2023 13:13

mtrmac reviewed May 2, 2023

View reviewed changes

internal/util/util.go Show resolved Hide resolved

internal/util/util.go Outdated Show resolved Hide resolved

danishprakash marked this pull request as ready for review June 8, 2023 11:51

openshift-ci bot removed the do-not-merge/work-in-progress label Jun 8, 2023

danishprakash force-pushed the bud-push-output branch from bd7e5cf to 4b19451 Compare June 8, 2023 12:18

danishprakash force-pushed the bud-push-output branch from 4b19451 to aca3866 Compare July 18, 2023 07:53

danishprakash force-pushed the bud-push-output branch from aca3866 to 684d7ae Compare July 18, 2023 07:54

mtrmac reviewed Jul 18, 2023

View reviewed changes

danishprakash force-pushed the bud-push-output branch 3 times, most recently from 9989786 to 47f85d4 Compare August 23, 2023 05:19

flouthoc requested changes Aug 23, 2023

View reviewed changes

flouthoc mentioned this pull request Sep 16, 2023

support buildah build --push #5052

Closed

flouthoc mentioned this pull request Oct 4, 2023

podman buildx build is missing --push flag #5073

Closed

github-actions bot added the stale-pr label Dec 17, 2023

github-actions bot removed the stale-pr label Dec 18, 2023

danishprakash force-pushed the bud-push-output branch 3 times, most recently from 0036033 to f9d20a2 Compare January 18, 2024 05:59

danishprakash force-pushed the bud-push-output branch from f9d20a2 to 61ed32a Compare February 15, 2024 12:37

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2024

danishprakash force-pushed the bud-push-output branch from 61ed32a to 7a1ef03 Compare February 15, 2024 12:43

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2024

build: add support for --push

b2c313d

* pass systemCtx to push util * fix test Signed-off-by: danishprakash <danish.prakash@suse.com>

danishprakash force-pushed the bud-push-output branch from 7a1ef03 to b2c313d Compare March 13, 2024 07:58

		libimageOptions := &libimage.PushOptions{}
		libimageOptions.Writer = os.Stdout

cmd/build: add support for --push #4677

Are you sure you want to change the base?

cmd/build: add support for --push #4677

Conversation

danishprakash commented Mar 21, 2023

What type of PR is this?

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

rhatdan commented Mar 21, 2023

rhatdan commented Mar 21, 2023

nalind commented Mar 21, 2023

TomSweeneyRedHat commented Mar 21, 2023

vrothberg commented Mar 22, 2023

rhatdan commented Mar 22, 2023

flouthoc commented Mar 22, 2023

danishprakash commented Mar 23, 2023

danishprakash commented Mar 23, 2023

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 Apr 25, 2023 • 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

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

StarpTech commented May 20, 2023 • edited

rhatdan commented Jun 27, 2023

danishprakash commented Jul 4, 2023

openshift-ci bot commented Jul 18, 2023

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

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

rhatdan commented Aug 22, 2023

flouthoc left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 17, 2023

rhatdan commented Dec 17, 2023

danishprakash commented Dec 20, 2023

packit-as-a-service bot commented Feb 15, 2024

cmd/build: add support for `--push` #4677

cmd/build: add support for `--push` #4677

mtrmac Apr 25, 2023 •

edited

StarpTech commented May 20, 2023 •

edited