Skip to content

Commit f40db66

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 cba8712 commit f40db66

8 files changed

Lines changed: 167 additions & 28 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: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"github.com/docker/docker/api/types/backend"
1313
containertypes "github.com/docker/docker/api/types/container"
1414
"github.com/docker/docker/api/types/events"
15+
"github.com/docker/docker/api/types/mount"
1516
networktypes "github.com/docker/docker/api/types/network"
1617
"github.com/docker/docker/container"
1718
"github.com/docker/docker/daemon/config"
@@ -21,6 +22,7 @@ import (
2122
"github.com/docker/docker/internal/multierror"
2223
"github.com/docker/docker/pkg/idtools"
2324
"github.com/docker/docker/runconfig"
25+
volumemounts "github.com/docker/docker/volume/mounts"
2426
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
2527
"github.com/opencontainers/selinux/go-selinux"
2628
archvariant "github.com/tonistiigi/go-archvariant"
@@ -126,6 +128,41 @@ func (daemon *Daemon) containerCreate(ctx context.Context, daemonCfg *configStor
126128
return containertypes.CreateResponse{ID: ctr.ID, Warnings: warnings}, nil
127129
}
128130

131+
// transformBinds transforms read-only Binds into Mounts and enables
132+
// ReadOnlyNonRecursive by default.
133+
func transformBinds(hostConfig *containertypes.HostConfig) error {
134+
var newBinds []string
135+
parser := volumemounts.NewParser()
136+
for _, b := range hostConfig.Binds {
137+
if bind, err := parser.ParseMountRaw(b, hostConfig.VolumeDriver); err == nil {
138+
relabel := false
139+
for _, m := range strings.Split(bind.Mode, ",") {
140+
if m == "z" || m == "Z" {
141+
relabel = true
142+
break
143+
}
144+
}
145+
146+
// There's no equivalent of z and Z mode in the long --mount option.
147+
if !bind.RW && !relabel {
148+
mnt := bind.Spec
149+
if mnt.BindOptions == nil {
150+
mnt.BindOptions = &mount.BindOptions{}
151+
}
152+
// This is default for the short bind syntax (-v src:dst).
153+
mnt.BindOptions.CreateMountpoint = true
154+
155+
hostConfig.Mounts = append(hostConfig.Mounts, mnt)
156+
continue
157+
}
158+
}
159+
newBinds = append(newBinds, b)
160+
}
161+
hostConfig.Binds = newBinds
162+
163+
return nil
164+
}
165+
129166
// Create creates a new container from the given configuration with a given name.
130167
func (daemon *Daemon) create(ctx context.Context, daemonCfg *config.Config, opts createOpts) (retC *container.Container, retErr error) {
131168
var (
@@ -217,7 +254,7 @@ func (daemon *Daemon) create(ctx context.Context, daemonCfg *config.Config, opts
217254
return nil, err
218255
}
219256

220-
if err := daemon.setHostConfig(ctr, opts.params.HostConfig); err != nil {
257+
if err := daemon.setHostConfig(ctr, opts.params.HostConfig, opts.params.DefaultReadOnlyNonRecursive); err != nil {
221258
return nil, err
222259
}
223260

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

0 commit comments

Comments
 (0)