Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

oci: make sure cgroupns is enabled if supported #4003

Merged
merged 2 commits into from
Jul 10, 2023
Merged

oci: make sure cgroupns is enabled if supported #4003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c963649
oci: make sure cgroupns is enabled if supported
tonistiigi Jul 7, 2023
45b3856
update cgroup parent test to work with cgroupns
tonistiigi Jul 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 30 additions & 4 deletions client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -853,6 +853,10 @@ func testCgroupParent(t *testing.T, sb integration.Sandbox) {
t.SkipNow()
}

if _, err := os.Lstat("/sys/fs/cgroup/cgroup.subtree_control"); os.IsNotExist(err) {
t.Skipf("test requires cgroup v2")
}

c, err := New(sb.Context(), sb.Address())
require.NoError(t, err)
defer c.Close()
Expand All @@ -864,8 +868,21 @@ func testCgroupParent(t *testing.T, sb integration.Sandbox) {
st = img.Run(append(ro, llb.Shlex(cmd), llb.Dir("/wd"))...).AddMount("/wd", st)
}

run(`sh -c "cat /proc/self/cgroup > first"`, llb.WithCgroupParent("foocgroup"))
run(`sh -c "cat /proc/self/cgroup > second"`)
cgroupName := "test." + identity.NewID()

err = os.MkdirAll(filepath.Join("/sys/fs/cgroup", cgroupName), 0755)
require.NoError(t, err)

defer func() {
err := os.RemoveAll(filepath.Join("/sys/fs/cgroup", cgroupName))
require.NoError(t, err)
}()

err = os.WriteFile(filepath.Join("/sys/fs/cgroup", cgroupName, "pids.max"), []byte("10"), 0644)
require.NoError(t, err)

run(`sh -c "(for i in $(seq 1 10); do sleep 1 & done 2>first.error); cat /proc/self/cgroup >> first"`, llb.WithCgroupParent(cgroupName))
run(`sh -c "(for i in $(seq 1 10); do sleep 1 & done 2>second.error); cat /proc/self/cgroup >> second"`)

def, err := st.Marshal(sb.Context())
require.NoError(t, err)
Expand All @@ -882,13 +899,22 @@ func testCgroupParent(t *testing.T, sb integration.Sandbox) {
}, nil)
require.NoError(t, err)

// neither process leaks parent cgroup name inside container
dt, err := os.ReadFile(filepath.Join(destDir, "first"))
require.NoError(t, err)
require.Contains(t, strings.TrimSpace(string(dt)), `/foocgroup/buildkit/`)
require.NotContains(t, strings.TrimSpace(string(dt)), cgroupName)

dt2, err := os.ReadFile(filepath.Join(destDir, "second"))
require.NoError(t, err)
require.NotContains(t, strings.TrimSpace(string(dt2)), `/foocgroup/buildkit/`)
require.NotContains(t, strings.TrimSpace(string(dt2)), cgroupName)

dt, err = os.ReadFile(filepath.Join(destDir, "first.error"))
require.NoError(t, err)
require.Contains(t, strings.TrimSpace(string(dt)), "Resource temporarily unavailable")

dt, err = os.ReadFile(filepath.Join(destDir, "second.error"))
require.NoError(t, err)
require.Equal(t, strings.TrimSpace(string(dt)), "")
}

