feat: support conditional response rewrite #3577

lilien1010 · 2021-02-10T09:24:58Z

What this PR does / why we need it:

Allow user modify http reponse with conditional vars.

Eg:

we can rewrite response code 502 to 504 with this plugin, rather than write a new plugin
add special http header for request witch specific vars
dropif directives when transfer from nginx to APISIX

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

moonming · 2021-02-13T00:44:02Z

Can this function be realized by plugin orchestration?

membphis · 2021-02-14T02:34:40Z

This PR only changes the response HTTP status, and we still cannot use proxy_next_upstream.

lilien1010 · 2021-02-18T03:22:23Z

@membphis @moonming
the 504/502 example might not quite reasonable
my idea is that vars allows do conditional overwrite logic(mainly rewrite header), which is useful for users to transfer their dated mannuly maintained nginx.conf that with numberous if directives to APISIX.

also the plugin orchestration is a little bit obscure for someone new to APISIX.

lilien1010 · 2021-02-18T03:31:11Z

the idea to achieve this feature was inspired by friends in Tencent while they are moving their old nginx to APISIX.

spacewander · 2021-02-19T01:20:44Z

apisix/plugins/response-rewrite.lua

@@ -107,6 +145,12 @@ function _M.body_filter(conf, ctx)
 end

 function _M.header_filter(conf, ctx)
+


Don't need to extra blank line

spacewander · 2021-02-19T01:20:55Z

apisix/plugins/response-rewrite.lua

    return true
 end


 do

 function _M.body_filter(conf, ctx)
+


spacewander · 2021-02-19T01:21:53Z

apisix/plugins/response-rewrite.lua

    return true
 end


 do

 function _M.body_filter(conf, ctx)
+
+    local ok =  vars_matched(conf, ctx)


We should cache the result from header_filter.

spacewander · 2021-02-19T01:22:09Z

apisix/plugins/response-rewrite.lua

+    if conf.vars then
+        local ok, err = expr.new(conf.vars)
+        if not ok then
+            return nil, "failed to validate the 'vars' expression: " .. err


Need a test to validate the vars.

spacewander · 2021-02-22T06:41:54Z

t/plugin/response-rewrite.t

+            local plugin = require("apisix.plugins.response-rewrite")
+            local ok, err = plugin.check_schema({
+                vars = {
+                    {"status","==",200}


Need a case fails to validate, as the successful case is already covered later.

spacewander · 2021-02-22T06:42:11Z

apisix/plugins/response-rewrite.lua

@@ -107,6 +143,11 @@ function _M.body_filter(conf, ctx)
 end

 function _M.header_filter(conf, ctx)
+    ctx.reponse_rewrite_matched =  vars_matched(conf, ctx)


Extra space found

tokers · 2021-02-27T01:36:56Z

doc/plugins/response-rewrite.md

@@ -45,6 +45,7 @@ response rewrite plugin, rewrite the content returned by the upstream as well as
 | body        | string  | optional    |         |            | New `body` to client, and the content-length will be reset too.                                                                                                                                                               |
 | body_base64 | boolean | optional    | false   |            | Identify if `body` in configuration need base64 decoded before rewrite to client.                                                                                                                                             |
 | headers     | object  | optional    |         |            | Set the new `headers` for client, can set up multiple. If it exists already from upstream, will rewrite the header, otherwise will add the header. You can set the corresponding value to an empty string to remove a header. |
+| vars     | array[]  | optional    |         |            | A DSL to evaluate with the given ngx.var. See `vars` [lua-resty-expr](https://github.com/api7/lua-resty-expr#operator-list). |


Should mention here that "if vars is absent, rewrite operations will be executed unconditionally".

spacewander

Please merge master to make CI pass.

membphis · 2021-03-01T15:50:56Z

@lilien1010 please merge the master branch: Conflicting files

lilien1010 changed the title ~~support conditinal response rewrite~~ feature:support conditinal response rewrite Feb 10, 2021

lilien1010 changed the title ~~feature:support conditinal response rewrite~~ feature:support conditional response rewrite Feb 11, 2021

lilien1010 changed the title ~~feature:support conditional response rewrite~~ feat: support conditional response rewrite Feb 12, 2021

lilien1010 force-pushed the support-conditional-response-rewrite branch from c0f146e to 32bd68d Compare February 12, 2021 16:04

lilien1010 marked this pull request as ready for review February 12, 2021 17:21

lilien1010 requested a review from membphis February 12, 2021 17:21

lilien1010 requested a review from spacewander February 18, 2021 03:33

spacewander reviewed Feb 19, 2021

View reviewed changes

spacewander reviewed Feb 22, 2021

View reviewed changes

tokers reviewed Feb 27, 2021

View reviewed changes

spacewander approved these changes Mar 1, 2021

View reviewed changes

tokers approved these changes Mar 1, 2021

View reviewed changes

lilien1010 added 9 commits March 3, 2021 10:55

support conditinal response rewrite

3ba4708

fix code style

e554431

fix typo

db9649d

fix typo

8e3c065

fix test case

8b2fd04

add test case and fix code style

ae04053

drop space

050488e

add fail test cases

11d6409

enhance docs

c712dea

lilien1010 force-pushed the support-conditional-response-rewrite branch from 629be80 to c712dea Compare March 3, 2021 02:56

lilien1010 merged commit 66539b3 into apache:master Mar 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support conditional response rewrite #3577

feat: support conditional response rewrite #3577

lilien1010 commented Feb 10, 2021 •

edited

Loading

moonming commented Feb 13, 2021

membphis commented Feb 14, 2021

lilien1010 commented Feb 18, 2021

lilien1010 commented Feb 18, 2021

spacewander Feb 19, 2021

lilien1010 Feb 21, 2021

spacewander Feb 19, 2021

lilien1010 Feb 21, 2021

spacewander Feb 19, 2021

lilien1010 Feb 21, 2021

spacewander Feb 19, 2021

lilien1010 Feb 21, 2021

spacewander Feb 22, 2021

lilien1010 Feb 26, 2021

spacewander Feb 22, 2021

lilien1010 Feb 26, 2021

tokers Feb 27, 2021

lilien1010 Feb 27, 2021

spacewander left a comment

membphis commented Mar 1, 2021

		@@ -107,6 +145,12 @@ function _M.body_filter(conf, ctx)
		end

		function _M.header_filter(conf, ctx)

feat: support conditional response rewrite #3577

feat: support conditional response rewrite #3577

Conversation

lilien1010 commented Feb 10, 2021 • edited Loading

What this PR does / why we need it:

Pre-submission checklist:

moonming commented Feb 13, 2021

membphis commented Feb 14, 2021

lilien1010 commented Feb 18, 2021

lilien1010 commented Feb 18, 2021

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

spacewander left a comment

Choose a reason for hiding this comment

membphis commented Mar 1, 2021

lilien1010 commented Feb 10, 2021 •

edited

Loading