ingress-nginx icon indicating copy to clipboard operation
ingress-nginx copied to clipboard

Set grpc :authority header from request header

Open lapwingcloud opened this issue 2 years ago • 5 comments

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

  1. If people don't have grpc_set_header Host in configuration_snippet, it means the :authority header will change from upstream_balancer to the request :authority header. This part I think it's fine because no one will rely on the :authority header to be upstream_balancer
  2. if people have grpc_set_header Host in configuration_snippet, they will need to change it back to use nginx.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.

lapwingcloud avatar Aug 08 '22 09:08 lapwingcloud

CLA Signed

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.

k8s-ci-robot avatar Aug 08 '22 09:08 k8s-ci-robot

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:

k8s-ci-robot avatar Aug 08 '22 09:08 k8s-ci-robot

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.

k8s-ci-robot avatar Aug 08 '22 09:08 k8s-ci-robot

/ok-to-test

Volatus avatar Aug 25 '22 20:08 Volatus

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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot avatar Aug 28 '22 06:08 k8s-ci-robot

/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

tao12345666333 avatar Sep 27 '22 14:09 tao12345666333

just a kind follow up, is there any update @tao12345666333 ? thank you

lapwingcloud avatar Nov 03 '22 01:11 lapwingcloud

Sorry for the delay, I will do this today.

tao12345666333 avatar Feb 17 '23 01:02 tao12345666333

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

tao12345666333 avatar Feb 18 '23 00:02 tao12345666333

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.

Dunge avatar Apr 04 '23 21:04 Dunge

I'm glad to hear your feedback. @Dunge

tao12345666333 avatar May 08 '23 22:05 tao12345666333

I could really use this :). Are we waiting on anything before merging?

fordneild avatar Jun 15 '23 17:06 fordneild

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?

fordneild avatar Jun 21 '23 18:06 fordneild

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

tao12345666333 avatar Jun 27 '23 06:06 tao12345666333

[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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot avatar Jun 27 '23 06:06 k8s-ci-robot