WIP: Allow kops cluster behind a proxy #2749

johnzeringue · 2017-06-14T17:44:00Z

Allow a kops cluster to operate behind a proxy by passing proxy
configuration to addons and nodes. The API will change to be consistent
with the rest of the project, but currently uses the environment
variables CLUSTER_HTTP_PROXY, CLUSTER_HTTPS_PROXY, and
CLUSTER_NO_PROXY available when and where kops is invoked.

Relates to #2481

This change is

k8s-ci-robot · 2017-06-14T17:44:03Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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.

k8s-ci-robot · 2017-06-14T17:44:08Z

Hi @johnzeringue. 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 @k8s-bot 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.

justinsb

Love the approach. I think few helper function could reduce some of the boilerplate code, but more importantly I think we should avoid adding too much new functionality to the bootstrap script, and we should consider storing the proxies in fields rather than env vars that must be passed every time.

justinsb · 2017-06-15T13:57:14Z

nodeup/pkg/bootstrap/install.go

+		buffer.WriteString(os.Getenv("NO_PROXY"))
+		buffer.WriteString("\" ")
+
+		manifest.Set("Service", "Environment", buffer.String())


So this would override the S3_ENDPOINT env. I would if we can refactor to build up a var env map[string]string, and then we manifest.Set("Service", "Environment"... with that only if the map is non-empty.

justinsb · 2017-06-15T13:58:08Z

nodeup/pkg/model/kubecontrollermanager.go

@@ -188,6 +189,14 @@ func (b *KubeControllerManagerBuilder) buildPod() (*v1.Pod, error) {
 		},
 	}

