http filter: Add overrideable function to http filter factory interface for specifying FilterDependencies#15157
Conversation
Signed-off-by: Auni Ahsan <auni@google.com>
|
@htuch would it make sense to land chunk by itself? If so, could you help me find a reviewer to tag? |
Signed-off-by: Auni Ahsan <auni@google.com>
| * http: added support for `Envoy::ScopeTrackedObject` for HTTP/1 and HTTP/2 dispatching. Crashes while inside the dispatching loop should dump debug information. | ||
| * http: added support for :ref:`preconnecting <envoy_v3_api_msg_config.cluster.v3.Cluster.PreconnectPolicy>`. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic, especially if using HTTP/1.1. | ||
| * http: change frame flood and abuse checks to the upstream HTTP/2 codec to ON by default. It can be disabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to false. | ||
| * http filter: add function to `NamedHttpFilterConfigFactory` which can be overridden to return a :ref`FilterDependencies <envoy_api_file_envoy/extensions/filters/common/dependency/v3/dependency.proto>` specification. Config-plane validation of dependencies is currently WIP. |
There was a problem hiding this comment.
I think you can skip this for now, since its very internally focused. I'd add a single release note when we have the filter chain deps validation working.
include/envoy/server/filter_config.h
Outdated
| * @return FilterDependencies specification of dependencies required or | ||
| * provided on the decode and encode paths. | ||
| */ | ||
| virtual envoy::extensions::filters::common::dependency::v3::FilterDependencies dependencies() { |
There was a problem hiding this comment.
filter_config.h:212:12: error: reference to stack memory associated with local variable 'dependency' returned [-Werror,-Wreturn-stack-address]
return dependency;
I could try declaring a static helper function to return it? But I imagine this will cause problems for overriders of this function anyway, who will have to construct the proto somehow.
There was a problem hiding this comment.
Oh, I see what's going on. This is more similar I think to createEmptyRouteConfigProto() above. I think you should return a unique_ptr with the allocated dependencies in this case.
There was a problem hiding this comment.
I don't understand what the value of that would be; the dependency specification protos won't typically be very large, so passing it by copy seems ok to me. What do you think?
@snowp in case you have any thoughts
There was a problem hiding this comment.
With the protos being small I don't think it matters a whole lot, but a unique_ptr might better future proof this in case this changes? I can imagine some filter potentially providing a lot of different dependencies even if none do so today
There was a problem hiding this comment.
Right, the protos contain multiple strings and we try not copy them around in general.
This reverts commit ec1d1e4. Signed-off-by: Auni Ahsan <auni@google.com>
Signed-off-by: Auni Ahsan <auni@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
include/envoy/server/filter_config.h
Outdated
| * @return FilterDependencies specification of dependencies required or | ||
| * provided on the decode and encode paths. | ||
| */ | ||
| virtual envoy::extensions::filters::common::dependency::v3::FilterDependencies dependencies() { |
There was a problem hiding this comment.
With the protos being small I don't think it matters a whole lot, but a unique_ptr might better future proof this in case this changes? I can imagine some filter potentially providing a lot of different dependencies even if none do so today
Signed-off-by: Auni Ahsan <auni@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
This looks good to me, just one nit
Signed-off-by: Auni Ahsan <auni@google.com>
|
Flakes related to #12209 /retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
|
@snowp /retest seems to be stuck since some of the running tests are still queued, and the failure appears to be a flake. |
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
LGTM! I restarted CI, let's see how it goes
4 participants
Commit message:
Additional description: I thought it would be useful to add the interface itself with documentation and get that out of the way, and I noted in the release notes that validation of the dependencies is WIP.
Risk Level: Low.
See design doc attached to #14470 for more details.