[quadlet] add back support for directory symlinks in the podman unit search paths

xduugu commented 11 months ago

Issue Description

I had symlinked ~/.config/containers/systemd/ to the directory ~/containers/quadlet which worked fine in podman version v4.6.2 and before. My setup broke with podman v4.7.0, because of commit 413552e that added support for unit files in subdirectories.

https://github.com/containers/podman/blob/c3fae0136882e69b0f3fb4aa382156d63aa3f20f/cmd/quadlet/main.go#L148-L156

filepath.WalkDir explicitly does not support directory symlinks:

WalkDir does not follow symbolic links.

Directory symlinks inside of a directory ~/.config/containers/systemd/ also don't work because of this (and probably the info.isDir() check).

Steps to reproduce the issue

create a symlink from ~/.config/containers/systemd/ to a directory that contains quadlet unit files
run quadlet

Describe the results you received

Quadlet does not find the unit files anymore.

Describe the results you expected

Quadlet finds the unit files.

podman info output

host:
  arch: amd64
  buildahVersion: 1.33.0-dev
  cgroupControllers:
  - cpu
  - io
  - memory
  - pids
  cgroupManager: systemd
  cgroupVersion: v2
  conmon:
    package: conmon-2.1.7-3.fc39.x86_64
    path: /usr/bin/conmon
    version: 'conmon version 2.1.7, commit: '
  cpuUtilization:
    idlePercent: 97.66
    systemPercent: 0.83
    userPercent: 1.5
  cpus: 16
  databaseBackend: boltdb
  distribution:
    distribution: fedora
    variant: workstation
    version: "39"
  eventLogger: journald
  freeLocks: 2048
  hostname: fedora
  idMappings:
    gidmap:
    - container_id: 0
      host_id: 1000
      size: 1
    - container_id: 1
      host_id: 100000
      size: 65536
    uidmap:
    - container_id: 0
      host_id: 1000
      size: 1
    - container_id: 1
      host_id: 100000
      size: 65536
  kernel: 6.5.6-300.fc39.x86_64
  linkmode: dynamic
  logDriver: journald
  memFree: 17862057984
  memTotal: 32817713152
  networkBackend: netavark
  networkBackendInfo:
    backend: netavark
    dns:
      package: aardvark-dns-1.7.0-2.fc39.x86_64
      path: /usr/libexec/podman/aardvark-dns
      version: aardvark-dns 1.7.0
    package: netavark-1.7.0-2.fc39.x86_64
    path: /usr/libexec/podman/netavark
    version: netavark 1.7.0
  ociRuntime:
    name: crun
    package: crun-1.9-1.fc39.x86_64
    path: /usr/bin/crun
    version: |-
      crun version 1.9
      commit: a538ac4ea1ff319bcfe2bf81cb5c6f687e2dc9d3
      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.fc39.x86_64
    version: |
      pasta 0^20230908.g05627dc-1.fc39.x86_64
      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: false
  serviceIsRemote: false
  slirp4netns:
    executable: /usr/bin/slirp4netns
    package: slirp4netns-1.2.1-1.fc39.x86_64
    version: |-
      slirp4netns version 1.2.1
      commit: 09e31e92fa3d2a1d3ca261adaeb012c8d75a8194
      libslirp: 4.7.0
      SLIRP_CONFIG_VERSION_MAX: 4
      libseccomp: 2.5.3
  swapFree: 8589930496
  swapTotal: 8589930496
  uptime: 1h 13m 17.00s (Approximately 0.04 days)
  variant: ""
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: /home/xduugu/.config/containers/storage.conf
  containerStore:
    number: 0
    paused: 0
    running: 0
    stopped: 0
  graphDriverName: overlay
  graphOptions: {}
  graphRoot: /home/xduugu/.local/share/containers/storage
  graphRootAllocated: 998380109824
  graphRootUsed: 335406395392
  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: 4
  runRoot: /run/user/1000/containers
  transientStore: false
  volumePath: /home/xduugu/.local/share/containers/storage/volumes
version:
  APIVersion: 4.8.0-dev
  Built: 1698350044
  BuiltTime: Thu Oct 26 21:54:04 2023
  GitCommit: e5101b28deff158feb8cd69a25c082a441c07c9a
  GoVersion: go1.21.3
  Os: linux
  OsArch: linux/amd64
  Version: 4.8.0-dev

Podman in a container

No

Privileged Or Rootless

None

Upstream Latest Release

Yes

Additional environment details

Additional information

Instead of adding back support for directory symlinks, it would also help if additional quadlet unit directories could be set via configuration file.

ygalblum commented 10 months ago

Hi @xduugu thanks for bringing this up.

The problem is that once Quadlet supports traversing a directory tree, following symlinks may lead to loops (see the reason why it is not supported in WalkDir: https://github.com/golang/go/issues/4759).

I do like the idea of adding directories via the configuration file. @rhatdan @vrothberg WDYT?

vrothberg commented 10 months ago

We can't really use containers.conf as it would binary-bloat Quadlet. And I do not like introducing "yet another" configuration file just for Quadlet.

I think we should restore the previous behavior as I consider a regression. Couldn't resolve the root directories before recursing/walking it? In this case, we could first resolve ~/.config/containers/systemd/ which resolves to ~/containers/quadlet and then walk ~/containers/quadlet for sub-directories.

If this works and we agree to move forward, the PR fixing it could also make the behavior explicit in the docs.

ygalblum commented 10 months ago

If the symlink resolution is only for the root directory, then I guess we can handle it and it will not cause any loops (since it is done only once). Is this the only case we want to support?

For this case, I think we can use: https://pkg.go.dev/io/fs#WalkDir

xduugu commented 10 months ago

At least for my use case, root directory symlink support would be sufficient. However, I would somehow prefer smylinks at the first level below the root directory, but that is of course a little bit more complicated to implement.

The QUADLET_UNIT_DIRS environment variable, which is intended for debugging purpose, would actually fulfill my need, but I don't know how to set the environment variable when quadlet is not executed from command line. Of course, it would be better, if it would not replace the unit search path, only adds additional directories to search for unit files.

xduugu commented 10 months ago

Thanks a lot for the PR, @ygalblum!

containers / podman