compressor: expose compressor_library for per-route overrides#41177
compressor: expose compressor_library for per-route overrides#41177KBaichoo merged 7 commits intoenvoyproxy:mainfrom
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
agrawroh
force-pushed
the
feat-compressor
branch
2 times, most recently
from
d6b7121 to
946d93b
Compare
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
agrawroh
force-pushed
the
feat-compressor
branch
from
946d93b to
c7a96e6
Compare
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
|
I think this was stuck waiting on a reviewer from the compressor extension side. As I am one of the owners there... /assign @KBaichoo |
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
| return std::make_shared<CompressorPerRouteFilterConfig>(proto_config); | ||
| Server::Configuration::ServerFactoryContext& context, | ||
| ProtobufMessage::ValidationVisitor& validator) { | ||
| // Create a temporary factory context that wraps the generic factory context. |
There was a problem hiding this comment.
Can you help me understand the changes in this file / why they're needed?
There was a problem hiding this comment.
I think it's needed because compressor library factories require FactoryContext but in the per-route config we only get ServerFactoryContext. If you have another recommendation, I can look at a reference and do that instead.
There was a problem hiding this comment.
AFAICT from the public compressor filters usage -- it seems like these only use the serverFactoryContext if they use anything beyond the configuration proto.
I'd be surprised if we needed anything listener related given our impl here for listener is effectively NOPed. Does it make more sense to downgrade the compressor filter to only need a GenericFactoryContext instead? It'd get rid of some of the NOP stuff we end up working around.
It might just have incidentally been added as the arg for the ctor in #20434
There was a problem hiding this comment.
I see. Thanks for sharing the reference.
Are you okay if we keep the current approach for now and follow-up with the interface refactor rather than doing the full refactor now by changing all the compressor factories?
There was a problem hiding this comment.
we can do this as a follow up
test/extensions/filters/http/compressor/compressor_filter_integration_test.cc
Outdated
Show resolved
Hide resolved
test/extensions/filters/http/compressor/compressor_filter_integration_test.cc
Show resolved
Hide resolved
|
would you also be able to update the docs: https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/compressor_filter#per-route-configuration with this since it's pretty neat |
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
| return std::make_shared<CompressorPerRouteFilterConfig>(proto_config); | ||
| Server::Configuration::ServerFactoryContext& context, | ||
| ProtobufMessage::ValidationVisitor& validator) { | ||
| // Create a temporary factory context that wraps the generic factory context. |
There was a problem hiding this comment.
AFAICT from the public compressor filters usage -- it seems like these only use the serverFactoryContext if they use anything beyond the configuration proto.
I'd be surprised if we needed anything listener related given our impl here for listener is effectively NOPed. Does it make more sense to downgrade the compressor filter to only need a GenericFactoryContext instead? It'd get rid of some of the NOP stuff we end up working around.
It might just have incidentally been added as the arg for the ctor in #20434
4 participants
Description
We have a requirement to select GZip vs. Brotli on some specific routes. In this PR, we are adding support for per-route compressor library override in the compressor filter. With this change, routes would be able to specify a different compressor library (e.g., gzip, brotli) via the
compressor_libraryfield on a per-route basis.This would allow different routes to use different compression algorithms and settings while maintaining the same filter configuration.
Commit Message: compressor: expose compressor_library for per-route overrides
Additional Description: Added support to allow different routes to use different compression algorithms and settings while maintaining the same filter configuration.
Risk Level: Low
Testing: Added Unit + Integration Tests
Docs Changes: Added
Release Notes: Added