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

fix(plugins): fix competing redis configs #12343

Merged
merged 1 commit into from
Jan 15, 2024
Merged

fix(plugins): fix competing redis configs #12343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion kong/db/schema/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

data[k] = tablex.merge(data[k] or {}, v, true)
else
data[k] = v
end
end
end
end
Expand Down
76 changes: 34 additions & 42 deletions kong/plugins/acme/schema.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,28 +38,42 @@ local SHM_STORAGE_SCHEMA = {
local KONG_STORAGE_SCHEMA = {
}

-- deprecated old schema
local REDIS_LEGACY_SCHEMA_FIELDS = {
{ auth = { type = "string", referenceable = true, description = "The Redis password to use for authentication. " } },
{ ssl_server_name = typedefs.sni { required = false, description = "The expected server name for the SSL/TLS certificate presented by the Redis server." }},
{
namespace = {
type = "string",
description = "A namespace to prepend to all keys stored in Redis.",
required = true,
default = "",
len_min = 0,
custom_validator = validate_namespace
}
},
{ scan_count = { type = "number", required = false, default = 10, description = "The number of keys to return in Redis SCAN calls." } },
local LEGACY_SCHEMA_TRANSLATIONS = {
{ auth = {
type = "string",
func = function(value)
deprecation("acme: config.storage_config.redis.auth is deprecated, please use config.storage_config.redis.password instead",
{ after = "4.0", })
return { password = value }
end
}},
{ ssl_server_name = {
type = "string",
func = function(value)
deprecation("acme: config.storage_config.redis.ssl_server_name is deprecated, please use config.storage_config.redis.server_name instead",
{ after = "4.0", })
return { server_name = value }
end
}},
{ namespace = {
type = "string",
func = function(value)
deprecation("acme: config.storage_config.redis.namespace is deprecated, please use config.storage_config.redis.extra_options.namespace instead",
{ after = "4.0", })
return { extra_options = { namespace = value } }
end
}},
{ scan_count = {
type = "integer",
func = function(value)
deprecation("acme: config.storage_config.redis.scan_count is deprecated, please use config.storage_config.redis.extra_options.scan_count instead",
{ after = "4.0", })
return { extra_options = { scan_count = value } }
end
}},
}

local REDIS_STORAGE_SCHEMA = tablex.copy(redis_schema.config_schema.fields)
for _,v in ipairs(REDIS_LEGACY_SCHEMA_FIELDS) do
table.insert(REDIS_STORAGE_SCHEMA, v)
end

table.insert(REDIS_STORAGE_SCHEMA, { extra_options = {
description = "Custom ACME Redis options",
type = "record",
Expand Down Expand Up @@ -217,7 +231,7 @@ local schema = {
fields = {
{ shm = { type = "record", fields = SHM_STORAGE_SCHEMA, } },
{ kong = { type = "record", fields = KONG_STORAGE_SCHEMA, } },
{ redis = { type = "record", fields = REDIS_STORAGE_SCHEMA, } },
{ redis = { type = "record", fields = REDIS_STORAGE_SCHEMA, shorthand_fields = LEGACY_SCHEMA_TRANSLATIONS } },
{ consul = { type = "record", fields = CONSUL_STORAGE_SCHEMA, } },
{ vault = { type = "record", fields = VAULT_STORAGE_SCHEMA, } },
},
Expand Down Expand Up @@ -271,28 +285,6 @@ local schema = {
end
}
},
{ custom_entity_check = {
field_sources = { "config.storage_config.redis.namespace", "config.storage_config.redis.scan_count", "config.storage_config.redis.auth", "config.storage_config.redis.ssl_server_name" },
fn = function(entity)
if (entity.config.storage_config.redis.namespace or ngx.null) ~= ngx.null and entity.config.storage_config.redis.namespace ~= "" then
deprecation("acme: config.storage_config.redis.namespace is deprecated, please use config.storage_config.redis.extra_options.namespace instead",
{ after = "4.0", })
end
if (entity.config.storage_config.redis.scan_count or ngx.null) ~= ngx.null and entity.config.storage_config.redis.scan_count ~= 10 then
deprecation("acme: config.storage_config.redis.scan_count is deprecated, please use config.storage_config.redis.extra_options.scan_count instead",
{ after = "4.0", })
end
if (entity.config.storage_config.redis.auth or ngx.null) ~= ngx.null then
deprecation("acme: config.storage_config.redis.auth is deprecated, please use config.storage_config.redis.password instead",
{ after = "4.0", })
end
if (entity.config.storage_config.redis.ssl_server_name or ngx.null) ~= ngx.null then
deprecation("acme: config.storage_config.redis.ssl_server_name is deprecated, please use config.storage_config.redis.server_name instead",
{ after = "4.0", })
end
return true
end
} }
},
}

Expand Down
8 changes: 4 additions & 4 deletions kong/plugins/acme/storage/config_adapters/redis.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ local function redis_config_adapter(conf)
host = conf.host,
port = conf.port,
database = conf.database,
auth = conf.password or conf.auth, -- allow conf.auth until 4.0 version
auth = conf.password,
ssl = conf.ssl,
ssl_verify = conf.ssl_verify,
ssl_server_name = conf.server_name or conf.ssl_server_name, -- allow conf.ssl_server_name until 4.0 version
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

namespace = conf.extra_options.namespace,
scan_count = conf.extra_options.scan_count,
Comment on lines +9 to +12
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
end

Expand Down
18 changes: 9 additions & 9 deletions kong/plugins/rate-limiting/policies/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,15 @@ local EXPIRATION = require "kong.plugins.rate-limiting.expiration"

local function get_redis_configuration(plugin_conf)
return {
host = plugin_conf.redis.host or plugin_conf.redis_host,
port = plugin_conf.redis.port or plugin_conf.redis_port,
username = plugin_conf.redis.username or plugin_conf.redis_username,
password = plugin_conf.redis.password or plugin_conf.redis_password,
database = plugin_conf.redis.database or plugin_conf.redis_database,
timeout = plugin_conf.redis.timeout or plugin_conf.redis_timeout,
ssl = plugin_conf.redis.ssl or plugin_conf.redis_ssl,
ssl_verify = plugin_conf.redis.ssl_verify or plugin_conf.redis_ssl_verify,
server_name = plugin_conf.redis.server_name or plugin_conf.redis_server_name,
host = plugin_conf.redis.host,
port = plugin_conf.redis.port,
username = plugin_conf.redis.username,
password = plugin_conf.redis.password,
database = plugin_conf.redis.database,
timeout = plugin_conf.redis.timeout,
ssl = plugin_conf.redis.ssl,
ssl_verify = plugin_conf.redis.ssl_verify,
server_name = plugin_conf.redis.server_name,
}
end

Expand Down
153 changes: 81 additions & 72 deletions kong/plugins/rate-limiting/schema.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,40 +93,103 @@ return {
{ policy = policy },
{ fault_tolerant = { description = "A boolean value that determines if the requests should be proxied even if Kong has troubles connecting a third-party data store. If `true`, requests will be proxied anyway, effectively disabling the rate-limiting function until the data store is working again. If `false`, then the clients will see `500` errors.", type = "boolean", required = true, default = true }, },
{ redis = redis_schema.config_schema },
{ redis_host = typedefs.host },
{ redis_port = typedefs.port({ default = 6379 }), },
{ redis_password = { description = "When using the `redis` policy, this property specifies the password to connect to the Redis server.", type = "string", len_min = 0, referenceable = true }, },
{ redis_username = { description = "When using the `redis` policy, this property specifies the username to connect to the Redis server when ACL authentication is desired.", type = "string", referenceable = true }, },
{ redis_ssl = { description = "When using the `redis` policy, this property specifies if SSL is used to connect to the Redis server.", type = "boolean", required = true, default = false, }, },
{ redis_ssl_verify = { description = "When using the `redis` policy with `redis_ssl` set to `true`, this property specifies it server SSL certificate is validated. Note that you need to configure the lua_ssl_trusted_certificate to specify the CA (or server) certificate used by your Redis server. You may also need to configure lua_ssl_verify_depth accordingly.", type = "boolean", required = true, default = false }, },
{ redis_server_name = typedefs.sni },
{ redis_timeout = { description = "When using the `redis` policy, this property specifies the timeout in milliseconds of any command submitted to the Redis server.", type = "number", default = 2000, }, },
{ redis_database = { description = "When using the `redis` policy, this property specifies the Redis database to use.", type = "integer", default = 0 }, },
{ hide_client_headers = { description = "Optionally hide informative response headers.", type = "boolean", required = true, default = false }, },
{ error_code = { description = "Set a custom error code to return when the rate limit is exceeded.", type = "number", default = 429, gt = 0 }, },
{ error_message = { description = "Set a custom error message to return when the rate limit is exceeded.", type = "string", default = "API rate limit exceeded" }, },
{ sync_rate = { description = "How often to sync counter data to the central data store. A value of -1 results in synchronous behavior.", type = "number", required = true, default = -1 }, },
},
custom_validator = validate_periods_order,
shorthand_fields = {
-- TODO: deprecated forms, to be removed in Kong 4.0
{ redis_host = {
type = "string",
func = function(value)
deprecation("rate-limiting: config.redis_host is deprecated, please use config.redis.host instead",
{ after = "4.0", })
return { redis = { host = value } }
end
} },
{ redis_port = {
type = "integer",
func = function(value)
deprecation("rate-limiting: config.redis_port is deprecated, please use config.redis.port instead",
{ after = "4.0", })
return { redis = { port = value } }
end
} },
{ redis_password = {
type = "string",
func = function(value)
deprecation("rate-limiting: config.redis_password is deprecated, please use config.redis.password instead",
{ after = "4.0", })
return { redis = { password = value } }
end
} },
{ redis_username = {
type = "string",
func = function(value)
deprecation("rate-limiting: config.redis_username is deprecated, please use config.redis.username instead",
{ after = "4.0", })
return { redis = { username = value } }
end
} },
{ redis_ssl = {
type = "boolean",
func = function(value)
deprecation("rate-limiting: config.redis_ssl is deprecated, please use config.redis.ssl instead",
{ after = "4.0", })
return { redis = { ssl = value } }
end
} },
{ redis_ssl_verify = {
type = "boolean",
func = function(value)
deprecation("rate-limiting: config.redis_ssl_verify is deprecated, please use config.redis.ssl_verify instead",
{ after = "4.0", })
return { redis = { ssl_verify = value } }
end
} },
{ redis_server_name = {
type = "string",
func = function(value)
deprecation("rate-limiting: config.redis_server_name is deprecated, please use config.redis.server_name instead",
{ after = "4.0", })
return { redis = { server_name = value } }
end
} },
{ redis_timeout = {
type = "integer",
func = function(value)
deprecation("rate-limiting: config.redis_timeout is deprecated, please use config.redis.timeout instead",
{ after = "4.0", })
return { redis = { timeout = value } }
end
} },
{ redis_database = {
type = "integer",
func = function(value)
deprecation("rate-limiting: config.redis_database is deprecated, please use config.redis.database instead",
{ after = "4.0", })
return { redis = { database = value } }
end
} },
},
},
},
},
entity_checks = {
{ at_least_one_of = { "config.second", "config.minute", "config.hour", "config.day", "config.month", "config.year" } },
{ conditional_at_least_one_of = {
{ conditional = {
if_field = "config.policy", if_match = { eq = "redis" },
then_at_least_one_of = { "config.redis.host", "config.redis_host" },
then_err = "must set one of %s when 'policy' is 'redis'",
then_field = "config.redis.host", then_match = { required = true },
} },
{ conditional_at_least_one_of = {
{ conditional = {
if_field = "config.policy", if_match = { eq = "redis" },
then_at_least_one_of = { "config.redis.port", "config.redis_port" },
then_err = "must set one of %s when 'policy' is 'redis'",
then_field = "config.redis.port", then_match = { required = true },
} },
{ conditional_at_least_one_of = {
{ conditional = {
if_field = "config.policy", if_match = { eq = "redis" },
then_at_least_one_of = { "config.redis.timeout", "config.redis_timeout" },
then_err = "must set one of %s when 'policy' is 'redis'",
then_field = "config.redis.timeout", then_match = { required = true },
} },
{ conditional = {
if_field = "config.limit_by", if_match = { eq = "header" },
Expand All @@ -136,59 +199,5 @@ return {
if_field = "config.limit_by", if_match = { eq = "path" },
then_field = "config.path", then_match = { required = true },
} },
{ custom_entity_check = {
field_sources = {
"config.redis_host",
"config.redis_port",
"config.redis_password",
"config.redis_username",
"config.redis_ssl",
"config.redis_ssl_verify",
"config.redis_server_name",
"config.redis_timeout",
"config.redis_database"
},
fn = function(entity)

if (entity.config.redis_host or ngx.null) ~= ngx.null then
deprecation("rate-limiting: config.redis_host is deprecated, please use config.redis.host instead",
{ after = "4.0", })
end
if (entity.config.redis_port or ngx.null) ~= ngx.null and entity.config.redis_port ~= 6379 then
deprecation("rate-limiting: config.redis_port is deprecated, please use config.redis.port instead",
{ after = "4.0", })
end
if (entity.config.redis_password or ngx.null) ~= ngx.null then
deprecation("rate-limiting: config.redis_password is deprecated, please use config.redis.password instead",
{ after = "4.0", })
end
if (entity.config.redis_username or ngx.null) ~= ngx.null then
deprecation("rate-limiting: config.redis_username is deprecated, please use config.redis.username instead",
{ after = "4.0", })
end
if (entity.config.redis_ssl or ngx.null) ~= ngx.null and entity.config.redis_ssl ~= false then
deprecation("rate-limiting: config.redis_ssl is deprecated, please use config.redis.ssl instead",
{ after = "4.0", })
end
if (entity.config.redis_ssl_verify or ngx.null) ~= ngx.null and entity.config.redis_ssl_verify ~= false then
deprecation("rate-limiting: config.redis_ssl_verify is deprecated, please use config.redis.ssl_verify instead",
{ after = "4.0", })
end
if (entity.config.redis_server_name or ngx.null) ~= ngx.null then
deprecation("rate-limiting: config.redis_server_name is deprecated, please use config.redis.server_name instead",
{ after = "4.0", })
end
if (entity.config.redis_timeout or ngx.null) ~= ngx.null and entity.config.redis_timeout ~= 2000 then
deprecation("rate-limiting: config.redis_timeout is deprecated, please use config.redis.timeout instead",
{ after = "4.0", })
end
if (entity.config.redis_database or ngx.null) ~= ngx.null and entity.config.redis_database ~= 0 then
deprecation("rate-limiting: config.redis_database is deprecated, please use config.redis.database instead",
{ after = "4.0", })
end

return true
end
} }
},
}
18 changes: 9 additions & 9 deletions kong/plugins/response-ratelimiting/policies/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@ end

local function get_redis_configuration(plugin_conf)
return {
host = plugin_conf.redis.host or plugin_conf.redis_host,
port = plugin_conf.redis.port or plugin_conf.redis_port,
username = plugin_conf.redis.username or plugin_conf.redis_username,
password = plugin_conf.redis.password or plugin_conf.redis_password,
database = plugin_conf.redis.database or plugin_conf.redis_database,
timeout = plugin_conf.redis.timeout or plugin_conf.redis_timeout,
ssl = plugin_conf.redis.ssl or plugin_conf.redis_ssl,
ssl_verify = plugin_conf.redis.ssl_verify or plugin_conf.redis_ssl_verify,
server_name = plugin_conf.redis.server_name or plugin_conf.redis_server_name,
host = plugin_conf.redis.host,
port = plugin_conf.redis.port,
username = plugin_conf.redis.username,
password = plugin_conf.redis.password,
database = plugin_conf.redis.database,
timeout = plugin_conf.redis.timeout,
ssl = plugin_conf.redis.ssl,
ssl_verify = plugin_conf.redis.ssl_verify,
server_name = plugin_conf.redis.server_name,
}
end

Expand Down
Loading
Loading