[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.

Author: tkan145

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
 

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
 

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)
 

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 <password> 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'))

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