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

[wip/proposal] fix(pdk): service.response.get_headers behavior #13827

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gszr
Copy link
Member

@gszr gszr commented Nov 4, 2024

This commit attempts to fix several issues in the
kong.service.response.get_headers() module.

Issues

  1. Said method must only return response headers set by the upstream service. However, since its introcution, it is broken; in the original implementation, it added an __index metamethod to the full headers table (both Kong and upstream headers) to ensure a given header being indexed was found in a ngx.var.upstream_http_<header_name> variable (ie, it was set by the upstream). However, it responded with the table itself, which contained the headers, so the __index metamethod would never be called. Summarizing, since its introduction, the method has been returning both Kong and upstream headers. Tests were also broken due to a case-sensitiveness issue (implementation described above bypased the original headers table metatable, effectively breaking case-insensitiveness implemented by the ngx.resp.get_headers API).

  2. Buffered proxying, later added, introduced new logic to handle buffered headers. This logic corretly implemented the metamethod by using a so-called "proxy table"; this is an empty table to which the __index metamethod is added. Since it's empty, __index is correctly invoked and ensures only upstream headers are accessible (as described by 1.). However, being empty it does not allow the headers table to be iterated on, leading to inconsistent and unexpected behavior -- a source of bug reports (e.g., Cannot iterate on headers from kong.service.response.get_headers() #11546, KAG-4866, to name two).

What does it change?

  1. Standardize the interface: both buffered and unbuffered respond with an iterable table.
  2. Add more tests, for both buffered and unbuffered modes
    • test that returned table is indeed iterable
    • test that returned table only has upstream headers

Caveats

  • Buffered proxying already reads all headers; adding additional logic to limit by max_headers arguably does not make sense.
  • Calls to this method will always respond with less than max_headers, as the call to ngx.resp.get_headers includes both Kong and upstream headers and we filter out non-upstream headers after said call. The ideal solution would use a native API that reads upstream only headers directly, and has a similar interface as ngx.resp.get_headers, taking a max_headers argument.

KAG-4866

This commit attempts to fix several issues in the
`kong.service.response.get_headers()` module.

Issues
======

1. Said method must only return response headers set by the upstream
   service. However, since its introcution, it is broken; in the
   original implementation, it added an `__index` metamethod to the full headers
   table (both Kong and upstream headers) to ensure a given header being indexed
   was found in a `ngx.var.upstream_http_<header_name> variable (ie, it was
   set by the upstream). However, it responded with the table itself, which
   contained the headers, so the `__index` metamethod would never be called.
   Summarizing, since its introduction, the method has been returning
   both Kong and upstream headers. Tests were also broken due to a
   case-sensitiveness issue (implementation described above bypased the
   original headers table metatable, effectively breaking
   case-insensitiveness implemented by the `ngx.resp.get_headers` API).

2. Buffered proxying, later added, introduced new logic to handle
   buffered headers. This logic corretly implemented the metamethod by
   using a so-called "proxy table"; this is an empty table to which the
   `__index` metamethod is added. Since it's empty, `__index` is
   correctly invoked and ensures only upstream headers are accessible
   (as described by 1.). However, being empty it does not allow the
   headers table to be iterated on, leading to inconsistent and
   unexpected behavior -- a source of bug reports (e.g., #11546, KAG-4866,
   to name two).

What does it change?
====================

1. Standardize the interface: both buffered and unbuffered respond
   with an iterable table.
2. Add more tests, for both buffered and unbuffered modes
    - test that returned table is indeed iterable
    - test that returned table only has upstream headers

Caveats
=======

- Buffered proxying already reads all headers; adding additional logic
  to limit by `max_headers` arguably does not make sense.
- Calls to this method will always respond with less than `max_headers`,
  as the call to `ngx.resp.get_headers` includes both Kong and upstream
  headers and we filter out non-upstream headers after said call. The
  ideal solution would use a native API that reads upstream only headers
  directly, and has a similar interface as `ngx.resp.get_headers`,
  taking a `max_headers` argument.
@github-actions github-actions bot added core/pdk cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Nov 4, 2024
@gszr gszr requested a review from bungle November 4, 2024 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/pdk size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant