Update gvproxy to v0.8.0 and disable ssh port forwarding on wsl #24394

vyasgun · 2024-10-28T14:22:11Z

This fixes #20327.
This change will pass -1 to the -ssh-port flag of gvproxy to disable port forwarding on wsl which will prevent port conflict with crc.

Does this PR introduce a user-facing change?

No

None

baude · 2024-10-28T14:37:02Z

is it possible to add a test to this PR?

baude · 2024-10-28T14:37:06Z

/approve

baude · 2024-10-28T14:37:15Z

lgtm

cfergeau · 2024-10-28T15:28:55Z

is it possible to add a test to this PR?

Are there some 'high level' tests which start podman-machine/gvproxy on Windows/wsl? We could add a test there to check that on the host, nothing listens on port 2222

l0rd · 2024-10-28T16:39:57Z

These are some machine e2e SSH tests here. The README.md file has instructions to run them (on windows too).

lsm5 · 2024-10-29T10:59:52Z

/packit retest-failed

baude · 2024-10-29T12:08:52Z

Are there some 'high level' tests which start podman-machine/gvproxy on Windows/wsl? We could add a test there to check that on the host, nothing listens on port 2222

yes ofc there are!

vyasgun · 2024-10-29T13:15:02Z

Thanks for the reviews! I have added a small modification to the windows init test to ensure port 2222 is open

baude · 2024-11-04T18:49:09Z

This PR has been marked for 5.3 inclusion but it must be merged prior to 11/5 for inclusion in 5.3 RC3. PRs not merged by that date are considered on a case by case basis for backporting.

cfergeau

A few comments, but looks good to me

cfergeau · 2024-11-05T08:25:30Z

pkg/machine/wsl/usermodenet.go

@@ -32,7 +32,7 @@ fi
 if [[ ! $ROUTE =~ default\ via ]]; then
 	exit 3
 fi
-nohup $GVFORWARDER -iface podman-usermode -stop-if-exist ignore -url "stdio:$GVPROXY?listen-stdio=accept" > /var/log/vm.log 2> /var/log/vm.err  < /dev/null &
+nohup $GVFORWARDER -iface podman-usermode -stop-if-exist ignore -url "stdio:$GVPROXY?listen-stdio=accept&ssh-port=-1" > /var/log/vm.log 2> /var/log/vm.err  < /dev/null &


You could add in the commit log that this port forwarding is not needed in the wsl case

Updated the message to:

gvproxy: Disable port-forwarding on WSL This commit disables ssh port forwarding on WSL by passing -1 to the -ssh-port flag of gvproxy. Port forwarding is not required on WSL and disabling it prevents port conflict with CRC. Fixes: https://github.com/containers/podman/issues/20327

cfergeau · 2024-11-05T08:26:08Z

winmake.ps1

