Templated quadlet container name does not escape invalid characters

alaviss commented 5 months ago

Issue Description

Templated containers will not start if invalid characters (ie. :) are present in the instance name. This will happen if systemd socket activation with Accept=true is used.

Sample files:

test@.container:

```ini [Unit] Description=Test container [Container] Image=docker.io/library/alpine:latest Exec=echo "hello" Network=none LogDriver=passthrough [Service] StandardInput=socket StandardOutput=socket StandardError=journal ```

test.socket:

```ini [Unit] Description=Test socket [Socket] ListenStream=8080 Accept=true ```

Steps to reproduce the issue

Install test@.container and test.socket in appropriate locations
Start test.socket
Run socat tcp:localhost:8080 stdout

Describe the results you received

Got no output, related journal entries:

Jun 01 21:57:22 workstation test@15-::1:8080-::1:59290[18245]: Error: running container create option: names must match [a-zA-Z0-9][a-zA-Z0-9_.-]*: invalid argument
Jun 01 21:57:22 workstation podman[18245]: 2024-06-01 21:57:22.809971989 -0500 CDT m=+0.017692997 image pull 1d34ffeaf190be23d3de5a8de0a436676b758f48f835c3a2d4768b798c15a7f1 docker.io/library/alpine:latest
Jun 01 21:57:22 workstation systemd[2414]: test@15-::1:8080-::1:59290.service: Main process exited, code=exited, status=125/n/a

Describe the results you expected

This should show up:

hello

podman info output

host:
  arch: amd64
  buildahVersion: 1.35.4
  cgroupControllers:
  - pids
  cgroupManager: systemd
  cgroupVersion: v2
  conmon:
    package: conmon-2.1.12-1.1.x86_64
    path: /usr/bin/conmon
    version: 'conmon version 2.1.12, commit: unknown'
  cpuUtilization:
    idlePercent: 99.82
    systemPercent: 0.06
    userPercent: 0.12
  cpus: 12
  databaseBackend: sqlite
  distribution:
    distribution: opensuse-microos
    version: "20240524"
  eventLogger: journald
  freeLocks: 2029
  hostname: workstation
  idMappings:
    gidmap:
    - container_id: 0
      host_id: 60252
      size: 1
    - container_id: 1
      host_id: 524288
      size: 65536
    uidmap:
    - container_id: 0
      host_id: 60252
      size: 1
    - container_id: 1
      host_id: 524288
      size: 65536
  kernel: 6.9.1-1-default
  linkmode: dynamic
  logDriver: journald
  memFree: 485367808
  memTotal: 16693542912
  networkBackend: netavark
  networkBackendInfo:
    backend: netavark
    dns:
      package: aardvark-dns-1.10.0-1.3.x86_64
      path: /usr/libexec/podman/aardvark-dns
      version: aardvark-dns 1.10.0
    package: netavark-1.10.3-1.2.x86_64
    path: /usr/libexec/podman/netavark
    version: netavark 1.10.3
  ociRuntime:
    name: crun
    package: crun-1.14.4-1.2.x86_64
    path: /usr/bin/crun
    version: |-
      crun version 1.14.4
      commit: a220ca661ce078f2c37b38c92e66cf66c012d9c1
      rundir: /run/user/60252/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-20240510.7288448-1.1.x86_64
    version: |
      pasta 20240510.7288448-1.1
      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/60252/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: /etc/containers/seccomp.json
    selinuxEnabled: true
  serviceIsRemote: false
  slirp4netns:
    executable: ""
    package: ""
    version: ""
  swapFree: 8589930496
  swapTotal: 8589930496
  uptime: 42h 44m 48.00s (Approximately 1.75 days)
  variant: ""
plugins:
  authorization: null
  log:
  - k8s-file
  - none
  - passthrough
  - journald
  network:
  - bridge
  - macvlan
  - ipvlan
  volume:
  - local
