From 1c394673a574852c6c591d20f6f3bb9d1785702e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Nowak?= Date: Thu, 23 May 2024 15:44:35 +0200 Subject: [PATCH] fix(logs): misleading acme deprecation logs in hybrid mode When running kong in hybrid mode a plugin configuration is pushed from CP to DP. When introducing shorthand field expansion to Admin responses the deprecated fields (defined as shorthand fields) were also pushed from CP to DP which resulted in DP receiving config with both new fields and also old fields. It resulted with DP reporting in logs that the plugin is being configured with deprecated fields. It affected the plugins: - ACME - Rate-Limiting - Response-RateLimiting This commit disables deprecation warning on data planes since those nodes cannot be configured manually so this deprecation message is not actionable on data planes. KAG-4515 --- .../fix-acme-misleading-deprecation-logs.yml | 3 + ...esponse-rl-misleading-deprecation-logs.yml | 3 + .../fix-rl-misleading-deprecation-logs.yml | 3 + kong/db/schema/init.lua | 5 +- .../09-hybrid_mode/13-deprecations_spec.lua | 114 ++++++++++++++++++ .../23-rate-limiting/07-hybrid_mode_spec.lua | 108 +++++++++++++++++ 6 files changed, 234 insertions(+), 2 deletions(-) create mode 100644 changelog/unreleased/kong/fix-acme-misleading-deprecation-logs.yml create mode 100644 changelog/unreleased/kong/fix-response-rl-misleading-deprecation-logs.yml create mode 100644 changelog/unreleased/kong/fix-rl-misleading-deprecation-logs.yml create mode 100644 spec/02-integration/09-hybrid_mode/13-deprecations_spec.lua create mode 100644 spec/03-plugins/23-rate-limiting/07-hybrid_mode_spec.lua diff --git a/changelog/unreleased/kong/fix-acme-misleading-deprecation-logs.yml b/changelog/unreleased/kong/fix-acme-misleading-deprecation-logs.yml new file mode 100644 index 0000000000000..6e7a1accc36dd --- /dev/null +++ b/changelog/unreleased/kong/fix-acme-misleading-deprecation-logs.yml @@ -0,0 +1,3 @@ +message: "**ACME**: Fixed an issue of DP reporting that deprecated config fields are used when configuration from CP is pushed" +type: bugfix +scope: Plugin diff --git a/changelog/unreleased/kong/fix-response-rl-misleading-deprecation-logs.yml b/changelog/unreleased/kong/fix-response-rl-misleading-deprecation-logs.yml new file mode 100644 index 0000000000000..20f0c8b5290ba --- /dev/null +++ b/changelog/unreleased/kong/fix-response-rl-misleading-deprecation-logs.yml @@ -0,0 +1,3 @@ +message: "**Response-RateLimiting**: Fixed an issue of DP reporting that deprecated config fields are used when configuration from CP is pushed" +type: bugfix +scope: Plugin diff --git a/changelog/unreleased/kong/fix-rl-misleading-deprecation-logs.yml b/changelog/unreleased/kong/fix-rl-misleading-deprecation-logs.yml new file mode 100644 index 0000000000000..d0bc463b7beca --- /dev/null +++ b/changelog/unreleased/kong/fix-rl-misleading-deprecation-logs.yml @@ -0,0 +1,3 @@ +message: "**Rate-Limiting**: Fixed an issue of DP reporting that deprecated config fields are used when configuration from CP is pushed" +type: bugfix +scope: Plugin diff --git a/kong/db/schema/init.lua b/kong/db/schema/init.lua index a62a3660de2de..983d8accd974f 100644 --- a/kong/db/schema/init.lua +++ b/kong/db/schema/init.lua @@ -887,8 +887,9 @@ function Schema:validate_field(field, value) if field.deprecation then local old_default = field.deprecation.old_default - local should_warn = old_default == nil - or not deepcompare(value, old_default) + local should_warn = kong.configuration.role ~= "data_plane" and + (old_default == nil + or not deepcompare(value, old_default)) if should_warn then deprecation(field.deprecation.message, { after = field.deprecation.removal_in_version, }) diff --git a/spec/02-integration/09-hybrid_mode/13-deprecations_spec.lua b/spec/02-integration/09-hybrid_mode/13-deprecations_spec.lua new file mode 100644 index 0000000000000..2814d5e282d24 --- /dev/null +++ b/spec/02-integration/09-hybrid_mode/13-deprecations_spec.lua @@ -0,0 +1,114 @@ +local helpers = require("spec.helpers") +local join = require("pl.stringx").join + +local ENABLED_PLUGINS = { "dummy" , "reconfiguration-completion"} + +for _, strategy in helpers.each_strategy({"postgres"}) do + describe("deprecations are not reported on DP but on CP", function() + local cp_prefix = "servroot1" + local dp_prefix = "servroot2" + local cp_logfile, dp_logfile, route + + lazy_setup(function() + local bp = helpers.get_db_utils(strategy, { + "services", + "routes", + "plugins", + }, ENABLED_PLUGINS) + + local service = bp.services:insert { + name = "example", + host = helpers.mock_upstream_host, + port = helpers.mock_upstream_port, + } + + route = assert(bp.routes:insert { + hosts = { "mock_upstream" }, + protocols = { "http" }, + service = service, + }) + + assert(helpers.start_kong({ + role = "control_plane", + database = strategy, + prefix = cp_prefix, + cluster_cert = "spec/fixtures/kong_clustering.crt", + cluster_cert_key = "spec/fixtures/kong_clustering.key", + lua_ssl_trusted_certificate = "spec/fixtures/kong_clustering.crt", + cluster_listen = "127.0.0.1:9005", + cluster_telemetry_listen = "127.0.0.1:9006", + plugins = "bundled," .. join(",", ENABLED_PLUGINS), + nginx_conf = "spec/fixtures/custom_nginx.template", + admin_listen = "0.0.0.0:9001", + proxy_listen = "off", + })) + + assert(helpers.start_kong({ + role = "data_plane", + database = "off", + prefix = dp_prefix, + cluster_cert = "spec/fixtures/kong_clustering.crt", + cluster_cert_key = "spec/fixtures/kong_clustering.key", + lua_ssl_trusted_certificate = "spec/fixtures/kong_clustering.crt", + cluster_control_plane = "127.0.0.1:9005", + cluster_telemetry_endpoint = "127.0.0.1:9006", + plugins = "bundled," .. join(",", ENABLED_PLUGINS), + admin_listen = "off", + proxy_listen = "0.0.0.0:9002", + })) + dp_logfile = helpers.get_running_conf(dp_prefix).nginx_err_logs + cp_logfile = helpers.get_running_conf(cp_prefix).nginx_err_logs + end) + + lazy_teardown(function() + helpers.stop_kong(dp_prefix) + helpers.stop_kong(cp_prefix) + end) + + describe("deprecations are not reported on DP but on CP", function() + before_each(function() + helpers.clean_logfile(dp_logfile) + end) + + it("deprecation warnings are only fired on CP not DP", function() + local proxy_client, admin_client = helpers.make_synchronized_clients({ + proxy_client = helpers.proxy_client(nil, 9002), + admin_client = helpers.admin_client(nil, 9001) + }) + local res = assert(admin_client:send { + method = "POST", + path = "/plugins", + body = { + name = "dummy", + route = { id = route.id }, + config = { + old_field = 10, + append_body = "appended from body filtering" + }, + }, + headers = { + ["Content-Type"] = "application/json", + } + }) + assert.res_status(201, res) + + local res = assert(proxy_client:send { + method = "GET", + path = "/status/200", + headers = { + ["Host"] = "mock_upstream", + } + }) + local body = assert.res_status(200, res) + + -- TEST: ensure that the dummy plugin was executed by checking + -- that the body filtering phase has run + + assert.matches("appended from body filtering", body, nil, true) + + assert.logfile(cp_logfile).has.line("dummy: old_field is deprecated", true, 2) + assert.logfile(dp_logfile).has.no.line("dummy: old_field is deprecated", true, 2) + end) + end) + end) +end diff --git a/spec/03-plugins/23-rate-limiting/07-hybrid_mode_spec.lua b/spec/03-plugins/23-rate-limiting/07-hybrid_mode_spec.lua new file mode 100644 index 0000000000000..4c1bbcc199e6c --- /dev/null +++ b/spec/03-plugins/23-rate-limiting/07-hybrid_mode_spec.lua @@ -0,0 +1,108 @@ +local helpers = require "spec.helpers" + +local REDIS_HOST = helpers.redis_host +local REDIS_PORT = helpers.redis_port + +for _, strategy in helpers.each_strategy({"postgres"}) do + describe("Plugin: rate-limiting (handler.access) worked with [#" .. strategy .. "]", function() + local dp_prefix = "servroot2" + local dp_logfile, bp, db, route + + lazy_setup(function() + bp, db = helpers.get_db_utils(strategy, { + "services", + "routes", + "plugins", + }, { "rate-limiting", }) + + local service = assert(bp.services:insert { + host = helpers.mock_upstream_host, + port = helpers.mock_upstream_port, + }) + + route = assert(bp.routes:insert { + paths = { "/rate-limit-test" }, + service = service + }) + + assert(helpers.start_kong({ + role = "control_plane", + database = strategy, + cluster_cert = "spec/fixtures/kong_clustering.crt", + cluster_cert_key = "spec/fixtures/kong_clustering.key", + lua_ssl_trusted_certificate = "spec/fixtures/kong_clustering.crt", + cluster_listen = "127.0.0.1:9005", + cluster_telemetry_listen = "127.0.0.1:9006", + nginx_conf = "spec/fixtures/custom_nginx.template", + admin_listen = "0.0.0.0:9001", + proxy_listen = "off", + })) + + assert(helpers.start_kong({ + role = "data_plane", + database = "off", + prefix = dp_prefix, + cluster_cert = "spec/fixtures/kong_clustering.crt", + cluster_cert_key = "spec/fixtures/kong_clustering.key", + lua_ssl_trusted_certificate = "spec/fixtures/kong_clustering.crt", + cluster_control_plane = "127.0.0.1:9005", + cluster_telemetry_endpoint = "127.0.0.1:9006", + admin_listen = "off", + proxy_listen = "0.0.0.0:9002", + })) + dp_logfile = helpers.get_running_conf(dp_prefix).nginx_err_logs + end) + + lazy_teardown(function() + helpers.stop_kong("servroot2") + helpers.stop_kong() + end) + + describe("\"redis\" storage mode in Hybrid mode", function() + lazy_setup(function () + local admin_client = helpers.admin_client(nil, 9001) + local res = assert(admin_client:send { + method = "POST", + path = "/plugins", + body = { + name = "rate-limiting", + route = { + id = route.id + }, + config = { + minute = 2, + policy = "redis", + redis = { + host = REDIS_HOST, + port = REDIS_PORT, + } + }, + }, + headers = { + ["Content-Type"] = "application/json", + } + }) + assert.res_status(201, res) + admin_client:close() + end) + + lazy_teardown(function () + db:truncate("plugins") + end) + + before_each(function() + helpers.clean_logfile(dp_logfile) + end) + + it("sanity test - check if old fields are not pushed & visible in logs as deprecation warnings", function() + helpers.wait_until(function() + local proxy_client = helpers.proxy_client(nil, 9002) + local res = assert(proxy_client:get("/rate-limit-test")) + proxy_client:close() + + return res.status == 429 + end, 10, 1) + end) + end) + end) +end