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.

Sign up for GitHub

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

Allow privileged ports on WSL #19370

Merged
merged 2 commits into from
Aug 8, 2024
Merged

Allow privileged ports on WSL #19370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a032b91
Allow privileged ports on WSL
daniel-iwaniec Aug 2, 2024
dd51e72
Update tests for start.go
daniel-iwaniec Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions cmd/minikube/cmd/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1365,7 +1365,7 @@ func validateFlags(cmd *cobra.Command, drvName string) { //nolint:gocyclo
validateInsecureRegistry()
}

// validatePorts validates that the --ports are not below 1024 for the host and not outside range
// validatePorts validates that the --ports are not outside range
func validatePorts(ports []string) error {
var exposedPorts, hostPorts, portSpecs []string
for _, p := range ports {
Expand All @@ -1386,29 +1386,26 @@ func validatePorts(ports []string) error {
}
}
for _, p := range exposedPorts {
if err := validatePort(p, false); err != nil {
if err := validatePort(p); err != nil {
return err
}
}
for _, p := range hostPorts {
if err := validatePort(p, true); err != nil {
if err := validatePort(p); err != nil {
return err
}
}
return nil
}

func validatePort(port string, isHost bool) error {
func validatePort(port string) error {
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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@daniel-iwaniec daniel-iwaniec Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

return errors.Errorf("Sorry, you cannot use privileged ports on the host (below 1024): %s", port)
}
return nil
}

Expand Down
74 changes: 0 additions & 74 deletions cmd/minikube/cmd/start_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (
cfg "k8s.io/minikube/pkg/minikube/config"
"k8s.io/minikube/pkg/minikube/constants"
"k8s.io/minikube/pkg/minikube/cruntime"
"k8s.io/minikube/pkg/minikube/detect"
"k8s.io/minikube/pkg/minikube/driver"
"k8s.io/minikube/pkg/minikube/proxy"
)
Expand Down Expand Up @@ -581,171 +580,98 @@ func TestIsTwoDigitSemver(t *testing.T) {
}

func TestValidatePorts(t *testing.T) {
isMicrosoftWSL := detect.IsMicrosoftWSL()
type portTest struct {
// isTarget indicates whether or not the test case is covered
// because validatePorts behaves differently depending on whether process is running in WSL in windows or not.
isTarget bool
ports []string
errorMsg string
}
var tests = []portTest{
{
isTarget: true,
ports: []string{"8080:80"},
errorMsg: "",
},
{
isTarget: true,
ports: []string{"8080:80/tcp", "8080:80/udp"},
errorMsg: "",
},
{
isTarget: true,
ports: []string{"test:8080"},
errorMsg: "Sorry, one of the ports provided with --ports flag is not valid [test:8080] (Invalid hostPort: test)",
},
{
isTarget: true,
ports: []string{"0:80"},
errorMsg: "Sorry, one of the ports provided with --ports flag is outside range: 0",
},
{
isTarget: true,
ports: []string{"0:80/tcp"},
errorMsg: "Sorry, one of the ports provided with --ports flag is outside range: 0",
},
{
isTarget: true,
ports: []string{"65536:80/udp"},
errorMsg: "Sorry, one of the ports provided with --ports flag is not valid [65536:80/udp] (Invalid hostPort: 65536)",
},
{
isTarget: true,
ports: []string{"0-1:80-81/tcp"},
errorMsg: "Sorry, one of the ports provided with --ports flag is outside range: 0",
},
{
isTarget: true,
ports: []string{"0-1:80-81/udp"},
errorMsg: "Sorry, one of the ports provided with --ports flag is outside range: 0",
},
{
isTarget: !isMicrosoftWSL,
ports: []string{"80:80", "1023-1025:8023-8025", "1023-1025:8023-8025/tcp", "1023-1025:8023-8025/udp"},
errorMsg: "",
},
{
isTarget: isMicrosoftWSL,
ports: []string{"80:80"},
errorMsg: "Sorry, you cannot use privileged ports on the host (below 1024): 80",
},
{
isTarget: isMicrosoftWSL,
ports: []string{"1023-1025:8023-8025"},
errorMsg: "Sorry, you cannot use privileged ports on the host (below 1024): 1023",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

},
{
isTarget: isMicrosoftWSL,
ports: []string{"1023-1025:8023-8025/tcp"},
errorMsg: "Sorry, you cannot use privileged ports on the host (below 1024): 1023",
},
{
isTarget: isMicrosoftWSL,
ports: []string{"1023-1025:8023-8025/udp"},
errorMsg: "Sorry, you cannot use privileged ports on the host (below 1024): 1023",
},
{
isTarget: true,
ports: []string{"127.0.0.1:8080:80", "127.0.0.1:8081:80/tcp", "127.0.0.1:8081:80/udp", "127.0.0.1:8082-8083:8082-8083/tcp"},
errorMsg: "",
},
{
isTarget: true,
ports: []string{"1000.0.0.1:80:80"},
errorMsg: "Sorry, one of the ports provided with --ports flag is not valid [1000.0.0.1:80:80] (Invalid ip address: 1000.0.0.1)",
},
{
isTarget: !isMicrosoftWSL,
ports: []string{"127.0.0.1:80:80", "127.0.0.1:81:81/tcp", "127.0.0.1:81:81/udp", "127.0.0.1:82-83:82-83/tcp", "127.0.0.1:82-83:82-83/udp"},
errorMsg: "",
},
{
isTarget: isMicrosoftWSL,
ports: []string{"127.0.0.1:80:80"},
errorMsg: "Sorry, you cannot use privileged ports on the host (below 1024): 80",
},
{
isTarget: isMicrosoftWSL,
ports: []string{"127.0.0.1:81:81/tcp"},
errorMsg: "Sorry, you cannot use privileged ports on the host (below 1024): 81",
},
{
isTarget: isMicrosoftWSL,
ports: []string{"127.0.0.1:81:81/udp"},
errorMsg: "Sorry, you cannot use privileged ports on the host (below 1024): 81",
},
{
isTarget: isMicrosoftWSL,
ports: []string{"127.0.0.1:80-83:80-83/tcp"},
errorMsg: "Sorry, you cannot use privileged ports on the host (below 1024): 80",
},
{
isTarget: isMicrosoftWSL,
ports: []string{"127.0.0.1:80-83:80-83/udp"},
errorMsg: "Sorry, you cannot use privileged ports on the host (below 1024): 80",
},
{
isTarget: true,
ports: []string{"80"},
errorMsg: "",
},
{
isTarget: true,
ports: []string{"80", "65535", "65536"},
errorMsg: "Sorry, one of the ports provided with --ports flag is outside range: 65536",
},
{
isTarget: true,
ports: []string{"0", "80", "65535"},
errorMsg: "Sorry, one of the ports provided with --ports flag is outside range: 0",
},
{
isTarget: true,
ports: []string{"cats"},
errorMsg: "Sorry, one of the ports provided with --ports flag is not valid: cats",
},
{
isTarget: true,
ports: []string{"127.0.0.1:81:0/tcp"},
errorMsg: "Sorry, one of the ports provided with --ports flag is outside range: 0",
},
{
isTarget: true,
ports: []string{"127.0.0.1:81:65536/tcp"},
errorMsg: "Sorry, one of the ports provided with --ports flag is not valid [127.0.0.1:81:65536/tcp] (Invalid containerPort: 65536)",
},
{
isTarget: true,
ports: []string{"1-65536:80-81/tcp"},
errorMsg: "Sorry, one of the ports provided with --ports flag is not valid [1-65536:80-81/tcp] (Invalid hostPort: 1-65536)",
},
{
isTarget: true,
ports: []string{"1-80:0-81/tcp"},
errorMsg: "Sorry, one of the ports provided with --ports flag is not valid [1-80:0-81/tcp] (Invalid ranges specified for container and host Ports: 0-81 and 1-80)",
},
{
isTarget: true,
ports: []string{"1-80:1-65536/tcp"},
errorMsg: "Sorry, one of the ports provided with --ports flag is not valid [1-80:1-65536/tcp] (Invalid containerPort: 1-65536)",
},
}
for _, test := range tests {
t.Run(strings.Join(test.ports, ","), func(t *testing.T) {
if !test.isTarget {
return
}
gotError := ""
got := validatePorts(test.ports)
if got != nil {
Expand Down