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

Configuration file errors are hidden #25362

Open
hanwen-flow opened this issue Feb 19, 2025 · 7 comments
Open

Configuration file errors are hidden #25362

hanwen-flow opened this issue Feb 19, 2025 · 7 comments
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@hanwen-flow
Copy link

Issue Description

I was trying to get podman to recognize runc in a custom location, and had written an incorrect toml file.

The diagnostics (logrus.Tracef, logrus.Debugf calls) for this are emitted are sufficient, but as the configuration happens as global initialization, they are suppressed: processing the --log-level flag happens later.

I looked for an env var to control this, but couldn't find one in the manpage.

I finally resolved it by recompiling podman and inserting a logrus.SetLevel() call in vendor/github.com/containers/common/pkg/config/new.go

Steps to reproduce the issue

  1. write invalid configuration (eg. non-existent keys) in /usr/share/containers/containers.conf
  2. run podman --log-level=trace info

Describe the results you received

observe that nothing is printed about invalid keys:

#  go build -o bin/podman -tags "apparmor   systemd exclude_graphdriver_devicemapper seccomp "  ./cmd/podman && ./bin/podman --log-level=trace info
INFO[0000] ./bin/podman filtering at log level trace

Describe the results you expected

I wanted to see some of the diagnostic such as

TRAC[0000] Reading configuration file "/usr/share/containers/containers.conf" 
DEBU[0000] Failed to decode the keys ["runtime"] from "/usr/share/containers/containers.conf". 

podman info output

# git rev-parse HEAD
aafc3739fbeab6afb192723f7be3ef537af10eb3
#  go build -o bin/podman -tags "apparmor   systemd exclude_graphdriver_devicemapper seccomp "  ./cmd/podman && ./bin/podman info
host:
  arch: amd64
  buildahVersion: 1.39.0
  cgroupControllers:
  - cpuset
  - cpu
  - io
  - memory
  - hugetlb
  - pids
  - rdma
  - misc
  cgroupManager: systemd
  cgroupVersion: v2
  conmon:
    package: Unknown
    path: /usr/local/libexec/podman/conmon
    version: 'conmon version 2.1.12, commit: unknown'
  cpuUtilization:
    idlePercent: 99.69
    systemPercent: 0.05
    userPercent: 0.26
  cpus: 2
  databaseBackend: sqlite
  distribution:
    codename: bookworm
    distribution: debian
    version: "12"
  eventLogger: journald
  freeLocks: 2047
  hostname: ip-172-31-7-139
  idMappings:
    gidmap: null
    uidmap: null
  kernel: 6.1.0-31-cloud-amd64
  linkmode: dynamic
  logDriver: journald
  memFree: 912863232
  memTotal: 4115464192
  networkBackend: netavark
  networkBackendInfo:
    backend: netavark
    dns:
      package: Unknown
      path: /usr/local/libexec/podman/aardvark-dns
      version: aardvark-dns 1.14.0
    package: Unknown
    path: /usr/local/libexec/podman/netavark
    version: netavark 1.14.0
  ociRuntime:
    name: runc
    package: Unknown
    path: /usr/local/libexec/podman/runc
    version: |-
      runc version 1.2.4
      spec: 1.2.0
      go: go1.23.6
      libseccomp: 2.5.4
  os: linux
  pasta:
    executable: ""
    package: ""
    version: ""
  remoteSocket:
    exists: true
    path: /run/podman/podman.sock
  rootlessNetworkCmd: pasta
  security:
    apparmorEnabled: true
    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: false
    seccompEnabled: true
    seccompProfilePath: ""
    selinuxEnabled: false
  serviceIsRemote: false
  slirp4netns:
    executable: ""
    package: ""
    version: ""
  swapFree: 0
  swapTotal: 0
  uptime: 23h 3m 13.00s (Approximately 0.96 days)
  variant: ""
plugins:
  authorization: null
  log:
  - k8s-file
  - none
  - passthrough
  - journald
  network:
  - bridge
  - macvlan
  - ipvlan
  volume:
  - local
