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: ctx being contaminated due to a new feature of openresty 1.19 #3105

Merged
merged 4 commits into from
Dec 24, 2020

Conversation

nic-chen
Copy link
Member

What this PR does / why we need it:

A new feature has been added in OpenResty 1.19.3.1:
Shared ngx.ctx among SSL_* phases and the following phases.

In the ssl phase, ctx is created but not cleared.

In the access phase, it is judged that if ctx already exists, it will not be created.

This causes a same client to access an https service and then access another http service, it will be reused the previous ctx, resulting in this bug.

fix #3079

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
    we could add test cases later for quick fix. we could track it by test: add TLS testcase in apisix #3103
  • Have you modified the corresponding document?
    we don't need to modify docs for this change.
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

Copy link
Member

@moonming moonming left a comment

Choose a reason for hiding this comment

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

I don't think this is a good title of PR, and we need test cases

@membphis
Copy link
Member

related issue: #3103

@@ -185,6 +185,7 @@ function _M.http_ssl_phase()
end
ngx_exit(-1)
end
ngx.ctx = nil
Copy link
Member

Choose a reason for hiding this comment

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

need comments

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

ngx_ctx.api_ctx = api_ctx
end
local api_ctx = core.tablepool.fetch("api_ctx", 0, 32)
ngx_ctx.api_ctx = api_ctx
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

@nic-chen
Copy link
Member Author

I don't think this is a good title of PR, and we need test cases

@moonming
we confirmed the changes in the user's env.
maybe we could add test cases later for quick fix. we could track it by #3103

@nic-chen nic-chen changed the title fix: the bug that traffic is forwarded to wrong upstream due to ctx being contaminated fix: ctx being contaminated due to a new feature of openresty 1.19 Dec 22, 2020
@membphis
Copy link
Member

@nic-chen CI failed

@spacewander spacewander merged commit 5de9990 into apache:master Dec 24, 2020
@membphis
Copy link
Member

@nic-chen please update your source code editor to avoid those issue: 7f00004

@nic-chen
Copy link
Member Author

@nic-chen please update your source code editor to avoid those issue: 7f00004

OK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

apisix转发流量错误
6 participants