upstream: allow custom extension protocol options#4098
Merged
zuercher merged 5 commits intoenvoyproxy:masterfrom Aug 11, 2018
Merged
upstream: allow custom extension protocol options#4098zuercher merged 5 commits intoenvoyproxy:masterfrom
zuercher merged 5 commits intoenvoyproxy:masterfrom
Conversation
Allows extension protocols to specify custom options. *Risk Level*: low, ignored by existing protocols *Testing*: unit tests *Docs Changes*: inline *Release Notes*: n/a Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
Member
Author
|
(There's a bug in here that I need to fix, but the general shape of this can be reviewed.) |
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
Member
Author
|
Ok. I think it's correct now. |
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
brian-pane
previously approved these changes
Aug 10, 2018
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
htuch
suggested changes
Aug 10, 2018
Member
htuch
left a comment
There was a problem hiding this comment.
This is a good design for protocol options handling IMHO.
| template <class Factory> | ||
| static ProtobufTypes::MessagePtr | ||
| translateToFactoryProtocolOptionsConfig(const Protobuf::Message& source, Factory& factory) { | ||
| ProtobufTypes::MessagePtr config = factory.createEmptyProtocolOptionsProto(); |
Member
There was a problem hiding this comment.
Could you use NamedNetworkFilterConfigFactory instead of templates here for factory?
Member
Author
There was a problem hiding this comment.
Yes. I copied translateToFactoryRouteConfig which also doesn't need to be templated. I'll fix them both.
include/envoy/upstream/upstream.h
Outdated
| * created on behalf of this cluster. | ||
| */ | ||
| virtual ProtocolOptionsConfigConstSharedPtr | ||
| extensionProtocolOptions(const std::string& name) const PURE; |
Member
There was a problem hiding this comment.
Should this be protected instead of public? There's no value in direct access to this from outside the class other than to dynamic_cast..
| "filter envoy.test.filter does not support protocol options"); | ||
| } | ||
|
|
||
| TEST_F(ClusterInfoImplTest, ExtensionProtocolOptionsForFilterWithOptions) { |
Member
There was a problem hiding this comment.
Please add some simple description of what the test does above each TEST_F.
| }; | ||
|
|
||
| // Cluster metadata retrieval. | ||
| TEST_F(ClusterInfoImplTest, Metadata) { |
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
mattklein123
approved these changes
Aug 10, 2018
htuch
pushed a commit
that referenced
this pull request
Aug 20, 2018
…rs (#4165) #4098 Added protocol extensions for clusters and network filters. This PR expands this to http filters as well (this can be used for example in a filter that translates http to a different protocol, like our NATS Streaming filter) Risk Level: low, ignored by existing protocols Testing: unit tests Docs Changes: inline Release Notes: n/a Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Allows extension protocols to specify custom options. This is a precursor to adding Thrift-specific protocol options to the cluster configuration, but is extensible to any future extension that implements a non-HTTP protocol.
Risk Level: low, ignored by existing protocols
Testing: unit tests
Docs Changes: inline
Release Notes: n/a
Signed-off-by: Stephan Zuercher stephan@turbinelabs.io