[wip] Add maintenance mode policy #561

alanmoran · 2018-01-23T05:36:52Z

First attempt at a PR to apicast. Really basic policy just to introduce myself to Lua and writing some tests.
Wasn't sure what was the best way to test my request didn't hit upstream.

Any comments, tips or suggestions?

davidor · 2018-01-23T09:48:42Z

gateway/src/apicast/policy/maintenance_mode/README.md

@@ -0,0 +1,25 @@
+#Maintenance Mode


Github fails to render this correctly. You need to add a space after '#'.

Same with all the other titles in this doc.

davidor · 2018-01-23T09:55:23Z

gateway/src/apicast/policy/maintenance_mode/README.md

+
+##Example Configuration
+```json
+"policy_chain": [{


I would wrap this between { and } so it becomes a valid json. Also, I think that indenting this would help readability.

davidor · 2018-01-23T09:57:38Z

gateway/src/apicast/policy/maintenance_mode/policy.lua

+
+
+function _M.new(configuration)
+  local policy = new(configuration)


There's no need to pass configuration as a param here.

This made me realize that we are doing the same in the echo policy. We'll need to fix that.

@davidor I think we might want to keep this as this is the signature of the initializer. That the underlying policy does not do anything with it is an implementation detail that can change in the future. What if the policy would actually store it in self.configuration ? We would have to change all policies to pass it again.

@mikz I see what you mean, but new() here is calling function policy.new() in policy.lua which does not accept any parameters. Maybe we should add it then to make it less confusing?

I guess we should document it and accept _configuration parameter which would not be used.

@davidor Ah damn. I actually followed the echo as my baseline. Are you both happy for me to keep this as the signature or change it for now?

davidor · 2018-01-23T09:58:47Z

gateway/src/apicast/policy/maintenance_mode/policy.lua

+
+function _M:access()
+
+local status = self.status_code


These 2 lines are not properly indented.

davidor · 2018-01-23T09:59:27Z

gateway/src/apicast/policy/maintenance_mode/policy.lua

+  return ngx.exit(ngx.status)
+end
+
+return _M


Github is complaining because there isn't a newline here.
Same with the rest of files included in this PR.

Mmm is this still complaining? I'm fairly sure my git removes the last newline of a file. (I need to check this)

@alanmoran I think there needs to be a new line.

@kevprice83 I added it and its there locally but I think git or github removing it.

davidor · 2018-01-23T10:09:26Z

t/apicast-policy-maintenance-mode.t

+      "proxy": {
+        "policy_chain": [
+        { "name": "apicast.policy.maintenance_mode",
+          	"configuration": {


Check indentation here.

davidor · 2018-01-23T10:24:56Z

t/apicast-policy-maintenance-mode.t

+Testing 3 things:
+1) Check default status code
+2) Check default response message
+3) TODO - validate upstream doesn't get the request


There are several options. You could print something in upstream and then check that it does not appear in the output, or you could put and assertion using luassert.

davidor · 2018-01-23T10:31:26Z

Great job @alanmoran 👍

I added some comments about minor issues I found, but this is a good example of how to write and test a policy. I would be interested to hear if you found the documentation to be good enough and the problems you had while writing the policy.

I don't think we should include this in the gateway/src/apicast/policy, but rather in the examples folder.

alanmoran · 2018-01-24T06:08:11Z

@davidor Thanks for the review. I really appreciate it. I've made most of the changes you've recommended and put my feedback into @vramosp feedback doc.

Agree on not adding it as one of the default OOTB polices. I'll wait until I hear back about where community policies might live. Could be a good first candidate.

andrewdavidmackenzie · 2018-01-24T09:30:17Z

consider adding as an example in a sub-folder for them?

mikz

Would be good to rebase this PR as #581 added an option to add policies to the example folder. And #579 changed the structure so the main file is called init.lua.
That should be the final structure of policies. Sorry for the hassle.

Guess we will have to figure out how to write and where to store the tests. I'd probably go for the policy itself or the examples/policies/t for tests using several of them. But that will require some changes to our CI and the test runner. I'll make those changes soon.

alanmoran · 2018-02-20T01:38:45Z

@mikz I've rebased and moved policy to the examples folder. Updated structure to use init.lua also.

You should be able to test by updating examples/policies/example.json:

{
  "services": [
    {
      "proxy": {
        "policy_chain": [
          {"name": "maintenance-mode", "version": "1.0.0",
            "configuration": {"message": "Be back soon..", "status": 503} },
          { "name": "ngx-example", "version": "1.0.0",
            "configuration": { "set_header": [{"name": "Example", "value": "Value" }] } },
          { "name": "apicast.policy.upstream",
            "configuration": {
              "rules": [{
                "regex": "/",
                "url": "http://echo:8081"
              }]
            }
          }
        ]
      }
    }
  ]
}

I haven't moved the tests yet but will do once we've decided what to do with them.

davidor · 2018-02-20T14:53:45Z

@alanmoran good job 👍

Unfortunately, as you can see, the tests failed on CircleCI.

nginx: [error] init_by_lua error: module 'init' not found:
no file '/opt/app-root/src/gateway/policies/maintenance_mode/builtin/init.lua'
no file '/opt/app-root/src/gateway/src/apicast/policy/maintenance_mode/init.lua'

This is because by default, Apicast does not look for policies in the examples directory where your policy is. We need to configure Apicast to look in that directory. This can be done using the APICAST_POLICY_LOAD_PATH https://github.com/3scale/apicast/blob/master/doc/parameters.md#apicast_policy_load_path or the --policy-load-path option. In the tests, this is the way we pass ENVs:

env_to_apicast(
    'APICAST_POLICY_LOAD_PATH' => "$ENV{PWD}/examples/policies"
);

That needs to be specified before run_tests(). You can find several examples in our existing tests.

There's another problem. All the policies that are not the built-in ones, need to have a version in the configuration so Apicast knows how to load them. So you need something like this in the configuration:

{
    "name": "maintenance-mode", "version": "1.0.0"
}

Also, notice that you specified maintenance-mode, but the path is called maintenance_mode. They need to match.

Hope this helps. If anything is not clear or you need help please let me know.

alanmoran · 2018-02-28T06:22:39Z

@davidor I've fixed up the PR. Biggest change really is that I moved the tests under examples/policies/t. I believe we need to do further work to get the CI testing those.

In terms of maintenance-mode and maintenance_mode, I actually followed ngx-example but happy to change. Doesn't seem to break?

mikz · 2018-02-28T07:31:17Z

@alanmoran after #632 is merged CI will run integration tests in examples folder too.

mikz · 2018-02-28T07:31:40Z

examples/policies/t/apicast-policy-maintenance-mode.t

+    "backend_authentication_value": "token-value",
+    "proxy": {
+      "policy_chain": [{
+          "name": "maintenance-mode", "version": "1.0.0"


This is invalid JSON and the test fails. There is missing , at the and of the line.

mikz · 2018-02-28T07:32:51Z

examples/policies/maintenance-mode/1.0.0/maintenance_mode.lua

+  return policy
+end
+
+function _M:access()


Both tests fail because this is supposed to be rewrite.

APIcast policy rejects the requests in rewrite phase because they don't have authentication.
So you either want to first require authentication and then show a maintenance page or show maintenance regardless of passed authentication parameters.

davidor · 2019-08-06T09:17:29Z

@alanmoran There's a new requirement to include this policy as a builtin one. I used your commits in #1105

octobot assigned alanmoran Jan 23, 2018

alanmoran force-pushed the add_maintenance_policy branch from 1c08f2b to 6c515ec Compare January 23, 2018 08:28

davidor reviewed Jan 23, 2018

View reviewed changes

alanmoran force-pushed the add_maintenance_policy branch 2 times, most recently from 04440d0 to e38cfa4 Compare January 24, 2018 04:48

mikz suggested changes Feb 12, 2018

View reviewed changes

alanmoran force-pushed the add_maintenance_policy branch 2 times, most recently from 0fdb184 to ca64a78 Compare February 20, 2018 01:35

alanmoran force-pushed the add_maintenance_policy branch from ca64a78 to 5278df3 Compare February 20, 2018 01:55

alanmoran force-pushed the add_maintenance_policy branch 6 times, most recently from 90c906f to d0fcdc3 Compare February 20, 2018 23:43

Adding Maintenance Mode policy

80cd176

alanmoran force-pushed the add_maintenance_policy branch 2 times, most recently from 7109d0e to 8546b16 Compare February 28, 2018 05:54

mikz reviewed Feb 28, 2018

View reviewed changes

mikz mentioned this pull request Feb 28, 2018

[make] run .t files in examples folder #632

Merged

t: test maintenance mode policy

4579a82

alanmoran force-pushed the add_maintenance_policy branch from 8546b16 to 4579a82 Compare February 28, 2018 21:49

davidor mentioned this pull request Aug 6, 2019

[THREESCALE-3189] Add maintenance policy #1105

Merged

davidor closed this Aug 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] Add maintenance mode policy #561

[wip] Add maintenance mode policy #561

alanmoran commented Jan 23, 2018 •

edited

Loading

davidor Jan 23, 2018

davidor Jan 23, 2018

davidor Jan 23, 2018

davidor Jan 23, 2018

davidor Jan 23, 2018

mikz Jan 23, 2018

davidor Jan 23, 2018

mikz Jan 23, 2018

alanmoran Jan 24, 2018

davidor Jan 23, 2018

davidor Jan 23, 2018

alanmoran Jan 24, 2018

kevprice83 Jan 24, 2018

alanmoran Jan 25, 2018

davidor Jan 23, 2018

davidor Jan 23, 2018

davidor commented Jan 23, 2018

alanmoran commented Jan 24, 2018

andrewdavidmackenzie commented Jan 24, 2018

mikz left a comment •

edited

Loading

alanmoran commented Feb 20, 2018 •

edited

Loading

davidor commented Feb 20, 2018 •

edited

Loading

alanmoran commented Feb 28, 2018

mikz commented Feb 28, 2018

mikz Feb 28, 2018

mikz Feb 28, 2018

davidor commented Aug 6, 2019



		function _M.new(configuration)
		local policy = new(configuration)

[wip] Add maintenance mode policy #561

[wip] Add maintenance mode policy #561

Conversation

alanmoran commented Jan 23, 2018 • 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

davidor commented Jan 23, 2018

alanmoran commented Jan 24, 2018

andrewdavidmackenzie commented Jan 24, 2018

mikz left a comment • edited Loading

Choose a reason for hiding this comment

alanmoran commented Feb 20, 2018 • edited Loading

davidor commented Feb 20, 2018 • edited Loading

alanmoran commented Feb 28, 2018

mikz commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidor commented Aug 6, 2019

alanmoran commented Jan 23, 2018 •

edited

Loading

mikz left a comment •

edited

Loading

alanmoran commented Feb 20, 2018 •

edited

Loading

davidor commented Feb 20, 2018 •

edited

Loading