Merge pull request #4563 from mheon/fix_change_parsing

Fix parsing for arrays of values in image changes

2 files changed, 70 insertions, 102 deletions

diff --git a/libpod/container_commit.go b/libpod/container_commit.go
index 42f298a81..a0ba57f4f 100644
--- a/libpod/container_commit.go
+++ b/libpod/container_commit.go
@@ -3,7 +3,6 @@ package libpod
 import (
 	"context"
 	"fmt"
-	"os"
 	"strings"
 
 	"github.com/containers/buildah"
@@ -12,6 +11,7 @@ import (
 	"github.com/containers/libpod/libpod/define"
 	"github.com/containers/libpod/libpod/events"
 	"github.com/containers/libpod/libpod/image"
+	libpodutil "github.com/containers/libpod/pkg/util"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
@@ -32,10 +32,6 @@ type ContainerCommitOptions struct {
 // Commit commits the changes between a container and its image, creating a new
 // image
 func (c *Container) Commit(ctx context.Context, destImage string, options ContainerCommitOptions) (*image.Image, error) {
-	var (
-		isEnvCleared, isLabelCleared, isExposeCleared, isVolumeCleared bool
-	)
-
 	if c.config.Rootfs != "" {
 		return nil, errors.Errorf("cannot commit a container that uses an exploded rootfs")
 	}
@@ -51,7 +47,7 @@ func (c *Container) Commit(ctx context.Context, destImage string, options Contai
 
 	if c.state.State == define.ContainerStateRunning && options.Pause {
 		if err := c.pause(); err != nil {
-			return nil, errors.Wrapf(err, "error pausing container %q", c.ID())
+			return nil, errors.Wrapf(err, "error pausing container %q to commit", c.ID())
 		}
 		defer func() {
 			if err := c.unpause(); err != nil {
@@ -103,7 +99,7 @@ func (c *Container) Commit(ctx context.Context, destImage string, options Contai
 	}
 	// Expose ports
 	for _, p := range c.config.PortMappings {
-		importBuilder.SetPort(fmt.Sprintf("%d", p.ContainerPort))
+		importBuilder.SetPort(fmt.Sprintf("%d/%s", p.ContainerPort, p.Protocol))
 	}
 	// Labels
 	for k, v := range c.Labels() {
@@ -111,7 +107,9 @@ func (c *Container) Commit(ctx context.Context, destImage string, options Contai
 	}
 	// No stop signal
 	// User
-	importBuilder.SetUser(c.User())
+	if c.config.User != "" {
+		importBuilder.SetUser(c.config.User)
+	}
 	// Volumes
 	if options.IncludeVolumes {
 		for _, v := range c.config.UserVolumes {
@@ -119,107 +117,76 @@ func (c *Container) Commit(ctx context.Context, destImage string, options Contai
 				importBuilder.AddVolume(v)
 			}
 		}
-	}
-	// Workdir
-	importBuilder.SetWorkDir(c.Spec().Process.Cwd)
-
-	genCmd := func(cmd string) []string {
-		trim := func(cmd []string) []string {
-			if len(cmd) == 0 {
-				return cmd
+	} else {
+		// Only include anonymous named volumes added by the user by
+		// default.
+		for _, v := range c.config.NamedVolumes {
+			include := false
+			for _, userVol := range c.config.UserVolumes {
+				if userVol == v.Dest {
+					include = true
+					break
+				}
 			}
-
-			retCmd := []string{}
-			for _, c := range cmd {
-				if len(c) >= 2 {
-					if c[0] == '"' && c[len(c)-1] == '"' {
-						retCmd = append(retCmd, c[1:len(c)-1])
-						continue
-					}
+			if include {
+				vol, err := c.runtime.GetVolume(v.Name)
+				if err != nil {
+					return nil, errors.Wrapf(err, "volume %s used in container %s has been removed", v.Name, c.ID())
+				}
+				if vol.IsCtrSpecific() {
+					importBuilder.AddVolume(v.Dest)
 				}
-				retCmd = append(retCmd, c)
 			}
-			return retCmd
 		}
-		if strings.HasPrefix(cmd, "[") {
-			cmd = strings.TrimPrefix(cmd, "[")
-			cmd = strings.TrimSuffix(cmd, "]")
-			return trim(strings.Split(cmd, ","))
-		}
-		return []string{"/bin/sh", "-c", cmd}
 	}
-	// Process user changes
-	for _, change := range options.Changes {
-		splitChange := strings.SplitN(change, "=", 2)
-		if len(splitChange) != 2 {
-			splitChange = strings.SplitN(change, " ", 2)
-			if len(splitChange) < 2 {
-				return nil, errors.Errorf("invalid change %s format", change)
-			}
-		}
+	// Workdir
+	importBuilder.SetWorkDir(c.config.Spec.Process.Cwd)
 
-		switch strings.ToUpper(splitChange[0]) {
-		case "CMD":
-			importBuilder.SetCmd(genCmd(splitChange[1]))
-		case "ENTRYPOINT":
-			importBuilder.SetEntrypoint(genCmd(splitChange[1]))
-		case "ENV":
-			change := strings.Split(splitChange[1], " ")
-			name := change[0]
-			val := ""
-			if len(change) < 2 {
-				change = strings.Split(change[0], "=")
-			}
-			if len(change) < 2 {
-				var ok bool
-				val, ok = os.LookupEnv(name)
-				if !ok {
-					return nil, errors.Errorf("invalid env variable %q: not defined in your environment", name)
-				}
-			} else {
-				name = change[0]
-				val = strings.Join(change[1:], " ")
-			}
-			if !isEnvCleared { // Multiple values are valid, only clear once.
-				importBuilder.ClearEnv()
-				isEnvCleared = true
-			}
-			importBuilder.SetEnv(name, val)
-		case "EXPOSE":
-			if !isExposeCleared { // Multiple values are valid, only clear once
-				importBuilder.ClearPorts()
-				isExposeCleared = true
-			}
-			importBuilder.SetPort(splitChange[1])
-		case "LABEL":
-			change := strings.Split(splitChange[1], " ")
-			if len(change) < 2 {
-				change = strings.Split(change[0], "=")
-			}
-			if len(change) < 2 {
-				return nil, errors.Errorf("invalid label %s format, requires to NAME=VAL", splitChange[1])
-			}
-			if !isLabelCleared { // multiple values are valid, only clear once
-				importBuilder.ClearLabels()
-				isLabelCleared = true
-			}
-			importBuilder.SetLabel(change[0], strings.Join(change[1:], " "))
-		case "ONBUILD":
-			importBuilder.SetOnBuild(splitChange[1])
-		case "STOPSIGNAL":
-			// No Set StopSignal
-		case "USER":
-			importBuilder.SetUser(splitChange[1])
-		case "VOLUME":
-			if !isVolumeCleared { // multiple values are valid, only clear once
-				importBuilder.ClearVolumes()
-				isVolumeCleared = true
-			}
-			importBuilder.AddVolume(splitChange[1])
-		case "WORKDIR":
-			importBuilder.SetWorkDir(splitChange[1])
+	// Process user changes
+	newImageConfig, err := libpodutil.GetImageConfig(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)
+	}
+
 	candidates, _, _, err := util.ResolveName(destImage, "", sc, c.runtime.store)
 	if err != nil {
 		return nil, errors.Wrapf(err, "error resolving name %q", destImage)
diff --git a/libpod/runtime_img.go b/libpod/runtime_img.go
index f2784c07d..abd8b581d 100644
--- a/libpod/runtime_img.go
+++ b/libpod/runtime_img.go
@@ -160,10 +160,11 @@ func (r *Runtime) Import(ctx context.Context, source string, reference string, c
 
 	ic := v1.ImageConfig{}
 	if len(changes) > 0 {
-		ic, err = util.GetImageConfig(changes)
+		config, err := util.GetImageConfig(changes)
 		if err != nil {
 			return "", errors.Wrapf(err, "error adding config changes to image %q", source)
 		}
+		ic = config.ImageConfig
 	}
 
 	hist := []v1.History{