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.

Sign up for GitHub

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

Jump to bottom

feat(core): support inbound and outbound Via header #12733

Merged
merged 2 commits into from
Jul 15, 2024
Merged

feat(core): support inbound and outbound Via header #12733

merged 2 commits into from
Jul 15, 2024

Conversation

outsinre
Copy link
Contributor

@outsinre outsinre commented Mar 14, 2024 •
edited
Loading

Summary

According to RFC7230 and RFC9110 gateway MUST append protocol, version and host (or pseudonym) information when forwarding HTTP messages from clients to upstreams. Currently, Kong just ignores this part.

And optionally append the information when forwarding messages in the reverse direction, depending on the headers configuration. Currently, Kong blindly overrides the response Via header with server token.

This PR do the following works:

  1. Append inbound info like 1.1 kong/3.8.0, and outbound info like 1.1 kong/3.8.0.
  2. Supports gRPC and gRPCs protocols, like inbound 2 kong/3.8.0 and outbound 2 kong/3.8.0.
  3. When the upstream does not include the Server header, it inserts one like kong/3.8.0.
  4. Fix Via assignment of aws-lamdba and azure-function plugins.

This is scheduled for 3.8.0.

Checklist

Issue reference

Fix #FTI-5807

@outsinre
Copy link
Contributor Author

Will add tests to spec/02-integration/05-proxy/03-upstream_headers_spec.lua

@bungle
Copy link
Member

bungle commented Mar 14, 2024

Latest RFC: https://datatracker.ietf.org/doc/html/rfc9110#name-via

bungle
bungle reviewed Mar 14, 2024
View reviewed changes
kong/runloop/handler.lua Outdated Show resolved Hide resolved
@github-actions github-actions bot added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Mar 18, 2024
@outsinre outsinre force-pushed the FTI-5807-paypal-kong-gateway-via-header-is-not-conforming-to-the-rfc-standard branch from 1e45f29 to dcbe2db Compare March 19, 2024 04:07
@pull-request-size pull-request-size bot added size/L and removed size/M labels Mar 19, 2024
@AndyZhang0707 AndyZhang0707 requested review from bungle and dndx March 19, 2024 09:34
@outsinre outsinre force-pushed the FTI-5807-paypal-kong-gateway-via-header-is-not-conforming-to-the-rfc-standard branch from dcbe2db to cd54a0e Compare March 20, 2024 02:24
@outsinre outsinre marked this pull request as draft March 20, 2024 03:47
@outsinre outsinre marked this pull request as ready for review March 20, 2024 03:47
chronolaw
chronolaw reviewed Mar 29, 2024
View reviewed changes
spec/02-integration/05-proxy/35-via_spec.lua Outdated Show resolved Hide resolved
kong/runloop/handler.lua Show resolved Hide resolved
kong/runloop/handler.lua Outdated Show resolved Hide resolved
@outsinre outsinre force-pushed the FTI-5807-paypal-kong-gateway-via-header-is-not-conforming-to-the-rfc-standard branch from 986762e to 12a5655 Compare March 29, 2024 09:35
@outsinre
Copy link
Contributor Author

outsinre commented Apr 3, 2024

This is for 3.8.0. Please do not merge it until 3.7.0 GA.

@outsinre
Copy link
Contributor Author

outsinre commented Apr 7, 2024

This is for 3.8.0 as Paypal (customer candidate) may have further requirements.

@outsinre outsinre added this to the 3.8.0 milestone Apr 15, 2024
@outsinre outsinre force-pushed the FTI-5807-paypal-kong-gateway-via-header-is-not-conforming-to-the-rfc-standard branch from 62e2459 to 85521bf Compare July 3, 2024 03:17
@outsinre
Copy link
Contributor Author

outsinre commented Jul 3, 2024

Just did a rebase and waiting for CI.

chobits
chobits reviewed Jul 3, 2024
View reviewed changes
spec/02-integration/05-proxy/35-via_spec.lua Outdated Show resolved Hide resolved
kong/runloop/handler.lua Outdated Show resolved Hide resolved
@bungle
Copy link
Member

bungle commented Jul 4, 2024 •
edited
Loading

