builder: making compressor configurable#2321
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
library/cc/engine_builder.cc
Outdated
| return *this; | ||
| } | ||
|
|
||
| std::string EngineBuilder::generateFiltersStr() { |
There was a problem hiding this comment.
I think with the alias reworking, this is pretty minimal YAML for adjusting the filter chain, WDYT?
There was a problem hiding this comment.
For specifically this sort of use case, the string "#{custom_filters}" is intended to be the token you target to replace with the extra filters you're adding. So rather than re-defining the core filters like network_configuration or the router externally, instead you simply substitute in whatever you want to add wherever "#{custom_filters}" appears.
(This was always meant to be an interim solution until we could remove interpolation altogether, but I think it still simplifies things.)
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
Oh CI, RIP my inbox. edit; sorted. I had the string in the wrong tempale section so the "add a filter" substitutions were not working |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
goaway
left a comment
There was a problem hiding this comment.
Thanks @alyssawilk - this is indeed what I meant (you can also see another case this is used in EnvoyConfiguration.java).
The cleanup looks good as well - I just see one issue with the setup that should be straightforward to fix.
library/common/config/config.cc
Outdated
| !ignore network_defs: &network_configuration_config | ||
| "@type": type.googleapis.com/envoymobile.extensions.filters.http.network_configuration.NetworkConfiguration | ||
| enable_drain_post_dns_refresh: *enable_drain_post_dns_refresh | ||
| enable_interface_binding: *enable_interface_binding | ||
|
|
||
| !ignore dfp_defs: &dfp_config | ||
| "@type": type.googleapis.com/envoy.extensions.filters.http.dynamic_forward_proxy.v3.FilterConfig | ||
| dns_cache_config: &dns_cache_config | ||
| name: base_dns_cache | ||
| preresolve_hostnames: *dns_preresolve_hostnames | ||
| dns_lookup_family: *dns_lookup_family | ||
| host_ttl: 86400s | ||
| dns_min_refresh_rate: *dns_min_refresh_rate | ||
| dns_refresh_rate: *dns_refresh_rate | ||
| dns_failure_refresh_rate: | ||
| base_interval: *dns_fail_base_interval | ||
| max_interval: *dns_fail_max_interval | ||
| dns_query_timeout: *dns_query_timeout | ||
| typed_dns_resolver_config: | ||
| name: *dns_resolver_name | ||
| typed_config: *dns_resolver_config |
There was a problem hiding this comment.
I like the cleanup here, but any of these that contain aliases need to be in config_template string rather than config_header string in order to maintain their ability to be overridden via EngineBuilders. Should be sufficient to simply relocate this block.
|
cool. I'll go wire up the other language bits and ping back when it's ready for another pass! |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
That was actually horribly painful! Either this was easier than prior changes or I'm finally getting the hang of this. Good for another pass :-) |
| mDnsQueryTimeoutSeconds, mDnsMinRefreshSeconds, mDnsPreresolveHostnames, | ||
| mDnsFallbackNameservers, mEnableDnsFilterUnroutableFamilies, mEnableDrainPostDnsRefresh, | ||
| mEnableHttp3, mEnableHappyEyeballs, mEnableInterfaceBinding, | ||
| mEnableHttp3, true, mEnableHappyEyeballs, mEnableInterfaceBinding, |
There was a problem hiding this comment.
nit: The pattern of passing all named values here is sort of convenient for remembering which parameter is which.
library/cc/engine_builder.cc
Outdated
| return *this; | ||
| } | ||
|
|
||
| EngineBuilder& EngineBuilder::setDecompressor(bool decompressor_on) { |
There was a problem hiding this comment.
nit: enableDecompressor seems to be the convention we've landed on for boolean values in here.
goaway
left a comment
There was a problem hiding this comment.
Just a couple nits - feel free to update as you feel warranted and otherwise merge.
Thanks @alyssawilk!
|
With my almost approval I started on the brotli filter to find it also uses the decompressor filter, so renaming everything to gzip. |
b6a6e5a to
7d74e8e
Compare
7d74e8e to
d1f2e57
Compare
* origin/main: (33 commits) iOS: fix xcframework upload in release workflow (#2366) config: hopefully fixing C++ config default for apple (#2355) Update Envoy (#2364) Bump Lyft Support Rotation (#2365) ci: pin external GitHub Action (#2363) cleanup: fix warning in JNI layer (#2361) cleanup: convert some more uses of NULL to nullptr (#2359) cleanup: consistently use nullptr in cc contexts (#2351) cleanup: remove unused function and resolve warning (#2350) iOS: add configurable gzip and brotli decompression options (#2349) iOS: stop embedding bitcode in releases (#2347) ci: update Android setup (#2354) docs: update the list of clusters (#2344) bazel: update rules_apple (#2346) iOS: add a way to disable network monitoring (#2345) api: adding brotli knobs (#2342) android: create persistent SharedPreferences-based KV store (#2319) ios: add support for registering a platform KV store (#2334) builder: making compressor configurable (#2321) iOS: add SwiftPM example (#2333) ... Signed-off-by: JP Simard <jp@jpsim.com>
* origin/main: (33 commits) iOS: fix xcframework upload in release workflow (#2366) config: hopefully fixing C++ config default for apple (#2355) Update Envoy (#2364) Bump Lyft Support Rotation (#2365) ci: pin external GitHub Action (#2363) cleanup: fix warning in JNI layer (#2361) cleanup: convert some more uses of NULL to nullptr (#2359) cleanup: consistently use nullptr in cc contexts (#2351) cleanup: remove unused function and resolve warning (#2350) iOS: add configurable gzip and brotli decompression options (#2349) iOS: stop embedding bitcode in releases (#2347) ci: update Android setup (#2354) docs: update the list of clusters (#2344) bazel: update rules_apple (#2346) iOS: add a way to disable network monitoring (#2345) api: adding brotli knobs (#2342) android: create persistent SharedPreferences-based KV store (#2319) ios: add support for registering a platform KV store (#2334) builder: making compressor configurable (#2321) iOS: add SwiftPM example (#2333) ... Signed-off-by: JP Simard <jp@jpsim.com>
As a precursor to adding brotli, making the decompressor configurable.
TODO: wire up the APIs once I have Mike's approval of the general design
Description:
Risk Level: low
Testing: new unit tests
Docs Changes: n/a
Release Notes: NEED TO DO