fix(proxy-mirror): use with uri rewrite#8718
Merged
Merged
Conversation
spacewander
previously approved these changes
Jan 28, 2023
|
|
||
|
|
||
|
|
||
| === TEST 30: use proxy-rewrite to change uri before mirror |
Member
There was a problem hiding this comment.
Let's move it to a new file.
When the test file is too large, for example > 800 lines, you should split it to a new file. Please take a look at t/plugin/limit-conn.t and t/plugin/limit-conn2.t.
soulbird
approved these changes
Jan 29, 2023
spacewander
approved these changes
Jan 29, 2023
hongbinhsu
added a commit
to fitphp/apix
that referenced
this pull request
Jan 30, 2023
* upstream/master: (67 commits) fix: grpc-transcode plugin: fix map data population (apache#8731) change(jwt-auth): unify apisix/core/vault.lua and apisix/secret/vault.lua (apache#8660) feat: stream subsystem support consul_kv service discovery (apache#8633) fix(proxy-mirror): use with uri rewrite (apache#8718) ci: move helper script to the right dir (apache#8691) refactor(pubsub): simpify the get_cmd implementation (apache#8608) feat: stream subsystem support kubernetes service discovery (apache#8640) docs: fix deployment links (apache#8714) fix: remove backslash before slash when encoding (apache#8684) ci: kafka should register port in the zookeeper same as exposed (apache#8672) docs: fix plugin config naming (apache#8701) docs: fix code block (apache#8700) docs: rename kms to secret (apache#8697) docs: replace transparent logos with white background logos (apache#8689) fix: upgrade lua-resty-etcd to 1.10.3 (apache#8668) fix: upgrade casbin to 1.41.3 to improve performance (apache#8676) chore: make send_stream_request more clear (apache#8627) feat: stream subsystem support nacos service discovery (apache#8584) feat: stream subsystem support dns service discovery (apache#8593) refactor(admin): refactor resource routes (apache#8611) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
APISIX started to provide
_meta.priorityplugin field in 2.13 to help users rewrite the hard-coded plugin priority at runtime, so users can arbitrarily combine various plug-ins and use different order of precedence.When trying to use a combination of proxy-rewrite and proxy-mirror, a logical error occurs. In the default configuration, mirror takes precedence over rewrite, and when the user rewrites the uri, it does not take effect in mirror, which is not the user's expectation, so the user can rewrite the priority.
However, when you rewrite the priority you will find that the uri rewrite still does not take effect, because the mirror plugin uses the uri field in the nginx variable directly, ignoring the upstream_uri variable that other APISIX plugins can rewrite, which is the key to uri rewriting.
We can fix this behavior by using logic such as:
upstream_uri ! = nilandupstream_uri ! = ""(which means that other plugins or APISIX itself has rewritten the uri), mirror uses theupstream_urivalue as the new uri, while the existing prefix settings are not affected.upstream_uriis empty, it follows the same logic as before and usesvar.urito splice args.With this logic, any old usage is not affected (as you can see, I did not modify any of the old test cases), and this modification only takes effect when the user modifies the priority, modifies the uri in advance, or modifies the uri through other custom plugins, with minimal impact.
Fixes #8167
Mail list: https://lists.apache.org/thread/8y3kxrwxl9yh9f049660kbc1ob8vhqkk
BTW, I noticed that no one has raised any issues in the formal discussion channels (mail list), that thread has been there for three months. So I will assume that no one has objected to that.
And as I showed in the description above, this solution has minimal impact and does not cause any disruptive behavior.
Checklist