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

fix(core): response phase skips header.before #10056

Merged
merged 7 commits into from
Jan 20, 2023

Conversation

StarlightIbuki
Copy link
Contributor

This causes some of the header handlings to be skipped.

Full changelog:

runloop.lua: moving the header phase before and after handlers and changing the way to get status in before handler
init.lua: adjust the ordering of handlers and set status (send headers).

Fix #10031

@bungle
Copy link
Member

bungle commented Jan 4, 2023

As discussed offline, I feel the right fix would be to change this:
https://github.com/Kong/kong/blob/master/kong/runloop/handler.lua#L1727

to

header[upstream_status_header] = ctx.buffered_status or tonumber(sub(var.upstream_status or "", -3))

@StarlightIbuki
Copy link
Contributor Author

As discussed offline, I feel the right fix would be to change this: https://github.com/Kong/kong/blob/master/kong/runloop/handler.lua#L1727

to

header[upstream_status_header] = ctx.buffered_status or tonumber(sub(var.upstream_status or "", -3))

Does this work the same way:

      local upstream_status = ctx.buffered_status or var.upstream_status
      header[upstream_status_header] = tostring(tonumber(sub(upstream_status or "", -3)))

@bungle
Copy link
Member

bungle commented Jan 6, 2023

As discussed offline, I feel the right fix would be to change this: https://github.com/Kong/kong/blob/master/kong/runloop/handler.lua#L1727
to

header[upstream_status_header] = ctx.buffered_status or tonumber(sub(var.upstream_status or "", -3))

Does this work the same way:

      local upstream_status = ctx.buffered_status or var.upstream_status
      header[upstream_status_header] = tostring(tonumber(sub(upstream_status or "", -3)))

I think not. There is only possible multiple status codes on var, as it collects all status codes (from possible retries with balancer), but i think just one code in ctx that is read from location capture res.status.

@StarlightIbuki
Copy link
Contributor Author

I think not. There is only possible multiple status codes on var, as it collects all status codes (from possible retries with balancer), but i think just one code in ctx that is read from location capture res.status.

I've updated the fix according to your comment.

Comment on lines 1436 to 1437
local upstream_status = ctx.buffered_status or tonumber(sub(var.upstream_status or "", -3))
header[upstream_status_header] = tostring(upstream_status)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you added tostring here. Not sure why. Perhaps then the tonumber above is unwanted. That is now it can set header to "nil". Not sure can it happen but still. Perhaps just remove the tostring?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing. I was trying to eliminate a warning.

@dndx dndx force-pushed the fix/response_phase_header_handle branch from 359e260 to 2376517 Compare January 20, 2023 16:36
@dndx dndx merged commit d9d3725 into master Jan 20, 2023
@dndx dndx deleted the fix/response_phase_header_handle branch January 20, 2023 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Response header 'X-Kong-Upstream-Status' is eliminated while custom plugin overwrite :response() function
4 participants