Merge pull request #1657 from Ace-Tang/exec_user

bugfix: fix exec record user as container config user

Author: Wei Fu

daemon/config/config.go

@@ -113,6 +113,9 @@ type Config struct {
 	// TODO(Ace-Tang): runtime args is not support, since containerd is not support,
 	// add a resolution later if it needed.
 	Runtimes map[string]types.Runtime `json:"add-runtime,omitempty"`
+
+	// Namespace is passed to containerd
+	Namespace string
 }
 
 // Validate validates the user input config.

daemon/daemon.go

@@ -19,7 +19,6 @@ import (
 	"github.com/alibaba/pouch/pkg/meta"
 	"github.com/alibaba/pouch/pkg/system"
 
-	"github.com/containerd/containerd/namespaces"
 	systemddaemon "github.com/coreos/go-systemd/daemon"
 	systemdutil "github.com/coreos/go-systemd/util"
 	"github.com/gorilla/mux"
@@ -71,10 +70,9 @@ func NewDaemon(cfg *config.Config) *Daemon {
 		containerdBinaryFile = cfg.ContainerdPath
 	}
 
-	defaultns := namespaces.Default
 	// the default unix socket path to the containerd socket
 	if cfg.IsCriEnabled {
-		defaultns = criconfig.K8sNamespace
+		cfg.Namespace = criconfig.K8sNamespace
 	}
 
 	containerd, err := ctrd.NewClient(cfg.HomeDir,
@@ -83,7 +81,7 @@ func NewDaemon(cfg *config.Config) *Daemon {
 		ctrd.WithContainerdBinary(containerdBinaryFile),
 		ctrd.WithRPCAddr(cfg.ContainerdAddr),
 		ctrd.WithOOMScoreAdjust(cfg.OOMScoreAdjust),
-		ctrd.WithDefaultNamespace(defaultns),
+		ctrd.WithDefaultNamespace(cfg.Namespace),
 	)
 	if err != nil {
 		logrus.Errorf("failed to new containerd's client: %v", err)

daemon/mgr/container.go

@@ -34,7 +34,6 @@ import (
 	containerdtypes "github.com/containerd/containerd/api/types"
 	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/mount"
-	"github.com/containerd/containerd/namespaces"
 	"github.com/docker/libnetwork"
 	"github.com/go-openapi/strfmt"
 	"github.com/imdario/mergo"
@@ -347,7 +346,8 @@ func (mgr *ContainerManager) Create(ctx context.Context, name string, config *ty
 		return nil, errors.Wrap(err, "failed to set mount point disk quota")
 	}
 
-	// set container basefs
+	// set container basefs, basefs is not created in pouchd, it will created
+	// after create options passed to containerd.
 	mgr.setBaseFS(ctx, container, id)
 
 	// set network settings
@@ -2486,7 +2486,7 @@ func (mgr *ContainerManager) setBaseFS(ctx context.Context, c *Container, id str
 
 	// io.containerd.runtime.v1.linux as a const used by runc
 	c.Lock()
-	c.BaseFS = filepath.Join(mgr.Config.HomeDir, "containerd/state", "io.containerd.runtime.v1.linux", namespaces.Default, info.Name, "rootfs")
+	c.BaseFS = filepath.Join(mgr.Config.HomeDir, "containerd/state", "io.containerd.runtime.v1.linux", mgr.Config.Namespace, info.Name, "rootfs")
 	c.Unlock()
 }
 

daemon/mgr/container_exec.go

@@ -9,6 +9,7 @@ import (
 	"github.com/alibaba/pouch/ctrd"
 	"github.com/alibaba/pouch/pkg/errtypes"
 	"github.com/alibaba/pouch/pkg/randomid"
+	"github.com/alibaba/pouch/pkg/user"
 
 	"github.com/docker/docker/pkg/stdcopy"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
@@ -78,29 +79,32 @@ func (mgr *ContainerManager) StartExec(ctx context.Context, execid string, attac
 		return err
 	}
 
-	c.Lock()
+	// set exec process user, user decided by exec config
+	if execConfig.User == "" {
+		execConfig.User = c.Config.User
+	}
+
+	uid, gid, err := user.Get(c.BaseFS, execConfig.User)
+	if err != nil {
+		return err
+	}
+
 	process := &specs.Process{
 		Args:     execConfig.Cmd,
 		Terminal: execConfig.Tty,
 		Cwd:      "/",
 		Env:      c.Config.Env,
+		User: specs.User{
+			UID:            uid,
+			GID:            gid,
+			AdditionalGids: user.GetAdditionalGids(c.HostConfig.GroupAdd),
+		},
 	}
 
-	if execConfig.User != "" {
-		c.Config.User = execConfig.User
-	}
-
-	if err = setupUser(ctx, c, &specs.Spec{Process: process}); err != nil {
-		c.Unlock()
-		return err
-	}
-
-	// set exec process ulimit
+	// set exec process ulimit, ulimit not decided by exec config
 	if err := setupRlimits(ctx, c.HostConfig, &specs.Spec{Process: process}); err != nil {
-		c.Unlock()
 		return err
 	}
-	c.Unlock()
 
 	execConfig.Running = true
 

main.go

@@ -21,6 +21,7 @@ import (
 	"github.com/alibaba/pouch/storage/quota"
 	"github.com/alibaba/pouch/version"
 
+	"github.com/containerd/containerd/namespaces"
 	"github.com/docker/docker/pkg/reexec"
 	"github.com/google/gops/agent"
 	"github.com/sirupsen/logrus"
@@ -120,7 +121,6 @@ func setupFlags(cmd *cobra.Command) {
 	flagSet.StringVar(&cfg.Pidfile, "pidfile", "/var/run/pouch.pid", "Save daemon pid")
 	flagSet.IntVar(&cfg.OOMScoreAdjust, "oom-score-adj", -500, "Set the oom_score_adj for the daemon")
 	flagSet.Var(optscfg.NewRuntime(&cfg.Runtimes), "add-runtime", "register a OCI runtime to daemon")
-
 }
 
 // runDaemon prepares configs, setups essential details and runs pouchd daemon.
@@ -129,6 +129,9 @@ func runDaemon(cmd *cobra.Command) error {
 		return fmt.Errorf("failed to load daemon file: %s", err)
 	}
 
+	// set containerd namespace, we use containerd default namespace as pouch namespaces
+	cfg.Namespace = namespaces.Default
+
 	// parse log driver config
 	logOptMap, err := opts.ParseLogOptions(cfg.DefaultLogConfig.LogDriver, logOpts)
 	if err != nil {

test/cli_exec_test.go

@@ -151,3 +151,30 @@ func (suite *PouchExecSuite) TestExecFail(c *check.C) {
 	defer DelContainerForceMultyTime(c, name)
 	c.Assert(res.Stderr(), check.NotNil)
 }
+
+// TestExecUser test exec with user.
+func (suite *PouchExecSuite) TestExecUser(c *check.C) {
+	name := "TestExecUser"
+	res := command.PouchRun("run", "-d", "-u=1001", "--name", name, busyboxImage, "top")
+	defer DelContainerForceMultyTime(c, name)
+	res.Assert(c, icmd.Success)
+
+	res = command.PouchRun("exec", name, "id", "-u")
+	res.Assert(c, icmd.Success)
+	if !strings.Contains(res.Stdout(), "1001") {
+		c.Fatalf("failed to run a container with expected user: %s, but got %s", "1001", res.Stdout())
+	}
+
+	res = command.PouchRun("exec", "-u=1002", name, "id", "-u")
+	res.Assert(c, icmd.Success)
+	if !strings.Contains(res.Stdout(), "1002") {
+		c.Fatalf("failed to run a container with expected user: %s, but got %s", "1002", res.Stdout())
+	}
+
+	// test user should not changed by exec process
+	res = command.PouchRun("exec", name, "id", "-u")
+	res.Assert(c, icmd.Success)
+	if !strings.Contains(res.Stdout(), "1001") {
+		c.Fatalf("failed to run a container with expected user: %s, but got %s", "1001", res.Stdout())
+	}
+}