ingress-nginx
ingress-nginx copied to clipboard
Set grpc :authority header from request header
What this PR does / why we need it:
This is to fix that the :authority
header by default is set to upstream_balancer
, this PR sets :authority
header from request :authority
header.
Note using in nginx http2 :authority
header merged with the Host header concept, so grpc_set_header Host $http_host
actually sets :authority
together with Host header
Types of changes
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] Documentation only
Note this is breaking change
- If people don't have
grpc_set_header Host
inconfiguration_snippet
, it means the:authority
header will change fromupstream_balancer
to the request:authority
header. This part I think it's fine because no one will rely on the:authority
header to beupstream_balancer
- if people have
grpc_set_header Host
inconfiguration_snippet
, they will need to change it back to usenginx.ingress.kubernetes.io/upstream-vhost
annotation
Which issue/s this PR fixes
fixes #8843
How Has This Been Tested?
Haven't got time to run it yet, will run ingress nginx e2e test
Checklist:
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I've read the CONTRIBUTION guide
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: jchenship / name: Junrui Chen (441996a63a04046652c5a1204f8bcfcbc7c1d64f)
@jchenship: 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.
Welcome @jchenship!
It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.
You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.
You can also check if kubernetes/ingress-nginx has its own contribution guidelines.
You may want to refer to our testing guide if you run into trouble with your tests not passing.
If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!
Thank you, and welcome to Kubernetes. :smiley:
Hi @jchenship. Thanks for your PR.
I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test
on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test
label.
I understand the commands that are listed here.
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.
/ok-to-test
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: EUFELIPEPIMENTEL, jchenau
Once this PR has been reviewed and has the lgtm label, please assign strongjz for approval by writing /assign @strongjz
in a comment. For more information see:The Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve
in a comment
Approvers can cancel approval by writing /approve cancel
in a comment
/assign
I've been exposed to some use cases and I'd like to pick up this PR to continue after I dig deeper. thanks
just a kind follow up, is there any update @tao12345666333 ? thank you
Sorry for the delay, I will do this today.
After my inspection, this content was introduced by https://github.com/kubernetes/ingress-nginx/pull/4212 and https://github.com/kubernetes/ingress-nginx/issues/3706#issuecomment-495157068
The NGINX used in the project at that time was v1.17.0.
There are some changes in this behavior between versions. I need a moment to verify. Also, as you commented in the issue, both Kong and APISIX have done something about this part of the behavior
Just chipping in to report that I managed to apply this PR on the current 1.7.0 main branch and build a docker image and reference it in my helm deployment on my cluster and it fixed my issue with host being reported as "upstream_balancer" in my application. Personally I would approve the PR.
I'm glad to hear your feedback. @Dunge
I could really use this :). Are we waiting on anything before merging?
Is there anything (annotation/snippet etc) that I can add to my Ingress to set :authority
to be host with the grpc annotation while we wait for this to be merged?
Sorry for the long delay.
In fact, I tested various scenarios, including whether to use it only as an Ingress or place it in the Istio service mesh. In addition, I also tested cases involving mixed use of multiple protocols and various annotation settings. Sorry for being interrupted many times. Now we can finally merge this PR.
/lgtm /approve
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: lapwingcloud, tao12345666333
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [tao12345666333]
Approvers can indicate their approval by writing /approve
in a comment
Approvers can cancel approval by writing /approve cancel
in a comment