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: initial wasm support #5288

Merged
merged 2 commits into from
Oct 22, 2021
Merged

feat: initial wasm support #5288

merged 2 commits into from
Oct 22, 2021

Conversation

spacewander
Copy link
Member

@spacewander spacewander commented Oct 20, 2021

Signed-off-by: spacewander spacewanderlzx@gmail.com

What this PR does / why we need it:

Fix #157

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

Signed-off-by: spacewander <spacewanderlzx@gmail.com>

* All plugin run in the same WASM VM, like the Lua plugin in the Lua VM
* Each plugin has its own VMContext (the root ctx)
* Each configured route/global rules has its own PluginContext (the plugin ctx).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Each configured route/global rules has its own PluginContext (the plugin ctx).
* Each configured route/global rules has its own PluginContext (the plugin ctx).

Copy link
Member Author

Choose a reason for hiding this comment

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

The For example in the next line belongs to this item.

* All plugin run in the same WASM VM, like the Lua plugin in the Lua VM
* Each plugin has its own VMContext (the root ctx)
* Each configured route/global rules has its own PluginContext (the plugin ctx).
For example, if we have a service configures with WASM plugin, and two routes inherit from it,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
For example, if we have a service configures with WASM plugin, and two routes inherit from it,
For example, if we have a service configuring the WASM plugin, and two routes inherit from it,

there will be two plugin ctxs.
* Each HTTP request which hits the configuration will have its own HttpContext (the HTTP ctx).
For example, if we configure both global rules and route, the HTTP request will
have two HTTP ctxs, one for the plugin ctx from global rules and another for the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
have two HTTP ctxs, one for the plugin ctx from global rules and another for the
have two HTTP ctxs, one for the plugin ctx from global rules and the other for the

Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

A benchmark is missing, confirm that it works

type = "object",
properties = {
name = {
type = "string"
Copy link
Member

Choose a reason for hiding this comment

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

Lack of a reasonable length range?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need to have a limitation of the length. Otherwise people will ask they can't set a xxx with length yyy.

type = "string"
},
file = {
type = "string"
Copy link
Member

Choose a reason for hiding this comment

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

Ditoo

function get_plugin_ctx_key(ctx)
key_buf[1] = ctx.conf_type
key_buf[2] = ctx.conf_id
return concat(key_buf, "#", 1, 2)
Copy link
Member

Choose a reason for hiding this comment

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

style: ctx.conf_type .. '#' .. ctx.conf_id is better

Copy link
Member Author

Choose a reason for hiding this comment

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

The current way is faster

└────────────────────────────────────────────────────────────────┘
```

* All plugin run in the same WASM VM, like the Lua plugin in the Lua VM
Copy link
Member

Choose a reason for hiding this comment

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

All plugin -> All plugins

@spacewander
Copy link
Member Author

A benchmark is missing, confirm that it works

I will add it later, this PR already has more than 1000 lines.

Signed-off-by: spacewander <spacewanderlzx@gmail.com>
@spacewander
Copy link
Member Author

@membphis @tokers
Updated.

Comment on lines +26 to +27
This plugin requires APISIX to run on [APISIX-OpenResty](../how-to-build.md#step-6-build-openresty-for-apache-apisix), and is under construction.
Currently, only a few APIs are implemented. Please follow [wasm-nginx-module](https://github.com/api7/wasm-nginx-module) to know the progress.
Copy link
Contributor

Choose a reason for hiding this comment

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

and is under construction -> which is under construction

Copy link
Member Author

Choose a reason for hiding this comment

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

No, after this change it seems APISIX-OpenResty is under construction.

apisix/wasm.lua Show resolved Hide resolved
apisix/wasm.lua Show resolved Hide resolved
@spacewander spacewander merged commit fa8a34f into apache:master Oct 22, 2021
@spacewander spacewander deleted the was branch October 22, 2021 08:29
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.

feature: support WebAssembly in apisix.
6 participants