Allow http route and cluster metadata to contain typed metadata in An… by yanjunxiang-google · Pull Request #16107 · envoyproxy/envoy

yanjunxiang-google · 2021-04-21T23:15:05Z

Problem Description:
This PR is to address issue #13986: Allow http route and cluster metadata to contain typed metadata in Any in addition to Struct format.

Solution:

Add google.protobuf.Any field in metadata proto.
Put google.protobuf.Struct field in metadata proto as deprecated.
Add a pure virtual parse() method for Any data in TypedMetadataFactory.
Override the pure parse() method for Any in config_impl_test.cc and upstream_impl_test.cc.
Modify populateFrom() method in TypedMetadataImpl() class to parse both Struct and Any data. The logic is:
search and parse Any data first for that factory for a metadata. If Any data doesn't exist or Any parse() return nullptr, fallback to search and parse Struct data. Note if there are both Struct and Any data in a metadata but with different factory, both will be parsed and pouplated.
Below unit test code to test the populateFrom() logic are added in metadata_test.cc:
Created three different factories for testing above code change, and testing below cases
6.1) no data in metadata
6.2) only Struct in metadata
6.3) only Any in in metadata
6.4) both Struct and Any in metadata in the same factory
6.5) both Struct and Any in metadata in the different factory
6.6) both Struct and Any in metadata in the same factory but parse() method for Any just return nullptr.

and some other unit test to test data update, delete, and exception handling.

Testing:
The code change passed bazel test.

Release Notes:
N/A

Issues: Allow http route and cluster metadata to contain typed metadata in Any in addition to Struct format #13986

Fix #13986

Signed-off-by: Yanjun Xiang yanjunxiang@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

…y in addition to Struct format envoyproxy#13986. Also adding GTEST code for the new field. Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only · 2021-04-21T23:15:11Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #16107 was opened by yanjunxiang-google.

see: more, trace.

yanjunxiang-google · 2021-04-21T23:17:31Z

/assign @yanavlasov @adisuissa

adisuissa

Thanks for working on this, I've left a few comments.
Please also update the PR's main commit message fields.

adisuissa · 2021-04-22T03:44:37Z

api/envoy/config/core/v3/base.proto

  // Key is the reverse DNS filter name, e.g. com.acme.widget. The envoy.*
  // namespace is reserved for Envoy's built-in filters.
  map<string, google.protobuf.Struct> filter_metadata = 1;
+


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.

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.

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.

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

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

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.

Yeah, I think a TypedMetadata class would make sense.

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.

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:

Should I continue this catch and patch style to mark all those test cases which exercising Metadata:Struct with DEPRECATED_FEATURE_TEST()?

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.

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!

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.

include/envoy/config/typed_metadata.h

source/common/config/metadata.h

test/common/config/metadata_test.cc

yanjunxiang-google · 2021-04-23T15:35:08Z

/assign @htuch @stevenzzzz

stevenzzzz

I think we should have some consideration around the default impl of "parse(const ProtobufWkt::Any& data)" and the "fallback" behavior in populateFrom

stevenzzzz · 2021-04-23T15:40:02Z

api/envoy/config/core/v3/base.proto

  // Key is the reverse DNS filter name, e.g. com.acme.widget. The envoy.*
  // namespace is reserved for Envoy's built-in filters.
  map<string, google.protobuf.Struct> filter_metadata = 1;
+


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.

source/common/config/metadata.h

yanjunxiang-google · 2021-04-23T16:58:39Z

/assign @AdiJohn

test/common/config/metadata_test.cc

include/envoy/config/typed_metadata.h

source/common/config/metadata.h

test/common/config/metadata_test.cc

yanavlasov · 2021-04-27T15:28:24Z

This PR needs tests in the TypedMetadataTest suite. It needs a factory that supports decoding from both Any and Struct as well as factory that only decodes from Struct. And then tests that go through various fallback cases that you've added. I.e.

there is both Struct and Any but factory can only decode Struct
there is both Struct and Any but factory can decode Any
only Struct and factory can decode both Any and Struct
not sure if there are any other cases

yanavlasov · 2021-04-27T15:28:32Z

/wait

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2021-04-30T19:35:51Z

Thanks for working on this, I've left a few comments.
Please also update the PR's main commit message fields.

Thanks for the great comments! Done. please take a look!

yanjunxiang-google · 2021-04-30T19:49:27Z

This PR needs tests in the TypedMetadataTest suite. It needs a factory that supports decoding from both Any and Struct as well as factory that only decodes from Struct. And then tests that go through various fallback cases that you've added. I.e.

there is both Struct and Any but factory can only decode Struct

there is both Struct and Any but factory can decode Any

only Struct and factory can decode both Any and Struct

not sure if there are any other cases

Thanks for the good comments. I had modified the change to cover them. Please take a look the new unit test code and let me know if something is missing and need to be added.

yanavlasov · 2021-05-01T01:35:23Z

code changes are good, but it looks like proto format picked up a lot more changes than warranted for this PR. I think this needs to have its proto format results cleaned-up.

adisuissa

Thanks for working on this.
I've left a few comments.

source/common/config/metadata.h

include/envoy/config/typed_metadata.h

source/common/config/metadata.h

test/common/config/metadata_test.cc

yanavlasov · 2021-05-03T18:48:27Z

/wait

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

…ata_proto Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2021-05-12T13:55:36Z

Waiting for #16434 commit, which is to fix the flaky testing issue.

yanjunxiang-google · 2021-05-13T00:17:39Z

/retest

