-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 WASM support for json.is_valid built-in. #4204
Add WASM support for json.is_valid built-in. #4204
Conversation
wasm: Add support for WASM and simple tests. internal: Add opa_json_is_valid to map of wasm built-ins. docs: Indicate that WASM support is now available for json.is_valid. Fixes open-policy-agent#4140 Signed-off-by: Kristian Svalland <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful. Thanks a lot for picking this up! I've got a couple of nitpicks but nothing major really.
Thanks again 🚀
wasm/src/encoding.c
Outdated
if (r == NULL) | ||
{ | ||
return opa_boolean(false); | ||
} | ||
|
||
return opa_boolean(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] I'd probably go with
if (r == NULL) | |
{ | |
return opa_boolean(false); | |
} | |
return opa_boolean(true); | |
return opa_boolean(r != NULL); |
but it really doesn't matter. I guess it's all the same to the compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this more, maybe a free wouldn't hurt?
if (r == NULL) | |
{ | |
return opa_boolean(false); | |
} | |
return opa_boolean(true); | |
if (r == NULL) | |
{ | |
return opa_boolean(false); | |
} | |
opa_free(r); | |
return opa_boolean(true); |
WDYT? It's been a while since I've played with those parts of the code base 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like both suggestions. The latter probably don't hurt, as it did not break the tests at least, so I went with that. After all, we do not need the content of r, so we can free it.
I am not too familiar with the GC of WASM/C though.
wasm/tests/test.c
Outdated
@@ -1790,6 +1790,8 @@ void test_json(void) | |||
{ | |||
test("json/marshal", opa_value_compare(opa_json_marshal(opa_string_terminated("string")), opa_string_terminated("\"string\"")) == 0); | |||
test("json/unmarshal", opa_value_compare(opa_json_unmarshal(opa_string_terminated("\"string\"")), opa_string_terminated("string")) == 0); | |||
test("json/is_valid_true", opa_value_compare(opa_json_is_valid(opa_string_terminated("\"string\"")), opa_boolean(true)) == 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] We're already dealing with booleans, can we do
test("json/is_valid_true", opa_value_compare(opa_json_is_valid(opa_string_terminated("\"string\"")), opa_boolean(true)) == 0); | |
test("json/is_valid_true", opa_cast_boolean(opa_json_is_valid(opa_string_terminated("\"string\""))); |
and !opa_cast_boolean(...)
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, seems that it does not work the way we would expect for opa_boolean_t. The following fails
test("json/false_is_false", !opa_cast_boolean(opa_boolean(false)));
But the following seems to work by referencing the bool field v
of opa_boolean_t
,
test("json/false_is_false", !opa_cast_boolean(opa_boolean(false))->v);
I am not entirely sure that this is the correct way?
Fixes open-policy-agent#4140 Signed-off-by: Kristian Svalland <[email protected]>
43bb262
to
1c7d15a
Compare
Thanks for your feedback and review :) |
@kristiansvalland thanks for picking this up! Have you been trying to improve the state of the world, or do you have a specific interest in Wasm? (Sorry, I'm just curious) |
@srenatus I recently started using OPA professionally, and thought that I might as well contribute some back to the project and learn something along the way. While I do find Wasm interesting, that was not the main motivation for me picking this up -- I thought that the suggested steps in the original issue was quite helpful and gave it a go. But I like the sound of "improve the state of the world", so I will consider using that moving forward 😄 |
commit dbf3e9c9817ea625e0868d83c6e56ebd75e8c248 Merge: 6572cf6e 24050684 Author: Justin Lindh <[email protected]> Date: Wed Jan 12 14:07:56 2022 -0700 Merge branch 'reachable_paths' of github.com:justinlindh-wf/opa into reachable_paths Signed-off-by: Justin Lindh <[email protected]> commit 6572cf6e040dbff50e4518ba646429b9ba0414ce Author: Justin Lindh <[email protected]> Date: Wed Jan 12 14:07:47 2022 -0700 ast: remove unnecessary array nesting and cleanup tests Signed-off-by: Justin Lindh <[email protected]> commit 2405068419e9299b12c43cdd09828e0009ba3fbd Merge: dd39b4bd 38d0df0 Author: Justin Lindh <[email protected]> Date: Wed Jan 12 11:08:10 2022 -0700 Merge branch 'main' into reachable_paths commit dd39b4bdf340f46182a34058490f1ed1bd3dacf9 Author: Justin Lindh <[email protected]> Date: Wed Jan 12 11:04:42 2022 -0700 Update docs/content/policy-reference.md Co-authored-by: Stephan Renatus <[email protected]> commit 38d0df0 Author: Anders Eknert <[email protected]> Date: Tue Jan 11 09:24:31 2022 +0100 cicd: Update release notes mentions (open-policy-agent#4207) Since Github will automatically link to user profiles mentioned by their username, _and_ create a "Contributors" section for the release notes with user avatars, it seems good to follow that convention. Signed-off-by: Anders Eknert <[email protected]> commit c676a7e Author: wasm-updater <[email protected]> Date: Tue Jan 11 06:49:24 2022 +0000 wasm: Update generated binaries commit 3250a2c Author: Kristian Svalland <[email protected]> Date: Tue Jan 11 07:47:33 2022 +0100 wasm: Add native support for json.is_valid (open-policy-agent#4204) wasm: Add support for WASM and simple tests. internal: Add opa_json_is_valid to map of wasm built-ins. docs: Indicate that WASM support is now available for json.is_valid. Fixes open-policy-agent#4140 Signed-off-by: Kristian Svalland <[email protected]> commit abc08cd8977cb677cbb28c6c55afe9e17658cf3a Author: Justin Lindh <[email protected]> Date: Mon Jan 10 16:44:09 2022 -0700 ast: rename test cases from yml to yaml Signed-off-by: Justin Lindh <[email protected]> commit 3fdce9e23406422a24b82f21a792c2ac92b8f650 Author: Justin Lindh <[email protected]> Date: Mon Jan 10 14:17:02 2022 -0700 ast: graph.reachable_paths is SDK-dependant Signed-off-by: Justin Lindh <[email protected]> commit 79d86d0a1af1c220eb4da180e85b6ebccab7738e Merge: 791f1bbc 04425f05 Author: Justin Lindh <[email protected]> Date: Mon Jan 10 13:13:31 2022 -0700 Merge branch 'reachable_paths' of github.com:justinlindh-wf/opa into reachable_paths commit 791f1bbc952919660f024f094b7e2579895c26e0 Merge: fe6fef47 6090608 Author: Justin Lindh <[email protected]> Date: Mon Jan 10 13:12:13 2022 -0700 Merge branch 'main' of github.com:open-policy-agent/opa into reachable_paths commit fe6fef470f4aea8035d7ab175ce20a53deab2052 Author: Justin Lindh <[email protected]> Date: Mon Jan 10 13:11:54 2022 -0700 ast: add graph.reachable_paths Signed-off-by: Justin Lindh <[email protected]> commit 04425f0543ba149cefde7a6880abac7d8161354b Merge: 1afc295c 6090608 Author: Justin Lindh <[email protected]> Date: Mon Jan 10 12:56:34 2022 -0700 Merge branch 'main' into reachable_paths commit 6090608 Author: Anders Eknert <[email protected]> Date: Mon Jan 10 20:51:39 2022 +0100 opa inspect: unhide command (open-policy-agent#4194) People aren't going to find it unless we show it's there. Signed-off-by: Anders Eknert <[email protected]> commit 1afc295c6b246eecd383a02afb6c79a118bd7ffd Author: Justin Lindh <[email protected]> Date: Mon Jan 10 12:50:27 2022 -0700 ast: add graph.reachable_paths Signed-off-by: Justin Lindh <[email protected]> commit 8b33bca Author: Kristian Svalland <[email protected]> Date: Mon Jan 10 20:04:30 2022 +0100 topdown: Use `json.Valid` instead of `util.UnmarshalJSON` to avoid unnecessary allocations. (open-policy-agent#4203) Signed-off-by: Kristian Svalland <[email protected]> commit 4985e4b Author: Anders Eknert <[email protected]> Date: Mon Jan 10 10:39:55 2022 +0100 docs: Consistent indentation (open-policy-agent#4201) Must have missed these last iteration.. Signed-off-by: Anders Eknert <[email protected]> commit 75ba6bf Author: Corey Hinkle <[email protected]> Date: Fri Jan 7 15:56:13 2022 -0500 Add detail-tab for collapsable markdown (open-policy-agent#4199) Default markdown renderer does not allow for unsafe combinations. Shortcode provided to wrap markdown that may contain URLs as opposed to allowing unsafe rendering. Signed-off-by: Corey Hinkle <[email protected]> commit 3cf8839 Author: Dan Oliver <[email protected]> Date: Fri Jan 7 12:52:37 2022 +0000 docs/management-bundles: add hint that S3 regional endpoint should be used (open-policy-agent#4196) Global endpoints lead to 307 responses until they're fully provisioned; that in turn causes the Authorization header to not be forwarded, and the GET request thus fails. Signed-off-by: Dan Oliver <[email protected]> commit 829086a Author: Anders Eknert <[email protected]> Date: Fri Jan 7 13:18:09 2022 +0100 Ensure http.send caching works in system.authz (open-policy-agent#4195) Fixes open-policy-agent#3946 Signed-off-by: Anders Eknert <[email protected]> commit cf37313 Author: Anders Eknert <[email protected]> Date: Fri Jan 7 09:50:24 2022 +0100 opa eval: add description to all formats (open-policy-agent#4191) Add description for `--format=source` and `--format=raw` to `opa eval -h` output. Signed-off-by: Anders Eknert <[email protected]> commit 61c0c46 Author: Peter ONeill <[email protected]> Date: Fri Jan 7 08:29:58 2022 +0200 docs/ssh-and-sudo-authorization: Add Missing Filename (open-policy-agent#4192) Signed-off-by: Peter ONeill <[email protected]> commit b3ef19e Author: Matt Mahnke <[email protected]> Date: Thu Jan 6 15:03:31 2022 -0600 docs: fix typo for tls-cert-refresh-period (open-policy-agent#4190) Signed-off-by: Matt Mahnke <[email protected]> commit 52ddfd9 Author: Shuhei Kitagawa <[email protected]> Date: Thu Jan 6 17:58:27 2022 +0900 topdown: Support indexof_n built-in function (open-policy-agent#4172) Fixes open-policy-agent#4155 Signed-off-by: shuheiktgw <[email protected]> commit 449fdfe Author: José Carlos Chávez <[email protected]> Date: Thu Jan 6 09:13:57 2022 +0100 chore: improves auth plugin resolution. (open-policy-agent#4175) * chore: improves auth plugin resolution. Currently when aiming to use a Plugin in credentials section, if the plugin is known then it will be resolved, if it isn't, it will be passed to the supported credentials and tried to be cast as HTTPAuthPlugin which ends up in a casting issue without further feedback on what was the plugin string. Signed-off-by: José Carlos Chávez <[email protected]> commit 16f85a4 Author: Stephan Renatus <[email protected]> Date: Thu Jan 6 08:49:39 2022 +0100 website: update redirects (open-policy-agent#4103) The "Option 5" redirect never worked, the fragment (i.e. behind the #) never reaches the server, it's a client-side thing. Adds a redirect for the renamed contrib section. Signed-off-by: Stephan Renatus <[email protected]> commit c0a692d Author: Vlad Iovanov <[email protected]> Date: Thu Jan 6 08:42:22 2022 +0200 logging: Remove logger GetFields function (open-policy-agent#4116) This removes the GetFields function from the logger interface, as mentioned in open-policy-agent#4114. GetFields used to be called in one place, creating a new logger using fields from an http client afaict. I am not sure if my changes have the desired effect in that case, or how this was desired to work - since the fields of the client are always changing when making requests. Fixes open-policy-agent#4114. Signed-off-by: viovanov <[email protected]> commit ca6259c Author: Anders Eknert <[email protected]> Date: Wed Jan 5 18:54:55 2022 +0100 docs: Fix integration policy (open-policy-agent#4185) `some id` left after policy cleanup caused the Rego compiler to rightfully protest when I tried integrating OPA as a library today. Signed-off-by: Anders Eknert <[email protected]> commit 78f0ae2 Author: rvalkenaers <[email protected]> Date: Wed Jan 5 16:57:59 2022 +0100 docs: fix configuration example (open-policy-agent#4184) Signed-off-by: rvalkenaers <[email protected]> commit f22e9cc Author: Anders Eknert <[email protected]> Date: Wed Jan 5 07:51:08 2022 +0100 Apply credentials masking on opa.runtime().config (open-policy-agent#4165) In order to prevent sensitive data to accidentally leak out into policies, reuse masking logic previously serving the /v1/config endpoint. Fixes open-policy-agent#4159 Signed-off-by: Anders Eknert <[email protected]> commit 50dc871 Author: Shuhei Kitagawa <[email protected]> Date: Wed Jan 5 15:34:16 2022 +0900 topdown: Improve the builtin indexof function performance (open-policy-agent#4169) Signed-off-by: shuheiktgw <[email protected]> commit d3fbd53 Author: Anders Eknert <[email protected]> Date: Tue Jan 4 19:22:53 2022 +0100 Build darwin/arm64 in post tag workflow (open-policy-agent#4182) Signed-off-by: Anders Eknert <[email protected]> commit 67def9b Author: Anders Eknert <[email protected]> Date: Tue Jan 4 18:35:32 2022 +0100 Prepare v0.37.0 development (open-policy-agent#4180) Signed-off-by: Anders Eknert <[email protected]> commit c2b2c62 Author: Anders Eknert <[email protected]> Date: Tue Jan 4 16:49:24 2022 +0100 Prepare v0.36.0 release (open-policy-agent#4178) Signed-off-by: Anders Eknert <[email protected]> commit 0ddf1db Author: Anders Eknert <[email protected]> Date: Thu Dec 30 20:25:32 2021 +0100 Add Open Service Mesh to ecosystem (open-policy-agent#4171) Also: * Add some links to Kubernetes authorization item * Add SPIFFE/SPIRE blog * Extend Rego tests to verify added/modified YAML files as valid The last point was intended to be for the integrations.yaml file only, but thinking more about it made sense not to limit the check to a single file. Signed-off-by: Anders Eknert <[email protected]> commit 06664d0 Author: Shuhei Kitagawa <[email protected]> Date: Tue Dec 28 16:43:28 2021 +0900 ci: Update golangci-lint to v1.43.0 (open-policy-agent#4173) Signed-off-by: shuheiktgw <[email protected]> commit 31422b4 Author: wasm-updater <[email protected]> Date: Mon Dec 27 11:49:38 2021 +0000 wasm: Update generated binaries commit 6f81c4a Author: Kristian Svalland <[email protected]> Date: Mon Dec 27 12:47:39 2021 +0100 Add `array.reverse(array)` and `strings.reverse(string)` built-in functions. (open-policy-agent#4161) The function `array.reverse` takes an array as an argument, and returns an array with a reversed order of elements. The function `strings.reverse` takes a string as an argument, and returns a string with a reversed order of unicode code points. WASM support is included for both built-ins. Fixes open-policy-agent#3736 Signed-off-by: Kristian Svalland <[email protected]> commit 328ffcd Author: yilinzeng <[email protected]> Date: Fri Dec 24 23:33:23 2021 +0800 docs/website add blog links for apisix blog (open-policy-agent#4168) Signed-off-by: yilin <[email protected]>
wasm: Add support for WASM and simple tests.
internal: Add opa_json_is_valid to map of wasm built-ins.
docs: Indicate that WASM support is now available for json.is_valid.
This code could also benefit from an allocation-less implementation as the change in #4203 was motivated by, but since that is not readily available I opted for this low-hanging version which I still think improves the codebase, and that can be changed to an allocation-less implementation at a later time by someone with capacity and know-how.
Fixes #4140