registries: {}
store:
  configFile: /usr/share/containers/storage.conf
  containerStore:
    number: 1
    paused: 0
    running: 1
    stopped: 0
  graphDriverName: overlay
  graphOptions: {}
  graphRoot: /var/lib/containers/storage
  graphRootAllocated: 16665681920
  graphRootUsed: 6612668416
  graphStatus:
    Backing Filesystem: extfs
    Native Overlay Diff: "true"
    Supports d_type: "true"
    Supports shifting: "true"
    Supports volatile: "true"
    Using metacopy: "false"
  imageCopyTmpDir: /var/tmp
  imageStore:
    number: 1
  runRoot: /run/containers/storage
  transientStore: false
  volumePath: /var/lib/containers/storage/volumes
version:
  APIVersion: 5.5.0-dev
  Built: 0
  BuiltTime: Thu Jan  1 00:00:00 1970
  GitCommit: ""
  GoVersion: go1.23.6
  Os: linux
  OsArch: linux/amd64
  Version: 5.5.0-dev

Podman in a container

No

Privileged Or Rootless

None

Upstream Latest Release

Yes

Additional environment details

Additional environment details

Additional information

No response

@hanwen-flow hanwen-flow added the kind/bug Categorizes issue or PR as related to a bug. label Feb 19, 2025
@Luap99
Copy link
Member

Luap99 commented Feb 19, 2025

This is not an easy fix, generally speaking we abuse init() functions way to much and do to much work there. And we cannot parse the arguments earlier.

But even if we would not use init() we still have a bit of a catch-22 between config parsing and cli parsing. The config contains options that influence the cli setup so we cannot parse the cli before the config. That means we cannot use the log-level argument to set the level during config parsing as that happened before.

So I don't see how we would go about fixing that without introducing an even bigger mess.

@hanwen-flow
Copy link
Author

yes, IMO the init functions should not be used at all.

Would it be possible to introduce a CONTAINER_LOGLEVEL env var that if set would call logrus.SetLevel from an init function somewhere deeply in the stack? It's not pretty, but would get the job done.

@Luap99
Copy link
Member

Luap99 commented Feb 19, 2025

A env var is an option, but then we still have to ensure somehow it is parsed first (easy enough if we just consider the config parsing code but not so much if we consider all imported packages). I think we still would need to fix the init() issues IMO so we have a sane order.

Then there is the question if we actually want to commit to such an env var and maintain that in the log term, so far there never really was a need for it. And if we just do it as work around and then have different behavior between env and cli flag I think that makes things objectively worse for users to understand.

@hanwen-flow
Copy link
Author

I think we still would need to fix the init() issues IMO so we have a sane order.

Is there an issue somewhere that gives an overview of what the problems are?

@Luap99
Copy link
Member

Luap99 commented Feb 20, 2025

No, I don't have a issue but we have little to no control over the order of the imports and in what order init() runs. As such reading the env in one of them is to some extend undefined as the part before and after behave differently, the init() function that we run before still cannot log correctly.

The proper fix is of course to not abuse init() and where it is needed or makes sense make sure to never log from that context.

@hanwen-flow
Copy link
Author

I don't mean the general problems of using init, but which podman dependencies (ab)use init, and where one could start to unravel the ball of yarn.

@Luap99
Copy link
Member

Luap99 commented Feb 20, 2025

The big thing are all the cobra command definitions in cmd/podman/... that setup the commands and flags. They in turn the call out to a bunch of other function, i.e. ones that trigger the config parsing and so on. I am not sure about other places, I fixed a few in the past.

That said even if all the of the command/flag setup is moved to be called from main() there is still the problem that the flag setup needs the config values so we must read the file before we parsed --log-level so we don't gain that much. But adding and env would be a lot more simple then.

I guess there is always the cheap hack, the current version uses special "remote" and "module" parsing in cmd/podman/registry to pass the option early, in theory one could try to do for log-level. But I really really hate this approach, it just feels so wrong to parse the same arguments several times to work around our bad code structure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

2 participants