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

add filter WasmHost (close #1) #116

Merged
merged 6 commits into from Jul 15, 2021
Merged

add filter WasmHost (close #1) #116

merged 6 commits into from Jul 15, 2021

Conversation

localvar
Copy link
Collaborator

@localvar localvar commented Jul 10, 2021

add the WasmHost filter, it is disabled by default in the build because of the requirements of CGo, and can be enabled by the build command below:

$ make GOTAGS=wasmhost

and this PR only includes the filter itself, we still need to develop SDKs for different languages to empower users to write their own code for the filter.

@benja-wu benja-wu added the enhancement New feature or request label Jul 10, 2021
@benja-wu benja-wu added this to In progress in Easegress Project via automation Jul 10, 2021
@benja-wu benja-wu added this to the v.1.1.0 milestone Jul 10, 2021
@localvar localvar marked this pull request as draft July 10, 2021 11:53
@localvar localvar marked this pull request as ready for review July 11, 2021 03:04
README.md Outdated Show resolved Hide resolved
doc/filters.md Outdated Show resolved Hide resolved
doc/filters.md Outdated Show resolved Hide resolved
pkg/api/api.go Show resolved Hide resolved
pkg/cluster/layout.go Outdated Show resolved Hide resolved
pkg/filter/wasm/wasm.go Outdated Show resolved Hide resolved
pkg/filter/wasm/hostfunc.go Outdated Show resolved Hide resolved
pkg/filter/wasm/wasm.go Outdated Show resolved Hide resolved
pkg/filter/wasm/wasm.go Outdated Show resolved Hide resolved
pkg/filter/wasm/wasm.go Outdated Show resolved Hide resolved
Copy link
Contributor

@benja-wu benja-wu left a comment

Choose a reason for hiding this comment

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

LGTM

# Cgo is disabled by default
ENABLE_CGO= CGO_ENABLED=0

# Check Go build tags, the tags are from command line of make
Copy link
Contributor

Choose a reason for hiding this comment

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

Check whether docker image with alpine runs correctly please.

ENABLE_CGO= CGO_ENABLED=0

# Check Go build tags, the tags are from command line of make
ifdef GOTAGS
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I don't think it's a big deal to include more than 20MB in EG with wasm. I think it brings more concern in compilation, which gives more confusing stuff to developers, operators.


const (
// Kind is the kind of WasmFilter.
Kind = "WasmFilter"
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a filter naming WasmFilter that sounds weird/redundant to me, can we rename it to WasmRuntime or something else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wasmtime and wasmer are wasm runtimes, so WasmRuntime is not a suitable name for the filter, in fact, I tried to find another one, but none seems better.

Copy link
Contributor

Choose a reason for hiding this comment

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

WasmVM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there's WasmVM already in our code...

Copy link
Contributor

Choose a reason for hiding this comment

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

How about Wasm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wasm often refers to many other things like the wasm code and may bring additional confusion.
I'll rename it to WasmHost as it is a host environment of WebAssembly, any idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with it

pkg/filter/wasm/wasm.go Outdated Show resolved Hide resolved
pkg/filter/wasm/wasm.go Outdated Show resolved Hide resolved
pkg/filter/wasm/hostfunc.go Outdated Show resolved Hide resolved
@localvar localvar mentioned this pull request Jul 14, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@6d63a22). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #116   +/-   ##
=======================================
  Coverage        ?   46.48%           
=======================================
  Files           ?       38           
  Lines           ?     2590           
  Branches        ?        0           
=======================================
  Hits            ?     1204           
  Misses          ?     1305           
  Partials        ?       81           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d63a22...f0ec2b9. Read the comment docs.

@localvar localvar changed the title add wasm filter (close #1) add filter WasmHost (close #1) Jul 15, 2021
Easegress Project automation moved this from In progress to Reviewer approved Jul 15, 2021
@xxx7xxxx xxx7xxxx merged commit dbb8ffb into easegress-io:main Jul 15, 2021
Easegress Project automation moved this from Reviewer approved to Done Jul 15, 2021
@localvar localvar deleted the wasm branch July 15, 2021 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants