Matchers: Add dynamic metadata to the http inputs#34891
Matchers: Add dynamic metadata to the http inputs#34891yanavlasov merged 21 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>
9dfab6e to
995b21b
Compare
Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>
07b7fa3 to
7d03b8b
Compare
|
@vikaschoudhary16 what is still needed to get this forward from draft status? |
tests were pending. Now marked ready |
mattklein123
left a comment
There was a problem hiding this comment.
API LGTM. Needs a release note. add @kyessenov to do the code review.
/wait
Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>
Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>
kyessenov
left a comment
There was a problem hiding this comment.
Looks pretty good -> mostly minor comments.
| * :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>`. |
| message FilterStateInput { | ||
| string key = 1 [(validate.rules).string = {min_len: 1}]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Should this be placed into network inputs? Are you planning to add network input support as well?
There was a problem hiding this comment.
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
| name = "metadata_input_lib", | ||
| srcs = ["meta_input.cc"], | ||
| hdrs = ["meta_input.h"], | ||
| extra_visibility = [ |
There was a problem hiding this comment.
delete extra visibility, also why skip on windows?
| class MetadataMatchData : public ::Envoy::Matcher::CustomMatchData { | ||
| public: | ||
| explicit MetadataMatchData(const ProtobufWkt::Value& value) : value_(value) {} | ||
| const ProtobufWkt::Value& value_; |
There was a problem hiding this comment.
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.
| public: | ||
| DynamicMetadataInput( | ||
| const envoy::extensions::matching::common_inputs::network::v3::DynamicMetadataInput& | ||
| inputConfig) |
There was a problem hiding this comment.
style: inputConfig -> input_config
| 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(); |
| 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); |
There was a problem hiding this comment.
these two should be const
| : value_matcher_(value_matcher), invert_(invert) {} | ||
|
|
||
| bool Matcher::match(const Envoy::Matcher::MatchingDataType& input) { | ||
| if (absl::holds_alternative<absl::monostate>(input)) { |
There was a problem hiding this comment.
Is this needed with get_if?
| 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()); |
There was a problem hiding this comment.
(*ptr).get() is odd -> doesn't ptr work?
There was a problem hiding this comment.
with only ptr, I get
error: 'std::shared_ptrEnvoy::Matcher::CustomMatchData' is not polymorphic
Changed to ptr->get() though
| @@ -0,0 +1,34 @@ | |||
| load( | |||
There was a problem hiding this comment.
Test directory should match the source: dyn_meta_matcher -> metadata
Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>
|
/api lgtm |
Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>
Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>
Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>
1c827ba
fixes: envoyproxy#34092 --------- Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com> Signed-off-by: Martin Duke <martin.h.duke@gmail.com>
fixes: envoyproxy#34092 --------- Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com> Signed-off-by: asingh-g <abhisinghx@google.com>
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:]