Accept a config blob alongside the "changes" slice when committing

When committing containers to create new images, accept a container
config blob being passed in the body of the API request by adding a
Config field to our API structures.  Populate it from the body of
requests that we receive, and use its contents as the body of requests
that we make.

Make the libpod commit endpoint split changes values at newlines, just
like the compat endpoint does.

Pass both the config blob and the "changes" slice to buildah's Commit()
API, so that it can handle cases where they overlap or conflict.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

cmd/podman/containers/commit.go

@@ -9,6 +9,7 @@ import (
 	"github.com/containers/common/pkg/completion"
 	"github.com/containers/podman/v4/cmd/podman/common"
 	"github.com/containers/podman/v4/cmd/podman/registry"
+	"github.com/containers/podman/v4/pkg/api/handlers"
 	"github.com/containers/podman/v4/pkg/domain/entities"
 	"github.com/spf13/cobra"
 )
@@ -47,7 +48,7 @@ var (
 	commitOptions = entities.CommitOptions{
 		ImageName: "",
 	}
-	iidFile string
+	configFile, iidFile string
 )
 
 func commitFlags(cmd *cobra.Command) {
@@ -57,6 +58,10 @@ func commitFlags(cmd *cobra.Command) {
 	flags.StringArrayVarP(&commitOptions.Changes, changeFlagName, "c", []string{}, "Apply the following possible instructions to the created image (default []): "+strings.Join(common.ChangeCmds, " | "))
 	_ = cmd.RegisterFlagCompletionFunc(changeFlagName, common.AutocompleteChangeInstructions)
 
+	configFileFlagName := "config"
+	flags.StringVar(&configFile, configFileFlagName, "", "`file` containing a container configuration to merge into the image")
+	_ = cmd.RegisterFlagCompletionFunc(configFileFlagName, completion.AutocompleteDefault)
+
 	formatFlagName := "format"
 	flags.StringVarP(&commitOptions.Format, formatFlagName, "f", "oci", "`Format` of the image manifest and metadata")
 	_ = cmd.RegisterFlagCompletionFunc(formatFlagName, common.AutocompleteImageFormat)
@@ -100,7 +105,16 @@ func commit(cmd *cobra.Command, args []string) error {
 	if !commitOptions.Quiet {
 		commitOptions.Writer = os.Stderr
 	}
-
+	if len(commitOptions.Changes) > 0 {
+		commitOptions.Changes = handlers.DecodeChanges(commitOptions.Changes)
+	}
+	if len(configFile) > 0 {
+		cfg, err := os.ReadFile(configFile)
+		if err != nil {
+			return fmt.Errorf("--config: %w", err)
+		}
+		commitOptions.Config = cfg
+	}
 	response, err := registry.ContainerEngine().ContainerCommit(context.Background(), container, commitOptions)
 	if err != nil {
 		return err

docs/source/markdown/podman-commit.1.md

@@ -36,6 +36,13 @@ Apply the following possible instructions to the created image:
 
 Can be set multiple times.
 
+#### **--config**=*ConfigBlobFile*
+
+Merge the container configuration from the specified file into the configuration for the image
+as it is being committed. The file contents should be a JSON-encoded version of
+a Schema2Config structure, which is defined at
+https://github.com/containers/image/blob/v5.29.0/manifest/docker_schema2.go#L67.
+
 #### **--format**, **-f**=**oci** | *docker*
 
 Set the format of the image manifest and metadata.  The currently supported formats are **oci** and *docker*.\

libpod/container_commit.go

@@ -20,16 +20,16 @@ import (
 
 // ContainerCommitOptions is a struct used to commit a container to an image
 // It uses buildah's CommitOptions as a base. Long-term we might wish to
-// add these to the buildah struct once buildah is more integrated with
-// libpod
+// decouple these because it includes duplicates of fields that are in, or
+// could later be added, to buildah's CommitOptions, which gets confusing
 type ContainerCommitOptions struct {
 	buildah.CommitOptions
 	Pause          bool
 	IncludeVolumes bool
 	Author         string
 	Message        string
-	Changes        []string
-	Squash         bool
+	Changes        []string // gets merged with CommitOptions.OverrideChanges
+	Squash         bool     // always used instead of CommitOptions.Squash
 }
 
 // Commit commits the changes between a container and its image, creating a new
@@ -69,6 +69,8 @@ func (c *Container) Commit(ctx context.Context, destImage string, options Contai
 		Squash:                options.Squash,
 		SystemContext:         c.runtime.imageContext,
 		PreferredManifestType: options.PreferredManifestType,
+		OverrideChanges:       append(append([]string{}, options.Changes...), options.CommitOptions.OverrideChanges...),
+		OverrideConfig:        options.CommitOptions.OverrideConfig,
 	}
 	importBuilder, err := buildah.ImportBuilder(ctx, c.runtime.store, builderOptions)
 	importBuilder.Format = options.PreferredManifestType
@@ -150,51 +152,6 @@ func (c *Container) Commit(ctx context.Context, destImage string, options Contai
 	// Workdir
 	importBuilder.SetWorkDir(c.config.Spec.Process.Cwd)
 
-	// Process user changes
-	newImageConfig, err := libimage.ImageConfigFromChanges(options.Changes)
-	if err != nil {
-		return nil, err
-	}
-	if newImageConfig.User != "" {
-		importBuilder.SetUser(newImageConfig.User)
-	}
-	// EXPOSE only appends
-	for port := range newImageConfig.ExposedPorts {
-		importBuilder.SetPort(port)
-	}
-	// ENV only appends
-	for _, env := range newImageConfig.Env {
-		splitEnv := strings.SplitN(env, "=", 2)
-		key := splitEnv[0]
-		value := ""
-		if len(splitEnv) == 2 {
-			value = splitEnv[1]
-		}
-		importBuilder.SetEnv(key, value)
-	}
-	if newImageConfig.Entrypoint != nil {
-		importBuilder.SetEntrypoint(newImageConfig.Entrypoint)
-	}
-	if newImageConfig.Cmd != nil {
-		importBuilder.SetCmd(newImageConfig.Cmd)
-	}
-	// VOLUME only appends
-	for vol := range newImageConfig.Volumes {
-		importBuilder.AddVolume(vol)
-	}
-	if newImageConfig.WorkingDir != "" {
-		importBuilder.SetWorkDir(newImageConfig.WorkingDir)
-	}
-	for k, v := range newImageConfig.Labels {
-		importBuilder.SetLabel(k, v)
-	}
-	if newImageConfig.StopSignal != "" {
-		importBuilder.SetStopSignal(newImageConfig.StopSignal)
-	}
-	for _, onbuild := range newImageConfig.OnBuild {
-		importBuilder.SetOnBuild(onbuild)
-	}
-
 	var commitRef types.ImageReference
 	if destImage != "" {
 		// Now resolve the name.

pkg/api/handlers/changes.go

@@ -0,0 +1,34 @@
+package handlers
+
+import (
+	"strings"
+	"unicode"
+)
+
+// DecodeChanges reads one or more changes from a slice and cleans them up,
+// since what we've advertised as being acceptable in the past isn't really.
+func DecodeChanges(changes []string) []string {
+	result := make([]string, 0, len(changes))
+	for _, possiblyMultilineChange := range changes {
+		for _, change := range strings.Split(possiblyMultilineChange, "\n") {
+			// In particular, we document that we accept values
+			// like "CMD=/bin/sh", which is not valid Dockerfile
+			// syntax, so we can't just pass such a value directly
+			// to a parser that's going to rightfully reject it.
+			// If we trim the string of whitespace at both ends,
+			// and the first occurrence of "=" is before the first
+			// whitespace, replace that "=" with whitespace.
+			change = strings.TrimSpace(change)
+			if change == "" {
+				continue
+			}
+			firstEqualIndex := strings.Index(change, "=")
+			firstSpaceIndex := strings.IndexFunc(change, unicode.IsSpace)
+			if firstEqualIndex != -1 && (firstSpaceIndex == -1 || firstEqualIndex < firstSpaceIndex) {
+				change = change[:firstEqualIndex] + " " + change[firstEqualIndex+1:]
+			}
+			result = append(result, change)
+		}
+	}
+	return result
+}

pkg/api/handlers/changes_test.go

@@ -0,0 +1,52 @@
+package handlers
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestDecodeChanges(t *testing.T) {
+	testCases := []struct {
+		description string
+		input       string
+		output      []string
+	}{
+		{
+			description: "nothing",
+			input:       "",
+			output:      []string{},
+		},
+		{
+			description: "space",
+			input:       `CMD=/bin/bash`,
+			output:      []string{"CMD /bin/bash"},
+		},
+		{
+			description: "equal",
+			input:       `CMD=/bin/bash`,
+			output:      []string{"CMD /bin/bash"},
+		},
+		{
+			description: "both-but-right-first",
+			input:       `LABEL A=B`,
+			output:      []string{"LABEL A=B"},
+		},
+		{
+			description: "both-but-right-second",
+			input:       `LABEL A=B C=D`,
+			output:      []string{"LABEL A=B C=D"},
+		},
+		{
+			description: "both-but-wrong",
+			input:       `LABEL=A=B C=D`,
+			output:      []string{"LABEL A=B C=D"},
+		},
+	}
+	for _, testCase := range testCases {
+		t.Run(testCase.description, func(t *testing.T) {
+			output := DecodeChanges([]string{testCase.input})
+			assert.Equalf(t, testCase.output, output, "decoded value was not what we expected")
+		})
+	}
+}

pkg/api/handlers/compat/images.go

@@ -2,7 +2,6 @@ package compat
 
 import (
 	"context"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"net/http"
@@ -133,18 +132,17 @@ func CommitContainer(w http.ResponseWriter, r *http.Request) {
 		PreferredManifestType: manifest.DockerV2Schema2MediaType,
 	}
 
-	input := handlers.CreateContainerConfig{}
-	if err := json.NewDecoder(r.Body).Decode(&input); err != nil {
-		utils.Error(w, http.StatusInternalServerError, fmt.Errorf("Decode(): %w", err))
-		return
-	}
-
 	options.Message = query.Comment
 	options.Author = query.Author
 	options.Pause = query.Pause
 	options.Squash = query.Squash
-	for _, change := range query.Changes {
-		options.Changes = append(options.Changes, strings.Split(change, "\n")...)
+	options.Changes = handlers.DecodeChanges(query.Changes)
+	if r.Body != nil {
+		defer r.Body.Close()
+		if options.CommitOptions.OverrideConfig, err = abi.DecodeOverrideConfig(r.Body); err != nil {
+			utils.Error(w, http.StatusBadRequest, err)
+			return
+		}
 	}
 	ctr, err := runtime.LookupContainer(query.Container)
 	if err != nil {

pkg/api/handlers/libpod/images.go

@@ -483,6 +483,13 @@ func CommitContainer(w http.ResponseWriter, r *http.Request) {
 		SystemContext:         sc,
 		PreferredManifestType: mimeType,
 	}
+	if r.Body != nil {
+		defer r.Body.Close()
+		if options.CommitOptions.OverrideConfig, err = abi.DecodeOverrideConfig(r.Body); err != nil {
+			utils.Error(w, http.StatusBadRequest, err)
+			return
+		}
+	}
 	if len(query.Tag) > 0 {
 		tag = query.Tag
 	}
@@ -490,7 +497,7 @@ func CommitContainer(w http.ResponseWriter, r *http.Request) {
 	options.Author = query.Author
 	options.Pause = query.Pause
 	options.Squash = query.Squash
-	options.Changes = query.Changes
+	options.Changes = handlers.DecodeChanges(query.Changes)
 	ctr, err := runtime.LookupContainer(query.Container)
 	if err != nil {
 		utils.Error(w, http.StatusNotFound, err)

pkg/bindings/containers/commit.go

@@ -33,7 +33,11 @@ func Commit(ctx context.Context, nameOrID string, options *CommitOptions) (entit
 		return entities.IDResponse{}, err
 	}
 	params.Set("container", nameOrID)
-	response, err := conn.DoRequest(ctx, nil, http.MethodPost, "/commit", params, nil)
+	var requestBody io.Reader
+	if options.Config != nil {
+		requestBody = *options.Config
+	}
+	response, err := conn.DoRequest(ctx, requestBody, http.MethodPost, "/commit", params, nil)
 	if err != nil {
 		return id, err
 	}

pkg/bindings/containers/types.go

@@ -29,6 +29,7 @@ type LogOptions struct {
 type CommitOptions struct {
 	Author  *string
 	Changes []string
+	Config  *io.Reader `schema:"-"`
 	Comment *string
 	Format  *string
 	Pause   *bool

pkg/bindings/containers/types_commit_options.go

@@ -2,6 +2,7 @@
 package containers
 
 import (
+	"io"
 	"net/url"
 
 	"github.com/containers/podman/v4/pkg/bindings/internal/util"
@@ -47,6 +48,21 @@ func (o *CommitOptions) GetChanges() []string {
 	return o.Changes
 }
 
+// WithConfig set field Config to given value
+func (o *CommitOptions) WithConfig(value io.Reader) *CommitOptions {
+	o.Config = &value
+	return o
+}
+
+// GetConfig returns value of field Config
+func (o *CommitOptions) GetConfig() io.Reader {
+	if o.Config == nil {
+		var z io.Reader
+		return z
+	}
+	return *o.Config
+}
+
 // WithComment set field Comment to given value
 func (o *CommitOptions) WithComment(value string) *CommitOptions {
 	o.Comment = &value

pkg/domain/entities/containers.go

@@ -164,6 +164,7 @@ type ContainerStatReport struct {
 type CommitOptions struct {
 	Author         string
 	Changes        []string
+	Config         []byte
 	Format         string
 	ImageName      string
 	IncludeVolumes bool

pkg/domain/infra/abi/config.go

@@ -0,0 +1,22 @@
+package abi
+
+import (
+	"encoding/json"
+	"errors"
+	"io"
+
+	"github.com/containers/image/v5/manifest"
+)
+
+// DecodeOverrideConfig reads a Schema2Config from a Reader, suppressing EOF
+// errors.
+func DecodeOverrideConfig(reader io.Reader) (*manifest.Schema2Config, error) {
+	config := manifest.Schema2Config{}
+	if reader != nil {
+		err := json.NewDecoder(reader).Decode(&config)
+		if err != nil && !errors.Is(err, io.EOF) {
+			return nil, err
+		}
+	}
+	return &config, nil
+}

pkg/domain/infra/abi/config_test.go

@@ -0,0 +1,56 @@
+package abi
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/containers/image/v5/manifest"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestDecodeOverrideConfig(t *testing.T) {
+	testCases := []struct {
+		description   string
+		body          string
+		expectedValue *manifest.Schema2Config
+		expectedError bool
+	}{
+		{
+			description:   "nothing",
+			body:          ``,
+			expectedValue: &manifest.Schema2Config{},
+		},
+		{
+			description:   "empty",
+			body:          `{}`,
+			expectedValue: &manifest.Schema2Config{},
+		},
+		{
+			description:   "user",
+			body:          `{"User":"0:0"}`,
+			expectedValue: &manifest.Schema2Config{User: "0:0"},
+		},
+		{
+			description:   "malformed",
+			body:          `{"User":`,
+			expectedError: true,
+		},
+	}
+	t.Run("no reader", func(t *testing.T) {
+		value, err := DecodeOverrideConfig(nil)
+		assert.NoErrorf(t, err, "decoding nothing")
+		assert.NotNilf(t, value, "decoded value was unexpectedly nil")
+	})
+	for _, testCase := range testCases {
+		t.Run(testCase.description, func(t *testing.T) {
+			value, err := DecodeOverrideConfig(strings.NewReader(testCase.body))
+			if testCase.expectedError {
+				assert.Errorf(t, err, "decoding sample data")
+			} else {
+				assert.NoErrorf(t, err, "decoding sample data")
+				assert.NotNilf(t, value, "decoded value was unexpectedly nil")
+				assert.Equalf(t, *testCase.expectedValue, *value, "decoded value was not what we expected")
+			}
+		})
+	}
+}

pkg/domain/infra/abi/containers.go

@@ -1,6 +1,7 @@
 package abi
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -17,6 +18,7 @@ import (
 	"github.com/containers/podman/v4/libpod"
 	"github.com/containers/podman/v4/libpod/define"
 	"github.com/containers/podman/v4/libpod/logs"
+	"github.com/containers/podman/v4/pkg/api/handlers"
 	"github.com/containers/podman/v4/pkg/checkpoint"
 	"github.com/containers/podman/v4/pkg/domain/entities"
 	"github.com/containers/podman/v4/pkg/domain/entities/reports"
@@ -581,18 +583,29 @@ func (ic *ContainerEngine) ContainerCommit(ctx context.Context, nameOrID string,
 	}
 
 	sc := ic.Libpod.SystemContext()
+	var changes []string
+	if len(options.Changes) > 0 {
+		changes = handlers.DecodeChanges(options.Changes)
+	}
+	var overrideConfig *manifest.Schema2Config
+	if len(options.Config) > 0 {
+		if overrideConfig, err = DecodeOverrideConfig(bytes.NewReader(options.Config)); err != nil {
+			return nil, err
+		}
+	}
 	coptions := buildah.CommitOptions{
 		SignaturePolicyPath:   rtc.Engine.SignaturePolicyPath,
 		ReportWriter:          options.Writer,
 		SystemContext:         sc,
 		PreferredManifestType: mimeType,
+		OverrideConfig:        overrideConfig,
 	}
 	opts := libpod.ContainerCommitOptions{
 		CommitOptions:  coptions,
 		Pause:          options.Pause,
 		IncludeVolumes: options.IncludeVolumes,
 		Message:        options.Message,
-		Changes:        options.Changes,
+		Changes:        changes,
 		Author:         options.Author,
 		Squash:         options.Squash,
 	}

pkg/domain/infra/tunnel/containers.go

@@ -1,6 +1,7 @@
 package tunnel
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -346,7 +347,15 @@ func (ic *ContainerEngine) ContainerCommit(ctx context.Context, nameOrID string,
 			return nil, fmt.Errorf("invalid image name %q", opts.ImageName)
 		}
 	}
-	options := new(containers.CommitOptions).WithAuthor(opts.Author).WithChanges(opts.Changes).WithComment(opts.Message).WithSquash(opts.Squash).WithStream(!opts.Quiet)
+	var changes []string
+	if len(opts.Changes) > 0 {
+		changes = handlers.DecodeChanges(opts.Changes)
+	}
+	var configReader io.Reader
+	if len(opts.Config) > 0 {
+		configReader = bytes.NewReader(opts.Config)
+	}
+	options := new(containers.CommitOptions).WithAuthor(opts.Author).WithChanges(changes).WithComment(opts.Message).WithConfig(configReader).WithSquash(opts.Squash).WithStream(!opts.Quiet)
 	options.WithFormat(opts.Format).WithPause(opts.Pause).WithRepo(repo).WithTag(tag)
 	response, err := containers.Commit(ic.ClientCtx, nameOrID, options)
 	if err != nil {

test/apiv2/14-commit.at

@@ -0,0 +1,30 @@
+# Create a container for testing the container initializing later
+podman create -t -i --name myctr $IMAGE ls
+
+config=$(mktemp -t config.XXXXXXXXXX.json)
+cat > "$config" <<- EOF
+{
+  "Entrypoint": ["/bin/crash"],
+  "Cmd": ["and", "burn"],
+  "Labels": {"for": "ever", "and": "ever"}
+}
+EOF
+
+# Create a new image based on the container
+t POST 'libpod/commit?container=myctr&repo=nativeimage&tag=1' $config 200
+
+# Check some things
+t GET libpod/images/nativeimage:1/json 200 ".Config.Cmd=$(jq .Cmd $config)" ".Config.Entrypoint=$(jq .Entrypoint $config)"
+
+# Create a new image based on the container
+t POST 'commit?container=myctr&repo=compatimage&tag=1' $config 201
+
+# Check some things
+t GET images/compatimage:1/json 200 ".Config.Cmd=$(jq .Cmd $config)" ".Config.Entrypoint=$(jq .Entrypoint $config)"
+
+# Clean up
+t DELETE containers/myctr 204
+t DELETE images/nativeimage:1 200
+t DELETE images/compatimage:1 200
+rm -f "$config"
+unset config

test/e2e/commit_test.go

@@ -21,7 +21,7 @@ var _ = Describe("Podman commit", func() {
 		session := podmanTest.Podman([]string{"commit", "test1", "--change", "BOGUS=foo", "foobar.com/test1-image:latest"})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(Exit(125))
-		Expect(session.ErrorToString()).To(Equal("Error: invalid change \"BOGUS=foo\" - invalid instruction BOGUS"))
+		Expect(session.ErrorToString()).To(HaveSuffix(`applying changes: processing change "BOGUS foo": did not understand change instruction "BOGUS foo"`))
 
 		session = podmanTest.Podman([]string{"commit", "test1", "foobar.com/test1-image:latest"})
 		session.WaitWithDefaultTimeout()
@@ -127,6 +127,45 @@ var _ = Describe("Podman commit", func() {
 		Expect(inspectResults[0].Labels).To(HaveKeyWithValue("image", "blue"))
 	})
 
+	It("podman commit container with --config flag", func() {
+		test := podmanTest.Podman([]string{"run", "--name", "test1", "-d", ALPINE, "ls"})
+		test.WaitWithDefaultTimeout()
+		Expect(test).Should(ExitCleanly())
+		Expect(podmanTest.NumberOfContainers()).To(Equal(1))
+
+		configFile, err := os.CreateTemp(podmanTest.TempDir, "")
+		Expect(err).Should(Succeed())
+		_, err = configFile.WriteString(`{"Labels":{"image":"green"}}`)
+		Expect(err).Should(Succeed())
+		configFile.Close()
+
+		session := podmanTest.Podman([]string{"commit", "-q", "--config", configFile.Name(), "test1", "foobar.com/test1-image:latest"})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(ExitCleanly())
+
+		check := podmanTest.Podman([]string{"inspect", "foobar.com/test1-image:latest"})
+		check.WaitWithDefaultTimeout()
+		inspectResults := check.InspectImageJSON()
+		Expect(inspectResults[0].Labels).To(HaveKeyWithValue("image", "green"))
+	})
+
+	It("podman commit container with --config pointing to trash", func() {
+		test := podmanTest.Podman([]string{"run", "--name", "test1", "-d", ALPINE, "ls"})
+		test.WaitWithDefaultTimeout()
+		Expect(test).Should(ExitCleanly())
+		Expect(podmanTest.NumberOfContainers()).To(Equal(1))
+
+		configFile, err := os.CreateTemp(podmanTest.TempDir, "")
+		Expect(err).Should(Succeed())
+		_, err = configFile.WriteString("this is not valid JSON\n")
+		Expect(err).Should(Succeed())
+		configFile.Close()
+
+		session := podmanTest.Podman([]string{"commit", "-q", "--config", configFile.Name(), "test1", "foobar.com/test1-image:latest"})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(Not(ExitCleanly()))
+	})
+
 	It("podman commit container with --squash", func() {
 		test := podmanTest.Podman([]string{"run", "--name", "test1", "-d", ALPINE, "ls"})
 		test.WaitWithDefaultTimeout()