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

Wasm configuration v2 #37

Merged
merged 14 commits into from
Aug 16, 2023
Merged

Wasm configuration v2 #37

merged 14 commits into from
Aug 16, 2023

Conversation

eguzki
Copy link
Contributor

@eguzki eguzki commented Jul 18, 2023

What

Fixes #35
Depends on Kuadrant/wasm-test-framework#1 for e2e tests to pass

Example to illustrate the new wasm shim configuration struct

failureMode: deny
rateLimitPolicies:
  - name: rlp-ns-A/rlp-name-A
    domain: rlp-ns-A/rlp-name-A
    service: rate-limit-cluster
    hostnames: ["*.toystore.com"]
    rules:   
      - conditions:      
        - allOf:
          - selector: <value>
            operator: <value>
            value: <value>
          - selector: <value>
            operator: <value>
            value: <value>
        - allOf:
          - selector: <value>
            operator: <value>
            value: <value>
          - selector: <value>
            operator: <value>
            value: <value>
        data:
        - selector:
            selector: <value>       # Any attribute from a well-known specification TBD ([ref](https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/attributes#arch-overview-request-attributes))
            key: <value>             # Optional. If not set it defaults to `selector` field value as the descriptor key.
            default: <value>        # Optional. An optional value to use if metadata_key is empty. If not set and no value is present under the metadata_key then no descriptor is generated.
        - static:   
            key: <value>             # Required
            value: <value>          # Required

TODO:

@eguzki eguzki force-pushed the wasm-configuration-v2 branch from 948697d to 793ee32 Compare July 19, 2023 09:09
@guicassolato
Copy link
Contributor

LGTM so far, @eguzki. Nice job!

@eguzki eguzki force-pushed the wasm-configuration-v2 branch from 13d1fa8 to 9fe81fc Compare July 20, 2023 21:17
@eguzki eguzki force-pushed the wasm-configuration-v2 branch from 18f5484 to 2350f7e Compare July 21, 2023 15:14
EqualOperator,
#[serde(rename = "neq")]
NotEqualOperator,
#[serde(rename = "starts_with")]
Copy link
Contributor

Choose a reason for hiding this comment

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

RLPv2 specifies and effectively the Kuadrant Operator generates for the WasmPlugin config startswith, not starts_with.

In fact, while testing this branch, I've observed the following error thrown by the gateway:

2023-07-25T12:20:20.200842Z     warning envoy wasm external/envoy/source/extensions/common/wasm/context.cc:1151 wasm log: failed to parse plugin config: unknown variant `startswith`, expected one of `eq`, `neq`, `starts_with`, `ends_with`, `matches` at line 1 column 184     thread=20

Copy link
Contributor

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

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

Operators startswith and endswith do not match with how they are generated by the controller.

@@ -46,9 +46,9 @@ pub enum WhenConditionOperator {
EqualOperator,
#[serde(rename = "neq")]
NotEqualOperator,
#[serde(rename = "starts_with")]
#[serde(rename = "startsWith")]
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
#[serde(rename = "startsWith")]
#[serde(rename = "startswith")]

😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

already pushed the changes :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, you mean startswith, no camel case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok ok... fixing it

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@eguzki eguzki marked this pull request as ready for review July 25, 2023 16:12
@eguzki eguzki force-pushed the wasm-configuration-v2 branch from a4bac8a to e522bf6 Compare July 25, 2023 17:09
@eguzki
Copy link
Contributor Author

eguzki commented Jul 25, 2023

@guicassolato ready for another review as you requested changes

@eguzki
Copy link
Contributor Author

eguzki commented Jul 25, 2023

green 🥳

@eguzki eguzki merged commit 9042214 into main Aug 16, 2023
@eguzki eguzki deleted the wasm-configuration-v2 branch August 16, 2023 13:11
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.

WASM Configuration v2
3 participants