[cli] allow changing policy load path #581

mikz · 2018-02-07T14:22:24Z

Allow configuring policy load paths from the CLI.
That makes easier testing and development.

davidor · 2018-02-08T11:05:45Z

gateway/src/apicast/policy_loader.lua

@@ -211,16 +214,24 @@ local mt = {
  __call = function(loader, ...) return loader.env.require(...) end
 }

-function _M.new(name, version, dir)
-  local apicast_dir = dir or getenv('APICAST_DIR') or '.'
+do


Why is this in a do block?

It makes a variable that is not really needed to be exposed for the rest of the module.

I've seen this in resty-core and other resty libraries. I think it is quite nice pattern to create temporary variables.

davidor · 2018-02-08T11:17:07Z

gateway/conf.d/apicast.conf

-  set $service_id null;
-  set $proxy_pass null;
-  set $secret_token null;
+  set $cached_key '';


There's something I don't understand about this. I see in the commit comment that you're doing this to avoid sending this in headers for example. Why are they sent when set to null and not sent when set to '' ?

Nginx is not going to send values that are empty strings. But it was sending null. Which was pretty ugly. Imo better to keep it empty than send null as a string.

👍
That's a surprising behavior.

davidor · 2018-02-08T11:24:20Z

examples/policies/ngx-example/1.0.0/init.lua

@@ -0,0 +1,35 @@
+


I think we should follow the same pattern we follow for our policies, that is, having a init.lua with only a require and the code in a different .lua file.

We should probably add a json schema as well to show a complete example.

First part done. Will do the schema now.

Done in e6785ae

davidor · 2018-02-08T11:28:19Z

examples/policies/README.md

@@ -0,0 +1,31 @@
+# Policies examples
+
+You can start example configuration with example policy by runing this from the APIcast root directory:


I think that in order to be able to follow the example, we need to specify here what's the policy being loaded in the example configuration file and what it does.

Better now?

davidor · 2018-02-08T11:29:41Z

examples/policies/README.md

