From d0fd05a545215759b2f0a058e46aec14dfe68d70 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Fri, 29 Mar 2019 16:02:32 -0300 Subject: [PATCH 1/5] tests(conf) add strict mode for conf loader during tests --- bin/busted | 1 + kong/conf_loader.lua | 48 +++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/bin/busted b/bin/busted index 9b883a58640..27c7dbbd119 100755 --- a/bin/busted +++ b/bin/busted @@ -35,6 +35,7 @@ if not os.getenv("KONG_BUSTED_RESPAWNED") then end -- add cli recursion detection table.insert(script, "export KONG_BUSTED_RESPAWNED=1") + table.insert(script, "export KONG_STRICT_CONF=1") -- rebuild the invoked commandline, while inserting extra resty-flags local resty_flags = DEFAULT_RESTY_FLAGS diff --git a/kong/conf_loader.lua b/kong/conf_loader.lua index fc52a84d240..29c3b48e87e 100644 --- a/kong/conf_loader.lua +++ b/kong/conf_loader.lua @@ -187,6 +187,28 @@ local CONF_INFERENCES = { lua_ssl_verify_depth = { typ = "number" }, lua_socket_pool_size = { typ = "number" }, + + -- known keys which require no type inference: + prefix = {}, + pg_user = {}, + pg_host = {}, + pg_schema = {}, + pg_database = {}, + cassandra_username = {}, + cassandra_keyspace = {}, + declarative_config = {}, + mem_cache_size = {}, + lua_package_path = {}, + lua_package_cpath = {}, + lua_ssl_trusted_certificate = {}, + ssl_cert = {}, + ssl_cert_key = {}, + ssl_ciphers = {}, + ssl_cipher_suite = {}, + admin_ssl_cert = {}, + admin_ssl_cert_key = {}, + client_ssl_cert = {}, + client_ssl_cert_key = {}, } @@ -221,13 +243,33 @@ local _nop_tostring_mt = { } +local function is_dynamic_key(k) + for _, dyn_key_prefix in pairs(DYNAMIC_KEY_PREFIXES) do + if string.match(k, "^(" .. dyn_key_prefix .. ".+)") then + return true + end + end + return false +end + + -- Validate properties (type/enum/custom) and infer their type. -- @param[type=table] conf The configuration table to treat. -local function check_and_infer(conf) +-- @param[type=boolean] strict Report unknown fields as errors. +local function check_and_infer(conf, strict) local errors = {} for k, value in pairs(conf) do - local v_schema = CONF_INFERENCES[k] or {} + local v_schema = CONF_INFERENCES[k] + if not v_schema then + if strict then + if not is_dynamic_key(k) then + errors[#errors + 1] = fmt("unknown key: '%s'", k) + end + end + v_schema = {} + end + local typ = v_schema.typ if type(value) == "string" then @@ -750,7 +792,7 @@ local function load(path, custom_conf) local conf = tablex.pairmap(overrides, defaults, from_file_conf, custom_conf) -- validation - local ok, err, errors = check_and_infer(conf) + local ok, err, errors = check_and_infer(conf, os.getenv("KONG_STRICT_CONF")) if not ok then return nil, err, errors end From 92eb32dc9058249f47dceaff24df592e9802b26a Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Fri, 29 Mar 2019 17:02:27 -0300 Subject: [PATCH 2/5] tests(conf) verify that PRs changing kong/conf_loader.lua also change kong.conf.default Since tests run with the conf loader in strict mode, any addition of directives (or changes to directive enums) requires a change to kong/conf_loader.lua. In this test, we verify that a PR including a change to this file also includes a change to kong.conf.default. When a change to kong.conf_loader does _not_ require a change in kong.conf.default (for example, when performing an internal refactor), this test can be silenced by adding the hashtag #noconfchange to the commit message. This is still a approximation (an even stricter test would require kong.conf.default to be changed in the same commit), but it should be sufficient to require all new configuration entries added via pull requests to be documented. --- spec/01-unit/03-conf_loader_spec.lua | 55 ++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/spec/01-unit/03-conf_loader_spec.lua b/spec/01-unit/03-conf_loader_spec.lua index 69d2db2e5d6..2cd817cde56 100644 --- a/spec/01-unit/03-conf_loader_spec.lua +++ b/spec/01-unit/03-conf_loader_spec.lua @@ -1,4 +1,5 @@ local conf_loader = require "kong.conf_loader" +local pl_utils = require "pl.utils" local helpers = require "spec.helpers" local tablex = require "pl.tablex" @@ -965,4 +966,58 @@ describe("Configuration loader", function() assert.equal("123456", conf.cassandra_password) end) end) + + describe("changes", function() + local pr = os.getenv("TRAVIS_PULL_REQUEST") + local pr_it = pr and it or pending + + pr_it("are reflected in kong.conf.default", function() + local base = os.getenv("TRAVIS_BRANCH") + print("Travis PR " .. pr .. " (" ..os.getenv("TRAVIS_PULL_REQUEST_BRANCH") .. ") targeting " .. base) + + local cmd = "git diff " .. base .. " --numstat | cut -f3" + local ok, _, files, stderr = pl_utils.executeex(cmd) + assert(ok, "failed running: " .. cmd .. "\n" .. (stderr or "")) + files = "\n" .. files .. "\n" + + if not files:match("\nkong/conf_loader.lua\n") then + print("[PASS]: No changes made to conf_loader.") + return + end + + if files:match("\nkong.conf.default\n") then + print("[PASS]: PR includes changes to both kong/conf_loader.lua and kong.conf.default") + return + end + + -- Some PRs modify conf_loader but do not require changes to kong.conf.default. + -- These should be noted with a hashtag in the commit message: #noconfchange + + local log + cmd = "git log " .. base .. "..HEAD" + ok, _, log, stderr = pl_utils.executeex(cmd) + assert(ok, "failed running: " .. cmd .. "\n" .. (stderr or "")) + + if log:match("#noconfchange") then + print("[PASS]: PR includes changes to kong/conf_loader.lua and uses #noconfchange hashtag") + return + end + + print("*************************************************************************") + print("[FAIL]: Your pull request modifies kong.conf_loader (possibly modifying") + print("or adding directives) but does not include an accompanying change to") + print("kong.conf.default. Please document any changes in kong.conf.default") + print("and include in this PR.") + print() + print("If your PR modifies kong.conf_loader but does not require changes to") + print("kong.conf.default (i.e. no user-visible changes to the configuration") + print("format), please add the hashtag #noconfchange to your commit message") + print("to make this test pass.") + print("*************************************************************************") + print() + + assert(false) + end) + end) + end) From 5b414a4af9e42ee1a2cfdcee756c31a3e67f19d9 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Fri, 29 Mar 2019 17:28:08 -0300 Subject: [PATCH 3/5] added an option without adding documentation --- .travis.yml | 2 +- kong/conf_loader.lua | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 25ae01ef672..aba1dd7a424 100644 --- a/.travis.yml +++ b/.travis.yml @@ -67,7 +67,7 @@ jobs: - stage: lint and unit script: - luacheck -q . - - bin/busted -v -o gtest spec/01-unit + - TRAVIS_PULL_REQUEST=$TRAVIS_PULL_REQUEST TRAVIS_BRANCH=$TRAVIS_BRANCH bin/busted -v -o gtest spec/01-unit env: - KONG_DATABASE=none - stage: functional tests diff --git a/kong/conf_loader.lua b/kong/conf_loader.lua index 29c3b48e87e..6173df508c7 100644 --- a/kong/conf_loader.lua +++ b/kong/conf_loader.lua @@ -209,6 +209,7 @@ local CONF_INFERENCES = { admin_ssl_cert_key = {}, client_ssl_cert = {}, client_ssl_cert_key = {}, + my_new_option = {}, } From 9bf499680f3b35d77d0b91056de95089d73da4d0 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Fri, 29 Mar 2019 18:34:31 -0300 Subject: [PATCH 4/5] hack! please fail --- .travis.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index aba1dd7a424..b1296c35c36 100644 --- a/.travis.yml +++ b/.travis.yml @@ -67,9 +67,12 @@ jobs: - stage: lint and unit script: - luacheck -q . - - TRAVIS_PULL_REQUEST=$TRAVIS_PULL_REQUEST TRAVIS_BRANCH=$TRAVIS_BRANCH bin/busted -v -o gtest spec/01-unit + - TRAVIS_PULL_REQUEST=$TRAVIS_PULL_REQUEST TRAVIS_PULL_REQUEST_BRANCH=$TRAVIS_PULL_REQUEST_BRANCH TRAVIS_BRANCH=$TRAVIS_BRANCH bin/busted -v -o gtest spec/01-unit env: - KONG_DATABASE=none + - TRAVIS_PULL_REQUEST=$TRAVIS_PULL_REQUEST + - TRAVIS_BRANCH=$TRAVIS_BRANCH + - TRAVIS_PULL_REQUEST_BRANCH=$TRAVIS_PULL_REQUEST_BRANCH - stage: functional tests script: make functional_tests if: type=cron From 60ae5261d600037d8f1eb2498a4aac3c2de51155 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Fri, 29 Mar 2019 19:07:43 -0300 Subject: [PATCH 5/5] does this work? --- .travis.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index b1296c35c36..683d782fadf 100644 --- a/.travis.yml +++ b/.travis.yml @@ -33,6 +33,9 @@ env: - KONG_TEST_PG_DATABASE=travis - KONG_TEST_PG_USER=postgres - JOBS=2 + - TRAVIS_PULL_REQUEST=$TRAVIS_PULL_REQUEST + - TRAVIS_BRANCH=$TRAVIS_BRANCH + - TRAVIS_PULL_REQUEST_BRANCH=$TRAVIS_PULL_REQUEST_BRANCH matrix: - KONG_TEST_DATABASE=postgres TEST_SUITE=integration - KONG_TEST_DATABASE=cassandra CASSANDRA=2.2 TEST_SUITE=integration @@ -67,12 +70,9 @@ jobs: - stage: lint and unit script: - luacheck -q . - - TRAVIS_PULL_REQUEST=$TRAVIS_PULL_REQUEST TRAVIS_PULL_REQUEST_BRANCH=$TRAVIS_PULL_REQUEST_BRANCH TRAVIS_BRANCH=$TRAVIS_BRANCH bin/busted -v -o gtest spec/01-unit + - bin/busted -v -o gtest spec/01-unit env: - KONG_DATABASE=none - - TRAVIS_PULL_REQUEST=$TRAVIS_PULL_REQUEST - - TRAVIS_BRANCH=$TRAVIS_BRANCH - - TRAVIS_PULL_REQUEST_BRANCH=$TRAVIS_PULL_REQUEST_BRANCH - stage: functional tests script: make functional_tests if: type=cron