Merge pull request #2156 from Ace-Tang/merge_image_config

bugfix: missing merge some config from image

Author: Wei Fu

daemon/mgr/container.go

@@ -355,6 +355,21 @@ func (mgr *ContainerManager) Create(ctx context.Context, name string, config *ty
 		HostConfig: config.HostConfig,
 	}
 
+	// merge image's config into container
+	if err := container.merge(func() (ocispec.ImageConfig, error) {
+		img, err := mgr.Client.GetImage(ctx, config.Image)
+		if err != nil {
+			return ocispec.ImageConfig{}, err
+		}
+		ociImage, err := containerdImageToOciImage(ctx, img)
+		if err != nil {
+			return ocispec.ImageConfig{}, err
+		}
+		return ociImage.Config, nil
+	}); err != nil {
+		return nil, err
+	}
+
 	// set container basefs, basefs is not created in pouchd, it will created
 	// after create options passed to containerd.
 	mgr.setBaseFS(ctx, container, id)
@@ -382,18 +397,6 @@ func (mgr *ContainerManager) Create(ctx context.Context, name string, config *ty
 		return nil, err
 	}
 
-	// merge image's config into container
-	if err := container.merge(func() (ocispec.ImageConfig, error) {
-		img, err := mgr.Client.GetImage(ctx, config.Image)
-		ociImage, err := containerdImageToOciImage(ctx, img)
-		if err != nil {
-			return ocispec.ImageConfig{}, err
-		}
-		return ociImage.Config, nil
-	}); err != nil {
-		return nil, err
-	}
-
 	// Get snapshot UpperDir
 	mounts, err := mgr.Client.GetMounts(ctx, id)
 	if err != nil {

daemon/mgr/container_types.go

@@ -285,7 +285,7 @@ func (c *Container) StopTimeout() int64 {
 func (c *Container) merge(getconfig func() (v1.ImageConfig, error)) error {
 	c.Lock()
 	defer c.Unlock()
-	config, err := getconfig()
+	imageConf, err := getconfig()
 	if err != nil {
 		return err
 	}
@@ -294,19 +294,66 @@ func (c *Container) merge(getconfig func() (v1.ImageConfig, error)) error {
 	// Otherwise use the image's configuration to fill it.
 	if len(c.Config.Entrypoint) == 0 {
 		if len(c.Config.Cmd) == 0 {
-			c.Config.Cmd = config.Cmd
+			c.Config.Cmd = imageConf.Cmd
 		}
-		c.Config.Entrypoint = config.Entrypoint
+		c.Config.Entrypoint = imageConf.Entrypoint
 	}
 
 	// ContainerConfig.Env is new, and the ImageConfig.Env is old
-	newEnvSlice, err := mergeEnvSlice(c.Config.Env, config.Env)
+	newEnvSlice, err := mergeEnvSlice(c.Config.Env, imageConf.Env)
 	if err != nil {
 		return err
 	}
 	c.Config.Env = newEnvSlice
 	if c.Config.WorkingDir == "" {
-		c.Config.WorkingDir = config.WorkingDir
+		c.Config.WorkingDir = imageConf.WorkingDir
+	}
+
+	// merge user from image image config.
+	if c.Config.User == "" {
+		c.Config.User = imageConf.User
+	}
+
+	// merge stop signal from image config.
+	if c.Config.StopSignal == "" {
+		c.Config.StopSignal = imageConf.StopSignal
+	}
+
+	// merge label from image image config, if same label key exist,
+	// use container config.
+	if imageConf.Labels != nil {
+		if c.Config.Labels == nil {
+			c.Config.Labels = make(map[string]string)
+		}
+		for k, v := range c.Config.Labels {
+			imageConf.Labels[k] = v
+		}
+		c.Config.Labels = imageConf.Labels
+	}
+
+	// merge exposed ports from image config, if same label key exist,
+	// use container config.
+	if len(imageConf.ExposedPorts) > 0 {
+		if c.Config.ExposedPorts == nil {
+			c.Config.ExposedPorts = make(map[string]interface{})
+		}
+		for k, v := range imageConf.ExposedPorts {
+			if _, exist := c.Config.ExposedPorts[k]; !exist {
+				c.Config.ExposedPorts[k] = interface{}(v)
+			}
+		}
+	}
+
+	// merge volumes from image config.
+	if len(imageConf.Volumes) > 0 {
+		if c.Config.Volumes == nil {
+			c.Config.Volumes = make(map[string]interface{})
+		}
+		for k, v := range imageConf.Volumes {
+			if _, exist := c.Config.Volumes[k]; !exist {
+				c.Config.Volumes[k] = interface{}(v)
+			}
+		}
 	}
 
 	return nil

daemon/mgr/container_types_test.go

@@ -1,11 +1,15 @@
 package mgr
 
 import (
+	"fmt"
+	"reflect"
+	"sort"
 	"testing"
 	"time"
 
 	"github.com/alibaba/pouch/apis/types"
 	"github.com/alibaba/pouch/pkg/utils"
+	"github.com/opencontainers/image-spec/specs-go/v1"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -82,3 +86,173 @@ func TestContainer_FormatStatus(t *testing.T) {
 		assert.Equal(t, err, tc.err, tc.name)
 	}
 }
+
+func TestMerge(t *testing.T) {
+	assert := assert.New(t)
+
+	type TPort struct {
+		p int
+	}
+
+	type TVolume struct {
+		v int
+	}
+
+	for idx, tc := range []struct {
+		c        *Container
+		image    v1.ImageConfig
+		expected *types.ContainerConfig
+	}{
+		{
+			// test merge from image when both config is not empty
+			c: &Container{
+				Config: &types.ContainerConfig{
+					Cmd:        []string{"a"},
+					Entrypoint: []string{"b"},
+					Env:        []string{"e1=e1", "e2=e2"},
+					User:       "user1",
+					StopSignal: "1",
+					Labels: map[string]string{
+						"l1": "l2",
+						"l3": "l4",
+					},
+					ExposedPorts: map[string]interface{}{
+						"e1": interface{}(TPort{
+							p: 1,
+						}),
+						"e2": interface{}(TPort{
+							p: 2,
+						}),
+					},
+					Volumes: map[string]interface{}{
+						"v1": interface{}(TVolume{
+							v: 1,
+						}),
+						"v2": interface{}(TVolume{
+							v: 2,
+						}),
+					},
+				},
+			},
+			image: v1.ImageConfig{
+				Cmd:        []string{"ia"},
+				Entrypoint: []string{"ib"},
+				Env:        []string{"e1=e1", "ie2=ie2"},
+				User:       "iuser1",
+				StopSignal: "2",
+				Labels: map[string]string{
+					"il1": "l2",
+					"il3": "l4",
+				},
+				ExposedPorts: map[string]struct{}{
+					"ie1": {},
+					"e2":  {},
+				},
+				Volumes: map[string]struct{}{
+					"iv1": {},
+					"iv2": {},
+				},
+			},
+			expected: &types.ContainerConfig{
+				Cmd:        []string{"a"},
+				Entrypoint: []string{"b"},
+				Env:        []string{"e1=e1", "e2=e2", "ie2=ie2"},
+				User:       "user1",
+				StopSignal: "1",
+				Labels: map[string]string{
+					"l3":  "l4",
+					"il1": "l2",
+					"l1":  "l2",
+					"il3": "l4",
+				},
+				ExposedPorts: map[string]interface{}{
+					"e1": interface{}(TPort{
+						p: 1,
+					}),
+					"e2": interface{}(TPort{
+						p: 2,
+					}),
+					"ie1": struct{}{},
+				},
+				Volumes: map[string]interface{}{
+					"v1": interface{}(TVolume{
+						v: 1,
+					}),
+					"v2": interface{}(TVolume{
+						v: 2,
+					}),
+					"iv1": struct{}{},
+					"iv2": struct{}{},
+				},
+			},
+		},
+		{
+			// test merge image config, when image config is empty
+			c: &Container{
+				Config: &types.ContainerConfig{
+					Cmd:        []string{"a"},
+					Entrypoint: []string{"b"},
+					Env:        []string{"e1=e1", "e2=e2"},
+					User:       "user1",
+					StopSignal: "1",
+					Labels: map[string]string{
+						"l1": "l2",
+						"l3": "l4",
+					},
+				},
+			},
+			image: v1.ImageConfig{},
+			expected: &types.ContainerConfig{
+				Cmd:        []string{"a"},
+				Entrypoint: []string{"b"},
+				Env:        []string{"e1=e1", "e2=e2"},
+				User:       "user1",
+				StopSignal: "1",
+				Labels: map[string]string{
+					"l1": "l2",
+					"l3": "l4",
+				},
+			},
+		},
+		{
+			// test merge image config, when container config is empty
+			c: &Container{
+				Config: &types.ContainerConfig{},
+			},
+			image: v1.ImageConfig{
+				Cmd:        []string{"ia"},
+				Entrypoint: []string{"ib"},
+				Env:        []string{"ie1=ie1", "ie2=ie2"},
+				User:       "iuser1",
+				StopSignal: "1",
+				Labels: map[string]string{
+					"il1": "l2",
+					"il3": "l4",
+				},
+			},
+			expected: &types.ContainerConfig{
+				Cmd:        []string{"ia"},
+				Entrypoint: []string{"ib"},
+				Env:        []string{"ie1=ie1", "ie2=ie2"},
+				User:       "iuser1",
+				StopSignal: "1",
+				Labels: map[string]string{
+					"il1": "l2",
+					"il3": "l4",
+				},
+			},
+		},
+	} {
+		err := tc.c.merge(func() (v1.ImageConfig, error) {
+			return tc.image, nil
+		})
+		assert.NoError(err)
+
+		// sort slice
+		sort.Strings(tc.c.Config.Env)
+		sort.Strings(tc.expected.Env)
+
+		ret := reflect.DeepEqual(tc.c.Config, tc.expected)
+		assert.Equal(true, ret, fmt.Sprintf("test %d fails\n %+v should equal with %+v\n", idx, tc.c.Config, tc.expected))
+	}
+}