From fe82fa85d2d2b7bebaab3845ea4511a120b0518c Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 25 Feb 2025 12:24:14 +0100 Subject: [PATCH] pkg/specgenutil: rework volume/mount parsing Use a helper struct to hold the mounts instead of returning 5+ return values from the functions. This allows use to easily add more volume types without having to update all return lines every time in the future. And 5+ return values are really not readable anymore so this should make it easier to follow the code. Signed-off-by: Paul Holzinger --- pkg/specgenutil/specgen.go | 10 ++--- pkg/specgenutil/volumes.go | 91 ++++++++++++++++++++++++-------------- 2 files changed, 62 insertions(+), 39 deletions(-) diff --git a/pkg/specgenutil/specgen.go b/pkg/specgenutil/specgen.go index f516a8ab14..f3ee9877cf 100644 --- a/pkg/specgenutil/specgen.go +++ b/pkg/specgenutil/specgen.go @@ -762,15 +762,15 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions // Only add read-only tmpfs mounts in case that we are read-only and the // read-only tmpfs flag has been set. - mounts, volumes, overlayVolumes, imageVolumes, err := parseVolumes(rtc, c.Volume, c.Mount, c.TmpFS) + containerMounts, err := parseVolumes(rtc, c.Volume, c.Mount, c.TmpFS) if err != nil { return err } if len(s.Mounts) == 0 || len(c.Mount) != 0 { - s.Mounts = mounts + s.Mounts = containerMounts.mounts } if len(s.Volumes) == 0 || len(c.Volume) != 0 { - s.Volumes = volumes + s.Volumes = containerMounts.volumes } if s.LabelNested != nil && *s.LabelNested { @@ -785,10 +785,10 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions } // TODO make sure these work in clone if len(s.OverlayVolumes) == 0 { - s.OverlayVolumes = overlayVolumes + s.OverlayVolumes = containerMounts.overlayVolumes } if len(s.ImageVolumes) == 0 { - s.ImageVolumes = imageVolumes + s.ImageVolumes = containerMounts.imageVolumes } devices := c.Devices diff --git a/pkg/specgenutil/volumes.go b/pkg/specgenutil/volumes.go index 2e4e6efe0c..1dea8fa2a4 100644 --- a/pkg/specgenutil/volumes.go +++ b/pkg/specgenutil/volumes.go @@ -22,6 +22,20 @@ var ( errNoDest = errors.New("must set volume destination") ) +type containerMountSlice struct { + mounts []spec.Mount + volumes []*specgen.NamedVolume + overlayVolumes []*specgen.OverlayVolume + imageVolumes []*specgen.ImageVolume +} + +// containerMountMap contains the container mounts with the destination path as map key +type containerMountMap struct { + mounts map[string]spec.Mount + volumes map[string]*specgen.NamedVolume + imageVolumes map[string]*specgen.ImageVolume +} + type universalMount struct { mount spec.Mount // Used only with Named Volume type mounts @@ -34,57 +48,57 @@ type universalMount struct { // Does not handle image volumes, init, and --volumes-from flags. // Can also add tmpfs mounts from read-only tmpfs. // TODO: handle options parsing/processing via containers/storage/pkg/mount -func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) ([]spec.Mount, []*specgen.NamedVolume, []*specgen.OverlayVolume, []*specgen.ImageVolume, error) { +func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) (*containerMountSlice, error) { // Get mounts from the --mounts flag. // TODO: The runtime config part of this needs to move into pkg/specgen/generate to avoid querying containers.conf on the client. - unifiedMounts, unifiedVolumes, unifiedImageVolumes, err := mounts(mountFlag, rtc.Mounts()) + unifiedContainerMounts, err := mounts(mountFlag, rtc.Mounts()) if err != nil { - return nil, nil, nil, nil, err + return nil, err } // Next --volumes flag. volumeMounts, volumeVolumes, overlayVolumes, err := specgen.GenVolumeMounts(volumeFlag) if err != nil { - return nil, nil, nil, nil, err + return nil, err } // Next --tmpfs flag. tmpfsMounts, err := getTmpfsMounts(tmpfsFlag) if err != nil { - return nil, nil, nil, nil, err + return nil, err } // Unify mounts from --mount, --volume, --tmpfs. // Start with --volume. for dest, mount := range volumeMounts { - if vol, ok := unifiedMounts[dest]; ok { + if vol, ok := unifiedContainerMounts.mounts[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) + return nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest) } - unifiedMounts[dest] = mount + unifiedContainerMounts.mounts[dest] = mount } for dest, volume := range volumeVolumes { - if vol, ok := unifiedVolumes[dest]; ok { + if vol, ok := unifiedContainerMounts.volumes[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) + return nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest) } - unifiedVolumes[dest] = volume + unifiedContainerMounts.volumes[dest] = volume } // Now --tmpfs for dest, tmpfs := range tmpfsMounts { - if vol, ok := unifiedMounts[dest]; ok { + if vol, ok := unifiedContainerMounts.mounts[dest]; ok { if vol.Type != define.TypeTmpfs { - return nil, nil, nil, nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest) + return nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest) } continue } - unifiedMounts[dest] = tmpfs + unifiedContainerMounts.mounts[dest] = tmpfs } // Check for conflicts between named volumes, overlay & image volumes, @@ -97,53 +111,58 @@ func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) allMounts[dest] = true return nil } - for dest := range unifiedMounts { + for dest := range unifiedContainerMounts.mounts { if err := testAndSet(dest); err != nil { - return nil, nil, nil, nil, err + return nil, err } } - for dest := range unifiedVolumes { + for dest := range unifiedContainerMounts.volumes { if err := testAndSet(dest); err != nil { - return nil, nil, nil, nil, err + return nil, err } } for dest := range overlayVolumes { if err := testAndSet(dest); err != nil { - return nil, nil, nil, nil, err + return nil, err } } - for dest := range unifiedImageVolumes { + for dest := range unifiedContainerMounts.imageVolumes { if err := testAndSet(dest); err != nil { - return nil, nil, nil, nil, err + return nil, err } } // Final step: maps to arrays - finalMounts := make([]spec.Mount, 0, len(unifiedMounts)) - for _, mount := range unifiedMounts { + finalMounts := make([]spec.Mount, 0, len(unifiedContainerMounts.mounts)) + for _, mount := range unifiedContainerMounts.mounts { if mount.Type == define.TypeBind { absSrc, err := specgen.ConvertWinMountPath(mount.Source) if err != nil { - return nil, nil, nil, nil, fmt.Errorf("getting absolute path of %s: %w", mount.Source, err) + return nil, fmt.Errorf("getting absolute path of %s: %w", mount.Source, err) } mount.Source = absSrc } finalMounts = append(finalMounts, mount) } - finalVolumes := make([]*specgen.NamedVolume, 0, len(unifiedVolumes)) - for _, volume := range unifiedVolumes { + finalVolumes := make([]*specgen.NamedVolume, 0, len(unifiedContainerMounts.volumes)) + for _, volume := range unifiedContainerMounts.volumes { finalVolumes = append(finalVolumes, volume) } - finalOverlayVolume := make([]*specgen.OverlayVolume, 0) + finalOverlayVolume := make([]*specgen.OverlayVolume, 0, len(overlayVolumes)) for _, volume := range overlayVolumes { finalOverlayVolume = append(finalOverlayVolume, volume) } - finalImageVolumes := make([]*specgen.ImageVolume, 0, len(unifiedImageVolumes)) - for _, volume := range unifiedImageVolumes { + finalImageVolumes := make([]*specgen.ImageVolume, 0, len(unifiedContainerMounts.imageVolumes)) + for _, volume := range unifiedContainerMounts.imageVolumes { finalImageVolumes = append(finalImageVolumes, volume) } - return finalMounts, finalVolumes, finalOverlayVolume, finalImageVolumes, nil + return &containerMountSlice{ + mounts: finalMounts, + volumes: finalVolumes, + overlayVolumes: finalOverlayVolume, + imageVolumes: finalImageVolumes, + }, nil } // mounts takes user-provided input from the --mount flag as well as mounts @@ -151,7 +170,7 @@ func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) // podman run --mount type=bind,src=/etc/resolv.conf,target=/etc/resolv.conf ... // podman run --mount type=tmpfs,target=/dev/shm ... // podman run --mount type=volume,source=test-volume, ... -func mounts(mountFlag []string, configMounts []string) (map[string]spec.Mount, map[string]*specgen.NamedVolume, map[string]*specgen.ImageVolume, error) { +func mounts(mountFlag []string, configMounts []string) (*containerMountMap, error) { finalMounts := make(map[string]spec.Mount) finalNamedVolumes := make(map[string]*specgen.NamedVolume) finalImageVolumes := make(map[string]*specgen.ImageVolume) @@ -246,17 +265,21 @@ func mounts(mountFlag []string, configMounts []string) (map[string]spec.Mount, m // Parse mounts passed in from the user if err := parseMounts(mountFlag, false); err != nil { - return nil, nil, nil, err + return nil, err } // If user specified a mount flag that conflicts with a containers.conf flag, then ignore // the duplicate. This means that the parsing of the containers.conf configMounts should always // happen second. if err := parseMounts(configMounts, true); err != nil { - return nil, nil, nil, fmt.Errorf("parsing containers.conf mounts: %w", err) + return nil, fmt.Errorf("parsing containers.conf mounts: %w", err) } - return finalMounts, finalNamedVolumes, finalImageVolumes, nil + return &containerMountMap{ + mounts: finalMounts, + volumes: finalNamedVolumes, + imageVolumes: finalImageVolumes, + }, nil } func parseMountOptions(mountType string, args []string) (*universalMount, error) {