Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
7a11210
Allow http route and cluster metadata to contain typed metadata in An…
yanjunxiang-google Apr 21, 2021
c997e15
Update PR based on comments
yanjunxiang-google Apr 30, 2021
6276b4d
update diff based on comments
yanjunxiang-google May 3, 2021
2ab8508
Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…
yanjunxiang-google May 3, 2021
82d73eb
Revert "Update PR based on comments"
yanjunxiang-google May 4, 2021
009ede2
update change based on comments-remove unnecessary changes generated …
yanjunxiang-google May 4, 2021
1673473
fix spelling error
yanjunxiang-google May 5, 2021
97986c0
Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…
yanjunxiang-google May 5, 2021
8936371
Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…
yanjunxiang-google May 5, 2021
6d62272
commit the generated v4alpha proto files
yanjunxiang-google May 6, 2021
b48a943
Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…
yanjunxiang-google May 6, 2021
91e31bc
adding back the two missed test file changes
yanjunxiang-google May 6, 2021
c5254db
Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…
yanjunxiang-google May 6, 2021
03500dc
adding change to deal with pre_submit check error flaged by gcc
yanjunxiang-google May 7, 2021
c21cb0b
merge upstream/main
yanjunxiang-google May 11, 2021
4042f5d
run fix_and_check scripts
yanjunxiang-google May 11, 2021
7b22234
Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…
yanjunxiang-google May 11, 2021
9d8d04f
Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…
yanjunxiang-google May 13, 2021
e59b4c4
Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…
yanjunxiang-google May 13, 2021
d2b51da
Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…
yanjunxiang-google May 14, 2021
4767834
address comments regarding base.proto document format
yanjunxiang-google May 17, 2021
33bb388
Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…
yanjunxiang-google May 17, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions api/envoy/config/core/v3/base.proto
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,19 @@ message Metadata {

// Key is the reverse DNS filter name, e.g. com.acme.widget. The envoy.*
// namespace is reserved for Envoy's built-in filters.
// If both *filter_metadata* and
// :ref:`typed_filter_metadata <envoy_v3_api_field_config.core.v3.Metadata.typed_filter_metadata>`
// fields are present in the metadata with same keys,
// only *typed_filter_metadata* field will be parsed.
map<string, google.protobuf.Struct> filter_metadata = 1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some comments here.
You might also want to update the comment for the Metadata message to describe the differences between the typed and non-typed filter metadata fields.
In addition, can the same key be in both filter_metadata and typed_filter_metadata. If so, what happens in this case?

BTW: if there should only be either filter_metadata or typed_filter_metadata but not both, then consider using oneof here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question here is shall we deprecate filter_metadata? When we changed filter configs from Struct to Any we deprecated Struct because Any can have Struct in it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's mark it as deprecated.

I think Oneof will make it harder to migrate.
For now we need both to have a smooth migration: let the metadata impl first look at typed_filter_metadata and fallback to typed_metadata when it's not populated. This way we can decouple dataplane with control plane.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah don't make it oneof, it breaks the compatibility on generated codes and violates our API policy.

Copy link
Copy Markdown
Contributor

@AdiJohn AdiJohn Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Oneof is infeasible here. For Oneof fields, "You can add fields of any type, except map fields and repeated fields." See https://developers.google.com/protocol-buffers/docs/proto3#oneof

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main present issue with transitioning to using Struct-inside-of-Any is that the Metadata utility class has static methods only. It will be inefficient to use with the Struct-inside-of-Any as there is no way to cache parsed Struct. The Metadata utility class is used heavily within Envoy and also by proprietary filters, so changing its API will be challenging.
One potential way forward is to make current implementation of the Metadata class inefficient with respect to Struct-inside-of-Any and mark it as deprecated to prevent further use. And then add a new class that can cache parsed Struct from Any.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think a TypedMetadata class would make sense.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments. Based on the discussion, I put Struct as deprecated, and added Any. Please take a look the latest code change and let me know whether this look good to you.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Struct field being deprecated, this pre_submit check scripts: " ci/run_envoy_docker.sh 'ci/do_ci.sh bazel.compile_time_options' ", is complaining below and failed the check constantly:

2021-05-07T23:34:58.2181735Z C++ exception with description "type envoy.config.core.v3.Metadata Using deprecated option 'envoy.config.core.v3.Metadata.filter_metadata' from file base.proto. This configuration will be removed from Envoy soon. Please see https://www.envoyproxy.io/docs/envoy/latest/version_history/version_history for details. If continued use of this field is absolutely necessary, see https://www.envoyproxy.io/docs/envoy/latest/configuration/operations/runtime#using-runtime-overrides-for-deprecated-features for how to apply a temporary and highly discouraged override." thrown in the test body.
2021-05-07T23:34:58.2184225Z [ FAILED ] OmitHostsRetryPredicateTest.PredicateTest (22 ms)

This happend on a lot of test cases, and one scripts run only report some of them. Once some test cases are failed as above, I just use DEPRECATED_FEATURE_TEST() to guard them. Another scripts run will report new ones. I have below questions:

  1. Should I continue this catch and patch style to mark all those test cases which exercising Metadata:Struct with DEPRECATED_FEATURE_TEST()?
  2. Is there a way to let the scripts run to the end and report all the test cases which exercising Metadata:Struct ? That way I can patch once for all.
  3. Is there a timing pressure to commit Any in the metadata proto? If there is, should we first commit the Any without flag Struct as deprecated? That way we deprecate Struct and fix all the text cases in a separate PR.

Please let me know your opinion.

Thanks!

Copy link
Copy Markdown
Contributor Author

@yanjunxiang-google yanjunxiang-google May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of test cases broken by deprecating Struct field. Fixing those test cases need time. Given the timing pressure for adding Any support in this protobuf, we decided to tackle Struct field deprecation and fixing the related test cases in a separate PR.

Please let me know whether this sound good to you.

// Key is the reverse DNS filter name, e.g. com.acme.widget. The envoy.*
// namespace is reserved for Envoy's built-in filters.
// The value is encoded as google.protobuf.Any.
// If both :ref:`filter_metadata <envoy_v3_api_field_config.core.v3.Metadata.filter_metadata>`
// and *typed_filter_metadata* fields are present in the metadata with same keys,
// only *typed_filter_metadata* field will be parsed.
map<string, google.protobuf.Any> typed_filter_metadata = 2;
}

