[Proposal] WIP: Add support for setDynamicdata (#15148) #15196

Gsantomaggio · 2021-02-25T20:31:43Z

Commit Message: Add support for setDynamicdata (#15148)
Additional Description: This is a proposal to solve #15148
The idea is to recognize the dynamic_metada ed apply them
The RUST code will be something like:

self.set_property(vec!["dynamicmetadata"],
                                  Some("myitem".as_ref()));

Second option:

We have also a second option to solve the issue by adding another method setDynamicdata
We already did it here the code:

envoy
proxy-wasm-cpp
rust sdk
The rust code is:

 self.set_dynamicdata(vec!["rabbitmq.filters.network"],
                                  Some("items".as_ref()));
            }

Please ignore the debug logs, and the code is not complete yet.
I am not an expert and before go headed I'd like to hear your opinion of which one can be the best solution or maybe none of them

Risk Level: Low
Testing: not yet
Release Notes: not yet

repokitteh-read-only · 2021-02-25T20:31:46Z

Hi @Gsantomaggio, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #15196 was opened by Gsantomaggio.

see: more, trace.

PiotrSikora

I think this option is preferable, but the same path should be used for both get and set operations.

I believe that you can already retrieve dynamic metadata using:

self.get_property(vec!["metadata", "filter_metadata", "envoy.filters.network.rbac", "something"]);

so the same path should be used for the set operation.

source/extensions/common/wasm/context.cc

mathetake · 2021-03-02T04:58:36Z

Agree with @PiotrSikora and we don't need to introduce new property tokens here, and this should be aligned with existing getProperty(["metadata",...]) call.

Gsantomaggio · 2021-03-03T09:19:18Z

@mathetake we will remove the property tokens.

What we are still trying to understand is how to distinguish metadata from dinamic_metadata.

The set and get methods seem a bit incongruent, since with the get you can retrieve the dynamic_metadata but there is no way to set.

We'd like to add this feature given the same interface Context::setProperty(absl::string_view path, absl::string_view value) but the question is:

How would you set the metadata and dynamic metadata using the wasm spec?

for example in rust SDK is something like:

self.set_property(vec!["metadata", "filter_state", .... ], Some("value".as_ref()));

How would be for dynamic metadata?

Thank you

PiotrSikora · 2021-03-05T03:33:59Z

@kyessenov could you help with this?

source/extensions/common/wasm/context.cc

kyessenov · 2021-03-09T20:49:14Z

source/extensions/common/wasm/context.cc

+    fields_a[v1].set_bool_value(true);
+    std::string key3;
+    absl::StrAppend(&key3, path);
+    stream_info->setDynamicMetadata(key3, metadata_val);


Unclear how key and value are derived.

That what just a test, the problem is how to pass ProtobufWkt::Struct metadata_val client side:

I was thinking something like:

ProtobufWkt::Struct metadata_val; std::string json_string; absl::StrAppend(&json_string, value); MessageUtil::loadFromJson(json_string, metadata_val); stream_info->setDynamicMetadata(key, metadata_val);

and client side send a JSON

JSON sounds fine, although it's parsing is pretty slow. Using dynamic metadata has inherent performance issues, so it might be ok with parsing. WDYT @PiotrSikora ?

Hi @kyessenov, thanks for the review! I'm working with Gabriele on this proposal. My understanding now is that we can expect value to always be a JSON string (assuming the parsing performance is acceptable) when path starts with metadata\0. Is my understanding correct?
In such case, should we return WasmResult::BadArgument if the string is not JSON-parsable?

We changed the PR, the idea is to parse the path, identify the metadata and set the dynamic metadata.
client side would be:

self.set_property(vec!["metadata","my.filters.network.network"], Some("{\"noaccess\": true}".as_ref()));

cc @kyessenov

Note that using JSON to set_property like the above for metadata does not match the intuition of getting back JSON from get_property with metadata - in particular, get_property can get you a protobuf struct when asking just for vec!["metadata"] or or a "pairs" length and 0-delimited key-value array without type information for values if you specify a path after "metadata" (at least for the use cases I've had).

I think there is some work to be done in this area to define what is expected in both directions.

Signed-off-by: Gabriele Santomaggio <g.santomaggio@gmail.com>

Gsantomaggio · 2021-03-17T09:20:54Z

The PR is ready for another round, we'd like to understand if the idea is valid before go ahead with tests etc..
Thank you for your time.

yaronp68 · 2021-03-19T10:11:38Z

The PR is ready for another round, we'd like to understand if the idea is valid before go ahead with tests etc..
Thank you for your time.

@PiotrSikora @kyessenov

NomadXD · 2021-03-29T08:13:53Z

@PiotrSikora @kyessenov Can we read dynamic metadata that is set from a previous filter ? I'm trying to access dynamic metadata that is set from ext_authz as protobuf::struct. I'm doing the following but it's not working.

google::protobuf::Struct metadata;
  if (!getMessageValue(
          {"metadata", "filter_metadata", "envoy.filters.http.ext_authz"}, &metadata)) {
    LOG_ERROR(std::string("filter_metadata Error ") + std::to_string(id()));
  }

It logs the error. When I trace the ext_authz response, I can see metadata there. So I guess I'm doing something wrong with reading it from my filter.

yaronp68 · 2021-03-29T12:48:55Z

@PiotrSikora this PR is waiting for quite sometime now. We need this capability. Please advise if anything missing to continue the review

wrowe · 2021-04-01T19:49:27Z

I understand @PiotrSikora may be buried in other efforts, would any other @envoyproxy/wasm-dev folks be available to comment on this proposal? There is additional work to be done around test dev, if the proposal is generally acceptable.

PiotrSikora

@Gsantomaggio sorry for the delay!

At the very least, you need to add tests with a round-trip set/get example to see how this could be used in practice.

Regarding JSON - we don't really use it across ABI boundary, and get_property returns serialized list of pairs (at least for the filter_state), so as @unleashed already pointed out, set_property should use the same format.

unleashed · 2021-04-01T23:51:07Z

@PiotrSikora IMO using pairs is quite the limitation, as we lose type information and the need to deal with ambiguities is pushed over to the wasm modules. For example, distinguishing between a value that could be both a string or an array of strings becomes an interesting problem, as some values could be valid interpreted as both types.

I think keeping this type information is important here, though I'd prefer it if it was kept using protobuf rather than JSON.

PiotrSikora · 2021-04-08T09:22:21Z

@unleashed I agree, but we need to be consistent between getters and setters. proxy_get_property used to return a protobuf, but it was unfortunately changed at some point, and now it returns a pairs type.

In general, proxy_{get,set}_property being an opaque pass-through to CEL is a mess, it's impossible to standardize, and it resulted in the aforementioned regression without any changes to the Proxy-Wasm ABI.

As for this issue, I think we should introduce envoy_{get,set}_dynamic_metadata and envoy_{get,set}_filter_state that take and return protobufs. They are not portable anyway, and should have never been a part of the core ABI.

If that sounds reasonable, then you can search for resolve_dns in this repo to see how Envoy-specific extensions can be implemented.

Gsantomaggio · 2021-04-08T09:31:54Z

thank you @PiotrSikora I was already working on something like that as a second option:

We have also a second option to solve the issue by adding another method setDynamicdata
We already did it here the code:

envoy
proxy-wasm-cpp
rust sdk
The rust code is:

 self.set_dynamicdata(vec!["rabbitmq.filters.network"],
                                  Some("items".as_ref()));
            }

PiotrSikora · 2021-04-08T09:42:24Z

@Gsantomaggio oh, that's right! Sorry for initially sending you on the wrong path.

As for those commits, since this is an Envoy specific extension, you should be able to implement everything in this repo, without touching Proxy-Wasm C++ Host. Please take a look at resolve_dns for a working example.

Also, you need to add getters, and ideally do the same for the filter_state.

github-actions · 2021-05-08T12:01:12Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2021-05-15T12:01:14Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

unleashed · 2021-05-20T06:46:07Z

Is there still interest in working on this? I don't know whether it's been dealt with elsewhere, but if not it looks to me like this is an important missing feature for these modules.

Gsantomaggio · 2021-05-20T07:06:06Z

@unleashed at the moment I can't work on it. Hope to restart working on it in the next months.

CalbeeMing0530 · 2021-09-15T09:03:03Z

thank you @PiotrSikora I was already working on something like that as a second option:

We have also a second option to solve the issue by adding another method setDynamicdata
We already did it here the code:

envoy

proxy-wasm-cpp

rust sdk
The rust code is:
 self.set_dynamicdata(vec!["rabbitmq.filters.network"],
                                  Some("items".as_ref()));
            }

