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: Wasm extension HTTP code source #3164

Merged
merged 26 commits into from
Apr 20, 2024
Merged

Conversation

zhaohuabing
Copy link
Member

@zhaohuabing zhaohuabing commented Apr 10, 2024

This PR implements the HTTP code source for Wasm extension.

@zhaohuabing zhaohuabing requested a review from a team as a code owner April 10, 2024 09:54
@zhaohuabing zhaohuabing marked this pull request as draft April 10, 2024 09:54
@zhaohuabing zhaohuabing force-pushed the wasm-http branch 3 times, most recently from ea8e717 to 07495ec Compare April 11, 2024 07:30
// SHA256 *string `json:"sha256,omitempty"`
//
// kubebuilder:validation:Pattern=`^[a-f0-9]{64}$`
SHA256 string `json:"sha256"`
Copy link
Member Author

@zhaohuabing zhaohuabing Apr 11, 2024

Choose a reason for hiding this comment

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

Make SHA256 mandatory for now to avoid downloading wasm and calculate SHA in the control plane.

We can make it optional without breaking changes if we opt to build a cache in the control plane in the future.

@zhaohuabing zhaohuabing force-pushed the wasm-http branch 7 times, most recently from d81834d to adaa4c8 Compare April 11, 2024 07:50
@zhaohuabing zhaohuabing marked this pull request as ready for review April 11, 2024 07:50
@zhaohuabing zhaohuabing marked this pull request as draft April 11, 2024 07:52
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@zhaohuabing zhaohuabing marked this pull request as ready for review April 11, 2024 09:26
@zhaohuabing zhaohuabing requested review from arkodg and guydc April 11, 2024 09:28
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@zhaohuabing zhaohuabing requested a review from Xunzhuo April 12, 2024 08:42
This reverts commit 64b76a9.
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Copy link

codecov bot commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 44.44444% with 135 lines in your changes are missing coverage. Please review.

Project coverage is 66.48%. Comparing base (29946b0) to head (67977a3).
Report is 67 commits behind head on main.

❗ Current head 67977a3 differs from pull request most recent head 0c52d50. Consider uploading reports for the commit 0c52d50 to get more accurate results

Files Patch % Lines
internal/xds/translator/wasm.go 15.44% 97 Missing and 7 partials ⚠️
internal/gatewayapi/envoyextensionpolicy.go 74.69% 16 Missing and 5 partials ⚠️
internal/xds/translator/utils.go 70.00% 6 Missing and 3 partials ⚠️
internal/xds/translator/jwt.go 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3164      +/-   ##
==========================================
- Coverage   66.51%   66.48%   -0.03%     
==========================================
  Files         161      163       +2     
  Lines       22673    22994     +321     
==========================================
+ Hits        15080    15288     +208     
- Misses       6720     6824     +104     
- Partials      873      882       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@@ -31,14 +31,16 @@ type Wasm struct {
// RootID is a unique ID for a set of extensions in a VM which will share a
// RootContext and Contexts if applicable (e.g., an Wasm HttpFilter and an Wasm AccessLog).
// If left blank, all extensions with a blank root_id with the same vm_id will share Context(s).
// RootID *string `json:"rootID,omitempty"`
// RootID must match the root_id parameter used to register the Context in the Wasm code.
RootID *string `json:"rootID,omitempty"`
Copy link
Member Author

@zhaohuabing zhaohuabing Apr 18, 2024

Choose a reason for hiding this comment

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

code:
type: HTTP
http:
url: https://raw.githubusercontent.com/envoyproxy/envoy/main/examples/wasm-cc/lib/envoy_filter_http_wasm_example.wasm
Copy link
Member Author

Choose a reason for hiding this comment

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

@arkodg The HTTP code source does support HTTPS schema.

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@zhaohuabing zhaohuabing requested a review from guydc April 18, 2024 14:00
@@ -46,12 +46,12 @@ type EnvoyExtensionPolicySpec struct {
// TargetRef
TargetRef gwapiv1a2.PolicyTargetReferenceWithSectionName `json:"targetRef"`

// WASM is a list of Wasm extensions to be loaded by the Gateway.
// Wasm is a list of Wasm extensions to be loaded by the Gateway.
Copy link
Contributor

Choose a reason for hiding this comment

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

pascal case semantics ?

Copy link
Member Author

@zhaohuabing zhaohuabing Apr 18, 2024

Choose a reason for hiding this comment

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

Following the name convention here:https://webassembly.org/

WebAssembly (abbreviated Wasm) is a binary instruction format for a stack-based virtual machine. Wasm is designed as a portable compilation target for programming languages, enabling deployment on the web for client and server applications.

api/v1alpha1/wasm_types.go Outdated Show resolved Hide resolved
@@ -31,14 +31,16 @@ type Wasm struct {
// RootID is a unique ID for a set of extensions in a VM which will share a
// RootContext and Contexts if applicable (e.g., an Wasm HttpFilter and an Wasm AccessLog).
// If left blank, all extensions with a blank root_id with the same vm_id will share Context(s).
// RootID *string `json:"rootID,omitempty"`
// RootID must match the root_id parameter used to register the Context in the Wasm code.
RootID *string `json:"rootID,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

why can't we reuse name internally here ?

Copy link
Member Author

@zhaohuabing zhaohuabing Apr 18, 2024

Choose a reason for hiding this comment

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

In theory, yes, Name can be the same as RootID. But while RootID must match the root_id in the wasm code, Name can be any meaningful name the user wants to name this wasm extension. They are set in different phases:

  • RootID: by the developer when writing the wasm source code, can't be changed after wasm module is published
  • Name: by the admin when writing the EnvoyExtensionPolicy, can be changed on the fly
          name: envoy.filters.http.wasm/envoyextensionpolicy/envoy-gateway/policy-for-gateway/0
          typedConfig:
            '@type': type.googleapis.com/envoy.extensions.filters.http.wasm.v3.Wasm
            config:
              configuration:
                '@type': type.googleapis.com/google.protobuf.StringValue
                value: '{"parameter1":{"key1":"value1","key2":"value2"},"parameter2":"value3"}'
              name: some-meaningful-name # can be anything
              rootId: my-root-id. # must be the same root_id in the wasm source code
              vmConfig:
                code:
                  remote:
                    httpUri:
                      cluster: www_example_com_443
                      timeout: 10s
                      uri: https://www.example.com/wasm-filter-1.wasm
                    sha256: 746df05c8f3a0b07a46c0967cfbc5cbe5b9d48d0f79b6177eeedf8be6c8b34b5
                runtime: envoy.wasm.runtime.v8
                vmId: envoyextensionpolicy/envoy-gateway/policy-for-gateway/0

Copy link
Contributor

Choose a reason for hiding this comment

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

If we can generate this field internally, using name, my vote is to do that and not expose another field the user needs to think about how to populate

Copy link
Member Author

@zhaohuabing zhaohuabing Apr 18, 2024

Choose a reason for hiding this comment

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

We can't generate RootID, See: https://github.com/proxy-wasm/proxy-wasm-cpp-sdk/blob/921039ae983ce053bf5cba78a85a3c08ff9791e5/proxy_wasm_intrinsics.cc#L27-L28

And it can be optional if the wasm source code leaves it empty.

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@zhaohuabing zhaohuabing requested a review from arkodg April 19, 2024 01:34
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team April 19, 2024 22:10
@zirain zirain merged commit fcfeefd into envoyproxy:main Apr 20, 2024
20 checks passed
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.

5 participants