Add support for Platform and Native filters to C++ EngineBuilder#2626
Add support for Platform and Native filters to C++ EngineBuilder#2626RyanTheOptimist merged 3 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Ryan Hamilton <rch@google.com>
Signed-off-by: Ryan Hamilton <rch@google.com>
|
/assign @Augustyniak |
467a71a to
039f189
Compare
| } | ||
|
|
||
| EngineBuilder& EngineBuilder::addPlatformFilter(const std::string& name) { | ||
| platform_filters_.push_back(name); |
There was a problem hiding this comment.
- any reason for why we use both
emplace_backandpush_backin the implementations of these 2 methods or could we use just one of them instead? - Not entirely sure how is
addPlatformFiltersupported to work given that it does take only 1 argument as opposed to its Swift and Kotlin counterparts that take 2. I realize that we discussed this briefly in our last community meeting but I thought that we would not addaddPlatformFiltermethod at all.
There was a problem hiding this comment.
- emplace_back is used because we are constructing a new NativeFilterConfig object which we don't have yet, so the arguments to emplace_back are the arguments to the NativeFilterConfig constructor. I could do push_back(NativeFilterConfig(name, typed_config)) but that would be slightly more verbose. I
The abseil advice (https://abseil.io/tips/112) boils down to:
So in general, if both push_back() and emplace_back() would work with the same arguments, you should prefer push_back(), and likewise for insert() vs. emplace().
- this implementation of addPlatformFilter simply adds the platform filter name to the envoy config. It does not, however, register the PlatformFilter into the Envoy API. The Swift and Kotlin builders do both of those things. Once the Kotllin and Swift builder call into the C++ builder, they will still do the language specific Filter registration, but they will need the C++ builder to configure Envoy to use it. I hope that helps!
There was a problem hiding this comment.
this implementation of addPlatformFilter simply adds the platform filter name to the envoy config. It does not, however, register the PlatformFilter into the Envoy API. The Swift and Kotlin builders do both of those things. Once the Kotllin and Swift builder call into the C++ builder, they will still do the language specific Filter registration, but they will need the C++ builder to configure Envoy to use it. I hope that helps!
That does definitely help and makes sense! Maybe it would be a good idea to drop some comment with regard to that in addPlatformFilter method's implementation? leaving the decision up to you.
Follow up from #2626. Risk Level: None Testing: N/A Docs Changes: N/A Release Notes: N/A Signed-off-by: Ryan Hamilton <rch@google.com>
…builder-function * origin/main: remove the use of deprecated flag (#2658) Cronvoy: Map EM Errors to Cronet API Errors II (#1594) (#2633) build: revert boring patch (#2651) Bump Lyft Support Rotation (#2654) fix one issue blocking bumping Envoy (#2649) ci: remove Snow from Lyft EM rotation (#2650) ci: increasing timeouts (#2653) python: Apply Envoy python-yapf formatting (#2648) repo: Shellcheck cleanups (#2646) repo: Switch `pip_install` -> `pip_parse` (#2647) Use safe_malloc instead of new when creating new_envoy_map_entry (#2632) python: Pin requirement hashes (#2643) Disable flaky TestConfig.StringAccessors (#2642) ci: migrate from set-output to GITHUB_OUTPUT (#2625) Add a comment to addPlatformFilter (#2634) Allow Cronvoy to build with proguard. (#2635) Update Envoy (#2630) Add support for Platform and Native filters to C++ EngineBuilder (#2626) Register getaddrinfo in extension_registry (#2627) dns: stop using cares DNS resolver (#2618) Signed-off-by: JP Simard <jp@jpsim.com>
As discussed in the weekly meeting, this does not provide a C++ implementation of Platform filters, merely the ability to configure Envoy to use them.
Part of: #2498
Risk Level: Low
Testing: New unit tests
Docs Changes: N/A
Release Notes: Updated version_history.rst
Signed-off-by: Ryan Hamilton rch@google.com