INCOMPLETE: upstream: add filter chains to upstream connections (#173) #3571
Conversation
This is an initial pull request for review, it is not yet or ready to merge. It is working for https://github.com/alanconway/envoy-amqp but has (at least) the following issues: 1. The upstream uses a dummy NOT_IMPLEMENTED FactoryContext. FactoryContext has listener-specific methods, I'm not sure if/how they can be implemented by a Cluster. 2. The "Filter" configuration is defined in listener.proto. I made cds.proto and upstream_impl.cc depend on listener.proto and added server:configuration_lib to upstream_includes. Probably the Filter definition and related bits should be moved to core. 3. Need automated unit and integration tests. The code works with the AMQP filters but needs independent tests in the envoy repo. There are TODO(aconway) comments at the code in question. I would appreciate help and/or direction on how to resolve these and any other issues that come up during review. Signed-off-by: Alan Conway <[email protected]>
|
cc @ggreenway can you also take a pass on this w/ initial comments? Related to upstream proxy proto. |
| // An optional list of network filters that make up the filter chain for | ||
| // outgoing connections made by the cluster. Order matters as the filters are | ||
| // processed sequentially as connection events happen. | ||
| repeated listener.Filter filters = 34 [(gogoproto.nullable) = false]; |
There was a problem hiding this comment.
I agree that that type should probably be moved to core. I don't know if such a change breaks protobuf compatibility or not; we'll need to sort that out before making a change.
There was a problem hiding this comment.
I started to do it and then backed off till I got feedback. I'll resurrect it for review. On the -dev list @mattklein123 seemed to feel it wouldn't be a breaking change but best put it in code first.
There was a problem hiding this comment.
This type of change will cause a compile time breakage, but should not cause a wire breakage. I think it's worth it in this case. @alanconway can you make this change and then I can take a look?
There was a problem hiding this comment.
Please also test that the canonical-json-protobuf is compatible after the change.
There was a problem hiding this comment.
@ggreenway can you point to any existing tests for that kind of compatibility? I have yet to dive into the envoy test suite, I've been testing it from the outside so far.
There was a problem hiding this comment.
I don't know of any, but all I was thinking was to write a config that current-envoy will load that includes these bits, and add a test this config loads (and that the relevant bits seem to take effect).
| @@ -253,6 +256,30 @@ ClusterLoadReportStats ClusterInfoImpl::generateLoadReportStats(Stats::Scope& sc | |||
| return {ALL_CLUSTER_LOAD_REPORT_STATS(POOL_COUNTER(scope))}; | |||
| } | |||
|
|
|||
| // TODO(alanconway): dummy factory context, how do we implement this? Some of | |||
There was a problem hiding this comment.
I think the only fields that are listener-specific are listenerScope(), listenerMetadata(), and possibly drainDecision().
listenerScope() is only used in a few places in the code-base. We may be able to leave it NOT_IMPLEMENTED here, or maybe rename it to ownerScope() or something more generic, and pass it the cluster's scope.
listenerMetadata() isn't called anywhere except one test. Can it be deleted? Or am I missing something?
| @@ -340,6 +367,30 @@ ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config, | |||
| idle_timeout_ = std::chrono::milliseconds( | |||
| DurationUtil::durationToMilliseconds(config.common_http_protocol_options().idle_timeout())); | |||
| } | |||
|
|
|||
| auto filters = config.filters(); | |||
There was a problem hiding this comment.
This looks very similar to the code in listemer_manager_impl. Dedupe?
There was a problem hiding this comment.
Yep, cut-and-paste. It should be pulled up to a common place. Thought I had a TODO on that one, well spotted.
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This is an initial pull request for review, it is not yet or ready to merge.
It is working for https://github.com/alanconway/envoy-amqp but has (at least)
the following issues:
The upstream uses a dummy NOT_IMPLEMENTED FactoryContext. FactoryContext has
listener-specific methods, I'm not sure if/how they can be implemented by a
Cluster.
The "Filter" configuration is defined in listener.proto. I made cds.proto and
upstream_impl.cc depend on listener.proto and added server:configuration_lib
to upstream_includes. Probably the Filter definition and related bits should
be moved to core.
Need automated unit and integration tests. The code works with the AMQP filters
but needs independent tests in the envoy repo.
There are TODO(aconway) comments at the code in question.
I would appreciate help and/or direction on how to resolve these and any other
issues that come up during review.
Signed-off-by: Alan Conway [email protected]