kubeadm: pod-cidr, service-cidr should not go through any proxy by dixudx · Pull Request #58698 · kubernetes/kubernetes

dixudx · 2018-01-23T15:23:26Z

What this PR does / why we need it:
kubeadm passes proxy variables to static pods during init stage by #37494. But all the pods and service ips in pod-cidr, service-cidr should not go through any proxy.

This PR fixes this issue.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/kubeadm#666

Special notes for your reviewer:
/assign @luxas
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm

Release note:

kubeadm: pod-cidr, service-cidr should not go through any proxy. Bypassing proxies for intra-cluster communication.

jamiehannaford

thanks for fixing this, just one minor nit

jamiehannaford · 2018-01-23T15:59:07Z

you could also continue if name == no_proxy and set the env in 1 place rather than 2. WDYT? not a big issue IMO

kad · 2018-01-23T17:29:15Z

First of all, kubeadm produce warnings if connections to any used CIDRs will end-up via proxy.
Altering or ignoring user set environments some time ago was considered as not appropriate solution, as in some corner cases it might be needed by cluster administrator.

As for PR itself, it handles only IPv4 addresses, that is not acceptable, as since 1.9.x release kubernetes can be both IPv4 and IPv6.

/assign kad

kad · 2018-01-23T17:29:54Z

kubeadm must be able to handle both IPv4 and IPv6.

@kad Thanks for reminding. This function works both for IPv4 and IPv6. Locally tested.

errordeveloper · 2018-01-23T23:04:37Z

The release note is not very clear at all. This is about bypassing proxies for intra-cluster communication, right? If so, please say something like that, so that it makes sense to end-users. To be honest it took me a while to get what it is about exactly. Also, as far as I am aware, the communication of all control plane components should not even leave the network, accept for fetching images from a public registry, or check version (like kubeadm and weave net do), so I am not entirely sure why we have it set everywhere in the first place...

kad · 2018-01-23T23:19:31Z

@errordeveloper It is related to issues #36573 kubernetes/kubeadm#5

k8s-ci-robot · 2018-01-24T03:20:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dixudx
We suggest the following additional approver: luxas

Assign the PR to them by writing /assign @luxas in a comment when ready.

Associated issue: #37494

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

cmd/kubeadm/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

dixudx · 2018-01-24T03:21:44Z

@kad @jamiehannaford Updated. PTAL. Thanks.

dixudx · 2018-01-24T03:27:09Z

Also, as far as I am aware, the communication of all control plane components should not even leave the network, accept for fetching images from a public registry, or check version (like kubeadm and weave net do), so I am not entirely sure why we have it set everywhere in the first place...

@kad Related to issue kubernetes/kubeadm#666 as well.

dixudx · 2018-01-24T05:31:34Z

/retest

dixudx · 2018-01-24T07:45:59Z

/retest

roberthbailey · 2018-01-24T17:15:57Z

This could be a really long list if you have a large CIDR block. Isn't there a limit on how long an environment variable can be (less than 32k)?

There is limit. And transport connections supports CIDR notation of subnets in no_proxy.
There is no need to expand them at all.

@kad AFAIK, golang does not support no_proxy with CIDR notation of subnet.
Here is an on-going issue golang/go#16704 on this.

Isn't there a limit on how long an environment variable can be (less than 32k)
There is limit.

Yeah. https://www.in-ulm.de/~mascheck/various/argmax/ lists Actual values for ARG_MAX. This is a risk when setting environments.

That is exactly the reason why I consider that patch is incorrect:

kubeadm and other kubernetes components are utilizing modified http transport which supports CIDR notations.

kubeadm gives warnings during init stage if access to any IP address in pod or service CIDRs will be going via proxies, according to values of *_proxy variables.

If system admin require traffic to be going via proxies, he is able to ignore warning.

if system admin wants to have traffic going directly = values of environment variables before running kubeadm should be including pod and service cidrs.

with current idea of the patch behaviour becomes inconsistent: kubeadm will produce warning, but anyway will alter variables put into manifests. That is less predictable for sysadmins and will cause more issues.

trying to expand IPv6 pod/service cidrs will lead to OOM-kill of kubeadm.

So, to summarise: if in issue kubernetes/kubeadm#666 warning was correctly shown to user of kubeadm but was ignored by user, we don't need this PR at all. If for some reason warning was not shown, we need to investigate more.

@kad Thanks for the explanation.

And transport connections supports CIDR notation of subnets in no_proxy.
There is no need to expand them at all.

I did found the link

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/net/http.go

Lines 86 to 88 in f31ac9e

// http.ProxyFromEnvironment doesn't respect CIDRs and that makes it impossible to exclude things like pod and service IPs from proxy settings

// ProxierWithNoProxyCIDR allows CIDR rules in NO_PROXY

t.Proxy = NewProxierWithNoProxyCIDR(http.ProxyFromEnvironment)

.

Will update it.

dixudx · 2018-01-26T06:01:02Z

ping @jamiehannaford @roberthbailey @kad Updated. PTAL. Thanks.

dixudx · 2018-01-26T08:27:31Z

