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

Cannot iterate on headers from kong.service.response.get_headers() #11546

Closed
1 task done
mfeledyn opened this issue Sep 11, 2023 · 9 comments · Fixed by #12006
Closed
1 task done

Cannot iterate on headers from kong.service.response.get_headers() #11546

mfeledyn opened this issue Sep 11, 2023 · 9 comments · Fixed by #12006
Assignees
Labels
pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... task/bug

Comments

@mfeledyn
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Kong version ($ kong version)

3.4.0

Current Behavior

Trying to iterate over kong.service.response headers which should be a table. I cannot iterate on the object returned by kong.service.response.get_headers()

Expected Behavior

I expected to be able to iterate on the object returned by kong.service.response.get_headers() to traverse all headers.

Steps To Reproduce

Make a test Lua plugin for Kong.
For response() function of the test plugin, here is the code I use:

function _M:response(config)
    kong.log.info(log_prefix().." - INSPECT(kong.service.response.get_headers())="..inspect(kong.service.response.get_headers()))
    # gives {  <metatable> = {    __index = <function 1>  } }  - I expected a table of headers

    kong.log.info(log_prefix().." - INSPECT(#kong.service.response.get_headers())="..inspect(#kong.service.response.get_headers()))
    # gives 0 - I expected the headers count

    kong.log.info(log_prefix().." - INSPECT(kong.service.response.get_headers()[\"Server\"])="..inspect(kong.service.response.get_headers()["Server"]))
    # gives "My GO test server" - Works as expected

    # The 2 "for" loops below try to log all headers, but do not log anything
    for k, v in pairs(kong.service.response.get_headers()) do
      kong.log.info(log_prefix().."header: key="..k..", value="..json.encode(v))
    end
    for k, v in ipairs(kong.service.response.get_headers()) do
      kong.log.info(log_prefix().."header: key="..k..", value="..json.encode(v))
    end
end

In summary I can access a service.response header if I know its name. I cannot iterate over headers, nor know headers count.

Anything else?

No response

@StarlightIbuki
Copy link
Contributor

StarlightIbuki commented Sep 13, 2023

We're using metatable to handle name aliases (underscore vs. hyphen, upper vs. lowercase characters) and max headers limitation, thus you find an empty table with inspect().
It's possible to support ipairs/pairs and # with some modification to the metatable implementation. Also, maybe we could introduce a parameter or another API to allow fetching raw headers without this processing.
We will be discussing this. Ideas are welcomed.

@StarlightIbuki StarlightIbuki added the task/feature Requests for new features in Kong label Sep 13, 2023
@hanshuebner hanshuebner added task/bug and removed task/feature Requests for new features in Kong labels Sep 18, 2023
@hanshuebner hanshuebner self-assigned this Sep 18, 2023
@hanshuebner
Copy link
Contributor

Created internal ticket KAG-2602

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

This issue is marked as stale because it has been open for 14 days with no activity.

@github-actions github-actions bot added the stale label Oct 5, 2023
@mfeledyn
Copy link
Author

mfeledyn commented Oct 5, 2023

Hello !
I don't have a JIRA account and cannot access the ticket. Hope this is ongoing to resolution :-)
Also this thread is now stale so I hope it won't be forgotten.
Cheers!

@github-actions github-actions bot removed the stale label Oct 6, 2023
@StarlightIbuki
Copy link
Contributor

@mfeledyn Hi. Thanks for caring. We maintain an internal task track system and that system is not open. We've created the ticket but no conclusion has been made yet. I'm confirming if this can be scheduled recently.

@StarlightIbuki
Copy link
Contributor

I did a little bit of research on this and noticed that this is not a very trivial problem. Referer: openresty/lua-nginx-module#1595

I've created a fix, but it also changes some of the behavior.

@StarlightIbuki
Copy link
Contributor

As it's not very clear how this should be resolved, I'd like to doc the current behavior and leave it for now.
If possible, could you share the use case of this? So we can evaluate and see if some workaround could be applied.

@StarlightIbuki StarlightIbuki added the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Nov 13, 2023
@mfeledyn
Copy link
Author

As it's not very clear how this should be resolved, I'd like to doc the current behavior and leave it for now. If possible, could you share the use case of this? So we can evaluate and see if some workaround could be applied.

Thx for your feed-back on this.

The use case is to trace the headers sent in service's response. The work around I found is to use kong.response.get_headers() in a very high priority plugin so it gets the headers before any other plugin can modify them.

For now I consider it's not only a documentation problem, because the kong.service.response.get_headers() name is simply misleading. This function should be removed and documented as missing.

@StarlightIbuki
Copy link
Contributor

The use case is to trace the headers sent in service's response. The work around I found is to use kong.response.get_headers() in a very high priority plugin so it gets the headers before any other plugin can modify them.

Glad to know a workaround works for you.

For now I consider it's not only a documentation problem, because the kong.service.response.get_headers() name is simply misleading. This function should be removed and documented as missing.

I agree. The current behavior is not logical (the API does not function as designed) and it should be eventually fixed. However, it is not a top priority at the moment and a resolution is not planned in the foreseeable future.

We welcome community PRs. If you are interested, we could use PR #11708 as a starting point for your involvement. Also, please be assured that there is no pressure to contribute.

bungle pushed a commit that referenced this issue Nov 14, 2023
gszr added a commit that referenced this issue Nov 3, 2024
This commit attempts to fix several issues in the
`kong.service.response.get_headers()` module:

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.

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).
gszr added a commit that referenced this issue Nov 3, 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., #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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... task/bug
Projects
None yet
3 participants