Policy chain - first version #450

mikz · 2017-10-10T12:39:03Z

Early prototype of how we could chain policies.

Policies can export data that is stored in read only structure and resolved in order they are defined in.
So values can be overlaid, but not overridden and only the latest one is being used.
This would allow overriding some internal structures being passed between policies.

Context object is passed to each policy phase invocation method. This context object has last layer mutable so policies can pass data.

Policy chain is itself a policy. All phase methods are compatible. So policy chain can contain policy chains.
This is used to split the chain into two: global and local. Global chain is responsible for loading configuration and local can be created based on downloaded configuration.

TODO

~ figure out how installed policies can change the nginx template ~ Allow policies to edit the nginx config #477
- the list of policies enabled globally can be defined by some global config
- the important piece is how would the policy be able to extend the nginx template
- consider how policies would interact with each other and blocks that could extend
- decide if it is template manipulation (like something like https://github.com/spree/deface ) or some dsl compiled to nginx config

octobot · 2017-10-10T12:41:34Z

	1 Error
🚫	Please include a CHANGELOG entry. You can find it at CHANGELOG.md.

	1 Warning
⚠️	PR is classed as Work in Progress

	1 Message
📖	Note, we hard-wrap at 80 chars and use 2 spaces after the last line.

Generated by 🚫 Danger

davidor · 2017-10-20T14:15:45Z

apicast/src/policy.lua

+local _M = {}
+
+local setmetatable = setmetatable
+local policy_chain = require('policy_chain')


How about requiring only what is needed? require('policy_chain').PHASES

That would not really save anything. The whole file is executed and evaluated anyway.
And it would require unrolling it back when we need second property from that module.

👍
policy no longer depends on policy_chain, so this no longer applies.

davidor · 2017-10-20T14:17:53Z

apicast/src/policy.lua

+        policy[policy_chain.PHASES[i]] = noop
+    end
+
+    return policy, mt


I'd document what this method returns as I think returning the mt might be a bit counter-intuitive. I think we should document that it's returned so policy.new can be overriden. Any other reason?

That is the only reason so far. I could also store it in _M like _M.class or _M.mt. Not sure what is better though.

But for sure has to be documented as this is public API for new policies.

I'll try to set the mt as metatable of the returned module. Then you could get it by getmetatable. Maybe it won't cause an infinite loop :)

In the end went with not returning it at all and letting the user to decide what to do with it. They should upvalue the new function to a variable and call it later.

davidor · 2017-10-20T14:18:48Z

apicast/src/policy/load_configuration.lua

@@ -0,0 +1,33 @@
+local setmetatable = setmetatable
+
+local _M, mt = require('policy').new()


We should probably call new() with a name. Otherwise, it will use the default one.

Good point!

All the policies are now initialized with names 👍

mikz · 2017-11-07T15:37:08Z

apicast/src/configuration_loader.lua

@@ -55,9 +55,9 @@ local function ttl()
 end

 function _M.global(contents)
-  local module = require('module')
+  local context = require('executor'):context()


This will not work when APICAST_MODULE is set.

mikz · 2017-11-08T07:57:22Z

apicast/src/management.lua

@@ -1,7 +1,7 @@
 local _M = {}

 local cjson = require('cjson')
-local module = require('module')
+local context = require('executor'):context()


This won't work when APICAST_MODULE env is set.

mikz · 2017-11-08T08:49:58Z

apicast/src/executor.lua

+local resty_env = require('resty.env')
+
+if resty_env.get('APICAST_MODULE') then
+  return require('module')


Maybe we should do this in the apicast module itself? To keep the compatibility with the old module system. Basically allow people to replace the APIcast module. But we need to figure out how to allow them to require it but to outside return their module.
Maybe this belongs to the policy_chain.build where it defaults to the apicast module?

How about doing this in the local_chain module?

When building the local chain, the default is require('policy_chain').build({ 'apicast' }). I think we could check the env 'APICAST_MODULE' and do require('policy_chain').build({ 'module' }) instead if it's set.

Hmmmm. That sounds good 👍

mikz · 2017-11-08T08:50:58Z

spec/proxy_spec.lua

@@ -66,7 +66,7 @@ describe('Proxy', function()
    assert.same('function', type(proxy.post_action))
  end)

