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

Allow setting headers per location #9693

Closed
rikatz opened this issue Mar 5, 2023 · 8 comments
Closed

Allow setting headers per location #9693

rikatz opened this issue Mar 5, 2023 · 8 comments
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

@rikatz
Copy link
Contributor

rikatz commented Mar 5, 2023

One thing I've realized is that people has been using snippets to add headers to be sent to upstream per location or per server. I think we can do better here. If this is a case where people would stop using snippets in favor of using a new way of setting headers, I would +1 and prioritize this.

My proposal:

  • We define a new custom annotation prefix (configurable by admin) like headers.nginx.ingress.kubernetes.io (or something more trivial) that allows people to set something like
headers.nginx.ingress.kubernetes.io/X-Forwarded-Prefix: "/bla"

Above will set on the location a new header "X-Forwarded-Prefix = /bla"

We do some check on annotations to verify if some header annotation exists, and use it. My only concern here is always the arbitrary data problem and if this can be used, as an example, to bypass some restrictions, force nginx to connect to some other upstream, etc.

Maybe another thing is saying that "we allow headers as soon as the ingress controller owner allows headers", so admin would set a list of allowed headers, and then users could use it.

@rikatz rikatz added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 5, 2023
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Mar 5, 2023
@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/test-infra repository.

@longwuyuan
Copy link
Contributor

As a open source project, this seems appropriate ;

Maybe another thing is saying that "we allow headers as soon as the ingress controller owner allows headers", so admin would set a list of allowed headers, and then users could use it.

Its a difficult choice.

@cgroschupp
Copy link
Contributor

I created a PR #9742 with a different way to define the headers. I used a configmap for the headers.

@longwuyuan
Copy link
Contributor

longwuyuan commented Mar 16, 2023 via email

@cgroschupp
Copy link
Contributor

With the current implementation of my PR it is not possible to restrict it directly.
One way is to add an admission controller to allow only certain configmaps.

Implementing some kind of whitelist can be a complex task, here are some thoughts:

  • Do we need a way to manage the whitelist per namespace and/or per ingress?
  • Allow a specified header name only, perhaps filtering by a regex pattern.
  • Allow a specified value filter, perhaps filtering by a regex pattern.

@github-actions
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 Apr 16, 2023
@longwuyuan
Copy link
Contributor

This https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#custom-headers exists now . There is very little resources available to allocate to this work. The little developer-time is all occupied with security and Gateway-API work. We are actually stopping support for useful features that are not close implications of the Ingress-API as its hard to support/maintain them.

Since there is no action-item being tracked here and this issue is just adding to the tally of open issues without tracking any action-item, I will close the issue for now.

/close

@k8s-ci-robot
Copy link
Contributor

@longwuyuan: Closing this issue.

In response to this:

This https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#custom-headers exists now . There is very little resources available to allocate to this work. The little developer-time is all occupied with security and Gateway-API work. We are actually stopping support for useful features that are not close implications of the Ingress-API as its hard to support/maintain them.

Since there is no action-item being tracked here and this issue is just adding to the tally of open issues without tracking any action-item, I will close the issue for now.

/close

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.

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

No branches or pull requests

4 participants