+	if os.Getenv("HTTP_PROXY") != "" || os.Getenv("HTTPS_PROXY") != "" {


We could probably put this into a helper function, in case the logic gets more complicated.

Is there a project convention for helper functions that span nodeup, kops, and protokube?

@johnzeringue putting the helper functions into upup is probably the best place for the code to live. There is not an official convention. If you want to recommend a convention let us know.

justinsb · 2017-06-15T14:03:36Z

pkg/model/resources/nodeup.go

+
+  systemctl daemon-reload
+  systemctl restart docker
+fi


If (if) we can put this logic into nodeup, it's a much safer place for it. The strategy in this file is to get nodeup running, but because doing retries etc is so hard in bash, we try not to put any additional logic into it.

Could we just export the 3 PROXY env vars, and then curl will pick them up?

Could we just export the 3 PROXY env vars, and then curl will pick them up?

That's definitely the way to go

justinsb · 2017-06-15T14:10:15Z

pkg/model/bootstrapscript.go

+		"ProxyEnv": func() string {
+			if os.Getenv("CLUSTER_HTTP_PROXY") != "" || os.Getenv("CLUSTER_HTTPS_PROXY") != "" {
+				return fmt.Sprintf("export HTTP_PROXY=%s\nexport HTTPS_PROXY=%s\nexport NO_PROXY=%s\n",
+					os.Getenv("CLUSTER_HTTP_PROXY"),


So my understanding is that we would have to set these env vars every time we called kops update.

I think this could instead be a couple of fields; it could either be a new top-level object (like authorization) or we could put it into the networking object. I think my preference would be a new top level object, because the networking object is currently an enum-style, where we only expect a single value to be set.

If you agree I'm happy to assist / write a doc & WIP PR on how to do this - let me know.

Yeah, the environment variables are terrible. I just haven't yet taken the time to familiarize myself with apimachinery.

Looking at it now, I think it should be a new top level object. I don't know whether proxy or proxying is preferred as the object name, but the fields should just be httpProxy, httpsProxy, and noProxy.

I'm not sure what kind of docs are needed, but I'd be happy to work with you

@johnzeringue here is some documentation that will help you with adding the new values into the API. https://github.com/kubernetes/kops/blob/master/docs/development/api_updates.md

I aggree with @justinsb that having these values as a new struct in the ClusterSpec makes a lot of sense. Here is an example of a struct that lives in the ClusterSpec https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/cluster.go#L225

Allow a kops cluster to operate behind a proxy by passing proxy configuration to addons and nodes. The API will change to be consistent with the rest of the project, but currently uses the environment variables `CLUSTER_HTTP_PROXY`, `CLUSTER_HTTPS_PROXY`, and `CLUSTER_NO_PROXY` available when and where kops is invoked. Relates to kubernetes#2481

chrislovecnm · 2017-06-15T18:39:12Z

@johnzeringue can you sign the CLA?

johnzeringue · 2017-06-15T19:36:29Z

@chrislovecnm, I'm working on getting my employer to sign.

DerekV · 2017-06-19T23:08:06Z

I've got my own version up now as well finally #2777
I'm sorry I didn't get it up earlier.
Hopefully between the two sets we can find the cleanest solution.

chrislovecnm

Looks great, if you have questions let us know. Mostly concerned about having variables that are unset. Does it still work with values that are not set?

chrislovecnm · 2017-06-30T17:09:40Z

nodeup/pkg/model/kubeapiserver.go

@@ -160,6 +161,14 @@ func (b *KubeAPIServerBuilder) buildPod() (*v1.Pod, error) {
 		},
 	}

+	if os.Getenv("HTTP_PROXY") != "" || os.Getenv("HTTPS_PROXY") != "" {
+		container.Env = []v1.EnvVar{
+			{Name: "HTTP_PROXY", Value: os.Getenv("HTTP_PROXY")},


Do we want to set this if it does not exist? This will set both env variables regardless if one does not exist.

In my experience export HTTP_PROXY= functions the same as unset HTTP_PROXY. There's an argument to be made for easy debugging (although maybe it goes both ways). Maybe it's neater with only the ones that get set. If anyone has a strong opinion, it's a quick change for me.

chrislovecnm · 2017-06-30T17:10:50Z

nodeup/pkg/model/kubecontrollermanager.go

@@ -188,6 +189,14 @@ func (b *KubeControllerManagerBuilder) buildPod() (*v1.Pod, error) {
 		},
 	}

+	if os.Getenv("HTTP_PROXY") != "" || os.Getenv("HTTPS_PROXY") != "" {


@johnzeringue putting the helper functions into upup is probably the best place for the code to live. There is not an official convention. If you want to recommend a convention let us know.

chrislovecnm · 2017-06-30T17:15:49Z

pkg/model/bootstrapscript.go

+		"ProxyEnv": func() string {
+			if os.Getenv("CLUSTER_HTTP_PROXY") != "" || os.Getenv("CLUSTER_HTTPS_PROXY") != "" {
+				return fmt.Sprintf("export HTTP_PROXY=%s\nexport HTTPS_PROXY=%s\nexport NO_PROXY=%s\n",
+					os.Getenv("CLUSTER_HTTP_PROXY"),


@johnzeringue here is some documentation that will help you with adding the new values into the API. https://github.com/kubernetes/kops/blob/master/docs/development/api_updates.md

I aggree with @justinsb that having these values as a new struct in the ClusterSpec makes a lot of sense. Here is an example of a struct that lives in the ClusterSpec https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/cluster.go#L225

chrislovecnm · 2017-06-30T17:23:46Z

We need a rebase, I know it is a pain, but if you could sign the CLA please.

johnzeringue · 2017-07-01T00:07:50Z

I'll rebase the next chance I get to work on the code (I'm OOO for a couple weeks). The CLA should be signed now. It looks like the robot figured that out after you commented.

k8s-github-robot · 2017-07-20T22:23:12Z

@johnzeringue PR needs rebase

k8s-github-robot · 2017-07-26T20:31:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: johnzeringue
We suggest the following additional approver: justinsb

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

Associated issue: 2481

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

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

OWNERS

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

johnzeringue · 2017-07-31T13:04:56Z

Closing in favor of #2777

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jun 14, 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 Jun 14, 2017

johnzeringue force-pushed the cluster-behind-proxy branch from 0d78eec to 3192c2e Compare June 14, 2017 19:29

justinsb reviewed Jun 15, 2017

View reviewed changes

johnzeringue force-pushed the cluster-behind-proxy branch from 3192c2e to c9dab72 Compare June 15, 2017 15:02

chrislovecnm suggested changes Jun 30, 2017

View reviewed changes

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 30, 2017

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

johnzeringue closed this Jul 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Allow kops cluster behind a proxy #2749

WIP: Allow kops cluster behind a proxy #2749

johnzeringue commented Jun 14, 2017 •

edited by k8s-reviewable

Loading

k8s-ci-robot commented Jun 14, 2017

k8s-ci-robot commented Jun 14, 2017

justinsb left a comment

justinsb Jun 15, 2017

johnzeringue Jun 15, 2017

justinsb Jun 15, 2017

johnzeringue Jun 15, 2017

chrislovecnm Jun 30, 2017

justinsb Jun 15, 2017

johnzeringue Jun 15, 2017

justinsb Jun 15, 2017

johnzeringue Jun 15, 2017

chrislovecnm Jun 30, 2017

chrislovecnm commented Jun 15, 2017

johnzeringue commented Jun 15, 2017

DerekV commented Jun 19, 2017

chrislovecnm left a comment

chrislovecnm Jun 30, 2017

johnzeringue Jul 1, 2017

chrislovecnm Jun 30, 2017

chrislovecnm Jun 30, 2017

chrislovecnm commented Jun 30, 2017

johnzeringue commented Jul 1, 2017

k8s-github-robot commented Jul 20, 2017

k8s-github-robot commented Jul 26, 2017

johnzeringue commented Jul 31, 2017

WIP: Allow kops cluster behind a proxy #2749

WIP: Allow kops cluster behind a proxy #2749

Conversation

johnzeringue commented Jun 14, 2017 • edited by k8s-reviewable Loading

k8s-ci-robot commented Jun 14, 2017

k8s-ci-robot commented Jun 14, 2017

justinsb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm commented Jun 15, 2017

johnzeringue commented Jun 15, 2017

DerekV commented Jun 19, 2017

chrislovecnm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm commented Jun 30, 2017

johnzeringue commented Jul 1, 2017

k8s-github-robot commented Jul 20, 2017

k8s-github-robot commented Jul 26, 2017

johnzeringue commented Jul 31, 2017

johnzeringue commented Jun 14, 2017 •

edited by k8s-reviewable

Loading