+And see similar line in the APIcast log:
+
+```
+2018/02/08 10:01:48 [warn] 72249#10132050: *9 [lua] init.lua:19: op(): setting header: Example to: Value, client: 127.0.0.1, server: _, request: "GET /test HTTP/1.1", host: "localhost:8080"


This shouldn't be a warning, no? It's the purpose of the policy. Warning sounds like something is not working as it should, I'd use a different log level for the example.

Ok. Will tune it to info/notice level.

davidor · 2018-02-08T11:36:14Z

examples/policies/ngx-example/1.0.0/init.lua

+end
+
+function _M:rewrite()
+  for _,op in pairs(self.ops) do


It should be ipairs, no?

davidor · 2018-02-08T14:22:24Z

examples/policies/ngx-example/1.0.0/apicast-policy.json

+  "version": "0.1",
+  "configuration": {
+    "type": "object",
+    "additionalProperties": {


Maybe this would be easier to understand if we made the fields explicit by specifying a set_header object with two properties: header and value. What do you think?

True. That is what I wanted to do :) I might need to change it to an array, but that is probably for the better.

Changed to an array.

mikz · 2018-02-09T10:22:20Z

gateway/http.d/init.conf

@@ -12,6 +12,13 @@ init_by_lua_block {
    local context = env:context()

    local resty_env = require('resty.env')
+    -- this file both liquid template and pure nginx file
+    -- {% raw %} and {{ ENV }} is both valid Lua code and Liquid variable interpolation {% endraw %}
+    -- so we can push ENV from outside when this file is included and inject extra environment


@davidor I definitely have to improve this and properly document it, because this is serious mindfuck.

I was thinking about something like:

-- WARNING, WARNING, WARNING: this is insane hack and should not be touched -- This file is not templated by Liquid, because it is used by not blackbox integration tests (Search for TEST_NGINX_HTTP_CONFIG). -- So it can't be templated by liquid, to lift env variables and persist them in the configuration. -- This is a workaround so we can store environment at the time of building the config into the config itself. -- {% raw %} {{ ENV }} {% endraw %} is both valid Liquid template and Lua code. -- In Lua it is a table with another empty table inside. In Liquid it prints variable ENV. -- 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.

* set variables to empty strings instead of `null` - so they are not sent in headers for example * define echo upstream, so it is detected by the resolver

So the perl script can detect where are builtin policies and set up policy loader accordingly. Not all the time the source code is in APICAST_DIR.

* because nginx `env` directive is applied before master works workers - so it is invisible in the init phase

octobot assigned mikz Feb 7, 2018

mikz force-pushed the policy-load-paths branch 3 times, most recently from b3b96d6 to 976d6c5 Compare February 7, 2018 15:19

mikz requested a review from davidor February 7, 2018 15:19

mikz force-pushed the policy-load-paths branch 3 times, most recently from bd3ff39 to cf8637c Compare February 8, 2018 09:41

davidor reviewed Feb 8, 2018

View reviewed changes

mikz force-pushed the policy-load-paths branch 2 times, most recently from 9414490 to e6785ae Compare February 8, 2018 13:03

davidor reviewed Feb 8, 2018

View reviewed changes

mikz force-pushed the policy-load-paths branch 3 times, most recently from 0cc5d71 to aaa7ab5 Compare February 9, 2018 07:14

mikz commented Feb 9, 2018

View reviewed changes

mikz force-pushed the policy-load-paths branch from 41bc0df to 189585f Compare February 9, 2018 10:55

davidor mentioned this pull request Feb 9, 2018

Add more documentation on Policies #574

Closed

mikz force-pushed the policy-load-paths branch from 189585f to f8ff3cc Compare February 9, 2018 10:59

mikz added 5 commits February 9, 2018 12:39

[cli] allow changing policy load path

fccfd46

[policy/upstream] allow use of internal Echo API

8c43df8

* set variables to empty strings instead of `null` - so they are not sent in headers for example * define echo upstream, so it is detected by the resolver

[example] add policy example

1667f06

[cli] internal env APICAST_BUILTIN_POLICY_LOAD_PATH

29ad6b7

So the perl script can detect where are builtin policies and set up policy loader accordingly. Not all the time the source code is in APICAST_DIR.

[nginx] move setting env to init phase

9d29550

* because nginx `env` directive is applied before master works workers - so it is invisible in the init phase

mikz force-pushed the policy-load-paths branch from f8ff3cc to 9d29550 Compare February 9, 2018 12:13

mikz requested a review from davidor February 9, 2018 12:49

davidor approved these changes Feb 9, 2018

View reviewed changes

davidor merged commit e23fd55 into master Feb 9, 2018

davidor deleted the policy-load-paths branch February 9, 2018 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cli] allow changing policy load path #581

[cli] allow changing policy load path #581

mikz commented Feb 7, 2018

davidor Feb 8, 2018

mikz Feb 8, 2018

davidor Feb 8, 2018

davidor Feb 8, 2018

mikz Feb 8, 2018

davidor Feb 8, 2018

davidor Feb 8, 2018

davidor Feb 8, 2018

mikz Feb 8, 2018

mikz Feb 8, 2018

mikz Feb 8, 2018

davidor Feb 8, 2018

mikz Feb 8, 2018

davidor Feb 8, 2018

davidor Feb 8, 2018

mikz Feb 8, 2018

mikz Feb 8, 2018

davidor Feb 8, 2018

mikz Feb 8, 2018

mikz Feb 8, 2018

davidor Feb 8, 2018

mikz Feb 8, 2018

mikz Feb 8, 2018

mikz Feb 9, 2018

		@@ -0,0 +1,31 @@
		# Policies examples

		You can start example configuration with example policy by runing this from the APIcast root directory:

[cli] allow changing policy load path #581

[cli] allow changing policy load path #581

Conversation

mikz commented Feb 7, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment