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.

Sign up for GitHub

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

Jump to bottom

Proxy: handle lower case proxy env vars #4602

Merged
merged 1 commit into from
Jun 26, 2019
Merged

Proxy: handle lower case proxy env vars #4602

merged 1 commit into from
Jun 26, 2019

Conversation

medyagh
Copy link
Member

@medyagh medyagh commented Jun 26, 2019 •
edited
Loading

Closes #4600 and #3242 (hopefully)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 26, 2019
@k8s-ci-robot k8s-ci-robot requested a review from RA489 June 26, 2019 02:46
@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 size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 26, 2019
@dfang
Copy link
Contributor

dfang commented Jun 26, 2019

nice,

currently i have to start minikube like this

alias minikube-start='minikube start --vm-driver hyperkit --insecure-registry raspberrypi.local --registry-mirror https://registry.docker-cn.com --docker-env http_proxy=http://`ipconfig getifaddr en0`:8118 --docker-env https_proxy=http://`ipconfig getifaddr en0`:8118 --logtostderr --v=3'

and it's possible for minikube to read REGISTRY-MIRROR (MINIKUBE-REGISTRY-MIRROR) env ?

and also print it to stdout like HTTP_PROXY ?

dfang
dfang reviewed Jun 26, 2019
View reviewed changes
cmd/minikube/cmd/start.go
@@ -447,6 +447,9 @@ func generateConfig(cmd *cobra.Command, k8sVersion string) (cfg.Config, error) {
if !cmd.Flags().Changed("docker-env") {
for _, k := range proxy.EnvVars {
if v := os.Getenv(k); v != "" {
// convert https_proxy to HTTPS_PROXY for linux
// TODO (@medyagh): if user has both http_proxy & HTTPS_PROXY set merge them.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no idea on merge, you mean just take one's precedence over the other one ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ! thanks for correcting me, I meant that for no_proxy and NO_PROXY, if there are both merge them into one bigger NO_PROXY. good catch !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, and also, would you mind me to REGISTRY_MIRROR env to EnvVars in the same file (proxy.go) ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tried to use viper to load MINIKUBE_REGISTRY_MIRROR env

see PR #4607

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not familiar with REGISTRY_MIRROR env var. the purpose of this PR is to plumb the standard networking env vars. (the ones that operating systems use to change their proxies). does REGISTRY_MIRROR env var change the behaviour of OS ? or is it minikube specific en var ?

if it is minikube specific that would be for a different PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on a second thought, is that env var a standard docker env var that everyone uses to set their registery mirror ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also have another in progress PR by by @tstromberg https://github.com/kubernetes/minikube/pull/3835/files

I think we can address the registry mirror in a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@medyagh REGISTRY_MIRROR is docker env var, it's important for users to pull images faster in China, no need to change here, i've send a PR, see #4607

tstromberg
tstromberg approved these changes Jun 26, 2019
View reviewed changes
Copy link
Contributor

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How difficult would it be to add an integration test for this?

@medyagh
Copy link
Member Author

medyagh commented Jun 26, 2019 •
edited
Loading

alias minikube-start='minikube start --vm-driver hyperkit --insecure-registry raspberrypi.local --registry-mirror https://registry.docker-cn.com --docker-env http_proxy=http://ipconfig getifaddr en0:8118 --docker-env https_proxy=http://ipconfig getifaddr en0:8118 --logtostderr --v=3'

@dfang That would be a great feature for another PR, would you mind creating issue so someone work on it?

@medyagh
Copy link
Member Author

medyagh commented Jun 26, 2019

How difficult would it be to add an integration test for this?

not difficult at all ! :)

@dfang
Copy link
Contributor

dfang commented Jun 26, 2019 •
edited
Loading

alias minikube-start='minikube start --vm-driver hyperkit --insecure-registry raspberrypi.local --registry-mirror https://registry.docker-cn.com --docker-env http_proxy=http://ipconfig getifaddr en0:8118 --docker-env https_proxy=http://ipconfig getifaddr en0:8118 --logtostderr --v=3'

@dfang That would be a great feature for another PR, would you mind creating issue so someone work on it?

i'm working on it , trying to help and learn more about minikube .

@medyagh medyagh merged commit 886d159 into kubernetes:master Jun 26, 2019
@medyagh medyagh deleted the lowercase_proxy branch June 26, 2019 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dfang dfang dfang left review comments

@tstromberg tstromberg tstromberg approved these changes

@afbjorklund afbjorklund Awaiting requested review from afbjorklund

@RA489 RA489 Awaiting requested review from RA489

Assignees
No one assigned
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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Minikube should read lowercase http_proxy too
4 participants
@medyagh @k8s-ci-robot @dfang @tstromberg