Matchers: Add dynamic metadata to the http inputs #34891

vikaschoudhary16 · 2024-06-25T08:00:40Z

fixes: #34092
Commit Message: Add dynamic metadata to the http inputs
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Vikas Choudhary <[email protected]>

repokitteh-read-only · 2024-06-25T08:00:50Z

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

🐱

Caused by: #34891 was opened by vikaschoudhary16.

see: more, trace.

Signed-off-by: Vikas Choudhary <[email protected]>

zetaab · 2024-07-26T07:30:43Z

@vikaschoudhary16 what is still needed to get this forward from draft status?

Signed-off-by: Vikas Choudhary <[email protected]>

vikaschoudhary16 · 2024-07-29T09:56:35Z

@vikaschoudhary16 what is still needed to get this forward from draft status?

tests were pending. Now marked ready

mattklein123

API LGTM. Needs a release note. add @kyessenov to do the code review.

/wait

Signed-off-by: Vikas Choudhary <[email protected]>

kyessenov

Looks pretty good -> mostly minor comments.

kyessenov · 2024-07-30T20:55:59Z

docs/root/intro/arch_overview/advanced/matching/matching_api.rst

@@ -30,6 +30,7 @@ These input functions are available for matching HTTP requests:
 * :ref:`Response header value <extension_envoy.matching.inputs.response_headers>`.
 * :ref:`Response trailer value <extension_envoy.matching.inputs.response_trailers>`.
 * :ref:`Query parameters value <extension_envoy.matching.inputs.query_params>`.
+* :ref:`Metadata <extension_envoy.matching.inputs.dynamic_metadata>`.


nit: Dynamic metadata

kyessenov · 2024-07-30T20:57:39Z

api/envoy/extensions/matching/common_inputs/network/v3/network_inputs.proto

