-
Notifications
You must be signed in to change notification settings - Fork 171
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
THREESCALE-6552 - RFE #1255
THREESCALE-6552 - RFE #1255
Conversation
samugi
commented
Jan 26, 2021
- Adds the ability to select a list of upstrams (backends) to apply maintenance mode to + key/value liquid enabled operations.
- Updates a variable in the liquid context when the Upstream Policy changes the Upstream.
@@ -58,6 +58,10 @@ function _M:rewrite(context) | |||
end | |||
end | |||
|
|||
function _M:access(context) | |||
context.route_upstream = context[self] or context.route_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.
is this ok?
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.
so basically you're exposing the upstream URL defined in the the proxy_config during the access phase here right? Do we do this anywhere else? One quick test to check this doesn't break anything is to run the entire test suite locally.
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.
yeah we are doing it here in the routing policy. I thought it would be nice to have a default variable in the context where, should a policy modify the upstream, it would store the new value, so I used the same variable here. This way we would know where to look for an updated upstream, regardless of which policy changed it. The reason I ask is I am not sure this was intended to be used this way and maybe I should use a dedicated variable for the Upstream policy instead.
According to the tests it doesn't break anything at the moment :P
local path2 = uri2.path or "" | ||
path2 = string.gsub(path2, "/$", "") | ||
|
||
return scheme1 == scheme2 and uri1.host == uri2.host and port1 == port2 and path1 == path2 |
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.
Why are you checking the path too?
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.
@palmieric fixed it, I was supposed to "match" the paths instead of using ==. Thanks!
43ee5e6
to
0593a5e
Compare
@@ -58,6 +58,10 @@ function _M:rewrite(context) | |||
end | |||
end | |||
|
|||
function _M:access(context) | |||
context.route_upstream = context[self] or context.route_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.
so basically you're exposing the upstream URL defined in the the proxy_config during the access phase here right? Do we do this anywhere else? One quick test to check this doesn't break anything is to run the entire test suite locally.
before_each(function() | ||
ctx = { service = { api_backend = 'http://backend2.example.org:80' } } | ||
ctx2 = { |
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.
does this mean we have 2 different instances of the context object during runtime? that could be problematic no as from anywhere we only ever reference it as context
or ctx
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.
my intention here was to use a different context (ctx2) just for some of the tests. I guess I can also achieve this by modifying the same ctx object from within the test where I need a different value.
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.
fixed this as per our discussion, I also added a couple more tests.
0593a5e
to
b20cb03
Compare
49d4245
to
0330364
Compare
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.
Now only nitpicks! I'm happy to see this PR :-)
@@ -19,20 +22,54 @@ function _M.new(configuration) | |||
policy.message_content_type = default_message_content_type | |||
|
|||
if configuration then | |||
policy.maintenance_upstreams = configuration.upstreams |
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.
This is no longer needed no?
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.
yup, forgot it :)
ngx.header['Content-Type'] = self.message_content_type | ||
ngx.status = self.status_code | ||
ngx.say(self.message) | ||
|
||
return ngx.exit(ngx.status) | ||
end | ||
|
||
function _M:access(context) | ||
-- Before the condition was not set, so the maintenance mode when condition is |
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.
In this past, this policy does not have condition. So by default, if condition is not set it set the maintenance mode.
end | ||
|
||
context.upstream = context.route_upstream or context:get_upstream() or {} | ||
context.upstream = context.upstream.uri or context.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.
This is incorrect, no?
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.
local upstream = context.route_upstream or context:get_upstream() or {}
context.upstream = upstream.uri or upstream
This is a bit better
context('when Maintenance Mode is configured for a specific upstream', function() | ||
local maintenance_policy = MaintenancePolicy.new({ | ||
condition = { | ||
operations={{op="==", left="{{ upstream.scheme }}://{{ upstream.host }}{{ upstream.port }}{{ upstream.path }}", left_type="liquid", right="https://"..test_host.."443"..test_path, right_type="plain"}}, |
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 would indent this a bit, in all testsuite TBH.
end) | ||
end) | ||
|
||
context('when Maintenance Mode is configured for a specific upstream (different host)', function() |
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.
This can be all in the it expression TBH, and the same for all others
--- response_body | ||
{ "msg": "Be back soon" } | ||
--- response_headers | ||
Content-Type: application/json | ||
--- error_code: 503 | ||
--- no_error_log | ||
[error] | ||
|
||
|
||
=== TEST 5: Maintenance mode is applied with routing policy + matching upstream condition |
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 missing here the following test:
-> Works with api-backend, I think that it's not longer used by system, but just in case.
-> A OR condition, where the first is not matching, but matches the second one. It's covered by unit, but I prefer to have here too.
Regards
0330364
to
5ec3282
Compare
Add a Changelog and it'll be merged :-) |
5ec3282
to
e677830
Compare
all done! |