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.

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

fix proxy env not being passed to docker engine #8198

Merged
merged 4 commits into from
May 19, 2020

Conversation

medyagh
Copy link
Member

@medyagh medyagh commented May 18, 2020

closes a regression fixes #8120

before this PR

make && HTTP_PROXY=192.167.1.113:1313 ./out/minikube start --driver=hyperkit
minikube ssh
$ cat /lib/systemd/system/docker.service | grep Env
(empty)

after this PR

make && HTTP_PROXY=192.167.1.113:1313 ./out/minikube start --driver=hyperkit
minikube ssh
$ cat /lib/systemd/system/docker.service | grep Env
Environment=HTTP_PROXY=192.167.1.113:1313

also improve the icon for proxy

before

😄  minikube v1.10.1 on Darwin 10.13.6
✨  Using the docker driver based on user configuration
👍  Starting control plane node minikube in cluster minikube
🔥  Creating docker container (CPUs=2, Memory=4000MB) ...
🌐  Found network options:
    ▪ HTTP_PROXY=192.167.1.113:1313
❗  You appear to be using a proxy, but your NO_PROXY environment does not include the minikube IP (172.17.0.3). Please see https://minikube.sigs.k8s.io/docs/handbook/vpn_and_proxy/ for more details

after

😄  minikube v1.10.1 on Darwin 10.13.6
✨  Using the docker driver based on user configuration
👍  Starting control plane node minikube in cluster minikube
🔥  Creating docker container (CPUs=2, Memory=4000MB) ...
🌐  Found network options:
    ▪ HTTP_PROXY=192.167.1.113:1313
❗  You appear to be using a proxy, but your NO_PROXY environment does not include the minikube IP (172.17.0.3).
📘  Please see https://minikube.sigs.k8s.io/docs/handbook/vpn_and_proxy/ for more details
🐳  Preparing Kubernetes v1.18.2 on Docker 19.03.2 ...

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 18, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: medyagh

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2020
@medyagh medyagh changed the title wip: fix proxy wip: fix proxy not passing to docker-service May 18, 2020
@medyagh medyagh changed the title wip: fix proxy not passing to docker-service wip: fix proxy env not being passed to docker-service May 18, 2020
@TravisBuddy
Copy link

Travis tests have failed

Hey @medyagh,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 9a81fd80-995d-11ea-a150-93ebf46199e2

@k8s-ci-robot 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 May 19, 2020
@medyagh medyagh changed the title wip: fix proxy env not being passed to docker-service fix proxy env not being passed to docker-service May 19, 2020
@k8s-ci-robot 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 19, 2020
@medyagh medyagh changed the title fix proxy env not being passed to docker-service fix proxy env not being passed to docker engine May 19, 2020
@medyagh
Copy link
Member Author

medyagh commented May 19, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 19, 2020
@minikube-pr-bot
Copy link

kvm2 Driver
docker Driver

@codecov-commenter
Copy link

Codecov Report

Merging #8198 into master will decrease coverage by 0.08%.
The diff coverage is 39.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8198      +/-   ##
==========================================
- Coverage   34.60%   34.52%   -0.09%     
==========================================
  Files         147      147              
  Lines        9378     9397      +19     
==========================================
- Hits         3245     3244       -1     
- Misses       5736     5756      +20     
  Partials      397      397              
Impacted Files Coverage Δ
cmd/minikube/cmd/start.go 13.75% <ø> (-1.35%) ⬇️
pkg/minikube/proxy/proxy.go 77.10% <0.00%> (-19.87%) ⬇️
cmd/minikube/cmd/start_flags.go 50.80% <100.00%> (ø)
pkg/minikube/machine/start.go 62.67% <100.00%> (+2.22%) ⬆️
cmd/minikube/cmd/node_stop.go 13.33% <0.00%> (-0.96%) ⬇️
cmd/minikube/cmd/node_start.go 9.37% <0.00%> (-0.63%) ⬇️

@minikube-pr-bot
Copy link