Hi @Gsantomaggio , is there a specific rust implementation code for second option? Or is there a runnable git demo for reference, passing dynamicData between Envoy wasm filter and rbac filter is also a requirement for us, I think this proposal is very critical to implement dynamic authorization, we use proxy-wasm-cpp-sdk, but it seems that this feature is not yet integrated into the SDK. Looking forward to your reply!

Gsantomaggio · 2021-09-15T09:48:54Z

Hi @CalbeeMing0530

Or is there a runnable git demo for reference

Personally, I don't have anything.
Only the code you see here:

But it is a work In progress and needs to be changed.

cetanu · 2023-10-12T05:27:17Z

It seems there is still intention to perform this work and the issue was auto-closed. If it can be re-opened that would be good.

If someone with expertise can inform me as to what they think needs to get done next, I can try to progress this work.

As far as I understand there is a concern around how to set the data, and whether it should be a serialized format.
In addition, I gather that some tests need to be written. Are these assumptions accurate?

Gsantomaggio requested review from lizan and PiotrSikora as code owners February 25, 2021 20:31

Gsantomaggio force-pushed the add_dynamic_metadata branch from a5d1b6b to a7a88ac Compare February 25, 2021 20:48

PiotrSikora suggested changes Feb 26, 2021

View reviewed changes