func testNetworkMode(t *testing.T, sb integration.Sandbox) {
Expand Down
6 changes: 6 additions & 0 deletions executor/oci/spec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,12 @@ func GenerateSpec(ctx context.Context, meta executor.Meta, mounts []executor.Mou
return nil, nil, err
}

if cgroupNamespaceSupported() {
s.Linux.Namespaces = append(s.Linux.Namespaces, specs.LinuxNamespace{
Type: specs.CgroupNamespace,
})
}

if len(meta.Ulimit) == 0 {
// reset open files limit
s.Process.Rlimits = nil
Expand Down
16 changes: 16 additions & 0 deletions executor/oci/spec_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ package oci
import (
"context"
"fmt"
"os"
"strings"
"sync"

"github.com/containerd/containerd/containers"
"github.com/containerd/containerd/oci"
Expand All @@ -21,6 +23,11 @@ import (
"github.com/pkg/errors"
)

var (
cgroupNSOnce sync.Once
supportsCgroupNS bool
)

const (
tracingSocketPath = "/dev/otel-grpc.sock"
)
Expand Down Expand Up @@ -139,3 +146,12 @@ func getTracingSocketMount(socket string) specs.Mount {
func getTracingSocket() string {
return fmt.Sprintf("unix://%s", tracingSocketPath)
}

func cgroupNamespaceSupported() bool {
Copy link
Member

Choose a reason for hiding this comment

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

Moby, containerd, etc. disables cgroup namespace on cgroup v1 regardless to kernel version/config.

Copy link
Member Author

@tonistiigi tonistiigi Jul 8, 2023 •
edited

Choose a reason for hiding this comment

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

Where is this done? In Runc code I see it checks ns existence for both cgroup versions.

https://github.com/opencontainers/runc/blob/main/libcontainer/rootfs_linux.go#L268

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/moby/moby/blob/c57097bcd461fcbc31f52b249c336f5c2e9732e8/daemon/daemon_unix.go#L345-L347

Copy link
Member Author

Choose a reason for hiding this comment

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

Iiuc this means privileged containers get host cgroups on v1? Is this what you mean and we should replicate it for --security=insecure. I don't quite see how it makes sense though to have different behaviour for privileged containers based on group versions.

Copy link
Member

Choose a reason for hiding this comment

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

Also see these consts in moby https://github.com/moby/moby/blob/462d6ef826861fad021fb565c0481fb61d2db6bc/daemon/config/config_linux.go#L23-L27 now used in https://github.com/moby/moby/blob/462d6ef826861fad021fb565c0481fb61d2db6bc/daemon/config/config_linux.go#L189-L193. So if not v2 then always set host mode.

cgroupNSOnce.Do(func() {
if _, err := os.Stat("/proc/self/ns/cgroup"); !os.IsNotExist(err) {
supportsCgroupNS = true
}
})
return supportsCgroupNS
}
4 changes: 4 additions & 0 deletions executor/oci/spec_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,7 @@ func getTracingSocketMount(socket string) specs.Mount {
func getTracingSocket() string {
return fmt.Sprintf("npipe://%s", filepath.ToSlash(tracingSocketPath))
}

func cgroupNamespaceSupported() bool {
return false
}
30 changes: 26 additions & 4 deletions frontend/dockerfile/dockerfile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5193,10 +5193,27 @@ func testCgroupParent(t *testing.T, sb integration.Sandbox) {
t.SkipNow()
}

if _, err := os.Lstat("/sys/fs/cgroup/cgroup.subtree_control"); os.IsNotExist(err) {
t.Skipf("test requires cgroup v2")
}

cgroupName := "test." + identity.NewID()

err := os.MkdirAll(filepath.Join("/sys/fs/cgroup", cgroupName), 0755)
require.NoError(t, err)

defer func() {
err := os.RemoveAll(filepath.Join("/sys/fs/cgroup", cgroupName))
require.NoError(t, err)
}()

err = os.WriteFile(filepath.Join("/sys/fs/cgroup", cgroupName, "pids.max"), []byte("10"), 0644)
require.NoError(t, err)

f := getFrontend(t, sb)
dockerfile := []byte(`
FROM alpine AS base
RUN cat /proc/self/cgroup > /out
RUN mkdir /out; (for i in $(seq 1 10); do sleep 1 & done 2>/out/error); cat /proc/self/cgroup > /out/cgroup
FROM scratch
COPY --from=base /out /
`)
Expand All @@ -5215,7 +5232,7 @@ COPY --from=base /out /

_, err = f.Solve(sb.Context(), c, client.SolveOpt{
FrontendAttrs: map[string]string{
"cgroup-parent": "foocgroup",
"cgroup-parent": cgroupName,
},
LocalDirs: map[string]string{
dockerui.DefaultLocalNameDockerfile: dir,
Expand All @@ -5230,9 +5247,14 @@ COPY --from=base /out /
}, nil)
require.NoError(t, err)

dt, err := os.ReadFile(filepath.Join(destDir, "out"))
dt, err := os.ReadFile(filepath.Join(destDir, "cgroup"))
require.NoError(t, err)
// cgroupns does not leak the parent cgroup name
require.NotContains(t, strings.TrimSpace(string(dt)), `foocgroup`)

dt, err = os.ReadFile(filepath.Join(destDir, "error"))
require.NoError(t, err)
require.Contains(t, strings.TrimSpace(string(dt)), `/foocgroup/buildkit/`)
require.Contains(t, strings.TrimSpace(string(dt)), `Resource temporarily unavailable`)
}

func testNamedImageContext(t *testing.T, sb integration.Sandbox) {
Expand Down