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

Add support for cluster using http forward proxy #2481 #2777

Merged
merged 1 commit into from
Aug 7, 2017
Merged

Add support for cluster using http forward proxy #2481 #2777

merged 1 commit into from
Aug 7, 2017

Conversation

DerekV
Copy link
Contributor

@DerekV DerekV commented Jun 19, 2017

Adds support for running a cluster where access to external resources must be done through an http forward proxy. This adds a new element to the ClusterSpec, EgressProxy, and then sets up environment variables where appropriate. Access to API servers is additionally assumed to be done through the proxy, in particular this is necessary for AWS VPCs with private topology and egress by proxy (no NAT), at least until Amazon implements VPC Endpoints for the APIs.

Additionally, see my notes in #2481

TODOs


This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

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.


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

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

@k8s-ci-robot 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 19, 2017
@DerekV
Copy link
Contributor Author

DerekV commented Jun 19, 2017

TODO: Add documentation
CLA on its way.

@@ -91,7 +91,7 @@ func (_ *AttachISO) CheckChanges(a, e, changes *AttachISO) error {

// RenderVSphere executes the actual task logic, for vSphere cloud.
func (_ *AttachISO) RenderVSphere(t *vsphere.VSphereAPITarget, a, e, changes *AttachISO) error {
startupScript, err := changes.BootstrapScript.ResourceNodeUp(changes.IG)
startupScript, err := changes.BootstrapScript.ResourceNodeUp(changes.IG,nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will need to open a new ticket to fill this in to add support for VMWare.

url += "@"
}
url += httpProxy.Host + ":" + strconv.Itoa(httpProxy.Port)
//envs["http_proxy"] = url
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove

@DerekV
Copy link
Contributor Author

DerekV commented Jun 19, 2017

TODO - We'll want to open an issue to add this support for vpshere, as I am just passing a nil in that path (I have no cluster to validate on currently)
TODO - all the code around username and password for the proxy, since I didn't test it, and open a new ticket to support proxy auth

@@ -1476,7 +1546,6 @@ func autoConvert_kops_KubeProxyConfig_To_v1alpha2_KubeProxyConfig(in *kops.KubeP
out.CPURequest = in.CPURequest
out.LogLevel = in.LogLevel
out.ClusterCIDR = in.ClusterCIDR
// WARNING: in.HostnameOverride requires manual conversion: does not exist in peer-type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not explicitly remove this comment from this generated file, should re-add it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Already removed in master

@k8s-ci-robot 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 20, 2017
@chrislovecnm
Copy link
Contributor

@k8s-bot ok to test

@k8s-ci-robot 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 Jun 20, 2017
@chrislovecnm
Copy link
Contributor

Travis CI is failing btw

@DerekV
Copy link
Contributor Author

DerekV commented Jun 20, 2017

@chrislovecnm looking at Travis..

Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Looks great. Some comments.

@@ -1476,7 +1546,6 @@ func autoConvert_kops_KubeProxyConfig_To_v1alpha2_KubeProxyConfig(in *kops.KubeP
out.CPURequest = in.CPURequest
out.LogLevel = in.LogLevel
out.ClusterCIDR = in.ClusterCIDR
// WARNING: in.HostnameOverride requires manual conversion: does not exist in peer-type
Copy link
Contributor

Choose a reason for hiding this comment

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

Already removed in master

@@ -349,6 +352,18 @@ type ClusterSubnetSpec struct {
Type SubnetType `json:"type,omitempty"`
}

type EgressProxySpec struct {
HTTPProxy HTTPProxySpec `json:"httpProxy,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have HTTP / HTTPS / and S3?

Copy link
Contributor

Choose a reason for hiding this comment

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

@johnzeringue here is an example of EgressProxySpec that is set for HTTP proxy. Both you and @DerekV were working on the same problem at the same time. I think a combination of both your and his PR would be awesome.

Guys let me know if I can assist.

Copy link
Contributor

Choose a reason for hiding this comment

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

HTTPProxySpec to HTTPProxy basically don't use the word 'Spec'

ProxyExcludes string `json:"excludes,omitempty"`
}

type HTTPProxySpec struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we abstract this to a Proxy Object, and then reuse for seperate HTTP and HTTPS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Supporting a seperate HTTPS is why I constructed the objects this way. I took it out when I was cleaning up just because I wasn't using it, so it was unexecuted code. Easy to put back in.

@@ -349,6 +352,18 @@ type ClusterSubnetSpec struct {
Type SubnetType `json:"type,omitempty"`
}

type EgressProxySpec struct {
HTTPProxy HTTPProxySpec `json:"httpProxy,omitempty"`
ProxyExcludes string `json:"excludes,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to figure out how to exclude the internal AWS urls as well, if the cluster is in AWS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I omit them by setting ProxyExcludes. I was going to include documentation about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be part of the API or can we configure this automatically for users? My idea was that the proxy is only for the benefit of the cluster and not any applications running in it, so I'm struggling to think of edge cases that would need manual excludes.

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be part of the API, many enterprises will have internal URLs that are not proxied.

httpProxyUrl += ps.HTTPProxy.Host + ":" + strconv.Itoa(ps.HTTPProxy.Port)
scriptSnippet =
"export http_proxy=" + httpProxyUrl + "\n" +
"export https_proxy=${http_proxy}\n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need to separate values for http and https ... thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only needed if the user has seperate servers or ports for http and https proxies. If you only specify one, most clients will try to used that one for both. I'm guessing the more common configuration is to have one proxy and port for both protocols. However, to support the full gamut of possible configurations, you would need both, and also ftp_proxy as well. My first iteration had this, but I removed it since it wasn't needed, and thought it could be added back if someone requested it.

@@ -38,11 +38,17 @@ spec:
{{ range $arg := DnsControllerArgv }}
- "{{ $arg }}"
{{ end }}
env:
Copy link
Contributor

Choose a reason for hiding this comment

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

we need an if statement here. If no ProxyEnv don't print this block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

{{- if .EgressProxy -}}
        env:
{{ range $name, $value := ProxyEnv }}
        - name: {{ $name }}
          value: {{ $value }}
{{ end }}
{{- end -}}

Works btw.

}
url += "@"
}
url += httpProxy.Host + ":" + strconv.Itoa(httpProxy.Port)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can port be optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would it default to?

Copy link
Contributor

Choose a reason for hiding this comment

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

80 for http, 443 for ssl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In practice, it rarely works this way, and ports are nearly always specified in proxy configurations. Both http_proxy and https_proxy work over scheme http to connect to the proxy (typically, some digging tells me https for client-to-proxy is possible but I am not sure how common it is in practice) For example, curl will, if the port is ommited, assume port 1080. wget is undocumented (seems it might use the default for the url scheme in the proxy), and likewise with golang net/http. I see two possible approaches: We could check check for nil, making the port optional in the configuration and here leaving colon and port off if it was omitted in the cluster spec, which would leave behavior up to the specific client and fate. The other option is to require an explicit port for a valid configuration.


type EgressProxySpec struct {
HTTPProxy HTTPProxySpec `json:"httpProxy,omitempty"`
ProxyExcludes string `json:"excludes,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

We are going to need some validation around this. See the validation folder in this pkg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

Copy link
Contributor

Choose a reason for hiding this comment

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

File an issue to do validation.

@chrislovecnm
Copy link
Contributor

/retest

}
if proxies.ProxyExcludes != "" {
x := v1.EnvVar{
Name: "no_proxy",
Copy link
Contributor

Choose a reason for hiding this comment

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

My coworker noted that we had issues with the case variable names in our setup. I think it's okay in this spot but if you're having issues with no_proxy elsewhere try NO_PROXY.

- key: "CriticalAddonsOnly"
operator: "Exists"
serviceAccountName: kube-dns-autoscaler

---

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted the file, apparently I didn't need to mess with it.

Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

yaml template needs if statement

@@ -38,11 +38,17 @@ spec:
{{ range $arg := DnsControllerArgv }}
- "{{ $arg }}"
{{ end }}
env:
Copy link
Contributor

Choose a reason for hiding this comment

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

{{- if .EgressProxy -}}
        env:
{{ range $name, $value := ProxyEnv }}
        - name: {{ $name }}
          value: {{ $value }}
{{ end }}
{{- end -}}

Works btw.

@chrislovecnm
Copy link
Contributor

I fixed the cloudformation unit test here: chrislovecnm@cbad385#diff-ce8b64820650adfbe4912e5395227992

@chrislovecnm
Copy link
Contributor

@DerekV the cluster is not starting during e2e. Have you run a local e2e test with a full build that does not use a proxy?

@DerekV
Copy link
Contributor Author

DerekV commented Jul 19, 2017

Two questions:
Should we add an e2e test (and how does one go about that?)
Should we support proxy auth in this PR or move that to a new PR?

@druidsbane
Copy link

After testing it appears that no_proxy needs to be replaced with NO_PROXY. kube-dns and other packages fail to start up properly otherwise. Log scraping and possibly health-checks also fail.

I'd also vote to have all the other variables set to all uppercase to match standard conventions except in /etc/wgetrc where it is documented to be lowercase.

@chrislovecnm
Copy link
Contributor

Thanks @druidsbane!

@chrislovecnm
Copy link
Contributor

Do we have a list of components that need the http proxy config?

@chrislovecnm
Copy link
Contributor

File a ticket for unit tests, we won't be able to have a e2e test for this. We can sorta do an integration test, but we need to look at how.

Can we comment out the auth code or pull it. I would rather have another PR where we add auth in. Does anyone need auth? I do not.

Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

FInally had a chance to give this PR a good once over.

@@ -349,6 +352,18 @@ type ClusterSubnetSpec struct {
Type SubnetType `json:"type,omitempty"`
}

type EgressProxySpec struct {
HTTPProxy HTTPProxySpec `json:"httpProxy,omitempty"`
ProxyExcludes string `json:"excludes,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be part of the API, many enterprises will have internal URLs that are not proxied.

@@ -349,6 +352,18 @@ type ClusterSubnetSpec struct {
Type SubnetType `json:"type,omitempty"`
}

type EgressProxySpec struct {
HTTPProxy HTTPProxySpec `json:"httpProxy,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

HTTPProxySpec to HTTPProxy basically don't use the word 'Spec'

buffer.WriteString(" -e ftp_proxy=")
buffer.WriteString(os.Getenv("ftp_proxy"))
buffer.WriteString(" ")
buffer.WriteString(" -e no_proxy=")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make update the case? Where are we using NO_PROXY

type HTTPProxySpec struct {
Host string `json:"host,omitempty"`
Port int `json:"port,omitempty"`
User string `json:"user,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we pull User and Password for now? You can comment out.


type EgressProxySpec struct {
HTTPProxy HTTPProxySpec `json:"httpProxy,omitempty"`
ProxyExcludes string `json:"excludes,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

File an issue to do validation.

httpProxy := proxies.HTTPProxy
if httpProxy.Host != "" {
url := "http://"
if httpProxy.User != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we remove auth stuff?

if httpProxy.Host != "" {
url := "http://"
if httpProxy.User != "" {
url += httpProxy.User
Copy link
Contributor

Choose a reason for hiding this comment

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

pull auth please


if ps != nil && ps.HTTPProxy.Host != "" {
httpProxyUrl := "http://"
if ps.HTTPProxy.User != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove auth

}
httpProxyUrl += ps.HTTPProxy.Host + ":" + strconv.Itoa(ps.HTTPProxy.Port)
scriptSnippet =
"export http_proxy=" + httpProxyUrl + "\n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

@andrewsykim should we use a bytes.Buffer here? 🤔

"echo DefaultEnvironment=\\\"http_proxy=${http_proxy}\\\" \\\"https_proxy=${http_proxy}\\\" \\\"ftp_proxy=${http_proxy}\\\" \\\"no_proxy=${no_proxy}\\\" >> /etc/systemd/system.conf\n" +
"systemctl daemon-reload\n" +
"systemctl daemon-reexec\n" +
"echo \"Acquire::http::Proxy \\\"${http_proxy}\\\";\" > /etc/apt/apt.conf.d/30proxy\n\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, we need to support more than just Debian. kops supports RHEL, Centos, Ubuntu, CoreOS, and Debian. We are probably going to need to move this into nodeup. We do not check the OS type when executing this, and this may be getting messy enough that we may need nodeup to do this. Let me sleep on it, and can you ping me tomorrow via slack?

Bottom line we need to support apt, yum and whatever the heck CoreOS uses.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am also reading https://docs.docker.com/engine/admin/systemd/#httphttps-proxy

  1. /etc/default/docker may not exist unless docker is installed. We do not have that guarantee as people use there own images.
  2. Since you echo'ed it into /etc/systemd/system.conf do we even need the docker config settings?

Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Another question for you, not sure what is going on in protokube.

@@ -279,5 +280,21 @@ func (t *ProtokubeBuilder) ProtokubeEnvironmentVariables() string {
buffer.WriteString(" ")
}

buffer.WriteString(" -e http_proxy=")
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need to test if we need to write this out? A bit confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should skip if it isn't set.

@chrislovecnm
Copy link
Contributor

@DerekV take a peek at chrislovecnm@cfe1427#diff-d510984f3211ef071e61626401f1f793 I have not tested it yet, but I am thinking it should support different distros.

@chrislovecnm
Copy link
Contributor

Fix an issue for you #3032

Copy link
Contributor Author

@DerekV DerekV left a comment

Choose a reason for hiding this comment

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

We should use the EgressProxySpec object instead of reading from the os environment.

return nil
}

httpProxy := os.Getenv("http_proxy")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrislovecnm Let's change this back to using the EgressProxySpec?

@@ -91,7 +91,7 @@ func (_ *AttachISO) CheckChanges(a, e, changes *AttachISO) error {

// RenderVSphere executes the actual task logic, for vSphere cloud.
func (_ *AttachISO) RenderVSphere(t *vsphere.VSphereAPITarget, a, e, changes *AttachISO) error {
startupScript, err := changes.BootstrapScript.ResourceNodeUp(changes.IG)
startupScript, err := changes.BootstrapScript.ResourceNodeUp(changes.IG, nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

envs := map[string]string{}
proxies := tf.cluster.Spec.EgressProxy
if proxies == nil {
return envs, nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

EgressProxy is optional, having it be nil is not an error to me, returning an empty map means this function could be composed with others that return environment variables.

return nil
}

httpProxy := os.Getenv("http_proxy")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't we use the EgressProxySpec here?

@@ -279,5 +282,24 @@ func (t *ProtokubeBuilder) ProtokubeEnvironmentVariables() string {
buffer.WriteString(" ")
}

if t.Cluster.Spec.EgressProxy != nil {
proxy := os.Getenv("http_proxy")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use EgressProxy not os.Getenv

@k8s-github-robot
Copy link

@DerekV PR needs rebase

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 3, 2017
@DerekV
Copy link
Contributor Author

DerekV commented Aug 3, 2017

@chrislovecnm I'm all set, do you see anything else?

@justinsb
Copy link
Member

justinsb commented Aug 4, 2017

This looks really good :-)

I had a few minor questions. The only big one is - as always - on the API itself (the API being the thing that is very hard to change, hence I am such a stickler for it!). Should egressProxy be egress, so that this is comparable to the egress option we've defined on subnets. i.e. is this a replacement for a NAT gateway? Or would people use an HTTP proxy and a NAT gateway (or VPN gateway)?

Copy link
Member

@justinsb justinsb left a comment

Choose a reason for hiding this comment

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

Looking good!


``` yaml
spec:
egressProxy:
Copy link
Member

Choose a reason for hiding this comment

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

We do have egress which is where we configure a natGateway. Currently that is defined on the subnet level, because of the limitations of NAT subnets. We should consider whether this is the same thing, and whether we want to expand egress to cover this. My knee-jerk is yes...

(In practice, we only have to preserve serialized API compatibility, so this means renaming the field. We can harmonize the two fields later.)

Copy link
Member

Choose a reason for hiding this comment

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

On the flip-side, I guess the argument is that we could have both egress and egressProxy. i.e. we want to use an HTTP proxy, but we also want to tunnel through a VPN / gateway.

I'm not the expert here - if http proxy & gateway are both possible, then this makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think conceivably the http proxy would be in addition to one or more of these other options.

port: 3128
```

Currently we assume the same configuration for http and https traffic.
Copy link
Member

Choose a reason for hiding this comment

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

👍 easy to change later (add an httpsProxy field), great to call out the limitation explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! That was my intent. I opened an issue to track it (#3069)

httpProxy:
host: proxy.corp.local
port: 3128
excludes: corp.local,internal.corp.com
Copy link
Member

Choose a reason for hiding this comment

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

So this is only going to work for layer 7 proxies, and if we - for example - but a NAT gateway in here it would not work as expected. That said I think this is fine, it can be a validation error. YAGNI :-)

@@ -27,3 +34,31 @@ func s(v string) *string {
func i64(v int64) *int64 {
return fi.Int64(v)
}

func getProxyEnvVars(proxies *kops.EgressProxySpec) []v1.EnvVar {
glog.Info("in getProxyEnvVars")
Copy link
Member

Choose a reason for hiding this comment

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

Suggest removal of this one (it could be v(8), but I think just remove)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Crap! I meant to remove those tracer logs, was troubleshooting something this weekend. Thanks

func getProxyEnvVars(proxies *kops.EgressProxySpec) []v1.EnvVar {
glog.Info("in getProxyEnvVars")
if proxies == nil {
glog.Info("proxies is == nil, returning empty list")
Copy link
Member

Choose a reason for hiding this comment

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

v(8) or remove?

// TODO double check that all the code does this
// TODO move this into a validate so we can enforce the string syntax
if !strings.HasPrefix(httpProxyUrl, "http://") {
httpProxyUrl = "http://"
Copy link
Member

Choose a reason for hiding this comment

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

httpProxyUrl is always going to be empty at this point in the code, we created the variable a few lines above...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. There's something else here that is more subtle... the egressProxy specifies a "Host" not a "URL". This means the transport is not really expected to be specified in the host string. In most cases, the transport from client to proxy is http, so for this PR this is what we assume, though it is possible in some situations to handle encryption from client to proxy, via https. To support this feature, we would just add an api for HTTPProxy.Transport. Meanwhile, what I think we wanted here was for httpProxyUrl to be ps.HTTPProxy.Host, I'll make that change now.

buffer.WriteString("export no_proxy=" + ps.ProxyExcludes + "\n")
buffer.WriteString("export NO_PROXY=${no_proxy}\n")

// TODO move the rest of this configuration work to nodeup
Copy link
Member

Choose a reason for hiding this comment

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

Yes - I am very wary of doing stuff in the bootstrap script.

I guess the argument here is that we need the proxy to download nodeup. That makes sense.

We should move the rest of this to nodeup asap though...

@@ -121,7 +121,7 @@
"Ref": "AWSEC2SecurityGroupnodesminimalexamplecom"
}
],
"UserData": "IyEvYmluL2Jhc2gKIyBDb3B5cmlnaHQgMjAxNiBUaGUgS3ViZXJuZXRlcyBBdXRob3JzIEFsbCByaWdodHMgcmVzZXJ2ZWQuCiMKIyBMaWNlbnNlZCB1bmRlciB0aGUgQXBhY2hlIExpY2Vuc2UsIFZlcnNpb24gMi4wICh0aGUgIkxpY2Vuc2UiKTsKIyB5b3UgbWF5IG5vdCB1c2UgdGhpcyBmaWxlIGV4Y2VwdCBpbiBjb21wbGlhbmNlIHdpdGggdGhlIExpY2Vuc2UuCiMgWW91IG1heSBvYnRhaW4gYSBjb3B5IG9mIHRoZSBMaWNlbnNlIGF0CiMKIyAgICAgaHR0cDovL3d3dy5hcGFjaGUub3JnL2xpY2Vuc2VzL0xJQ0VOU0UtMi4wCiMKIyBVbmxlc3MgcmVxdWlyZWQgYnkgYXBwbGljYWJsZSBsYXcgb3IgYWdyZWVkIHRvIGluIHdyaXRpbmcsIHNvZnR3YXJlCiMgZGlzdHJpYnV0ZWQgdW5kZXIgdGhlIExpY2Vuc2UgaXMgZGlzdHJpYnV0ZWQgb24gYW4gIkFTIElTIiBCQVNJUywKIyBXSVRIT1VUIFdBUlJBTlRJRVMgT1IgQ09ORElUSU9OUyBPRiBBTlkgS0lORCwgZWl0aGVyIGV4cHJlc3Mgb3IgaW1wbGllZC4KIyBTZWUgdGhlIExpY2Vuc2UgZm9yIHRoZSBzcGVjaWZpYyBsYW5ndWFnZSBnb3Zlcm5pbmcgcGVybWlzc2lvbnMgYW5kCiMgbGltaXRhdGlvbnMgdW5kZXIgdGhlIExpY2Vuc2UuCgpzZXQgLW8gZXJyZXhpdApzZXQgLW8gbm91bnNldApzZXQgLW8gcGlwZWZhaWwKCk5PREVVUF9VUkw9aHR0cHM6Ly9rdWJldXB2Mi5zMy5hbWF6b25hd3MuY29tL2tvcHMvMS41LjAvbGludXgvYW1kNjQvbm9kZXVwCk5PREVVUF9IQVNIPQoKCgoKZnVuY3Rpb24gZW5zdXJlLWluc3RhbGwtZGlyKCkgewogIElOU1RBTExfRElSPSIvdmFyL2NhY2hlL2t1YmVybmV0ZXMtaW5zdGFsbCIKICAjIE9uIENvbnRhaW5lck9TLCB3ZSBpbnN0YWxsIHRvIC92YXIvbGliL3Rvb2xib3ggaW5zdGFsbCAoYmVjYXVzZSBvZiBub2V4ZWMpCiAgaWYgW1sgLWQgL3Zhci9saWIvdG9vbGJveCBdXTsgdGhlbgogICAgSU5TVEFMTF9ESVI9Ii92YXIvbGliL3Rvb2xib3gva3ViZXJuZXRlcy1pbnN0YWxsIgogIGZpCiAgbWtkaXIgLXAgJHtJTlNUQUxMX0RJUn0KICBjZCAke0lOU1RBTExfRElSfQp9CgojIFJldHJ5IGEgZG93bmxvYWQgdW50aWwgd2UgZ2V0IGl0LiBUYWtlcyBhIGhhc2ggYW5kIGEgc2V0IG9mIFVSTHMuCiMKIyAkMSBpcyB0aGUgc2hhMSBvZiB0aGUgVVJMLiBDYW4gYmUgIiIgaWYgdGhlIHNoYTEgaXMgdW5rbm93bi4KIyAkMisgYXJlIHRoZSBVUkxzIHRvIGRvd25sb2FkLgpkb3dubG9hZC1vci1idXN0KCkgewogIGxvY2FsIC1yIGhhc2g9IiQxIgogIHNoaWZ0IDEKCiAgdXJscz0oICQqICkKICB3aGlsZSB0cnVlOyBkbwogICAgZm9yIHVybCBpbiAiJHt1cmxzW0BdfSI7IGRvCiAgICAgIGxvY2FsIGZpbGU9IiR7dXJsIyMqL30iCiAgICAgIHJtIC1mICIke2ZpbGV9IgogICAgICBpZiAhIGN1cmwgLWYgLS1pcHY0IC1MbyAiJHtmaWxlfSIgLS1jb25uZWN0LXRpbWVvdXQgMjAgLS1yZXRyeSA2IC0tcmV0cnktZGVsYXkgMTAgIiR7dXJsfSI7IHRoZW4KICAgICAgICBlY2hvICI9PSBGYWlsZWQgdG8gZG93bmxvYWQgJHt1cmx9LiBSZXRyeWluZy4gPT0iCiAgICAgIGVsaWYgW1sgLW4gIiR7aGFzaH0iIF1dICYmICEgdmFsaWRhdGUtaGFzaCAiJHtmaWxlfSIgIiR7aGFzaH0iOyB0aGVuCiAgICAgICAgZWNobyAiPT0gSGFzaCB2YWxpZGF0aW9uIG9mICR7dXJsfSBmYWlsZWQuIFJldHJ5aW5nLiA9PSIKICAgICAgZWxzZQogICAgICAgIGlmIFtbIC1uICIke2hhc2h9IiBdXTsgdGhlbgogICAgICAgICAgZWNobyAiPT0gRG93bmxvYWRlZCAke3VybH0gKFNIQTEgPSAke2hhc2h9KSA9PSIKICAgICAgICBlbHNlCiAgICAgICAgICBlY2hvICI9PSBEb3dubG9hZGVkICR7dXJsfSA9PSIKICAgICAgICBmaQogICAgICAgIHJldHVybgogICAgICBmaQogICAgZG9uZQoKICAgIGVjaG8gIkFsbCBkb3dubG9hZHMgZmFpbGVkOyBzbGVlcGluZyBiZWZvcmUgcmV0cnlpbmciCiAgICBzbGVlcCA2MAogIGRvbmUKfQoKdmFsaWRhdGUtaGFzaCgpIHsKICBsb2NhbCAtciBmaWxlPSIkMSIKICBsb2NhbCAtciBleHBlY3RlZD0iJDIiCiAgbG9jYWwgYWN0dWFsCgogIGFjdHVhbD0kKHNoYTFzdW0gJHtmaWxlfSB8IGF3ayAneyBwcmludCAkMSB9JykgfHwgdHJ1ZQogIGlmIFtbICIke2FjdHVhbH0iICE9ICIke2V4cGVjdGVkfSIgXV07IHRoZW4KICAgIGVjaG8gIj09ICR7ZmlsZX0gY29ycnVwdGVkLCBzaGExICR7YWN0dWFsfSBkb2Vzbid0IG1hdGNoIGV4cGVjdGVkICR7ZXhwZWN0ZWR9ID09IgogICAgcmV0dXJuIDEKICBmaQp9CgpmdW5jdGlvbiBzcGxpdC1jb21tYXMoKSB7CiAgZWNobyAkMSB8IHRyICIsIiAiXG4iCn0KCmZ1bmN0aW9uIHRyeS1kb3dubG9hZC1yZWxlYXNlKCkgewogICMgVE9ETyh6bWVybHlubik6IE5vdyB3ZSBSRUFMTFkgaGF2ZSBubyBleGN1c2Ugbm90IHRvIGRvIHRoZSByZWJvb3QKICAjIG9wdGltaXphdGlvbi4KCiAgbG9jYWwgLXIgbm9kZXVwX3VybHM9KCAkKHNwbGl0LWNvbW1hcyAiJHtOT0RFVVBfVVJMfSIpICkKICBsb2NhbCAtciBub2RldXBfZmlsZW5hbWU9IiR7bm9kZXVwX3VybHNbMF0jIyovfSIKICBpZiBbWyAtbiAiJHtOT0RFVVBfSEFTSDotfSIgXV07IHRoZW4KICAgIGxvY2FsIC1yIG5vZGV1cF9oYXNoPSIke05PREVVUF9IQVNIfSIKICBlbHNlCiAgIyBUT0RPOiBSZW1vdmU/CiAgICBlY2hvICJEb3dubG9hZGluZyBzaGExIChub3QgZm91bmQgaW4gZW52KSIKICAgIGRvd25sb2FkLW9yLWJ1c3QgIiIgIiR7bm9kZXVwX3VybHNbQF0vJS8uc2hhMX0iCiAgICBsb2NhbCAtciBub2RldXBfaGFzaD0kKGNhdCAiJHtub2RldXBfZmlsZW5hbWV9LnNoYTEiKQogIGZpCgogIGVjaG8gIkRvd25sb2FkaW5nIG5vZGV1cCAoJHtub2RldXBfdXJsc1tAXX0pIgogIGRvd25sb2FkLW9yLWJ1c3QgIiR7bm9kZXVwX2hhc2h9IiAiJHtub2RldXBfdXJsc1tAXX0iCgogIGNobW9kICt4IG5vZGV1cAp9CgpmdW5jdGlvbiBkb3dubG9hZC1yZWxlYXNlKCkgewogICMgSW4gY2FzZSBvZiBmYWlsdXJlIGNoZWNraW5nIGludGVncml0eSBvZiByZWxlYXNlLCByZXRyeS4KICB1bnRpbCB0cnktZG93bmxvYWQtcmVsZWFzZTsgZG8KICAgIHNsZWVwIDE1CiAgICBlY2hvICJDb3VsZG4ndCBkb3dubG9hZCByZWxlYXNlLiBSZXRyeWluZy4uLiIKICBkb25lCgogIGVjaG8gIlJ1bm5pbmcgbm9kZXVwIgogICMgV2UgY2FuJ3QgcnVuIGluIHRoZSBmb3JlZ3JvdW5kIGJlY2F1c2Ugb2YgaHR0cHM6Ly9naXRodWIuY29tL2RvY2tlci9kb2NrZXIvaXNzdWVzLzIzNzkzCiAgKCBjZCAke0lOU1RBTExfRElSfTsgLi9ub2RldXAgLS1pbnN0YWxsLXN5c3RlbWQtdW5pdCAtLWNvbmY9JHtJTlNUQUxMX0RJUn0va3ViZV9lbnYueWFtbCAtLXY9OCAgKQp9CgojIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMKCi9iaW4vc3lzdGVtZC1tYWNoaW5lLWlkLXNldHVwIHx8IGVjaG8gImZhaWxlZCB0byBzZXQgdXAgZW5zdXJlIG1hY2hpbmUtaWQgY29uZmlndXJlZCIKCmVjaG8gIj09IG5vZGV1cCBub2RlIGNvbmZpZyBzdGFydGluZyA9PSIKZW5zdXJlLWluc3RhbGwtZGlyCgpjYXQgPiBrdWJlX2Vudi55YW1sIDw8IF9fRU9GX0tVQkVfRU5WCkFzc2V0czoKLSA3ZDcwZTA5MDk1MTQ4NmNhZTUyZDlhODJiN2FhZjUwNTZmODRmOGVkQGh0dHBzOi8vc3RvcmFnZS5nb29nbGVhcGlzLmNvbS9rdWJlcm5ldGVzLXJlbGVhc2UvcmVsZWFzZS92MS40LjYvYmluL2xpbnV4L2FtZDY0L2t1YmVsZXQKLSA5YWRjZDEyMGZkYjdhZDZlNjRjMDYxZTU2YTA1ZmVmYzEyZTk2MThiQGh0dHBzOi8vc3RvcmFnZS5nb29nbGVhcGlzLmNvbS9rdWJlcm5ldGVzLXJlbGVhc2UvcmVsZWFzZS92MS40LjYvYmluL2xpbnV4L2FtZDY0L2t1YmVjdGwKLSAxOWQ0OWY3YjJiOTljZDI0OTNkNWFlMGFjZTg5NmM2NGUyODljY2JiQGh0dHBzOi8vc3RvcmFnZS5nb29nbGVhcGlzLmNvbS9rdWJlcm5ldGVzLXJlbGVhc2UvbmV0d29yay1wbHVnaW5zL2NuaS0wN2E4YTI4NjM3ZTk3YjIyZWI4ZGZlNzEwZWVhZTEzNDRmNjlkMTZlLnRhci5negotIGNiYmE4NTY3NDZhNDQxYzdkMWE5ZTk1ZTE0MWM5ODJhMWI4ODY0ZTZAaHR0cHM6Ly9rdWJldXB2Mi5zMy5hbWF6b25hd3MuY29tL2tvcHMvMS41LjAvbGludXgvYW1kNjQvdXRpbHMudGFyLmd6CkNsdXN0ZXJOYW1lOiBtaW5pbWFsLmV4YW1wbGUuY29tCkNvbmZpZ0Jhc2U6IG1lbWZzOi8vY2x1c3RlcnMuZXhhbXBsZS5jb20vbWluaW1hbC5leGFtcGxlLmNvbQpJbnN0YW5jZUdyb3VwTmFtZTogbm9kZXMKVGFnczoKLSBfYXV0b21hdGljX3VwZ3JhZGVzCi0gX2F3cwpjaGFubmVsczoKLSBtZW1mczovL2NsdXN0ZXJzLmV4YW1wbGUuY29tL21pbmltYWwuZXhhbXBsZS5jb20vYWRkb25zL2Jvb3RzdHJhcC1jaGFubmVsLnlhbWwKcHJvdG9rdWJlSW1hZ2U6CiAgaGFzaDogN2MzYTBlYzA3MjNmZDM1MDYwOWIyOTU4YmM1YjhhYjAyNTgzODUxYwogIG5hbWU6IHByb3Rva3ViZToxLjUuMAogIHNvdXJjZTogaHR0cHM6Ly9rdWJldXB2Mi5zMy5hbWF6b25hd3MuY29tL2tvcHMvMS41LjAvaW1hZ2VzL3Byb3Rva3ViZS50YXIuZ3oKCl9fRU9GX0tVQkVfRU5WCgpkb3dubG9hZC1yZWxlYXNlCmVjaG8gIj09IG5vZGV1cCBub2RlIGNvbmZpZyBkb25lID09Igo="
"UserData": "IyEvYmluL2Jhc2gKIyBDb3B5cmlnaHQgMjAxNiBUaGUgS3ViZXJuZXRlcyBBdXRob3JzIEFsbCByaWdodHMgcmVzZXJ2ZWQuCiMKIyBMaWNlbnNlZCB1bmRlciB0aGUgQXBhY2hlIExpY2Vuc2UsIFZlcnNpb24gMi4wICh0aGUgIkxpY2Vuc2UiKTsKIyB5b3UgbWF5IG5vdCB1c2UgdGhpcyBmaWxlIGV4Y2VwdCBpbiBjb21wbGlhbmNlIHdpdGggdGhlIExpY2Vuc2UuCiMgWW91IG1heSBvYnRhaW4gYSBjb3B5IG9mIHRoZSBMaWNlbnNlIGF0CiMKIyAgICAgaHR0cDovL3d3dy5hcGFjaGUub3JnL2xpY2Vuc2VzL0xJQ0VOU0UtMi4wCiMKIyBVbmxlc3MgcmVxdWlyZWQgYnkgYXBwbGljYWJsZSBsYXcgb3IgYWdyZWVkIHRvIGluIHdyaXRpbmcsIHNvZnR3YXJlCiMgZGlzdHJpYnV0ZWQgdW5kZXIgdGhlIExpY2Vuc2UgaXMgZGlzdHJpYnV0ZWQgb24gYW4gIkFTIElTIiBCQVNJUywKIyBXSVRIT1VUIFdBUlJBTlRJRVMgT1IgQ09ORElUSU9OUyBPRiBBTlkgS0lORCwgZWl0aGVyIGV4cHJlc3Mgb3IgaW1wbGllZC4KIyBTZWUgdGhlIExpY2Vuc2UgZm9yIHRoZSBzcGVjaWZpYyBsYW5ndWFnZSBnb3Zlcm5pbmcgcGVybWlzc2lvbnMgYW5kCiMgbGltaXRhdGlvbnMgdW5kZXIgdGhlIExpY2Vuc2UuCgpzZXQgLW8gZXJyZXhpdApzZXQgLW8gbm91bnNldApzZXQgLW8gcGlwZWZhaWwKCk5PREVVUF9VUkw9aHR0cHM6Ly9rdWJldXB2Mi5zMy5hbWF6b25hd3MuY29tL2tvcHMvMS41LjAvbGludXgvYW1kNjQvbm9kZXVwCk5PREVVUF9IQVNIPQoKCgoKCgpmdW5jdGlvbiBlbnN1cmUtaW5zdGFsbC1kaXIoKSB7CiAgSU5TVEFMTF9ESVI9Ii92YXIvY2FjaGUva3ViZXJuZXRlcy1pbnN0YWxsIgogICMgT24gQ29udGFpbmVyT1MsIHdlIGluc3RhbGwgdG8gL3Zhci9saWIvdG9vbGJveCBpbnN0YWxsIChiZWNhdXNlIG9mIG5vZXhlYykKICBpZiBbWyAtZCAvdmFyL2xpYi90b29sYm94IF1dOyB0aGVuCiAgICBJTlNUQUxMX0RJUj0iL3Zhci9saWIvdG9vbGJveC9rdWJlcm5ldGVzLWluc3RhbGwiCiAgZmkKICBta2RpciAtcCAke0lOU1RBTExfRElSfQogIGNkICR7SU5TVEFMTF9ESVJ9Cn0KCiMgUmV0cnkgYSBkb3dubG9hZCB1bnRpbCB3ZSBnZXQgaXQuIFRha2VzIGEgaGFzaCBhbmQgYSBzZXQgb2YgVVJMcy4KIwojICQxIGlzIHRoZSBzaGExIG9mIHRoZSBVUkwuIENhbiBiZSAiIiBpZiB0aGUgc2hhMSBpcyB1bmtub3duLgojICQyKyBhcmUgdGhlIFVSTHMgdG8gZG93bmxvYWQuCmRvd25sb2FkLW9yLWJ1c3QoKSB7CiAgbG9jYWwgLXIgaGFzaD0iJDEiCiAgc2hpZnQgMQoKICB1cmxzPSggJCogKQogIHdoaWxlIHRydWU7IGRvCiAgICBmb3IgdXJsIGluICIke3VybHNbQF19IjsgZG8KICAgICAgbG9jYWwgZmlsZT0iJHt1cmwjIyovfSIKICAgICAgcm0gLWYgIiR7ZmlsZX0iCiAgICAgIGlmICEgY3VybCAtZiAtLWlwdjQgLUxvICIke2ZpbGV9IiAtLWNvbm5lY3QtdGltZW91dCAyMCAtLXJldHJ5IDYgLS1yZXRyeS1kZWxheSAxMCAiJHt1cmx9IjsgdGhlbgogICAgICAgIGVjaG8gIj09IEZhaWxlZCB0byBkb3dubG9hZCAke3VybH0uIFJldHJ5aW5nLiA9PSIKICAgICAgZWxpZiBbWyAtbiAiJHtoYXNofSIgXV0gJiYgISB2YWxpZGF0ZS1oYXNoICIke2ZpbGV9IiAiJHtoYXNofSI7IHRoZW4KICAgICAgICBlY2hvICI9PSBIYXNoIHZhbGlkYXRpb24gb2YgJHt1cmx9IGZhaWxlZC4gUmV0cnlpbmcuID09IgogICAgICBlbHNlCiAgICAgICAgaWYgW1sgLW4gIiR7aGFzaH0iIF1dOyB0aGVuCiAgICAgICAgICBlY2hvICI9PSBEb3dubG9hZGVkICR7dXJsfSAoU0hBMSA9ICR7aGFzaH0pID09IgogICAgICAgIGVsc2UKICAgICAgICAgIGVjaG8gIj09IERvd25sb2FkZWQgJHt1cmx9ID09IgogICAgICAgIGZpCiAgICAgICAgcmV0dXJuCiAgICAgIGZpCiAgICBkb25lCgogICAgZWNobyAiQWxsIGRvd25sb2FkcyBmYWlsZWQ7IHNsZWVwaW5nIGJlZm9yZSByZXRyeWluZyIKICAgIHNsZWVwIDYwCiAgZG9uZQp9Cgp2YWxpZGF0ZS1oYXNoKCkgewogIGxvY2FsIC1yIGZpbGU9IiQxIgogIGxvY2FsIC1yIGV4cGVjdGVkPSIkMiIKICBsb2NhbCBhY3R1YWwKCiAgYWN0dWFsPSQoc2hhMXN1bSAke2ZpbGV9IHwgYXdrICd7IHByaW50ICQxIH0nKSB8fCB0cnVlCiAgaWYgW1sgIiR7YWN0dWFsfSIgIT0gIiR7ZXhwZWN0ZWR9IiBdXTsgdGhlbgogICAgZWNobyAiPT0gJHtmaWxlfSBjb3JydXB0ZWQsIHNoYTEgJHthY3R1YWx9IGRvZXNuJ3QgbWF0Y2ggZXhwZWN0ZWQgJHtleHBlY3RlZH0gPT0iCiAgICByZXR1cm4gMQogIGZpCn0KCmZ1bmN0aW9uIHNwbGl0LWNvbW1hcygpIHsKICBlY2hvICQxIHwgdHIgIiwiICJcbiIKfQoKZnVuY3Rpb24gdHJ5LWRvd25sb2FkLXJlbGVhc2UoKSB7CiAgIyBUT0RPKHptZXJseW5uKTogTm93IHdlIFJFQUxMWSBoYXZlIG5vIGV4Y3VzZSBub3QgdG8gZG8gdGhlIHJlYm9vdAogICMgb3B0aW1pemF0aW9uLgoKICBsb2NhbCAtciBub2RldXBfdXJscz0oICQoc3BsaXQtY29tbWFzICIke05PREVVUF9VUkx9IikgKQogIGxvY2FsIC1yIG5vZGV1cF9maWxlbmFtZT0iJHtub2RldXBfdXJsc1swXSMjKi99IgogIGlmIFtbIC1uICIke05PREVVUF9IQVNIOi19IiBdXTsgdGhlbgogICAgbG9jYWwgLXIgbm9kZXVwX2hhc2g9IiR7Tk9ERVVQX0hBU0h9IgogIGVsc2UKICAjIFRPRE86IFJlbW92ZT8KICAgIGVjaG8gIkRvd25sb2FkaW5nIHNoYTEgKG5vdCBmb3VuZCBpbiBlbnYpIgogICAgZG93bmxvYWQtb3ItYnVzdCAiIiAiJHtub2RldXBfdXJsc1tAXS8lLy5zaGExfSIKICAgIGxvY2FsIC1yIG5vZGV1cF9oYXNoPSQoY2F0ICIke25vZGV1cF9maWxlbmFtZX0uc2hhMSIpCiAgZmkKCiAgZWNobyAiRG93bmxvYWRpbmcgbm9kZXVwICgke25vZGV1cF91cmxzW0BdfSkiCiAgZG93bmxvYWQtb3ItYnVzdCAiJHtub2RldXBfaGFzaH0iICIke25vZGV1cF91cmxzW0BdfSIKCiAgY2htb2QgK3ggbm9kZXVwCn0KCmZ1bmN0aW9uIGRvd25sb2FkLXJlbGVhc2UoKSB7CiAgIyBJbiBjYXNlIG9mIGZhaWx1cmUgY2hlY2tpbmcgaW50ZWdyaXR5IG9mIHJlbGVhc2UsIHJldHJ5LgogIHVudGlsIHRyeS1kb3dubG9hZC1yZWxlYXNlOyBkbwogICAgc2xlZXAgMTUKICAgIGVjaG8gIkNvdWxkbid0IGRvd25sb2FkIHJlbGVhc2UuIFJldHJ5aW5nLi4uIgogIGRvbmUKCiAgZWNobyAiUnVubmluZyBub2RldXAiCiAgIyBXZSBjYW4ndCBydW4gaW4gdGhlIGZvcmVncm91bmQgYmVjYXVzZSBvZiBodHRwczovL2dpdGh1Yi5jb20vZG9ja2VyL2RvY2tlci9pc3N1ZXMvMjM3OTMKICAoIGNkICR7SU5TVEFMTF9ESVJ9OyAuL25vZGV1cCAtLWluc3RhbGwtc3lzdGVtZC11bml0IC0tY29uZj0ke0lOU1RBTExfRElSfS9rdWJlX2Vudi55YW1sIC0tdj04ICApCn0KCiMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIwoKL2Jpbi9zeXN0ZW1kLW1hY2hpbmUtaWQtc2V0dXAgfHwgZWNobyAiZmFpbGVkIHRvIHNldCB1cCBlbnN1cmUgbWFjaGluZS1pZCBjb25maWd1cmVkIgoKZWNobyAiPT0gbm9kZXVwIG5vZGUgY29uZmlnIHN0YXJ0aW5nID09IgplbnN1cmUtaW5zdGFsbC1kaXIKCmNhdCA+IGt1YmVfZW52LnlhbWwgPDwgX19FT0ZfS1VCRV9FTlYKQXNzZXRzOgotIDdkNzBlMDkwOTUxNDg2Y2FlNTJkOWE4MmI3YWFmNTA1NmY4NGY4ZWRAaHR0cHM6Ly9zdG9yYWdlLmdvb2dsZWFwaXMuY29tL2t1YmVybmV0ZXMtcmVsZWFzZS9yZWxlYXNlL3YxLjQuNi9iaW4vbGludXgvYW1kNjQva3ViZWxldAotIDlhZGNkMTIwZmRiN2FkNmU2NGMwNjFlNTZhMDVmZWZjMTJlOTYxOGJAaHR0cHM6Ly9zdG9yYWdlLmdvb2dsZWFwaXMuY29tL2t1YmVybmV0ZXMtcmVsZWFzZS9yZWxlYXNlL3YxLjQuNi9iaW4vbGludXgvYW1kNjQva3ViZWN0bAotIDE5ZDQ5ZjdiMmI5OWNkMjQ5M2Q1YWUwYWNlODk2YzY0ZTI4OWNjYmJAaHR0cHM6Ly9zdG9yYWdlLmdvb2dsZWFwaXMuY29tL2t1YmVybmV0ZXMtcmVsZWFzZS9uZXR3b3JrLXBsdWdpbnMvY25pLTA3YThhMjg2MzdlOTdiMjJlYjhkZmU3MTBlZWFlMTM0NGY2OWQxNmUudGFyLmd6Ci0gY2JiYTg1Njc0NmE0NDFjN2QxYTllOTVlMTQxYzk4MmExYjg4NjRlNkBodHRwczovL2t1YmV1cHYyLnMzLmFtYXpvbmF3cy5jb20va29wcy8xLjUuMC9saW51eC9hbWQ2NC91dGlscy50YXIuZ3oKQ2x1c3Rlck5hbWU6IG1pbmltYWwuZXhhbXBsZS5jb20KQ29uZmlnQmFzZTogbWVtZnM6Ly9jbHVzdGVycy5leGFtcGxlLmNvbS9taW5pbWFsLmV4YW1wbGUuY29tCkluc3RhbmNlR3JvdXBOYW1lOiBub2RlcwpUYWdzOgotIF9hdXRvbWF0aWNfdXBncmFkZXMKLSBfYXdzCmNoYW5uZWxzOgotIG1lbWZzOi8vY2x1c3RlcnMuZXhhbXBsZS5jb20vbWluaW1hbC5leGFtcGxlLmNvbS9hZGRvbnMvYm9vdHN0cmFwLWNoYW5uZWwueWFtbApwcm90b2t1YmVJbWFnZToKICBoYXNoOiA3YzNhMGVjMDcyM2ZkMzUwNjA5YjI5NThiYzViOGFiMDI1ODM4NTFjCiAgbmFtZTogcHJvdG9rdWJlOjEuNS4wCiAgc291cmNlOiBodHRwczovL2t1YmV1cHYyLnMzLmFtYXpvbmF3cy5jb20va29wcy8xLjUuMC9pbWFnZXMvcHJvdG9rdWJlLnRhci5negoKX19FT0ZfS1VCRV9FTlYKCmRvd25sb2FkLXJlbGVhc2UKZWNobyAiPT0gbm9kZXVwIG5vZGUgY29uZmlnIGRvbmUgPT0iCg=="
Copy link
Member

Choose a reason for hiding this comment

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

Aside: I am wondering if we should inline-expand the UserData during the tests, just so these diffs are less annoying :-)

Copy link
Contributor Author

@DerekV DerekV Aug 4, 2017

Choose a reason for hiding this comment

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

My personal impulse is that, in a perfect world, it would be broken up into a large series of property-by-property based tests rather than a byte for byte test. That refactor would be a time consuming one, so I guess it depends on how much time it would save. Errors and changes would definitely be clearer.

@@ -38,6 +38,13 @@ spec:
{{ range $arg := DnsControllerArgv }}
- "{{ $arg }}"
{{ end }}
{{- if .EgressProxy }}
Copy link
Member

Choose a reason for hiding this comment

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

Could we check if len(ProxyEnv) == 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like there was some back and forth on what the check should be, not sure there is any particular reason I ended up with this one.

egressProxy.ProxyExcludes = strings.Join(egressSlice, ",")
glog.V(4).Infof("Completed setting up Proxy Excludes: %q", egressProxy.ProxyExcludes)
} else {
glog.V(4).Info("Not setting up Proxy Excludes")
Copy link
Member

Choose a reason for hiding this comment

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

V(8) or remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove the most of these tracer logs or v(8) if I think they might be interesting enough

Copy link
Contributor

Choose a reason for hiding this comment

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

V(8) then please

@justinsb justinsb self-assigned this Aug 4, 2017
@chrislovecnm
Copy link
Contributor

/ok-to-test

@justinsb
Copy link
Member

justinsb commented Aug 5, 2017

As we discussed in office hours, I think you're right and we do want to keep egressProxy even if we eventually have a top-level egress option (because layer 7 vs layer 4, i.e. they are not exclusive).

If you want to do a pass on some of the smaller items (e.g. the logging) then I think we can get this merged :-)

@k8s-github-robot
Copy link

@DerekV PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 7, 2017
@DerekV
Copy link
Contributor Author

DerekV commented Aug 7, 2017

Should I also squash to one commit? What is preferred?

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 7, 2017
@DerekV
Copy link
Contributor Author

DerekV commented Aug 7, 2017

@chrislovecnm did I address all your requested changes OK?

@chrislovecnm
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DerekV, chrislovecnm

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

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

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

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2017
@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 38608bd into kubernetes:master Aug 7, 2017
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. lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants