Allow minikube to function with misconfigured NO_PROXY value #4229

medyagh · 2019-05-08T22:05:59Z

fixes #4130 as well as #3844

minikube-bot · 2019-05-08T22:06:02Z

Can one of the admins verify this patch?

k8s-ci-robot · 2019-05-08T22:06:07Z

Hi @medyagh. 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.

medyagh · 2019-05-08T22:12:52Z

unit tests to be added soon.

tstromberg · 2019-05-08T22:28:11Z

@minikube-bot OK to test

tstromberg

It's tricky, but we should also add an integration test for proxies, something like test/integration/proxy_test.go.

To test it in a way that works for users who don't currently have a proxy, it might perhaps start one up using something like https://github.com/elazarl/goproxy

cmd/minikube/cmd/start.go

pkg/minikube/proxy/noproxy.go

pkg/minikube/proxy/env.go

pkg/minikube/proxy/noproxy.go

cmd/minikube/cmd/dashboard.go

pkg/minikube/proxy/cidir.go

pkg/minikube/proxy/cidir_test.go

tstromberg · 2019-05-10T19:16:28Z

Code looks great. Just minor nits.

codecov-io · 2019-05-16T00:38:09Z

Codecov Report

Merging #4229 into master will increase coverage by 0.05%.
The diff coverage is 31.86%.

@@            Coverage Diff             @@
##           master    #4229      +/-   ##
==========================================
+ Coverage   32.78%   32.83%   +0.05%     
==========================================
  Files          93       94       +1     
  Lines        6031     6113      +82     
==========================================
+ Hits         1977     2007      +30     
- Misses       3784     3835      +51     
- Partials      270      271       +1

