From d885782df1ecb0a60c8b76caf652644aa62a71ef Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Thu, 10 May 2018 17:43:41 -0700 Subject: [PATCH] namespace: Check if pid namespaces need to be shared k8s provides a configuration for sharing PID namespace among containers. In case of crio and cri plugin, an infra container is started first. All following containers are supposed to share the pid namespace of this container. In case a non-empty pid namespace path is provided for a container, we check for the above condition while creating a container and pass this out to the kata agent in the CreatContainer request as SandboxPidNs flag. We clear out the PID namespaces in the configuration passed to the kata agent. Fixes #343 Signed-off-by: Archana Shinde --- virtcontainers/container.go | 7 +++ virtcontainers/kata_agent.go | 67 ++++++++++++++++++++++---- virtcontainers/kata_agent_test.go | 79 +++++++++++++++++++++++++++++++ virtcontainers/sandbox.go | 13 +++++ 4 files changed, 158 insertions(+), 8 deletions(-) diff --git a/virtcontainers/container.go b/virtcontainers/container.go index 2c5ac2d082..af9b459953 100644 --- a/virtcontainers/container.go +++ b/virtcontainers/container.go @@ -15,6 +15,7 @@ import ( "syscall" "time" + "github.com/kata-containers/runtime/virtcontainers/pkg/annotations" specs "github.com/opencontainers/runtime-spec/specs-go" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" @@ -657,6 +658,12 @@ func createContainer(sandbox *Sandbox, contConfig ContainerConfig) (c *Container } c.process = *process + // If this is a sandbox container, store the pid for sandbox + ann := c.GetAnnotations() + if ann[annotations.ContainerTypeKey] == string(PodSandbox) { + sandbox.setSandboxPid(c.process.Pid) + } + // Store the container process returned by the agent. if err = c.storeProcess(); err != nil { return diff --git a/virtcontainers/kata_agent.go b/virtcontainers/kata_agent.go index 70440a2493..de1d0daa21 100644 --- a/virtcontainers/kata_agent.go +++ b/virtcontainers/kata_agent.go @@ -506,9 +506,8 @@ func (k *kataAgent) startSandbox(sandbox *Sandbox) error { } req := &grpc.CreateSandboxRequest{ - Hostname: hostname, - Storages: []*grpc.Storage{sharedVolume}, - SandboxPidns: false, + Hostname: hostname, + Storages: []*grpc.Storage{sharedVolume}, } _, err = k.sendReq(req) @@ -777,16 +776,22 @@ func (k *kataAgent) createContainer(sandbox *Sandbox, c *Container) (p *Process, // We need to give the OCI spec our absolute rootfs path in the guest. grpcSpec.Root.Path = rootPath + sharedPidNs, err := k.handlePidNamespace(grpcSpec, sandbox) + if err != nil { + return nil, err + } + // We need to constraint the spec to make sure we're not passing // irrelevant information to the agent. constraintGRPCSpec(grpcSpec) req := &grpc.CreateContainerRequest{ - ContainerId: c.id, - ExecId: c.id, - Storages: ctrStorages, - Devices: ctrDevices, - OCI: grpcSpec, + ContainerId: c.id, + ExecId: c.id, + Storages: ctrStorages, + Devices: ctrDevices, + OCI: grpcSpec, + SandboxPidns: sharedPidNs, } if _, err = k.sendReq(req); err != nil { @@ -843,6 +848,52 @@ func (k *kataAgent) handleBlockVolumes(c *Container) []*grpc.Storage { return volumeStorages } +// handlePidNamespace checks if Pid namespace for a container needs to be shared with its sandbox +// pid namespace. This function also modifies the grpc spec to remove the pid namespace +// from the list of namespaces passed to the agent. +func (k *kataAgent) handlePidNamespace(grpcSpec *grpc.Spec, sandbox *Sandbox) (bool, error) { + sharedPidNs := false + pidIndex := -1 + + for i, ns := range grpcSpec.Linux.Namespaces { + if ns.Type != string(specs.PIDNamespace) { + continue + } + + pidIndex = i + + if ns.Path == "" || sandbox.state.Pid == 0 { + break + } + + pidNsPath := fmt.Sprintf("/proc/%d/ns/pid", sandbox.state.Pid) + + // Check if pid namespace path is the same as the sandbox + if ns.Path == pidNsPath { + sharedPidNs = true + } else { + ln, err := filepath.EvalSymlinks(ns.Path) + if err != nil { + return sharedPidNs, err + } + + // We have arbitrary pid namespace path here. + if ln != pidNsPath { + return sharedPidNs, fmt.Errorf("Pid namespace path %s other than sandbox %s", ln, pidNsPath) + } + sharedPidNs = true + } + + break + } + + // Remove pid namespace. + if pidIndex >= 0 { + grpcSpec.Linux.Namespaces = append(grpcSpec.Linux.Namespaces[:pidIndex], grpcSpec.Linux.Namespaces[pidIndex+1:]...) + } + return sharedPidNs, nil +} + func (k *kataAgent) startContainer(sandbox *Sandbox, c *Container) error { req := &grpc.StartContainerRequest{ ContainerId: c.id, diff --git a/virtcontainers/kata_agent_test.go b/virtcontainers/kata_agent_test.go index 41ad8f15d9..081debdfea 100644 --- a/virtcontainers/kata_agent_test.go +++ b/virtcontainers/kata_agent_test.go @@ -455,3 +455,82 @@ func TestConstraintGRPCSpec(t *testing.T) { assert.NotEmpty(g.Mounts[0].Source) assert.NotEmpty(g.Mounts[0].Options) } + +func testIsPidNamespacePresent(grpcSpec *pb.Spec) bool { + for _, ns := range grpcSpec.Linux.Namespaces { + if ns.Type == string(specs.PIDNamespace) { + return true + } + } + + return false +} + +func TestHandlePidNamespace(t *testing.T) { + assert := assert.New(t) + + g := &pb.Spec{ + Linux: &pb.Linux{ + Namespaces: []pb.LinuxNamespace{ + { + Type: specs.NetworkNamespace, + Path: "/abc/123", + }, + { + Type: specs.MountNamespace, + Path: "/abc/123", + }, + }, + }, + } + + sandbox := &Sandbox{} + sandbox.state.Pid = 0 + + k := kataAgent{} + + sharedPid, err := k.handlePidNamespace(g, sandbox) + assert.Nil(err) + assert.False(sharedPid) + assert.False(testIsPidNamespacePresent(g)) + + pidNs := pb.LinuxNamespace{ + Type: string(specs.PIDNamespace), + Path: "", + } + + utsNs := pb.LinuxNamespace{ + Type: specs.UTSNamespace, + Path: "", + } + + g.Linux.Namespaces = append(g.Linux.Namespaces, pidNs) + g.Linux.Namespaces = append(g.Linux.Namespaces, utsNs) + + sharedPid, err = k.handlePidNamespace(g, sandbox) + assert.Nil(err) + assert.False(sharedPid) + assert.False(testIsPidNamespacePresent(g)) + + sandbox.state.Pid = 112 + pidNs = pb.LinuxNamespace{ + Type: string(specs.PIDNamespace), + Path: "/proc/112/ns/pid", + } + g.Linux.Namespaces = append(g.Linux.Namespaces, pidNs) + + sharedPid, err = k.handlePidNamespace(g, sandbox) + assert.Nil(err) + assert.True(sharedPid) + assert.False(testIsPidNamespacePresent(g)) + + // Arbitrary path + pidNs = pb.LinuxNamespace{ + Type: string(specs.PIDNamespace), + Path: "/proc/234/ns/pid", + } + g.Linux.Namespaces = append(g.Linux.Namespaces, pidNs) + + _, err = k.handlePidNamespace(g, sandbox) + assert.NotNil(err) +} diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index d644a67e0c..7b7459e5ac 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -70,6 +70,10 @@ type State struct { // PCI slot at which the block device backing the container rootfs is attached. RootfsPCIAddr string `json:"rootfsPCIAddr"` + + // Pid is the process id of the sandbox container which is the first + // container to be started. + Pid int `json:"pid"` } // valid checks that the sandbox state is valid. @@ -1274,6 +1278,15 @@ func (s *Sandbox) decrementSandboxBlockIndex() error { return nil } +// setSandboxPid sets the Pid of the the shim process belonging to the +// sandbox container as the Pid of the sandbox. +func (s *Sandbox) setSandboxPid(pid int) error { + s.state.Pid = pid + + // update on-disk state + return s.storage.storeSandboxResource(s.id, stateFileType, s.state) +} + func (s *Sandbox) setContainersState(state stateString) error { if state == "" { return errNeedState