search

containers / podman

Podman: A tool for managing OCI containers and pods.
https://podman.io
Apache License 2.0
22.4k stars 2.31k forks source link

podman: check for nil pointers in remote flags #22997

Closed jeckersb closed 2 weeks ago

jeckersb commented 3 weeks ago

I'm hitting url being nil when I have CONTAINER_CONNECTION=podman-machine-default-root, but the machine does not exist.

Fix nil pointer check in remote client flags

Signed-off-by: John Eckersberg jeckersb@redhat.com

jeckersb commented 3 weeks ago

I see it's failing due to missing tests, I've not used bats before so I'll look into adding that

mheon commented 3 weeks ago

LGTM once you add a test

baude commented 3 weeks ago

code lgtm; please add test and tighten up the commit message. You also need a release note. ill see if i can add that.

jeckersb commented 3 weeks ago

I've added a test but I'm having issues getting hack/bats --remote to work locally in order to test it, so throwing it at CI to see if it sticks.

packit-as-a-service[bot] commented 3 weeks ago

Ephemeral COPR build failed. @containers/packit-build please check.

packit-as-a-service[bot] commented 3 weeks ago

Ephemeral COPR build failed. @containers/packit-build please check.

mheon commented 2 weeks ago

Rawhide validate is probably a flake

baude commented 2 weeks ago

main is now fixed, please rebase and should be good to go!

baude commented 2 weeks ago

/approve

openshift-ci[bot] commented 2 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, jeckersb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/containers/podman/blob/main/OWNERS)~~ [baude] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
Luap99 commented 2 weeks ago

Your test passes both locally for me on current main nor am I able to reproduce the nil panic manually. The flag is always defined so I fail to see hot it can ever be nil in such case. As such I don't understand what this patch is supposed to fix.

Can you provide a full reproducer and the panic stack trace please?

Luap99 commented 2 weeks ago

Ok I understand the issue now, your fix is not realy correct. you fix the nil panic but not the underlying bug. The problem is that setupRemoteConnection() errors on the invlaid connection and then we just ignore said error and not define any flags on the root command

$ CONTAINER_CONNECTION=123 bin/podman --connection test  ps
Error: unknown flag: --connection
$ bin/podman --connection test  ps
Error: read cli flags: connection "test" not found

https://github.com/containers/podman/blob/afe55cded062ab0c56f57e99002686862f3327c9/cmd/podman/root.go#L466-L468

This fix is not so straight forward as we must allow CONTAINER_CONNECTION=invalid podman --connection valid ps and in case of an invalid connection still return a error saying so.

With this path a user gets unable to create connection. "" is not a supported schema: which does not make sense.

jeckersb commented 2 weeks ago

Your test passes both locally for me on current main nor am I able to reproduce the nil panic manually. The flag is always defined so I fail to see hot it can ever be nil in such case. As such I don't understand what this patch is supposed to fix. Can you provide a full reproducer and the panic stack trace please?

Oh I see CONTAINER_CONNECTION reproduces but CONTAINER_HOST does not as such your test is a NOP

Please make sure to have a test with CONTAINER_CONNECTION

Good catch, that was totally a copy/paste error on my part.

jeckersb commented 2 weeks ago

@Luap99 thanks for the pointers. Looking into this a bit deeper and trying to wrap my head around the order in which things are happening...

https://github.com/containers/podman/blob/b8d95a5893572b37c8257407e964ad06ba87ade6/cmd/podman/root.go#L465-L469

setupRemoteConnection gets called early, before the flags are even defined. And then...

https://github.com/containers/podman/blob/b8d95a5893572b37c8257407e964ad06ba87ade6/cmd/podman/root.go#L182-L188

Which the comment seems to not match reality, since this only considers the environment variables (or the default connection if no environment variables are set)

Should the logic from setupRemoteConnection just be removed entirely, and merged into readRemoteCliFlags (I don't follow why there's two places that deal with the remote config, although maybe there's a good reason that I'm missing)? The latter is called from persistentPreRunE which if I'm following the order of operations properly should have all of the necessary context from the --connection flag. Then in readRemoteCliFlags the logic just becomes:

cgwalters commented 2 weeks ago

Maybe just read the environment variables and have them become the defaults we pass to the cobra API when defining the CLI args?

Luap99 commented 2 weeks ago

Should the logic from setupRemoteConnection just be removed entirely, and merged into readRemoteCliFlags (I don't follow why there's two places that deal with the remote config, although maybe there's a good reason that I'm missing)? The latter is called from persistentPreRunE which if I'm following the order of operations properly should have all of the necessary context from the --connection flag. Then in readRemoteCliFlags the logic just becomes:

That is possible but in practice it means that podman --help will no longer show the default url on --url for example which is why it was done like this in the first place.

Maybe just read the environment variables and have them become the defaults we pass to the cobra API when defining the CLI args?

It is not that simple because we have to keep the exact ordering for defaults -> env var -> cli option priority. Doing this would merge default and env vars so it would not be possible to know what was actually set. Ok there is the option to parse twice but I really like to avoid the overhead of doing that which why it is like this today https://github.com/containers/podman/pull/19997

I am happy to take this over, I assume I have the most understanding of what should be done. My idea would be to store the conf.GetConnection() error in setupRemoteConnection() and then return it at the end of readRemoteCliFlags(), i.e. when no flags where set. And of course ensure setupRemoteConnection() never returns an error.

jeckersb commented 2 weeks ago

I am happy to take this over, I assume I have the most understanding of what should be done. My idea would be to store the conf.GetConnection() error in setupRemoteConnection() and then return it at the end of readRemoteCliFlags(), i.e. when no flags where set. And of course ensure setupRemoteConnection() never returns an error.

I am quite happy to defer to you if you want to run with it :smile:

Luap99 commented 2 weeks ago

Done, https://github.com/containers/podman/pull/23066