kubernetes / ingress-nginx

Ingress NGINX Controller for Kubernetes
https://kubernetes.github.io/ingress-nginx/
Apache License 2.0
17.52k stars 8.25k forks source link

Allow customizing the `real_ip_header` directive when `proxy_protocol` is enabled #11623

Open bossm8 opened 3 months ago

bossm8 commented 3 months ago

It should be possible to override the hardcoded value of real_ip_header even when proxy_protocol is used as this is an invalid setting for deployments where multiple proxies are involved.

Our current setup looks like this:

HAProxy - (via http) -> Octavia LB - (via proxy-protocol) -> ingress-nginx

The HAProxy already sets the X-Forwarded-For value to the real client IP address but there is no way to use this value when the proxy-protocol is enabled (in Helm: controller.config.use-proxy-protocol: "true") for the communication between the Octavia LB and the ingress-nginx because the nginx.tmpl hardcodes the value for the real_ip_header (source):

    {{/* Enable the real_ip module only if we use either X-Forwarded headers or Proxy Protocol. */}}
    {{/* we use the value of the real IP for the geo_ip module */}}
    {{ if or (or $cfg.UseForwardedHeaders $cfg.UseProxyProtocol) $cfg.EnableRealIP }}
    {{ if $cfg.UseProxyProtocol }}
    real_ip_header      proxy_protocol;
    {{ else }}
    real_ip_header      {{ $cfg.ForwardedForHeader }};
    {{ end }}

The only IP addresses which are being logged by the ingress access logs are now the ones of our HAProxies instead of the client IP addresses. We tried various configurations but were not able to get ingress-nginx to use the correct address except when we started it using a custom template with the following adjustments to the template above:

    {{/* Enable the real_ip module only if we use either X-Forwarded headers or Proxy Protocol. */}}
    {{/* we use the value of the real IP for the geo_ip module */}}
    {{ if or (or $cfg.UseForwardedHeaders $cfg.UseProxyProtocol) $cfg.EnableRealIP }}
    {{ if $cfg.UseProxyProtocol }}
-   real_ip_header      proxy_protocol;
+   real_ip_header      {{ or $cfg.ForwardedForHeader "proxy_protocol" }};
    {{ else }}
    real_ip_header      {{ $cfg.ForwardedForHeader }};
    {{ end }}

Now this would probably require a new variable as ForwardedForHeader always defaults to X-Forwarded-For(doc). But using this approach I was able to finally get the real client IP address logged by the ingress.

Is there currently another issue associated with this?

Could not find any.

Does it require a particular kubernetes version?

No (see Helm chart)

Additional Refs:

Is it possible to get a configuration option allowing overriding the hardcoded value as shown above. Because the approach with the custom template is just a hack which is not really usable in production as we would always need to fetch the template when updating the chart version.

k8s-ci-robot commented 3 months ago

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
longwuyuan commented 3 months ago

Your purpose may be better served if you can attempt to enable proxy-protocol on the Octavia LB.

You have not answered the questions that are asked in a new issue template so its not much info for analysis going to readers. You can look at the template of a new bug report and then edit this issue description to add answers to the questions asked here.

But also on a brief overview, enabling proxy-protocol on whichever/whatever LB (in this case Octavia) and also enabling proxyprotocol on the controller is the only configuration this project intends to support at L4.. If a infra provided LB can not support proxy-protocol, then the remaining choice is to set the externalTrafficPolicy field in the service object to a value of "Local".

bossm8 commented 3 months ago

I used the feature request template because I don't think this is a bug (<!-- What do you want to happen? --> seems to be answered). But I can convert it into a bug if needed.

We have enabled proxy protocol on the Octavia LB otherwise the communication between the two would not work properly. Below is the current configuration (the relevant part):

controller:
   service:
       annotations:
          loadbalancer.openstack.org/proxy-protocol: "true"
          loadbalancer.openstack.org/keep-floatingip: "true"
       externalTrafficPolicy: Cluster
   config:
       use-proxy-protocol: "true"
       use-forwarded-headers: "true"
       proxy-add-original-uri-header: "true"
       proxy-real-ip-cidr: "**redacted**"