repokitteh-read-only · 2021-05-13T00:17:43Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16107 (comment) was created by @yanjunxiang-google.

see: more, trace.

…ata_proto Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2021-05-13T14:25:12Z

/retest

repokitteh-read-only · 2021-05-13T14:25:15Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16107 (comment) was created by @yanjunxiang-google.

see: more, trace.

yanavlasov · 2021-05-13T14:31:59Z

It will need another main merge after the problem with tooling is fixed

…ata_proto Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanavlasov · 2021-05-14T00:55:52Z

/azp run

azure-pipelines · 2021-05-14T00:56:01Z

Azure Pipelines successfully started running 1 pipeline(s).

…ata_proto Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2021-05-14T14:50:56Z

/retest

repokitteh-read-only · 2021-05-14T14:51:00Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16107 (comment) was created by @yanjunxiang-google.

see: more, trace.

KBaichoo · 2021-05-14T16:12:39Z

@envoyproxy/api-shepherds PTAL

yanjunxiang-google · 2021-05-14T17:06:54Z

/retest

repokitteh-read-only · 2021-05-14T17:06:58Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16107 (comment) was created by @yanjunxiang-google.

see: more, trace.

htuch

LGTM API modulo format nit.

api/envoy/config/core/v3/base.proto

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

…ata_proto Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

adisuissa

/lgtm api

yanjunxiang-google · 2021-05-17T20:03:51Z

/retest

repokitteh-read-only · 2021-05-17T20:03:55Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16107 (comment) was created by @yanjunxiang-google.

see: more, trace.

envoyproxy#16107) Allow http route and cluster metadata to contain typed metadata in Any in addition to Struct format envoyproxy#13986. Signed-off-by: Yanjun Xiang <yanjunxiang@google.com> Signed-off-by: Sixiang Gu <sgu@twitter.com>

envoyproxy#16107) Allow http route and cluster metadata to contain typed metadata in Any in addition to Struct format envoyproxy#13986. Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

Allow http route and cluster metadata to contain typed metadata in An…

7a11210

…y in addition to Struct format envoyproxy#13986. Also adding GTEST code for the new field. Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only bot added the api label Apr 21, 2021

repokitteh-read-only bot assigned lizan Apr 21, 2021

repokitteh-read-only bot assigned adisuissa and yanavlasov Apr 21, 2021

adisuissa reviewed Apr 22, 2021

View reviewed changes

repokitteh-read-only bot assigned htuch and stevenzzzz Apr 23, 2021

stevenzzzz reviewed Apr 23, 2021

View reviewed changes

repokitteh-read-only bot assigned AdiJohn Apr 23, 2021

AdiJohn reviewed Apr 23, 2021

View reviewed changes

test/common/config/metadata_test.cc Outdated Show resolved Hide resolved

yanavlasov requested changes Apr 26, 2021

View reviewed changes

include/envoy/config/typed_metadata.h Outdated Show resolved Hide resolved

source/common/config/metadata.h Outdated Show resolved Hide resolved

test/common/config/metadata_test.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Apr 27, 2021

Update PR based on comments

c997e15

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only bot removed the waiting label Apr 30, 2021

adisuissa reviewed May 1, 2021

View reviewed changes

source/common/config/metadata.h Outdated Show resolved Hide resolved

include/envoy/config/typed_metadata.h Show resolved Hide resolved

source/common/config/metadata.h Show resolved Hide resolved

test/common/config/metadata_test.cc Show resolved Hide resolved

repokitteh-read-only bot added the waiting label May 3, 2021

update diff based on comments

6276b4d

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only bot removed the waiting label May 3, 2021

Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…

2ab8508

…ata_proto Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…

9d8d04f

…ata_proto Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanavlasov previously approved these changes May 13, 2021

View reviewed changes

Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…

e59b4c4

…ata_proto Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…

d2b51da

…ata_proto Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

htuch reviewed May 16, 2021

View reviewed changes

api/envoy/config/core/v3/base.proto Outdated Show resolved Hide resolved

address comments regarding base.proto document format

4767834

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google dismissed stale reviews from yanavlasov and stevenzzzz via 4767834 May 17, 2021 16:08

Merge branch 'main' of https://github.com/envoyproxy/envoy into metad…

33bb388

…ata_proto Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanavlasov approved these changes May 17, 2021

View reviewed changes

adisuissa reviewed May 17, 2021

View reviewed changes

repokitteh-read-only bot removed the api label May 17, 2021

yanavlasov merged commit 178c088 into envoyproxy:main May 18, 2021

Conversation

yanjunxiang-google commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Apr 21, 2021

Uh oh!

yanjunxiang-google commented Apr 21, 2021

Uh oh!

adisuissa left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdiJohn Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yanjunxiang-google commented Apr 23, 2021

Uh oh!

stevenzzzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yanjunxiang-google commented Apr 23, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yanavlasov commented Apr 27, 2021

Uh oh!

yanavlasov commented Apr 27, 2021

Uh oh!

yanjunxiang-google commented Apr 30, 2021

Uh oh!

yanjunxiang-google commented Apr 30, 2021

Uh oh!

yanavlasov commented May 1, 2021

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yanavlasov commented May 3, 2021

Uh oh!

yanjunxiang-google commented May 12, 2021

Uh oh!

yanjunxiang-google commented May 13, 2021

Uh oh!

repokitteh-read-only bot commented May 13, 2021

yanjunxiang-google commented Apr 21, 2021 •

edited

Loading

adisuissa left a comment •

edited

Loading

AdiJohn Apr 23, 2021 •

edited

Loading

yanjunxiang-google May 11, 2021 •

edited

Loading