Skip to content
This repository was archived by the owner on Dec 13, 2018. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions config.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ type MountConfig mount.MountConfig

type Network network.Network

// Namespace defines configuration for each namespace. It specifies an
// alternate path that is able to be joined via setns.
type Namespace struct {
Name string `json:"name"`
Path string `json:"path,omitempty"`
}

// Config defines configuration options for executing a process inside a contained environment.
type Config struct {
// Mount specific options.
Expand Down Expand Up @@ -38,7 +45,7 @@ type Config struct {

// Namespaces specifies the container's namespaces that it should setup when cloning the init process
// If a namespace is not provided that namespace is shared from the container's parent process
Namespaces map[string]bool `json:"namespaces,omitempty"`
Namespaces []Namespace `json:"namespaces,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can me more convenient for usage if we'd use map[string]string. Because with array you need rewrite it totally to disable one namespace. Also there can be conflicting names.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting map from name to path? Does that stop us from evolving the Namespace struct in future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they are saying something like Namespaces map[string]Namespace

I'm not a big fan because you could have they key of the map say NEWNET and the Name in the struct say some other namespace, it does not enforce data consistency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a map[string]Namespace, I think we'll end up never checking the key and always iterate over it. Basically, we'll end up using it as an array anyway. What we have now looks fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with not doing a map since we are only talking about a few namespaces.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I propose name to path mapping and I think that this datastructure won't evolve in fields-way anyway because it is used in Config and this will be PITA for user to fill some complicated structures for config.
And I don't see what bad with map for now, apart from that we don't need abominations like getNamespaceIndex

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libcontainer is a low level API so i don't think this is a big issue for people, it maybe a little harder but I would rather have a flexible and consistent way to enforce the changes then the map[string]string where you don't know what is supposed to be in the key and why some have an empty string as the value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, there is similar namespaces.Namespace, maybe we can at least have one datastructure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LK4D4 updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hope that this won't lead to circular dependencies later :)


// Capabilities specify the capabilities to keep when executing the process inside the container
// All capbilities not specified will be dropped from the processes capability mask
Expand All @@ -47,9 +54,6 @@ type Config struct {
// Networks specifies the container's network setup to be created
Networks []*Network `json:"networks,omitempty"`

// Ipc specifies the container's ipc setup to be created
IpcNsPath string `json:"ipc,omitempty"`

// Routes can be specified to create entries in the route table as the container is started
Routes []*Route `json:"routes,omitempty"`

Expand Down
13 changes: 11 additions & 2 deletions config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,12 @@ func TestConfigJsonFormat(t *testing.T) {
t.Fail()
}

if !container.Namespaces["NEWNET"] {
if getNamespaceIndex(container, "NEWNET") == -1 {
t.Log("namespaces should contain NEWNET")
t.Fail()
}

if container.Namespaces["NEWUSER"] {
if getNamespaceIndex(container, "NEWUSER") != -1 {
t.Log("namespaces should not contain NEWUSER")
t.Fail()
}
Expand Down Expand Up @@ -158,3 +158,12 @@ func TestSelinuxLabels(t *testing.T) {
t.Fatalf("expected mount label %q but received %q", label, container.MountConfig.MountLabel)
}
}

func getNamespaceIndex(config *Config, name string) int {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually didn't got this trick. This is something like _, ok := container.Namespaces[name]?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it. Sorry.

for i, v := range config.Namespaces {
if v.Name == name {
return i
}
}
return -1
}
23 changes: 17 additions & 6 deletions integration/exec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"os"
"strings"
"testing"

"github.com/docker/libcontainer"
)

func TestExecPS(t *testing.T) {
Expand Down Expand Up @@ -55,7 +57,6 @@ func TestIPCPrivate(t *testing.T) {
}

config := newTemplateConfig(rootfs)
config.Namespaces["NEWIPC"] = true
buffers, exitCode, err := runContainer(config, "", "readlink", "/proc/self/ns/ipc")
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -87,7 +88,8 @@ func TestIPCHost(t *testing.T) {
}

config := newTemplateConfig(rootfs)
config.Namespaces["NEWIPC"] = false
i := getNamespaceIndex(config, "NEWIPC")
config.Namespaces = append(config.Namespaces[:i], config.Namespaces[i+1:]...)
buffers, exitCode, err := runContainer(config, "", "readlink", "/proc/self/ns/ipc")
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -119,8 +121,8 @@ func TestIPCJoinPath(t *testing.T) {
}

config := newTemplateConfig(rootfs)
config.Namespaces["NEWIPC"] = false
config.IpcNsPath = "/proc/1/ns/ipc"
i := getNamespaceIndex(config, "NEWIPC")
config.Namespaces[i].Path = "/proc/1/ns/ipc"

buffers, exitCode, err := runContainer(config, "", "readlink", "/proc/self/ns/ipc")
if err != nil {
Expand Down Expand Up @@ -148,8 +150,8 @@ func TestIPCBadPath(t *testing.T) {
defer remove(rootfs)

config := newTemplateConfig(rootfs)
config.Namespaces["NEWIPC"] = false
config.IpcNsPath = "/proc/1/ns/ipcc"
i := getNamespaceIndex(config, "NEWIPC")
config.Namespaces[i].Path = "/proc/1/ns/ipcc"

_, _, err = runContainer(config, "", "true")
if err == nil {
Expand Down Expand Up @@ -177,3 +179,12 @@ func TestRlimit(t *testing.T) {
t.Fatalf("expected rlimit to be 1024, got %s", limit)
}
}

func getNamespaceIndex(config *libcontainer.Config, name string) int {
for i, v := range config.Namespaces {
if v.Name == name {
return i
}
}
return -1
}
12 changes: 6 additions & 6 deletions integration/template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ func newTemplateConfig(rootfs string) *libcontainer.Config {
"KILL",
"AUDIT_WRITE",
},
Namespaces: map[string]bool{
"NEWNS": true,
"NEWUTS": true,
"NEWIPC": true,
"NEWPID": true,
"NEWNET": true,
Namespaces: []libcontainer.Namespace{
{Name: "NEWNS"},
{Name: "NEWUTS"},
{Name: "NEWIPC"},
{Name: "NEWPID"},
{Name: "NEWNET"},
},
Cgroups: &cgroups.Cgroup{
Parent: "integration",
Expand Down
29 changes: 0 additions & 29 deletions ipc/ipc.go

This file was deleted.

29 changes: 24 additions & 5 deletions namespaces/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
"github.com/docker/libcontainer"
"github.com/docker/libcontainer/apparmor"
"github.com/docker/libcontainer/console"
"github.com/docker/libcontainer/ipc"
"github.com/docker/libcontainer/label"
"github.com/docker/libcontainer/mount"
"github.com/docker/libcontainer/netlink"
Expand Down Expand Up @@ -65,7 +64,10 @@ func Init(container *libcontainer.Config, uncleanRootfs, consolePath string, pip
if err := json.NewDecoder(pipe).Decode(&networkState); err != nil {
return err
}

// join any namespaces via a path to the namespace fd if provided
if err := joinExistingNamespaces(container.Namespaces); err != nil {
return err
}
if consolePath != "" {
if err := console.OpenAndDup(consolePath); err != nil {
return err
Expand All @@ -79,9 +81,7 @@ func Init(container *libcontainer.Config, uncleanRootfs, consolePath string, pip
return fmt.Errorf("setctty %s", err)
}
}
if err := ipc.Initialize(container.IpcNsPath); err != nil {
return fmt.Errorf("setup IPC %s", err)
}

if err := setupNetwork(container, networkState); err != nil {
return fmt.Errorf("setup networking %s", err)
}
Expand Down Expand Up @@ -308,3 +308,22 @@ func LoadContainerEnvironment(container *libcontainer.Config) error {
}
return nil
}

// joinExistingNamespaces gets all the namespace paths specified for the container and
// does a setns on the namespace fd so that the current process joins the namespace.
func joinExistingNamespaces(namespaces []libcontainer.Namespace) error {
for _, ns := range namespaces {
if ns.Path != "" {
f, err := os.OpenFile(ns.Path, os.O_RDONLY, 0)
if err != nil {
return err
}
err = system.Setns(f.Fd(), uintptr(namespaceInfo[ns.Name]))
f.Close()
if err != nil {
return err
}
}
}
return nil
}
50 changes: 0 additions & 50 deletions namespaces/types.go

This file was deleted.

16 changes: 0 additions & 16 deletions namespaces/types_linux.go

This file was deleted.

30 changes: 0 additions & 30 deletions namespaces/types_test.go

This file was deleted.

21 changes: 14 additions & 7 deletions namespaces/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ package namespaces
import (
"os"
"syscall"

"github.com/docker/libcontainer"
)

type initError struct {
Expand All @@ -15,6 +17,15 @@ func (i initError) Error() string {
return i.Message
}

var namespaceInfo = map[string]int{
"NEWNET": syscall.CLONE_NEWNET,
"NEWNS": syscall.CLONE_NEWNS,
"NEWUSER": syscall.CLONE_NEWUSER,
"NEWIPC": syscall.CLONE_NEWIPC,
"NEWUTS": syscall.CLONE_NEWUTS,
"NEWPID": syscall.CLONE_NEWPID,
}

// New returns a newly initialized Pipe for communication between processes
func newInitPipe() (parent *os.File, child *os.File, err error) {
fds, err := syscall.Socketpair(syscall.AF_LOCAL, syscall.SOCK_STREAM|syscall.SOCK_CLOEXEC, 0)
Expand All @@ -26,13 +37,9 @@ func newInitPipe() (parent *os.File, child *os.File, err error) {

// GetNamespaceFlags parses the container's Namespaces options to set the correct
// flags on clone, unshare, and setns
func GetNamespaceFlags(namespaces map[string]bool) (flag int) {
for key, enabled := range namespaces {
if enabled {
if ns := GetNamespace(key); ns != nil {
flag |= ns.Value
}
}
func GetNamespaceFlags(namespaces []libcontainer.Namespace) (flag int) {
for _, v := range namespaces {
flag |= namespaceInfo[v.Name]
}
return flag
}
Loading