Warn user if Pod/Service networks will be accessed via proxy. #52792

kad · 2017-09-20T13:38:46Z

What this PR does / why we need it:
In environments where HTTP proxies are used, it is important
to whitelist Pod and Services network ranges in the NO_PROXY
variable, so cluster will be properly operational.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

- kubeadm  will warn users if access to IP ranges for Pods or Services will be done via HTTP proxy.

k8s-ci-robot · 2017-09-20T13:38:54Z

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

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. I understand the commands that are listed here.

xiangpengzhao · 2017-09-20T16:19:56Z

/ok-to-test

kad · 2017-09-20T18:02:34Z

flakes of test infra :(

luxas

/assign @mikedanese @timothysc @mattmoyer
/unassign
(deferring to others, not familiar with all proxy edge cases)

luxas · 2017-09-26T19:45:41Z

cmd/kubeadm/app/preflight/checks.go

+		return nil, []error{err}
+	}
+	if proxy != nil {
+		return []error{fmt.Errorf("Connection to %q uses proxy %q. This may lead to malfunctional cluster setup. Make sure that Pod and Services IP ranges specified correctly as exceptions in proxy configuration", subnet.CIDR, proxy)}, nil


lowercase first

will fix in a moment.

luxas · 2017-09-26T19:45:59Z

cmd/kubeadm/app/preflight/checks.go

+	CIDR  string
+}
+
+func (subnet HTTPProxyCIDRCheck) Check() (warnings, errors []error) {


this will fail in golint

ok. will verify.

luxas · 2017-09-26T19:46:29Z

cmd/kubeadm/app/preflight/checks.go

@@ -627,6 +674,9 @@ func RunInitMasterChecks(cfg *kubeadmapi.MasterConfiguration) error {
 			ControllerManagerExtraArgs: cfg.ControllerManagerExtraArgs,
 			SchedulerExtraArgs:         cfg.SchedulerExtraArgs,
 		},
+		HTTPProxyCheck{Proto: "https", Host: cfg.API.AdvertiseAddress, Port: int(cfg.API.BindPort)},


is this still necessary?

Yes, that is another check. (master IP address is not part of pod/service CIDR checks). Just changed order, so all proxy related warnings will be in one place and not mixed with warnings/errors of something else.

kad · 2017-09-26T20:01:33Z

Updated.

luxas · 2017-09-26T20:20:36Z

/assign @mikedanese @timothysc @mattmoyer
/unassign
(didn't work in review comment it seems)

kad · 2017-09-29T12:55:22Z

@mikedanese @timothysc @mattmoyer can we make it retested and if there is no objections merged into master ?

kad · 2017-09-29T22:35:55Z

/retest

timothysc · 2017-10-11T21:20:39Z

cmd/kubeadm/app/preflight/checks.go

+		return nil, []error{err}
+	}
+	if proxy != nil {
+		return []error{fmt.Errorf("connection to %q uses proxy %q. This may lead to malfunctional cluster setup. Make sure that Pod and Services IP ranges specified correctly as exceptions in proxy configuration", subnet.CIDR, proxy)}, nil


I'm struggling to see why this is always an error and how we ensure we don't get false positives.

Would it be possible to add test cases here?

It will produce warning, not error. Test case potentially possible, just need to be careful with set/unset environment variables during test.

kad · 2017-10-13T17:07:31Z

@timothysc updated with added test case for CIDRs both in IPv4 and IPv6 format.

kad · 2017-10-14T08:46:44Z

/test pull-kubernetes-unit

kad · 2017-10-14T21:08:01Z

Something is in really weird state: net/http.ProxyFromEnvironment() inside CI somehow always return nil, nil regardless of URLs or environment variables. Exactly same tests happily passing locally.

liggitt · 2017-10-14T21:43:18Z

ProxyFromEnvironment doesn't react to changes in the env once it has been called once. see envOnce. CI tests the entire tree, and something else probably calls ProxyFromEnvironment prior to your package

kad · 2017-10-14T21:49:59Z

Thanks @liggitt, that seems to be reason. Strange thing that I'm not able to get similar result locally. But anyway, will try to find other way to implement unit test for this change.

In environments where HTTP proxies are used, it is important to whitelist Pod and Services network ranges in the NO_PROXY variable, so cluster will be properly operational.

kad · 2017-10-15T09:18:15Z

@timothysc PR updated with unit test that is compatible with CI. Unfortunately within CI it is not really useful as initialized environment has no proxy set and this is cached by http.ProxyFromEnvironment. In such scenarios, where unit test detects that http.ProxyFromEnvironment will not return expected result, it will skip test. Locally, test produces this:

Empty environment:

kad@kad:/work/go/src/k8s.io/kubernetes> env -i PATH=$PATH GOPATH=$GOPATH GOROOT=$GOROOT make test WHAT=./cmd/kubeadm/app/preflight/ GOFLAGS=-v
Running tests for APIVersion: v1,admissionregistration.k8s.io/v1alpha1,admission.k8s.io/v1alpha1,apps/v1beta1,apps/v1beta2,apps/v1,authentication.k8s.io/v1,authentication.k8s.io/v1beta1,authorization.k8s.io/v1,authorization.k8s.io/v1beta1,autoscaling/v1,autoscaling/v2beta1,batch/v1,batch/v1beta1,batch/v2alpha1,certificates.k8s.io/v1beta1,extensions/v1beta1,imagepolicy.k8s.io/v1alpha1,networking.k8s.io/v1,policy/v
1beta1,rbac.authorization.k8s.io/v1,rbac.authorization.k8s.io/v1beta1,rbac.authorization.k8s.io/v1alpha1,scheduling.k8s.io/v1alpha1,settings.k8s.io/v1alpha1,storage.k8s.io/v1beta1,storage.k8s.io/v1,,federatio
n/v1beta1
+++ [1015 12:13:52] Running tests without code coverage
=== RUN   TestRunInitMasterChecks
--- PASS: TestRunInitMasterChecks (0.00s)
=== RUN   TestRunJoinNodeChecks
--- PASS: TestRunJoinNodeChecks (0.00s)
=== RUN   TestRunChecks
--- PASS: TestRunChecks (0.00s)
=== RUN   TestConfigRootCAs
--- PASS: TestConfigRootCAs (0.00s)
=== RUN   TestConfigCertAndKey
--- PASS: TestConfigCertAndKey (0.00s)
=== RUN   TestKubernetesVersionCheck
--- PASS: TestKubernetesVersionCheck (0.00s)
=== RUN   TestHTTPProxyCIDRCheck
--- PASS: TestHTTPProxyCIDRCheck (0.00s)
        checks_test.go:483: Saved environment:  map[no_proxy:127.0.0.1,localhost]
        checks_test.go:503: http.ProxyFromEnvironment is usable, continue executing test
=== RUN   TestGetKubeletVersion
--- PASS: TestGetKubeletVersion (0.00s)
PASS
ok      k8s.io/kubernetes/cmd/kubeadm/app/preflight     0.052s

My environment where corporate proxies are used and defined in *_proxy

kad@kad:/work/go/src/k8s.io/kubernetes> make test WHAT=./cmd/kubeadm/app/preflight/ GOFLAGS=-v
Running tests for APIVersion: v1,admissionregistration.k8s.io/v1alpha1,admission.k8s.io/v1alpha1,apps/v1beta1,apps/v1beta2,apps/v1,authentication.k8s.io/v1,authentication.k8s.io/v1beta1,authorization.k8s.io/v
1,authorization.k8s.io/v1beta1,autoscaling/v1,autoscaling/v2beta1,batch/v1,batch/v1beta1,batch/v2alpha1,certificates.k8s.io/v1beta1,extensions/v1beta1,imagepolicy.k8s.io/v1alpha1,networking.k8s.io/v1,policy/v
1beta1,rbac.authorization.k8s.io/v1,rbac.authorization.k8s.io/v1beta1,rbac.authorization.k8s.io/v1alpha1,scheduling.k8s.io/v1alpha1,settings.k8s.io/v1alpha1,storage.k8s.io/v1beta1,storage.k8s.io/v1,,federatio
n/v1beta1
+++ [1015 12:14:10] Running tests without code coverage
=== RUN   TestRunInitMasterChecks
--- PASS: TestRunInitMasterChecks (0.00s)
=== RUN   TestRunJoinNodeChecks
--- PASS: TestRunJoinNodeChecks (0.00s)
=== RUN   TestRunChecks
--- PASS: TestRunChecks (0.00s)
=== RUN   TestConfigRootCAs
--- PASS: TestConfigRootCAs (0.00s)
=== RUN   TestConfigCertAndKey
--- PASS: TestConfigCertAndKey (0.00s)
=== RUN   TestKubernetesVersionCheck
--- PASS: TestKubernetesVersionCheck (0.00s)
=== RUN   TestHTTPProxyCIDRCheck
--- PASS: TestHTTPProxyCIDRCheck (0.00s)
        checks_test.go:483: Saved environment:  map[NO_PROXY:localhost,127.0.0.1,.fi.corp.com http_proxy:http://proxy.corp.com:911 ftp_proxy:http://proxy.corp.com:911 socks_proxy: gopher_proxy:
 SOCKS_PROXY: https_proxy:http://proxy.corp.com:911 no_proxy:127.0.0.1,localhost]
        checks_test.go:503: http.ProxyFromEnvironment is usable, continue executing test
=== RUN   TestGetKubeletVersion
--- PASS: TestGetKubeletVersion (0.00s)
PASS
ok      k8s.io/kubernetes/cmd/kubeadm/app/preflight     0.052s
kad@kad:/work/go/src/k8s.io/kubernetes>

timothysc

/lgtm

luxas

I didn't review this in detail, but trust the earlier reviewers

/approve no-issue

k8s-github-robot · 2017-10-19T19:02:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kad, luxas, timothysc

Associated issue requirement bypassed by: luxas

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

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

~~cmd/kubeadm/OWNERS~~ [luxas]

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

kad · 2017-10-21T20:22:11Z

/test pull-kubernetes-e2e-gce

k8s-github-robot · 2017-10-21T21:05:47Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-10-21T21:53:40Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 20, 2017

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 Sep 20, 2017

k8s-github-robot assigned luxas and justinsb Sep 20, 2017

k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 20, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 20, 2017

luxas reviewed Sep 26, 2017

View reviewed changes

kad force-pushed the warn-cidrs branch from 77a88ec to ec8ccd6 Compare September 26, 2017 20:00

k8s-ci-robot assigned mattmoyer, mikedanese and timothysc and unassigned luxas Sep 26, 2017

timothysc added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Oct 11, 2017

timothysc added this to the v1.9 milestone Oct 11, 2017

timothysc reviewed Oct 11, 2017

View reviewed changes

kad force-pushed the warn-cidrs branch from ec8ccd6 to b2211eb Compare October 13, 2017 17:04

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 Oct 13, 2017

kad force-pushed the warn-cidrs branch 2 times, most recently from ff218d3 to a0fa6f4 Compare October 14, 2017 19:33

kad changed the title ~~Warn user if Pod/Service networks will be accessed via proxy.~~ WIP: Warn user if Pod/Service networks will be accessed via proxy. Oct 14, 2017

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

kad force-pushed the warn-cidrs branch from a0fa6f4 to 8bc3977 Compare October 14, 2017 20:21

Warn user if Pod/Service networks will be accessed via proxy.

1ed7692

In environments where HTTP proxies are used, it is important to whitelist Pod and Services network ranges in the NO_PROXY variable, so cluster will be properly operational.

kad force-pushed the warn-cidrs branch from 8bc3977 to 1ed7692 Compare October 15, 2017 08:09

kad changed the title ~~WIP: Warn user if Pod/Service networks will be accessed via proxy.~~ Warn user if Pod/Service networks will be accessed via proxy. Oct 15, 2017

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 Oct 15, 2017

timothysc approved these changes Oct 16, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2017

luxas reviewed Oct 19, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2017

k8s-github-robot merged commit abfaada into kubernetes:master Oct 21, 2017

xiangpengzhao mentioned this pull request Nov 13, 2017

Address markdown format symbols in PR release note block? kubernetes/sig-release#29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn user if Pod/Service networks will be accessed via proxy. #52792

Warn user if Pod/Service networks will be accessed via proxy. #52792

kad commented Sep 20, 2017

k8s-ci-robot commented Sep 20, 2017

xiangpengzhao commented Sep 20, 2017

kad commented Sep 20, 2017

luxas left a comment

luxas Sep 26, 2017

kad Sep 26, 2017

luxas Sep 26, 2017

kad Sep 26, 2017

luxas Sep 26, 2017

kad Sep 26, 2017 •

edited

kad commented Sep 26, 2017

luxas commented Sep 26, 2017

kad commented Sep 29, 2017

kad commented Sep 29, 2017

timothysc Oct 11, 2017

kad Oct 11, 2017

kad commented Oct 13, 2017

kad commented Oct 14, 2017

kad commented Oct 14, 2017

liggitt commented Oct 14, 2017

kad commented Oct 14, 2017

kad commented Oct 15, 2017

timothysc left a comment

luxas left a comment

k8s-github-robot commented Oct 19, 2017

kad commented Oct 21, 2017

k8s-github-robot commented Oct 21, 2017

k8s-github-robot commented Oct 21, 2017

Warn user if Pod/Service networks will be accessed via proxy. #52792

Warn user if Pod/Service networks will be accessed via proxy. #52792

Conversation

kad commented Sep 20, 2017

k8s-ci-robot commented Sep 20, 2017

xiangpengzhao commented Sep 20, 2017

kad commented Sep 20, 2017

luxas left a comment

Choose a reason for hiding this comment

luxas Sep 26, 2017

Choose a reason for hiding this comment

kad Sep 26, 2017

Choose a reason for hiding this comment

luxas Sep 26, 2017

Choose a reason for hiding this comment

kad Sep 26, 2017

Choose a reason for hiding this comment

luxas Sep 26, 2017

Choose a reason for hiding this comment

kad Sep 26, 2017 • edited

Choose a reason for hiding this comment

kad commented Sep 26, 2017

luxas commented Sep 26, 2017

kad commented Sep 29, 2017

kad commented Sep 29, 2017

timothysc Oct 11, 2017

Choose a reason for hiding this comment

kad Oct 11, 2017

Choose a reason for hiding this comment

kad commented Oct 13, 2017

kad commented Oct 14, 2017

kad commented Oct 14, 2017

liggitt commented Oct 14, 2017

kad commented Oct 14, 2017

kad commented Oct 15, 2017

timothysc left a comment

Choose a reason for hiding this comment

luxas left a comment

Choose a reason for hiding this comment

k8s-github-robot commented Oct 19, 2017

kad commented Oct 21, 2017

k8s-github-robot commented Oct 21, 2017

k8s-github-robot commented Oct 21, 2017

kad Sep 26, 2017 •

edited