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

Fix OS specific url handling for unix:// scheme in transport #373

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

arixmkii
Copy link
Contributor

Fixes #371

Replaces #372

Details why it changed in #371 (comment)

This adds handling for unix:// scheme under Windows with absolute path (containing drive letter). This replicates the one from ssh_forwarder (

path := socketURI.Path
).

Test command:

gvproxy.exe -debug -mtu 1500 -ssh-port 50402 -listen-qemu unix:///C:/Users/Arthu/AppData/Local/Temp/podman/podman-machine-default-gvproxy.sock -forward-sock unix:///C:/Users/Arthu/AppData/Local/Temp/podman/podman-machine-default-api.sock -forward-dest /run/user/1000/podman/podman.sock -forward-user core -forward-identity C:\\Users\\Arthu\\.local\\share\\containers\\podman\\machine\\machine -pid-file C:\\Users\\Arthu\\AppData\\Local\\Temp\\podman\\gvproxy.pid

@arixmkii
Copy link
Contributor Author

@cfergeau could you review this fix? It makes behavior consistent between different sockets (ssh forwarder and vlan transport) of the same scheme unix://

@arixmkii
Copy link
Contributor Author

arixmkii commented Jul 23, 2024

I will check win-sshproxy-tests.

All passed locally

go test -v .\test-win-sshproxy
=== RUN   TestSuite
Running Suite: win-sshproxy suite
=================================
Random Seed: 1721728332
Will run 3 of 3 specs

+++
Ran 3 of 3 Specs in 0.856 seconds
SUCCESS! -- 3 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestSuite (0.86s)
PASS
ok      github.com/containers/gvisor-tap-vsock/test-win-sshproxy        1.122s

@cfergeau
Copy link
Collaborator

I will check win-sshproxy-tests.

It's flakey/racy, sometimes it needs to be run a few times in ghactions until it passes :-/

@cfergeau
Copy link
Collaborator

I will check win-sshproxy-tests.

It's flakey/racy, sometimes it needs to be run a few times in ghactions until it passes :-/

And the wikipedia failure in Go/tests will be fixed as part of #370

@cfergeau
Copy link
Collaborator

@cfergeau could you review this fix? It makes behavior consistent between different sockets (ssh forwarder and vlan transport) of the same scheme unix://

Hopefully there are no Windows users who rely on unix:///Absolute/Path/No/Drive on Windows. Can you add to the commit log what you put in the PR description? ( #373 (comment) )

@cfergeau
Copy link
Collaborator

Apart from this,
/lgtm

This adds handling for "unix://" scheme under Windows with absolute
paths (containing drive letter). This replicates the behavior from
`ssh_forwarder.go` (pkg/sshclient/ssh_forwarder.go#L114).

Test command:
gvproxy.exe -debug -listen-qemu unix:///C:/Users/User/AppData/Local/Temp/podman/gvproxy.sock

Signed-off-by: Arthur Sengileyev <[email protected]>
@openshift-ci openshift-ci bot removed the lgtm label Jul 23, 2024
@arixmkii
Copy link
Contributor Author

arixmkii commented Jul 23, 2024

Forced pushed with updated commit message.

Hopefully there are no Windows users who rely on unix:///Absolute/Path/No/Drive on Windows.

AFAIK there are no users (it used to work with QEMU machine setup, but was never a supported option advertised). I plan to submit issue and PR for exposing Podman API via unix:// socket on Windows for Hyper-V machine (so, one could potentially use automation via shell script + curl tool), but this is only on a concept level right now.

@cfergeau
Copy link
Collaborator

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm label Jul 23, 2024
Copy link
Contributor

openshift-ci bot commented Jul 23, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arixmkii, cfergeau

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-merge-bot openshift-merge-bot bot merged commit 2b36bdd into containers:main Jul 23, 2024
8 of 20 checks passed
@arixmkii arixmkii deleted the fix-qemu-socket branch July 23, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unix socket forwarding issue on Windows platform
2 participants