-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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): X-Kong-Upstream-Status header is not set #12744
Conversation
7b9ec99
to
d7df62d
Compare
…when the response is returned from the cache of `proxy-cache` plugin. FTI-5827
d7df62d
to
aba3f35
Compare
@@ -1438,7 +1438,7 @@ return { | |||
|
|||
local upstream_status_header = constants.HEADERS.UPSTREAM_STATUS | |||
if kong.configuration.enabled_headers[upstream_status_header] then | |||
local upstream_status = ctx.buffered_status or tonumber(sub(var.upstream_status or "", -3)) | |||
local upstream_status = ctx.buffered_status or tonumber(sub(var.upstream_status or "", -3)) or ngx.status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it would be more appropriate to set a special literal like nil
or 0
rather than setting it to ngx.status
. After all, in this case, the request doesn't actually reach the upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ms2008 If the response is returned from upstream, we set the value of this header with either ctx.buffered_status
(in case it is a buffered response) or var.upstream_status
(respect the status from upstream). In the case that the response is a cached one, we have to respect the status that's already cached. In the logic of proxy-cache
plugin, the ngx.status
is set with last cached status. Thus, I suppose here it should consider ngx.status
as the last alternative.
nil
is not a suitable one, as it is not a valid value for a header. And 0
is not suitable as well, as it doesn't coincide with the cached response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that sounds reasonable too. But I think in this scenario, we should clearly define the return value of these X-Kong-Upstream-*
response headers, otherwise it will be confusing for the user.
Lines 921 to 925 in ed0b96d
# - `X-Kong-Upstream-Status`: The HTTP status | |
# code returned by the upstream service. | |
# This is particularly useful for clients to | |
# distinguish upstream statuses if the | |
# response is rewritten by a plugin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me revise our doc and describe this more clearly: This header always keep same with the status cached by proxy-cache plugin when requests hit the cache.
Successfully created cherry-pick PR for |
FTI-5827 Related-pr: - Kong/kong#12744 - Kong/kong-ee#8609
Summary
Fix a bug that
X-Kong-Upstream-Status
header is not set when the response is provided by theproxy-cache
plugin.Checklist
changelog/unreleased/kong
orskip-changelog
label added on PR if changelog is unnecessary. README.mdIssue reference
FTI-5827