From e973b09a8e5d63a55890ae44e3978162947d64a0 Mon Sep 17 00:00:00 2001 From: SamirPS Date: Fri, 23 Dec 2022 19:39:07 +0100 Subject: [PATCH] Fix: List container with volume filter Modify the condition in line 149 in order to list container by mounting point. Closes #16019 Signed-off-by: SamirPS --- DOWNLOADS.md | 2 +- cmd/podman/common/create.go | 6 ++-- cmd/podman/common/create_opts.go | 2 +- cmd/podman/containers/create.go | 2 ++ cmd/podman/machine/init.go | 4 +-- docs/source/markdown/podman.1.md | 4 +-- pkg/api/handlers/compat/containers_create.go | 2 +- pkg/domain/entities/pods.go | 2 +- pkg/domain/filters/containers.go | 2 +- pkg/domain/infra/abi/play.go | 16 +++++++++ pkg/machine/e2e/config_init_test.go | 2 +- pkg/specgen/generate/kube/kube.go | 2 ++ pkg/specgen/generate/storage.go | 35 ++++++++++++++++++-- pkg/specgen/specgen.go | 4 +++ pkg/specgenutil/specgen.go | 13 +++----- pkg/specgenutil/volumes.go | 23 ------------- test/system/030-run.bats | 13 ++++++++ test/system/160-volumes.bats | 10 ++++++ test/system/700-play.bats | 35 ++++++++++++++++++++ 19 files changed, 133 insertions(+), 46 deletions(-) diff --git a/DOWNLOADS.md b/DOWNLOADS.md index 1140aafad696..046ff5eabb57 100644 --- a/DOWNLOADS.md +++ b/DOWNLOADS.md @@ -30,7 +30,7 @@ don't take them beyond that. WARNING: The items linked below all come from scripts in the `artifacts_task` map of `.cirrus.yml`. When adding or updating any item below, please ensure it -matches cooresponding changes in the artifacts task. +matches corresponding changes in the artifacts task. --> diff --git a/cmd/podman/common/create.go b/cmd/podman/common/create.go index 853e37b59ac3..42b6d28d5677 100644 --- a/cmd/podman/common/create.go +++ b/cmd/podman/common/create.go @@ -377,12 +377,12 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions, ) createFlags.BoolVar( &cf.ReadOnly, - "read-only", false, + "read-only", podmanConfig.ContainersConfDefaultsRO.Containers.ReadOnly, "Make containers root filesystem read-only", ) createFlags.BoolVar( - &cf.ReadOnlyTmpFS, - "read-only-tmpfs", cf.ReadOnlyTmpFS, + &cf.ReadWriteTmpFS, + "read-only-tmpfs", cf.ReadWriteTmpFS, "When running containers in read-only mode mount a read-write tmpfs on /run, /tmp and /var/tmp", ) requiresFlagName := "requires" diff --git a/cmd/podman/common/create_opts.go b/cmd/podman/common/create_opts.go index d77df29edb3b..b67ee8f70725 100644 --- a/cmd/podman/common/create_opts.go +++ b/cmd/podman/common/create_opts.go @@ -83,7 +83,7 @@ func DefineCreateDefaults(opts *entities.ContainerCreateOptions) { opts.MemorySwappiness = -1 opts.ImageVolume = podmanConfig.ContainersConfDefaultsRO.Engine.ImageVolumeMode opts.Pull = policy() - opts.ReadOnlyTmpFS = true + opts.ReadWriteTmpFS = true opts.SdNotifyMode = define.SdNotifyModeContainer opts.StopTimeout = podmanConfig.ContainersConfDefaultsRO.Engine.StopTimeout opts.Systemd = "true" diff --git a/cmd/podman/containers/create.go b/cmd/podman/containers/create.go index e15877023f94..d9da303cf590 100644 --- a/cmd/podman/containers/create.go +++ b/cmd/podman/containers/create.go @@ -409,6 +409,8 @@ func createPodIfNecessary(cmd *cobra.Command, s *specgen.SpecGenerator, netOpts infraOpts := entities.NewInfraContainerCreateOptions() infraOpts.Net = netOpts infraOpts.Quiet = true + infraOpts.ReadOnly = true + infraOpts.ReadWriteTmpFS = false infraOpts.Hostname, err = cmd.Flags().GetString("hostname") if err != nil { return err diff --git a/cmd/podman/machine/init.go b/cmd/podman/machine/init.go index c0cfe6ceaef6..57cb9094375e 100644 --- a/cmd/podman/machine/init.go +++ b/cmd/podman/machine/init.go @@ -89,11 +89,11 @@ func init() { _ = flags.MarkHidden("reexec") UsernameFlagName := "username" - flags.StringVar(&initOpts.Username, UsernameFlagName, cfg.ContainersConfDefaultsRO.Machine.User, "Username used in qcow image") + flags.StringVar(&initOpts.Username, UsernameFlagName, cfg.ContainersConfDefaultsRO.Machine.User, "Username used in image") _ = initCmd.RegisterFlagCompletionFunc(UsernameFlagName, completion.AutocompleteDefault) ImagePathFlagName := "image-path" - flags.StringVar(&initOpts.ImagePath, ImagePathFlagName, cfg.ContainersConfDefaultsRO.Machine.Image, "Path to qcow image") + flags.StringVar(&initOpts.ImagePath, ImagePathFlagName, cfg.ContainersConfDefaultsRO.Machine.Image, "Path to bootable image") _ = initCmd.RegisterFlagCompletionFunc(ImagePathFlagName, completion.AutocompleteDefault) VolumeFlagName := "volume" diff --git a/docs/source/markdown/podman.1.md b/docs/source/markdown/podman.1.md index d93f44159676..134765672637 100644 --- a/docs/source/markdown/podman.1.md +++ b/docs/source/markdown/podman.1.md @@ -164,9 +164,9 @@ NOTE --tmpdir is not used for the temporary storage of downloaded images. Use t #### **--transient-store** -Enables a global transient storaga mode where all container metadata is stored on non-persistant media (i.e. in the location specified by `--runroot`). +Enables a global transient storage mode where all container metadata is stored on non-persistent media (i.e. in the location specified by `--runroot`). This mode allows starting containers faster, as well as guaranteeing a fresh state on boot in case of unclean shutdowns or other problems. However -it is not compabible with a traditional model where containers persist across reboots. +it is not compatible with a traditional model where containers persist across reboots. Default value for this is configured in `containers-storage.conf(5)`. diff --git a/pkg/api/handlers/compat/containers_create.go b/pkg/api/handlers/compat/containers_create.go index 234c101c81ff..107070f2c086 100644 --- a/pkg/api/handlers/compat/containers_create.go +++ b/pkg/api/handlers/compat/containers_create.go @@ -413,7 +413,7 @@ func cliOpts(cc handlers.CreateContainerConfig, rtc *config.Config) (*entities.C PublishAll: cc.HostConfig.PublishAllPorts, Quiet: false, ReadOnly: cc.HostConfig.ReadonlyRootfs, - ReadOnlyTmpFS: true, // podman default + ReadWriteTmpFS: true, // podman default Rm: cc.HostConfig.AutoRemove, SecurityOpt: cc.HostConfig.SecurityOpt, StopSignal: cc.Config.StopSignal, diff --git a/pkg/domain/entities/pods.go b/pkg/domain/entities/pods.go index a70e3c8dcaaa..36676d56d2e4 100644 --- a/pkg/domain/entities/pods.go +++ b/pkg/domain/entities/pods.go @@ -249,7 +249,7 @@ type ContainerCreateOptions struct { Pull string Quiet bool ReadOnly bool - ReadOnlyTmpFS bool + ReadWriteTmpFS bool Restart string Replace bool Requires []string diff --git a/pkg/domain/filters/containers.go b/pkg/domain/filters/containers.go index 137f3da5247c..67f727fd838a 100644 --- a/pkg/domain/filters/containers.go +++ b/pkg/domain/filters/containers.go @@ -146,7 +146,7 @@ func GenerateContainerFilterFuncs(filter string, filterValues []string, r *libpo if dest != "" && (mount.Source == source && mount.Destination == dest) { return true } - if dest == "" && mount.Source == source { + if dest == "" && mount.Destination == source { return true } } diff --git a/pkg/domain/infra/abi/play.go b/pkg/domain/infra/abi/play.go index 430ce7bd4120..e3752c0a6191 100644 --- a/pkg/domain/infra/abi/play.go +++ b/pkg/domain/infra/abi/play.go @@ -68,6 +68,8 @@ func (ic *ContainerEngine) createServiceContainer(ctx context.Context, name stri ImageVolume: "bind", IsInfra: false, MemorySwappiness: -1, + ReadOnly: true, + ReadWriteTmpFS: false, // No need to spin up slirp etc. Net: &entities.NetOptions{Network: specgen.Namespace{NSMode: specgen.NoNetwork}}, } @@ -560,6 +562,8 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY infraImage := util.DefaultContainerConfig().Engine.InfraImage infraOptions := entities.NewInfraContainerCreateOptions() infraOptions.Hostname = podSpec.PodSpecGen.PodBasicConfig.Hostname + infraOptions.ReadOnly = true + infraOptions.ReadWriteTmpFS = false infraOptions.UserNS = options.Userns podSpec.PodSpecGen.InfraImage = infraImage podSpec.PodSpecGen.NoInfra = false @@ -605,6 +609,16 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY } } + cfg, err := ic.Libpod.GetConfigNoCopy() + if err != nil { + return nil, nil, err + } + + var readOnly types.OptionalBool + if cfg.Containers.ReadOnly { + readOnly = types.NewOptionalBool(cfg.Containers.ReadOnly) + } + ctrNames := make(map[string]string) for _, initCtr := range podYAML.Spec.InitContainers { // Error out if same name is used for more than one container @@ -643,6 +657,7 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY PodInfraID: podInfraID, PodName: podName, PodSecurityContext: podYAML.Spec.SecurityContext, + ReadOnly: readOnly, RestartPolicy: define.RestartPolicyNo, SeccompPaths: seccompPaths, SecretsManager: secretsManager, @@ -697,6 +712,7 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY PodInfraID: podInfraID, PodName: podName, PodSecurityContext: podYAML.Spec.SecurityContext, + ReadOnly: readOnly, RestartPolicy: ctrRestartPolicy, SeccompPaths: seccompPaths, SecretsManager: secretsManager, diff --git a/pkg/machine/e2e/config_init_test.go b/pkg/machine/e2e/config_init_test.go index 43313ad2f997..47657c8d4f4c 100644 --- a/pkg/machine/e2e/config_init_test.go +++ b/pkg/machine/e2e/config_init_test.go @@ -10,7 +10,7 @@ type initMachine struct { --disk-size uint Disk size in GB (default 100) --ignition-path string Path to ignition file --username string Username of the remote user (default "core" for FCOS, "user" for Fedora) - --image-path string Path to qcow image (default "testing") + --image-path string Path to bootable image (default "testing") -m, --memory uint Memory in MB (default 2048) --now Start machine now --rootful Whether this machine should prefer rootful container execution diff --git a/pkg/specgen/generate/kube/kube.go b/pkg/specgen/generate/kube/kube.go index 4ca74fc40ea9..29704e61498e 100644 --- a/pkg/specgen/generate/kube/kube.go +++ b/pkg/specgen/generate/kube/kube.go @@ -472,6 +472,8 @@ func ToSpecGen(ctx context.Context, opts *CtrSpecGenOptions) (*specgen.SpecGener if ro := opts.ReadOnly; ro != itypes.OptionalBoolUndefined { s.ReadOnlyFilesystem = (ro == itypes.OptionalBoolTrue) } + // This should default to true for kubernetes yaml + s.ReadWriteTmpfs = true // Make sure the container runs in a systemd unit which is // stored as a label at container creation. diff --git a/pkg/specgen/generate/storage.go b/pkg/specgen/generate/storage.go index 6b131c1ddc60..ffaaa0e1fb8f 100644 --- a/pkg/specgen/generate/storage.go +++ b/pkg/specgen/generate/storage.go @@ -159,14 +159,19 @@ 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, specgen.ErrDuplicateDest) + return nil, nil, nil, fmt.Errorf("baseMounts 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, specgen.ErrDuplicateDest) + return nil, nil, nil, fmt.Errorf("baseVolumes conflict at mount destination %v: %w", dest, specgen.ErrDuplicateDest) } } + + if s.ReadWriteTmpfs { + baseMounts = addReadWriteTmpfsMounts(baseMounts, s.Volumes) + } + // Final step: maps to arrays finalMounts := make([]spec.Mount, 0, len(baseMounts)) for _, mount := range baseMounts { @@ -427,3 +432,29 @@ func InitFSMounts(mounts []spec.Mount) error { } return nil } + +func addReadWriteTmpfsMounts(mounts map[string]spec.Mount, volumes []*specgen.NamedVolume) map[string]spec.Mount { + readonlyTmpfs := []string{"/tmp", "/var/tmp", "/run"} + options := []string{"rw", "rprivate", "nosuid", "nodev", "tmpcopyup"} + for _, dest := range readonlyTmpfs { + if _, ok := mounts[dest]; ok { + continue + } + for _, m := range volumes { + if m.Dest == dest { + continue + } + } + mnt := spec.Mount{ + Destination: dest, + Type: define.TypeTmpfs, + Source: define.TypeTmpfs, + Options: options, + } + if dest != "/run" { + mnt.Options = append(mnt.Options, "noexec") + } + mounts[dest] = mnt + } + return mounts +} diff --git a/pkg/specgen/specgen.go b/pkg/specgen/specgen.go index 6daa4dc018d3..2e7078115efc 100644 --- a/pkg/specgen/specgen.go +++ b/pkg/specgen/specgen.go @@ -384,6 +384,10 @@ type ContainerSecurityConfig struct { // ReadOnlyFilesystem indicates that everything will be mounted // as read-only ReadOnlyFilesystem bool `json:"read_only_filesystem,omitempty"` + // ReadWriteTmpfs indicates that when running with a ReadOnlyFilesystem + // mount temporary file systems + ReadWriteTmpfs bool `json:"read_write_tmpfs,omitempty"` + // Umask is the umask the init process of the container will be run with. Umask string `json:"umask,omitempty"` // ProcOpts are the options used for the proc mount. diff --git a/pkg/specgenutil/specgen.go b/pkg/specgenutil/specgen.go index 8c64508bef86..695018f6fcae 100644 --- a/pkg/specgenutil/specgen.go +++ b/pkg/specgenutil/specgen.go @@ -592,10 +592,11 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions s.DependencyContainers = c.Requires } - // TODO - // outside of specgen and oci though - // defaults to true, check spec/storage - // s.readonly = c.ReadOnlyTmpFS + // Only add ReadWrite tmpfs mounts iff the container is + // being run ReadOnly and ReadWriteTmpFS is not disabled, + // (user specifying --read-only-tmpfs=false.) + s.ReadWriteTmpfs = c.ReadOnly && c.ReadWriteTmpFS + // TODO convert to map? // check if key=value and convert sysmap := make(map[string]string) @@ -853,10 +854,6 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions s.PasswdEntry = c.PasswdEntry } - if c.ReadOnly && c.ReadOnlyTmpFS { - s.Mounts = addReadOnlyMounts(s.Mounts) - } - return nil } diff --git a/pkg/specgenutil/volumes.go b/pkg/specgenutil/volumes.go index 6b6c0d91ce28..c70206addfea 100644 --- a/pkg/specgenutil/volumes.go +++ b/pkg/specgenutil/volumes.go @@ -703,26 +703,3 @@ func validChownFlag(flag string) (bool, error) { func unixPathClean(p string) string { return path.Clean(p) } - -func addReadOnlyMounts(mounts []spec.Mount) []spec.Mount { - readonlyTmpfs := []string{"/tmp", "/var/tmp", "/run"} - options := []string{"rw", "rprivate", "nosuid", "nodev", "tmpcopyup"} - for _, dest := range readonlyTmpfs { - for _, m := range mounts { - if m.Destination == dest { - continue - } - } - mnt := spec.Mount{ - Destination: dest, - Type: define.TypeTmpfs, - Source: define.TypeTmpfs, - Options: options, - } - if dest != "/run" { - mnt.Options = append(mnt.Options, "noexec") - } - mounts = append(mounts, mnt) - } - return mounts -} diff --git a/test/system/030-run.bats b/test/system/030-run.bats index fff16a1d597c..171008393982 100644 --- a/test/system/030-run.bats +++ b/test/system/030-run.bats @@ -951,4 +951,17 @@ $IMAGE--c_ok" \ run_podman stop -t 0 $cid } +@test "podman run read-only from containers.conf" { + containersconf=$PODMAN_TMPDIR/containers.conf + cat >$containersconf <$containersconf <