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

real-ip configuration not applied to default server #1072

Closed
svvac opened this issue Jul 24, 2020 · 3 comments · Fixed by #1129
Closed

real-ip configuration not applied to default server #1072

svvac opened this issue Jul 24, 2020 · 3 comments · Fixed by #1129
Labels
proposal An issue that proposes a feature request

Comments

@svvac
Copy link
Contributor

svvac commented Jul 24, 2020

Configuration options like real-ip-header, set-real-ip-from and real-ip-recursive only get applied at the ingress level and not to the default server.

Since these are global options, they should be applicable there too.

Current workaround is redefining them in http-snippets.

On that note, these options are documented in the Global options, but don't appear in the Ingress options

Proposed solution :

  • The global options should end up in the http block
  • Annotations for overriding these at the server level should be added/documented
@pleshakov pleshakov added the proposal An issue that proposes a feature request label Jul 24, 2020
@pleshakov
Copy link
Contributor

Hi @svvac

It is an omission that we don't set those directives in the default server. It makes sense to add them!

The global options should end up in the http block
Annotations for overriding these at the server level should be added/documented

With the configmap/annotations, we don't necessary have this relationship that a configmap key sets a directive in the http block and the corresponding annotation overrides the directive in the server/location block.

So I wonder, if simply setting real-ip-header, set-real-ip-from and real-ip-recursive in the default server server block will do the job?
Also, typically those directives apply to all Ingress resources, not individual ones (for example, when configuring proxy protocol https://github.com/nginxinc/kubernetes-ingress/tree/master/examples/proxy-protocol), so that's why having annotation is not necessary. Could you clarify if you have a use case for those annotations?

@svvac
Copy link
Contributor Author

svvac commented Jul 25, 2020

So I wonder, if simply setting real-ip-header, set-real-ip-from and real-ip-recursive in the default server server block will do the job?

Well, that's what I mostly care about indeed

Also, typically those directives apply to all Ingress resources, not individual ones, so that's why having annotation is not necessary. Could you clarify if you have a use case for those annotations?

Not really. Skimming through the options code, I assumed those existed because the real ip params are defined in the ingress options struct, and not in the global options struct (though I ain't fluent in Go so maybe I'm missing something). Also duplicating them for each ingress rather than directly in the http block seemed to confirm that.

So in my opinion it'd be somewhat cleaner to move these in the global config (better reflecting the intent), but as I said I mostly care about having the default server using the right client IP address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal An issue that proposes a feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@svvac @pleshakov and others