Skip to content

Commit 0f8270b

Browse files
vvolandvvoland
committed
api/pre-1.44: Default ReadOnlyNonRecursive to true
Don't change the behavior for older clients and keep the same behavior. Otherwise client can't opt-out (because `ReadOnlyNonRecursive` is unsupported before 1.44). Signed-off-by: Paweł Gronowski <[email protected]>
1 parent 05b883b commit 0f8270b

8 files changed

Lines changed: 136 additions & 29 deletions

File tree

‎api/server/router/container/container_routes.go‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -556,17 +556,27 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo
556556
hostConfig.Annotations = nil
557557
}
558558

559+
defaultReadOnlyNonRecursive := false
559560
if versions.LessThan(version, "1.44") {
560561
if config.Healthcheck != nil {
561562
// StartInterval was added in API 1.44
562563
config.Healthcheck.StartInterval = 0
563564
}
564565

566+
// Set ReadOnlyNonRecursive to true because it was added in API 1.44
567+
// Before that all read-only mounts were non-recursive.
568+
// Keep that behavior for clients on older APIs.
569+
defaultReadOnlyNonRecursive = true
570+
565571
for _, m := range hostConfig.Mounts {
566-
if m.BindOptions != nil {
567-
// Ignore ReadOnlyNonRecursive because it was added in API 1.44.
568-
m.BindOptions.ReadOnlyNonRecursive = false
569-
if m.BindOptions.ReadOnlyForceRecursive {
572+
if m.Type == mount.TypeBind {
573+
if m.BindOptions != nil && m.BindOptions.ReadOnlyForceRecursive {
574+
// NOTE: that technically this is a breaking change for older
575+
// API versions, and we should ignore the new field.
576+
// However, this option may be incorrectly set by a client with
577+
// the expectation that the failing to apply recursive read-only
578+
// is enforced, so we decided to produce an error instead,
579+
// instead of silently ignoring.
570580
return errdefs.InvalidParameter(errors.New("BindOptions.ReadOnlyForceRecursive needs API v1.44 or newer"))
571581
}
572582
}
@@ -606,11 +616,12 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo
606616
}
607617

608618
ccr, err := s.backend.ContainerCreate(ctx, backend.ContainerCreateConfig{
609-
Name: name,
610-
Config: config,
611-
HostConfig: hostConfig,
612-
NetworkingConfig: networkingConfig,
613-
Platform: platform,
619+
Name: name,
620+
Config: config,
621+
HostConfig: hostConfig,
622+
NetworkingConfig: networkingConfig,
623+
Platform: platform,
624+
DefaultReadOnlyNonRecursive: defaultReadOnlyNonRecursive,
614625
})
615626
if err != nil {
616627
return err

‎api/swagger.yaml‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,11 @@ definitions:
391391
ReadOnlyNonRecursive:
392392
description: |
393393
Make the mount non-recursively read-only, but still leave the mount recursive
394-
(unless NonRecursive is set to true in conjunction).
394+
(unless NonRecursive is set to `true` in conjunction).
395+
396+
Addded in v1.44, before that version all read-only mounts were
397+
non-recursive by default. To match the previous behaviour this
398+
will default to `true` for clients on versions prior to v1.44.
395399
type: "boolean"
396400
default: false
397401
ReadOnlyForceRecursive:

‎api/types/backend/backend.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,12 @@ import (
1313

1414
// ContainerCreateConfig is the parameter set to ContainerCreate()
1515
type ContainerCreateConfig struct {
16-
Name string
17-
Config *container.Config
18-
HostConfig *container.HostConfig
19-
NetworkingConfig *network.NetworkingConfig
20-
Platform *ocispec.Platform
16+
Name string
17+
Config *container.Config
18+
HostConfig *container.HostConfig
19+
NetworkingConfig *network.NetworkingConfig
20+
Platform *ocispec.Platform
21+
DefaultReadOnlyNonRecursive bool
2122
}
2223

2324
// ContainerRmConfig holds arguments for the container remove

‎daemon/container.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -203,10 +203,10 @@ func (daemon *Daemon) setSecurityOptions(cfg *config.Config, container *containe
203203
return daemon.parseSecurityOpt(cfg, &container.SecurityOptions, hostConfig)
204204
}
205205

206-
func (daemon *Daemon) setHostConfig(container *container.Container, hostConfig *containertypes.HostConfig) error {
206+
func (daemon *Daemon) setHostConfig(container *container.Container, hostConfig *containertypes.HostConfig, defaultReadOnlyNonRecursive bool) error {
207207
// Do not lock while creating volumes since this could be calling out to external plugins
208208
// Don't want to block other actions, like `docker ps` because we're waiting on an external plugin
209-
if err := daemon.registerMountPoints(container, hostConfig); err != nil {
209+
if err := daemon.registerMountPoints(container, hostConfig, defaultReadOnlyNonRecursive); err != nil {
210210
return err
211211
}
212212

‎daemon/create.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ func (daemon *Daemon) create(ctx context.Context, daemonCfg *config.Config, opts
217217
return nil, err
218218
}
219219

220-
if err := daemon.setHostConfig(ctr, opts.params.HostConfig); err != nil {
220+
if err := daemon.setHostConfig(ctr, opts.params.HostConfig, opts.params.DefaultReadOnlyNonRecursive); err != nil {
221221
return nil, err
222222
}
223223

‎daemon/volumes.go‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ func (m mounts) parts(i int) int {
5454
// 2. Select the volumes mounted from another containers. Overrides previously configured mount point destination.
5555
// 3. Select the bind mounts set by the client. Overrides previously configured mount point destinations.
5656
// 4. Cleanup old volumes that are about to be reassigned.
57-
func (daemon *Daemon) registerMountPoints(container *container.Container, hostConfig *containertypes.HostConfig) (retErr error) {
57+
func (daemon *Daemon) registerMountPoints(container *container.Container, hostConfig *containertypes.HostConfig, defaultReadOnlyNonRecursive bool) (retErr error) {
5858
binds := map[string]bool{}
5959
mountPoints := map[string]*volumemounts.MountPoint{}
6060
parser := volumemounts.NewParser()
@@ -158,6 +158,15 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
158158
}
159159
}
160160

161+
if bind.Type == mount.TypeBind && !bind.RW {
162+
if defaultReadOnlyNonRecursive {
163+
if bind.Spec.BindOptions == nil {
164+
bind.Spec.BindOptions = &mounttypes.BindOptions{}
165+
}
166+
bind.Spec.BindOptions.ReadOnlyNonRecursive = true
167+
}
168+
}
169+
161170
binds[bind.Destination] = true
162171
dereferenceIfExists(bind.Destination)
163172
mountPoints[bind.Destination] = bind
@@ -212,8 +221,17 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
212221
}
213222
}
214223

215-
if mp.Type == mounttypes.TypeBind && (cfg.BindOptions == nil || !cfg.BindOptions.CreateMountpoint) {
216-
mp.SkipMountpointCreation = true
224+
if mp.Type == mounttypes.TypeBind {
225+
if cfg.BindOptions == nil || !cfg.BindOptions.CreateMountpoint {
226+
mp.SkipMountpointCreation = true
227+
}
228+
229+
if !mp.RW && defaultReadOnlyNonRecursive {
230+
if mp.Spec.BindOptions == nil {
231+
mp.Spec.BindOptions = &mounttypes.BindOptions{}
232+
}
233+
mp.Spec.BindOptions.ReadOnlyNonRecursive = true
234+
}
217235
}
218236

219237
binds[mp.Destination] = true

‎integration-cli/docker_cli_inspect_test.go‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -175,15 +175,22 @@ func (s *DockerCLIInspectSuite) TestInspectContainerFilterInt(c *testing.T) {
175175
}
176176

177177
func (s *DockerCLIInspectSuite) TestInspectBindMountPoint(c *testing.T) {
178-
modifier := ",z"
179178
prefix, slash := getPrefixAndSlashFromDaemonPlatform()
179+
mopt := prefix + slash + "data:" + prefix + slash + "data"
180+
181+
mode := ""
180182
if testEnv.DaemonInfo.OSType == "windows" {
181-
modifier = ""
182183
// Linux creates the host directory if it doesn't exist. Windows does not.
183184
os.Mkdir(`c:\data`, os.ModeDir)
185+
} else {
186+
mode = "z" // Relabel
187+
}
188+
189+
if mode != "" {
190+
mopt += ":" + mode
184191
}
185192

186-
cli.DockerCmd(c, "run", "-d", "--name", "test", "-v", prefix+slash+"data:"+prefix+slash+"data:ro"+modifier, "busybox", "cat")
193+
cli.DockerCmd(c, "run", "-d", "--name", "test", "-v", mopt, "busybox", "cat")
187194

188195
vol := inspectFieldJSON(c, "test", "Mounts")
189196

@@ -200,10 +207,8 @@ func (s *DockerCLIInspectSuite) TestInspectBindMountPoint(c *testing.T) {
200207
assert.Equal(c, m.Driver, "")
201208
assert.Equal(c, m.Source, prefix+slash+"data")
202209
assert.Equal(c, m.Destination, prefix+slash+"data")
203-
if testEnv.DaemonInfo.OSType != "windows" { // Windows does not set mode
204-
assert.Equal(c, m.Mode, "ro"+modifier)
205-
}
206-
assert.Equal(c, m.RW, false)
210+
assert.Equal(c, m.Mode, mode)
211+
assert.Equal(c, m.RW, true)
207212
}
208213

209214
func (s *DockerCLIInspectSuite) TestInspectNamedMountPoint(c *testing.T) {

‎integration/container/mounts_linux_test.go‎

Lines changed: 69 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -426,6 +426,70 @@ func TestContainerCopyLeaksMounts(t *testing.T) {
426426
assert.Equal(t, mountsBefore, mountsAfter)
427427
}
428428

429+
func TestContainerBindMountReadOnlyDefault(t *testing.T) {
430+
skip.If(t, testEnv.IsRemoteDaemon)
431+
skip.If(t, !isRROSupported(), "requires recursive read-only mounts")
432+
433+
ctx := setupTest(t)
434+
435+
// The test will run a container with a simple readonly /dev bind mount (-v /dev:/dev:ro)
436+
// It will then check /proc/self/mountinfo for the mount type of /dev/shm (submount of /dev)
437+
// If /dev/shm is rw, that will mean that the read-only mounts are NOT recursive by default.
438+
const nonRecursive = " /dev/shm rw,"
439+
// If /dev/shm is ro, that will mean that the read-only mounts ARE recursive by default.
440+
const recursive = " /dev/shm ro,"
441+
442+
for _, tc := range []struct {
443+
version string
444+
expectedOut string
445+
name string
446+
}{
447+
{version: "", expectedOut: recursive, name: "latest should be the same as 1.44"},
448+
{version: "1.44", expectedOut: recursive, name: "submount should be recursive by default on 1.44"},
449+
450+
{version: "1.43", expectedOut: nonRecursive, name: "older should be non-recursive by default"},
451+
} {
452+
t.Run(tc.name, func(t *testing.T) {
453+
apiClient := testEnv.APIClient()
454+
455+
if tc.version != "" {
456+
skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), tc.version), "requires API v"+tc.version)
457+
c, err := client.NewClientWithOpts(client.FromEnv, client.WithVersion(tc.version))
458+
assert.NilError(t, err, "failed to create client with version v%s", tc.version)
459+
apiClient = c
460+
}
461+
462+
for _, tc2 := range []struct {
463+
subname string
464+
mountOpt func(*container.TestContainerConfig)
465+
}{
466+
{"mount", container.WithMount(mounttypes.Mount{
467+
Type: mounttypes.TypeBind,
468+
Source: "/dev",
469+
Target: "/dev",
470+
ReadOnly: true,
471+
})},
472+
{"bind mount", container.WithBindRaw("/dev:/dev:ro")},
473+
} {
474+
t.Run(tc2.subname, func(t *testing.T) {
475+
cid := container.Run(ctx, t, apiClient, tc2.mountOpt,
476+
container.WithCmd("sh", "-c", "grep /dev/shm /proc/self/mountinfo"),
477+
)
478+
out, err := container.Output(ctx, apiClient, cid)
479+
assert.NilError(t, err)
480+
481+
assert.Check(t, is.Equal(out.Stderr, ""))
482+
// Output should be either:
483+
// 545 526 0:160 / /dev/shm ro,nosuid,nodev,noexec,relatime shared:90 - tmpfs shm rw,size=65536k
484+
// or
485+
// 545 526 0:160 / /dev/shm rw,nosuid,nodev,noexec,relatime shared:90 - tmpfs shm rw,size=65536k
486+
assert.Check(t, is.Contains(out.Stdout, tc.expectedOut))
487+
})
488+
}
489+
})
490+
}
491+
}
492+
429493
func TestContainerBindMountRecursivelyReadOnly(t *testing.T) {
430494
skip.If(t, testEnv.IsRemoteDaemon)
431495
skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.44"), "requires API v1.44")
@@ -450,7 +514,7 @@ func TestContainerBindMountRecursivelyReadOnly(t *testing.T) {
450514
}
451515
}()
452516

453-
rroSupported := kernel.CheckKernelVersion(5, 12, 0)
517+
rroSupported := isRROSupported()
454518

455519
nonRecursiveVerifier := []string{`/bin/sh`, `-xc`, `touch /foo/mnt/file; [ $? = 0 ]`}
456520
forceRecursiveVerifier := []string{`/bin/sh`, `-xc`, `touch /foo/mnt/file; [ $? != 0 ]`}
@@ -504,3 +568,7 @@ func TestContainerBindMountRecursivelyReadOnly(t *testing.T) {
504568
poll.WaitOn(t, container.IsSuccessful(ctx, apiClient, c), poll.WithDelay(100*time.Millisecond))
505569
}
506570
}
571+
572+
func isRROSupported() bool {
573+
return kernel.CheckKernelVersion(5, 12, 0)
574+
}

0 commit comments

Comments
 (0)