refactor(container): rename the rootless option UnsafeAllowRootlessForTests and test that production never sets it (G4)
- commit
- 610fdc0b4a640eb203d41689134605b44e5d1625
- author
- brice · · 2026-10-12 19:40 UTC
- parent
- 8ec9ad2
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +50 -7
M README.md +1 -1
| @@ -217,7 +217,7 @@ sudo env GOWORK=off ERMITE_RUNNER_IT_CONTAINER=1 ERMITE_RUNNER_IMAGE=ermite-runn | |||
| 217 | 217 | DOCKER_HOST=unix:///var/run/docker.sock "$(command -v go)" test -count=1 -v ./internal/sandbox/integration -run Container | |
| 218 | 218 | ``` | |
| 219 | 219 | ||
| 220 | - | **Jobs end to end** (`ERMITE_RUNNER_IT_E2E=1`, `internal/e2e`): the real executor in the real image, started through the container driver. They also run on a workstation with a *rootless* Docker daemon (the test-only option `AllowRootless`; the nested `dockerd` of the image cannot start there, and the jobs do not need it). `mount` and `umount` are replaced by stand-ins, since loop mounts need root. | |
| 220 | + | **Jobs end to end** (`ERMITE_RUNNER_IT_E2E=1`, `internal/e2e`): the real executor in the real image, started through the container driver. They also run on a workstation with a *rootless* Docker daemon (the test-only option `UnsafeAllowRootlessForTests`; the nested `dockerd` of the image cannot start there, and the jobs do not need it). `mount` and `umount` are replaced by stand-ins, since loop mounts need root. | |
| 221 | 221 | ||
| 222 | 222 | ```sh | |
| 223 | 223 | IMAGE_TAG=ermite-runner-ubuntu-24.04:dev scripts/build-image.sh tmp/image # or: docker build -t ermite-runner-ubuntu-24.04:dev images/ubuntu-24.04 | |
| @@ -217,7 +217,7 @@ sudo env GOWORK=off ERMITE_RUNNER_IT_CONTAINER=1 ERMITE_RUNNER_IMAGE=ermite-runn | |||||
| 217 | DOCKER_HOST=unix:///var/run/docker.sock "$(command -v go)" test -count=1 -v ./internal/sandbox/integration -run Container | 217 | DOCKER_HOST=unix:///var/run/docker.sock "$(command -v go)" test -count=1 -v ./internal/sandbox/integration -run Container | ||
| 218 | ``` | 218 | ``` | ||
| 219 | 219 | ||||
| 220 | - | **Jobs end to end** (`ERMITE_RUNNER_IT_E2E=1`, `internal/e2e`): the real executor in the real image, started through the container driver. They also run on a workstation with a *rootless* Docker daemon (the test-only option `AllowRootless`; the nested `dockerd` of the image cannot start there, and the jobs do not need it). `mount` and `umount` are replaced by stand-ins, since loop mounts need root. | 220 | + | **Jobs end to end** (`ERMITE_RUNNER_IT_E2E=1`, `internal/e2e`): the real executor in the real image, started through the container driver. They also run on a workstation with a *rootless* Docker daemon (the test-only option `UnsafeAllowRootlessForTests`; the nested `dockerd` of the image cannot start there, and the jobs do not need it). `mount` and `umount` are replaced by stand-ins, since loop mounts need root. |
| 221 | 221 | ||||
| 222 | ```sh | 222 | ```sh | ||
| 223 | IMAGE_TAG=ermite-runner-ubuntu-24.04:dev scripts/build-image.sh tmp/image # or: docker build -t ermite-runner-ubuntu-24.04:dev images/ubuntu-24.04 | 223 | IMAGE_TAG=ermite-runner-ubuntu-24.04:dev scripts/build-image.sh tmp/image # or: docker build -t ermite-runner-ubuntu-24.04:dev images/ubuntu-24.04 | ||
M internal/e2e/e2e_test.go +2 -2
| @@ -6,7 +6,7 @@ | |||
| 6 | 6 | // ERMITE_RUNNER_IT_E2E=1 ERMITE_RUNNER_TEST_IMAGE=ermite-runner-ubuntu-24.04:dev go test ./internal/e2e | |
| 7 | 7 | // | |
| 8 | 8 | // On a workstation with a rootless Docker daemon the container driver is used | |
| 9 | - | // with AllowRootless (a test-only option): the jobs run as in production except | |
| 9 | + | // with UnsafeAllowRootlessForTests (a test-only option): the jobs run as in production except | |
| 10 | 10 | // that the nested dockerd of the image cannot start, which these tests do not | |
| 11 | 11 | // need. The production paths that need root (jailer, rootful Docker, sysbox) | |
| 12 | 12 | // are not covered here. | |
| @@ -79,7 +79,7 @@ func newDriver(t *testing.T, image string) sandbox.Driver { | |||
| 79 | 79 | _ = os.WriteFile(empty, nil, 0o644) | |
| 80 | 80 | run := rootlessMountRunner{image: image, mountinfo: empty, mu: &sync.Mutex{}} | |
| 81 | 81 | d, err := container.New(context.Background(), cfg, container.Options{ | |
| 82 | - | Net: sandboxtest.NewPlumbing(), AllowRootless: true, ConnectTimeout: 90 * time.Second, Run: run, | |
| 82 | + | Net: sandboxtest.NewPlumbing(), UnsafeAllowRootlessForTests: true, ConnectTimeout: 90 * time.Second, Run: run, | |
| 83 | 83 | Mounter: &disk.Mounter{Run: run, Mountinfo: empty}, | |
| 84 | 84 | }) | |
| 85 | 85 | if err != nil { | |
| @@ -6,7 +6,7 @@ | |||||
| 6 | // ERMITE_RUNNER_IT_E2E=1 ERMITE_RUNNER_TEST_IMAGE=ermite-runner-ubuntu-24.04:dev go test ./internal/e2e | 6 | // ERMITE_RUNNER_IT_E2E=1 ERMITE_RUNNER_TEST_IMAGE=ermite-runner-ubuntu-24.04:dev go test ./internal/e2e | ||
| 7 | // | 7 | // | ||
| 8 | // On a workstation with a rootless Docker daemon the container driver is used | 8 | // On a workstation with a rootless Docker daemon the container driver is used | ||
| 9 | - | // with AllowRootless (a test-only option): the jobs run as in production except | 9 | + | // with UnsafeAllowRootlessForTests (a test-only option): the jobs run as in production except |
| 10 | // that the nested dockerd of the image cannot start, which these tests do not | 10 | // that the nested dockerd of the image cannot start, which these tests do not | ||
| 11 | // need. The production paths that need root (jailer, rootful Docker, sysbox) | 11 | // need. The production paths that need root (jailer, rootful Docker, sysbox) | ||
| 12 | // are not covered here. | 12 | // are not covered here. | ||
| @@ -79,7 +79,7 @@ func newDriver(t *testing.T, image string) sandbox.Driver { | |||||
| 79 | _ = os.WriteFile(empty, nil, 0o644) | 79 | _ = os.WriteFile(empty, nil, 0o644) | ||
| 80 | run := rootlessMountRunner{image: image, mountinfo: empty, mu: &sync.Mutex{}} | 80 | run := rootlessMountRunner{image: image, mountinfo: empty, mu: &sync.Mutex{}} | ||
| 81 | d, err := container.New(context.Background(), cfg, container.Options{ | 81 | d, err := container.New(context.Background(), cfg, container.Options{ | ||
| 82 | - | Net: sandboxtest.NewPlumbing(), AllowRootless: true, ConnectTimeout: 90 * time.Second, Run: run, | 82 | + | Net: sandboxtest.NewPlumbing(), UnsafeAllowRootlessForTests: true, ConnectTimeout: 90 * time.Second, Run: run, |
| 83 | Mounter: &disk.Mounter{Run: run, Mountinfo: empty}, | 83 | Mounter: &disk.Mounter{Run: run, Mountinfo: empty}, | ||
| 84 | }) | 84 | }) | ||
| 85 | if err != nil { | 85 | if err != nil { | ||
M internal/sandbox/container/docker_api_test.go +1 -1
| @@ -62,7 +62,7 @@ func TestDockerAPIWiring(t *testing.T) { | |||
| 62 | 62 | run := noMountRunner{} | |
| 63 | 63 | empty := filepath.Join(dir, "mountinfo") | |
| 64 | 64 | _ = os.WriteFile(empty, nil, 0o644) | |
| 65 | - | d, err := New(ctx, cfg, Options{Net: plumb, AllowRootless: true, ConnectTimeout: 60 * time.Second, Run: run, | |
| 65 | + | d, err := New(ctx, cfg, Options{Net: plumb, UnsafeAllowRootlessForTests: true, ConnectTimeout: 60 * time.Second, Run: run, | |
| 66 | 66 | Mounter: &disk.Mounter{Run: run, Mountinfo: empty}}) | |
| 67 | 67 | if err != nil { | |
| 68 | 68 | t.Fatal(err) | |
| @@ -62,7 +62,7 @@ func TestDockerAPIWiring(t *testing.T) { | |||||
| 62 | run := noMountRunner{} | 62 | run := noMountRunner{} | ||
| 63 | empty := filepath.Join(dir, "mountinfo") | 63 | empty := filepath.Join(dir, "mountinfo") | ||
| 64 | _ = os.WriteFile(empty, nil, 0o644) | 64 | _ = os.WriteFile(empty, nil, 0o644) | ||
| 65 | - | d, err := New(ctx, cfg, Options{Net: plumb, AllowRootless: true, ConnectTimeout: 60 * time.Second, Run: run, | 65 | + | d, err := New(ctx, cfg, Options{Net: plumb, UnsafeAllowRootlessForTests: true, ConnectTimeout: 60 * time.Second, Run: run, |
| 66 | Mounter: &disk.Mounter{Run: run, Mountinfo: empty}}) | 66 | Mounter: &disk.Mounter{Run: run, Mountinfo: empty}}) | ||
| 67 | if err != nil { | 67 | if err != nil { | ||
| 68 | t.Fatal(err) | 68 | t.Fatal(err) | ||
M internal/sandbox/container/driver.go +3 -3
| @@ -53,11 +53,11 @@ type Options struct { | |||
| 53 | 53 | Net netsetup.Plumbing | |
| 54 | 54 | // engine overrides the Docker client (tests). | |
| 55 | 55 | engine engine | |
| 56 | - | // AllowRootless lets tests exercise the Engine API and the executor against | |
| 56 | + | // UnsafeAllowRootlessForTests lets tests exercise the Engine API and the executor against | |
| 57 | 57 | // the rootless daemon of a workstation. Never set outside tests: a nested | |
| 58 | 58 | // dockerd cannot work in a rootless daemon, so the jobs would run without | |
| 59 | 59 | // Docker. The factories of package drivers never set it. | |
| 60 | - | AllowRootless bool | |
| 60 | + | UnsafeAllowRootlessForTests bool | |
| 61 | 61 | // ConnectTimeout bounds container start + init + executor connection | |
| 62 | 62 | // (default 90 s). | |
| 63 | 63 | ConnectTimeout time.Duration | |
| @@ -134,7 +134,7 @@ func New(ctx context.Context, cfg sandbox.Config, opt Options) (*Driver, error) | |||
| 134 | 134 | if err != nil { | |
| 135 | 135 | return fail(fmt.Errorf("cannot reach the Docker daemon (DOCKER_HOST): %w", err)) | |
| 136 | 136 | } | |
| 137 | - | if info.Rootless && !opt.AllowRootless { | |
| 137 | + | if info.Rootless && !opt.UnsafeAllowRootlessForTests { | |
| 138 | 138 | return fail(errRootless) | |
| 139 | 139 | } | |
| 140 | 140 | if info.OSType != "" && info.OSType != "linux" { | |
| @@ -53,11 +53,11 @@ type Options struct { | |||||
| 53 | Net netsetup.Plumbing | 53 | Net netsetup.Plumbing | ||
| 54 | // engine overrides the Docker client (tests). | 54 | // engine overrides the Docker client (tests). | ||
| 55 | engine engine | 55 | engine engine | ||
| 56 | - | // AllowRootless lets tests exercise the Engine API and the executor against | 56 | + | // UnsafeAllowRootlessForTests lets tests exercise the Engine API and the executor against |
| 57 | // the rootless daemon of a workstation. Never set outside tests: a nested | 57 | // the rootless daemon of a workstation. Never set outside tests: a nested | ||
| 58 | // dockerd cannot work in a rootless daemon, so the jobs would run without | 58 | // dockerd cannot work in a rootless daemon, so the jobs would run without | ||
| 59 | // Docker. The factories of package drivers never set it. | 59 | // Docker. The factories of package drivers never set it. | ||
| 60 | - | AllowRootless bool | 60 | + | UnsafeAllowRootlessForTests bool |
| 61 | // ConnectTimeout bounds container start + init + executor connection | 61 | // ConnectTimeout bounds container start + init + executor connection | ||
| 62 | // (default 90 s). | 62 | // (default 90 s). | ||
| 63 | ConnectTimeout time.Duration | 63 | ConnectTimeout time.Duration | ||
| @@ -134,7 +134,7 @@ func New(ctx context.Context, cfg sandbox.Config, opt Options) (*Driver, error) | |||||
| 134 | if err != nil { | 134 | if err != nil { | ||
| 135 | return fail(fmt.Errorf("cannot reach the Docker daemon (DOCKER_HOST): %w", err)) | 135 | return fail(fmt.Errorf("cannot reach the Docker daemon (DOCKER_HOST): %w", err)) | ||
| 136 | } | 136 | } | ||
| 137 | - | if info.Rootless && !opt.AllowRootless { | 137 | + | if info.Rootless && !opt.UnsafeAllowRootlessForTests { |
| 138 | return fail(errRootless) | 138 | return fail(errRootless) | ||
| 139 | } | 139 | } | ||
| 140 | if info.OSType != "" && info.OSType != "linux" { | 140 | if info.OSType != "" && info.OSType != "linux" { | ||
A internal/sandbox/drivers/drivers_test.go +43 -0
| @@ -0,0 +1,43 @@ | |||
| 1 | + | // SPDX-License-Identifier: MIT | |
| 2 | + | ||
| 3 | + | package drivers | |
| 4 | + | ||
| 5 | + | import ( | |
| 6 | + | "go/ast" | |
| 7 | + | "go/parser" | |
| 8 | + | "go/token" | |
| 9 | + | "testing" | |
| 10 | + | ) | |
| 11 | + | ||
| 12 | + | // The container option that lets a driver run against a rootless Docker daemon | |
| 13 | + | // exists for tests: with it the nested dockerd of the job cannot start, so jobs | |
| 14 | + | // would run without Docker and with weaker isolation than the runner promises. | |
| 15 | + | // Production code (this package builds the drivers of `ermite-runner run`) must | |
| 16 | + | // never mention it, neither as a field of Options nor in a value. | |
| 17 | + | func TestProductionFactoriesNeverSetTheTestOnlyRootlessOption(t *testing.T) { | |
| 18 | + | fset := token.NewFileSet() | |
| 19 | + | files, err := parser.ParseFile(fset, "drivers.go", nil, 0) | |
| 20 | + | if err != nil { | |
| 21 | + | t.Fatal(err) | |
| 22 | + | } | |
| 23 | + | found := false | |
| 24 | + | ast.Inspect(files, func(n ast.Node) bool { | |
| 25 | + | switch x := n.(type) { | |
| 26 | + | case *ast.Ident: | |
| 27 | + | if x.Name == "UnsafeAllowRootlessForTests" || x.Name == "AllowRootless" { | |
| 28 | + | t.Errorf("%s: %s is a test-only option", fset.Position(x.Pos()), x.Name) | |
| 29 | + | } | |
| 30 | + | case *ast.CompositeLit: | |
| 31 | + | if sel, ok := x.Type.(*ast.SelectorExpr); ok && sel.Sel.Name == "Options" { | |
| 32 | + | found = true | |
| 33 | + | if len(x.Elts) != 0 { | |
| 34 | + | t.Errorf("%s: %s{...} sets options in production code", fset.Position(x.Pos()), sel.Sel.Name) | |
| 35 | + | } | |
| 36 | + | } | |
| 37 | + | } | |
| 38 | + | return true | |
| 39 | + | }) | |
| 40 | + | if !found { | |
| 41 | + | t.Fatal("drivers.go no longer builds container.Options{}: update this test") | |
| 42 | + | } | |
| 43 | + | } | |
| @@ -0,0 +1,43 @@ | |||||
| 1 | + | // SPDX-License-Identifier: MIT | |||
| 2 | + | ||||
| 3 | + | package drivers | |||
| 4 | + | ||||
| 5 | + | import ( | |||
| 6 | + | "go/ast" | |||
| 7 | + | "go/parser" | |||
| 8 | + | "go/token" | |||
| 9 | + | "testing" | |||
| 10 | + | ) | |||
| 11 | + | ||||
| 12 | + | // The container option that lets a driver run against a rootless Docker daemon | |||
| 13 | + | // exists for tests: with it the nested dockerd of the job cannot start, so jobs | |||
| 14 | + | // would run without Docker and with weaker isolation than the runner promises. | |||
| 15 | + | // Production code (this package builds the drivers of `ermite-runner run`) must | |||
| 16 | + | // never mention it, neither as a field of Options nor in a value. | |||
| 17 | + | func TestProductionFactoriesNeverSetTheTestOnlyRootlessOption(t *testing.T) { | |||
| 18 | + | fset := token.NewFileSet() | |||
| 19 | + | files, err := parser.ParseFile(fset, "drivers.go", nil, 0) | |||
| 20 | + | if err != nil { | |||
| 21 | + | t.Fatal(err) | |||
| 22 | + | } | |||
| 23 | + | found := false | |||
| 24 | + | ast.Inspect(files, func(n ast.Node) bool { | |||
| 25 | + | switch x := n.(type) { | |||
| 26 | + | case *ast.Ident: | |||
| 27 | + | if x.Name == "UnsafeAllowRootlessForTests" || x.Name == "AllowRootless" { | |||
| 28 | + | t.Errorf("%s: %s is a test-only option", fset.Position(x.Pos()), x.Name) | |||
| 29 | + | } | |||
| 30 | + | case *ast.CompositeLit: | |||
| 31 | + | if sel, ok := x.Type.(*ast.SelectorExpr); ok && sel.Sel.Name == "Options" { | |||
| 32 | + | found = true | |||
| 33 | + | if len(x.Elts) != 0 { | |||
| 34 | + | t.Errorf("%s: %s{...} sets options in production code", fset.Position(x.Pos()), sel.Sel.Name) | |||
| 35 | + | } | |||
| 36 | + | } | |||
| 37 | + | } | |||
| 38 | + | return true | |||
| 39 | + | }) | |||
| 40 | + | if !found { | |||
| 41 | + | t.Fatal("drivers.go no longer builds container.Options{}: update this test") | |||
| 42 | + | } | |||
| 43 | + | } | |||