Allow privileged ports on WSL #19370

daniel-iwaniec · 2024-08-02T21:13:25Z

Using ports below 1024 should be allowed on WSL in the same way as on Linux.
It should be left for the OS to decide whether it is allowed, respecting Linux Capabilities (e.g. CAP_NET_BIND_SERVICE), etc.

Going through the git history I got the impression, that it was left out for WSL to err on the safe side, but I don't see any explicit reason it should stay. Correct me if I'm wrong, please.

It doesn't fix any known issue as far as I know, I just noticed it myself and decided to create a new PR

linux-foundation-easycla · 2024-08-02T21:13:30Z

The committers listed above are authorized under a signed CLA.

✅ login: daniel-iwaniec / name: Daniel Iwaniec (a032b91, dd51e72)

k8s-ci-robot · 2024-08-02T21:13:33Z

Welcome @daniel-iwaniec!

It looks like this is your first PR to kubernetes/minikube 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/minikube has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-08-02T21:13:34Z

Hi @daniel-iwaniec. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

minikube-bot · 2024-08-02T21:14:02Z

Can one of the admins verify this patch?

daniel-iwaniec · 2024-08-03T06:50:07Z

/sig cli

daniel-iwaniec · 2024-08-03T06:57:47Z

/kind cleanup

daniel-iwaniec · 2024-08-03T07:03:12Z

/assign medyagh

daniel-iwaniec · 2024-08-03T07:08:02Z

I tried to go through the documentation regarding the PR process and did what I could - let me know if there is anything more I can do :)

daniel-iwaniec · 2024-08-03T09:19:51Z

I went through some of the WSL's GitHub issues and:

here some capability not support in setxattr microsoft/WSL#574 you can see that setcap was implemented
here Bind socket < 1023 EACCES (Permission denied) microsoft/WSL#2007 you can see that people were able to use it and bind to ports lower than 1024

daniel-iwaniec · 2024-08-03T20:47:16Z

/sig network

medyagh · 2024-08-03T22:31:02Z

cmd/minikube/cmd/start.go

 	p, err := strconv.Atoi(port)
 	if err != nil {
 		return errors.Errorf("Sorry, one of the ports provided with --ports flag is not valid: %s", port)
 	}
 	if p > 65535 || p < 1 {
 		return errors.Errorf("Sorry, one of the ports provided with --ports flag is outside range: %s", port)
 	}
-	if isHost && detect.IsMicrosoftWSL() && p < 1024 {


why isHost been removed from the logic ? doesnt seem related to this PR

The way I understand it, the current logic is "on WSL (and only on WSL), check if the host port (and only host port) is below 1024", and since the change is to allow these ports on WSL, we don't need any special checks for host ports.
All that is left is port validation that applies to "every kind" of port - no matter if it is a host port or not.

medyagh · 2024-08-03T22:31:37Z

@daniel-iwaniec thank you for this contribution do you mind sharing the Before/After this PR, with an example of using the WSL on windows?

medyagh · 2024-08-03T22:31:46Z

/ok-to-test

minikube-pr-bot · 2024-08-04T02:11:50Z

Here are the number of top 10 failed tests in each environments with lowest flake rate.

Environment	Test Name	Flake Rate
Docker_Linux_containerd_arm64 (2 failed)	TestStartStop/group/old-k8s-version/serial/SecondStart(gopogh)	47.53% (chart)

Besides the following environments also have failed tests:

Docker_Cloud_Shell: 5 failed (gopogh)
Hyperkit_macOS: 11 failed (gopogh)
Docker_Linux_crio_arm64: 2 failed (gopogh)
KVM_Linux_crio: 11 failed (gopogh)
Docker_Linux_crio: 2 failed (gopogh)

To see the flake rates of all tests by environment, click here.

daniel-iwaniec · 2024-08-04T09:26:06Z

Current behavior on WSL

$ uname -a
Linux DESKTOP-2VAN5M9 5.15.153.1-microsoft-standard-WSL2 #1 SMP Fri Mar 29 23:14:13 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

$ minikube version
minikube version: v1.33.1
commit: 5883c09216182566a63dff4c326a6fc9ed2982ff

$ minikube start --ports=80:80
😄  minikube v1.33.1 on Ubuntu 22.04 (amd64)
✨  Automatically selected the docker driver

❌  Exiting due to MK_USAGE: Sorry, you cannot use privileged ports on the host (below 1024): 80

daniel-iwaniec · 2024-08-04T09:56:56Z

Expected behavior

Well, I obtained the output after building version with my changes, hence -dirty etc.

$ uname -a
Linux DESKTOP-2VAN5M9 5.15.153.1-microsoft-standard-WSL2 #1 SMP Fri Mar 29 23:14:13 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

$ minikube version
minikube version: v1.33.1
commit: adc0c841af400141e073e0e45061d84afa6c9617-dirty

$ telnet ::1 80
Trying ::1...
telnet: Unable to connect to remote host: Connection refused

$ minikube start --ports=80:80
😄  minikube v1.33.1 on Ubuntu 22.04 (amd64)
✨  Automatically selected the docker driver. Other choices: none, ssh
📌  Using Docker driver with root privileges
👍  Starting "minikube" primary control-plane node in "minikube" cluster
🚜  Pulling base image v0.0.44-1721902582-19326 ...
💾  Downloading Kubernetes v1.30.3 preload ...
    > preloaded-images-k8s-v18-v1...:  342.95 MiB / 342.95 MiB  100.00% 22.42 M
    > gcr.io/k8s-minikube/kicbase...:  487.17 MiB / 487.18 MiB  100.00% 22.29 M
🔥  Creating docker container (CPUs=2, Memory=2200MB) ...
🐳  Preparing Kubernetes v1.30.3 on Docker 27.1.1 ...
    ▪ Generating certificates and keys ...
    ▪ Booting up control plane ...
    ▪ Configuring RBAC rules ...
🔗  Configuring bridge CNI (Container Networking Interface) ...
🔎  Verifying Kubernetes components...
    ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🌟  Enabled addons: storage-provisioner, default-storageclass
💡  kubectl not found. If you need it, try: 'minikube kubectl -- get pods -A'
🏄  Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default

$ telnet ::1 80
Trying ::1...
Connected to ::1.
Escape character is '^]'.
Connection closed by foreign host.

daniel-iwaniec · 2024-08-04T10:05:09Z

@medyagh I replied to your comment and gave a before/after example
I'm not sure if I should do something about the failed tests or if they are just flaky
Let me know if everything's ok or if there's something else

Anyway, thanks for the quick response :)

medyagh · 2024-08-05T21:49:19Z

thank you @daniel-iwaniec for the before/after do you mind doing an example of this on a on WSL ? I know this PR wont break that I but I am suspecious that the code intended to be a" logical OR" and my be we meant to not allow ports bellow 1024 anywhere...it work on linux ?

please also take a look at the unit test and make sure the unit tests are passing

daniel-iwaniec · 2024-08-06T06:48:14Z

@medyagh these examples are done on WSL already

Take a look at uname -a output, you can see there is 5.15.153.1-microsoft-standard-WSL2 indicating that it is a WSL

minikube-pr-bot · 2024-08-06T07:41:29Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 19370) |
+----------------+----------+---------------------+
| minikube start | 52.8s    | 54.0s               |
| enable ingress | 25.8s    | 27.1s               |
+----------------+----------+---------------------+

Times for minikube (PR 19370) start: 54.1s 52.8s 55.3s 53.3s 54.3s
Times for minikube start: 55.0s 50.0s 52.1s 54.2s 52.5s

Times for minikube ingress: 24.5s 24.2s 28.1s 28.1s 24.2s
Times for minikube (PR 19370) ingress: 26.6s 28.0s 25.0s 29.0s 26.6s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 19370) |
+----------------+----------+---------------------+
| minikube start | 24.1s    | 24.1s               |
| enable ingress | 21.9s    | 22.4s               |
+----------------+----------+---------------------+

Times for minikube start: 24.5s 22.5s 25.3s 24.9s 23.6s
Times for minikube (PR 19370) start: 21.8s 25.2s 25.1s 25.5s 22.8s

Times for minikube ingress: 21.8s 21.3s 22.8s 21.8s 21.8s
Times for minikube (PR 19370) ingress: 22.3s 23.3s 23.8s 21.3s 21.3s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 19370) |
+----------------+----------+---------------------+
| minikube start | 23.7s    | 23.2s               |
| enable ingress | 44.3s    | 44.1s               |
+----------------+----------+---------------------+

