daniel-iwaniec commented 1 month ago

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[bot] commented 1 month ago

The committers listed above are authorized under a signed CLA.

:white_check_mark: login: daniel-iwaniec / name: Daniel Iwaniec (a032b916dd6db54479be394a38e76ef81c36d38d, dd51e72d60a15da3a1a4a8c267729efa6313a896)

k8s-ci-robot commented 1 month ago

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. :smiley:

k8s-ci-robot commented 1 month ago

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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.

minikube-bot commented 1 month ago

Can one of the admins verify this patch?

daniel-iwaniec commented 1 month ago

/sig cli

daniel-iwaniec commented 1 month ago

/kind cleanup

daniel-iwaniec commented 1 month ago

/assign medyagh

daniel-iwaniec commented 1 month ago

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 commented 1 month ago

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

here https://github.com/microsoft/WSL/issues/574 you can see that setcap was implemented
here https://github.com/microsoft/WSL/issues/2007 you can see that people were able to use it and bind to ports lower than 1024

daniel-iwaniec commented 1 month ago

/sig network

medyagh commented 1 month ago

@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 commented 1 month ago

/ok-to-test

minikube-pr-bot commented 1 month ago

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 19370) |
+----------------+----------+---------------------+
| minikube start | 48.5s    | 50.6s               |
| enable ingress | 23.6s    | 24.9s               |
+----------------+----------+---------------------+

Times for minikube start: 47.2s 45.8s 49.9s 49.2s 50.2s Times for minikube (PR 19370) start: 50.7s 48.1s 50.1s 53.4s 50.6s Times for minikube ingress: 23.9s 23.4s 23.4s 23.9s 23.4s Times for minikube (PR 19370) ingress: 27.5s 23.9s 23.9s 24.9s 24.4s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 19370) |
+----------------+----------+---------------------+
| minikube start | 22.1s    | 24.3s               |
| enable ingress | 21.6s    | 21.7s               |
+----------------+----------+---------------------+

Times for minikube start: 21.2s 22.4s 21.2s 23.8s 22.0s Times for minikube (PR 19370) start: 25.1s 23.9s 24.0s 24.0s 24.6s Times for minikube ingress: 21.8s 22.3s 21.8s 21.3s 20.8s Times for minikube (PR 19370) ingress: 20.8s 21.8s 21.3s 22.8s 21.8s

docker driver with containerd runtime

+-------------------+----------+---------------------+
|      COMMAND      | MINIKUBE | MINIKUBE (PR 19370) |
+-------------------+----------+---------------------+
| minikube start    | 20.9s    | 21.4s               |
| ⚠️  enable ingress | 38.3s    | 45.0s ⚠️             |
+-------------------+----------+---------------------+

Times for minikube start: 20.0s 19.4s 20.6s 24.3s 20.3s Times for minikube (PR 19370) start: 23.2s 23.4s 20.2s 19.8s 20.2s Times for minikube ingress: 32.3s 31.8s 39.3s 40.3s 47.8s Times for minikube (PR 19370) ingress: 47.8s 47.8s 31.8s 48.3s 49.3s

minikube-pr-bot commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

medyagh commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

/lgtm

k8s-ci-robot commented 1 month ago

[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](https://github.com/kubernetes/minikube/blob/master/OWNERS)~~ [medyagh] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

kubernetes / minikube

Allow privileged ports on WSL #19370

Current behavior on WSL

Expected behavior