Masking http(s)_proxy password from startup output. #17116

rmsilva1973 · 2023-08-24T00:55:15Z

linux-foundation-easycla · 2023-08-24T00:55:18Z

The committers listed above are authorized under a signed CLA.

✅ login: rmsilva1973 / name: Renato Silva (ed550a0, efd15e8, 5aeb4a5, 7f75e80, df10b09)

k8s-ci-robot · 2023-08-24T00:55:23Z

Welcome @rmsilva1973!

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 · 2023-08-24T00:55:24Z

Hi @rmsilva1973. 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 · 2023-08-24T00:57:03Z

Can one of the admins verify this patch?

medyagh · 2023-08-30T20:33:21Z

fixes #17115

Can u plz post a before after this PR output (with dummy passwords)

rmsilva1973 · 2023-08-31T00:18:02Z

@medyagh Sure. Here is a before

rmsilva1973 · 2023-08-31T00:50:55Z

And here's the after:

rmsilva1973 · 2023-08-31T11:04:41Z

@medyagh BTW: don't worry. The password on the first screenshot is completely bogus. It's not my password. 😉

medyagh · 2023-08-31T16:59:09Z

/ok-to-test

medyagh · 2023-08-31T17:00:08Z

pkg/minikube/node/config.go

@@ -47,6 +49,16 @@ func showVersionInfo(k8sVersion string, cr cruntime.Manager) {
 		out.Infof("opt {{.docker_option}}", out.V{"docker_option": v})
 	}
 	for _, v := range config.DockerEnv {
+		parts := strings.Split(v, "=")


I recommend, putting this into a func and then add unit tests for them.
in config_test.go

@medyagh Will do that. But turns out "make test" is completely inconsistent in my WSL/Ubuntu-22.04. This function:

minikube/cmd/minikube/cmd/start_test.go

Line 570 in 4b4ad95

func TestValidatePorts(t *testing.T) {

is failing on different ports each time I run "make test" and sometimes it's even running without any failures. To be sure it's not related to my commit, I ran it on the original repo and it's behaving the same way. Guess I'll try to figure out what's happening here before going any further. Perhaps should I file a new issue? What do you think?

Anyway... I'll also test on Ubuntu running on a Virtual Machine to rule out some WSL related quirky behaviour. 🤔

@medyagh Moved the mask password to a func and implemented config_test.go. Let me know how it looks.

Merging from upstream

medyagh

thank you very much the PR looks much better, just a few more changes and it would be ready to go

medyagh · 2023-09-05T19:27:53Z

pkg/minikube/node/config_test.go

+			output: "myDockerOption=value",
+		},
+		{
+			input:  "http_proxy=http://myproxy.company.com",


lets use minikube domain name here, so we dont get spam

minikube.sigs.k8s.io

medyagh · 2023-09-05T19:29:26Z

pkg/minikube/node/config_test.go

+			output: "HTTPS_PROXY=http://jdoe@myproxy.company.com:8080",
+		},
+		{
+			input:  "https_proxy=https://mary:am$uT8zB(rP@myproxy.company.com:8080",


lets make this word obvious it is a fake password
such as
mary@iam$Fake!password

also lets make more example of password with other types of characters such as ' %&* (things that could break our regex)

Improved the regex robustness and the tests. I can't remember torturing regexes that much since I used to parse sendmail logs on SunOS 4.. 😊
Anyway, this is my first PR and I'm thankful for the feedback. I hope it's better now.

medyagh · 2023-09-06T18:32:24Z

/retest-this-please

minikube-pr-bot · 2023-09-06T19:53:46Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 17116) |
+----------------+----------+---------------------+
| minikube start | 51.4s    | 51.1s               |
| enable ingress | 27.7s    | 28.1s               |
+----------------+----------+---------------------+

Times for minikube start: 50.4s 51.4s 51.7s 51.6s 52.0s
Times for minikube (PR 17116) start: 51.7s 51.1s 50.6s 49.9s 52.2s

Times for minikube ingress: 26.7s 28.3s 27.7s 27.7s 28.2s
Times for minikube (PR 17116) ingress: 28.2s 27.7s 27.7s 27.8s 29.1s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 17116) |
+----------------+----------+---------------------+
| minikube start | 24.0s    | 23.9s               |
| enable ingress | 21.1s    | 21.2s               |
+----------------+----------+---------------------+

Times for minikube start: 22.6s 22.2s 24.4s 24.7s 25.9s
Times for minikube (PR 17116) start: 22.1s 25.2s 25.3s 24.3s 22.4s

Times for minikube ingress: 20.8s 20.8s 20.9s 21.9s 20.9s
Times for minikube (PR 17116) ingress: 20.9s 21.3s 21.9s 20.9s 20.9s

docker driver with containerd runtime

+-------------------+----------+---------------------+
|      COMMAND      | MINIKUBE | MINIKUBE (PR 17116) |
+-------------------+----------+---------------------+
| minikube start    | 23.8s    | 23.2s               |
| ⚠️  enable ingress | 31.5s    | 41.0s ⚠️             |
+-------------------+----------+---------------------+

Times for minikube (PR 17116) ingress: 31.4s 79.4s 31.4s 31.3s 31.3s
Times for minikube ingress: 32.3s 31.3s 31.3s 31.4s 31.4s

