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: adds support for newer config for wasm plugins. #10791

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jcchavezs
Copy link
Contributor

@jcchavezs jcchavezs commented Jun 7, 2024

Right now what we receive under config for a wasm plugin we forward it to the wasm guest without having the opportunity to config the runtime. This change allows to support a new config structure which fields under 'runtime' will configure the runtime and those under 'config' will be sent directly to the guest as well as supporting the old structure. This is related to #10739 and jcchavezs/coraza-http-wasm#19

Both configurations will be supported but I suggest we update Traefik documents to support the later one.

# old way
http:
# ...
  middlewares:
    waf:
      plugin:
        coraza:
          directives:
            - SecRuleEngine On
            - SecDebugLog /dev/stdout
# new way
http:
# ...
  middlewares:
    waf:
      plugin:
        coraza:
          runtime: 
            rootFS: /etc/traefik 
          guestConfig:
            directives:
              - SecRuleEngine On
              - SecDebugLog /dev/stdout

Tests will come if the design of the feature is approved.

Right now what we receive under config for a wasm plugin we forward it to the wasm guest without having the opportunity to config the runtime. This change allows to support a new config structure which fields under 'runtime' will configure the runtime and those under 'config' will be sent directly to the guest as well as supporting the old structure.
@mmatur
Copy link
Member

mmatur commented Jun 12, 2024

Hi @jcchavezs,

Thanks for this PR, with @juliens we are thinking about a different approach to configure the fs mount on the host.

WDYT of configuring the runtime mounts in the static configuration like that:

experimental:
  plugins:
    coraza:
      moduleName: "github.com/jcchavezs/coraza-http-wasm-traefik"
      version: "v0.2.1"
      runtime:
        mounts:
          - /src:/dst:ro

To us it will ease the UX because the mounts file need to be on the same host as Traefik and we are thinking that configuring mounts on the dynamic configuration could be confusing for end user.

@jcchavezs
Copy link
Contributor Author

@mmatur that sounds very legit but I wonder if runtime should be a first class element in plugin. Is there any other plugin that has extra configuration? shall we call it runtime or something like "host_config"?

Maybe we make this generic? Also, is it possible to pass this all the way up to the middleware creation?

@jcchavezs
Copy link
Contributor Author

jcchavezs commented Jun 12, 2024

I guess this is mainly for wasm environments as other plugins have direct access to filesystem.

@mmatur
Copy link
Member

mmatur commented Jun 12, 2024

@jcchavezs thanks for your answer

I wonder if runtime should be a first class element in plugin

We think that it should not because with this approach you can't have multiple plugins with different fs mount like the following

experimental:
  plugins:
    coraza:
      moduleName: "github.com/jcchavezs/coraza-http-wasm-traefik"
      version: "v0.2.1"
      runtime:
        mounts:
          - /src:/dst:ro
    coraza2:
      moduleName: "github.com/jcchavezs/coraza-http-wasm-traefik"
      version: "v0.2.1"
      runtime:
        mounts:
          - /tmp:/tmp:rw

Is there any other plugin that has extra configuration?

For now there is no other plugin that require this extra configuration.

Maybe we make this generic? Also, is it possible to pass this all the way up to the middleware creation?

It should.

I guess this is mainly for wasm environments as other plugins have direct access to filesystem.

Today, yes it is only for WASM plugin but we can imagine introducing that for yaegi in the future for security reason (like enabling unsafe)

@jcchavezs
Copy link
Contributor Author

jcchavezs commented Jun 12, 2024 via email

@juliens
Copy link
Member

juliens commented Jun 20, 2024

Hello @jcchavezs

During our work on this PR #10829 we integrated this feature, if it's seems ok for you, may be we can close this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants