From 4f249bf73d7870392651bbc975a72518256aa76f Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Tue, 5 Mar 2019 14:14:03 -0800 Subject: [PATCH 1/3] sysctl: Add complete sysctl support We support most of the namespaced sysctls today as we pass them to libcontainer as part of the OCI spec. libcontainer then applies them for the container after veryfing they can be applied. However, the verification fails for network related sysctls as libcontainer expects a separate network namespace for network sysctls. This check fails for us as we create network namspace on the host side. To fix these, apply the network sysctls manually and purge them from the spec, leaving other sysctls to be applied by libcontainer. Fixes #472 Signed-off-by: Archana Shinde --- grpc.go | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/grpc.go b/grpc.go index c0cfe38c95..620fef0af6 100644 --- a/grpc.go +++ b/grpc.go @@ -657,6 +657,10 @@ func (a *agentGRPC) CreateContainer(ctx context.Context, req *pb.CreateContainer ociSpec.Linux.Resources.CPU.Cpus = availableCpuset } + if err := a.applyNetworkSysctls(ociSpec); err != nil { + return emptyResp, err + } + if a.sandbox.guestHooksPresent { // Add any custom OCI hooks to the spec a.sandbox.addGuestHooks(ociSpec) @@ -699,6 +703,41 @@ func (a *agentGRPC) CreateContainer(ctx context.Context, req *pb.CreateContainer return a.finishCreateContainer(ctr, req, config) } +// Path overridden in unit tests +var procSysDir = "/proc/sys" + +// writeSystemProperty writes the value to a path under /proc/sys as determined from the key. +// For e.g. net.ipv4.ip_forward translated to /proc/sys/net/ipv4/ip_forward. +func writeSystemProperty(key, value string) error { + keyPath := strings.Replace(key, ".", "/", -1) + return ioutil.WriteFile(filepath.Join(procSysDir, keyPath), []byte(value), 0644) +} + +func isNetworkSysctl(sysctl string) bool { + return strings.HasPrefix(sysctl, "net.") +} + +// libcontainer checks if the container is running in a separate network namespace +// before applying the network related sysctls. If it sees that the network namespace of the container +// is the same as the "host", it errors out. Since we do no create a new net namespace inside the guest, +// libcontainer would error out while verifying network sysctls. To overcome this, we dont pass +// network sysctls to libcontainer, we instead have the agent directly apply them. All other namespaced +// sysctls are applied by libcontainer. +func (a *agentGRPC) applyNetworkSysctls(ociSpec *specs.Spec) error { + sysctls := ociSpec.Linux.Sysctl + for key, value := range sysctls { + if isNetworkSysctl(key) { + if err := writeSystemProperty(key, value); err != nil { + return err + } + delete(sysctls, key) + } + } + + ociSpec.Linux.Sysctl = sysctls + return nil +} + func posixRlimitsToRlimits(posixRlimits []specs.POSIXRlimit) []configs.Rlimit { var rlimits []configs.Rlimit From 1f9ac240d45a15ae7f246cd09558cb69d19c0cd1 Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Tue, 5 Mar 2019 17:29:34 -0800 Subject: [PATCH 2/3] cpu: re-arrange code to reduce cyclomatic complexity Static checks complaining about cyclomatic complexity > 15 for CreateContainer Signed-off-by: Archana Shinde --- grpc.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/grpc.go b/grpc.go index 620fef0af6..b4946bd8b8 100644 --- a/grpc.go +++ b/grpc.go @@ -648,13 +648,8 @@ func (a *agentGRPC) CreateContainer(ctx context.Context, req *pb.CreateContainer return emptyResp, err } - if ociSpec.Linux.Resources.CPU != nil && ociSpec.Linux.Resources.CPU.Cpus != "" { - availableCpuset, err := getAvailableCpusetList(ociSpec.Linux.Resources.CPU.Cpus) - if err != nil { - return emptyResp, err - } - - ociSpec.Linux.Resources.CPU.Cpus = availableCpuset + if err := a.handleCPUSet(ociSpec); err != nil { + return emptyResp, err } if err := a.applyNetworkSysctls(ociSpec); err != nil { @@ -738,6 +733,18 @@ func (a *agentGRPC) applyNetworkSysctls(ociSpec *specs.Spec) error { return nil } +func (a *agentGRPC) handleCPUSet(ociSpec *specs.Spec) error { + if ociSpec.Linux.Resources.CPU != nil && ociSpec.Linux.Resources.CPU.Cpus != "" { + availableCpuset, err := getAvailableCpusetList(ociSpec.Linux.Resources.CPU.Cpus) + if err != nil { + return err + } + + ociSpec.Linux.Resources.CPU.Cpus = availableCpuset + } + return nil +} + func posixRlimitsToRlimits(posixRlimits []specs.POSIXRlimit) []configs.Rlimit { var rlimits []configs.Rlimit From d5e64f6018b8f1e73f2d2c039cb42fd0c2d255cd Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Wed, 6 Mar 2019 14:16:06 -0800 Subject: [PATCH 3/3] tests: Add test for cheking sysctls Tests check if network related sysctls are applied correctly by the agent. Signed-off-by: Archana Shinde --- grpc_test.go | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/grpc_test.go b/grpc_test.go index 7979d97873..f4f414cbbc 100644 --- a/grpc_test.go +++ b/grpc_test.go @@ -357,6 +357,98 @@ func TestListProcesses(t *testing.T) { assert.NotEmpty(r.ProcessList) } +func TestIsNetworkSysctl(t *testing.T) { + assert := assert.New(t) + + sysctl := "net.core.somaxconn" + isNet := isNetworkSysctl(sysctl) + assert.True(isNet) + + sysctl = "kernel.shmmax" + isNet = isNetworkSysctl(sysctl) + assert.False(isNet) +} + +func TestWriteSystemProperty(t *testing.T) { + assert := assert.New(t) + + tmpDir, err := ioutil.TempDir("", "procsys") + assert.Nil(err) + defer os.RemoveAll(tmpDir) + + key := "net.core.somaxconn" + value := "1024" + procSysDir = filepath.Join(tmpDir, "proc", "sys") + err = os.MkdirAll(procSysDir, 0755) + assert.Nil(err) + + netCoreDir := filepath.Join(procSysDir, "net", "core") + err = os.MkdirAll(netCoreDir, 0755) + assert.Nil(err) + + sysFile := filepath.Join(netCoreDir, "somaxconn") + fd, err := os.Create(sysFile) + assert.Nil(err) + fd.Close() + + err = writeSystemProperty(key, value) + assert.Nil(err) + + // Read file and verify + content, err := ioutil.ReadFile(sysFile) + assert.Nil(err) + assert.Equal(value, string(content)) + + // Following checks require root privileges to remove a read-only dir + if os.Geteuid() != 0 { + return + } + + // Remove write permissions for procSysDir to what they normally are + // for /proc/sys so that files cannot be created + err = os.Chmod(procSysDir, 0555) + assert.Nil(err) + + // Nonexistent sys file + key = "net.ipv4.ip_forward" + value = "1" + err = writeSystemProperty(key, value) + assert.NotNil(err) +} + +func TestApplyNetworkSysctls(t *testing.T) { + assert := assert.New(t) + a := &agentGRPC{} + + spec := &specs.Spec{} + spec.Linux = &specs.Linux{} + + spec.Linux.Sysctl = make(map[string]string) + spec.Linux.Sysctl["kernel.shmmax"] = "512" + + err := a.applyNetworkSysctls(spec) + assert.Nil(err) + assert.Equal(len(spec.Linux.Sysctl), 1) + assert.Equal(spec.Linux.Sysctl["kernel.shmmax"], "512") + + // Check with network sysctl + spec.Linux.Sysctl["net.core.somaxconn"] = "1024" + tmpDir, err := ioutil.TempDir("", "procsys") + assert.Nil(err) + defer os.RemoveAll(tmpDir) + + procSysDir = filepath.Join(tmpDir, "proc", "sys") + netCoreDir := filepath.Join(procSysDir, "net", "core") + err = os.MkdirAll(netCoreDir, 0755) + assert.Nil(err) + + assert.Equal(len(spec.Linux.Sysctl), 2) + err = a.applyNetworkSysctls(spec) + assert.Nil(err) + assert.Equal(len(spec.Linux.Sysctl), 1) + assert.Equal(spec.Linux.Sysctl["kernel.shmmax"], "512") +} + func TestUpdateContainer(t *testing.T) { containerID := "1" assert := assert.New(t)