Skip to content
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.

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

api: MetaProtocol Proxy #18823

Merged
merged 30 commits into from
Dec 3, 2021
Merged

api: MetaProtocol Proxy #18823

merged 30 commits into from
Dec 3, 2021

Conversation

zhaohuabing
Copy link
Member

@zhaohuabing zhaohuabing commented Oct 29, 2021

This PR proposes a MetaProtocol proxy that provides a framework for layer-7 protocols. The common capabilities, including routing, tracing, metrics, logging, etc., will be built into the MetaProtocol proxy. This approach has the potential to significantly lower the barrier to writing new Envoy filters: instead of writing fully functional network filters and duplicating similar codes, now we only need to implement the codec interface. Furthermore, MetaProtocol makes it much easier to build a unified control plane and it could be very helpful for the service mesh ecosystem.

Screen Shot 2021-10-29 at 4 09 49 PM

This PR is a merged efforts of #18768 and #18209

This PR is the APIs of the MetaProtocol Proxy. The implementation will be pushed later after the community agrees on the design and finalizes the API.

Related issues:
Common framework for layer-7 protocols #18761
generic proxy support: #18209
Generic Access Log: #13085
Generic Routing & Rds: #9656

Commit Message: Add API for MetaProtocol Proxy
Additional Description:
Design doc: https://docs.google.com/document/d/1y_ALyjGp_H1_TRrrRxZxnqENISqeZI6eLn71FAFsZPk/edit?usp=sharing
Istio blog about MetaProtocol: https://istio.io/latest/blog/2021/aeraki/
This design has been proven working in Aeraki : https://github.com/aeraki-framework/meta-protocol-proxy
Risk Level: low
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:]

zhaohuabing and others added 10 commits October 28, 2021 10:34
Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
Signed-off-by: ubuntu <ubuntu@localhost.localdomain>
@repokitteh-read-only
Copy link

Hi @zhaohuabing, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #18823 was opened by zhaohuabing.

see: more, trace.

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #18823 was opened by zhaohuabing.

see: more, trace.

@zhaohuabing
Copy link
Member Author

zhaohuabing commented Oct 29, 2021

@mattklein123 @htuch @rojkov @wbpcode Per our discussion, the merged work starts here.

@wbpcode
Copy link
Member

wbpcode commented Oct 29, 2021

Thanks, this would be a great start.

Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
wbpcode and others added 3 commits October 30, 2021 12:08
Signed-off-by: wbpcode <comems@msn.com>
Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
@htuch
Copy link
Member

htuch commented Nov 1, 2021

Thanks @zhaohuabing @wbpcode. As mentioned before, I'm highly supportive of this work and this seems a solid architectural pattern. I've left a first round of comments on your doc; let's make sure we're all aligned on the details there before diving into code review.

@wbpcode
Copy link
Member

wbpcode commented Nov 1, 2021

@htuch Thanks very much. 🌷 We will update our documentation as soon as possible.

Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
@zhaohuabing
Copy link
Member Author

zhaohuabing commented Nov 9, 2021

To fix formatting automatically you can run ./tools/code_format/check_format.py fix. Also it's possible to install git hooks doing prechecks locally upon git push. The hooks can be installed with ./support/bootstrap.

Thanks for the tips @rojkov

Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks this is super cool. Overall this LGTM but per some comments it would be nice to have some more fully formed config examples we can look at?

/wait

config.route.v3.WeightedCluster weighted_clusters = 2;
}
//TODO other configuration: RetryPolicy, Mirroring, Fault Injection, etc.
}
Copy link
Member

Choose a reason for hiding this comment

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

I agree it's not great to have the duplication, but I would be OK starting from scratch and making sure we don't have ambiguity.


// [#protodoc-title: Meta Protocol Proxy Route Matcher Configuration]

// Used to match request service of the downstream request. Only relevant when the application
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible in this PR to have some config snippets? somewhere? I think that would really help.

@@ -0,0 +1 @@
Protocol buffer definitions for the MetaProtocol proxy.
Copy link
Member

Choose a reason for hiding this comment

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

nit: delete, we don't need this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Deleted.

// The meta protocol proxies route table will be dynamically loaded via the Meta RDS API.
MetaRds rds = 3;

// The route table for the meta protocol proxy is static and is specified in this property.
Copy link
Member

Choose a reason for hiding this comment

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

You could potentially put examples here, or in the proto docs for RouteConfiguration.

Copy link
Member Author

Choose a reason for hiding this comment

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

An example of the meta protocol route configuration with matcher API has been added to the route.proto.

Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
MetaRds rds = 3;

// The route table for the meta protocol proxy is static and is specified in this property.
RouteConfiguration route_config = 4;
}

// The codec which encodes and decodes the application protocol.
config.core.v3.TypedExtensionConfig codec = 5 [(validate.rules).message = {required: true}];

// A list of individual Layer-7 filters that make up the filter chain for requests made to the
// meta protocol proxy. Order matters as the filters are processed sequentially as request events
// happen. If no meta_protocol_filters are specified, a default router filter
// (`aeraki.meta_protocol.filters.router`) is used.
Copy link
Member

Choose a reason for hiding this comment

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

remove aeraki.meta_protocol.filters.router

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

some nit comments.

// The name of the application protocol built on top of meta protocol.
string application_protocol = 2 [(validate.rules).string = {min_len: 1}];
// The application protocol built on top of the meta protocol proxy.
ApplicationProtocol application_protocol = 2 [(validate.rules).string = {min_len: 1}];
Copy link
Member

Choose a reason for hiding this comment

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

[(validate.rules).message = {required: true}];

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
Signed-off-by: zhaohuabing <zhaohuabing@gmail.com>
@zhaohuabing
Copy link
Member Author

zhaohuabing commented Nov 24, 2021

Hi, @mattklein123

Is there anything I can do to move this forward? Or did I miss something?

mattklein123
mattklein123 previously approved these changes Nov 29, 2021
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks this LGTM and given that it's marked WIP I think we can ship and iterate as needed. @htuch @snowp any further comments on this one?

@htuch
Copy link
Member

htuch commented Nov 30, 2021

SGTM to merge as WiP, defer to @snowp for final look.

Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Just a small comment, this looks great to me!

// Used to match request service of the downstream request. Only relevant when the application
// protocol is RPC such as Dubbo or Thrift.
// [#not-implemented-hide:]
message ServiceMatchInput {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are service and method just special properties that we think are common enough to have specific types? Or is there something else that makes them special?

I would probably have the comment be a bit more vague here if they are just special properties, like Only applicable if a method provided by the application protocol. gRPC for example is an RPC protocol that doesn't use methods (outside of google.api.annotations)

Copy link
Member Author

Choose a reason for hiding this comment

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

@snowp your concern is legitimate, not every RPC protocol has these two properties.

Signed-off-by: huabingzhao <huabingzhao@tencent.com>
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@snowp snowp merged commit de51441 into envoyproxy:main Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants