Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Platform and Native filters to C++ EngineBuilder #2626

Merged
merged 3 commits into from
Oct 25, 2022

Conversation

RyanTheOptimist
Copy link
Contributor

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

Signed-off-by: Ryan Hamilton <rch@google.com>
Signed-off-by: Ryan Hamilton <rch@google.com>
@RyanTheOptimist
Copy link
Contributor Author

/assign @Augustyniak

version history

Signed-off-by: Ryan Hamilton <rch@google.com>
@RyanTheOptimist RyanTheOptimist enabled auto-merge (squash) October 20, 2022 22:20
@Augustyniak Augustyniak self-assigned this Oct 21, 2022
}

EngineBuilder& EngineBuilder::addPlatformFilter(const std::string& name) {
platform_filters_.push_back(name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. any reason for why we use both emplace_back and push_back in the implementations of these 2 methods or could we use just one of them instead?
  2. Not entirely sure how is addPlatformFilter supported 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 add addPlatformFilter method at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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().

  1. 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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RyanTheOptimist RyanTheOptimist merged commit 3d505f2 into envoyproxy:main Oct 25, 2022
RyanTheOptimist added a commit that referenced this pull request Oct 26, 2022
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>
jpsim added a commit that referenced this pull request Nov 4, 2022
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants