From 7495fbc0e3f23c932562cb02ea6e7df204d29dfa Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Thu, 21 Aug 2014 17:57:46 -0400 Subject: [PATCH] Cleanup: applyVolumesFrom Docker-DCO-1.1-Signed-off-by: Brian Goff (github: cpuguy83) --- daemon/container.go | 44 +++++++ daemon/volumes.go | 159 ++++++++++--------------- integration-cli/docker_cli_run_test.go | 19 ++- 3 files changed, 123 insertions(+), 99 deletions(-) diff --git a/daemon/container.go b/daemon/container.go index df6bd66190..9ecb6556b9 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -1145,3 +1145,47 @@ func (container *Container) getNetworkedContainer() (*Container, error) { return nil, fmt.Errorf("network mode not set to container") } } + +func (container *Container) GetVolumes() (map[string]*Volume, error) { + // Get all the bind-mounts + volumes, err := container.getBindMap() + if err != nil { + return nil, err + } + + // Get all the normal volumes + for volPath, hostPath := range container.Volumes { + if _, exists := volumes[volPath]; exists { + continue + } + volumes[volPath] = &Volume{VolPath: volPath, HostPath: hostPath, isReadWrite: container.VolumesRW[volPath]} + } + + return volumes, nil +} + +func (container *Container) getBindMap() (map[string]*Volume, error) { + var ( + // Create the requested bind mounts + volumes = map[string]*Volume{} + // Define illegal container destinations + illegalDsts = []string{"/", "."} + ) + + for _, bind := range container.hostConfig.Binds { + vol, err := parseBindVolumeSpec(bind) + if err != nil { + return nil, err + } + vol.isBindMount = true + // Bail if trying to mount to an illegal destination + for _, illegal := range illegalDsts { + if vol.VolPath == illegal { + return nil, fmt.Errorf("Illegal bind destination: %s", vol.VolPath) + } + } + + volumes[filepath.Clean(vol.VolPath)] = &vol + } + return volumes, nil +} diff --git a/daemon/volumes.go b/daemon/volumes.go index b60118c953..85836f1be6 100644 --- a/daemon/volumes.go +++ b/daemon/volumes.go @@ -16,14 +16,10 @@ import ( type Volume struct { HostPath string VolPath string - Mode string + isReadWrite bool isBindMount bool } -func (v *Volume) isRw() bool { - return v.Mode == "" || strings.ToLower(v.Mode) == "rw" -} - func (v *Volume) isDir() (bool, error) { stat, err := os.Stat(v.HostPath) if err != nil { @@ -42,10 +38,7 @@ func prepareVolumesForContainer(container *Container) error { } } - if err := createVolumes(container); err != nil { - return err - } - return nil + return createVolumes(container) } func setupMountsForContainer(container *Container) error { @@ -74,87 +67,82 @@ func setupMountsForContainer(container *Container) error { return nil } +func parseVolumesFromSpec(container *Container, spec string) (map[string]*Volume, error) { + specParts := strings.SplitN(spec, ":", 2) + if len(specParts) == 0 { + return nil, fmt.Errorf("Malformed volumes-from specification: %s", spec) + } + + c := container.daemon.Get(specParts[0]) + if c == nil { + return nil, fmt.Errorf("Container %s not found. Impossible to mount its volumes", specParts[0]) + } + + volumes, err := c.GetVolumes() + if err != nil { + return nil, err + } + + if len(specParts) == 2 { + mode := specParts[1] + if validVolumeMode(mode) { + return nil, fmt.Errorf("Invalid mode for volumes-from: %s", mode) + } + + // Set the mode for the inheritted volume + for _, v := range volumes { + v.isReadWrite = mode != "ro" + } + } + + return volumes, nil +} + func applyVolumesFrom(container *Container) error { volumesFrom := container.hostConfig.VolumesFrom - if len(volumesFrom) > 0 { - for _, containerSpec := range volumesFrom { - var ( - mountRW = true - specParts = strings.SplitN(containerSpec, ":", 2) - ) - switch len(specParts) { - case 0: - return fmt.Errorf("Malformed volumes-from specification: %s", containerSpec) - case 2: - switch specParts[1] { - case "ro": - mountRW = false - case "rw": // mountRW is already true - default: - return fmt.Errorf("Malformed volumes-from specification: %s", containerSpec) - } + for _, spec := range volumesFrom { + volumes, err := parseVolumesFromSpec(container, spec) + if err != nil { + return err + } + + for _, v := range volumes { + if err = v.initialize(container); err != nil { + return err } - - c := container.daemon.Get(specParts[0]) - if c == nil { - return fmt.Errorf("Container %s not found. Impossible to mount its volumes", specParts[0]) - } - - if err := c.Mount(); err != nil { - return fmt.Errorf("Container %s failed to mount. Impossible to mount its volumes", specParts[0]) - } - defer c.Unmount() - - for volPath, id := range c.Volumes { - if _, exists := container.Volumes[volPath]; exists { - continue - } - - pth, err := c.getResourcePath(volPath) - if err != nil { - return err - } - - stat, err := os.Stat(pth) - if err != nil { - return err - } - - if err := createIfNotExists(pth, stat.IsDir()); err != nil { - return err - } - - container.Volumes[volPath] = id - if isRW, exists := c.VolumesRW[volPath]; exists { - container.VolumesRW[volPath] = isRW && mountRW - } - } - } } return nil } +func validVolumeMode(mode string) bool { + validModes := map[string]bool{ + "rw": true, + "ro": true, + } + + return validModes[mode] +} + func parseBindVolumeSpec(spec string) (Volume, error) { var ( arr = strings.Split(spec, ":") vol Volume ) - vol.isBindMount = true switch len(arr) { case 1: vol.VolPath = spec - vol.Mode = "rw" + vol.isReadWrite = true case 2: vol.HostPath = arr[0] vol.VolPath = arr[1] - vol.Mode = "rw" + vol.isReadWrite = true case 3: vol.HostPath = arr[0] vol.VolPath = arr[1] - vol.Mode = arr[2] + vol.isReadWrite = validVolumeMode(arr[2]) && arr[2] == "rw" default: return vol, fmt.Errorf("Invalid volume specification: %s", spec) } @@ -166,34 +154,9 @@ func parseBindVolumeSpec(spec string) (Volume, error) { return vol, nil } -func getBindMap(container *Container) (map[string]Volume, error) { - var ( - // Create the requested bind mounts - volumes = map[string]Volume{} - // Define illegal container destinations - illegalDsts = []string{"/", "."} - ) - - for _, bind := range container.hostConfig.Binds { - vol, err := parseBindVolumeSpec(bind) - if err != nil { - return volumes, err - } - // Bail if trying to mount to an illegal destination - for _, illegal := range illegalDsts { - if vol.VolPath == illegal { - return nil, fmt.Errorf("Illegal bind destination: %s", vol.VolPath) - } - } - - volumes[filepath.Clean(vol.VolPath)] = vol - } - return volumes, nil -} - func createVolumes(container *Container) error { // Get all the bindmounts - volumes, err := getBindMap(container) + volumes, err := container.GetVolumes() if err != nil { return err } @@ -202,9 +165,9 @@ func createVolumes(container *Container) error { for volPath := range container.Config.Volumes { // Make sure the the volume isn't already specified as a bindmount if _, exists := volumes[volPath]; !exists { - volumes[volPath] = Volume{ + volumes[volPath] = &Volume{ VolPath: volPath, - Mode: "rw", + isReadWrite: true, isBindMount: false, } } @@ -215,8 +178,8 @@ func createVolumes(container *Container) error { return err } } - return nil + return nil } func createVolumeHostPath(container *Container) (string, error) { @@ -247,7 +210,7 @@ func (v *Volume) initialize(container *Container) error { } // If it's not a bindmount we need to create the dir on the host - if !v.isBindMount { + if !v.isBindMount && v.HostPath == "" { v.HostPath, err = createVolumeHostPath(container) if err != nil { return err @@ -269,7 +232,7 @@ func (v *Volume) initialize(container *Container) error { } container.Volumes[v.VolPath] = hostPath - container.VolumesRW[v.VolPath] = v.isRw() + container.VolumesRW[v.VolPath] = v.isReadWrite volIsDir, err := v.isDir() if err != nil { @@ -280,7 +243,7 @@ func (v *Volume) initialize(container *Container) error { } // Do not copy or change permissions if we are mounting from the host - if v.isRw() && !v.isBindMount { + if v.isReadWrite && !v.isBindMount { return copyExistingContents(fullVolPath, hostPath) } return nil diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index b95fd86d2b..22d47b1b99 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -374,6 +374,23 @@ func TestVolumesFromInReadWriteMode(t *testing.T) { logDone("run - volumes from as read write mount") } +func TestVolumesFromInheritsReadOnly(t *testing.T) { + cmd := exec.Command(dockerBinary, "run", "--name", "parent", "-v", "/test:/test:ro", "busybox", "true") + if _, err := runCommand(cmd); err != nil { + t.Fatal(err) + } + + // Expect this "rw" mode to be be ignored since the inheritted volume is "ro" + cmd = exec.Command(dockerBinary, "run", "--volumes-from", "parent:rw", "busybox", "touch", "/test/file") + if _, err := runCommand(cmd); err == nil { + t.Fatal("Expected to inherit read-only volume even when passing in `rw`") + } + + deleteAllContainers() + + logDone("run - volumes from ignores `rw` if inherrited volume is `ro`") +} + // Test for #1351 func TestApplyVolumesFromBeforeVolumes(t *testing.T) { cmd := exec.Command(dockerBinary, "run", "--name", "parent", "-v", "/test", "busybox", "touch", "/test/foo") @@ -1181,7 +1198,7 @@ func TestDockerRunWithVolumesIsRecursive(t *testing.T) { deleteAllContainers() - logDone("run - volumes are bind mounted recuursively") + logDone("run - volumes are bind mounted recursively") } func TestDnsDefaultOptions(t *testing.T) {