Skip to content

Commit

Permalink
Allow volume mount dups, iff source and dest dirs
Browse files Browse the repository at this point in the history 
Also create one constant for ErrDuplicateDest, rather then have the same
value set three times.

Fixes: containers#4217

Signed-off-by: Daniel J Walsh <[email protected]>
  • Loading branch information
@rhatdan
rhatdan committed Oct 15, 2022
1 parent d21a356 commit 75222ad
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 32 deletions.
14 changes: 6 additions & 8 deletions pkg/specgen/generate/storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ import (
"github.com/sirupsen/logrus"
)

var errDuplicateDest = errors.New("duplicate mount destination")

// Produce final mounts and named volumes for a container
func finalizeMounts(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Runtime, rtc *config.Config, img *libimage.Image) ([]spec.Mount, []*specgen.NamedVolume, []*specgen.OverlayVolume, error) {
// Get image volumes
Expand Down Expand Up @@ -63,7 +61,7 @@ func finalizeMounts(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Ru
}
cleanDestination := filepath.Clean(m.Destination)
if _, ok := unifiedMounts[cleanDestination]; ok {
return nil, nil, nil, fmt.Errorf("conflict in specified mounts - multiple mounts at %q: %w", cleanDestination, errDuplicateDest)
return nil, nil, nil, fmt.Errorf("%q: %w", cleanDestination, specgen.ErrDuplicateDest)
}
unifiedMounts[cleanDestination] = m
}
Expand All @@ -84,7 +82,7 @@ func finalizeMounts(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Ru
}
cleanDestination := filepath.Clean(v.Dest)
if _, ok := unifiedVolumes[cleanDestination]; ok {
return nil, nil, nil, fmt.Errorf("conflict in specified volumes - multiple volumes at %q: %w", cleanDestination, errDuplicateDest)
return nil, nil, nil, fmt.Errorf("conflict in specified volumes - multiple volumes at %q: %w", cleanDestination, specgen.ErrDuplicateDest)
}
unifiedVolumes[cleanDestination] = v
}
Expand All @@ -105,7 +103,7 @@ func finalizeMounts(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Ru
}
cleanDestination := filepath.Clean(v.Destination)
if _, ok := unifiedOverlays[cleanDestination]; ok {
return nil, nil, nil, fmt.Errorf("conflict in specified volumes - multiple volumes at %q: %w", cleanDestination, errDuplicateDest)
return nil, nil, nil, fmt.Errorf("conflict in specified volumes - multiple volumes at %q: %w", cleanDestination, specgen.ErrDuplicateDest)
}
unifiedOverlays[cleanDestination] = v
}
Expand All @@ -131,7 +129,7 @@ func finalizeMounts(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Ru
return nil, nil, nil, err
}
if _, ok := unifiedMounts[initMount.Destination]; ok {
return nil, nil, nil, fmt.Errorf("conflict with mount added by --init to %q: %w", initMount.Destination, errDuplicateDest)
return nil, nil, nil, fmt.Errorf("conflict with mount added by --init to %q: %w", initMount.Destination, specgen.ErrDuplicateDest)
}
unifiedMounts[initMount.Destination] = initMount
}
Expand Down Expand Up @@ -161,12 +159,12 @@ func finalizeMounts(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Ru
// Check for conflicts between named volumes and mounts
for dest := range baseMounts {
if _, ok := baseVolumes[dest]; ok {
return nil, nil, nil, fmt.Errorf("conflict at mount destination %v: %w", dest, errDuplicateDest)
return nil, nil, nil, fmt.Errorf("conflict at mount destination %v: %w", dest, specgen.ErrDuplicateDest)
}
}
for dest := range baseVolumes {
if _, ok := baseMounts[dest]; ok {
return nil, nil, nil, fmt.Errorf("conflict at mount destination %v: %w", dest, errDuplicateDest)
return nil, nil, nil, fmt.Errorf("conflict at mount destination %v: %w", dest, specgen.ErrDuplicateDest)
}
}
// Final step: maps to arrays
Expand Down
14 changes: 14 additions & 0 deletions pkg/specgen/specgen.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,8 @@ var (
// ErrNoStaticMACRootless is used when a rootless user requests to assign a static MAC address
// to a pod or container
ErrNoStaticMACRootless = errors.New("rootless containers and pods cannot be assigned static MAC addresses")
// Multiple volume mounts to the same destination is not allowed
ErrDuplicateDest = errors.New("duplicate mount destination")
)

// NewSpecGenerator returns a SpecGenerator struct given one of two mandatory inputs
Expand All @@ -607,3 +609,15 @@ func NewSpecGeneratorWithRootfs(rootfs string) *SpecGenerator {
csc := ContainerStorageConfig{Rootfs: rootfs}
return &SpecGenerator{ContainerStorageConfig: csc}
}

func StringSlicesEqual(a, b []string) bool {
if len(a) != len(b) {
return false
}
for i, v := range a {
if v != b[i] {
return false
}
}
return true
}
26 changes: 18 additions & 8 deletions pkg/specgen/volumes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ type ImageVolume struct {

// GenVolumeMounts parses user input into mounts, volumes and overlay volumes
func GenVolumeMounts(volumeFlag []string) (map[string]spec.Mount, map[string]*NamedVolume, map[string]*OverlayVolume, error) {
errDuplicateDest := errors.New("duplicate mount destination")

mounts := make(map[string]spec.Mount)
volumes := make(map[string]*NamedVolume)
overlayVolumes := make(map[string]*OverlayVolume)
Expand Down Expand Up @@ -153,8 +151,12 @@ func GenVolumeMounts(volumeFlag []string) (map[string]spec.Mount, map[string]*Na
newOverlayVol.Source = source
newOverlayVol.Options = options

if _, ok := overlayVolumes[newOverlayVol.Destination]; ok {
return nil, nil, nil, fmt.Errorf("%v: %w", newOverlayVol.Destination, errDuplicateDest)
if vol, ok := overlayVolumes[newOverlayVol.Destination]; ok {
if vol.Source == newOverlayVol.Source &&
StringSlicesEqual(vol.Options, newOverlayVol.Options) {
continue
}
return nil, nil, nil, fmt.Errorf("%v: %w", newOverlayVol.Destination, ErrDuplicateDest)
}
overlayVolumes[newOverlayVol.Destination] = newOverlayVol
} else {
Expand All @@ -164,8 +166,13 @@ func GenVolumeMounts(volumeFlag []string) (map[string]spec.Mount, map[string]*Na
Source: src,
Options: options,
}
if _, ok := mounts[newMount.Destination]; ok {
return nil, nil, nil, fmt.Errorf("%v: %w", newMount.Destination, errDuplicateDest)
if vol, ok := mounts[newMount.Destination]; ok {
if vol.Source == newMount.Source &&
StringSlicesEqual(vol.Options, newMount.Options) {
continue
}

return nil, nil, nil, fmt.Errorf("%v: %w", newMount.Destination, ErrDuplicateDest)
}
mounts[newMount.Destination] = newMount
}
Expand All @@ -176,8 +183,11 @@ func GenVolumeMounts(volumeFlag []string) (map[string]spec.Mount, map[string]*Na
newNamedVol.Dest = dest
newNamedVol.Options = options

if _, ok := volumes[newNamedVol.Dest]; ok {
return nil, nil, nil, fmt.Errorf("%v: %w", newNamedVol.Dest, errDuplicateDest)
if vol, ok := volumes[newNamedVol.Dest]; ok {
if vol.Name == newNamedVol.Name {
continue
}
return nil, nil, nil, fmt.Errorf("%v: %w", newNamedVol.Dest, ErrDuplicateDest)
}
volumes[newNamedVol.Dest] = newNamedVol
}
Expand Down
44 changes: 28 additions & 16 deletions pkg/specgenutil/volumes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
)

var (
errDuplicateDest = errors.New("duplicate mount destination")
errOptionArg = errors.New("must provide an argument for option")
errNoDest = errors.New("must set volume destination")
errInvalidSyntax = errors.New("incorrect mount format: should be --mount type=<bind|tmpfs|volume>,[src=<host-dir|volume-name>,]target=<ctr-dir>[,options]")
Expand Down Expand Up @@ -49,21 +48,32 @@ func parseVolumes(volumeFlag, mountFlag, tmpfsFlag []string, addReadOnlyTmpfs bo
// Unify mounts from --mount, --volume, --tmpfs.
// Start with --volume.
for dest, mount := range volumeMounts {
if _, ok := unifiedMounts[dest]; ok {
return nil, nil, nil, nil, fmt.Errorf("%v: %w", dest, errDuplicateDest)
if vol, ok := unifiedMounts[dest]; ok {
if mount.Source == vol.Source &&
specgen.StringSlicesEqual(vol.Options, mount.Options) {
continue
}
return nil, nil, nil, nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest)
}
unifiedMounts[dest] = mount
}
for dest, volume := range volumeVolumes {
if _, ok := unifiedVolumes[dest]; ok {
return nil, nil, nil, nil, fmt.Errorf("%v: %w", dest, errDuplicateDest)
if vol, ok := unifiedVolumes[dest]; ok {
if volume.Name == vol.Name &&
specgen.StringSlicesEqual(vol.Options, volume.Options) {
continue
}
return nil, nil, nil, nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest)
}
unifiedVolumes[dest] = volume
}
// Now --tmpfs
for dest, tmpfs := range tmpfsMounts {
if _, ok := unifiedMounts[dest]; ok {
return nil, nil, nil, nil, fmt.Errorf("%v: %w", dest, errDuplicateDest)
if vol, ok := unifiedMounts[dest]; ok {
if vol.Type != define.TypeTmpfs {
return nil, nil, nil, nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest)
}
continue
}
unifiedMounts[dest] = tmpfs
}
Expand Down Expand Up @@ -93,7 +103,7 @@ func parseVolumes(volumeFlag, mountFlag, tmpfsFlag []string, addReadOnlyTmpfs bo
allMounts := make(map[string]bool)
testAndSet := func(dest string) error {
if _, ok := allMounts[dest]; ok {
return fmt.Errorf("conflict at mount destination %v: %w", dest, errDuplicateDest)
return fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest)
}
allMounts[dest] = true
return nil
Expand Down Expand Up @@ -199,7 +209,7 @@ func Mounts(mountFlag []string) (map[string]spec.Mount, map[string]*specgen.Name
return nil, nil, nil, err
}
if _, ok := finalMounts[mount.Destination]; ok {
return nil, nil, nil, fmt.Errorf("%v: %w", mount.Destination, errDuplicateDest)
return nil, nil, nil, fmt.Errorf("%v: %w", mount.Destination, specgen.ErrDuplicateDest)
}
finalMounts[mount.Destination] = mount
case define.TypeTmpfs:
Expand All @@ -208,7 +218,7 @@ func Mounts(mountFlag []string) (map[string]spec.Mount, map[string]*specgen.Name
return nil, nil, nil, err
}
if _, ok := finalMounts[mount.Destination]; ok {
return nil, nil, nil, fmt.Errorf("%v: %w", mount.Destination, errDuplicateDest)
return nil, nil, nil, fmt.Errorf("%v: %w", mount.Destination, specgen.ErrDuplicateDest)
}
finalMounts[mount.Destination] = mount
case define.TypeDevpts:
Expand All @@ -217,7 +227,7 @@ func Mounts(mountFlag []string) (map[string]spec.Mount, map[string]*specgen.Name
return nil, nil, nil, err
}
if _, ok := finalMounts[mount.Destination]; ok {
return nil, nil, nil, fmt.Errorf("%v: %w", mount.Destination, errDuplicateDest)
return nil, nil, nil, fmt.Errorf("%v: %w", mount.Destination, specgen.ErrDuplicateDest)
}
finalMounts[mount.Destination] = mount
case "image":
Expand All @@ -226,7 +236,7 @@ func Mounts(mountFlag []string) (map[string]spec.Mount, map[string]*specgen.Name
return nil, nil, nil, err
}
if _, ok := finalImageVolumes[volume.Destination]; ok {
return nil, nil, nil, fmt.Errorf("%v: %w", volume.Destination, errDuplicateDest)
return nil, nil, nil, fmt.Errorf("%v: %w", volume.Destination, specgen.ErrDuplicateDest)
}
finalImageVolumes[volume.Destination] = volume
case "volume":
Expand All @@ -235,7 +245,7 @@ func Mounts(mountFlag []string) (map[string]spec.Mount, map[string]*specgen.Name
return nil, nil, nil, err
}
if _, ok := finalNamedVolumes[volume.Dest]; ok {
return nil, nil, nil, fmt.Errorf("%v: %w", volume.Dest, errDuplicateDest)
return nil, nil, nil, fmt.Errorf("%v: %w", volume.Dest, specgen.ErrDuplicateDest)
}
finalNamedVolumes[volume.Dest] = volume
default:
Expand Down Expand Up @@ -665,10 +675,12 @@ func getTmpfsMounts(tmpfsFlag []string) (map[string]spec.Mount, error) {
options = strings.Split(spliti[1], ",")
}

if _, ok := m[destPath]; ok {
return nil, fmt.Errorf("%v: %w", destPath, errDuplicateDest)
if vol, ok := m[destPath]; ok {
if specgen.StringSlicesEqual(vol.Options, options) {
continue
}
return nil, fmt.Errorf("%v: %w", destPath, specgen.ErrDuplicateDest)
}

mount := spec.Mount{
Destination: unixPathClean(destPath),
Type: define.TypeTmpfs,
Expand Down
27 changes: 27 additions & 0 deletions test/system/160-volumes.bats
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,33 @@ function teardown() {
}


@test "podman volume duplicates" {
vol1=${PODMAN_TMPDIR}/v1_$(random_string)
vol2=${PODMAN_TMPDIR}/v2_$(random_string)
mkdir $vol1 $vol2

# if volumes source and dest match then pass
run_podman run --rm -v $vol1:/vol1 -v $vol1:/vol1 $IMAGE /bin/true
run_podman 125 run --rm -v $vol1:$vol1 -v $vol2:$vol1 $IMAGE /bin/true
is "$output" "Error: $vol1: duplicate mount destination" "diff volumes mounted on same dest should fail"

# if named volumes source and dest match then pass
run_podman run --rm -v vol1:/vol1 -v vol1:/vol1 $IMAGE /bin/true
run_podman 125 run --rm -v vol1:/vol1 -v vol2:/vol1 $IMAGE /bin/true
is "$output" "Error: /vol1: duplicate mount destination" "diff named volumes mounted on same dest should fail"

# if tmpfs volumes source and dest match then pass
run_podman run --rm --tmpfs /vol1 --tmpfs /vol1 $IMAGE /bin/true
run_podman 125 run --rm --tmpfs $vol1 --tmpfs $vol1:ro $IMAGE /bin/true
is "$output" "Error: $vol1: duplicate mount destination" "diff named volumes and tmpfs mounted on same dest should fail"

run_podman 125 run --rm -v vol2:/vol2 --tmpfs /vol2 $IMAGE /bin/true
is "$output" "Error: /vol2: duplicate mount destination" "diff named volumes and tmpfs mounted on same dest should fail"

run_podman 125 run --rm -v $vol1:/vol1 --tmpfs /vol1 $IMAGE /bin/true
is "$output" "Error: /vol1: duplicate mount destination" "diff named volumes and tmpfs mounted on same dest should fail"
}

# Filter volumes by name
@test "podman volume filter --name" {
suffix=$(random_string)
Expand Down

0 comments on commit 75222ad

Please sign in to comment.