We have tried almost all combinations and suggestions which can be found on about the topic, also setting various annotations on the service for the Octavia LB, setting the externalTrafficPolicy: Local, using the $proxy_protocol_addr in the log-format-upstream, ... none of them resulted in getting the real client IP logged on the ingress, except when we can use the Forwarded-For header which was set from HAProxy as described above.

Another note: by going through the template again I noticed that compute-full-forwarded-for uses the structure (source):

    {{ if and $cfg.UseForwardedHeaders $cfg.ComputeFullForwardedFor }}
    # We can't use $proxy_add_x_forwarded_for because the realip module
    # replaces the remote_addr too soon
    map $http_x_forwarded_for $full_x_forwarded_for {
        {{ if $all.Cfg.UseProxyProtocol }}
        default          "$http_x_forwarded_for, $proxy_protocol_addr";
        ''               "$proxy_protocol_addr";
        {{ else }}
        default          "$http_x_forwarded_for, $realip_remote_addr";
        ''               "$realip_remote_addr";
        {{ end}}
    }

I installed a whoami service in the cluster and can see the following:

X-Forwarded-For: <Client IP>, <HAProxy IP>   # "$http_x_forwarded_for, $proxy_protocol_addr"
X-Original-Forwarded-For: <Client IP>        # {{ $proxySetHeader }} X-Original-Forwarded-For {{ buildForwardedFor $all.Cfg.ForwardedForHeader }};
X-Real-Ip: <HAProxy IP>                       # proxy_set_header            X-Real-IP               $remote_addr;

All of those headers confirm what I've observed, with the patch mentioned above the X-Real-Ip header also contains the client ip instead of the one of the HAProxy.

longwuyuan commented 3 months ago
strongjz commented 3 months ago

/lifecycle frozen

We are in the middle of working on changes to the template to use nginx go crossplane. Once that is completed, we can come back to this one.

https://github.com/kubernetes/ingress-nginx/pull/11608

bossm8 commented 3 months ago

FYI (for others which might stumble across this issue), we are now using a custom template with the following patch (ingress-nginx chart version 4.11.0) so that in either case (access via HAProxy, or direct access via Octavia) the real client IP is set in real_ip_header:

--- nginx.tmpl  2024-07-18 08:46:53
+++ infrastructure/controllers/base/nginx.tmpl  2024-07-18 09:11:50
@@ -137,11 +137,7 @@
     {{/* Enable the real_ip module only if we use either X-Forwarded headers or Proxy Protocol. */}}
     {{/* we use the value of the real IP for the geo_ip module */}}
     {{ if or (or $cfg.UseForwardedHeaders $cfg.UseProxyProtocol) $cfg.EnableRealIP }}
-    {{ if $cfg.UseProxyProtocol }}
-    real_ip_header      proxy_protocol;
-    {{ else }}
-    real_ip_header      {{ $cfg.ForwardedForHeader }};
-    {{ end }}
+    real_ip_header      X-CORRECT-REAL-IP;

     real_ip_recursive   on;
     {{ range $trusted_ip := $cfg.ProxyRealIPCIDR }}
@@ -465,7 +461,21 @@
         default          "$http_x_forwarded_for, $realip_remote_addr";
         ''               "$realip_remote_addr";
         {{ end}}
+    }
+
+    {{ end }}
+
+    {{/* Set the correct real_ip header value for cases with multiple proxies involved */}}
+    {{/* We check 'hardcoded' if X-Forwarded-For (this would probably need to be configurable too) is present */}}
+    {{/* and use this value for the real ip, else we use the proxy_protocol_addr */}}
+    {{ if or (or $cfg.UseForwardedHeaders $cfg.UseProxyProtocol) $cfg.EnableRealIP }}
+    map $http_x_forwarded_for $correct_ip {
+        default $http_x_forwarded_for;
+        ''      $proxy_protocol_addr;
     }
+    # real_ip_header only accepts static configs, no variables so we need to define a custom one
+    # https://medium.com/@getpagespeed/how-to-use-multiple-real-ip-headers-with-nginx-a9ed0881dd67
+    more_set_input_headers "X-CORRECT-REAL-IP: $correct_ip";

     {{ end }}
github-actions[bot] commented 2 months ago

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #ingress-nginx-dev on Kubernetes Slack.