Impacted Files	Coverage Δ
pkg/minikube/cluster/cluster.go	`36.29% <0%> (-1.16%)`	⬇️
pkg/minikube/bootstrapper/kubeadm/kubeadm.go	`20.19% <0%> (-0.07%)`	⬇️
pkg/util/kubernetes.go	`0% <0%> (ø)`	⬆️
cmd/minikube/cmd/dashboard.go	`3.03% <0%> (-0.2%)`	⬇️
cmd/minikube/cmd/start.go	`11.26% <0%> (-0.22%)`	⬇️
pkg/minikube/service/service.go	`34.1% <12.5%> (-0.2%)`	⬇️
pkg/minikube/proxy/proxy.go	`49.12% <49.12%> (ø)`
pkg/util/kubeconfig.go	`46.05% <0%> (+1.31%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a4270c...f38efb9. Read the comment docs.

tstromberg · 2019-05-16T06:19:02Z

The "None" failure seems a bit strange to me, but not altogether unheard of. Go ahead and resolve the merge conflicts and we'll see if it comes up again.

medyagh · 2019-05-16T23:46:08Z

@minikube-bot OK to test

tstromberg · 2019-05-17T01:50:21Z

Looks like the test is failing because it's not possible to download kubelet / kubeadm with the proxy.

 00:53:58 | > @   Downloading kubelet v1.10.13
 00:53:58 | ! W0517 00:53:58.460580   25562 exit.go:99] Failed to update cluster: downloading binaries: downloading kubelet: Error downloading kubelet v1.10.13: failed to download: failed to download to temp file: download failed: 5 error(s) occurred:
 00:53:58 | ! * Temporary download error: Get https://storage.googleapis.com/kubernetes-release/release/v1.10.13/bin/linux/amd64/kubelet: proxyconnect tcp: dial tcp [::1]:37345: connect: connection refused
 00:53:58 | ! * Temporary download error: Get https://storage.googleapis.com/kubernetes-release/release/v1.10.13/bin/linux/amd64/kubelet: proxyconnect tcp: dial tcp [::1]:37345: connect: connection refused
 00:53:58 | ! * Temporary download error: Get https://storage.googleapis.com/kubernetes-release/release/v1.10.13/bin/linux/amd64/kubelet: proxyconnect tcp: dial tcp [::1]:37345: connect: connection refused
 00:53:58 | ! * Temporary download error: Get https://storage.googleapis.com/kubernetes-release/release/v1.10.13/bin/linux/amd64/kubelet: proxyconnect tcp: dial tcp [::1]:37345: connect: connection refused
 00:53:58 | ! * Temporary download error: Get https://storage.googleapis.com/kubernetes-release/release/v1.10.13/bin/linux/amd64/kubelet: proxyconnect tcp: dial tcp [::1]:37345: connect: connection refused
 00:53:58 | ! !   Failed to update cluster: downloading binaries: downloading kubelet: Error downloading kubelet v1.10.13: failed to download: failed to download to temp file: download failed: 5 error(s) occurred:
 00:53:58 | ! * Temporary download error: Get https://storage.googleapis.com/kubernetes-release/release/v1.10.13/bin/linux/amd64/kubelet: proxyconnect tcp: dial tcp [::1]:37345: connect: connection refused
 00:53:58 | ! * Temporary download error: Get https://storage.googleapis.com/kubernetes-release/release/v1.10.13/bin/linux/amd64/kubelet: proxyconnect tcp: dial tcp [::1]:37345: connect: connection refused
 00:53:58 | ! * Temporary download error: Get https://storage.googleapis.com/kubernetes-release/release/v1.10.13/bin/linux/amd64/kubelet: proxyconnect tcp: dial tcp [::1]:37345: connect: connection refused
 00:53:58 | ! * Temporary download error: Get https://storage.googleapis.com/kubernetes-release/release/v1.10.13/bin/linux/amd64/kubelet: proxyconnect tcp: dial tcp [::1]:37345: connect: connection refused
 00:53:58 | ! * Temporary download error: Get https://storage.googleapis.com/kubernetes-release/release/v1.10.13/bin/linux/amd64/kubelet: proxyconnect tcp: dial tcp [::1]:37345: connect: connection refused
 00:53:58 | ! *   Sorry that minikube crashed. If this was unexpected, we would love to hear from you:
 00:53:58 | ! -   https://github.com/kubernetes/minikube/issues/new

Interestingly, it seems to affect more than just the proxy test:

--- FAIL: TestVersionUpgrade (55.97s)

My suggestion: check that the HTTP_PROXY environment variable isn't leaking, and ensure that all of the dependencies are downloaded by calling "minikube start --download-only" before setting up the HTTP_PROXY - or make sure that the proxy routes packets to the internet.

test/integration/proxy_test.go

medyagh · 2019-05-17T04:13:43Z

@minikube-bot OK to test

medyagh · 2019-05-18T22:34:50Z

@minikube-bot OK to test

k8s-ci-robot · 2019-05-19T07:40:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: medyagh
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: luxas

If they are not already assigned, you can assign the PR to them by writing /assign @luxas in a comment when ready.

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

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

medyagh · 2019-05-19T07:42:10Z

@tstromberg I wonder if that error was because the testDashboard runs in parallel. I removed the parallel to see if it works. on my machine the integration test passes for osx hyperkit

medyagh · 2019-05-19T22:00:02Z

@minikube-bot OK to test

tstromberg · 2019-05-20T15:12:04Z

@minikube-bot OK test

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 8, 2019

medyagh added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 8, 2019

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 8, 2019

k8s-ci-robot requested review from balopat and dlorenc May 8, 2019 22:06

medyagh force-pushed the no_proxy_for_self branch from 446a0e7 to 80163d2 Compare May 8, 2019 22:09

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 8, 2019

Fixed API connectivity with http(s) proxy

581b63a

medyagh force-pushed the no_proxy_for_self branch from 80163d2 to 581b63a Compare May 8, 2019 22:10

medyagh requested a review from tstromberg May 8, 2019 22:11

Added more comment

0529f0b

medyagh requested a review from sharifelgamal May 8, 2019 22:18

tstromberg suggested changes May 9, 2019

View reviewed changes

cmd/minikube/cmd/start.go Outdated Show resolved Hide resolved

cmd/minikube/cmd/start.go Outdated Show resolved Hide resolved

cmd/minikube/cmd/start.go Outdated Show resolved Hide resolved

Fix dashboard behind proxy and refactor

8bc0010

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 May 10, 2019

tstromberg suggested changes May 10, 2019

View reviewed changes

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 14, 2019

medyagh added 4 commits May 14, 2019 20:16

Fixing K8S Client Config with TranportWrap

6a433d0

return error if nil

ddb95e1

No Proxy for Dashboard and moving logic to proxy package

8120cdd

Updated warn message and remove duplicate warning

830e18d

ne0h and others added 3 commits May 15, 2019 16:59

Make derivation of Host IP address more generic when using vmware.

88a1489

Adding more unit test

a2590ec

Improved Style

f38efb9

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2019

Added unittest and rebase

3ff806a

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2019

medyagh added 2 commits May 16, 2019 15:26

Improve http server shutdown handling

a1e34b0

fix formatting

6484548

tstromberg reviewed May 17, 2019

View reviewed changes

test/integration/proxy_test.go Show resolved Hide resolved

Clean up after unit test

0d58773

Merge remote-tracking branch 'upstream/master' into no_proxy_for_self

d4cf3b0

medyagh added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 17, 2019

Debug

42e40e3

medyagh added 3 commits May 19, 2019 14:07

Better clean up after tesdt proxy setup

871c2e6

Fix typo

c428ce5

Improve test clean up

b6bf1df

tstromberg merged commit 9d09ff0 into kubernetes:master May 20, 2019

medyagh deleted the no_proxy_for_self branch May 31, 2019 01:49

medyagh mentioned this pull request Jun 5, 2019

REQUEST: New membership for medyagh kubernetes/org#888

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow minikube to function with misconfigured NO_PROXY value #4229

Allow minikube to function with misconfigured NO_PROXY value #4229

medyagh commented May 8, 2019 •

edited by tstromberg

Loading

minikube-bot commented May 8, 2019

k8s-ci-robot commented May 8, 2019

medyagh commented May 8, 2019

tstromberg commented May 8, 2019

tstromberg left a comment

tstromberg commented May 10, 2019

codecov-io commented May 16, 2019

tstromberg commented May 16, 2019

medyagh commented May 16, 2019

tstromberg commented May 17, 2019 •

edited

Loading

medyagh commented May 17, 2019

medyagh commented May 18, 2019

k8s-ci-robot commented May 19, 2019

medyagh commented May 19, 2019

medyagh commented May 19, 2019

tstromberg commented May 20, 2019

Allow minikube to function with misconfigured NO_PROXY value #4229

Allow minikube to function with misconfigured NO_PROXY value #4229

Conversation

medyagh commented May 8, 2019 • edited by tstromberg Loading

minikube-bot commented May 8, 2019

k8s-ci-robot commented May 8, 2019

medyagh commented May 8, 2019

tstromberg commented May 8, 2019

tstromberg left a comment

Choose a reason for hiding this comment

tstromberg commented May 10, 2019

codecov-io commented May 16, 2019

Codecov Report

tstromberg commented May 16, 2019

medyagh commented May 16, 2019

tstromberg commented May 17, 2019 • edited Loading

medyagh commented May 17, 2019

medyagh commented May 18, 2019

k8s-ci-robot commented May 19, 2019

medyagh commented May 19, 2019

medyagh commented May 19, 2019

tstromberg commented May 20, 2019

medyagh commented May 8, 2019 •

edited by tstromberg

Loading

tstromberg commented May 17, 2019 •

edited

Loading