/retest

kad · 2018-01-26T11:50:27Z

I think that it is better to have consistent behavior, and this PR is not needed. Unless we get more information from kubernetes/kubeadm#666, I'd prefer to handle proxy settings as it was agreed in #35044: we have pre-flight checks and they are providing warnings to sysadmins. Sysadmins have choice which way is better for their particular setup.

errordeveloper · 2018-02-01T22:19:20Z

I agree with @kad's view on this.

…

On Fri, 26 Jan 2018, 11:50 am Alexander D. Kanevskiy, < ***@***.***> wrote: I think that it is better to have consistent behavior, and this PR is not needed. Unless we get more information from kubernetes/kubeadm#666 <kubernetes/kubeadm#666>, I'd prefer to handle proxy settings as it was agreed in #35044 <#35044>: we have pre-flight checks and they are providing warnings to sysadmins. Sysadmins have choice which way is better for their particular setup. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58698 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPWSziZn5zOGx7z8Lme1Gc5k2U6Si9qks5tObwIgaJpZM4Rp0Ua> .

dixudx · 2018-02-02T01:50:06Z

we have pre-flight checks and they are providing warnings to sysadmins. Sysadmins have choice which way is better for their particular setup.

Right. But most people would just ignore those warnings.

kad · 2018-02-02T13:28:22Z

We can make them as errors in theory. With fine-grained error skipping, that can be an option for those who has legitimate cases to use transport calls via proxies. Not a nice solution, but at least predictable and explicit.

k8s-github-robot · 2018-02-12T08:28:59Z

@dixudx PR needs rebase

kad · 2018-02-13T17:11:31Z

Let's close this PR. If we consider that proxy checks must become errors, it can be done in separate PR.

/close

errordeveloper · 2018-02-13T20:05:31Z

Thanks, @kad, I agree with your judgement on this tricky issue.

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 23, 2018

k8s-ci-robot assigned luxas Jan 23, 2018

k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. area/kubeadm size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 23, 2018

k8s-ci-robot requested review from fabriziopandini and xiangpengzhao January 23, 2018 15:23

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 23, 2018

dixudx mentioned this pull request Jan 23, 2018

Proxy environment variables shouldn't be passed to apiserver kubernetes/kubeadm#666

Closed

jamiehannaford reviewed Jan 23, 2018

View reviewed changes

k8s-ci-robot assigned kad Jan 23, 2018

kad suggested changes Jan 23, 2018

View reviewed changes

dixudx force-pushed the kubeadm_no_proxy branch from 08dff30 to 3725088 Compare January 24, 2018 03:20

dixudx force-pushed the kubeadm_no_proxy branch from 3725088 to e28be4c Compare January 24, 2018 06:57

roberthbailey reviewed Jan 24, 2018

View reviewed changes

kubeadm: pod-cidr, service-cidr should not go through any proxy

4075fc0

dixudx force-pushed the kubeadm_no_proxy branch from e28be4c to 4075fc0 Compare January 26, 2018 05:59

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

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2018

k8s-ci-robot closed this Feb 13, 2018

dixudx deleted the kubeadm_no_proxy branch February 23, 2018 09:02

This was referenced Feb 28, 2018

Update troublecshooting guide due to sidecar injection failures with proxy settings istio/istio.io#1043

Closed

Sidecar proxy help istio/istio.io#1044

Merged

luisyonaldo mentioned this pull request Jan 4, 2019

kubernetes/master should run without proxy environments being set kubernetes-sigs/kubespray#3978

Closed

ayj mentioned this pull request Jan 18, 2019

creating gateways times out on clean install istio/istio#10815

Closed

	// http.ProxyFromEnvironment doesn't respect CIDRs and that makes it impossible to exclude things like pod and service IPs from proxy settings
	// ProxierWithNoProxyCIDR allows CIDR rules in NO_PROXY
	t.Proxy = NewProxierWithNoProxyCIDR(http.ProxyFromEnvironment)

Uh oh!

Conversation

dixudx commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamiehannaford left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kad commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

errordeveloper commented Jan 23, 2018

Uh oh!

kad commented Jan 23, 2018

Uh oh!

k8s-ci-robot commented Jan 24, 2018

Uh oh!

dixudx commented Jan 24, 2018

Uh oh!

dixudx commented Jan 24, 2018

Uh oh!

dixudx commented Jan 24, 2018

Uh oh!

dixudx commented Jan 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dixudx commented Jan 26, 2018

Uh oh!

dixudx commented Jan 26, 2018

Uh oh!

kad commented Jan 26, 2018

Uh oh!

errordeveloper commented Feb 1, 2018 via email

Uh oh!

dixudx commented Feb 2, 2018

Uh oh!

kad commented Feb 2, 2018

Uh oh!

k8s-github-robot commented Feb 12, 2018

Uh oh!

kad commented Feb 13, 2018

Uh oh!

errordeveloper commented Feb 13, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dixudx commented Jan 23, 2018 •

edited

Loading

errordeveloper commented Feb 13, 2018 via email •

edited

Loading