Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quadlet option Readonly sets --read-only-tmpfs to false #20439

Closed
job79 opened this issue Oct 22, 2023 · 7 comments · Fixed by #20479
Closed

Quadlet option Readonly sets --read-only-tmpfs to false #20439

job79 opened this issue Oct 22, 2023 · 7 comments · Fixed by #20479
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@job79
Copy link

job79 commented Oct 22, 2023

Issue Description

When using ReadOnly=true inside a quadlet file, the following flags are added to the generated service file: --read-only --read-only-tmpfs=false
This is probably not a great idea because the default value for read-only-tmpfs is true when using podman run and there is no easy way to set this value to true using the quadlet file. This also breaks existing installations that expect to have a writable /dev/shm and are using ReadOnly=true

Used quadlet file and generated run line:

[Container]
Image=docker.io/postgres:16-alpine
AutoUpdate=registry

# environment
Secret=news-postgres-password,target=POSTGRES_PASSWORD,type=env

# storage
Volume=news-postgres.volume:/var/lib/postgresql/data
Volume=news-postgres-backup.volume:/backup
Tmpfs=/var/run

# networking
Network=news.network

# security
ReadOnly=true
NoNewPrivileges=true
DropCapability=ALL
AddCapability=CHOWN DAC_OVERRIDE FOWNER SETGID SETUID
ExecStart=/usr/bin/podman run --name=systemd-%N --cidfile=%t/%N.cid --replace --rm --cgroups=split --network=systemd-news --sdnotify=conmon -d --security-opt=no-new-privileges --cap-drop=all --cap-add=chown --cap-add=dac_override --cap-add=fowner --cap-add=setgid --cap-add=setuid --read-only --read-only-tmpfs=false --tmpfs /var/run -v systemd-news-postgres:/var/lib/postgresql/data -v systemd-news-postgres-backup:/backup --label io.containers.autoupdate=registry --secret news-postgres-password,target=POSTGRES_PASSWORD,type=env docker.io/postgres:16-alpine

Steps to reproduce the issue

Steps to reproduce the issue

  1. Create a quadlet file that sets ReadOnly to true
  2. See how the flag is set to false using /usr/libexec/podman/quadlet -dryrun -user

Describe the results you received

ReadOnly=true inside a quadlet file adds the following flags to the generated service: --read-only --read-only-tmpfs=false

Describe the results you expected

I expect --read-only-tmpfs=false to not be set at all, because the quadlet documentation implies that ReadOnly=true only adds --read-only.
There should be a separate setting that can set --read-only-tmpfs to false.

podman info output

host:
  arch: arm64
  buildahVersion: 1.32.0
  cgroupControllers:
  - cpu
  - memory
  - pids
  cgroupManager: systemd
  cgroupVersion: v2
  conmon:
    package: conmon-2.1.7-2.fc38.aarch64
    path: /usr/bin/conmon
    version: 'conmon version 2.1.7, commit: '
  cpuUtilization:
    idlePercent: 98.9
    systemPercent: 0.49
    userPercent: 0.61
  cpus: 4
  databaseBackend: boltdb
  distribution:
    distribution: fedora
    variant: coreos
    version: "38"
  eventLogger: journald
  freeLocks: 1999
  hostname: omega
  idMappings:
    gidmap:
    - container_id: 0
      host_id: 1000
      size: 1
    - container_id: 1
      host_id: 524288
      size: 65536
    uidmap:
    - container_id: 0
      host_id: 1000
      size: 1
    - container_id: 1
      host_id: 524288
      size: 65536
  kernel: 6.5.5-200.fc38.aarch64
  linkmode: dynamic
  logDriver: journald
  memFree: 4262461440
  memTotal: 8099545088
  networkBackend: netavark
  networkBackendInfo:
    backend: netavark
    dns:
      package: aardvark-dns-1.7.0-1.fc38.aarch64
      path: /usr/libexec/podman/aardvark-dns
      version: aardvark-dns 1.7.0
    package: netavark-1.7.0-1.fc38.aarch64
    path: /usr/libexec/podman/netavark
    version: netavark 1.7.0
  ociRuntime:
    name: crun
    package: crun-1.9.2-1.fc38.aarch64
    path: /usr/bin/crun
    version: |-
      crun version 1.9.2
      commit: 35274d346d2e9ffeacb22cc11590b0266a23d634
      rundir: /run/user/1000/crun
      spec: 1.0.0
      +SYSTEMD +SELINUX +APPARMOR +CAP +SECCOMP +EBPF +CRIU +LIBKRUN +WASM:wasmedge +YAJL
  os: linux
  pasta:
    executable: /usr/bin/pasta
    package: passt-0^20230908.g05627dc-1.fc38.aarch64
    version: |
      pasta 0^20230908.g05627dc-1.fc38.aarch64-pasta
      Copyright Red Hat
      GNU General Public License, version 2 or later
        <https://www.gnu.org/licenses/old-licenses/gpl-2.0.html>
      This is free software: you are free to change and redistribute it.
      There is NO WARRANTY, to the extent permitted by law.
  remoteSocket:
    exists: false
    path: /run/user/1000/podman/podman.sock
  security:
    apparmorEnabled: false
    capabilities: CAP_CHOWN,CAP_DAC_OVERRIDE,CAP_FOWNER,CAP_FSETID,CAP_KILL,CAP_NET_BIND_SERVICE,CAP_SETFCAP,CAP_SETGID,CAP_SETPCAP,CAP_SETUID,CAP_SYS_CHROOT
    rootless: true
    seccompEnabled: true
    seccompProfilePath: /usr/share/containers/seccomp.json
    selinuxEnabled: true
  serviceIsRemote: false
  slirp4netns:
    executable: /usr/bin/slirp4netns
    package: slirp4netns-1.2.1-1.fc38.aarch64
    version: |-
      slirp4netns version 1.2.1
      commit: 09e31e92fa3d2a1d3ca261adaeb012c8d75a8194
      libslirp: 4.7.0
      SLIRP_CONFIG_VERSION_MAX: 4
      libseccomp: 2.5.3
  swapFree: 4049596416
  swapTotal: 4049596416
  uptime: 1h 59m 11.00s (Approximately 0.04 days)