@@ -103,3 +103,48 @@ message ApplicationProtocolInput {
 message FilterStateInput {
  string key = 1 [(validate.rules).string = {min_len: 1}];
 }
+


Should this be placed into network inputs? Are you planning to add network input support as well?

I was not sure. Looked at existing filter state which is defined here in network input and being used for both http and network. So thought similar we could do with metadata.
Yeah, I would add network input support as well in follow-up

kyessenov · 2024-07-30T21:01:05Z

source/extensions/matching/http/metadata_input/BUILD

+    name = "metadata_input_lib",
+    srcs = ["meta_input.cc"],
+    hdrs = ["meta_input.h"],
+    extra_visibility = [


delete extra visibility, also why skip on windows?

kyessenov · 2024-07-30T21:05:00Z

source/extensions/matching/http/metadata_input/meta_input.h

+class MetadataMatchData : public ::Envoy::Matcher::CustomMatchData {
+public:
+  explicit MetadataMatchData(const ProtobufWkt::Value& value) : value_(value) {}
+  const ProtobufWkt::Value& value_;


note to myself: this is a dangerous capture by reference. I see CEL does the same thing, but we probably need to audit for potential dangling references.

kyessenov · 2024-07-30T21:05:33Z

source/extensions/matching/http/metadata_input/meta_input.h

+public:
+  DynamicMetadataInput(
+      const envoy::extensions::matching::common_inputs::network::v3::DynamicMetadataInput&
+          inputConfig)


style: inputConfig -> input_config

kyessenov · 2024-07-30T21:11:31Z

source/extensions/matching/input_matchers/metadata/config.cc

+  const auto& matcher_config = MessageUtil::downcastAndValidate<
+      const envoy::extensions::matching::input_matchers::metadata::v3::Metadata&>(
+      config, factory_context.messageValidationVisitor());
+  const auto& v = matcher_config.value();


style: v -> value

kyessenov · 2024-07-30T21:12:15Z

source/extensions/matching/input_matchers/metadata/config.cc

+      const envoy::extensions::matching::input_matchers::metadata::v3::Metadata&>(
+      config, factory_context.messageValidationVisitor());
+  const auto& v = matcher_config.value();
+  auto value_matcher = Envoy::Matchers::ValueMatcher::create(v, factory_context);


these two should be const

kyessenov · 2024-07-30T21:13:08Z

source/extensions/matching/input_matchers/metadata/matcher.cc

+    : value_matcher_(value_matcher), invert_(invert) {}
+
+bool Matcher::match(const Envoy::Matcher::MatchingDataType& input) {
+  if (absl::holds_alternative<absl::monostate>(input)) {


Is this needed with get_if?

kyessenov · 2024-07-30T21:15:15Z

source/extensions/matching/input_matchers/metadata/matcher.cc

+  if (auto* ptr = absl::get_if<std::shared_ptr<::Envoy::Matcher::CustomMatchData>>(&input);
+      ptr != nullptr) {
+    Matching::Http::MetadataInput::MetadataMatchData* match_data =
+        dynamic_cast<Matching::Http::MetadataInput::MetadataMatchData*>((*ptr).get());


(*ptr).get() is odd -> doesn't ptr work?

with only ptr, I get

error: 'std::shared_ptrEnvoy::Matcher::CustomMatchData' is not polymorphic

Changed to ptr->get() though

kyessenov · 2024-07-30T21:15:51Z

test/extensions/matching/input_matchers/dyn_meta_matcher/BUILD

@@ -0,0 +1,34 @@
+load(


Test directory should match the source: dyn_meta_matcher -> metadata

Signed-off-by: Vikas Choudhary <[email protected]>

mattklein123 · 2024-07-31T18:23:46Z

/api lgtm

Signed-off-by: Vikas Choudhary <[email protected]>

fixes: envoyproxy#34092 --------- Signed-off-by: Vikas Choudhary <[email protected]> Signed-off-by: Martin Duke <[email protected]>

fixes: envoyproxy#34092 --------- Signed-off-by: Vikas Choudhary <[email protected]> Signed-off-by: asingh-g <[email protected]>

Matchers: Add dynamic metadata to the http inputs

fff388d

Signed-off-by: Vikas Choudhary <[email protected]>

vikaschoudhary16 requested review from kyessenov, mattklein123 and yanavlasov as code owners June 25, 2024 08:00

repokitteh-read-only bot added the api label Jun 25, 2024

repokitteh-read-only bot assigned mattklein123 Jun 25, 2024

vikaschoudhary16 marked this pull request as draft June 25, 2024 08:31

doc update

995b21b

Signed-off-by: Vikas Choudhary <[email protected]>

vikaschoudhary16 force-pushed the metadata-matcher branch from 9dfab6e to 995b21b Compare June 25, 2024 08:36

vikaschoudhary16 added 6 commits June 26, 2024 16:52

Add matcher

f905baa

Signed-off-by: Vikas Choudhary <[email protected]>

update

a913944

Signed-off-by: Vikas Choudhary <[email protected]>

update

5268e5b

Signed-off-by: Vikas Choudhary <[email protected]>

format

23970f9

Signed-off-by: Vikas Choudhary <[email protected]>

format

bbede38

Signed-off-by: Vikas Choudhary <[email protected]>

format

7d03b8b

Signed-off-by: Vikas Choudhary <[email protected]>

vikaschoudhary16 force-pushed the metadata-matcher branch from 07b7fa3 to 7d03b8b Compare July 4, 2024 02:38

vikaschoudhary16 added 2 commits July 29, 2024 11:25

Add tests

4a7e77f

Signed-off-by: Vikas Choudhary <[email protected]>

test

225c35b

Signed-off-by: Vikas Choudhary <[email protected]>

vikaschoudhary16 marked this pull request as ready for review July 29, 2024 09:39

mattklein123 requested changes Jul 29, 2024

View reviewed changes

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

mattklein123 assigned kyessenov Jul 29, 2024

release notes

24f3b08

Signed-off-by: Vikas Choudhary <[email protected]>

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

Merge branch 'main' into metadata-matcher

ce9bcf6

Signed-off-by: Vikas Choudhary <[email protected]>

vikaschoudhary16 requested a review from mattklein123 July 30, 2024 02:59

ci

f0a4328

Signed-off-by: Vikas Choudhary <[email protected]>

kyessenov requested changes Jul 30, 2024

View reviewed changes

Address review comments

4b15252

Signed-off-by: Vikas Choudhary <[email protected]>

vikaschoudhary16 requested a review from kyessenov July 31, 2024 06:04

vikaschoudhary16 added 3 commits July 31, 2024 12:51

format

c6d1254

Signed-off-by: Vikas Choudhary <[email protected]>

ci

4b8e813

Signed-off-by: Vikas Choudhary <[email protected]>

Merge branch 'main' into metadata-matcher

a095394

Signed-off-by: Vikas Choudhary <[email protected]>

mattklein123 removed their assignment Jul 31, 2024

mattklein123 previously approved these changes Jul 31, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Jul 31, 2024

kyessenov previously approved these changes Jul 31, 2024

View reviewed changes

vikaschoudhary16 added 4 commits August 1, 2024 06:02

empty to kick ci

132f67a

Signed-off-by: Vikas Choudhary <[email protected]>

empty

f916c95

Signed-off-by: Vikas Choudhary <[email protected]>

Merge branch 'main' into metadata-matcher

6df0712

Signed-off-by: Vikas Choudhary <[email protected]>

Merge branch 'main' into metadata-matcher

1c827ba

Signed-off-by: Vikas Choudhary <[email protected]>

vikaschoudhary16 dismissed stale reviews from kyessenov and mattklein123 via 1c827ba August 1, 2024 15:43

repokitteh-read-only bot added the api label Aug 1, 2024

kyessenov approved these changes Aug 2, 2024

View reviewed changes

yanavlasov approved these changes Aug 5, 2024

View reviewed changes

yanavlasov merged commit 520d88e into envoyproxy:main Aug 5, 2024
50 of 51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Matchers: Add dynamic metadata to the http inputs #34891

Matchers: Add dynamic metadata to the http inputs #34891

vikaschoudhary16 commented Jun 25, 2024

repokitteh-read-only bot commented Jun 25, 2024

zetaab commented Jul 26, 2024

vikaschoudhary16 commented Jul 29, 2024

mattklein123 left a comment

kyessenov left a comment

kyessenov Jul 30, 2024

kyessenov Jul 30, 2024

vikaschoudhary16 Jul 31, 2024

kyessenov Jul 30, 2024

kyessenov Jul 30, 2024

kyessenov Jul 30, 2024

kyessenov Jul 30, 2024

kyessenov Jul 30, 2024

kyessenov Jul 30, 2024

kyessenov Jul 30, 2024

vikaschoudhary16 Jul 31, 2024

kyessenov Jul 30, 2024

mattklein123 commented Jul 31, 2024

Matchers: Add dynamic metadata to the http inputs #34891

Matchers: Add dynamic metadata to the http inputs #34891

Conversation

vikaschoudhary16 commented Jun 25, 2024

repokitteh-read-only bot commented Jun 25, 2024

zetaab commented Jul 26, 2024

vikaschoudhary16 commented Jul 29, 2024

mattklein123 left a comment

Choose a reason for hiding this comment

kyessenov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Jul 31, 2024