fix(plugins): fix competing redis configs #12343
Conversation
|
|
||
| namespace = conf.extra_options.namespace or conf.namespace, -- allow conf.namespace until 4.0 version | ||
| scan_count = conf.extra_options.scan_count or conf.scan_count, -- allow conf.scan_count until 4.0 version |
There was a problem hiding this comment.
Unfortunately this approach did not work because conf.extra_options.scan_count was populated with a default value o 10 so even if the user tried to configure the plugin using the old way it would get overwritten by the db default.
| @@ -363,123 +363,151 @@ describe("Plugin: rate-limiting (integration)", function() | |||
| end | |||
| end) | |||
| end) | |||
| end -- for each redis strategy | |||
| end | |||
There was a problem hiding this comment.
I noticed that the nesting was incorrect and those two tests executed in the for loop unnecessarily. I've just changed the nesting one level up.
I recommend viewing this file with the option to hide whitespace changes:
|
|
||
| local json = cjson.decode(assert.res_status(201, res)) | ||
|
|
||
| -- verify that legacy config got written into new structure |
There was a problem hiding this comment.
These assertions would fail right now on master branch.
|
Skipping changelog because it's a fix for a feature that's in development. |
| @@ -1670,7 +1670,11 @@ function Schema:process_auto_fields(data, context, nulls, opts) | |||
| local new_values = sdata.func(value) | |||
| if new_values then | |||
| for k, v in pairs(new_values) do | |||
| data[k] = v | |||
| if type(v) == "table" then | |||
There was a problem hiding this comment.
The previous approach of just overwriting data with new value would not work if I wanted to nest data from shorthand in a table. Therefore if the new value is a table I need to merge it with current data.
nowNick
force-pushed
the
fix/redis-competing-configs-using-shorthands
branch
3 times, most recently
from
31f1240 to
25df512
Compare
nowNick
force-pushed
the
fix/redis-competing-configs-using-shorthands
branch
from
25df512 to
e61f11d
Compare
| ssl_server_name = conf.server_name, | ||
|
|
||
| namespace = conf.extra_options.namespace or conf.namespace, -- allow conf.namespace until 4.0 version | ||
| scan_count = conf.extra_options.scan_count or conf.scan_count, -- allow conf.scan_count until 4.0 version | ||
| namespace = conf.extra_options.namespace, | ||
| scan_count = conf.extra_options.scan_count, |
There was a problem hiding this comment.
I agree on the path towards unification of config fields, but I'm wondering whether the new names read better than the old ones. For example, changing ssl-server_name to server_name seems making the name more ambigious, and an additional extra_options level seems unnessary.
As we are going to bring this breaking change in 4.0 anyway, does it make sense to change the kong/enterprise_edition/init.lua side instead? How do you think?
There was a problem hiding this comment.
On the other side, (I just caught up the design doc), as we are planning to add
redis configuration to a first class entity, should we just drop all the redis field from
plugin config and allows only config.redis.id = <the redis entity ID> form instead
of bringing this transitional change?
There was a problem hiding this comment.
The transitional step is being done to simplify the move from a plugin-individual redis configuration to a unified-global redis configuration.
In this step, we're moving from the ssl-server_name that is a non-prefixed redis specific field to redis.server_name for example.
The next step (implementing redis partials), will replace redis block with a reference, while maintaining the strucutre.
Regarding the extra_options - Some plugins have unique configuration options that are not unifiable in a global schema, so there needs to be a mechanism to include custom options in there.
ACME, RateLimiting and Response-RateLimiting now use the same redis configuration structure. The olds fields were left in place to maintain backwards compatibility. When resolving the configuration we looked into new fields and if they were empty then fallback to legacy fields. Unfortunately the new fields have their defaults as well which get written into db - so at the time of plugin resolution we'd have to implement complex logic to figure out if the new value came from user or from defualt. This approach removes the olds fields and uses shorthands to maintain backwards compatibility. KAG-3388
locao
force-pushed
the
fix/redis-competing-configs-using-shorthands
branch
from
e61f11d to
e1663b5
Compare
Summary
ACME, RateLimiting and Response-RateLimiting now use the same redis configuration structure. The olds fields were left in place to maintain backwards compatibility. When resolving the configuration we looked into new fields and if they were empty then fallback to legacy fields. Unfortunately the new fields have their defaults as well which get written into db - so at the time of plugin resolution we'd have to implement complex logic to figure out if the new value came from user or from defualt.
This approach removes the olds fields and uses shorthands to maintain backwards compatibility.
Checklist
A changelog file has been created underchangelog/unreleased/kongorskip-changeloglabel added on PR if changelog is unnecessary. README.mdIssue reference
KAG-3388