Skip to content

Allow customizing the real_ip_header directive when proxy_protocol is enabled #11623

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

Closed
bossm8 opened this issue Jul 16, 2024 · 11 comments · Fixed by #12768
Closed

Allow customizing the real_ip_header directive when proxy_protocol is enabled #11623

bossm8 opened this issue Jul 16, 2024 · 11 comments · Fixed by #12768
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@bossm8
Copy link
Contributor

bossm8 commented Jul 16, 2024

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.

@bossm8 bossm8 added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 16, 2024
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jul 16, 2024
@k8s-ci-robot
Copy link
Contributor

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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@longwuyuan
Copy link
Contributor

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
Copy link
Contributor Author

bossm8 commented Jul 17, 2024

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
Copy link
Contributor

  • In the CI, the project does not test a HAProxy in front of a L4 LB in front of the controller.
  • This use-case is not recommended or widely popular because there are multiple nuances applicable and the highest number of users is the use case of creating a service of --type LoadBalancer for the controller and limiting all configurations to just that.
  • It will not likely for any contributor to test your use case of HAProxy <--> Octavia <--> Controller
  • Some comments on use-case of HAProxy <--> LB <--> Controller may occur from other users of same architecture
  • But you may find more users and experts on this in the Kubernetes Slack

@strongjz
Copy link
Member

/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.

#11608

@k8s-ci-robot k8s-ci-robot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Jul 18, 2024
@bossm8
Copy link
Contributor Author

bossm8 commented Jul 22, 2024

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 github-actions bot removed the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Jul 23, 2024
Copy link

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.

@github-actions github-actions bot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Aug 22, 2024
@BardiaYaghmaie
Copy link

@bossm8
Big thanks for this PR, this feature is really needed.
In my use case I have to change real_ip_header and there is no way.

@strongjz
Copy link
Member

@bossm8 I dont see a PR for this?

@bossm8
Copy link
Contributor Author

bossm8 commented Jan 7, 2025

@strongjz no there is no PR, but I'm happy to create one if I'm sure it will be seriously looked at

@dmeremyanin
Copy link
Contributor

dmeremyanin commented Jan 29, 2025

I came here with exactly the same issue. @bossm8 thanks for the workaround you shared, and I hope it will be possible to configure it in the ingress-nginx itself.


Also, I wanted to share my 2 cents on this statement:

This use-case is not recommended or widely popular <...>

I might have missed something, but you'll encounter the same issue if you place a TCP LB (with proxy protocol enabled) behind Cloudflare or any other external L7 proxy. I don't think this is a niche case or something that's not popular.

flowchart LR
    proxy(Cloudflare) -- http --> lb(Cloud TCP LB)
    subgraph VPC
    lb -- proxy protocol --> nginx(ingress-nginx)
    end
Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
Development

Successfully merging a pull request may close this issue.

6 participants