Add use-forwarded-headers configmap option.

[Dirbaio]

This PR adds a configmap option use-forwarded-headers to enable/disable trust of incoming X-Forwarded-* headers.

It is set to true by default, which corresponds to the current ingress behavior, so this change is backwards compatible.

Fixes #1815 #1309 #1668

[k8s-reviewable]

This change is 

[coveralls]

Coverage increased (+0.01%) to 36.775% when pulling ef9587b on Dirbaio:master into fead908 on kubernetes:master.

[aledbf]

ping @maxlaverse

[coveralls]

Coverage increased (+0.01%) to 36.775% when pulling 38f0c6c on Dirbaio:master into fead908 on kubernetes:master.

[maxlaverse]

Hi @Dirbaio,
I like the idea of an option to disable the RealIP module completely for those who don't need it.
However, have you tested your PR ? The new map blocks look broken to me.

[Dirbaio]

Yes! It's all tested, I'm actually using it in production :)

If by "broken" you mean the "dummy" thing, it's the only workaround I could find to assign one variable to another in the http block.

For example, this sets the variable $pass_access_scheme to the value of $scheme unconditionally.

    map 'dummy' $pass_access_scheme {
         default          $scheme;
    }

I agree it's rather ugly, I'd love input on better ways to do this. Some things I've tried:

• Changing the template everywhere to use the right variable so we don't have to assign any. The issue is that these variables are used in tons of places and the template gets messy and error-prone very fast. Something like this.

proxy_set_header X-Forwarded-Proto      {{ if $cfg.UseForwardedHeaders }}$pass_access_scheme{{else}}$scheme{{end}};

• Using nginx set directive. Problem is it's not allowed inside http blocks :(
• Use go template variables that contain the variable: you can't set a variable inside an if and use it outside later.
• Move the logic to the Go code (so Go passes a value to the template which is $scheme or $pass_access_scheme. I don't think this is a good idea because it makes it less easy to customize nginx-ingress changing only the template.

Also note that the original template before my changes already had one instance of such hack: the map for $the_real_ip

[Dirbaio]

Rebased to latest master

[coveralls]

Coverage increased (+0.01%) to 36.775% when pulling 6c2b588 on Dirbaio:master into 519f72e on kubernetes:master.

[maxlaverse]

Thanks for the explanation @Dirbaio

As I said I like the idea. However I have the impression this PR makes the code around the X-Forwarded-* headers less readable than it is currently. It's a complex topic since the Nginx Ingress supports many different configurations. As a user and contributor I'd like to see PRs that make the Nginx Ingress behaviors more clear and understandable.

My opinion is that we should spend time on your last comment in #1815 (comment), see what can technically be done and agree on a clear implementation for the X-Forwarded-For headers.

[Dirbaio]

This PR implements mostly the behavior in that comment.

• WAN mode -> use-forwarded-headers=false, use-proxy-protocol=false
• forwarded header mode -> use-forwarded-headers=true, use-proxy-protocol=false
• proxy-protocol mode -> use-forwarded-headers=false, use-proxy-protocol=true

Maybe it would be cleaner to replace these two bools with a single option choosing the mode. Something like this?

• WAN mode -> proxy-mode=direct
• forwarded header mode -> proxy-mode=forwarded-headers
• proxy-protocol mode -> proxy-mode=proxy-protocol

Not sure on the naming though. proxy-mode, client-info-mode, client-info-source ... Also, this would remove the possibility of using proxy protocol AND forwarded headers at the same time, but I can't think of any case where you'd want that.

And of course, document it.

What do you think?

[maxlaverse]

I don't have a strong opinion about the naming.
I like the second option a bit more as it makes the notion of "proxy-mode" real, which I believe is easier to understand and document.

I think a mixed setup with forwarded headers and proxy protocol is broken anyway and can't be fixed (see #1489 (comment))

Now that I spent some time reading your proposition, I think those dummy maps are acceptable (if we can't find a cleaner way). At least it makes the proxy configuration easier to understand for users. We should make sure we have the rights comments in the template to make it easier for contributors to understand how the Nginx Controller works with the different "proxy-mode"s.

Let's discuss #1815 (comment) and if we reach an agreement, add documentation here ?

[aledbf]

@Dirbaio we need to try to not add new options. I mean

Maybe it would be cleaner to replace these two bools with a single option choosing the mode. >Something like this?
WAN mode -> proxy-mode=direct
forwarded header mode -> proxy-mode=forwarded-headers
proxy-protocol mode -> proxy-mode=proxy-protocol

Mode	Description
proxy-protocol	is the option use-proxy-protocol
forwarded header	default option when use-proxy-protocol is false
WAN	is "similar" to compute-full-forwarded-for
headers from connection	the changes introduced by this PR

If we add a new option called "proxy-mode" (just an example) we could brake a running ingress controller (overriding a configured option)

[aledbf]

I think those dummy maps are acceptable (if we can't find a cleaner way).

There's no other way to do this.

[aledbf]

Please add a comment at the start of the else section explaining why this is required

[maxlaverse]

I agree that in general we should always try limiting the addition of new options.
In that case, "proxy-mode" would replace two existing options after a deprecation period. And it would be set in the config-map dedicated to the Ingress.

Don't you think it's really easier to explain how the Nginx Ingress works with those "proxy-modes" @aledbf ? And having the documentation talking about proxy modes that match the configuration flags is straightforward to understand, which should lead to less issues on the topic :)

If we have a proper section in the documentation about those modes including the existing proxy-real-ip-cidr paragraph, that would already be a good improvement.

[aledbf]

In that case, "proxy-mode" would replace two existing options after a deprecation period. And it would be set in the config-map dedicated to the Ingress.

Good idea

[aledbf]

Don't you think it's really easier to explain how the Nginx Ingress works with those "proxy-modes" @aledbf ? And having the documentation talking about proxy modes that match the configuration flags is straightforward to understand, which should lead to less issues on the topic :)

You think the explanation in #1815 (comment) is the right format?

[maxlaverse]

I think we could start from it yes

The source of the headers is described for every case.
And I like this "designed for" sentence that also contains some real products. It believe it will help users that are not familiar with the topic.

The existing proxy-real-ip-cidr section should be integrated with a clear explanation what it implies regarding the headers and the proxy-modes. Basically proxy-real-ip-cidr should not be set (eq default 0.0.0.0/0) for the modes to work as described. We don't have to remove that option, but we should probably not encourage users to change it imo. If people still want to set it, why not, but it's a mixed mode and implies to know how the RealIP Nginx module behaves.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale