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 
Fixes: containers#4217

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
  • Loading branch information
@rhatdan
rhatdan committed Oct 13, 2022
1 parent 24b586e commit af9abfb
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 32 deletions.
14 changes: 6 additions & 8 deletions pkg/specgen/generate/storage.go
View file
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
2 changes: 2 additions & 0 deletions pkg/specgen/specgen.go
View file
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 Down
24 changes: 16 additions & 8 deletions pkg/specgen/volumes.go
View file
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,11 @@ 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 {
continue
}
return nil, nil, nil, fmt.Errorf("%v: %w", newOverlayVol.Destination, ErrDuplicateDest)
}
overlayVolumes[newOverlayVol.Destination] = newOverlayVol
} else {
Expand All @@ -164,8 +165,12 @@ 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 {
continue
}

return nil, nil, nil, fmt.Errorf("%v: %w", newMount.Destination, ErrDuplicateDest)
}
mounts[newMount.Destination] = newMount
}
Expand All @@ -176,8 +181,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
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 {
fmt.Println("DAN1", vol)
if mount.Source == vol.Source {
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 {
fmt.Println("DAN2", vol)
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 strings.Join(vol.Options, ",") == strings.Join(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
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 af9abfb

Please sign in to comment.