Times for minikube start: 24.1s 24.3s 23.5s 23.2s 24.0s
Times for minikube (PR 17116) start: 20.5s 24.2s 24.0s 23.2s 23.8s

minikube-pr-bot · 2023-09-06T21:09:42Z

These are the flake rates of all failed tests.

Environment	Failed Tests	Flake Rate (%)
Hyperkit_macOS	TestAddons/Setup (gopogh)	4.46 (chart)
Hyperkit_macOS	TestCertOptions (gopogh)	5.10 (chart)
Docker_Linux_crio_arm64	TestPause/serial/SecondStartNoReconfiguration (gopogh)	10.98 (chart)
QEMU_macOS	TestFunctional/parallel/TunnelCmd/serial/RunSecondTunnel (gopogh)	42.48 (chart)

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

rmsilva1973 · 2023-09-19T14:04:55Z

Hey @medyagh . Do you think it's ok to merge now?

medyagh · 2023-09-20T17:29:44Z

thank you for this contribution @rmsilva1973 I look forward to see more contributions from you

medyagh · 2023-09-20T17:29:48Z

/lgtm

k8s-ci-robot · 2023-09-20T17:30:00Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

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 24, 2023

k8s-ci-robot requested review from afbjorklund and sharifelgamal August 24, 2023 00:55

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

Masking http(s)_proxy password from output.

ed550a0

rmsilva1973 force-pushed the master branch from 3fa0254 to ed550a0 Compare August 24, 2023 01:21

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 24, 2023

Merge branch 'kubernetes:master' into master

efd15e8

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 31, 2023

medyagh requested changes Aug 31, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

Moved mask to a func and added test case.

5aeb4a5

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 2, 2023

This comment has been minimized.

Sign in to view

Merge pull request #1 from kubernetes/master

7f75e80

Merging from upstream

This comment has been minimized.

Sign in to view

medyagh requested changes Sep 5, 2023

View reviewed changes

Increasded regex robustness

df10b09

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 6, 2023

This comment has been minimized.

Sign in to view

medyagh merged commit e68882d into kubernetes:master Sep 20, 2023
27 of 41 checks passed

k8s-ci-robot assigned medyagh Sep 20, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 20, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 20, 2023

dennyferra mentioned this pull request Nov 13, 2023

Masking env passwords in start does not work when password includes encoded characters #17614

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Masking http(s)_proxy password from startup output. #17116

Masking http(s)_proxy password from startup output. #17116

rmsilva1973 commented Aug 24, 2023

linux-foundation-easycla bot commented Aug 24, 2023 •

edited

Loading

k8s-ci-robot commented Aug 24, 2023

k8s-ci-robot commented Aug 24, 2023

minikube-bot commented Aug 24, 2023

medyagh commented Aug 30, 2023

rmsilva1973 commented Aug 31, 2023 •

edited

Loading

rmsilva1973 commented Aug 31, 2023

rmsilva1973 commented Aug 31, 2023

medyagh commented Aug 31, 2023

medyagh Aug 31, 2023

rmsilva1973 Sep 1, 2023

rmsilva1973 Sep 2, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

medyagh left a comment

medyagh Sep 5, 2023

rmsilva1973 Sep 6, 2023

medyagh Sep 5, 2023

rmsilva1973 Sep 6, 2023 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

medyagh commented Sep 6, 2023

minikube-pr-bot commented Sep 6, 2023

minikube-pr-bot commented Sep 6, 2023

rmsilva1973 commented Sep 19, 2023

medyagh commented Sep 20, 2023

medyagh commented Sep 20, 2023

k8s-ci-robot commented Sep 20, 2023

Masking http(s)_proxy password from startup output. #17116

Masking http(s)_proxy password from startup output. #17116

Conversation

rmsilva1973 commented Aug 24, 2023

linux-foundation-easycla bot commented Aug 24, 2023 • edited Loading

k8s-ci-robot commented Aug 24, 2023

k8s-ci-robot commented Aug 24, 2023

minikube-bot commented Aug 24, 2023

medyagh commented Aug 30, 2023

rmsilva1973 commented Aug 31, 2023 • edited Loading

rmsilva1973 commented Aug 31, 2023

rmsilva1973 commented Aug 31, 2023

medyagh commented Aug 31, 2023

medyagh Aug 31, 2023

Choose a reason for hiding this comment

rmsilva1973 Sep 1, 2023

Choose a reason for hiding this comment

rmsilva1973 Sep 2, 2023

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

medyagh left a comment

Choose a reason for hiding this comment

medyagh Sep 5, 2023

Choose a reason for hiding this comment

rmsilva1973 Sep 6, 2023

Choose a reason for hiding this comment

medyagh Sep 5, 2023

Choose a reason for hiding this comment

rmsilva1973 Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

medyagh commented Sep 6, 2023

minikube-pr-bot commented Sep 6, 2023

minikube-pr-bot commented Sep 6, 2023

rmsilva1973 commented Sep 19, 2023

medyagh commented Sep 20, 2023

medyagh commented Sep 20, 2023

k8s-ci-robot commented Sep 20, 2023

linux-foundation-easycla bot commented Aug 24, 2023 •

edited

Loading

rmsilva1973 commented Aug 31, 2023 •

edited

Loading

rmsilva1973 Sep 6, 2023 •

edited

Loading