Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yanavlasov merged 22 commits into from
May 18, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 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
2 changes: 1 addition & 1 deletion api/envoy/config/cluster/v4alpha/BUILD
View file
Open in desktop

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

8 changes: 4 additions & 4 deletions api/envoy/config/cluster/v4alpha/cluster.proto
View file
Open in desktop

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

7 changes: 6 additions & 1 deletion api/envoy/config/core/v3/base.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,12 @@ 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.
map<string, google.protobuf.Struct> filter_metadata = 1;
map<string, google.protobuf.Struct> filter_metadata = 1 [deprecated = true];

@adisuissa adisuissa Apr 22, 2021

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.

@lizan lizan Apr 23, 2021

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.

@stevenzzzz stevenzzzz Apr 23, 2021

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.

@lizan lizan Apr 23, 2021

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.

@AdiJohn AdiJohn Apr 23, 2021 •
edited
Loading

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.

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

@yanavlasov yanavlasov Apr 26, 2021

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.

@htuch htuch Apr 26, 2021

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.

@yanjunxiang-google yanjunxiang-google Apr 30, 2021

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.

@yanjunxiang-google yanjunxiang-google May 9, 2021

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!

@yanjunxiang-google yanjunxiang-google May 11, 2021 •
edited
Loading

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.

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.
map<string, google.protobuf.Any> typed_filter_metadata = 2;
}

// Runtime derived uint32 with a default when not specified.
Expand Down
9 changes: 7 additions & 2 deletions api/envoy/config/core/v4alpha/base.proto
View file
Open in desktop

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

6 changes: 3 additions & 3 deletions api/envoy/config/core/v4alpha/health_check.proto
View file
Open in desktop

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

14 changes: 14 additions & 0 deletions api/envoy/config/endpoint/v4alpha/BUILD
View file
Open in desktop

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

119 changes: 119 additions & 0 deletions api/envoy/config/endpoint/v4alpha/endpoint.proto
View file
Open in desktop

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

158 changes: 158 additions & 0 deletions api/envoy/config/endpoint/v4alpha/endpoint_components.proto
View file
Open in desktop

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

Loading