plugins:
  authorization: null
  log:
  - k8s-file
  - none
  - passthrough
  - journald
  network:
  - bridge
  - macvlan
  - ipvlan
  volume:
  - local
registries:
  search:
  - registry.fedoraproject.org
  - registry.access.redhat.com
  - docker.io
  - quay.io
store:
  configFile: /var/home/core/.config/containers/storage.conf
  containerStore:
    number: 16
    paused: 0
    running: 16
    stopped: 0
  graphDriverName: overlay
  graphOptions: {}
  graphRoot: /var/home/core/.local/share/containers/storage
  graphRootAllocated: 81318096896
  graphRootUsed: 15708360704
  graphStatus:
    Backing Filesystem: xfs
    Native Overlay Diff: "true"
    Supports d_type: "true"
    Supports shifting: "false"
    Supports volatile: "true"
    Using metacopy: "false"
  imageCopyTmpDir: /var/tmp
  imageStore:
    number: 17
  runRoot: /run/user/1000/containers
  transientStore: false
  volumePath: /var/home/core/.local/share/containers/storage/volumes
version:
  APIVersion: 4.7.0
  Built: 1695839065
  BuiltTime: Wed Sep 27 20:24:25 2023
  GitCommit: ""
  GoVersion: go1.20.8
  Os: linux
  OsArch: linux/arm64
  Version: 4.7.0

Podman in a container

No

Privileged Or Rootless

Rootless

Upstream Latest Release

No

Additional environment details

No response

Additional information

No response

@job79 job79 added the kind/bug Categorizes issue or PR as related to a bug. label Oct 22, 2023
@ygalblum
Copy link
Contributor

ygalblum commented Oct 22, 2023

@job79 thanks for bringing this up

Quadlet uses the combination of two keys: ReadOnly and VolatileTmp to set the following flags:

ReadOnly=true ReadOnly=false
VolatileTmp=true --read-only --tmpfs /tmp:rw,size=512M,mode=1777
VolatileTmp=false --read-only --read-only-tmpfs=false

So, if all you're looking for is to remove --read-only-tmpfs=false you need to add VolatileTmp=true to your Quadlet file.

@alexlarsson do you remember why you tied these two keys together in this manner?

@rhatdan
Copy link
Member

rhatdan commented Oct 22, 2023

I agree this should stay with the default of read-only-tmpfs=true.

@job79
Copy link
Author

job79 commented Oct 23, 2023

So, if all you're looking for is to remove --read-only-tmpfs=false you need to add VolatileTmp=true to your Quadlet file.

Thanks! This does indeed work, but still don't know if it is a good idea to use read-only-tmpfs=false by default. This could be confusing for users that are used to podman run and are learning the quadlet format.

Personally I would expect a ReadOnlyTmpfs option in the quadlet file. This would keep podman run and the quadlet format the same. As a bonus the ReadOnlyTmpfs option would be right under ReadOnly in the documentation.

But maybe there is a good reason why VolatileTmp is used like this? And would it even be possible to change the format?

@alexlarsson
Copy link
Contributor

I can't say I 100% sure remember the details, but I think the reason I did this is that if --read-only* is not used, then there is no way to get a transient /tmp, so I added VolatileTmp to do this. This was added before the ReadOnly option. And once there was a VolatileTmp option it felt weird to have a newly added ReadOnly=true change the default value of the VolatileTmp option.

Otoh, I agree that its weird that ReadOnly differs from --read-only.

@rhatdan
Copy link
Member

rhatdan commented Oct 23, 2023

Lets change ReadOnly to match --read-only and then users could set the VolatileTmp=false to get the current behaviour. I am pretty sure noone is using ReadOnly in the current manner.

@ygalblum
Copy link
Contributor

I'll try to summarize the behavior we want to see before I implement it.

  • ReadOnly translates to --read-only.
  • ReadOnlyTmpfs translates to --read-only-tmpfs.

Since podman accepts --read-only-tmpfs regardless to that value of --read-only, there will be no validation, just a one-to-one translation. In both cases, if the key is not set, no argument is passed.

Now about, VolatileTmp; I think it's a leftover from the original opinionated version of Quadlet and I'm not sure what to do with it. Quadlet already supports the Tmpfs key. So, VolatileTmp is not really needed. Having said that, maybe for backward compatibility (though, not sure it is actually used), we should keep the current behavior. That is, if VolatileTmp=true and ReadOnly is either set to false or not set at all, add --tmpfs /tmp:rw,size=512M,mode=1777. However, I think we should add a comment in the man page stating that using the Tmpfs key is preferable.

WDYT?

@rhatdan
Copy link
Member

rhatdan commented Oct 24, 2023

Keep it but "undocument" it. If users are using it, it will continue to work, but remove the information from the man pages.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jan 24, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants