Open dmotte opened 2 months ago
dmotte
commented
2 months ago Update: I have a possible hint on what's going on here: IMHO, it could be possible that, for some reason, -p53:53/udp (without the space) is interpreted as -p -5 -3 -: -5 -3 -/ -u -d -p, which contains -d, while -p 53:53/udp (with the space) couldn't be interpreted like that, because it's composed of two separate arguments.
Anyway, I checked also with --publish=53:53/udp and this last one works fine too.
Luap99
commented
2 months ago Note that podman generate is deprecated in favour of quadlet so it is unlikely that we will fix it. Also note that quadlet was only added v4.4 so it is not supported by your older version. However if somebody else want to submit a PR to fix it we will consider it
eriksjolund
commented
2 months ago This issue reminds me of an old issue
in the sense that both issues concern option command-line parsing for the command podman generate systemd
(I'm don't know if the actual bugs causing the issues are related though)
eriksjolund
commented
2 months ago Regarding
Describe the results you expected The generated files should be equivalent.
~I would have expected the podman generate systemd ... command to fail when given the option
-p53:53/udp~
~Shouldn't there always be a space between a short option and its value?~
I tried both the commands
podman run -p8080:80 -ti docker.io/library/nginx
docker run -p8080:80 -ti docker.io/library/nginx
and noticed that both commands succeeded. I am bit surprised that the commands didn't fail, because I thought there always needs to be a space between a short option and its value. Aparantly it's not needed when using Podman and Docker. I learned something new today.
Luap99
commented
2 months ago -p53:53/udp -p 53:53/udp and -p=53:53/udp should all be equivalent.
The issue is that podman generate systemd depends on the cli and re parses that the cli to extract certain options because we must modify it slightly. However the issues results here because we parse it with a incomplete flag definition and without strict error enforcing. The parser simply has no idea what do to as it missing a lot of context.
Something like -it can mean -i -t but it could also mean -i with the value t, the only reason that works is because the flag are defined normaly so the parser knowns that both are bool options and do not take an arg.
In the generate systemd case -p is not defined as such the parser has no idea that the option accepts and arg and it seems it defaults to an bool so it seems to assume each char is a bool option which then results in d in udp being assumed as flag not a value.
Given we we recommend quadlet now (bugs like these are why quadlet is much nicer) and the fact that there is a simple workaround (never use the short form syntax without space) I think a wontfix is appropriate but I am also fine to keep it open in case someone else wants toi contribuate a simple bug fix, however this is no way limted to -p, and other shorthand flag can trigger the same thing, e.g. -unobody
dmotte
commented
2 months ago @eriksjolund thanks for sharing!
Well, in fact, if it's a short option (single letter, such as -p), usually you can also write it as a single argument (without the space) and it should work :) This behavior comes from the getopt function (POSIX standard). See e.g. https://superuser.com/questions/1652953/linux-program-argument-with-vs-without-space
Luap99
commented
2 months ago @eriksjolund thanks for sharing!
Well, in fact, if it's a short option (single letter, such as
-p), usually you can also write it as a single argument (without the space) and it should work :) This behavior comes from thegetoptfunction (POSIX standard). See e.g. https://superuser.com/questions/1652953/linux-program-argument-with-vs-without-space
Yes this syntax is supported by all the applications using getopt, the -p=53:53/udp however is not supported by getopt and a special extension by go parser we use https://github.com/spf13/pflag/
github-actions[bot]
commented
1 month ago A friendly reminder that this issue had no activity for 30 days.
Issue Description
When I create a container with
podman create ... -p53:53/udp ...and then I runpodman generate systemd ..., the-dflag is not put in the generated file. But if I usepodman create ... -p 53:53/udp ...(note the additional space between-pand53) instead, then the-dflag is there.As a side effect, this issue also causes a problem with logs when running a container as a
systemdservice unit: each log line is written twice injournald: the first time by thepodman run ...command's standard output (stdout) and the second time by thejournaldlog driver.For more details and a better explanation of the problem, see the steps below to reproduce the issue.
Steps to reproduce the issue
podman rm -f coredns; podman create --name=coredns -p53:53/udp docker.io/coredns/coredns; podman generate systemd -n --new coredns > coredns-01.ini(UDP port without the space)podman rm -f coredns; podman create --name=coredns -p 53:53/udp docker.io/coredns/coredns; podman generate systemd -n --new coredns > coredns-02.ini(UDP port with the space)Describe the results you received
The generated files look like this:
coredns-01.ini:coredns-02.ini:Differences (with
diff coredns-01.ini coredns-02.ini):As you can see, the
-dflag is present only incoredns-02.ini, even if the two generated should be equivalentDescribe the results you expected
The generated files should be equivalent.
podman info output
Podman in a container
No
Privileged Or Rootless
Rootless
Upstream Latest Release
No
Additional environment details
I'm running Debian 12 (bookworm) and I installed Podman using
apt-get update; apt-get install -y podman podman-composeMore specifically, I'm running a https://github.com/dmotte/vagrant-podmanbox Vagrant box (Virtual Machine), version
v2023.07.18.2319, but it doesn't matter. It's still the official Podman version from the official Debian 12aptrepos, and it's up-to-date. See thedpkg -s podmanoutput belowAdditional information
Even if I didn't test this issue with the latest Podman release, I think the problem could be around here:
https://github.com/containers/podman/blob/814b7b003cc630bf6ab188274706c383f9fb9915/pkg/systemd/generate/containers.go#L410
And this part didn't change, so I suspect it's still present in the latest version.