From 20e3b55f894059bcc9695f4ac09b77f59de2bed0 Mon Sep 17 00:00:00 2001 From: An Tran Date: Thu, 20 Feb 2025 11:38:25 +1000 Subject: [PATCH] [threescale_utils] Do not terminate request but return error instead Previously, if redis failed to connect for any reason, the next _M.error() call would call ngx.say and ngx.exit to terminate the current request and send an error back to the client, thereby revealing details about the Redis server. We want to return an error and let the caller decide how to handle the error. --- CHANGELOG.md | 1 + gateway/src/apicast/threescale_utils.lua | 11 +++--- spec/policy/rate_limit/rate_limit_spec.lua | 4 +- spec/policy/rate_limit/redis_shdict_spec.lua | 18 +++++++++ t/apicast-policy-rate-limit.t | 39 +++++++++++++++++++- 5 files changed, 65 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e2a46dd7..1cd71b702 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Remove "$id" from the policy schema [PR #1525](https://github.com/3scale/APIcast/pull/1525) [THEESCALE-11610](https://issues.redhat.com/browse/THREESCALE-11610) - Fixed Financial-grade API (FAPI) policy not showing up in the admin portal [PR #1528](https://github.com/3scale/APIcast/pull/1528) [THREESCALE-11620](https://issues.redhat.com/browse/THREESCALE-11620) - Remove Conditional Policy from the UI [PR #1534](https://github.com/3scale/APIcast/pull/1534) [THREESCALE-6116](https://issues.redhat.com/browse/THREESCALE-6116) +- Remove redis connection error message from response body in edge limiting policy [PR #1537](https://github.com/3scale/APIcast/pull/1537) [THREESCALE-11701](https://issues.redhat.com/browse/THREESCALE-11701) ### Added diff --git a/gateway/src/apicast/threescale_utils.lua b/gateway/src/apicast/threescale_utils.lua index d634178f0..c3fd2b1fe 100644 --- a/gateway/src/apicast/threescale_utils.lua +++ b/gateway/src/apicast/threescale_utils.lua @@ -1,5 +1,6 @@ local sub = string.sub local tonumber = tonumber +local fmt = string.format local redis = require 'resty.redis' local env = require 'resty.env' @@ -152,22 +153,22 @@ function _M.connect_redis(options) local ok, err = red:connect(_M.resolve(host, port)) if not ok then - return nil, _M.error("failed to connect to redis on ", host, ":", port, ": ", err) + return nil, fmt("failed to connect to redis on %s:%d, err: %s",host, port, err) end if opts.password then - ok = red:auth(opts.password) + ok, err = red:auth(opts.password) if not ok then - return nil, _M.error("failed to auth on redis ", host, ":", port) + return nil, fmt("failed to auth on redis %s:%d, err: %s", host, port, err) end end if opts.db then - ok = red:select(opts.db) + ok, err = red:select(opts.db) if not ok then - return nil, _M.error("failed to select db ", opts.db, " on redis ", host, ":", port) + return nil, fmt("failed to select db %s on redis %s:%d, err: %s", opts.db, host, port, err) end end diff --git a/spec/policy/rate_limit/rate_limit_spec.lua b/spec/policy/rate_limit/rate_limit_spec.lua index f4dce6594..56e443293 100644 --- a/spec/policy/rate_limit/rate_limit_spec.lua +++ b/spec/policy/rate_limit/rate_limit_spec.lua @@ -35,6 +35,7 @@ describe('Rate limit policy', function() redis:flushdb() stub(ngx, 'exit') + stub(ngx, 'say') stub(ngx, 'sleep') stub(ngx, 'time', function() return 11111 end) @@ -138,8 +139,9 @@ describe('Rate limit policy', function() redis_url = 'redis://invalidhost.domain:'..redis_port..'/1' }) - assert.returns_error('failed to connect to redis on invalidhost.domain:6379: invalidhost.domain could not be resolved (3: Host not found)', rate_limit_policy:access(context)) + rate_limit_policy:access(context) + assert.spy(ngx.say).was_not_called_with('failed to connect to redis on invalidhost.domain:6379: invalidhost.domain could not be resolved (3: Host not found)') assert.spy(ngx.exit).was_called_with(500) end) diff --git a/spec/policy/rate_limit/redis_shdict_spec.lua b/spec/policy/rate_limit/redis_shdict_spec.lua index 8df6505cb..f4886bec7 100644 --- a/spec/policy/rate_limit/redis_shdict_spec.lua +++ b/spec/policy/rate_limit/redis_shdict_spec.lua @@ -10,6 +10,23 @@ describe('Redis Shared Dictionary', function() local redis local shdict + describe('new', function() + it('invalid redis url', function() + local _, err = redis_shdict.new{ host = "invalid", port = 6379 } + assert.match("failed to connect to redis on invalid:6379", err) + end) + + it('invalid redis auth', function() + local _, err = redis_shdict.new{ host = redis_host, port = redis_port, db=1 , password = "invalid"} + assert.match("failed to auth on redis ".. redis_host .. ":" .. redis_port ..", err: ERR AUTH called without any password configured for the default user. Are you sure your configuration is correct?", err) + end) + + it('invalid redis db', function() + local _, err = redis_shdict.new{ host = redis_host, port = redis_port, db = 1000} + assert.match("failed to select db 1000 on redis " .. redis_host ..":" .. redis_port .. ", err: ERR DB index is out of range", err) + end) + end) + before_each(function() local options = { host = redis_host, port = redis_port, db = 1 } redis = assert(ts.connect_redis(options)) @@ -18,6 +35,7 @@ describe('Redis Shared Dictionary', function() assert(redis:flushdb()) end) + describe('flush_all', function() it('removes all records', function() assert(redis:set('foo', 'bar')) diff --git a/t/apicast-policy-rate-limit.t b/t/apicast-policy-rate-limit.t index 26f5ed649..8207837cc 100644 --- a/t/apicast-policy-rate-limit.t +++ b/t/apicast-policy-rate-limit.t @@ -213,8 +213,6 @@ Return 500 code. --- request GET / --- error_code: 500 ---- no_error_log -[error] --- error_log query for invalidhost finished with no answers @@ -1565,3 +1563,40 @@ location /transactions/authrep.xml { [error] --- error_log Requests over the limit. + + + +=== TEST 23: Invalid redis url and configuration_error set to log. +Return 200 code. +--- configuration +{ + "services" : [ + { + "id" : 42, + "proxy" : { + "policy_chain" : [ + { + "name" : "apicast.policy.rate_limit", + "configuration" : { + "connection_limiters" : [ + { + "key" : {"name" : "test3", "scope" : "global"}, + "conn" : 20, + "burst" : 10, + "delay" : 0.5 + } + ], + "redis_url" : "redis://invalidhost:$TEST_NGINX_REDIS_PORT/1", + "configuration_error": {"error_handling": "log"} + } + } + ] + } + } + ] +} +--- request +GET / +--- error_code: 200 +--- error_log +query for invalidhost finished with no answers