registries:
  search:
  - registry.opensuse.org
  - registry.suse.com
  - docker.io
store:
  configFile: /home/user/.config/containers/storage.conf
  containerStore:
    number: 1
    paused: 0
    running: 1
    stopped: 0
  graphDriverName: overlay
  graphOptions: {}
  graphRoot: /home/user/.local/share/containers/storage
  graphRootAllocated: 771888361472
  graphRootUsed: 106482032640
  graphStatus:
    Backing Filesystem: btrfs
    Native Overlay Diff: "true"
    Supports d_type: "true"
    Supports shifting: "false"
    Supports volatile: "true"
    Using metacopy: "false"
  imageCopyTmpDir: /var/tmp
  imageStore:
    number: 22
  runRoot: /run/user/60252/containers
  transientStore: false
  volumePath: /home/user/.local/share/containers/storage/volumes
version:
  APIVersion: 5.0.3
  Built: 1715364600
  BuiltTime: Fri May 10 13:10:00 2024
  GitCommit: ""
  GoVersion: go1.21.10
  Os: linux
  OsArch: linux/amd64
  Version: 5.0.3

Podman in a container

No

Privileged Or Rootless

Rootless

Upstream Latest Release

No

Additional environment details

No response

Additional information

No response

Luap99 commented 5 months ago

quadlet uses systemd-%P_%I for templates as name so it doesn't actually parse the name at all currently. I am not sure how qualdet should fix this, i.e. how can we convert a unit name to a valid container name?

cc @ygalblum @alexlarsson

ygalblum commented 4 months ago

@Luap99 what if we just add something at the end of the string. Not sure what, but whatever string. Will that do the trick?

Luap99 commented 4 months ago

Container names (pods, networks, volumes as well) must confirm to this regex [a-zA-Z0-9][a-zA-Z0-9_.-]* so no.

This seems a general problem, unit names allow different chars than our names so using any of the systemd specifies for names makes quadlet incompatible with podman in such case. One thing is to tell users to not name units like this but given the example is here template units with Accept=yes the users has no control over the name AFAICT

But even if we try to stop using the systemd specifiers and make qualdet convert the name it still cannot work for template units as it does not know the instance names ahead of time. I see there is %i that contains the escaped name so maybe using this over %I (unescaped) makes it work? Although if it used the normal escape logic it writes special chars as \x... and the back slash would not be accepted either by podman.

So really I see no way to fix this besides patching podman to allow such names which I think would have a lot of other consequences we do not like.

ygalblum commented 4 months ago

I thought the specifiers where returning an empty string. But, now I realize that no and that there is even more to it.

In testing this issue, I found that systemd replaces - with / for both the I and P specifiers. As a result naming either the template Quadlet file or the instance with - will result in an error.

For reference, in order to test different cases I used this service unit file:

[Unit]
Wants=network-online.target
After=network-online.target

[Service]
ExecStart=/usr/bin/echo systemd-%P_%I
Type=oneshot
RemainAfterExit=yes

So, I think that at minimum, we should consider replacing I and P with i and p. But, I'm not sure it won't open a new can of worms.

Luap99 commented 4 months ago

see https://www.freedesktop.org/software/systemd/man/latest/systemd.unit.html#String%20Escaping%20for%20Inclusion%20in%20Unit%20Names for the escape rules, the - to / is expected as they use this for path escaping.

Using %i and %p seems logical in that regard because we can at least keep the dash. I don't think it would cause other problems. All the special chars that might be used currently where not accepted by podman in the first place.

ygalblum commented 4 months ago

I've opened this PR: #23029 to change the behavior. I did not mark it as resolving this issue because from what I can tell, this specific case will still not work

github-actions[bot] commented 3 months ago

A friendly reminder that this issue had no activity for 30 days.

thmo commented 1 month ago

Maybe I'm reading the MR (#23029) incorrectly, but it seems to me the change was only done for .container, not for .pod?

containers / podman