Support ListValue for metadata matcher #3964

liminw · 2018-07-26T23:30:48Z

Description: This change added support for ListValue type in MetadataMatcher.
Risk Level: Low
Testing: Added unit tests.
Docs Changes: N/A.
Release Notes: N/A.

Signed-off-by: Limin Wang <[email protected]>

liminw · 2018-07-26T23:34:14Z

/cc @yangminzhu @mattklein123 @lizan

liminw · 2018-07-26T23:57:07Z

/cc @rodaine

yangminzhu · 2018-07-26T23:43:54Z

source/common/common/matchers.cc

@@ -78,6 +78,18 @@ bool MetadataMatcher::match(const envoy::api::v2::core::Metadata& metadata) cons
  case ProtobufWkt::Value::kBoolValue:
    return (bool_matcher_.has_value() && *bool_matcher_ == value.bool_value());
  case ProtobufWkt::Value::kListValue:
+    for (int i = 0; i < value.list_value().values_size(); ++i) {
+      const auto& lv = value.list_value().values(i);
+      if ((double_matcher_.has_value() && lv.kind_case() == ProtobufWkt::Value::kNumberValue &&


I think you should also check null_matcher_ unless you mention it in the doc that null_matcher_ is not supported in list.

Another Nit: as the matcher is stable so you should be able to decide the expected value and type from the matcher outside the for-loop, and during the for-loop just check if lv has the value and type. This should make it faster.

yangminzhu · 2018-07-26T23:46:48Z

api/envoy/type/matcher/metadata.proto

@@ -68,8 +68,6 @@ import "validate/validate.proto";
 // <envoy_api_msg_config.rbac.v2alpha.Principal>`.
 message MetadataMatcher {
  // Specifies the segment in a path to retrieve value from Metadata.
-  // Note: Currently it's not supported to retrieve a value from a list in Metadata. This means it


I think this comment should remain, we still don't support to retrieve a value from a list.

And the comment in line 80 should be updated that it now supports to compare a value to a list, but such comparison only supports primitive values in a list, embedded values in a list are still not supported.

@liminw can we revert the comment?

We already support list value. The current match method is one_of, as documented in API. I don't think we need the comments here. BTW, list embedded in another list is supported with the new change.

@liminw these comment are still valid for PathSegment, with this PR it will support matching to a list, doesn't mean we can specify a PathSegment that retrieve a value from a list.

I modified the origin comment to make it more accurate.

lizan · 2018-07-27T05:39:21Z

source/common/common/matchers.cc

@@ -78,6 +78,18 @@ bool MetadataMatcher::match(const envoy::api::v2::core::Metadata& metadata) cons
  case ProtobufWkt::Value::kBoolValue:
    return (bool_matcher_.has_value() && *bool_matcher_ == value.bool_value());
  case ProtobufWkt::Value::kListValue:
+    for (int i = 0; i < value.list_value().values_size(); ++i) {


I don't think we should automatically extend the value matchers to list (and treat it is matched when one of the list element matches to it), can we make a ListMatch in matcher/metadata.proto, and make this behavior explicitly from the API? In future we may want to do more than oneof match for lists.

lizan

Please make the oneof ListMatch explicit in API.

Signed-off-by: Limin Wang <[email protected]>

liminw · 2018-08-02T18:57:38Z

@lizan @yangminzhu @mattklein123 I refactored the matcher code. The comments you mentioned should be addressed. PTAL.

yangminzhu · 2018-08-02T19:18:16Z

source/common/common/matchers.cc

+  for (int i = 0; i < value.list_value().values_size(); ++i) {
+    const auto& lv = value.list_value().values(i);
+
+    if (oneof_value_matcher_ && oneof_value_matcher_->match(lv)) {


You can check oneof_value_matcher_ outside the loop.

Moved if (oneof_value_matcher) to outside the loop.

yangminzhu · 2018-08-02T19:21:20Z

api/envoy/type/matcher/metadata.proto

@@ -68,8 +68,6 @@ import "validate/validate.proto";
 // <envoy_api_msg_config.rbac.v2alpha.Principal>`.
 message MetadataMatcher {
  // Specifies the segment in a path to retrieve value from Metadata.
-  // Note: Currently it's not supported to retrieve a value from a list in Metadata. This means it


@liminw can we revert the comment?

yangminzhu · 2018-08-02T19:28:25Z

source/common/common/matchers.cc

+MatcherPtr ValueMatcher::create(const envoy::type::matcher::ValueMatcher& v) {
+  switch (v.match_pattern_case()) {
+  case envoy::type::matcher::ValueMatcher::kNullMatch:
+    return std::make_shared<const NullMatcher>();


Can we use unique_ptr here?

I feel using shared_ptr is safer because a ValueMatcher instance could be referred in multiple places.

yangminzhu · 2018-08-02T19:29:33Z

source/common/common/matchers.cc

  default:
    NOT_REACHED_GCOVR_EXCL_LINE;
  };
 }

-bool StringMatcher::match(const std::string& value) const {
+bool StringMatcher::match(const ProtobufWkt::Value& value) const {


Previously, the StringMatcher accepts a simple string so it could be used in anywhere with a string value, now it only can be only used with Value? is this expected?

Yes, match() is a virtual function defined in the parent class ValueMatcher. StringMatcher just implements the overrides here. If in the future, there is a need to use the primitive string as the parameter, we can always add that function.

Signed-off-by: Limin Wang <[email protected]>

lizan · 2018-08-02T22:17:02Z

test/common/common/matchers_test.cc

@@ -162,6 +162,145 @@ TEST(MetadataTest, MatchPresentValue) {
  EXPECT_FALSE(Envoy::Matchers::MetadataMatcher(matcher).match(metadata));
 }

+TEST(MetadataTest, MatchStringListValue) {
+  envoy::api::v2::core::Metadata metadata;


Can you make a utility function like this https://github.com/envoyproxy/envoy/blob/master/test/common/http/header_utility_test.cc#L17 and use it to construct those test fixtures from YAML? It is hard to read from the code with multiple mutable_*.

Added a utility function.

lizan · 2018-08-02T22:17:48Z

source/common/common/matchers.h

-   * @return true if it's matched otherwise false.
-   */
-  bool match(const std::string& value) const;
+  bool match(const ProtobufWkt::Value& value) const override;


not remove the comment?

I don't think the comments here is needed. It's overriding match() defined in parent class ValueMatcher. If we add the comment back, it would just repeats the one in ValueMatcher class.

lizan · 2018-08-02T22:19:29Z

source/common/common/matchers.h


 namespace Envoy {
 namespace Matchers {

-class DoubleMatcher {
+class ValueMatcher;
+typedef std::shared_ptr<const ValueMatcher> MatcherPtr;


You should name this ValueMatcherConstSharedPtr per style guide

Signed-off-by: Limin Wang <[email protected]>

liminw · 2018-08-03T18:46:41Z

@lizan @mattklein123 @yangminzhu All your comments are addressed. Let me know if you have further comments. Thanks.

lizan

seems the docs CI is failed, can you take a look?

lizan · 2018-08-03T09:44:00Z

source/common/common/matchers.cc

+  }
+
+  if (oneof_value_matcher_) {
+    for (int i = 0; i < value.list_value().values_size(); ++i) {


doesn't for (const auto& lv : value.list_value().values()) work?

Changed to what you suggested.

lizan · 2018-08-03T09:51:09Z

api/envoy/type/matcher/metadata.proto

@@ -68,8 +68,6 @@ import "validate/validate.proto";
 // <envoy_api_msg_config.rbac.v2alpha.Principal>`.
 message MetadataMatcher {
  // Specifies the segment in a path to retrieve value from Metadata.
-  // Note: Currently it's not supported to retrieve a value from a list in Metadata. This means it


@liminw these comment are still valid for PathSegment, with this PR it will support matching to a list, doesn't mean we can specify a PathSegment that retrieve a value from a list.

Signed-off-by: Limin Wang <[email protected]>

lizan

LGTM, thanks! one comment left for other reviewers.

lizan · 2018-08-03T22:43:03Z

api/envoy/type/matcher/value.proto

+
+// Specifies the way to match a ProtobufWkt::Value. Primitive values and ListValue are supported.
+// StructValue is not supported and is always not matched.
+message ValueMatcher {


Although the diff looks a bit scary, IIUC this refactoring out Value -> ValueMatcher shouldn't break backward compatibility of those protos, right?

Yeah from my eye LGTM, but would like if @htuch could double check.

mattklein123

LGTM, some small nits. Thank you!

mattklein123 · 2018-08-06T16:53:56Z

source/common/common/matchers.cc

+    return false;
+  }
+
+  double v = value.number_value();


mattklein123 · 2018-08-06T16:54:31Z

source/common/common/matchers.cc

  default:
    NOT_REACHED_GCOVR_EXCL_LINE;
  }
 }

+ListMatcher::ListMatcher(const envoy::type::matcher::ListMatcher& matcher) : matcher_(matcher) {
+  if (matcher_.match_pattern_case() != envoy::type::matcher::ListMatcher::kOneOf) {


nit: This should just be an ASSERT

mattklein123 · 2018-08-06T16:54:50Z

source/common/common/matchers.cc

+    NOT_REACHED_GCOVR_EXCL_LINE;
+  }
+
+  const auto& oneof = matcher_.one_of();


nit: local var not needed

mattklein123 · 2018-08-06T16:55:38Z

source/common/common/matchers.h

+
+class NullMatcher : public ValueMatcher {
+public:
+  NullMatcher() {}


nit: not needed

mattklein123 · 2018-08-06T16:55:51Z

source/common/common/matchers.h

+  bool match(const ProtobufWkt::Value& value) const override;
+
+private:
+  bool matcher_;


mattklein123 · 2018-08-06T16:55:59Z

source/common/common/matchers.h

+  bool match(const ProtobufWkt::Value& value) const override;
+
+private:
+  bool matcher_;


Signed-off-by: Limin Wang <[email protected]>

liminw · 2018-08-06T21:25:25Z

Thanks @mattklein123! All comments are addressed.

liminw added 3 commits July 26, 2018 16:06

Support ListValue for metadata matcher.

84787c5

Signed-off-by: Limin Wang <[email protected]>

Fix format.

00a5591

Signed-off-by: Limin Wang <[email protected]>

Update comments in PROTO.

0de164c

Signed-off-by: Limin Wang <[email protected]>

yangminzhu reviewed Jul 27, 2018

View reviewed changes

lizan reviewed Jul 27, 2018

View reviewed changes

lizan suggested changes Jul 27, 2018

View reviewed changes

mattklein123 assigned lizan Jul 30, 2018

snowp mentioned this pull request Jul 30, 2018

Smarter retry behavior/cross priority retries #3958

Closed

liminw added 2 commits August 2, 2018 11:44

Refactored matcher proto and implementation.

086dd78

Signed-off-by: Limin Wang <[email protected]>

Fix format.

e11b7fd

Signed-off-by: Limin Wang <[email protected]>

yangminzhu reviewed Aug 2, 2018

View reviewed changes

liminw added 2 commits August 2, 2018 14:59

Address comments.

9c97a5e

Signed-off-by: Limin Wang <[email protected]>

Fix format.

a2afa7b

Signed-off-by: Limin Wang <[email protected]>

lizan reviewed Aug 2, 2018

View reviewed changes

liminw added 2 commits August 2, 2018 15:58

Addressed comments.

1af1ea4

Signed-off-by: Limin Wang <[email protected]>

Fixed test.

e3898e4

Signed-off-by: Limin Wang <[email protected]>

lizan reviewed Aug 3, 2018

View reviewed changes

liminw added 2 commits August 3, 2018 15:21

Address comments and fixed docs circle/ci failure.

88346b2

Signed-off-by: Limin Wang <[email protected]>

Added comments to PathSegment.

42aa247

Signed-off-by: Limin Wang <[email protected]>

lizan previously approved these changes Aug 3, 2018

View reviewed changes

mattklein123 reviewed Aug 6, 2018

View reviewed changes

Addressed comments.

6eb90de

Signed-off-by: Limin Wang <[email protected]>

liminw dismissed lizan’s stale review via 6eb90de August 6, 2018 18:54

mattklein123 approved these changes Aug 6, 2018

View reviewed changes

mattklein123 merged commit 3868326 into envoyproxy:master Aug 7, 2018

liminw deleted the groups branch August 7, 2018 18:18

liminw mentioned this pull request Sep 17, 2018

Istio RBAC Improvements istio/istio#6984

Closed

8 tasks

Support ListValue for metadata matcher #3964

Support ListValue for metadata matcher #3964

Conversation

liminw commented Jul 26, 2018

liminw commented Jul 26, 2018

liminw commented Jul 26, 2018

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

lizan left a comment

Choose a reason for hiding this comment

liminw commented Aug 2, 2018

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

liminw commented Aug 3, 2018

lizan 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

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 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

liminw commented Aug 6, 2018