Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cli] decouple apicast environment and 3scale configuration channel #590

Merged
merged 6 commits into from
Feb 14, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions gateway/config/development.lua
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,6 @@ return {
worker_processes = '1',
master_process = 'off',
lua_code_cache = 'off',
configuration_loader = 'lazy',
configuration_cache = 0,
}
2 changes: 2 additions & 0 deletions gateway/config/production.lua
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
return {
master_process = 'on',
lua_code_cache = 'on',
configuration_loader = 'boot',
configuration_cache = os.getenv('APICAST_CONFIGURATION_CACHE') or 5*60,
}
1 change: 1 addition & 0 deletions gateway/config/sandbox.lua
6 changes: 6 additions & 0 deletions gateway/config/staging.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
return {
master_process = 'on',
lua_code_cache = 'on',
configuration_loader = 'lazy',
configuration_cache = 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not os.getenv('APICAST_CONFIGURATION_CACHE') or 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think our staging should always reload configuration. And because of the order of options and context in the start.lua the CLI options take precedence. So setting APICAST_CONFIGURATION_CACHE env would override this anyway.

That makes the production.lua os.getenv a bit redundant, but I wanted it there as an example of what can be done. But if it is confusing I could change/remove it.

}
2 changes: 1 addition & 1 deletion gateway/cpanfile
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
requires 'Test::APIcast', '0.07';
requires 'Test::APIcast', '0.08';
requires 'Crypt::JWT';
2 changes: 1 addition & 1 deletion gateway/http.d/init.conf
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ init_by_lua_block {
-- This ENV variable is defined in the main nginx.conf.liquid and injected when including this partial.
-- The content of the ENV variable is a Lua table, so when rendered it actually can run ipairs on it.
for k,v in pairs({{ ENV }}) do
if type(k) == 'string' and k ~= 'APICAST_CONFIGURATION_LOADER' then
if type(k) == 'string' and not resty_env.value(k) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this change. Can you explain it, please?

Copy link
Contributor Author

@mikz mikz Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sets only missing/empty variables.

This leaves the flexibility of overriding the ENV when starting nginx and treating the {{ ENV }} only as default values.

Because Test::APIcast runs APICAST_CONFIGURATION_LOADER=test bin/apicast and expects it to work. But in cases where APICAST_CONFIGURATION_LOADER=boot is already set in the ENV this would not apply and APICAST_CONFIGURATION_LOADER would have value boot during initialization. Basically ignoring what ENV it was started in.

So this change is for two reasons:

  • I think it makes sense for ENV of the shell to take priority than what is in the config file
  • Test::APIcast needs to start APIcast with APICAST_CONFIGURATION_LOADER=test when testing configuration with --test but then later start it with boot when the config is started for real (with no APICAST_CONFIGURATION_LOADER - but that is already hardcoded in the config as the default).

resty_env.set(k,v)
end
end
Expand Down
69 changes: 50 additions & 19 deletions gateway/src/apicast/cli/command/start.lua
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@ local max = math.max
local insert = table.insert
local concat = table.concat
local format = string.format
local tostring = tostring
local tonumber = tonumber

local exec = require('resty.execvp')
local resty_env = require('resty.env')
local re = require('ngx.re')

local Template = require('apicast.cli.template')
local Environment = require('apicast.cli.environment')
Expand Down Expand Up @@ -41,7 +44,7 @@ end

local function update_env(env)
for name, value in pairs(env) do
resty_env.set(name, value)
resty_env.set(name, tostring(value))
end
end

Expand Down Expand Up @@ -104,13 +107,13 @@ local function openresty_binary(candidates)
find_openresty_command(candidates)
end

local function build_env(options, config)
local function build_env(options, config, context)
return {
APICAST_CONFIGURATION = options.configuration,
APICAST_CONFIGURATION_LOADER = options.boot and 'boot' or 'lazy',
APICAST_CONFIGURATION_CACHE = options.cache,
THREESCALE_DEPLOYMENT_ENV = config.name,
APICAST_POLICY_LOAD_PATH = options.policy_load_path,
APICAST_CONFIGURATION_LOADER = tostring(options.configuration_loader or context.configuration_loader or 'lazy'),
APICAST_CONFIGURATION_CACHE = tostring(options.cache or context.configuration_cache or 0),
THREESCALE_DEPLOYMENT_ENV = context.configuration_channel or options.channel or config.name,
APICAST_POLICY_LOAD_PATH = options.policy_load_path or context.policy_load_path,
}
end

Expand Down Expand Up @@ -138,7 +141,7 @@ function mt:__call(options)
local openresty = openresty_binary(self.openresty)
local config = build_environment_config(options)
local context = build_context(options, config)
local env = build_env(options, config)
local env = build_env(options, config, context)

local template_path = options.template

Expand All @@ -164,15 +167,34 @@ function mt:__call(options)
return exec(openresty, cmd, env)
end

local function split_by(pattern)
return function(str)
return re.split(str or '', pattern, 'oj')
end
end

local load_env = split_by(':')

local function configure(cmd)
cmd:usage("Usage: apicast-cli start [OPTIONS]")
cmd:option("--template", "Nginx config template.", 'conf/nginx.conf.liquid')

local channel = resty_env.value('THREESCALE_DEPLOYMENT_ENV') or 'production'
local loaded_env = Environment.loaded()

insert(loaded_env, 1, channel)

cmd:option('-e --environment', "Deployment to start. Can also be a path to a Lua file.", resty_env.value('THREESCALE_DEPLOYMENT_ENV') or 'production'):count('*')
cmd:flag('--dev', 'Start in development environment')
cmd:option('-3 --channel', "3scale configuration channel to use.", channel):action(function(args, name, chan)
args.environment[1] = chan
args[name] = chan
end):count('0-1')
cmd:option('-e --environment', "Deployment to start. Can also be a path to a Lua file.", resty_env.value('APICAST_ENVIRONMENT'))
:count('*'):init(loaded_env):action('concat'):convert(load_env)
cmd:flag('--development --dev', 'Start in development environment'):action(function(arg, name)
insert(arg.environment, name)
end)

cmd:flag("-m --master", "Test the nginx config"):args('?')
cmd:flag("-m --master", "Control nginx master process.", 'on'):args('?')
cmd:flag("-t --test", "Test the nginx config")
cmd:flag("--debug", "Debug mode. Prints more information.")
cmd:option("-c --configuration",
Expand All @@ -183,17 +205,26 @@ local function configure(cmd)
"Number of worker processes to start.",
resty_env.value('APICAST_WORKERS') or Environment.default_config.worker_processes)
cmd:option("-p --pid", "Path to the PID file.")
cmd:mutex(
cmd:flag('-b --boot',
"Load configuration on boot.",
resty_env.value('APICAST_CONFIGURATION_LOADER') == 'boot'),
cmd:flag('-l --lazy',
"Load configuration on demand.",
resty_env.value('APICAST_CONFIGURATION_LOADER') == 'lazy')
)

do
local target = 'configuration_loader'
local configuration_loader = resty_env.value('APICAST_CONFIGURATION_LOADER')
local function set_configuration_loader(value)
return function(args) args[target] = value end
end

cmd:mutex(
cmd:flag('-b --boot',
"Load configuration on boot.",
configuration_loader == 'boot'):action(set_configuration_loader('boot')):target('configuration_loader'):init(configuration_loader),
cmd:flag('-l --lazy',
"Load configuration on demand.",
configuration_loader == 'lazy'):action(set_configuration_loader('lazy')):target('configuration_loader'):init(configuration_loader)
)
end
cmd:option("-i --refresh-interval",
"Cache configuration for N seconds. Using 0 will reload on every request (not for production).",
resty_env.value('APICAST_CONFIGURATION_CACHE'))
resty_env.value('APICAST_CONFIGURATION_CACHE')):convert(tonumber)

