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.

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

feat: Implement "rewrite" using Wasm Plugin #339

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

WeixinX
Copy link
Collaborator

@WeixinX WeixinX commented May 19, 2023

Ⅰ. Describe what this PR did

Implement rewrite strategy using Wasm plugin to gain a better extensibility.

This plugin functions as Rewrite Annotation.

The plugin model is as follows:

type RewriteConfig struct {
	rewriteRules []RewriteRule
}

type RewriteRule struct {
	matchPathType string // prefix | exact | regex
	caseSensitive bool
	matchHosts    []string
	matchPaths    []string
	rewriteHost   string
	rewritePath   string
}

The global configuration enables users to have more flexible rewriting policies (rules).

Ⅱ. Does this pull request fix one issue?

fixes #334

Ⅲ. Why don't you add test cases (unit test/integration test)?

I have used docker compose and WasmPlugin for local functional tests, and the test results are in line with expectations.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@WeixinX WeixinX requested a review from johnlanni as a code owner May 19, 2023 08:26
@codecov-commenter
Copy link

Codecov Report

Merging #339 (036879c) into main (625c06e) will decrease coverage by 0.07%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #339      +/-   ##
==========================================
- Coverage   42.01%   41.94%   -0.07%     
==========================================
  Files          44       44              
  Lines        5850     5850              
==========================================
- Hits         2458     2454       -4     
- Misses       3217     3221       +4     
  Partials      175      175              

see 1 file with indirect coverage changes

|-----------------|-----------------|------|-------|-------------------------------------------------------------------------------|
| match_path_type | string | 选填 | - | 配置请求路径的匹配类型,可选值为:前缀匹配 prefix, 精确匹配 exact, 正则匹配 regex |
| case_sensitive | bool | 选填 | false | 配置匹配时是否区分大小写,默认不区分 |
| match_hosts | array of string | 选填 | - | 配置会被重写的请求域名列表,支持精确匹配(hello.world.com),最左通配(\*.world.com)和最右通配(hello.world.\*) |
Copy link
Collaborator

Choose a reason for hiding this comment

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

match_hosts 和 match_paths 的功能跟插件机制自带的匹配功能重复了,这个插件应当只处理匹配后的重写机制。
此外,路径重写需要考虑正则重写(看到已经支持)和前缀重写。目前重写 annotation 的设计是根据当前路径匹配类型自动设定重写类型,例如精确匹配->精确重写;前缀匹配->前缀重写。
这个插件可以提供用户更灵活的选择,让用户自行选择重写机制。对于前缀和正则重写,需要设定一个重写目标,字段名可以是 rewrite_target。

Copy link
Collaborator Author

@WeixinX WeixinX May 21, 2023

Choose a reason for hiding this comment

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

@johnlanni 感谢指出问题 ~

那么这个插件的模型是否可以改写为:

type RewriteConfig struct {
	rewriteRules []RewriteRule
}

type RewriteRule struct {
	caseSensitive bool   // 大小写是否敏感
	rewriteType   string // 请求路径重写类型 prefix | exact | regex
	rewriteHost   string // 请求域名会被重写为 rewriteHost
	rewritePath   string // 请求路径会被重写为 rewritePath
}

根据文档中 rewrite annotation 的使用示例,我觉得 rewrite_target 和这里的 rewrite_path 功能一样,但在源码中同时有 rewrite-path 和 -target 两个 annotation,我想请问它们的区别是什么?🙏🏻

Copy link
Collaborator

@johnlanni johnlanni May 21, 2023

Choose a reason for hiding this comment

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

我上面说的是 rewrite-path 的机制;
rewrite-target 是为了兼容 nginx ingress 注解设计的,可以看下 nginx ingress的文档:https://kubernetes.github.io/ingress-nginx/examples/rewrite/

这个配置可以的,不过对于正则重写,还需有个配置用于正则匹配捕获,来实现上面 rewrite target 的能力

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

还有一个问题想讨教 ~
前缀重写指的是例如:
请求路径 /v1/api/get 匹配到了路由 /v1/api,此时重写类型为 prefix,重写路径为 /v2,因此该请求路径被重写为 /v2/get 吗?

Copy link
Collaborator

@johnlanni johnlanni May 21, 2023

Choose a reason for hiding this comment

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

type RewritePath struct {
     type string // prefix | exact | regex
     matchPath string // 用于正则匹配并捕获,或者前缀匹配
     caseSensitive bool
     rewritePath string
}
type RewriteRule struct {
	rewriteHost   string 
	rewritePath   RewritePath
}

这样可能更好,作用字段含义更明确些

Copy link
Collaborator

Choose a reason for hiding this comment

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

还有一个问题想讨教 ~ 前缀重写指的是例如: 请求路径 /v1/api/get 匹配到了路由 /v1/api,此时重写类型为 prefix,重写路径为 /v2,因此该请求路径被重写为 /v2/get 吗?

参考上面配置格式,如果 type是 prefix,matchPath是 /v1/api,rewritePath是/v2,那么会被重写为 /v2/get

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

感谢 ~ 我有空重构一下。另外是否需要添加 e2e usecase?

Copy link
Collaborator

Choose a reason for hiding this comment

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

嗯,目前做插件的 e2e 需要提供现成的 oci 镜像,这个我记个 issue ,需要优化下 e2e 流程来支持

@johnlanni johnlanni marked this pull request as draft May 25, 2023 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement "rewrite" strategy using Wasm Plugin
3 participants