Times for minikube start: 24.6s 24.2s 20.8s 24.8s 24.1s
Times for minikube (PR 19370) start: 23.8s 25.2s 21.3s 22.0s 23.9s

Times for minikube ingress: 48.8s 47.9s 48.9s 38.0s 37.8s
Times for minikube (PR 19370) ingress: 46.8s 46.8s 47.8s 31.9s 47.3s

minikube-pr-bot · 2024-08-06T10:04:11Z

Here are the number of top 10 failed tests in each environments with lowest flake rate.

Environment	Test Name	Flake Rate
Docker_Linux (1 failed)	TestFunctional/parallel/MountCmd/specific-port(gopogh)	0.61% (chart)
Docker_Linux_docker_arm64 (1 failed)	TestKubernetesUpgrade(gopogh)	0.62% (chart)

Besides the following environments also have failed tests:

QEMU_macOS: 94 failed (gopogh)
Docker_Linux_crio_arm64: 2 failed (gopogh)
KVM_Linux_crio: 29 failed (gopogh)
Docker_Cloud_Shell: 5 failed (gopogh)
Docker_Linux_crio: 2 failed (gopogh)
Docker_Linux_containerd_arm64: 1 failed (gopogh)
Hyperkit_macOS: 24 failed (gopogh)

To see the flake rates of all tests by environment, click here.

daniel-iwaniec · 2024-08-06T10:14:28Z

Unit tests should also be fixed now, although I'm not sure how to trigger them

medyagh · 2024-08-06T17:14:15Z

ring the Before/After this PR, with an example of using the WSL on windows?

yes I noticed that but I am wondering if this feature works on linux ?

I have a feeling this "if isHost" statement been there for a reason and maybe it was meant to not allow it on Linux either

daniel-iwaniec · 2024-08-06T20:47:30Z

I'm using Minikube like that on Linux all the time for years, without any issues. I only stumbled upon this issue when I started working with developers using WSL, which prompted me to create this PR. So yeah, it works fine on Linux 😄

Also, I already used binary with these changes on a few machines, and it is working fine

medyagh · 2024-08-07T21:37:17Z

cmd/minikube/cmd/start_test.go

-		{
-			isTarget: isMicrosoftWSL,
-			ports:    []string{"1023-1025:8023-8025"},
-			errorMsg: "Sorry, you cannot use privileged ports on the host (below 1024): 1023",


thank you @daniel-iwaniec for fixing the test, the Pr looks good just one last thing, can you plz emulate a secnario that the OS does NOT allow or can not create port bellow 1024 and see if minikube failed gracefuly ?

my fear is that for some new users this will blow with a large stack trace error if their OS does not allow this, it is good that we make sure minikube fails with a Nice one liner error that their OS didnt support it

I would have to really go out of my way to make it not work :)

This option (--ports) applies only to docker and podman

docker and podman use CAP_NET_BIND_SERVICE by default

which means binding ports below 1024 should work by default

In specific case of WSL, I confirmed it works both with

docker engine

docker desktop

which means you could even install docker desktop on windows, then install minikube using WSL, and it still would work

medyagh · 2024-08-08T19:36:16Z

/lgtm

k8s-ci-robot · 2024-08-08T19:36:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daniel-iwaniec, medyagh

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~~ [medyagh]

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

Allow privileged ports on WSL

a032b91

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 2, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 2, 2024

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 2, 2024

k8s-ci-robot requested review from medyagh and prezha August 2, 2024 21:14

k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Aug 3, 2024

daniel-iwaniec closed this Aug 3, 2024

daniel-iwaniec reopened this Aug 3, 2024

k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Aug 3, 2024

k8s-ci-robot assigned medyagh Aug 3, 2024

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Aug 3, 2024

medyagh reviewed Aug 3, 2024

View reviewed changes

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Aug 3, 2024

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 3, 2024

This comment has been minimized.

Sign in to view

daniel-iwaniec requested a review from medyagh August 4, 2024 11:36

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 6, 2024

daniel-iwaniec force-pushed the allow-privileged-ports-on-wsl branch from e0cd207 to 2101c4a Compare August 6, 2024 06:38

Update tests for start.go