cmd:option("--policy-load-path",
"Load path where to find policies. Entries separated by `:`.",
Expand Down
15 changes: 10 additions & 5 deletions gateway/src/apicast/cli/environment.lua
Original file line number Diff line number Diff line change
Expand Up @@ -75,18 +75,23 @@ _M.default_config = {

local mt = { __index = _M }

--- Return loaded environments defined as environment variable.
-- @treturn {string,...}
function _M.loaded()
local value = resty_env.value('APICAST_LOADED_ENVIRONMENTS')
return re.split(value or '', [[\|]], 'jo')
end

--- Load an environment from files in ENV.
-- @treturn Environment
function _M.load()
local value = resty_env.value('APICAST_LOADED_ENVIRONMENTS')
local env = _M.new()
local environments = _M.loaded()

if not value then
if not environments then
return env
end

local environments = re.split(value, '\\|', 'jo')

for i=1,#environments do
assert(env:add(environments[i]))
end
Expand Down Expand Up @@ -143,7 +148,7 @@ function _M:add(env)

local config = loadfile(path, 't', {
print = print, inspect = require('inspect'), context = self._context,
tonumber = tonumber, tostring = tostring,
tonumber = tonumber, tostring = tostring, os = { getenv = resty_env.value },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that overriding getenv here can be a bit confusing.
Although I get that you're doing this to be able to use it in the environment files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. I think it is more useful to expose resty_env.value than plain resty_env.get or os.getenv.
And the only difference is that it does not return empty strings. Which is actually useful for default values.

pcall = pcall, require = require, assert = assert, error = error,
})

Expand Down
6 changes: 4 additions & 2 deletions gateway/src/apicast/configuration_loader.lua
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ function boot.init(configuration)
if config and init then
ngx.log(ngx.DEBUG, 'downloaded configuration: ', config)
else
ngx.log(ngx.EMERG, 'failed to load configuration, exiting (code ', code, ')\n', err)
ngx.log(ngx.EMERG, 'failed to load configuration, exiting (code ', code, ')\n', err or ngx.config.debug and debug.traceback())
os.exit(1)
end

Expand Down Expand Up @@ -213,8 +213,10 @@ function lazy.rewrite(configuration, host)
return configuration
end

local test = { init = noop, init_worker = noop, rewrite = noop }

local modes = {
boot = boot, lazy = lazy, default = 'lazy'
boot = boot, lazy = lazy, default = 'lazy', test = test
}

function _M.new(mode)
Expand Down
6 changes: 2 additions & 4 deletions t/configuration-loading-boot-with-config.t
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
use lib 't';
use Test::APIcast::Blackbox 'no_plan';

$ENV{APICAST_CONFIGURATION_LOADER} = 'boot';

env_to_nginx(
'APICAST_CONFIGURATION_LOADER',
env_to_apicast(
'APICAST_CONFIGURATION_LOADER' => 'boot'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this allowed before or it's supported since Test::APIcast 0.08 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't really working before Test::APIcast 0.08. env_to_nginx was not working in Test::APIcast::Blackbox at all.

);

log_level('warn');
Expand Down