Routing Policy enhancements #1103
Conversation
eloycoto
force-pushed
the
Issue1084
branch
3 times, most recently
from
9bcfe76 to
5772edf
Compare
eloycoto
force-pushed
the
Issue1084
branch
4 times, most recently
from
9a65c86 to
a622f85
Compare
gateway/src/apicast/executor.lua
Outdated
| -- [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 | ||
|
|
||
| local result, _ = pcall(function() |
There was a problem hiding this comment.
Should we have an if here that sets context.original_request only when it's nil?
Otherwise, we might overwrite this var when some policy has already modified the headers, or the path. The shared_build_context function might guarantee that this never happens, but I'm not 100% sure.
Also, I have doubts about this. You mention that this is going to fail in the init_worker phase. However, the shared_build_context function will build the context in that phase and store it in ctx.context. Subsequent calls to the method will use what's stored in ctx.context instead of building again the context, so when is original_request initialized?
|
Hi, Finally, I remove the linked list to ngx.var due to the security concerts; some critical data can get from "ngx.var" variable (number of connections, client certs, etc..) I was a bit optimistic when I set that, so it's as it was the only thing that I added is the request path to the ngx variable. Thanks, David, for the catch! |
CHANGELOG.md
Outdated
| @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). | |||
| - Option to load service configurations one by one lazily [PR #1099](https://github.com/3scale/APIcast/pull/1099) | |||
| - New maintenance mode policy, useful for maintenance periods. [PR #1105](https://github.com/3scale/APIcast/pull/1105), [THREESCALE-3189](https://issues.jboss.org/browse/THREESCALE-3189) | |||
| - Remove dnsmasq process for APIcast [PR #1090](https://github.com/3scale/APIcast/pull/1090), [THREESCALE-1555](https://issues.jboss.org/browse/THREESCALE-1555) | |||
| - Enable liquid operations and original request variable on routing policy [PR #1103](https://github.com/3scale/APIcast/pull/1103) | |||
There was a problem hiding this comment.
Remember to include the JIRA as well.
| ctx.context = context | ||
| store_original_request(ctx) |
There was a problem hiding this comment.
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
ifruns and initializesctx.context store_original_requestis called but fails to assign the original request data- The body of this
ifis not executed again and the original request data is never initialized
There was a problem hiding this comment.
due to error happens in the same function call, I do not think that it's the case.
There was a problem hiding this comment.
@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:
contextis nil.store_original_requestis called and it fails (there's apcallso there are cases where it can fail). In that case, the original request would never be initialized in the context.contextis nil, but whenstore_original_requestis called it never fails. In that case, thepcallwould be unnecessary.
What am I missing?
eloycoto
force-pushed
the
Issue1084
branch
2 times, most recently
from
1c478ea to
d963afd
Compare
To be able to retrieve original request information on the policies without adding/deleting headers. This change allows users to handle routing policy with the original information, full disclosure on issue 3scale#1084 Fix 3scale#1084 Signed-off-by: Eloy Coto <[email protected]>
This change add liquid matching to the routing policy. The change allow
users to route based on different information that maybe the current
policies are not allowed to.
The example use case here is to route based on original_request
variable, so the user can render the information using liquid and get a
way to route in a better way.
Example config:
```
{
"services": [
{
"id": 42,
"proxy": {
"hosts": [
"one"
],
"policy_chain": [
{
"name": "apicast.policy.routing",
"configuration": {
"rules": [
{
"url": "https://echo-api.3scale.net/",
"condition": {
"operations": [
{
"match": "liquid",
"liquid_value": "{{original_request.path}}",
"op": "matches",
"value": "/bridge-1"
}
]
}
}
]
}
},
{
"name": "url_rewriting",
"configuration": {
"commands": [
{
"op": "sub",
"regex": "^/bridge",
"replace": "/"
}
]
}
},
{
"name": "apicast.policy.echo"
}
]
}
}
]
}
```
Signed-off-by: Eloy Coto <[email protected]>
Signed-off-by: Eloy Coto <[email protected]>
Signed-off-by: Eloy Coto <[email protected]>
Signed-off-by: Eloy Coto <[email protected]>
Signed-off-by: Eloy Coto <[email protected]>
To be able to filter on routing policy based on the path that it's currently set. Signed-off-by: Eloy Coto <[email protected]>
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 <[email protected]>
Hi,
Multiple commits here:
Other commits are to allow liquid match in the routing policy to accomplish @ppatierno feedback.
Regards