A couple of comments:

  1. I would love to see tests added where we have 2.0 as protocol version in incoming (http2/grpc) and 2.0 on upstream when doing grpc_pass (not sure about 1.0, should we test it too). (a real integration test)
  2. this has long been how Kong does it, so it has kinda become a "feature", thus fixing it according to RFC may cause breakage in customers systems, e.g. those that expect these headers to only ever have single value. so just as a not, not sure if that is enough to post-pone this to 4.0 (perhaps product can answer).
  3. additional breakage is proxy_set_header Via $upstream_via;, so any plugin that sets Via currently does not work anymore as they will get overridden (again I am not sure if that is a bad thing, just pointing out)

bungle
bungle previously requested changes Jul 4, 2024
View reviewed changes
kong/runloop/handler.lua Outdated Show resolved Hide resolved
@bungle
Copy link
Member

bungle commented Jul 4, 2024

@outsinre also things like these:
https://github.com/Kong/kong/blob/master/kong/plugins/aws-lambda/handler.lua#L240-L242

chobits
chobits reviewed Jul 11, 2024
View reviewed changes
kong/runloop/handler.lua Outdated Show resolved Hide resolved
chobits
chobits reviewed Jul 11, 2024
View reviewed changes
kong/runloop/handler.lua Outdated Show resolved Hide resolved
chobits
chobits reviewed Jul 11, 2024
View reviewed changes
kong/runloop/handler.lua Outdated Show resolved Hide resolved
@outsinre outsinre force-pushed the FTI-5807-paypal-kong-gateway-via-header-is-not-conforming-to-the-rfc-standard branch from 0bade6e to 308c1c7 Compare July 11, 2024 09:22
@outsinre
feat(core): support inbound and outbound Via header
c314e90 
According to [RFC7230](https://datatracker.ietf.org/doc/html/rfc7230#section-5.7.1)
and [RFC9110](https://datatracker.ietf.org/doc/html/rfc9110#name-via),
Kong gateway MUST append protocol name (optional), protocol version and
host (or pseudonym) information when forwarding HTTP messages from
clients to upstreams. Currently, Kong just ignores this part.

And optionally append the information when forwarding messages in
the reverse direction, depending on the `headers` configuration in
"kong.conf". Currently, Kong blindly overrides the response `Via`
header with server token.

This PR do the following work.

1. Append inbound info like `1.1 kong/3.8.0`, and outbound info
   like `1.1 kong/3.8.0`.
2. Supports gRPC and gRPCs protocols, like inbound `2 kong/3.8.0`
   and outbound `2 kong/3.8.0`.
3. When the upstream does not include the `Server` header, it
   inserts one like `kong/3.8.0`.
4. Fix `Via` assignment of `aws-lamdba` and `azure-function` plugins.
@outsinre outsinre force-pushed the FTI-5807-paypal-kong-gateway-via-header-is-not-conforming-to-the-rfc-standard branch from a69e7f4 to c314e90 Compare July 11, 2024 12:34
chronolaw
chronolaw reviewed Jul 11, 2024
View reviewed changes
kong/runloop/handler.lua Outdated Show resolved Hide resolved
@outsinre @chronolaw
Update kong/runloop/handler.lua
0c1d24e 
Co-authored-by: Chrono <chrono_cpp@me.com>
@hanshuebner hanshuebner dismissed bungle’s stale review July 15, 2024 06:04

Change request was addressed

@hanshuebner hanshuebner merged commit 2da7145 into master Jul 15, 2024
25 checks passed
@hanshuebner hanshuebner deleted the FTI-5807-paypal-kong-gateway-via-header-is-not-conforming-to-the-rfc-standard branch July 15, 2024 06:05
@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chronolaw chronolaw chronolaw approved these changes

@hanshuebner hanshuebner hanshuebner approved these changes

@chobits chobits chobits approved these changes

@bungle bungle bungle left review comments

@dndx dndx Awaiting requested review from dndx

Assignees

@outsinre outsinre

Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/proxy core/templates plugins/aws-lambda plugins/azure-functions size/L
Projects
None yet
Milestone
3.8.0
Development

Successfully merging this pull request may close these issues.
7 participants
@outsinre @bungle @team-gateway-bot @hanshuebner @chobits @dndx @chronolaw