source/extensions/common/wasm/context.cc Outdated Show resolved Hide resolved

zuercher assigned PiotrSikora Feb 26, 2021

PiotrSikora requested review from mathetake and kyessenov February 27, 2021 01:44

kyessenov reviewed Mar 9, 2021

View reviewed changes

source/extensions/common/wasm/context.cc Outdated Show resolved Hide resolved

kyessenov reviewed Mar 9, 2021

View reviewed changes

Add support for setDynamicdata (envoyproxy#15148)

ad1e313

Signed-off-by: Gabriele Santomaggio <g.santomaggio@gmail.com>

Gsantomaggio force-pushed the add_dynamic_metadata branch from a7a88ac to ad1e313 Compare March 10, 2021 16:29

PiotrSikora suggested changes Apr 1, 2021

View reviewed changes

wrowe added area/wasm enhancement Feature requests. Not bugs or questions. labels Apr 2, 2021

PiotrSikora mentioned this pull request Apr 8, 2021

Dynamic metadata of type google::protobuf::Struct not working proxy-wasm/proxy-wasm-cpp-sdk#98

Closed

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label May 8, 2021

github-actions bot closed this May 15, 2021

NomadXD mentioned this pull request May 22, 2021

Dev/header to metadata 3scale-labs/lahiru-workspace#2

Merged

PiotrSikora mentioned this pull request Mar 4, 2022

wasm: Add support for Dynamic Metadata #15148

Open

guydc mentioned this pull request Apr 24, 2024

api: add metadata and attribute options for ext-proc envoyproxy/gateway#3247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Proposal] WIP: Add support for setDynamicdata (#15148) #15196

[Proposal] WIP: Add support for setDynamicdata (#15148) #15196

Gsantomaggio commented Feb 25, 2021 •

edited

Loading

repokitteh-read-only bot commented Feb 25, 2021

PiotrSikora left a comment

mathetake commented Mar 2, 2021 •

edited

Loading

Gsantomaggio commented Mar 3, 2021

PiotrSikora commented Mar 5, 2021

kyessenov Mar 9, 2021

Gsantomaggio Mar 9, 2021

kyessenov Mar 9, 2021

Zerpet Mar 10, 2021

Gsantomaggio Mar 10, 2021

unleashed Mar 19, 2021

Gsantomaggio commented Mar 17, 2021

yaronp68 commented Mar 19, 2021

NomadXD commented Mar 29, 2021 •

edited

Loading

yaronp68 commented Mar 29, 2021

wrowe commented Apr 1, 2021

PiotrSikora left a comment

unleashed commented Apr 1, 2021

PiotrSikora commented Apr 8, 2021

Gsantomaggio commented Apr 8, 2021

PiotrSikora commented Apr 8, 2021

github-actions bot commented May 8, 2021

github-actions bot commented May 15, 2021

unleashed commented May 20, 2021

Gsantomaggio commented May 20, 2021

CalbeeMing0530 commented Sep 15, 2021

Gsantomaggio commented Sep 15, 2021

cetanu commented Oct 12, 2023

[Proposal] WIP: Add support for setDynamicdata (#15148) #15196

[Proposal] WIP: Add support for setDynamicdata (#15148) #15196

Conversation

Gsantomaggio commented Feb 25, 2021 • edited Loading

Second option:

repokitteh-read-only bot commented Feb 25, 2021

PiotrSikora left a comment

Choose a reason for hiding this comment

mathetake commented Mar 2, 2021 • edited Loading

Gsantomaggio commented Mar 3, 2021

PiotrSikora commented Mar 5, 2021

kyessenov Mar 9, 2021

Choose a reason for hiding this comment

Gsantomaggio Mar 9, 2021

Choose a reason for hiding this comment

kyessenov Mar 9, 2021

Choose a reason for hiding this comment

Zerpet Mar 10, 2021

Choose a reason for hiding this comment

Gsantomaggio Mar 10, 2021

Choose a reason for hiding this comment

unleashed Mar 19, 2021

Choose a reason for hiding this comment

Gsantomaggio commented Mar 17, 2021

yaronp68 commented Mar 19, 2021

NomadXD commented Mar 29, 2021 • edited Loading

yaronp68 commented Mar 29, 2021

wrowe commented Apr 1, 2021

PiotrSikora left a comment

Choose a reason for hiding this comment

unleashed commented Apr 1, 2021

PiotrSikora commented Apr 8, 2021

Gsantomaggio commented Apr 8, 2021

PiotrSikora commented Apr 8, 2021

github-actions bot commented May 8, 2021

github-actions bot commented May 15, 2021

unleashed commented May 20, 2021

Gsantomaggio commented May 20, 2021

CalbeeMing0530 commented Sep 15, 2021

Gsantomaggio commented Sep 15, 2021

cetanu commented Oct 12, 2023

Gsantomaggio commented Feb 25, 2021 •

edited

Loading

mathetake commented Mar 2, 2021 •

edited

Loading

NomadXD commented Mar 29, 2021 •

edited

Loading