kvm2 Driver
Times for minikube: [65.836998454 64.39831529899999 67.771931101]
Average time for minikube: 66.00241495133334

Times for Minikube (PR 8198): [65.365917656 66.580691607 63.772962386]
Average time for Minikube (PR 8198): 65.23985721633333

Averages Time Per Log

+--------------------------------+-----------+--------------------+
|              LOG               | MINIKUBE  | MINIKUBE (PR 8198) |
+--------------------------------+-----------+--------------------+
| * minikube v1.10.1 on Debian   |  0.068277 |           0.061046 |
|                           9.11 |           |                    |
| * Using the kvm2 driver based  |  0.022643 |           0.021523 |
| on existing profile            |           |                    |
| * Starting control plane node  |  0.003375 |           0.003607 |
| minikube in cluster minikube   |           |                    |
| * Creating kvm2 VM (CPUs=2,    | 40.623545 |          41.664673 |
| Memory=3700MB, Disk=20000MB)   |           |                    |
| ...                            |           |                    |
| * Preparing Kubernetes v1.18.2 | 22.856444 |          21.602212 |
| on Docker 19.03.8 ...          |           |                    |
| * Verifying Kubernetes         |  1.544299 |           1.280269 |
| components...                  |           |                    |
| * Enabled addons:              |  0.796563 |           0.524150 |
| default-storageclass,          |           |                    |
| storage-provisioner            |           |                    |
| * Done! kubectl is now         |  0.083639 |           0.076074 |
| configured to use "minikube"   |           |                    |
|                                |  0.003629 |           0.006303 |
+--------------------------------+-----------+--------------------+

docker Driver
Times for minikube: [28.12160220099999 25.638896388 27.151573497]
Average time for minikube: 26.97069069533333

Times for Minikube (PR 8198): [26.812146788 26.044307633000003 27.054481405]
Average time for Minikube (PR 8198): 26.636978608666663

Averages Time Per Log

+----------------------------------------+-----------+--------------------+
|                  LOG                   | MINIKUBE  | MINIKUBE (PR 8198) |
+----------------------------------------+-----------+--------------------+
| * minikube v1.10.1 on Debian           |  0.079128 |           0.076673 |
|                                   9.11 |           |                    |
| * Using the docker driver              |  0.003193 |           0.002991 |
| based on existing profile              |           |                    |
| * Starting control plane node          |  0.066546 |           0.065523 |
| minikube in cluster minikube           |           |                    |
| * Creating docker container            |  8.029731 |           8.018113 |
| (CPUs=2, Memory=3700MB) ...            |           |                    |
| * Preparing Kubernetes v1.18.2         |  0.151398 |           0.126406 |
| on Docker 19.03.2 ...                  |           |                    |
|   -                                    | 17.655911 |          17.341859 |
| kubeadm.pod-network-cidr=10.244.0.0/16 |           |                    |
| * Verifying Kubernetes                 |  0.869367 |           0.918231 |
| components...                          |           |                    |
| * Enabled addons:                      |  0.041776 |           0.003764 |
| default-storageclass,                  |           |                    |
| storage-provisioner                    |           |                    |
| * Done! kubectl is now                 |  0.066747 |           0.078093 |
| configured to use "minikube"           |           |                    |
|                                        |  0.006892 |           0.005326 |
+----------------------------------------+-----------+--------------------+

@tstromberg
Copy link
Contributor

Looks good, but do you mind adding an integration test so that we do not break this again?

Maybe just grep for the proxy definition within the docker config in our existing proxy test?

@medyagh
Copy link
Member Author

medyagh commented May 19, 2020

Looks good, but do you mind adding an integration test so that we do not break this again?

Maybe just grep for the proxy definition within the docker config in our existing proxy test?

unfortuantely we do not pass the Local Proxy envs to the docker service file.
and in the intergartion test we make our own proxy that is on localhost which wont be passed

@medyagh medyagh deleted the fix_proxy branch March 2, 2021 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP proxy environment variables not used anymore to configure docker daemon
7 participants