Added port validation

[kadern0]

Fixes #10495

Tests:

minikube start --driver=docker --ports=asdf:80 --ports=443:443
😄  minikube v1.22.0 on Ubuntu 20.04
✨  Using the docker driver based on existing profile

❌  Exiting due to MK_USAGE: Sorry, one of the ports provided with --ports flag is not valid [asdf:80 443:443]

minikube start --driver=docker --ports=8080:80 --ports=443:443
😄  minikube v1.22.0 on Ubuntu 20.04
✨  Using the docker driver based on existing profile

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

minikube start --driver=docker --ports=8080:80 --ports=6443:443
😄  minikube v1.22.0 on Ubuntu 20.04
✨  Using the docker driver based on existing profile
👍  Starting control plane node minikube in cluster minikube
🚜  Pulling base image ...
🔄  Restarting existing docker container for "minikube" ...
🐳  Preparing Kubernetes v1.21.3 on Docker 20.10.7 ...
🔎  Verifying Kubernetes components...
    ▪ Using image k8s.gcr.io/ingress-nginx/controller:v0.44.0
    ▪ Using image docker.io/jettech/kube-webhook-certgen:v1.5.1
    ▪ Using image docker.io/jettech/kube-webhook-certgen:v1.5.1
    ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🔎  Verifying ingress addon...
🌟  Enabled addons: storage-provisioner, default-storageclass, ingress

🏄  Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default

[k8s-ci-robot]

Hi @kadern0. 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/test-infra repository.

[minikube-bot]

Can one of the admins verify this patch?

[spowelljr]

Thanks for the validation @kadern0, it looks good! If I could ask you to do one more thing, if you wouldn't mind writing a test for this validation in start_test.go.

If you call

minikube/cmd/minikube/cmd/start_flags.go

Line 252 in a70bdb5

func generateClusterConfig(cmd *cobra.Command, existing *config.ClusterConfig, k8sVersion string, drvName string) (config.ClusterConfig, config.Node, error) {

in the test it will run flag validation, there are other tests in start_test.go that do this exact same thing if you're looking for an example.

Thanks!

[kadern0]

@spowelljr thanks for the feedback!
Adding tests for this function might not be as straightforward (for me, at least) as it seems. Unlike the other tests on the file, this function doesn't return anything (same as the other validation functions) and unlike the other two tests that call 'generateClusterConfig', we can't test the function from there since this function is called from 'runStart'. So unless I'm missing anything, options are: no test (as with most of validating functions), refactor and add a return statement and test the function directly (although this isn't being done for the other validation functions), making a call to the function during the configuration generation so we can test it from there.
What do you think?

[medyagh]

@spowelljr thanks for the feedback!
Adding tests for this function might not be as straightforward (for me, at least) as it seems. Unlike the other tests on the file, this function doesn't return anything (same as the other validation functions) and unlike the other two tests that call 'generateClusterConfig', we can't test the function from there since this function is called from 'runStart'. So unless I'm missing anything, options are: no test (as with most of validating functions), refactor and add a return statement and test the function directly (although this isn't being done for the other validation functions), making a call to the function during the configuration generation so we can test it from there.
What do you think?

how about adding a unit test in start_test.go
just for this func

validatePorts(ports []string)

[kadern0]

@medyagh so the plan is: refactoring the function so it returns error and then create a unit test for it, right?

[medyagh]

@medyagh so the plan is: refactoring the function so it returns error and then create a unit test for it, right?

Yes please . Thank you for your helpful prs

[kadern0]

@medyagh I've made the changes as suggested but now it seems the cyclomatic complexity of the function validateFlags is > 30. It seems it might require additional changes.

[kadern0]

Maybe these could be made functions in order to reduce the complexity of validateFlags:

https://github.com/kubernetes/minikube/blob/c3c4d0455dfed89650fdf54f9f70d551912b4969/cmd/minikube/cmd/start.go#L1090:L1099
https://github.com/kubernetes/minikube/blob/c3c4d0455dfed89650fdf54f9f70d551912b4969/cmd/minikube/cmd/start.go#L1121:L1145
https://github.com/kubernetes/minikube/blob/c3c4d0455dfed89650fdf54f9f70d551912b4969/cmd/minikube/cmd/start.go#L1147:L1164

What do you think?

[kadern0]

@medyagh , @spowelljr I think this PR is ready for another review :)

[spowelljr]

@kadern0 The code looks good! One last thing, could you run make generate-docs and push the changes up and then we'll be good to go, thanks for your patience.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadern0, spowelljr

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 [spowelljr]

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