From c4fbd2fc9489b318298ada61e4f98d0097bc0b0d Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 5 Dec 2019 13:10:59 -0500 Subject: Move Commit() to new parsing for --change It turns out we had two independent parsing impkementations for Dockerfile instructions out of --change. My previous commit fixed the one used in --change, but as I discovered to my dismay, commit used a different implementation. Remove that and use the new parsing implementation instead. While we're at it, fix some bugs in the current commit code. The addition of anonymous named volumes to Libpod recently means we can now include those in the image config when committing. Some changes (VOLUME, ENV, EXPOSE, LABEL) previously cleared the config of the former image when used; Docker does not do this, so I removed that behavior. Still needs fixing: the new implementation does not support ONBUILD, while the old one did. Signed-off-by: Matthew Heon --- libpod/container_commit.go | 166 ++++++++++++++++++--------------------------- 1 file changed, 65 insertions(+), 101 deletions(-) (limited to 'libpod') diff --git a/libpod/container_commit.go b/libpod/container_commit.go index 42f298a81..cab204546 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,73 @@ 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) + } + candidates, _, _, err := util.ResolveName(destImage, "", sc, c.runtime.store) if err != nil { return nil, errors.Wrapf(err, "error resolving name %q", destImage) -- cgit v1.2.3-54-g00ecf From 60bfa305a88c686c91ebc0f8f98ac14405ae10cf Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 5 Dec 2019 15:11:40 -0500 Subject: Add ONBUILD support to --change Return types had to change a bit for this, but since we can wrap the old v1.ImageConfig, changes are overall not particularly bad. At present, I believe this only works with commit, not import. This matches how things were before we changed to the new parsing so I think this is fine. Signed-off-by: Matthew Heon --- libpod/container_commit.go | 3 +++ libpod/runtime_img.go | 3 ++- pkg/util/utils.go | 47 ++++++++++++++++++++++++++++++---------------- pkg/util/utils_test.go | 16 ++++++++++++++++ pkg/varlinkapi/images.go | 2 +- 5 files changed, 53 insertions(+), 18 deletions(-) (limited to 'libpod') diff --git a/libpod/container_commit.go b/libpod/container_commit.go index cab204546..a0ba57f4f 100644 --- a/libpod/container_commit.go +++ b/libpod/container_commit.go @@ -183,6 +183,9 @@ func (c *Container) Commit(ctx context.Context, destImage string, options Contai 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 { 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{ diff --git a/pkg/util/utils.go b/pkg/util/utils.go index 5ffb05dda..5b4dfe9fa 100644 --- a/pkg/util/utils.go +++ b/pkg/util/utils.go @@ -73,10 +73,18 @@ func StringInSlice(s string, sl []string) bool { return false } +// ImageConfig is a wrapper around the OCIv1 Image Configuration struct exported +// by containers/image, but containing additional fields that are not supported +// by OCIv1 (but are by Docker v2) - notably OnBuild. +type ImageConfig struct { + v1.ImageConfig + OnBuild []string +} + // GetImageConfig produces a v1.ImageConfig from the --change flag that is // accepted by several Podman commands. It accepts a (limited subset) of // Dockerfile instructions. -func GetImageConfig(changes []string) (v1.ImageConfig, error) { +func GetImageConfig(changes []string) (ImageConfig, error) { // Valid changes: // USER // EXPOSE @@ -87,8 +95,9 @@ func GetImageConfig(changes []string) (v1.ImageConfig, error) { // WORKDIR // LABEL // STOPSIGNAL + // ONBUILD - config := v1.ImageConfig{} + config := ImageConfig{} for _, change := range changes { // First, let's assume proper Dockerfile format - space @@ -98,7 +107,7 @@ func GetImageConfig(changes []string) (v1.ImageConfig, error) { if len(split) != 2 { split = strings.SplitN(change, "=", 2) if len(split) != 2 { - return v1.ImageConfig{}, errors.Errorf("invalid change %q - must be formatted as KEY VALUE", change) + return ImageConfig{}, errors.Errorf("invalid change %q - must be formatted as KEY VALUE", change) } } @@ -108,7 +117,7 @@ func GetImageConfig(changes []string) (v1.ImageConfig, error) { case "USER": // Assume literal contents are the user. if value == "" { - return v1.ImageConfig{}, errors.Errorf("invalid change %q - must provide a value to USER", change) + return ImageConfig{}, errors.Errorf("invalid change %q - must provide a value to USER", change) } config.User = value case "EXPOSE": @@ -117,14 +126,14 @@ func GetImageConfig(changes []string) (v1.ImageConfig, error) { // Protocol must be "tcp" or "udp" splitPort := strings.Split(value, "/") if len(splitPort) > 2 { - return v1.ImageConfig{}, errors.Errorf("invalid change %q - EXPOSE port must be formatted as PORT[/PROTO]", change) + return ImageConfig{}, errors.Errorf("invalid change %q - EXPOSE port must be formatted as PORT[/PROTO]", change) } portNum, err := strconv.Atoi(splitPort[0]) if err != nil { - return v1.ImageConfig{}, errors.Wrapf(err, "invalid change %q - EXPOSE port must be an integer", change) + return ImageConfig{}, errors.Wrapf(err, "invalid change %q - EXPOSE port must be an integer", change) } if portNum > 65535 || portNum <= 0 { - return v1.ImageConfig{}, errors.Errorf("invalid change %q - EXPOSE port must be a valid port number", change) + return ImageConfig{}, errors.Errorf("invalid change %q - EXPOSE port must be a valid port number", change) } proto := "tcp" if len(splitPort) > 1 { @@ -133,7 +142,7 @@ func GetImageConfig(changes []string) (v1.ImageConfig, error) { case "tcp", "udp": proto = testProto default: - return v1.ImageConfig{}, errors.Errorf("invalid change %q - EXPOSE protocol must be TCP or UDP", change) + return ImageConfig{}, errors.Errorf("invalid change %q - EXPOSE protocol must be TCP or UDP", change) } } if config.ExposedPorts == nil { @@ -157,7 +166,7 @@ func GetImageConfig(changes []string) (v1.ImageConfig, error) { key = splitEnv[0] // We do need a key if key == "" { - return v1.ImageConfig{}, errors.Errorf("invalid change %q - ENV must have at least one argument", change) + return ImageConfig{}, errors.Errorf("invalid change %q - ENV must have at least one argument", change) } // Perfectly valid to not have a value if len(splitEnv) == 2 { @@ -219,11 +228,11 @@ func GetImageConfig(changes []string) (v1.ImageConfig, error) { testUnmarshal = strings.Split(value, " ") } if len(testUnmarshal) == 0 { - return v1.ImageConfig{}, errors.Errorf("invalid change %q - must provide at least one argument to VOLUME", change) + return ImageConfig{}, errors.Errorf("invalid change %q - must provide at least one argument to VOLUME", change) } for _, vol := range testUnmarshal { if vol == "" { - return v1.ImageConfig{}, errors.Errorf("invalid change %q - VOLUME paths must not be empty", change) + return ImageConfig{}, errors.Errorf("invalid change %q - VOLUME paths must not be empty", change) } if config.Volumes == nil { config.Volumes = make(map[string]struct{}) @@ -237,7 +246,7 @@ func GetImageConfig(changes []string) (v1.ImageConfig, error) { // WORKDIR c results in /A/b/c // Just need to check it's not empty... if value == "" { - return v1.ImageConfig{}, errors.Errorf("invalid change %q - must provide a non-empty WORKDIR", change) + return ImageConfig{}, errors.Errorf("invalid change %q - must provide a non-empty WORKDIR", change) } config.WorkingDir = filepath.Join(config.WorkingDir, value) case "LABEL": @@ -254,7 +263,7 @@ func GetImageConfig(changes []string) (v1.ImageConfig, error) { splitLabel := strings.SplitN(value, "=", 2) // Unlike ENV, LABEL must have a value if len(splitLabel) != 2 { - return v1.ImageConfig{}, errors.Errorf("invalid change %q - LABEL must be formatted key=value", change) + return ImageConfig{}, errors.Errorf("invalid change %q - LABEL must be formatted key=value", change) } key = splitLabel[0] val = splitLabel[1] @@ -267,7 +276,7 @@ func GetImageConfig(changes []string) (v1.ImageConfig, error) { } // Check key after we strip quotations if key == "" { - return v1.ImageConfig{}, errors.Errorf("invalid change %q - LABEL must have a non-empty key", change) + return ImageConfig{}, errors.Errorf("invalid change %q - LABEL must have a non-empty key", change) } if config.Labels == nil { config.Labels = make(map[string]string) @@ -279,11 +288,17 @@ func GetImageConfig(changes []string) (v1.ImageConfig, error) { // negative numbers. killSignal, err := signal.ParseSignal(value) if err != nil { - return v1.ImageConfig{}, errors.Wrapf(err, "invalid change %q - KILLSIGNAL must be given a valid signal", change) + return ImageConfig{}, errors.Wrapf(err, "invalid change %q - KILLSIGNAL must be given a valid signal", change) } config.StopSignal = fmt.Sprintf("%d", killSignal) + case "ONBUILD": + // Onbuild always appends. + if value == "" { + return ImageConfig{}, errors.Errorf("invalid change %q - ONBUILD must be given an argument", change) + } + config.OnBuild = append(config.OnBuild, value) default: - return v1.ImageConfig{}, errors.Errorf("invalid change %q - invalid instruction %s", change, outerKey) + return ImageConfig{}, errors.Errorf("invalid change %q - invalid instruction %s", change, outerKey) } } diff --git a/pkg/util/utils_test.go b/pkg/util/utils_test.go index e60c644fd..f4b03599d 100644 --- a/pkg/util/utils_test.go +++ b/pkg/util/utils_test.go @@ -238,6 +238,22 @@ func TestGetImageConfigStopSignal(t *testing.T) { assert.NotNil(t, err) } +func TestGetImageConfigOnBuild(t *testing.T) { + onBuildOne, err := GetImageConfig([]string{"ONBUILD ADD /testdir1"}) + require.Nil(t, err) + require.Equal(t, 1, len(onBuildOne.OnBuild)) + assert.Equal(t, onBuildOne.OnBuild[0], "ADD /testdir1") + + onBuildTwo, err := GetImageConfig([]string{"ONBUILD ADD /testdir1", "ONBUILD ADD /testdir2"}) + require.Nil(t, err) + require.Equal(t, 2, len(onBuildTwo.OnBuild)) + assert.Equal(t, onBuildTwo.OnBuild[0], "ADD /testdir1") + assert.Equal(t, onBuildTwo.OnBuild[1], "ADD /testdir2") + + _, err = GetImageConfig([]string{"ONBUILD "}) + assert.NotNil(t, err) +} + func TestGetImageConfigMisc(t *testing.T) { _, err := GetImageConfig([]string{""}) assert.NotNil(t, err) diff --git a/pkg/varlinkapi/images.go b/pkg/varlinkapi/images.go index 7abffa42a..604a455a5 100644 --- a/pkg/varlinkapi/images.go +++ b/pkg/varlinkapi/images.go @@ -602,7 +602,7 @@ func (i *LibpodAPI) ImportImage(call iopodman.VarlinkCall, source, reference, me {Comment: message}, } config := v1.Image{ - Config: configChanges, + Config: configChanges.ImageConfig, History: history, } newImage, err := i.Runtime.ImageRuntime().Import(getContext(), source, reference, nil, image.SigningOptions{}, config) -- cgit v1.2.3-54-g00ecf