Merge pull request #21549 from openshift-cherrypick-robot/cherry-pick-20657-to-v4.9

[v4.9] RHEL-14922: accept a config blob alongside the "changes" slice when committing

Author: openshift-merge-bot[bot]

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

cmd/podman/containers/create.go

@@ -400,7 +400,7 @@ func createPodIfNecessary(cmd *cobra.Command, s *specgen.SpecGenerator, netOpts
 	var err error
 	uns := specgen.Namespace{NSMode: specgen.Default}
 	if cliVals.UserNS != "" {
-		uns, err = specgen.ParseNamespace(cliVals.UserNS)
+		uns, err = specgen.ParseUserNamespace(cliVals.UserNS)
 		if err != nil {
 			return err
 		}

docs/source/markdown/options/volume.image.md

@@ -4,9 +4,8 @@
 ####> are applicable to all of those.
 #### **--volume**, **-v**=*[HOST-DIR:CONTAINER-DIR[:OPTIONS]]*
 
-Create a bind mount. Specifying the `-v /HOST-DIR:/CONTAINER-DIR` option, Podman
-bind mounts `/HOST-DIR` from the host to `/CONTAINER-DIR` in the Podman
-container.
+Mount a host directory into containers when executing RUN instructions during
+the build.
 
 The `OPTIONS` are a comma-separated list and can be: <sup>[[1]](#Footnote1)</sup>
 
@@ -17,12 +16,9 @@ The `OPTIONS` are a comma-separated list and can be: <sup>[[1]](#Footnote1)</sup
 
 The `CONTAINER-DIR` must be an absolute path such as `/src/docs`. The `HOST-DIR`
 must be an absolute path as well. Podman bind-mounts the `HOST-DIR` to the
-specified path. For example, when specifying the host path `/foo`,
-Podman copies the contents of `/foo` to the container filesystem on the host
-and bind mounts that into the container.
+specified path when processing RUN instructions.
 
-You can specify multiple  **-v** options to mount one or more mounts to a
-container.
+You can specify multiple  **-v** options to mount one or more mounts.
 
 You can add the `:ro` or `:rw` suffix to a volume to mount it read-only or
 read-write mode, respectively. By default, the volumes are mounted read-write.

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

@@ -487,14 +487,21 @@ 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
 	}
 	options.Message = query.Comment
 	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
 	}