// Runtime derived uint32 with a default when not specified.
Expand Down
12 changes: 12 additions & 0 deletions api/envoy/config/core/v4alpha/base.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions generated_api_shadow/envoy/config/core/v3/base.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions generated_api_shadow/envoy/config/core/v4alpha/base.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions include/envoy/config/typed_metadata.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,17 @@ class TypedMetadataFactory : public UntypedFactory {
virtual std::unique_ptr<const TypedMetadata::Object>
parse(const ProtobufWkt::Struct& data) const PURE;

/**
* Convert the google.protobuf.Any into an instance of TypedMetadata::Object.
* It should throw an EnvoyException in case the conversion can't be completed.
* @param data config data stored as a protobuf any.
* @return a derived class object pointer of TypedMetadata or return nullptr if
* one doesn't implement parse() method.
* @throw EnvoyException if the parsing can't be done.
*/
virtual std::unique_ptr<const TypedMetadata::Object>
parse(const ProtobufWkt::Any& data) const PURE;

std::string category() const override { return "envoy.typed_metadata"; }
};

Expand Down
12 changes: 12 additions & 0 deletions source/common/config/metadata.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,20 @@ template <typename factoryClass> class TypedMetadataImpl : public TypedMetadata
*/
void populateFrom(const envoy::config::core::v3::Metadata& metadata) {
auto& data_by_key = metadata.filter_metadata();
auto& typed_data_by_key = metadata.typed_filter_metadata();
for (const auto& [factory_name, factory] :
Registry::FactoryRegistry<factoryClass>::factories()) {
const auto& typed_meta_iter = typed_data_by_key.find(factory_name);
// If the key exists in Any metadata, and parse() does not return nullptr,
// populate data_.
if (typed_meta_iter != typed_data_by_key.end()) {
auto result = factory->parse(typed_meta_iter->second);
if (result != nullptr) {
data_[factory->name()] = std::move(result);
continue;
}
}
// Fall back cases to parsing Struct metadata and populate data_.
const auto& meta_iter = data_by_key.find(factory_name);
if (meta_iter != data_by_key.end()) {
data_[factory->name()] = factory->parse(meta_iter->second);
Expand Down
188 changes: 171 additions & 17 deletions test/common/config/metadata_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,76 +51,230 @@ TEST(MetadataTest, MetadataValuePath) {

class TypedMetadataTest : public testing::Test {
public:
TypedMetadataTest() : registered_factory_(foo_factory_) {}
TypedMetadataTest()
: registered_factory_foo_(foo_factory_), registered_factory_bar_(bar_factory_),
registered_factory_baz_(baz_factory_) {}

struct Foo : public TypedMetadata::Object {
Foo(std::string name) : name_(name) {}
std::string name_;
};

struct Bar : public TypedMetadata::Object {};
struct Qux : public TypedMetadata::Object {};

class FooFactory : public TypedMetadataFactory {
class FoobarFactory : public TypedMetadataFactory {
public:
std::string name() const override { return "foo"; }
// Throws EnvoyException (conversion failure) if d is empty.
std::unique_ptr<const TypedMetadata::Object>
parse(const ProtobufWkt::Struct& d) const override {
if (d.fields().find("name") != d.fields().end()) {
return std::make_unique<Foo>(d.fields().at("name").string_value());
}
throw EnvoyException("Cannot create a Foo when metadata is empty.");
throw EnvoyException("Cannot create a Foo when Struct metadata is empty.");
}

std::unique_ptr<const TypedMetadata::Object> parse(const ProtobufWkt::Any& d) const override {
if (!(d.type_url().empty())) {
return std::make_unique<Foo>(d.value());
}
throw EnvoyException("Cannot create a Foo when Any metadata is empty.");
}
};

// Three testing factories with different names
class FooFactory : public FoobarFactory {
public:
std::string name() const override { return "foo"; }
};

class BarFactory : public FoobarFactory {
public:
std::string name() const override { return "bar"; }
};

class BazFactory : public FoobarFactory {
public:
std::string name() const override { return "baz"; }
using FoobarFactory::parse;
// Override Any parse() to just return nullptr.
std::unique_ptr<const TypedMetadata::Object> parse(const ProtobufWkt::Any&) const override {
return nullptr;
}
};

protected:
FooFactory foo_factory_;
Registry::InjectFactory<TypedMetadataFactory> registered_factory_;
BarFactory bar_factory_;
BazFactory baz_factory_;
Registry::InjectFactory<TypedMetadataFactory> registered_factory_foo_;
Registry::InjectFactory<TypedMetadataFactory> registered_factory_bar_;
Registry::InjectFactory<TypedMetadataFactory> registered_factory_baz_;
};

TEST_F(TypedMetadataTest, OkTest) {
// Tests data parsing and retrieving when only Struct field present in the metadata.
TEST_F(TypedMetadataTest, OkTestStruct) {
envoy::config::core::v3::Metadata metadata;
(*metadata.mutable_filter_metadata())[foo_factory_.name()] =
MessageUtil::keyValueStruct("name", "foo");
MessageUtil::keyValueStruct("name", "garply");
TypedMetadataImpl<TypedMetadataFactory> typed(metadata);
EXPECT_NE(nullptr, typed.get<Foo>(foo_factory_.name()));
EXPECT_EQ("foo", typed.get<Foo>(foo_factory_.name())->name_);
EXPECT_EQ("garply", typed.get<Foo>(foo_factory_.name())->name_);
// A duck is a duck.
EXPECT_EQ(nullptr, typed.get<Bar>(foo_factory_.name()));
EXPECT_EQ(nullptr, typed.get<Qux>(foo_factory_.name()));
}

// Tests data parsing and retrieving when only Any field present in the metadata.
TEST_F(TypedMetadataTest, OkTestAny) {
envoy::config::core::v3::Metadata metadata;
ProtobufWkt::Any any;
any.set_type_url("type.googleapis.com/waldo");
any.set_value("fred");
(*metadata.mutable_typed_filter_metadata())[bar_factory_.name()] = any;
TypedMetadataImpl<TypedMetadataFactory> typed(metadata);
EXPECT_NE(nullptr, typed.get<Foo>(bar_factory_.name()));
EXPECT_EQ("fred", typed.get<Foo>(bar_factory_.name())->name_);
}

// Tests data parsing and retrieving when only Any field present in the metadata,
// also Any data parsing method just return nullptr.
TEST_F(TypedMetadataTest, OkTestAnyParseReturnNullptr) {
envoy::config::core::v3::Metadata metadata;
ProtobufWkt::Any any;
any.set_type_url("type.googleapis.com/waldo");
any.set_value("fred");
(*metadata.mutable_typed_filter_metadata())[baz_factory_.name()] = any;
TypedMetadataImpl<TypedMetadataFactory> typed(metadata);
EXPECT_EQ(nullptr, typed.get<Foo>(baz_factory_.name()));
}

// Tests data parsing and retrieving when both Struct and Any field
// present in the metadata, and in the same factory.
TEST_F(TypedMetadataTest, OkTestBothSameFactory) {
envoy::config::core::v3::Metadata metadata;
(*metadata.mutable_filter_metadata())[foo_factory_.name()] =
MessageUtil::keyValueStruct("name", "garply");
ProtobufWkt::Any any;
any.set_type_url("type.googleapis.com/waldo");
any.set_value("fred");
(*metadata.mutable_typed_filter_metadata())[foo_factory_.name()] = any;
TypedMetadataImpl<TypedMetadataFactory> typed(metadata);
EXPECT_NE(nullptr, typed.get<Foo>(foo_factory_.name()));
// If metadata has both Struct and Any field in same factory,
// only Any field is populated.
EXPECT_EQ("fred", typed.get<Foo>(foo_factory_.name())->name_);
}

// Tests data parsing and retrieving when both Struct and Any field
// present in the metadata, but in different factory.
TEST_F(TypedMetadataTest, OkTestBothDifferentFactory) {
envoy::config::core::v3::Metadata metadata;
(*metadata.mutable_filter_metadata())[foo_factory_.name()] =
MessageUtil::keyValueStruct("name", "garply");
ProtobufWkt::Any any;
any.set_type_url("type.googleapis.com/waldo");
any.set_value("fred");
(*metadata.mutable_typed_filter_metadata())[bar_factory_.name()] = any;

TypedMetadataImpl<TypedMetadataFactory> typed(metadata);
// If metadata has both Struct and Any field in different factory,
// both fields are populated.
EXPECT_NE(nullptr, typed.get<Foo>(foo_factory_.name()));
EXPECT_EQ("garply", typed.get<Foo>(foo_factory_.name())->name_);

EXPECT_NE(nullptr, typed.get<Foo>(bar_factory_.name()));
EXPECT_EQ("fred", typed.get<Foo>(bar_factory_.name())->name_);
}

// Tests data parsing and retrieving when both Struct and Any field
// present in the metadata, and in the same factory, but with the case
// that Any field parse() method returns nullptr.
TEST_F(TypedMetadataTest, OkTestBothSameFactoryAnyParseReturnNullptr) {
envoy::config::core::v3::Metadata metadata;
(*metadata.mutable_filter_metadata())[baz_factory_.name()] =
MessageUtil::keyValueStruct("name", "garply");
ProtobufWkt::Any any;
any.set_type_url("type.googleapis.com/waldo");
any.set_value("fred");
(*metadata.mutable_typed_filter_metadata())[baz_factory_.name()] = any;

// Since Any Parse() method in BazFactory just return nullptr,
// Struct data is populated.
TypedMetadataImpl<TypedMetadataFactory> typed(metadata);
EXPECT_NE(nullptr, typed.get<Foo>(baz_factory_.name()));
EXPECT_EQ("garply", typed.get<Foo>(baz_factory_.name())->name_);
}

// Tests data parsing and retrieving when no data present in the metadata.
TEST_F(TypedMetadataTest, NoMetadataTest) {
envoy::config::core::v3::Metadata metadata;
TypedMetadataImpl<TypedMetadataFactory> typed(metadata);
// metadata is empty, nothing is populated.
EXPECT_EQ(nullptr, typed.get<Foo>(foo_factory_.name()));
}

TEST_F(TypedMetadataTest, MetadataRefreshTest) {
// Tests data parsing and retrieving when Struct metadata updates.
TEST_F(TypedMetadataTest, StructMetadataRefreshTest) {
envoy::config::core::v3::Metadata metadata;
(*metadata.mutable_filter_metadata())[foo_factory_.name()] =
MessageUtil::keyValueStruct("name", "foo");
MessageUtil::keyValueStruct("name", "garply");
TypedMetadataImpl<TypedMetadataFactory> typed(metadata);
EXPECT_NE(nullptr, typed.get<Foo>(foo_factory_.name()));
EXPECT_EQ("foo", typed.get<Foo>(foo_factory_.name())->name_);
EXPECT_EQ("garply", typed.get<Foo>(foo_factory_.name())->name_);

// Updated.
(*metadata.mutable_filter_metadata())[foo_factory_.name()] =
MessageUtil::keyValueStruct("name", "bar");
MessageUtil::keyValueStruct("name", "plugh");
TypedMetadataImpl<TypedMetadataFactory> typed2(metadata);
EXPECT_NE(nullptr, typed2.get<Foo>(foo_factory_.name()));
EXPECT_EQ("bar", typed2.get<Foo>(foo_factory_.name())->name_);
EXPECT_EQ("plugh", typed2.get<Foo>(foo_factory_.name())->name_);

// Deleted.
(*metadata.mutable_filter_metadata()).erase(foo_factory_.name());
TypedMetadataImpl<TypedMetadataFactory> typed3(metadata);
EXPECT_EQ(nullptr, typed3.get<Foo>(foo_factory_.name()));
}

TEST_F(TypedMetadataTest, InvalidMetadataTest) {
// Tests data parsing and retrieving when Any metadata updates.
TEST_F(TypedMetadataTest, AnyMetadataRefreshTest) {
envoy::config::core::v3::Metadata metadata;
ProtobufWkt::Any any;
any.set_type_url("type.googleapis.com/waldo");
any.set_value("fred");
(*metadata.mutable_typed_filter_metadata())[bar_factory_.name()] = any;
TypedMetadataImpl<TypedMetadataFactory> typed(metadata);
EXPECT_NE(nullptr, typed.get<Foo>(bar_factory_.name()));
EXPECT_EQ("fred", typed.get<Foo>(bar_factory_.name())->name_);

// Updated.
any.set_type_url("type.googleapis.com/xyzzy");
any.set_value("thud");
(*metadata.mutable_typed_filter_metadata())[bar_factory_.name()] = any;
TypedMetadataImpl<TypedMetadataFactory> typed2(metadata);
EXPECT_NE(nullptr, typed2.get<Foo>(bar_factory_.name()));
EXPECT_EQ("thud", typed2.get<Foo>(bar_factory_.name())->name_);

// Deleted.
(*metadata.mutable_typed_filter_metadata()).erase(bar_factory_.name());
TypedMetadataImpl<TypedMetadataFactory> typed3(metadata);
EXPECT_EQ(nullptr, typed3.get<Foo>(bar_factory_.name()));
}

// Tests empty Struct metadata parsing case.
TEST_F(TypedMetadataTest, InvalidStructMetadataTest) {
envoy::config::core::v3::Metadata metadata;
(*metadata.mutable_filter_metadata())[foo_factory_.name()] = ProtobufWkt::Struct();
EXPECT_THROW_WITH_MESSAGE(TypedMetadataImpl<TypedMetadataFactory> typed(metadata),
Envoy::EnvoyException, "Cannot create a Foo when metadata is empty.");
Envoy::EnvoyException,
"Cannot create a Foo when Struct metadata is empty.");
}

// Tests empty Any metadata parsing case.
TEST_F(TypedMetadataTest, InvalidAnyMetadataTest) {
envoy::config::core::v3::Metadata metadata;
(*metadata.mutable_typed_filter_metadata())[bar_factory_.name()] = ProtobufWkt::Any();
EXPECT_THROW_WITH_MESSAGE(TypedMetadataImpl<TypedMetadataFactory> typed(metadata),
Envoy::EnvoyException,
"Cannot create a Foo when Any metadata is empty.");
}

} // namespace
Expand Down
5 changes: 5 additions & 0 deletions test/common/router/config_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5020,6 +5020,11 @@ class BazFactory : public HttpRouteTypedMetadataFactory {
}
throw EnvoyException("Cannot create a Baz when metadata is empty.");
}

std::unique_ptr<const Envoy::Config::TypedMetadata::Object>
parse(const ProtobufWkt::Any&) const override {
return nullptr;
}
};

TEST_F(RouteMatcherTest, WeightedClusters) {
Expand Down
5 changes: 5 additions & 0 deletions test/common/upstream/upstream_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2255,6 +2255,11 @@ class BazFactory : public ClusterTypedMetadataFactory {
}
throw EnvoyException("Cannot create a Baz when metadata is empty.");
}

std::unique_ptr<const Envoy::Config::TypedMetadata::Object>
parse(const ProtobufWkt::Any&) const override {
return nullptr;
}
};

// Cluster metadata and common config retrieval.
Expand Down