-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Use proxy-protocol to pass through source IP to nginx #675
Conversation
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. |
CLA signed. |
Full disclosure: This PR represents essentially my very first attempts at writing golang in anger, so please do not go easy on me if it sucks - I want to know if things could be done better/more idiomatic. |
//Write out the proxy-protocol header | ||
localAddr := conn.LocalAddr().(*net.TCPAddr) | ||
remoteAddr := conn.RemoteAddr().(*net.TCPAddr) | ||
protocol := "UNKNOWN" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be
protocol := "TCP4"
if remoteAddr.IP.To16() != nil {
protocol = "TCP6"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that - I wasn't sure if one of To4() and To16() != nil is guaranteed with the golang net stdlib - the proxy-protocol spec says that if it's not either IPv4 or IPv6 the protocol header should read 'UNKNOWN'.
Do you want me to push a commit that makes that change? |
Bump |
Just checked up on this and saw that there's been a merge conflict since I submitted. I'll push a resolution today. |
Conflict resolved and pushed. We're currently using a custom build of ingress for our internal controller - any chance of this being merged anytime soon? |
Coverage decreased (-0.1%) to 46.983% when pulling e77c4fb71d184f4a9e66cb2ac8fc9c76b71fd640 on dpratt:master into 317f222 on kubernetes:master. |
@dpratt please check your PR contains a not related commit. Please fix this and squash. |
@aledbf Not really sure what you mean - there are two commits, one that fixes the issue, and one that merges master in to fix the merge conflict. I'd be happy to rebase and resubmit, if that's what you're asking for. |
I've rebased. |
/lgtm |
@dpratt thanks! |
This still gives 127.0.0.1 as remote address |
How do you have things configured? We are running it in production and it fixes the problem. |
@dpratt I compiled apiVersion: extensions/v1beta1
kind: Deployment
metadata:
labels:
k8s-app: nginx-ingress-controller
name: nginx-ingress-controller
spec:
replicas: 5
selector:
matchLabels:
k8s-app: nginx-ingress-controller
template:
metadata:
labels:
k8s-app: nginx-ingress-controller
spec:
containers:
- args:
- /nginx-ingress-controller
- --default-backend-service=$(POD_NAMESPACE)/default-http-backend
- --configmap=$(POD_NAMESPACE)/nginx-conf
env:
- name: POD_NAME
valueFrom:
fieldRef:
apiVersion: v1
fieldPath: metadata.name
- name: POD_NAMESPACE
valueFrom:
fieldRef:
apiVersion: v1
fieldPath: metadata.namespace
image: zllovesuki/nginx-ingress-controller:master
imagePullPolicy: IfNotPresent
livenessProbe:
failureThreshold: 3
httpGet:
path: /healthz
port: 10254
scheme: HTTP
initialDelaySeconds: 10
periodSeconds: 10
successThreshold: 1
timeoutSeconds: 1
name: nginx-ingress-controller
ports:
- containerPort: 80
hostPort: 80
protocol: TCP
- containerPort: 443
hostPort: 443
protocol: TCP
- containerPort: 18080
hostPort: 18080
protocol: TCP
readinessProbe:
failureThreshold: 3
httpGet:
path: /healthz
port: 10254
scheme: HTTP
periodSeconds: 10
successThreshold: 1
timeoutSeconds: 1
volumeMounts:
- mountPath: /etc/nginx/template
name: nginx-template-volume
readOnly: true
hostNetwork: true
terminationGracePeriodSeconds: 30
volumes:
- configMap:
defaultMode: 420
items:
- key: nginx.tmpl
path: nginx.tmpl
name: nginx-template
name: nginx-template-volume default/nginx-conf: apiVersion: v1
data:
enable-vts-status: "true"
enabled-dynamic-tls-records: "true"
error-log-level: warn
keep-alive: "30"
max-worker-connections: "10240"
proxy-body-size: 2g
proxy-read-timeout: "300"
proxy-send-timeout: "300"
ssl-ciphers: ECDHE-ECDSA-CHACHA20-POLY1305-D:ECDHE-RSA-CHACHA20-POLY130-D:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA256:DHE-RSA-AES256-SHA:ECDHE-ECDSA-DES-CBC3-SHA:ECDHE-RSA-DES-CBC3-SHA:EDH-RSA-DES-CBC3-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:DES-CBC3-SHA:!DSS
ssl-dh-param: default/nginx-dhparam-4096
worker-processes: "8"
kind: ConfigMap
metadata:
name: nginx-conf
namespace: default
|
The important bit in the nginx template is in the server directive - make sure yours reads as follows, the part with proxy_protocol on port 442 is the important bit, if it's not configured the same, the remote address won't propagate properly.
Are you using the ingress controller to terminate SSL? If not, your problem is unrelated to this. Additionally, what nginx variable are you using to populate the remote address in your GELF message? Are you guys piping all your outputs to graylog? If so, you might be interested in the config we use internally - we don't use the stock fluentd-elasticsearch k8s addon, but rather a custom DaemonSet that runs fluent and pipes all logs from all containers to Graylog. To help with this, I've also modified our template to output access logs in JSON for better indexing - here's a relevant snippet.
|
(Sorry I'm very sleepy I apologize if I'm being incoherent...) sample from
|
Looks okay to me - not sure why it's not working for you. The only moving part that hasn't been eliminated is the bits that produce your GELF message. To track this down, I'd suggest that you temporarily swap out GELF logging in your template for bone stock text nginx access logging and see what you get there. |
@dpratt Using your template still gives 127.0.0.1... |
OK, I found the issue... I'm using nginx directly on host network, and without proxy_protocol enabled to all (since there are hosts with HTTP only). Therefore, set_real_ip doesn't work... Workaround would be:
|
This is a PR that helps address #672.