Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: meta.filter in plugin doesn't work in response-rewrite plugin #8162

Merged
merged 8 commits into from
Oct 28, 2022
Merged

fix: meta.filter in plugin doesn't work in response-rewrite plugin #8162

merged 8 commits into from
Oct 28, 2022

Conversation

soulbird
Copy link
Contributor

@soulbird soulbird commented Oct 25, 2022 •
edited
Loading

Description

Fixes #7852

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@soulbird soulbird marked this pull request as ready for review October 25, 2022 08:36
@soulbird soulbird requested review from tzssangglass, spacewander and starsz and removed request for tzssangglass October 25, 2022 09:31
tokers
tokers reviewed Oct 25, 2022
View reviewed changes
apisix/plugin.lua Outdated Show resolved Hide resolved
apisix/plugin.lua Outdated Show resolved Hide resolved
t/plugin/plugin.t Show resolved Hide resolved
starsz
starsz reviewed Oct 25, 2022
View reviewed changes
apisix/plugin.lua
@@ -433,6 +438,8 @@ local function meta_filter(ctx, plugin_name, plugin_conf)
" plugin_name: ", plugin_name)
return true
end

ctx[match_cache_key] = ok
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can't cache the result of the filter. Since we have a different phase for every plugin like "rewrite", "access" e.g.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter works better at the plugin level and is executed at each phase, which will increase the difficulty for users to use.

spacewander
spacewander reviewed Oct 26, 2022
View reviewed changes
apisix/plugin.lua Outdated
@@ -415,6 +415,12 @@ local function meta_filter(ctx, plugin_name, plugin_conf)
return true
end

local match_cache_key =
ctx.conf_type .. "#" .. plugin_name .. "#meta_filter_matched"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this key work with multiple global_rules? They have the same conf_type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the scene where the con_type is the same and the plugin_name is different? I think my test case already contains. The conf_type is route.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soulbird
Multiple global rules will share the same conf_type

apisix/apisix/plugin.lua

Line 1003 in e2d0f0b

api_ctx.conf_type = "global_rule"

Will this affect the cache of eval result?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is indeed a problem, I adjusted match_cache_key, added some test case：https://github.com/apache/apisix/pull/8162/files#diff-29e2373df60fd8fc082317d17e636b6fa17c60459c1a691c5838f36b31cc486aR532-R567

spacewander
spacewander reviewed Oct 27, 2022
View reviewed changes
t/plugin/plugin.t Outdated Show resolved Hide resolved
@spacewander
Copy link
Member

@soulbird

Fixes #7852 #8087

Need to use double fixes

@soulbird
Copy link
Contributor Author

@soulbird

Fixes #7852 #8087

Need to use double fixes

My mistake, these two don't seem to be the same problem.

@spacewander spacewander merged commit 48b8431 into apache:master Oct 28, 2022
Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022
@soulbird @Liu-Junlin
fix: meta.filter in plugin doesn't work in response-rewrite plugin (a…
0b6420d 
…pache#8162)

Co-authored-by: soulbird <zhaothree@gmail.com>
hongbinhsu added a commit to fitphp/apix that referenced this pull request Nov 7, 2022
@hongbinhsu
Merge branch 'upstream/master' into github/master
c09a3a2 
* upstream/master: (94 commits)
  test: add test cases for ai module (apache#8222)
  fix(ai): typo key name (apache#8232)
  docs:  update the docs of admin API  (apache#8191)
  Update standalone docs for change in SSL parameter (apache#8242)
  fix: can not modify x-forwarded-host in proxy rewrite (apache#8200)
  chore: export the set cert/key process so we can hook it (apache#8228)
  test: add cases of ai module (apache#8214)
  feat: add dafault value roundrobin for upstream_schema (apache#8180)
  docs: update limit plugin docs (apache#8154)
  dix remote_addr (apache#8210)
  fix(zipkin): send trace IDs with a reject sampling decision (apache#8099)
  docs: update hmac-auth doc (apache#8192)
  docs: update limit count plugin doc (apache#8196)
  docs: fix building apisix misled (apache#8189)
  fix: meta.filter in plugin doesn't work in response-rewrite plugin (apache#8162)
  feat: release 3.0 (apache#8179)
  docs: refactor admin api doc (apache#8145)
  docs: refactor health check doc (apache#8129)
  docs: protect-api.md typo, remove redundant braces. (apache#8139)
  feat: renew route lrucache when the routes change (apache#8157)
  ...
spacewander pushed a commit to spacewander/incubator-apisix that referenced this pull request Nov 9, 2022
@soulbird @spacewander
fix: meta.filter in plugin doesn't work in response-rewrite plugin (a…
abec2f5 
…pache#8162)

Co-authored-by: soulbird <zhaothree@gmail.com>
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
spacewander pushed a commit that referenced this pull request Nov 10, 2022
@soulbird @spacewander
fix: meta.filter in plugin doesn't work in response-rewrite plugin (#…
e262a90 
…8162)

Co-authored-by: soulbird <zhaothree@gmail.com>
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
@tokers tokers mentioned this pull request Jan 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spacewander spacewander spacewander approved these changes

@starsz starsz starsz approved these changes

@tzssangglass tzssangglass Awaiting requested review from tzssangglass

@tokers tokers Awaiting requested review from tokers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: meta.filter in plugin doesn't work when we use some variable
4 participants
@soulbird @spacewander @tokers @starsz