Skip to content

[Merged by Bors] - feat: capability to validate WASM files#2760

Closed
LeoBorai wants to merge 6 commits intofluvio-community:masterfrom
LeoBorai:feat/validate-smartmodule-wasmfile
Closed

[Merged by Bors] - feat: capability to validate WASM files#2760
LeoBorai wants to merge 6 commits intofluvio-community:masterfrom
LeoBorai:feat/validate-smartmodule-wasmfile

Conversation

@LeoBorai
Copy link
Copy Markdown
Contributor

Introduce validate_binary to validate WASM files (binary variant).
The validation approach consists on parsing WASM file bytes, when
an error is found the parser will short-circuit by returning an Err early.
To ensure the WASM file is valid, the file is parsed to the end.

Introduce `validate_binary` to validate WASM files (binary variant).
The validation approach consists on parsing WASM file bytes, when
an error is found the parser will short-circuit by returning an
`Err` early.

To ensure the WASM file is valid, the file is parsed to the end.
Comment thread crates/fluvio-controlplane-metadata/Cargo.toml Outdated
Comment thread crates/fluvio-controlplane-metadata/src/smartmodule/spec.rs Outdated
Comment thread crates/fluvio-controlplane-metadata/src/smartmodule/spec.rs Outdated
@LeoBorai LeoBorai requested a review from digikata October 27, 2022 22:41
Copy link
Copy Markdown
Contributor

@digikata digikata left a comment

Choose a reason for hiding this comment

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

The code LGTM. I like the unit tests cases - those cover some nice basics. Have you looked at the full range of users of fluvio-controlplane-metadata. I don't want to blow up the wasm size for the dashboard for example. One option is to put this fcn behind a feature.

@LeoBorai
Copy link
Copy Markdown
Contributor Author

The code LGTM. I like the unit tests cases - those cover some nice basics. Have you looked at the full range of users of fluvio-controlplane-metadata. I don't want to blow up the wasm size for the dashboard for example. One option is to put this fcn behind a feature.

Yeah! I thought about the feature approach as well. Let me do that so we can include it only when needed 😄

@LeoBorai LeoBorai requested a review from digikata October 28, 2022 03:48
Comment thread crates/fluvio-controlplane-metadata/Cargo.toml Outdated
@LeoBorai LeoBorai requested a review from digikata October 28, 2022 16:13
Copy link
Copy Markdown
Contributor

@digikata digikata left a comment

Choose a reason for hiding this comment

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

LGTM

@LeoBorai
Copy link
Copy Markdown
Contributor Author

bors r+

bors bot pushed a commit that referenced this pull request Oct 28, 2022
Introduce `validate_binary` to validate WASM files (binary variant).
The validation approach consists on parsing WASM file bytes, when
an error is found the parser will short-circuit by returning an `Err` early.
To ensure the WASM file is valid, the file is parsed to the end.
Comment thread crates/fluvio-controlplane-metadata/src/smartmodule/spec.rs Outdated
Comment thread crates/fluvio-controlplane-metadata/Cargo.toml Outdated
@bors
Copy link
Copy Markdown

bors bot commented Oct 28, 2022

Build failed:

@LeoBorai
Copy link
Copy Markdown
Contributor Author

I reduced the scope of the validation to Hub Util for our current use case.
Evaluating current API for SmartModuleWasm I can see that we have from_raw_wasm_bytes which also compresses
the file so I can't reuse it for our Hub Utility which uses another approach.

At some point it would be great to have a single representation we could use for both use cases, so validation is centralized
in a simple but reusable implementation.

@LeoBorai LeoBorai requested a review from sehz October 28, 2022 20:11
Copy link
Copy Markdown

@sehz sehz left a comment

Choose a reason for hiding this comment

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

LGTM. Later, we need to consolidate validation of WASM

@LeoBorai
Copy link
Copy Markdown
Contributor Author

LGTM. Later, we need to consolidate validation of WASM

Totally agree!

@LeoBorai
Copy link
Copy Markdown
Contributor Author

bors r+

bors bot pushed a commit that referenced this pull request Oct 29, 2022
Introduce `validate_binary` to validate WASM files (binary variant).
The validation approach consists on parsing WASM file bytes, when
an error is found the parser will short-circuit by returning an `Err` early.
To ensure the WASM file is valid, the file is parsed to the end.
@bors
Copy link
Copy Markdown

bors bot commented Oct 29, 2022

Build failed:

@LeoBorai
Copy link
Copy Markdown
Contributor Author

bors r+

bors bot pushed a commit that referenced this pull request Oct 29, 2022
Introduce `validate_binary` to validate WASM files (binary variant).
The validation approach consists on parsing WASM file bytes, when
an error is found the parser will short-circuit by returning an `Err` early.
To ensure the WASM file is valid, the file is parsed to the end.
@bors
Copy link
Copy Markdown

bors bot commented Oct 29, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: capability to validate WASM files [Merged by Bors] - feat: capability to validate WASM files Oct 29, 2022
@bors bors bot closed this Oct 29, 2022
davidbeesley pushed a commit to davidbeesley/fluvio that referenced this pull request Oct 31, 2022
Introduce `validate_binary` to validate WASM files (binary variant).
The validation approach consists on parsing WASM file bytes, when
an error is found the parser will short-circuit by returning an `Err` early.
To ensure the WASM file is valid, the file is parsed to the end.
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.

3 participants