[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

@@ -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{ url = redis_url }
+            assert.equal(err, "failed to connect to redis on 127.0.0.1:6379, err: connection refused")
+        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:6379, err: ERR AUTH", 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:6379, err: ERR DB", 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