-
Notifications
You must be signed in to change notification settings - Fork 478
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
Wasm middleware: add ability to pass config to guest #2918
Wasm middleware: add ability to pass config to guest #2918
Conversation
Signed-off-by: zhangchao <[email protected]>
@codefromthecrypt can you please take a look? |
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.
Looks good as only minor things. Like usual we need the docs updated with text on what "config" means because plain property names are ambiguous (e.g. is this the guest or is this the host?)
middleware/http/wasm/httpwasm.go
Outdated
@@ -54,7 +58,8 @@ func (m *middleware) getHandler(ctx context.Context, metadata dapr.Metadata) (*r | |||
WithRandSource(rand.Reader). | |||
WithSysNanosleep(). | |||
WithSysWalltime(). | |||
WithSysNanosleep())) | |||
WithSysNanosleep()), | |||
handler.GuestConfig(wasmConfig)) |
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.
can you raise a PR upstream to coerce empty to nil?
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.
When writing into memory, the upstream has checked the len
of config bytes, does it necessary to coerce empty to nil?
// getConfig implements the WebAssembly host function handler.FuncGetConfig.
func (m *middleware) getConfig(_ context.Context, mod wazeroapi.Module, stack []uint64) {
buf := uint32(stack[0])
bufLimit := handler.BufLimit(stack[1])
configLen := writeIfUnderLimit(mod.Memory(), buf, bufLimit, m.guestConfig)
stack[0] = uint64(configLen)
}
func writeIfUnderLimit(mem wazeroapi.Memory, offset, limit handler.BufLimit, v []byte) (vLen uint32) {
vLen = uint32(len(v))
if vLen > limit {
return // caller can retry with a larger limit
} else if vLen == 0 {
return // nothing to write
}
mem.Write(offset, v)
return
}
Signed-off-by: zhangchao <[email protected]> # Conflicts: # middleware/http/wasm/example/router.wasm # middleware/http/wasm/httpwasm.go # middleware/http/wasm/internal/e2e-guests/output/main.wasm # middleware/http/wasm/internal/e2e-guests/rewrite/main.wasm
Signed-off-by: zhangchao <[email protected]>
Signed-off-by: zhangchao <[email protected]>
Signed-off-by: zhangchao <[email protected]>
Thanks, I will open a doc issue and pr soon. |
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.
only request is to revert the change to the other binaries so that we don't thrash git with binary changes. I think tinygo isn't 100pct idempotent, and we only changed the config one (new one)
Signed-off-by: zhangchao <[email protected]>
Signed-off-by: zhangchao <[email protected]>
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 but can you please add the copyright headers?
Also, does the binding need the configuration too?
Signed-off-by: zhangchao <[email protected]>
Signed-off-by: zhangchao <[email protected]>
I think this PR only needs to rebase |
Signed-off-by: zhangchao <[email protected]> # Conflicts: # middleware/http/wasm/benchmark_test.go # middleware/http/wasm/httpwasm.go # middleware/http/wasm/internal/e2e_test.go
@Taction is this ready to be merged? Does the binding need configuration too? |
@ItalyPaleAle This is ready to be merged. The binding doesn't need this. |
@Taction ok I’ll merge once CI is green. why do bindings not need this? |
For middleware, since it can not access the env and file system, there needs a way for wasm code to get the configuration. |
well put @Taction. In technical terms, there are no ABI requirements for binding as it should be fully described via the command/exec pattern. |
Signed-off-by: zhangchao <[email protected]>
Description
Add config for http wasm middleware.
Wasm code can get this configuration via
handler.Host.GetConfig()
, examples inmiddleware/http/wasm/internal/e2e-guests/config/main.go
.Also, fix some inconsistencies in the benchmark tests.
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #2917
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: