Skip to content

Commit 4323903

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 4323903

8 files changed

Lines changed: 145 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: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"testing"
99
"time"
1010

11+
"github.com/docker/docker/api"
1112
containertypes "github.com/docker/docker/api/types/container"
1213
mounttypes "github.com/docker/docker/api/types/mount"
1314
"github.com/docker/docker/api/types/network"
@@ -426,6 +427,78 @@ func TestContainerCopyLeaksMounts(t *testing.T) {
426427
assert.Equal(t, mountsBefore, mountsAfter)
427428
}
428429

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

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

455528
nonRecursiveVerifier := []string{`/bin/sh`, `-xc`, `touch /foo/mnt/file; [ $? = 0 ]`}
456529
forceRecursiveVerifier := []string{`/bin/sh`, `-xc`, `touch /foo/mnt/file; [ $? != 0 ]`}
@@ -504,3 +577,7 @@ func TestContainerBindMountRecursivelyReadOnly(t *testing.T) {
504577
poll.WaitOn(t, container.IsSuccessful(ctx, apiClient, c), poll.WithDelay(100*time.Millisecond))
505578
}
506579
}
580+
581+
func isRROSupported() bool {
582+
return kernel.CheckKernelVersion(5, 12, 0)
583+
}

0 commit comments

Comments
 (0)