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

Added WithNoProxy dialopts to ignore proxies for grpc connection when dealing with unix socket. #980

Merged
merged 1 commit into from
May 19, 2020

Conversation

nithu0115
Copy link
Contributor

@nithu0115 nithu0115 commented May 19, 2020

Issue #, if available: #931

Description of changes:

The non-grpc things are mostly HTTP requests that we do need to proxy. The grpc things, as noted, as via local sockets, which can't and shouldn't be proxied.

Providing our own dialer is an option, but this seems a little cleaner and like other folks might also find it useful.

== Tests ==

[ec2-user@ip-10-0-1-130 ~]$ kn describe ds/aws-node |grep -i env -A9
    Environment Variables from:
      proxy-environment-variables  ConfigMap  Optional: false
    Environment:
      AWS_VPC_K8S_CNI_LOGLEVEL:    DEBUG
      AWS_VPC_K8S_CNI_VETHPREFIX:  eni
      AWS_VPC_ENI_MTU:             9001
      MY_NODE_NAME:                 (v1:spec.nodeName)
    Mounts:
      /host/etc/cni/net.d from cni-net-dir (rw)
      /host/opt/cni/bin from cni-bin-dir (rw)
      /host/var/log from log-dir (rw)
      /var/run/docker.sock from dockersock (rw)

[ec2-user@ip-10-0-1-130 ~]$ kn exec -it aws-node-j8c5z env |grep -i proxy
HTTPS_PROXY=http://192.168.140.166:3128
HTTP_PROXY=http://192.168.140.166:3128
NO_PROXY=192.168.0.0/16,/var/run/dockershim.sock,localhost,10.100.0.0/16,127.0.0.1,169.254.169.254,.s3.us-east-2.amazonaws.com,api.ecr.us-east-2.amazonaws.com,dkr.ecr.us-east-2.amazonaws.com,ec2.us-east-2.amazonaws.com,.us-east-2.eks.amazonaws.com,.us-east-2.compute.internal

[ec2-user@ip-10-0-1-130 ~]$ kn describe cm/proxy-environment-variables
Name:         proxy-environment-variables
Namespace:    kube-system
Labels:       <none>
Annotations:
Data
====
HTTPS_PROXY:
----
http://192.168.140.166:3128
HTTP_PROXY:
----
http://192.168.140.166:3128
NO_PROXY:
----
192.168.0.0/16,/var/run/dockershim.sock,localhost,10.100.0.0/16,127.0.0.1,169.254.169.254,.s3.us-east-2.amazonaws.com,api.ecr.us-east-2.amazonaws.com,dkr.ecr.us-east-2.amazonaws.com,ec2.us-east-2.amazonaws.com,.us-east-2.eks.amazonaws.com,.us-east-2.compute.internal
Events:  <none>

[ec2-user@ip-10-0-1-130 ~]$ k get deploy
NAME         READY   UP-TO-DATE   AVAILABLE   AGE
pv-deploy    25/50   50           25          7m19s
pv-deploy6   0/100   0            0           26m

[ec2-user@ip-10-0-1-130 ~]$ kn get po -w
NAME                       READY   STATUS    RESTARTS   AGE
aws-node-j8c5z             1/1     Running   0          20m
aws-node-ss4vp             1/1     Running   0          22m
coredns-74dd858ddc-jw2bm   1/1     Running   0          16h
coredns-74dd858ddc-zcxdd   1/1     Running   0          16h
kube-proxy-pm46g           1/1     Running   0          13h
kube-proxy-xfhbw           1/1     Running   0          13h

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@nithu0115 nithu0115 changed the base branch from master to release-1.6.1 May 19, 2020 13:34
@nithu0115
Copy link
Contributor Author

Waiting to complete my E2E tests.

Copy link
Contributor

@haouc haouc left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nithu0115
Copy link
Contributor Author

Attached tests to PR. Let me know if you have any questions.

Copy link
Contributor

@mogren mogren left a comment

Choose a reason for hiding this comment

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

Nice, great fix!

@mogren mogren merged commit 438abef into aws:release-1.6.1 May 19, 2020
@rimaulana
Copy link

Tested this patch and it works in my env

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants