From c81d2c9554437fd7a70b9e0c3b349d3f811353fc Mon Sep 17 00:00:00 2001
From: Aapo Talvensaari <aapo.talvensaari@gmail.com>
Date: Fri, 18 Jan 2019 13:29:42 +0200
Subject: [PATCH] chore(db) connector:close and connector:setkeepalive cleanup

Make both Postgres and Cassandra database connectors behave
similarly on `connector:close` and `connetor:setkeepalive`.

Also adds tests for the mentioned methods + :connect,
including tests with ssl connection.
---
 kong/db/strategies/cassandra/connector.lua |   4 +-
 kong/db/strategies/postgres/connector.lua  |  54 +-
 spec/02-integration/03-db/01-db_spec.lua   | 580 ++++++++++++++++++++-
 3 files changed, 590 insertions(+), 48 deletions(-)

diff --git a/kong/db/strategies/cassandra/connector.lua b/kong/db/strategies/cassandra/connector.lua
index 520837de1fa9..bf706cb7df7b 100644
--- a/kong/db/strategies/cassandra/connector.lua
+++ b/kong/db/strategies/cassandra/connector.lua
@@ -223,11 +223,11 @@ function CassandraConnector:setkeepalive()
     return true
   end
 
-  local ok, err = conn:setkeepalive()
+  local _, err = conn:setkeepalive()
 
   self:store_connection(nil)
 
-  if not ok then
+  if err then
     return nil, err
   end
 
diff --git a/kong/db/strategies/postgres/connector.lua b/kong/db/strategies/postgres/connector.lua
index abdfc9a48006..debe5ad2862a 100644
--- a/kong/db/strategies/postgres/connector.lua
+++ b/kong/db/strategies/postgres/connector.lua
@@ -171,56 +171,20 @@ local function connect(config)
 end
 
 
-local function close(connection)
-  if not connection or not connection.sock then
-    return nil, "no active connection"
-  end
-
-  local ok, err = connection:disconnect()
-  if not ok then
-    if err then
-      log(WARN, "unable to close postgres connection (", err, ")")
-
-    else
-      log(WARN, "unable to close postgres connection")
-    end
-
-    return nil, err
-  end
-
-  return true
-end
-
-
 setkeepalive = function(connection)
   if not connection or not connection.sock then
-    return nil, "no active connection"
+    return true
   end
 
-  local ok, err
   if connection.sock_type == "luasocket" then
-    ok, err = connection:disconnect()
-    if not ok then
-      if err then
-        log(WARN, "unable to close postgres connection (", err, ")")
-
-      else
-        log(WARN, "unable to close postgres connection")
-      end
-
+    local _, err = connection:disconnect()
+    if err then
       return nil, err
     end
 
   else
-    ok, err = connection:keepalive()
-    if not ok then
-      if err then
-        log(WARN, "unable to set keepalive for postgres connection (", err, ")")
-
-      else
-        log(WARN, "unable to set keepalive for postgres connection")
-      end
-
+    local _, err = connection:keepalive()
+    if err then
       return nil, err
     end
   end
@@ -373,11 +337,11 @@ function _mt:close()
     return true
   end
 
-  local ok, err = close(conn)
+  local _, err = conn:disconnect()
 
   self:store_connection(nil)
 
-  if not ok then
+  if err then
     return nil, err
   end
 
@@ -391,11 +355,11 @@ function _mt:setkeepalive()
     return true
   end
 
-  local ok, err = setkeepalive(conn)
+  local _, err = setkeepalive(conn)
 
   self:store_connection(nil)
 
-  if not ok then
+  if err then
     return nil, err
   end
 
diff --git a/spec/02-integration/03-db/01-db_spec.lua b/spec/02-integration/03-db/01-db_spec.lua
index b0d09cbeb8ad..509ad751d6ab 100644
--- a/spec/02-integration/03-db/01-db_spec.lua
+++ b/spec/02-integration/03-db/01-db_spec.lua
@@ -1,9 +1,11 @@
 local DB      = require "kong.db"
 local helpers = require "spec.helpers"
-
+local utils   = require "kong.tools.utils"
 
 
 for _, strategy in helpers.each_strategy() do
+  local it_ssl = strategy == "cassandra" and pending or it
+
   describe("kong.db.init [#" .. strategy .. "]", function()
     describe(".new()", function()
       it("errors on invalid arg", function()
@@ -94,15 +96,591 @@ for _, strategy in helpers.each_strategy() do
 
 
   describe(":connect() [#" .. strategy .. "]", function()
+    lazy_setup(function()
+      helpers.get_db_utils(strategy, {})
+    end)
+
+    it("returns connection", function()
+      ngx.IS_CLI = false
+
+      local db, err = DB.new(helpers.test_conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("nginx", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(false, db.connector:get_stored_connection().ssl)
+
+      db:close()
+    end)
+
+    it("returns connection using luasocket", function()
+      ngx.IS_CLI = true
+
+      local db, err = DB.new(helpers.test_conf, strategy)
 
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("luasocket", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(false, db.connector:get_stored_connection().ssl)
+
+      db:close()
+    end)
+
+    it_ssl("returns connection with ssl", function()
+      ngx.IS_CLI = false
+
+      local conf = utils.deep_copy(helpers.test_conf)
+
+      conf.pg_ssl = true
+      conf.cassandra_ssl = true
+
+      local db, err = DB.new(conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("nginx", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(true, db.connector:get_stored_connection().ssl)
+
+      db:close()
+    end)
+
+    it_ssl("returns connection using luasocket with ssl", function()
+      ngx.IS_CLI = true
+
+      local conf = utils.deep_copy(helpers.test_conf)
+
+      conf.pg_ssl = true
+      conf.cassandra_ssl = true
+
+      local db, err = DB.new(conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("luasocket", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(true, db.connector:get_stored_connection().ssl)
+
+      db:close()
+    end)
   end)
 
 
   describe(":setkeepalive() [#" .. strategy .. "]", function()
+    lazy_setup(function()
+      helpers.get_db_utils(strategy, {})
+    end)
+
+    it("return true when there is a stored connection", function()
+      ngx.IS_CLI = false
+
+      local db, err = DB.new(helpers.test_conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("nginx", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(false, db.connector:get_stored_connection().ssl)
+      assert.equal(true, db:setkeepalive())
+
+      db:close()
+    end)
+
+    it("return true when there is a stored connection using luasocket", function()
+      ngx.IS_CLI = true
+
+      local db, err = DB.new(helpers.test_conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("luasocket", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(false, db.connector:get_stored_connection().ssl)
+      assert.equal(true, db:setkeepalive())
+
+      db:close()
+    end)
+
+    it_ssl("return true when there is a stored connection with ssl", function()
+      ngx.IS_CLI = false
+
+      local conf = utils.deep_copy(helpers.test_conf)
+
+      conf.pg_ssl = true
+      conf.cassandra_ssl = true
+
+      local db, err = DB.new(conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("nginx", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(true, db.connector:get_stored_connection().ssl)
+      assert.equal(true, db:setkeepalive())
+
+      db:close()
+    end)
+
+    it_ssl("return true when there is a stored connection using luasocket with ssl", function()
+      ngx.IS_CLI = true
+
+      local conf = utils.deep_copy(helpers.test_conf)
+
+      conf.pg_ssl = true
+      conf.cassandra_ssl = true
+
+      local db, err = DB.new(conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("luasocket", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(true, db.connector:get_stored_connection().ssl)
+      assert.equal(true, db:setkeepalive())
+
+      db:close()
+    end)
+
+
+    it("return true when there is no stored connection", function()
+      ngx.IS_CLI = false
+
+      local db, err = DB.new(helpers.test_conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("nginx", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(false, db.connector:get_stored_connection().ssl)
+
+      db:close()
+
+      assert.is_nil(db.connector:get_stored_connection())
+      assert.equal(true, db:setkeepalive())
+    end)
+
+    it("return true when there is no stored connection using luasocket", function()
+      ngx.IS_CLI = true
+
+      local db, err = DB.new(helpers.test_conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("luasocket", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(false, db.connector:get_stored_connection().ssl)
+
+      db:close()
+
+      assert.is_nil(db.connector:get_stored_connection())
+      assert.equal(true, db:setkeepalive())
+    end)
 
+
+    it_ssl("return true when there is no stored connection with ssl", function()
+      ngx.IS_CLI = false
+
+      local conf = utils.deep_copy(helpers.test_conf)
+
+      conf.pg_ssl = true
+      conf.cassandra_ssl = true
+
+      local db, err = DB.new(conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("nginx", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(true, db.connector:get_stored_connection().ssl)
+
+      db:close()
+
+      assert.is_nil(db.connector:get_stored_connection())
+      assert.equal(true, db:setkeepalive())
+    end)
+
+    it_ssl("return true when there is no stored connection using luasocket with ssl", function()
+      ngx.IS_CLI = true
+
+      local conf = utils.deep_copy(helpers.test_conf)
+
+      conf.pg_ssl = true
+      conf.cassandra_ssl = true
+
+      local db, err = DB.new(conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("luasocket", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(true, db.connector:get_stored_connection().ssl)
+
+      db:close()
+
+      assert.is_nil(db.connector:get_stored_connection())
+      assert.equal(true, db:setkeepalive())
+    end)
   end)
 
   describe(":close() [#" .. strategy .. "]", function()
+    lazy_setup(function()
+      helpers.get_db_utils(strategy, {})
+    end)
+
+    it("return true when there is a stored connection", function()
+      ngx.IS_CLI = false
+
+      local db, err = DB.new(helpers.test_conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("nginx", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(false, db.connector:get_stored_connection().ssl)
+      assert.equal(true, db:close())
+    end)
+
+    it("return true when there is a stored connection using luasocket", function()
+      ngx.IS_CLI = true
+
+      local db, err = DB.new(helpers.test_conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("luasocket", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(false, db.connector:get_stored_connection().ssl)
+      assert.equal(true, db:close())
+    end)
+
+    it_ssl("return true when there is a stored connection with ssl", function()
+      ngx.IS_CLI = false
+
+      local conf = utils.deep_copy(helpers.test_conf)
+
+      conf.pg_ssl = true
+      conf.cassandra_ssl = true
+
+      local db, err = DB.new(conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("nginx", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(true, db.connector:get_stored_connection().ssl)
+      assert.equal(true, db:close())
+    end)
+
+    it_ssl("return true when there is a stored connection using luasocket with ssl", function()
+      ngx.IS_CLI = true
 
+      local conf = utils.deep_copy(helpers.test_conf)
+
+      conf.pg_ssl = true
+      conf.cassandra_ssl = true
+
+      local db, err = DB.new(conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("luasocket", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(true, db.connector:get_stored_connection().ssl)
+      assert.equal(true, db:close())
+    end)
+
+    it("return true when there is no stored connection", function()
+      ngx.IS_CLI = false
+
+      local db, err = DB.new(helpers.test_conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("nginx", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(false, db.connector:get_stored_connection().ssl)
+
+      db:close()
+
+      assert.is_nil(db.connector:get_stored_connection())
+      assert.equal(true, db:close())
+    end)
+
+    it("return true when there is no stored connection using luasocket", function()
+      ngx.IS_CLI = true
+
+      local db, err = DB.new(helpers.test_conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("luasocket", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(false, db.connector:get_stored_connection().ssl)
+
+      db:close()
+
+      assert.is_nil(db.connector:get_stored_connection())
+      assert.equal(true, db:close())
+    end)
+
+    it_ssl("return true when there is no stored connection with ssl", function()
+      ngx.IS_CLI = false
+
+      local conf = utils.deep_copy(helpers.test_conf)
+
+      conf.pg_ssl = true
+      conf.cassandra_ssl = true
+
+      local db, err = DB.new(conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("nginx", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(true, db.connector:get_stored_connection().ssl)
+
+      db:close()
+
+      assert.is_nil(db.connector:get_stored_connection())
+      assert.equal(true, db:close())
+    end)
+
+    it_ssl("return true when there is no stored connection using luasocket with ssl", function()
+      ngx.IS_CLI = true
+
+      local conf = utils.deep_copy(helpers.test_conf)
+
+      conf.pg_ssl = true
+      conf.cassandra_ssl = true
+
+      local db, err = DB.new(conf, strategy)
+
+      assert.is_nil(err)
+      assert.is_table(db)
+
+      assert(db:init_connector())
+
+      local conn, err = db:connect()
+      assert.is_nil(err)
+      assert.is_table(conn)
+
+      if strategy == "postgres" then
+        assert.equal("luasocket", db.connector:get_stored_connection().sock_type)
+      --elseif strategy == "cassandra" then
+      --TODO: cassandra forces luasocket on timer
+      end
+
+      assert.equal(true, db.connector:get_stored_connection().ssl)
+
+      db:close()
+
+      assert.is_nil(db.connector:get_stored_connection())
+      assert.equal(true, db:close())
+    end)
   end)
 end