@@ -279,7 +279,7 @@ switch ($target) {
        if ($args.Count -gt 1) {
            $ref = $args[1]
        }
-        Win-SSHProxy -Ref $ref
+        Win-SSHProxy($ref)


Not sure what this change does, was this broken before?

yes, the -Ref flag wasn't returning the expected result.

pkg/machine/e2e/config_test.go

cfergeau · 2024-11-05T08:34:00Z

CI reports:

pkg/machine/e2e/init_windows_test.go:49:3: ginkgo-linter: wrong boolean assertion. Consider using `Expect(isSSHPortOpen()).To(BeTrue())` instead (ginkgolinter)
		Expect(isSSHPortOpen()).To(Equal(true))

pkg/machine/e2e/config_test.go

pkg/machine/e2e/init_windows_test.go

Signed-off-by: Gunjan Vyas <vyasgun20@gmail.com>

This commit disables ssh port forwarding on WSL by passing -1 to the -ssh-port flag of gvproxy. Port forwarding is not required on WSL and disabling it prevents port conflict with CRC. Fixes: containers#20327 Signed-off-by: Gunjan Vyas <vyasgun20@gmail.com>

cfergeau · 2024-11-06T11:54:58Z

/lgtm

openshift-ci · 2024-11-06T11:55:01Z

@cfergeau: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

cfergeau · 2024-11-06T11:56:39Z

There is a machine-wsl test failure, is it related to the changes in this PR? https://github.com/containers/podman/pull/24394/checks?check_run_id=32587768711

vyasgun · 2024-11-06T12:47:31Z

@cfergeau it was due to the missing end of line in test/python/requirements.txt. Just fixed it.

Luap99 · 2024-11-06T12:55:33Z

test/python/requirements.txt

This is not correct at all! I don't know why you changed this, none of your chnages have to do anything with the api/python tests so this cannot be related to that at all.

And this is not a regular file, it is a symlink and adding \n just breaks the link as it now points to something non existing.

$ ls -l test/python/requirements.txt lrwxrwxrwx. 1 pholzing pholzing 33 Nov 6 13:52 test/python/requirements.txt -> '../apiv2/python/requirements.txt'$'\n'

I didn't change this - the winmake.ps1 script did when I ran winmake.ps1 lint (which fails on CI otherwise)

winmake.ps1 lint (which fails on CI otherwise)

This scripts is never run in CI AFAICT so I don't see how CI can fail there. Do you have a link of the failure?
If the script is editing totally unrelated things that winmake.ps1 is broken and you should report this as bug. I don't use windows so I cannot test that.

But the fact is this is not a normal file but a symlink and changing the content to add a newline simply breaks the link as it point to a non existing file with a \n ending.

I ran the winmake.ps1 lint on my local machine and it modified the file:

PS C:\Users\gvyas\GolandProjects\podman> git status On branch pr/gvproxy080 Your branch is up to date with 'origin/pr/gvproxy080'. nothing to commit, working tree clean PS C:\Users\gvyas\GolandProjects\podman> .\winmake.ps1 lint golangci-lint run --timeout=10m --build-tags="remote exclude_graphdriver_btrfs btrfs_noversion exclude_graphdriver_devicemapper containers_image_openpgp" C:\Users\gvyas\GolandProjects\podman\cmd\podman pre-commit run --all-files Fix End of Files.........................................................Failed - hook id: end-of-file-fixer - exit code: 1 - files were modified by this hook Fixing test/python/requirements.txt Trim Trailing Whitespace.................................................Passed Mixed line ending........................................................Passed check BOM - deprecated: use fix-byte-order-marker........................Passed Check that executables have shebangs.....................................Passed Check for merge conflicts................................................Passed Check Yaml...............................................................Passed codespell................................................................Passed Exit code = '1' running 'pre-commit run --all-files' at C:\Users\gvyas\GolandProjects\podman\winmake.ps1:188 At C:\Users\gvyas\GolandProjects\podman\contrib\cirrus\win-lib.ps1:80 char:9 + throw "Exit code = '$exitCode' running $command at $($caller. ... + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + CategoryInfo : OperationStopped: (Exit code = '1'...winmake.ps1:188:String) [], RuntimeException + FullyQualifiedErrorId : Exit code = '1' running 'pre-commit run --all-files' at C:\Users\gvyas\GolandProjects\podman\winmake.ps1:18 8 PS C:\Users\gvyas\GolandProjects\podman> git status On branch pr/gvproxy080 Your branch is up to date with 'origin/pr/gvproxy080'. Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory) modified: test/python/requirements.txt no changes added to commit (use "git add" and/or "git commit -a") PS C:\Users\gvyas\GolandProjects\podman> git diff diff --git a/test/python/requirements.txt b/test/python/requirements.txt index 455f09f7d..d5ac3f29d 120000 --- a/test/python/requirements.txt +++ b/test/python/requirements.txt @@ -1 +1 @@ -../apiv2/python/requirements.txt \ No newline at end of file +../apiv2/python/requirements.txt PS C:\Users\gvyas\GolandProjects\podman>

I reverted this change.

@l0rd Can you have a look at what this script does? I guess the problem is that git + windows do not create an actual symlink so the pre-commit considers this a normal file?

@Luap99 Yes, symlinks in Git repositories are problematic on Windows (see this so thread and this pre-commit hook). We are ignoring the ones that are in contrib/systemd/user, we may want to ignore this one too and I have a PR for that.

Apart from that, .\winmake.ps1 lint works fine and is useful for Windows contributors. The fact that's not used in CI is problematic as it get broken with nobody noticing it. The same thing applies to most of the winmake.ps1 targets as in CI we cross-compile rather than compiling on Windows. If we want to maintain these scripts we'd better use them in CI.

@Luap99 A symbolic link created using ln on Linux is not going to be interpreted the same way on Windows after the repo being cloned - on Windows, it will just contain the path to the actual file which is the reason why the pre-commit linter is adding an end of line to it. I fixed it on Windows by running git config --global core.symlinks true

I just have one question - Why is podman using symbolic link instead of using -r prefix to refer to the other requirements.txt file?

Add missing end of line in test/python/requirements.txt Signed-off-by: Gunjan Vyas <vyasgun20@gmail.com>

Signed-off-by: Gunjan Vyas <vyasgun20@gmail.com>

Luap99

/lgtm

openshift-ci · 2024-11-07T12:16:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, Luap99, vyasgun

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~~ [Luap99,baude]

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

openshift-ci bot added the release-note-none label Oct 28, 2024

github-actions bot added the machine label Oct 28, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 28, 2024

cfergeau mentioned this pull request Oct 28, 2024

wsl: usernet: Don't always bind to port 2222 #24070

Closed

vyasgun force-pushed the pr/gvproxy080 branch from 4218495 to 213de16 Compare October 29, 2024 13:14

vyasgun force-pushed the pr/gvproxy080 branch 2 times, most recently from 3645d55 to c9014e8 Compare October 29, 2024 13:44

baude added the 5.3 label Nov 4, 2024

cfergeau reviewed Nov 5, 2024

View reviewed changes

Luap99 reviewed Nov 5, 2024

View reviewed changes

pkg/machine/e2e/config_test.go Outdated Show resolved Hide resolved

pkg/machine/e2e/init_windows_test.go Outdated Show resolved Hide resolved

vyasgun force-pushed the pr/gvproxy080 branch from c9014e8 to cdeb415 Compare November 6, 2024 09:54

build: update gvisor-tap-vsock to 0.8.0

52caa0f

Signed-off-by: Gunjan Vyas <vyasgun20@gmail.com>

vyasgun force-pushed the pr/gvproxy080 branch 4 times, most recently from 7bcc2cf to 2dd5851 Compare November 6, 2024 10:12

vyasgun force-pushed the pr/gvproxy080 branch from 2dd5851 to bda7eb7 Compare November 6, 2024 10:15

vyasgun force-pushed the pr/gvproxy080 branch 3 times, most recently from c96d80b to 0b53f7e Compare November 6, 2024 12:42

Luap99 requested changes Nov 6, 2024

View reviewed changes

vyasgun force-pushed the pr/gvproxy080 branch from 0b53f7e to cd3f7c7 Compare November 6, 2024 13:04

vyasgun added 2 commits November 6, 2024 18:40

winmake.ps1: Fix the syntax of the function call Win-SSHProxy

6cf3cb1

Add missing end of line in test/python/requirements.txt Signed-off-by: Gunjan Vyas <vyasgun20@gmail.com>

wsl-e2e: Add a test to ensure port 2222 is free with usermode networking

4e8d2dd

Signed-off-by: Gunjan Vyas <vyasgun20@gmail.com>

vyasgun force-pushed the pr/gvproxy080 branch from cd3f7c7 to 4e8d2dd Compare November 6, 2024 13:11

Luap99 approved these changes Nov 7, 2024

View reviewed changes

openshift-ci bot assigned Luap99 Nov 7, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2024

openshift-merge-bot bot merged commit cbb5d7f into containers:main Nov 7, 2024
85 checks passed

l0rd mentioned this pull request Nov 14, 2024

Podman 5.3.0 win-sshproxy.tid: The system cannot find the file specified. (install via scoop) #24557

Closed

cfergeau mentioned this pull request Nov 15, 2024

[BUG] crc seems to conflict with Podman on Windows crc-org/crc#3855

Closed

stale-locking-app 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 6, 2025

stale-locking-app bot locked as resolved and limited conversation to collaborators Feb 6, 2025

Update gvproxy to v0.8.0 and disable ssh port forwarding on wsl #24394

Update gvproxy to v0.8.0 and disable ssh port forwarding on wsl #24394

Uh oh!

Conversation

vyasgun commented Oct 28, 2024

Does this PR introduce a user-facing change?

Uh oh!

baude commented Oct 28, 2024

Uh oh!

baude commented Oct 28, 2024

Uh oh!

baude commented Oct 28, 2024

Uh oh!

cfergeau commented Oct 28, 2024

Uh oh!

l0rd commented Oct 28, 2024

Uh oh!

lsm5 commented Oct 29, 2024

Uh oh!

baude commented Oct 29, 2024

Uh oh!

vyasgun commented Oct 29, 2024

Uh oh!

baude commented Nov 4, 2024

Uh oh!

cfergeau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vyasgun Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cfergeau commented Nov 5, 2024

Uh oh!

Uh oh!

Uh oh!

cfergeau commented Nov 6, 2024

Uh oh!

openshift-ci bot commented Nov 6, 2024

Uh oh!

cfergeau commented Nov 6, 2024

Uh oh!

vyasgun commented Nov 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vyasgun Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 7, 2024

Uh oh!

Uh oh!

Uh oh!

vyasgun Nov 6, 2024 •

edited

Loading

vyasgun Nov 7, 2024 •

edited

Loading