ext_proc: send and receive dynamic metadata #30747

jbohanon · 2023-11-06T13:40:02Z

extract metadata changes from #29069

Commit Message:
Introduce the ability to send dynamic metadata in the External Processing Request. Also implements the API for returning dynamic metadata as part of the External Processing Response.
Additional Description:
Risk Level: Low
Testing:

Unit/Integration tests for processing request/response dynamic metadata
Release Notes: N/A
Platform Specific Features: N/A
Fixes Dynamic Metadata for External Processing Filters Similar to Dynamic Metadata for External Authorization #19881

repokitteh-read-only · 2023-11-06T13:40:14Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #30747 was opened by jbohanon.

see: more, trace.

api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto

api/envoy/service/ext_proc/v3/external_processor.proto

jbohanon · 2023-11-06T19:47:28Z

@markdroth you had previously approved the API changes on the now-closed PR (#29069) from which this work was extracted. What do you think about the API as-is and the comments above? I'm happy to change the API naming if it's deemed necessary, but as we have some internal references and some customers using this early-access provisional API, it'd be preferred to keep the naming if the benefit is marginal.

abeyad · 2023-11-09T16:21:57Z

@markdroth you had previously approved the API changes on the now-closed PR (#29069) from which this work was extracted. What do you think about the API as-is and the comments above? I'm happy to change the API naming if it's deemed necessary, but as we have some internal references and some customers using this early-access provisional API, it'd be preferred to keep the naming if the benefit is marginal.

I chatted w/ Mark and he clarified to me that "dynamic" in this case is redundant since the term metadata in xDS is used specifically for dynamic metadata, so please ignore those changes. But as a general rule, the API should be amenable to fixes and "getting it right" before being relied upon, and changes made up front are far less costly than mistakes down the road. But I understand you got approval from Mark before on this API change so your case is a bit different.

Also, if you can please address the comments around adding a . to the end of the sentence comments.

Happy to approve this PR after that's done, thanks for your effort on this!

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

abeyad · 2023-11-09T16:39:17Z

Note: please avoid force pushing commits. According to the contributing guide:

Once your PR is under review, please do not rebase it. If you rebase, you will need to force push to github, 
and github's user interface will force your reviewer to review the PR from scratch rather than simply look 
at your latest changes. It's much easier to review new commits and/or merges. We squash rebase the final 
merged commit so the number of commits you have in the PR don't matter. Again once your PR is assigned 
a reviewer, unless you need to fix DCO please do not force push.

If you need to resolve merge conflicts:

branch=$(git status|head -1|cut -f3 -d\ )
git checkout main
git pull
git checkout "$branch"
git merge main

abeyad

LGTM /api

abeyad · 2023-11-09T16:56:25Z

Adding @tyxia as a code owner

tyxia

Thanks for working on this!

Overall approach make senses to me. Left some comments after first pass.

tyxia · 2023-11-14T01:57:09Z

source/extensions/filters/http/ext_proc/processor_state.h

@@ -73,10 +73,19 @@ class ProcessorState : public Logger::Loggable<Logger::Id::ext_proc> {
  };

  explicit ProcessorState(Filter& filter,
-                          envoy::config::core::v3::TrafficDirection traffic_direction)
+                          envoy::config::core::v3::TrafficDirection traffic_direction,
+                          std::vector<std::string> untyped_forwarding_namespaces,


We should avoid the copy here. Please pass argument by reference or use absl::Span (https://abseil.io/tips/93).

Please also fix other places in this PR

source/extensions/filters/http/ext_proc/processor_state.h

tyxia · 2023-11-14T02:15:53Z

api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto

+
+  // Describes which typed or untyped dynamic metadata namespaces to accept from
+  // the external processing server. Set to empty or leave unset to disallow writing
+  // any received dynamic metadata. Receiving of typed metadata is not supported.


Re: receiving of typed metadata is not supported: Looks like it is because that only google.protobuf.Struct is defined in ProcessingResponse? Actually I am not very sure why protobuf.Any is not defined. Though this could be out of scope of this PR.

cc @gbrail

source/extensions/filters/http/ext_proc/ext_proc.cc

source/extensions/filters/http/ext_proc/ext_proc.h

source/extensions/filters/http/ext_proc/ext_proc.cc

tyxia · 2023-11-14T04:16:32Z

test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc

@@ -79,7 +79,7 @@ class ExtProcIntegrationTest : public HttpIntegrationTest,
    scoped_runtime_.mergeValues(
        {{"envoy.reloadable_features.send_header_raw_value", header_raw_value_}});
    scoped_runtime_.mergeValues(
-        {{"envoy_reloadable_features_immediate_response_use_filter_mutation_rule",
+        {{"envoy.reloadable_features.immediate_response_use_filter_mutation_rule",


Thanks for fixing this!

Could you please also add integration test cases for send/receive dynamic metadata that is introduced in this PR?

@tyxia I have added integration testing for this feature.

adisuissa · 2023-11-14T15:01:35Z

@yanjunxiang-google can you PTAL?
Also assigning Yan as ext_proc code owner.
/assign @yanavlasov

yanavlasov · 2023-11-14T15:34:11Z

Waiting for comments to be addressed.

/wait

- pass vectors by reference instead of copying - cleaner check for receiving namespaces - logging when returned metadata doesn't match any configured namespaces - pass response by reference instead of smart pointer Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

jbohanon · 2024-01-25T02:28:53Z

/retest

edit: attempted to rerun failed Azure Pipelines job. This did not have any effect.

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

jbohanon · 2024-01-25T10:53:15Z

/retest

edit: attempted to rerun failed GitHub Actions job. This did not have any effect.

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

jbohanon · 2024-01-25T14:24:34Z

@yanavlasov this is ready now, PTAL, thanks

@tyxia some changes were made since you reviewed, namely the responsibility for the merging of more-specific namespaces lists into less-specific namespaces lists has been offloaded to the control-plane. This simplified the API and merge logic. Notably, I was still getting junk memory values if I did not store the new merged vectors for namespaces, presumably due to copying into absl::optional, so these vectors are stored on the Filter object to extend lifetime.

tyxia · 2024-01-27T02:26:33Z

source/extensions/filters/http/ext_proc/ext_proc.cc

+      untyped_forwarding_namespaces_(more_specific.untypedForwardingMetadataNamespaces()),
+      typed_forwarding_namespaces_(more_specific.typedForwardingMetadataNamespaces()),
+      untyped_receiving_namespaces_(more_specific.untypedReceivingMetadataNamespaces()) {}


@jbohanon I agree with getting rid of merge proto. That probably has unnecessary complexity that we don' need for now.

However, I am not sure that we should let control plane do the merge . At least it seems to be inconsistent with existing ext_proc behavior. e.g., mergeProcessingMode, grpc_service_ above. Can we just have similar simple logic : if most_specific has namespaces, use that; if most_specific doesn't , use less_specific.

This is probably not critical thing to this PR and I am ok with discussing it in a separate PR. Small PR which can help review and move things faster is preferred and appreciated .

However, I am not sure that we should let control plane do the merge . At least it seems to be inconsistent with existing ext_proc behavior. e.g., mergeProcessingMode, grpc_service_ above. Can we just have similar simple logic : if most_specific has namespaces, use that; if most_specific doesn't , use less_specific.

I might have worded this poorly. This is currently the behavior. What was meant by letting the control plane handle the merge is that the if a control plane implementation has abstractions over Envoy APIs, then during the translation down to Envoy API, it is the responsibility of the control plane to ensure that the most specific config defined is the desired state of the metadata namespaces config/overrides

SGTM, Thanks

After discussing with @tyxia I need to revisit the merge logic here to make sure nonexistent more-specific config doesn't blow out existing less-specific config

/wait

yanavlasov

LGTM from me. Waiting for @tyxia final review.

/wait-any

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

jbohanon · 2024-01-30T14:21:01Z

/retest

Introduce the ability to send dynamic metadata in the External Processing Request. Also implements the API for returning dynamic metadata as part of the External Processing Response. --------- Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

* ext_proc: send attributes (envoyproxy#31090) Introduce the ability to send attributes in the External Processing Request --------- Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io> * ext_proc: send and receive dynamic metadata (envoyproxy#30747) Introduce the ability to send dynamic metadata in the External Processing Request. Also implements the API for returning dynamic metadata as part of the External Processing Response. --------- Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io> * ext_proc: revise service api for attributes (envoyproxy#32176) --------- Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io> --------- Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

jbohanon requested review from mattklein123, htuch and yanavlasov as code owners November 6, 2023 13:40

repokitteh-read-only bot added the api label Nov 6, 2023

repokitteh-read-only bot assigned abeyad Nov 6, 2023

jbohanon mentioned this pull request Nov 6, 2023

ext_proc: metadata and attributes #29069

Closed

abeyad reviewed Nov 6, 2023

View reviewed changes

abeyad assigned yanavlasov Nov 6, 2023

rshriram approved these changes Nov 6, 2023

View reviewed changes

jbohanon added 3 commits November 9, 2023 11:30

extract metadata changes from envoyproxy#29069

8685df9

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

add helloworld include to test BUILD

e0faad9

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

nits

cbfc6e7

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

jbohanon force-pushed the feature/ext_proc/dynamic-metadata branch from ca0d0eb to cbfc6e7 Compare November 9, 2023 16:34

abeyad previously approved these changes Nov 9, 2023

View reviewed changes

repokitteh-read-only bot removed the api label Nov 9, 2023

abeyad assigned tyxia and unassigned yanavlasov Nov 9, 2023

tyxia reviewed Nov 14, 2023

View reviewed changes

repokitteh-read-only bot assigned yanavlasov Nov 14, 2023

repokitteh-read-only bot added the waiting label Nov 14, 2023

PR feedback

d14c525

- pass vectors by reference instead of copying - cleaner check for receiving namespaces - logging when returned metadata doesn't match any configured namespaces - pass response by reference instead of smart pointer Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

jbohanon dismissed abeyad’s stale review via d14c525 November 16, 2023 19:41

repokitteh-read-only bot removed the waiting label Nov 16, 2023

jbohanon dismissed abeyad’s stale review via f93e168 January 24, 2024 20:01

jbohanon added 2 commits January 24, 2024 16:41

fix streamInfo() invocation

8785017

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

fix streamInfo() invocation

9516aaa

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

kick CI

b48aae8

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

kick CI

c8f8a54

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

tyxia reviewed Jan 27, 2024

View reviewed changes

yanavlasov previously approved these changes Jan 29, 2024

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Jan 29, 2024

tyxia approved these changes Jan 29, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 29, 2024

use less-specific namespaces if no more-specific exists

b159d7f

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

jbohanon dismissed yanavlasov’s stale review via b159d7f January 30, 2024 11:48

repokitteh-read-only bot removed the waiting label Jan 30, 2024

Merge branch 'main' into feature/ext_proc/dynamic-metadata

14d368d

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

repokitteh-read-only bot added the api label Jan 30, 2024

fix test name

019feca

Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>

jbohanon requested review from yanavlasov, abeyad and tyxia January 30, 2024 18:16

yanavlasov approved these changes Jan 30, 2024

View reviewed changes

yanavlasov merged commit 8f95f9e into envoyproxy:main Jan 30, 2024
53 of 54 checks passed

wbpcode mentioned this pull request Feb 5, 2024

ext_proc: revise service api for attributes #32176

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ext_proc: send and receive dynamic metadata #30747

ext_proc: send and receive dynamic metadata #30747

jbohanon commented Nov 6, 2023 •

edited

Loading

repokitteh-read-only bot commented Nov 6, 2023

jbohanon commented Nov 6, 2023

abeyad commented Nov 9, 2023

abeyad commented Nov 9, 2023 •

edited

Loading

abeyad left a comment

abeyad commented Nov 9, 2023

tyxia left a comment

tyxia Nov 14, 2023

tyxia Nov 14, 2023

tyxia Nov 14, 2023 •

edited

Loading

jbohanon Dec 19, 2023

adisuissa commented Nov 14, 2023

yanavlasov commented Nov 14, 2023

jbohanon commented Jan 25, 2024 •

edited

Loading

jbohanon commented Jan 25, 2024 •

edited

Loading

jbohanon commented Jan 25, 2024

tyxia Jan 27, 2024 •

edited

Loading

jbohanon Jan 29, 2024

tyxia Jan 29, 2024

jbohanon Jan 29, 2024

yanavlasov left a comment

jbohanon commented Jan 30, 2024

ext_proc: send and receive dynamic metadata #30747

ext_proc: send and receive dynamic metadata #30747

Conversation

jbohanon commented Nov 6, 2023 • edited Loading

repokitteh-read-only bot commented Nov 6, 2023

jbohanon commented Nov 6, 2023

abeyad commented Nov 9, 2023

abeyad commented Nov 9, 2023 • edited Loading

abeyad left a comment

Choose a reason for hiding this comment

abeyad commented Nov 9, 2023

tyxia left a comment

Choose a reason for hiding this comment

tyxia Nov 14, 2023

Choose a reason for hiding this comment

tyxia Nov 14, 2023

Choose a reason for hiding this comment

tyxia Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

jbohanon Dec 19, 2023

Choose a reason for hiding this comment

adisuissa commented Nov 14, 2023

yanavlasov commented Nov 14, 2023

jbohanon commented Jan 25, 2024 • edited Loading

jbohanon commented Jan 25, 2024 • edited Loading

jbohanon commented Jan 25, 2024

tyxia Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

jbohanon Jan 29, 2024

Choose a reason for hiding this comment

tyxia Jan 29, 2024

Choose a reason for hiding this comment

jbohanon Jan 29, 2024

Choose a reason for hiding this comment

yanavlasov left a comment

Choose a reason for hiding this comment

jbohanon commented Jan 30, 2024

jbohanon commented Nov 6, 2023 •

edited

Loading

abeyad commented Nov 9, 2023 •

edited

Loading

tyxia Nov 14, 2023 •

edited

Loading

jbohanon commented Jan 25, 2024 •

edited

Loading

jbohanon commented Jan 25, 2024 •

edited

Loading

tyxia Jan 27, 2024 •

edited

Loading