ability to configure policy chains globally #496

mikz · 2017-11-22T10:34:02Z

closes #481

It is possible to return a table with field policy_chain that is going to be used as the policy chain instead of the default one.

Example:

local policy_chain = require('apicast.policy_chain').default()

policy_chain:insert(mypolicy)

return {
  policy_chain = policy_chain
}

mikz · 2017-11-24T13:58:37Z

examples/configuration/echo.lua

+policy_chain:insert(policy_chain.load('apicast.policy.echo'))
+
+return {
+	policy_chain = policy_chain


We will have to start documenting the environment config format. It is Lua, so no JSON schema.

davidor · 2017-11-27T09:42:41Z

spec/policy_chain_spec.lua

@@ -92,6 +92,38 @@ describe('policy_chain', function()
    end, 'readonly table')
  end)

+  describe('.insert', function()
+
+    it('adds policy to the chain', function()


I'd modify this test or add a new one. For .insert we need to test that it adds the policy to the end of the list. If we do not initialize the chain with a few elements, we won't be able to test that. This test would pass even if the code added the policy to the beginning of the list or in a random position.

Good point 👍 Will fix.

Fixed in d340bf1 but going to be squashed.

davidor · 2017-11-27T09:45:29Z

spec/policy_chain_spec.lua

+      assert.equal(0, #chain)
+    end)
+  end)
+


Would be good to test .default for completeness. The other two methods introduced in this PR for that module are tested.

Good point. Fixing 👍

Fixed in 1fbfff8 and will squash it in a rebase.

davidor · 2017-11-27T09:49:21Z

gateway/src/apicast/policy_chain.lua

+-- @tparam[opt] int position the position to add the policy to, defaults to last one
+function _M:insert(policy, position)
+    if self.frozen then
+        return nil, 'frozen chain'


Maybe we should be more specific in the error message. Not sure this will be obvious to contributors.

I think the error messages in openresty are pretty simple because people can treat them like error code and match them. I'd probably even go for just 'frozen' so it is easier to match.

Error codes from lua-resty-redis:

not initialized

not subscribed

closed

timeout

If "frozen" is not understandable I could rename it to: immutable, finalized, locked (other suggestions welcome).

Fair enough. I was thinking more about clarifying what the error is about to possible contributors rather than making sure it's easy to match.

That probably should be addressed by the method documentation describing the error messages. Will do 👍

Fixed in f4be8fb and going to be squashed.

davidor · 2017-11-27T09:52:25Z

gateway/src/apicast/policy_chain.lua

@@ -96,6 +109,25 @@ function _M:export()
    return chain
 end

+--- Freeze the policy chain to prevent modifications.
+-- After calling this method it won't be possible to insert more policies.
+-- @treturn self


nitpick: missing type

👍

This file is probably not generating the doc as we had a whitelist in config.ld.

Will add it there and see the generated documentation.

Fixed f4be8fb and going to be squashed.

davidor · 2017-11-27T09:56:43Z

examples/configuration/echo.lua

@@ -0,0 +1,7 @@
+local policy_chain = require('apicast.policy_chain').build({ })


I think these 2 lines could be simplified to:
local policy_chain = require('apicast.policy_chain').build({'apicast.policy.echo'})

Yes. I wanted to show the :insert as an example. But that would be better with the default chain.

What about:

local policy_chain = require('apicast.policy_chain').default() local echo = require('apicast.policy.echo').new({ exit = 0 }) policy_chain:insert(echo, 1)

?

It shows off the insert - how to extend the default chain and how to pass options to policy.

But apicast.policy.echo doesn't have a constructor that can receive that table.

Well I'd change it to accept a configuration for sake of the example.

OK. In that case, I think that the latest example looks better than the original one 👍

Fixed in 02ad4ab and going to be squashed or extracted into own commit.

davidor · 2017-11-27T10:03:06Z

examples/configuration/echo.lua

@@ -0,0 +1,7 @@
+local policy_chain = require('apicast.policy_chain').build({ })
+
+policy_chain:insert(policy_chain.load('apicast.policy.echo'))


This insert + load combination looks a bit confusing to me. I wonder if we should make .load private.

And just rely on require(...).new() instead of .load ?

What about .insert calling .load like .build does and making .load private?

I think that policy_chain:insert('apicast.policy.echo') or policy_chain:insert(echo.new(something)) looks better than policy_chain:insert(policy_chain.load(...)).

I'd like insert to be an API that does one thing only. So I'd prefer it does not initialize the module because then it would also accept the configuration.

I agree .load is kind of an useless abstraction and it would be better to use plain require. The only reason for is existence was that I wanted to have an abstraction so the internals can be replaced without much hassle.
But when replacing internal and for example changing :new to .new the policies would have to change anyway and it would leak this concept to the policy. But at least all the uses of the policy would not have to change.

So we should agree if we want to expose the policy initialization to users and deal with the consequences if we need to ever change it or expose some method to load/initialize policies.

The new example looks like 02ad4ab so it uses plain require. Guess that is ok for now.

davidor · 2017-11-27T10:08:13Z

t/configuration-loading-boot-with-config.t

 $ENV{APICAST_CONFIGURATION_LOADER} = 'boot';
-$ENV{THREESCALE_CONFIG_FILE} = 't/servroot/html/config.json';
+#$ENV{TEST_NGINX_NO_CLEAN} = 1;


Well spotted. 👍

Fixed in 19a3bb8 and will be squashed.

davidor · 2017-11-27T10:16:32Z

gateway/src/apicast/cli/command/start.lua


-    local template = Template:new(context, dir, true)
+local function apicast_root()
+    return resty_env.get('APICAST_DIR') or pl.path.abspath('.')


Should we call .value instead?

Yes. Good point!

Did mass change in 569c520 and will squash it.

davidor · 2017-11-27T10:22:55Z

gateway/src/apicast/cli/command/start.lua

+local function openresty_binary(candidates)
+    return resty_env.value('APICAST_OPENRESTY_BINARY') or
+        resty_env.value('TEST_NGINX_BINARY') or
+        pick_openesty(candidates)


typo, also, I find naming here a bit confusing. This method is choosing the openresty binary but it's calling one named pick_openresty.

Yes. I was too lazy to rename this function. Will do 👍

Fixed in d5a7ca0 and will squash it.

davidor · 2017-11-27T10:31:05Z

gateway/src/apicast/cli/environment.lua

+        insert(nameservers, tostring(nameserver))
+    end
+
+    if #nameservers > 0 then


Why return nil instead of {} ?

Because then it is easier to verify that there are no nameservers. resolver directive in the nginx conf has to have at least one server. So if there are none we can't set it.

Would be good to document this.

Hopefully 5aa2e0c is clear enough and describes that the type of this can be a table of strings or nil. Would that be enough?

davidor · 2017-11-27T10:32:33Z

gateway/src/apicast/cli/environment.lua

@@ -0,0 +1,129 @@
+local pl_path = require('pl.path')


Would be good to document the purpose of this module.

Yep, Opened #500 so we can see the documentation artifacts and verify it looks correctly. Will write it after that one is merged and I can see the documetation.

Documented in 5aa2e0c but going to squash it with the commit that introduced it.

davidor · 2017-11-27T10:49:04Z

I think it would be good to add an integration test that shows how to configure policy chains via environment.

mikz · 2017-11-27T10:49:40Z

@davidor yep that is a good idea 👍 Will do.

mikz · 2017-11-28T11:28:26Z

gateway/src/apicast/cli/environment.lua

+local _M = {}
+---
+-- @field default_environment Default environment name.
+-- @table self


Couldn't figure out a better way to document this. This actually means there would be a property self of the module returned by this file.
But there is no way I could find to have properties of the module just plain values with description. Only tables can be properties of the module and only tables can have fields.

returning policy_chain from the environment configuration will use that policy chain in nginx

davidor · 2017-11-28T17:08:53Z

examples/configuration/echo.lua

@@ -0,0 +1,8 @@
+local policy_chain = require('apicast.policy_chain').default()


examples/configurationcontains many examples for different things. I think it'd be good from now on to explain what each one of them is about. A sentence at the begginning of the file should be enough.

Good point. This one probably should be in own folder.

davidor · 2017-11-28T17:10:36Z

gateway/src/apicast/policy/echo.lua

@@ -1,9 +1,22 @@
-local policy = require('apicast.policy')
-local _M = policy.new('Echo Policy')
+local _M = require('apicast.policy').new('Echo Policy')


Similar to the comment above. This policy is trivial, but thinking about future contributors, I think that a description of 1 or 2 lines would help.

davidor

As I pointed out in the comments, I think there are a couple of descriptions that we need to add. Apart from that, looks good.

👌

so enironment configurations can do conversions

* show how environment configuration can define a policy_chain * show that it has access to ENV * change echo policy to have more granular configuration options

octobot assigned mikz Nov 22, 2017

mikz force-pushed the configure-policy-chain branch 4 times, most recently from 4574c66 to f2b9201 Compare November 24, 2017 13:32

mikz changed the title ~~[wip] ability to configure policy chains globally~~ ability to configure policy chains globally Nov 24, 2017

mikz requested a review from davidor November 24, 2017 13:57

mikz commented Nov 24, 2017

View reviewed changes

mikz force-pushed the configure-policy-chain branch from 3e32e24 to 9bf402f Compare November 24, 2017 14:10

davidor reviewed Nov 27, 2017

View reviewed changes

mikz force-pushed the configure-policy-chain branch from 9bf402f to 5aa2e0c Compare November 27, 2017 16:44

mikz commented Nov 28, 2017

View reviewed changes

davidor mentioned this pull request Nov 28, 2017

methods for policy chain manipulation #505

Merged

mikz mentioned this pull request Nov 28, 2017

allow passing multiple environment configuration files #504

Merged

mikz force-pushed the configure-policy-chain branch 2 times, most recently from 1690e57 to 6811716 Compare November 28, 2017 15:07

mikz mentioned this pull request Nov 28, 2017

[t] rewrite config loading test using blackbox #507

Merged

mikz force-pushed the configure-policy-chain branch from 6811716 to 90017b4 Compare November 28, 2017 16:14

[init] allow configuring policy chain

1d43cb5

returning policy_chain from the environment configuration will use that policy chain in nginx

mikz force-pushed the configure-policy-chain branch from 90017b4 to ee195d9 Compare November 28, 2017 16:40

mikz requested a review from davidor November 28, 2017 16:44

davidor reviewed Nov 28, 2017

View reviewed changes

davidor approved these changes Nov 28, 2017

View reviewed changes

[environment] expose tonumber and tostring

cdd64f7

so enironment configurations can do conversions

mikz force-pushed the configure-policy-chain branch from 464eff0 to 506c694 Compare November 29, 2017 08:57

mikz requested a review from davidor November 29, 2017 08:58

mikz force-pushed the configure-policy-chain branch 2 times, most recently from c7834b5 to a63eccc Compare November 29, 2017 09:18

[examples] policy chain configuration with echo policy

024363b

* show how environment configuration can define a policy_chain * show that it has access to ENV * change echo policy to have more granular configuration options

mikz force-pushed the configure-policy-chain branch from a63eccc to 024363b Compare November 29, 2017 09:19

davidor approved these changes Nov 29, 2017

View reviewed changes

davidor merged commit 2857766 into master Nov 29, 2017

davidor deleted the configure-policy-chain branch November 29, 2017 09:37

		@@ -0,0 +1,7 @@
		local policy_chain = require('apicast.policy_chain').build({ })

		@@ -0,0 +1,7 @@
		local policy_chain = require('apicast.policy_chain').build({ })

		policy_chain:insert(policy_chain.load('apicast.policy.echo'))

		@@ -0,0 +1,8 @@
		local policy_chain = require('apicast.policy_chain').default()

ability to configure policy chains globally #496

ability to configure policy chains globally #496

Conversation

mikz commented Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

davidor commented Nov 27, 2017

mikz commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidor left a comment

Choose a reason for hiding this comment

mikz commented Nov 22, 2017 •

edited

Loading

mikz Nov 27, 2017 •

edited

Loading

mikz Nov 28, 2017 •

edited

Loading

mikz Nov 28, 2017 •

edited

Loading