Improve validation for extra-config. #10886

vishjain · 2021-03-21T05:59:30Z

Here is the output with the PR changes:
vishals-MacBook-Pro:minikube vishal$ ./out/minikube start --driver=docker --extra-config=etcd.client-cert-auth=false”
Error: invalid argument "etcd.client-cert-auth=false”" for "--extra-config" flag: invalid value: cannot contain end quotation: "etcd.client-cert-auth=false”"
See 'minikube start --help' for usage.

Before, the ./out/minikube start command would just hang. This PR handles the case where end of --extra-config value has an open/close quotation because that's where the hanging behavior occurs.

k8s-ci-robot · 2021-03-21T05:59:38Z

Welcome @vishjain!

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 · 2021-03-21T05:59:38Z

Hi @vishjain. 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 · 2021-03-21T06:03:03Z

Can one of the admins verify this patch?

medyagh · 2021-03-23T04:31:10Z

pkg/minikube/config/extra_options.go

+
+	// Check doesn't start or end with an opening quotation.
+	if strings.HasPrefix(value, "“") || strings.HasSuffix(value, "“") {
+		return fmt.Errorf("invalid value: canot contain quotation: %q", value)


Can you verify if this is actually true ? Could it be it hangs only if the there is One Qutation without closing ?

Do u mind posting the output of minikube fore and after this PR?

My mistake: There is only hanging currently if at the end there is a quotation mark:
minikube vishal$ ./out/minikube start --driver=docker --extra-config=etcd.client-cert-auth=false“
😄 minikube v1.18.1 on Darwin 10.14.6
✨ Using the docker driver based on existing profile
👍 Starting control plane node minikube in cluster minikube
🏃 Updating the running docker "minikube" container ...
🐳 Preparing Kubernetes v1.20.2 on Docker 20.10.3 ...
▪ etcd.client-cert-auth=false“

There is already an error that shows up when there is a quote at the beginning too:
./out/minikube start --driver=docker --extra-config=“etcd.client-cert-auth=false”
😄 minikube v1.18.1 on Darwin 10.14.6
✨ Using the docker driver based on existing profile
❗ These --extra-config parameters are invalid: [“etcd]

❌ Exiting due to MK_USAGE: Valid components are: [apiserver controller-manager scheduler etcd kubeadm kubelet kube-proxy]

So we need to just check the end of the string for a quote.

./out/minikube start --driver=docker --extra-config=“etcd.client-cert-auth=false
😄 minikube v1.18.1 on Darwin 10.14.6
✨ Using the docker driver based on existing profile
❗ These --extra-config parameters are invalid: [“etcd]

❌ Exiting due to MK_USAGE: Valid components are: [apiserver controller-manager scheduler etcd kubeadm kubelet kube-proxy]

I have preserved the initial errors and also added the error when there's a suffix quote at the end:
./out/minikube start --driver=docker --extra-config=etcd.client-cert-auth=false”
Error: invalid argument "etcd.client-cert-auth=false”" for "--extra-config" flag: invalid value: cannot contain end quotation: "etcd.client-cert-auth=false”"

medyagh · 2021-03-25T01:02:52Z

pkg/minikube/config/extra_options.go

+	prefixExists := strings.HasPrefix(value, "”") || strings.HasPrefix(value, "“")
+	suffixExists := strings.HasSuffix(value, "”") || strings.HasSuffix(value, "“")
+	if !prefixExists && suffixExists {
+		return fmt.Errorf("invalid value: cannot contain end quotation: %q", value)


what happens when error happens ? could u paste in the PR description

Before and After this PR ?
how about adding Unit test for Set Function ? with example of things that should Error and example of things that should NOT error

also improve the error wording, -extra-config

I've added a test and in the PR description above I've included what happens when we do the error checking. We are stopping he hanging occurring when a extra-config value ends with a open or close quote. It's important to note if the extra-config value has a prefix with an open or close quote then there's a separate error check in start.go file (cli) - I have left that alone.

medyagh

@vishjain please fix the lint, run "make test"

vishjain · 2021-03-30T01:45:51Z

@medyagh I ran make test and make lint. Hope it's good now.

k8s-ci-robot · 2021-04-01T16:47:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: medyagh, vishjain

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

Added error for quotation.

c402edd

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 21, 2021

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 Mar 21, 2021

k8s-ci-robot requested review from afbjorklund and RA489 March 21, 2021 05:59

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 21, 2021

Add check for opening and closing quote.

e916793

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 21, 2021

medyagh requested changes Mar 23, 2021

View reviewed changes

Check suffix ends with quotation.

e1f9fb1

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 24, 2021

Fixed end quote.

28f2697

vishjain requested a review from medyagh March 24, 2021 07:22

medyagh requested changes Mar 25, 2021

View reviewed changes

vishjain added 2 commits March 26, 2021 12:59

Improve error message.

198fa8d

Added unit test.

b565130

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 28, 2021

Added comment.

bdd4aa1

vishjain requested a review from medyagh March 28, 2021 22:04

medyagh approved these changes Mar 29, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2021

medyagh requested changes Mar 29, 2021

View reviewed changes

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2021

Ran make lint

21477e3

Reset test.

f125300

vishjain requested a review from medyagh March 30, 2021 01:46

medyagh approved these changes Apr 1, 2021

View reviewed changes

medyagh merged commit 8bc0dfd into kubernetes:master Apr 1, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2021

vishjain deleted the user/vishjain/extra-config branch April 1, 2021 16:56

ilya-zuyev changed the title ~~Added error check for extra quotation for extra-config.~~ Improve validation for extra-config. Apr 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve validation for extra-config. #10886

Improve validation for extra-config. #10886

vishjain commented Mar 21, 2021 •

edited

Loading

k8s-ci-robot commented Mar 21, 2021

k8s-ci-robot commented Mar 21, 2021

minikube-bot commented Mar 21, 2021

medyagh Mar 23, 2021

vishjain Mar 24, 2021

vishjain Mar 24, 2021

vishjain Mar 24, 2021

vishjain Mar 24, 2021

medyagh Mar 25, 2021

medyagh Mar 25, 2021

vishjain Mar 28, 2021

medyagh left a comment

vishjain commented Mar 30, 2021

k8s-ci-robot commented Apr 1, 2021

Improve validation for extra-config. #10886

Improve validation for extra-config. #10886

Conversation

vishjain commented Mar 21, 2021 • edited Loading

k8s-ci-robot commented Mar 21, 2021

k8s-ci-robot commented Mar 21, 2021

minikube-bot commented Mar 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medyagh left a comment

Choose a reason for hiding this comment

vishjain commented Mar 30, 2021

k8s-ci-robot commented Apr 1, 2021

vishjain commented Mar 21, 2021 •

edited

Loading