dd51e72

daniel-iwaniec force-pushed the allow-privileged-ports-on-wsl branch from 2101c4a to dd51e72 Compare August 6, 2024 06:42

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 6, 2024

medyagh requested changes Aug 7, 2024

View reviewed changes

daniel-iwaniec requested a review from medyagh August 8, 2024 12:26

medyagh merged commit f780911 into kubernetes:master Aug 8, 2024
27 of 40 checks passed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow privileged ports on WSL #19370

Allow privileged ports on WSL #19370

daniel-iwaniec commented Aug 2, 2024

linux-foundation-easycla bot commented Aug 2, 2024 •

edited

Loading

k8s-ci-robot commented Aug 2, 2024

k8s-ci-robot commented Aug 2, 2024

minikube-bot commented Aug 2, 2024

daniel-iwaniec commented Aug 3, 2024

daniel-iwaniec commented Aug 3, 2024

daniel-iwaniec commented Aug 3, 2024

daniel-iwaniec commented Aug 3, 2024

daniel-iwaniec commented Aug 3, 2024

daniel-iwaniec commented Aug 3, 2024

medyagh Aug 3, 2024

daniel-iwaniec Aug 4, 2024 •

edited

Loading

medyagh commented Aug 3, 2024

medyagh commented Aug 3, 2024

This comment has been minimized.

minikube-pr-bot commented Aug 4, 2024

daniel-iwaniec commented Aug 4, 2024

daniel-iwaniec commented Aug 4, 2024 •

edited

Loading

daniel-iwaniec commented Aug 4, 2024

medyagh commented Aug 5, 2024 •

edited

Loading

daniel-iwaniec commented Aug 6, 2024

minikube-pr-bot commented Aug 6, 2024

minikube-pr-bot commented Aug 6, 2024

daniel-iwaniec commented Aug 6, 2024

medyagh commented Aug 6, 2024

daniel-iwaniec commented Aug 6, 2024

medyagh Aug 7, 2024

daniel-iwaniec Aug 8, 2024

medyagh commented Aug 8, 2024

k8s-ci-robot commented Aug 8, 2024

Allow privileged ports on WSL #19370

Allow privileged ports on WSL #19370

Conversation

daniel-iwaniec commented Aug 2, 2024

linux-foundation-easycla bot commented Aug 2, 2024 • edited Loading

k8s-ci-robot commented Aug 2, 2024

k8s-ci-robot commented Aug 2, 2024

minikube-bot commented Aug 2, 2024

daniel-iwaniec commented Aug 3, 2024

daniel-iwaniec commented Aug 3, 2024

daniel-iwaniec commented Aug 3, 2024

daniel-iwaniec commented Aug 3, 2024

daniel-iwaniec commented Aug 3, 2024

daniel-iwaniec commented Aug 3, 2024

medyagh Aug 3, 2024

Choose a reason for hiding this comment

daniel-iwaniec Aug 4, 2024 • edited Loading

Choose a reason for hiding this comment

medyagh commented Aug 3, 2024

medyagh commented Aug 3, 2024

This comment has been minimized.

minikube-pr-bot commented Aug 4, 2024

daniel-iwaniec commented Aug 4, 2024

Current behavior on WSL

daniel-iwaniec commented Aug 4, 2024 • edited Loading

Expected behavior

daniel-iwaniec commented Aug 4, 2024

medyagh commented Aug 5, 2024 • edited Loading

daniel-iwaniec commented Aug 6, 2024

minikube-pr-bot commented Aug 6, 2024

minikube-pr-bot commented Aug 6, 2024

daniel-iwaniec commented Aug 6, 2024

medyagh commented Aug 6, 2024

daniel-iwaniec commented Aug 6, 2024

medyagh Aug 7, 2024

Choose a reason for hiding this comment

daniel-iwaniec Aug 8, 2024

Choose a reason for hiding this comment

medyagh commented Aug 8, 2024

k8s-ci-robot commented Aug 8, 2024

linux-foundation-easycla bot commented Aug 2, 2024 •

edited

Loading

daniel-iwaniec Aug 4, 2024 •

edited

Loading

daniel-iwaniec commented Aug 4, 2024 •

edited

Loading

medyagh commented Aug 5, 2024 •

edited

Loading