-
Notifications
You must be signed in to change notification settings - Fork 454
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: optimize transformer go wasm plugin #712
feat: optimize transformer go wasm plugin #712
Conversation
…com/Uncle-Justice/higress into optimize-transformer-go-wasm-plugin
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #712 +/- ##
==========================================
+ Coverage 37.99% 38.10% +0.11%
==========================================
Files 61 61
Lines 10322 10336 +14
==========================================
+ Hits 3922 3939 +17
+ Misses 6099 6098 -1
+ Partials 301 299 -2 |
请 @WeixinX 看下 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
除了以下修改意见外,也请麻烦更新 README,另外由于 E2E 还未支持 body 的校验,在本地测试时也请多关注一下 ~
) | ||
|
||
type Param struct { | ||
paramType ParamType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
与父级的 operate 字段重合,个人认为可以去掉 ParamType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已将Param.paramType
类型修改为string
,直接根据operate
值进行初始化;删除ParamType
类型声明
mapSource := getMapSourceFromRule(config.reqRules) | ||
|
||
// TODO: not support mapping headers or querys from body in requestTransformer before #582 is fixed | ||
if mapSource == "body" && (isHeaderChange || isQueryChange) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isHeaderChange 似乎并不能说明一定存在 header map 操作,isQueryChange 同理,它们只说明了 headers 或 querys 中存在某种转换操作。尽管 mapSource 被配置能说明 headers, querys 中存在 map 操作,但不排除用户错误配置的可能?807 行同理。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已将不支持map的条件更加细化。比如此处,必须在headers或者query中检测出map要求,才不支持相应map操作
switch config.reqTrans.GetMapSource() { | ||
case "headers": | ||
{ | ||
headers, err := proxywasm.GetHttpRequestHeaders() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetHttpRequestHeaders is used for retrieving HTTP request headers. Only available during types.HttpContext.OnHttpRequestHeaders and types.HttpContext.OnHttpStreamDone.
有一种方案是在 onHttpRequestHeaders 时获取 headers,并通过 ctx 传递。586 行同理。
也请添加 map from header 的测试用例 ~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 已在
onHttpReq/RespBody
中对headers和query的获取方式改为通过ctx
- 已增加map from header to query的测试用例,发现因为在之前的代码读取headers时会将其全部改为小写(
func convertHeaders(hs [][2]string) map[string][]string
),而qquery并未做这样的操作, 导致如果headers中的某个fromKey含大写字母,query匹配不上,只能匹配小写版的fromKey,请问这个算预期内行为吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Uncle-Justice @johnlanni 可以考虑在匹配时将 query 转为小写进行匹配?或是保留 header 原本的状态?(不确定 proxywasm.GetHttpRequestHeaders() 获取到的 header 是否有进行小写转换)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已修改为:当映射来源为headers时,fromKey以小写形式去做匹配
|
||
type Param struct { | ||
paramType ParamType | ||
paramType string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
目前的逻辑是在使用到 paramType
的地方(constructParam)会将 operate
复制给 paramType
,所以我认为可以去掉该字段,用 operate
字段做相应的判断即可
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
你说得对,这个后面也没有使用,该字段现已删除
switch config.reqTrans.GetMapSource() { | ||
case "headers": | ||
{ | ||
headers, err := proxywasm.GetHttpRequestHeaders() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Uncle-Justice @johnlanni 可以考虑在匹配时将 query 转为小写进行匹配?或是保留 header 原本的状态?(不确定 proxywasm.GetHttpRequestHeaders() 获取到的 header 是否有进行小写转换)
@WeixinX proxywasm.GetHttpRequestHeaders() 获取到的header key都是小写的 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@Uncle-Justice Please resolve the conflicts. |
@johnlanni Roger that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
…optimize-transformer-go-wasm-plugin
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
fix #587
Ⅲ. Why don't you add test cases (unit test/integration test)?
已附带相应e2e测试
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews
目前的代码结构决定了,map操作的顺序是headers->querys->body,即如果我要做headers的map操作,映射来源是querys时,实际参考的数据是未被transform过的旧query,以上逻辑应该是合理的,不过或许需要在readme中追加说明。