From 8a28b896bbfa91f9de290dfaa42226205f2d0cfd Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Thu, 13 Oct 2022 13:57:49 -0400 Subject: [PATCH] Allow volume mount dups, iff source and dest dirs Also create one constant for ErrDuplicateDest, rather then have the same value set three times. Fixes: https://github.com/containers/podman/issues/4217 Signed-off-by: Daniel J Walsh --- pkg/specgen/generate/storage.go | 14 +++++------ pkg/specgen/specgen.go | 14 +++++++++++ pkg/specgen/volumes.go | 26 +++++++++++++------ pkg/specgenutil/volumes.go | 44 +++++++++++++++++++++------------ test/system/160-volumes.bats | 27 ++++++++++++++++++++ 5 files changed, 93 insertions(+), 32 deletions(-) diff --git a/pkg/specgen/generate/storage.go b/pkg/specgen/generate/storage.go index c3cd61b36f6e..b6246d824dab 100644 --- a/pkg/specgen/generate/storage.go +++ b/pkg/specgen/generate/storage.go @@ -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 @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 diff --git a/pkg/specgen/specgen.go b/pkg/specgen/specgen.go index 34418c132631..8c77df20f851 100644 --- a/pkg/specgen/specgen.go +++ b/pkg/specgen/specgen.go @@ -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 @@ -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 +} diff --git a/pkg/specgen/volumes.go b/pkg/specgen/volumes.go index e71d1433100c..0d886df53006 100644 --- a/pkg/specgen/volumes.go +++ b/pkg/specgen/volumes.go @@ -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) @@ -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 { @@ -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 } @@ -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 } diff --git a/pkg/specgenutil/volumes.go b/pkg/specgenutil/volumes.go index e6c5d8b9766a..fe2216d80ecc 100644 --- a/pkg/specgenutil/volumes.go +++ b/pkg/specgenutil/volumes.go @@ -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=,[src=,]target=[,options]") @@ -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 } @@ -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 @@ -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: @@ -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: @@ -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": @@ -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": @@ -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: @@ -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, diff --git a/test/system/160-volumes.bats b/test/system/160-volumes.bats index 08baaf468c59..13525e725f73 100644 --- a/test/system/160-volumes.bats +++ b/test/system/160-volumes.bats @@ -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)