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

Use url with scheme and path for the unix address #19917

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

afbjorklund
Copy link
Contributor

@afbjorklund afbjorklund commented Sep 11, 2023

Shortcuts like unix:path and unix:/path do not work everywhere,
so make sure to use unix://path when quoting the url (or address)

This avoids errors such as: (when trying to use it as the DOCKER_HOST)
error during connect: Get "http://unix:2375/run/user/1000/podman/podman.sock/v1.24/version": dial tcp: lookup unix: no such host

Does this PR introduce a user-facing change?

None

Help before:

      --url string                URL to access Podman service (CONTAINER_HOST) (default "unix:/run/user/1000/podman/podman.sock")

Help after:

      --url string                URL to access Podman service (CONTAINER_HOST) (default "unix:///run/user/1000/podman/podman.sock")

Shortcuts like unix:path and unix:/path do not work everywhere,
so make sure to use unix://path when quoting the url (or address)

Signed-off-by: Anders F Björklund <[email protected]>
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that using unix:// is better and correct for the code and docs but I think at least for backwards compatibility sake we should make sure we still have a test which makes sure unix: works as well for the service and remote client.

@afbjorklund
Copy link
Contributor Author

afbjorklund commented Sep 11, 2023

I didn't add any "deprecation warnings" or anything, because then it would there for years (like in kubernetes). I think that podman-remote was fine with either, the problem was when using docker. Adding a bats test sounds reasonable.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund, Luap99

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 11, 2023
@baude
Copy link
Member

baude commented Sep 11, 2023

im willing to merge once tests are passing, thanks @afbjorklund

@TomSweeneyRedHat
Copy link
Member

LGTM once the tests are hip.
I think we may have a test system issue. I'm seeing similar failures in other PRs.

@afbjorklund
Copy link
Contributor Author

One strange thing about the default address is that the error message talks about the "connections":

Cannot connect to Podman. Please verify your connection to the Linux system using podman system connection list, or try podman machine init and podman machine start to manage a new Linux VM

But yet there is no mention about the default connection, in the podman system connection list?

Name        URI         Identity    Default

It does mention the file-not-found error, but that only comes on the second line and is mixed with an imaginary http:// scheme so it is hard to know what to use for CONTAINER_CONNECTION or CONTAINER_HOST

Error: unable to connect to Podman socket: Get "http://d/v4.6.0/libpod/_ping": dial unix ///run/user/1000/podman/podman.sock: connect: no such file or directory

The user has to know that there is a colon involved in urls, or check the --help for the addresses.

export CONTAINER_HOST="unix:/run/user/1000/podman/podman.sock"
export CONTAINER_HOST="unix:/run/podman/podman.sock"

This is different from the docker client output, when running docker context ls:

NAME         DESCRIPTION                               DOCKER ENDPOINT                     ERROR
default      Current DOCKER_HOST based configuration   unix:///var/run/docker.sock         
rootless *   Rootless mode                             unix:///run/user/1000/docker.sock   

@mheon
Copy link
Member

mheon commented Sep 11, 2023

Test failure looks legitimate:

# #|     FAIL: RemoteSocket.Path using unix:
         # #| expected: '/tmp/podman_bats.Usi7rb/myunix.sock'
         # #|   actual: 'unix:/tmp/podman_bats.Usi7rb/myunix.sock'

@afbjorklund
Copy link
Contributor Author

afbjorklund commented Sep 12, 2023

So yet another regression, will have to see where this one comes from

podman3 had:
/run/user/1000/podman/podman.sock

podman4 says:
unix:///run/user/1000/podman/podman.sock

But none of them is showing the mix between the two, like in the test fail.

                uri := url.URL{
                        Scheme: "unix",
                        Path:   filepath.Join(xdg, "podman", "podman.sock"),
                }
                ic.Libpod.SetRemoteURI(uri.String())
                info.Host.RemoteSocket.Path = uri.Path

EDIT: podman-remote uses a different code path from podman

        info.Host.RemoteSocket = &define.RemoteSocket{Path: ic.Libpod.RemoteURI()}

@afbjorklund
Copy link
Contributor Author

afbjorklund commented Sep 12, 2023

This got broken in

Which changed the test, rather than report a path...

So now Path contains an Url, including the scheme.

Sometimes. Depending on which program you run.


$ ./bin/podman info --format '{{.Host.RemoteSocket.Path}}'
/run/user/1000/podman/podman.sock
$ ./bin/podman-remote info --format '{{.Host.RemoteSocket.Path}}'
unix:///run/user/1000/podman/podman.sock

If you start it with a string, it will just echo that back.

EDIT:
my terminology was off, it's part of "authority" not "scheme"

That is the scheme is unix. And unix:/some/path is still legal.

https://en.wikipedia.org/wiki/URL#syntax

URI = scheme ":" ["//" authority] path ["?" query] ["#" fragment]

It is perfectly valid to have only scheme and path (no "authority"),
but unfortunately it doesn't work with external clients like Docker.

Signed-off-by: Anders F Björklund <[email protected]>
@afbjorklund
Copy link
Contributor Author

afbjorklund commented Sep 12, 2023

Side note: This was already happening in podman machine, by hardcoding the unix:// everywhere.

        setDockerHost := `export DOCKER_HOST="unix://$(podman info -f "{{.Host.RemoteSocket.Path}}")"
`
                        fmtString = `You can %sconnect Docker API clients by setting DOCKER_HOST using the
following command in your terminal session:

        export DOCKER_HOST='unix://%s'

`

                        fmt.Printf(fmtString, stillString, forwardSock)

Or in the other documentation, that was constructing the remote docker socket address from scratch.

$ export DOCKER_HOST=unix://$XDG_RUNTIME_DIR/podman/podman.sock
$ docker-compose up
$ export DOCKER_HOST=unix://$XDG_RUNTIME_DIR/podman/podman.sock
$ docker-compose up

@afbjorklund
Copy link
Contributor Author

afbjorklund commented Sep 16, 2023

Unfortunately the broken URL "unix://run" appears both in documentation and in upstream library...

docs/source/markdown/podman-remote.1.md
docs/source/markdown/podman.1.md
pkg/bindings/README.md
vendor/github.com/containers/common/pkg/config/containers.conf

It needs to be fixed as well, it is supposed to be "unix:///run" (or unix:/run, but) and not relative path.

i.e. it only works if you do cd / before the command...

Cannot connect to the Docker daemon at unix://run/user/1000/podman/podman.sock. Is the docker daemon running?


EDIT:

@rhatdan
Copy link
Member

rhatdan commented Sep 18, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2023
@rhatdan
Copy link
Member

rhatdan commented Sep 18, 2023

Thanks @afbjorklund

@openshift-merge-robot openshift-merge-robot merged commit 5be2357 into containers:main Sep 18, 2023
@xduugu
Copy link
Contributor

xduugu commented Nov 7, 2023

I don't know your plans for the 4.8 / 5.0 release, but would it be possible to apply this fix to the 4.7 branch?

I have run into this issue when I switched from docker-compose 1.29 to docker-compose v2, which apparently requires the unix:// prefix. For now, I worked around this issue by manually setting DOCKER_HOST to the correct address, but this goes against the intention of podman compose.

@rhatdan
Copy link
Member

rhatdan commented Nov 8, 2023

4.8 should be released before the holidays.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Feb 7, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants