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.

Sign up for GitHub

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

Routing Policy enhancements #1103

Merged
merged 8 commits into from
Sep 9, 2019
+226 −2
Merged

Routing Policy enhancements #1103

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7906bd0
Executor: Add original request information.
eloycoto Aug 1, 2019
a4ad1c0
Policy: routing add liquid matching
eloycoto Aug 1, 2019
12ff1a4
Policy: Add unittest for routing changes.
eloycoto Aug 1, 2019
3c2efc5
Policy: Add routing policy liquid integration test.
eloycoto Aug 1, 2019
165958c
Policy: routing add liquid use case
eloycoto Aug 1, 2019
62af28c
Update Changelog
eloycoto Aug 1, 2019
a177a6b
Liquid: Add request path to liquid variables.
eloycoto Aug 16, 2019
b8ea649
Executor: Original request validation on shared_build_context
eloycoto Sep 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Executor: Original request validation on shared_build_context 
To make the code simpler to read change the store_original_request
function call into the shared_build_context to make it clear what
happens.

Signed-off-by: Eloy Coto <eloy.coto@gmail.com>
@eloycoto
eloycoto committed Sep 6, 2019
commit b8ea64953e5b7dad45e85a439a64279c9a9d2bd0
6 changes: 5 additions & 1 deletion gateway/src/apicast/executor.lua
View file
Original file line number Diff line number Diff line change
@@ -44,7 +44,8 @@ local function store_original_request(context)
-- is not a good method to check this. [1]
-- [0] invalid phases: init_worker, init, timer and ssl_cer
-- [1] https://github.com/openresty/lua-resty-core/blob/9937f5d83367e388da4fcc1d7de2141c9e38d7e2/lib/resty/core/request.lua#L96
if context.original_request then
--
if not context or context.original_request then
return
end

@@ -70,6 +71,9 @@ local function shared_build_context(executor)
if not context then
context = build_context(executor)
ctx.context = context
end

if not ctx.original_request then
store_original_request(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this guarantee that the original request will be initialized on every request?

In other words, store_original_request only runs when the context has not been initialized, but store_original_request fails in some phases, so I wonder if the following case is possible:

  • The body of this if runs and initializes ctx.context
  • store_original_request is called but fails to assign the original request data
  • The body of this if is not executed again and the original request data is never initialized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

due to error happens in the same function call, I do not think that it's the case.

Copy link
Contributor

@davidor davidor Sep 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@eloycoto I've reviewed the latest changes of the PR and everything looks good to me except this. Maybe I'm missing something.

I see 2 scenarios:

  • context is nil. store_original_request is called and it fails (there's a pcall so there are cases where it can fail). In that case, the original request would never be initialized in the context.
  • context is nil, but when store_original_request is called it never fails. In that case, the pcall would be unnecessary.

What am I missing?

end