-  it('finds service by host', function()
+  pending('finds service by host', function()


This needs to be moved to the find service policy spec.

mikz · 2017-11-08T08:51:05Z

spec/proxy_spec.lua

@@ -75,7 +75,7 @@ describe('Proxy', function()
    assert.falsy(proxy:find_service('unknown'))
  end)

-  it('does not return old configuration when new one is available', function()
+  pending('does not return old configuration when new one is available', function()


This needs to be moved to the find service policy spec.

I think this one belongs to configuration_store. In fact, the same thing it's already tested there. I think we can just delete this test.

davidor · 2017-11-08T13:35:33Z

There are some commits here already included in other PRs (curl in benchmarks, DEBUG=1, etc.)

This also moves one of the tests marked as 'pending' in the proxy specs to the new file.

Simple policy to be used in the tests. It will allow us to verify that the code define in the module for each phase is executed.

…y the apicast policy The policy_chain module initializes a chain with the apicast policy if none are specified, but I think that making the initialization explicit here improves readability.

As the documentation of the method says.

The module is designed so its users call phases() instead.

In fact, the same thing it's already tested there.

freeze() does not need to be exposed. We can remove it and perform the operation inline as it's just setting one attribute to true.

davidor · 2017-11-14T17:58:22Z

@mikz I addressed all the comments you and me wrote. I think this is ready.

mikz

I think this looks great! 👍 We can verify the APICAST_MODULE support later. I suspect there will be issues because the old modules don't export context. But we will solve that when the time comes.

octobot assigned mikz Oct 10, 2017

mikz force-pushed the policy-chain branch 2 times, most recently from 7f135dd to 4983ecf Compare October 11, 2017 11:57

mikz mentioned this pull request Oct 12, 2017

OIDC - Add support for logout and token revocation [THREESCALE-312] #435

Closed

davidor reviewed Oct 20, 2017

View reviewed changes

mikz force-pushed the policy-chain branch 9 times, most recently from 319b2e4 to 7624cd7 Compare November 7, 2017 15:35

mikz commented Nov 7, 2017

View reviewed changes

mikz force-pushed the policy-chain branch 2 times, most recently from 88ab50d to 7d4d450 Compare November 7, 2017 16:55

mikz commented Nov 8, 2017

View reviewed changes

davidor self-assigned this Nov 8, 2017

mikz added 3 commits November 8, 2017 15:14

prototype basic policy chain

e658d90

prototype of executor and context for sharing data

b1905a4

[policy] don't return metatable and let policy to figure it out

9d8c59d

mikz force-pushed the policy-chain branch from 7d4d450 to d01590d Compare November 8, 2017 14:14

davidor added 10 commits November 14, 2017 11:45

spec: add tests for PolicyChain

7a40112

spec: add tests for Executor

d9c3bc6

spec: add tests for find_service policy

d7db0df

This also moves one of the tests marked as 'pending' in the proxy specs to the new file.

spec: add tests for the load_configuration policy

c100bb4

Add phase logger policy

d5f1e6a

Simple policy to be used in the tests. It will allow us to verify that the code define in the module for each phase is executed.

t: add basic integration test for policy chains

5f89a97

spec: add tests for linked_list

83ed56d

policy/local_chain: make explicit that the default chain contains onl…

9dbb2a8

…y the apicast policy The policy_chain module initializes a chain with the apicast policy if none are specified, but I think that making the initialization explicit here improves readability.

linked_list: raise error when trying to modify read-only list

8075f69

policy: set version to 0.0 when not specified

34d5a94

As the documentation of the method says.

davidor force-pushed the policy-chain branch from f3e5fbf to 34d5a94 Compare November 14, 2017 10:46

policy: avoid exposing PHASES

e1a63cb

The module is designed so its users call phases() instead.

davidor force-pushed the policy-chain branch from 9deb048 to e1a63cb Compare November 14, 2017 10:57

davidor added 2 commits November 14, 2017 12:24

executor: extract global_chain() and DEFAULT_POLICIES

a6b9c94

spec/proxy: delete test that belongs to configuration_store

58b686f

In fact, the same thing it's already tested there.

mikz mentioned this pull request Nov 14, 2017

convert cloud hosted custom module into a policy 3scale/apicast-cloud-hosted#1

Merged

5 tasks

davidor added 3 commits November 14, 2017 15:37

Document policy-related modules

5dc8c54

policy_chain: delete unused add()

69ce941

policy_chain: simplify freeze of the chain

a6b047a

freeze() does not need to be exposed. We can remove it and perform the operation inline as it's just setting one attribute to true.

mikz mentioned this pull request Nov 14, 2017

Provide config to set global policies #481

Closed

davidor force-pushed the policy-chain branch from 8ada7d8 to ef63d2d Compare November 14, 2017 17:54

local_chain: keep compatibility with current module system

95af30e

davidor force-pushed the policy-chain branch from ef63d2d to 95af30e Compare November 14, 2017 17:57

davidor changed the title ~~[WIP] policy chain prototype~~ Policy chain - first version Nov 14, 2017

mikz commented Nov 14, 2017

View reviewed changes

davidor approved these changes Nov 15, 2017

View reviewed changes

davidor merged commit 72fac3f into master Nov 15, 2017

davidor deleted the policy-chain branch November 15, 2017 09:46

mikz mentioned this pull request Jun 11, 2018

Module/Plugin/Policy definition #427

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Policy chain - first version #450

Policy chain - first version #450

mikz commented Oct 10, 2017 •

edited

Loading

octobot commented Oct 10, 2017 •

edited

Loading

davidor Oct 20, 2017

mikz Nov 3, 2017

davidor Nov 13, 2017

davidor Oct 20, 2017

mikz Nov 3, 2017

mikz Nov 3, 2017 •

edited

Loading

mikz Nov 3, 2017

davidor Oct 20, 2017

mikz Nov 3, 2017

davidor Nov 14, 2017

mikz Nov 7, 2017

mikz Nov 8, 2017

mikz Nov 8, 2017

davidor Nov 14, 2017

mikz Nov 14, 2017

mikz Nov 8, 2017

davidor Nov 13, 2017

mikz Nov 8, 2017

davidor Nov 14, 2017

mikz Nov 14, 2017

davidor commented Nov 8, 2017

davidor commented Nov 14, 2017

mikz left a comment

		@@ -0,0 +1,33 @@
		local setmetatable = setmetatable

		local _M, mt = require('policy').new()

Policy chain - first version #450

Policy chain - first version #450

Conversation

mikz commented Oct 10, 2017 • edited Loading

TODO

octobot commented Oct 10, 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

mikz Nov 3, 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

davidor commented Nov 8, 2017

davidor commented Nov 14, 2017

mikz left a comment

Choose a reason for hiding this comment

mikz commented Oct 10, 2017 •

edited

Loading

octobot commented Oct 10, 2017 •

edited

Loading

mikz Nov 3, 2017 •

edited

Loading