ecds: network filter support #14717

kyessenov · 2021-01-15T04:06:55Z

Signed-off-by: Kuat Yessenov kuat@google.com

Commit Message: Implement extension config discovery for network filters
Additional Description: This is mostly a refactor of HTTP ECDS to be more general. Templatized factories make it somewhat hard and requires some type gymnastics unfortunately.

Risk Level: low (new features behind API)
Testing: unit
Docs Changes: yes
Release Notes: yes
Fixes: #14696

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch · 2021-01-15T13:48:48Z

@lambdai could you do a first pass on this?

kyessenov · 2021-01-15T15:32:28Z

This is WIP at the moment. I need to reconsider how factory/factorycb are templatized since gcc/msvc differ from clang here.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2021-01-15T22:23:13Z

This is mostly complete except for an integration test and docs. I tested manually with wasm network filters and works as expected.

kyessenov · 2021-01-16T02:33:41Z

/retest

htuch · 2021-01-17T23:36:12Z

I gather this is now ready for review, would appreciate any feedback you have @lambdai.

lambdai · 2021-01-18T01:03:55Z

@htuch ack

lambdai · 2021-01-18T02:34:18Z

source/server/filter_chain_manager_impl.h

+  std::vector<Network::FilterFactoryCb> networkFilterFactories() const override {
+    std::vector<Network::FilterFactoryCb> filters;
+    filters.reserve(filters_factory_.size());
+    for (const auto& provider : filters_factory_) {
+      auto filter_factory = provider->config();
+      if (filter_factory.has_value()) {
+        filters.push_back(filter_factory.value());
+      } else {
+        return {};
+      }
+    }
+    return filters;


networkFilterFactories() is a breaking change.
Previously FilterFactoryCb can be lambda that captures states. Those states are valid within the life of the network filter chain.
With this PR, the FilterFactoryCbs are fired and forgot. Some network filters may survive but probably not all. See also #14711 that utilizes the above assertion.

I am not sure I understand. Is there a difference with using static filter factory provider that wraps the callback vs using the callback directly?

Should the functors really own the config state though? The common style in envoy is to pass functors by value. I can change it to pass by reference but that sounds odd to me.

lambdai

This PR generally LGTM. Please leave whatever the decision and style as it was in http filter.

The only critical part is the life time of FIlterFactoryCallback. This function seems is invoked at filter creation. However, an undocumented behavior is that Envoy could use this callback to own states for the created filters. As a consequence, this callback cannot be destroyed upon thread local update. The destroy must happen after all the filters are gone(basically when the connection is destroyed).

lambdai · 2021-01-18T02:42:28Z

source/server/listener_manager_impl.cc

    const Protobuf::RepeatedPtrField<envoy::config::listener::v3::Filter>& filters,
    Server::Configuration::FilterChainFactoryContext& filter_chain_factory_context) {
-  std::vector<Network::FilterFactoryCb> ret;
+  std::vector<Filter::FilterConfigProviderPtr<Network::FilterFactoryCb>> ret;


Now that you are here, could you add ret.reserve(filters.size()); :)

lambdai · 2021-01-18T02:47:55Z

source/common/filter/config_discovery_impl.h

+    }
+    auto provider = std::make_unique<DynamicFilterConfigProviderImpl<FactoryCb>>(
+        std::move(subscription), require_type_urls, factory_context,
+        [this, stat_prefix, &factory_context](const ProtobufWkt::Any& proto_config) -> FactoryCb {


nit: &stat_prefix?

lambdai · 2021-01-18T02:51:32Z

source/common/filter/config_discovery_impl.h

+  Server::Configuration::FactoryContext& factory_context_;
+
+  Init::SharedTargetImpl init_target_;
+  bool started_{false};


Is started_ used elsewhere?

Yes, it prevents from double starting. See the associated cc file.

lambdai · 2021-01-18T02:57:55Z

source/common/filter/config_discovery_impl.h

+private:
+  struct ThreadLocalConfig : public ThreadLocal::ThreadLocalObject {
+    ThreadLocalConfig() : config_{absl::nullopt} {}
+    absl::optional<FactoryCb> config_{};


Maybe rename to factory_cb_?
Also, why are you prefer value to shared_ptr on this 64byte+ object?

This is just a reshuffle of the old code. There is really not much new in this file added.

kyessenov · 2021-01-18T06:33:52Z

@lambdai I think you are referring to the fact that factory callbacks carry a shared pointer to some state which must be deleted on the main thread (after all streams are done with it)? See this comment:

          // This happens after all workers have discarded the previous config so it can be safely
          // deleted on the main thread by an update with the new config.
          this->current_config_ = config;

kyessenov · 2021-01-18T06:38:21Z

@lambdai Unfortunately, as mentioned in the issue, I had to reorganize the code in the source/common/filter because templating them would put too much code in the headers. So per @htuch 's suggestion, I created several base classes to share code. I think I kept all the logic the same, but let me know if I missed something.

rgs1 · 2021-01-18T23:55:55Z

This PR generally LGTM. Please leave whatever the decision and style as it was in http filter.

The only critical part is the life time of FIlterFactoryCallback. This function seems is invoked at filter creation. However, an undocumented behavior is that Envoy could use this callback to own states for the created filters. As a consequence, this callback cannot be destroyed upon thread local update. The destroy must happen after all the filters are gone(basically when the connection is destroyed).

... Envoy ~~could~~ will use this callback to own states for created filters ... :-)

Somewhat related as well: #14737.

lambdai · 2021-01-19T18:21:07Z

@kyessenov Add a diagram. my concern is mainly about the moment after Callback Gen1 is destroyed while connection 1 is not destroyed yet

kyessenov · 2021-01-19T18:26:35Z

@lambdai Yeah, the undocumented behavior of runOnAllThreads is that it is called on main last. So the last reference is deleted on main.

lambdai · 2021-01-19T20:21:17Z

@lambdai Yeah, the undocumented behavior of runOnAllThreads is that it is called on main last. So the last reference is deleted on main.

@kyessenov It's not about which thread the Callback(FilterFactoryCb) is destroyed. The fact is, FilterFactoryCb is NOT a C-style function ptr. Instead, it is std::function that have captures.
In the diagram, connection1 owns the Filter that is created by Callback Gen1 but the connection can outlive Callback Gen1 if the network filter config is dynamic.

IMHO the next step can be one of below

Leave whatever the code as-is in this PR but explicitly document that any ECDS dynamic filter must not refer to anything in FilterFactoryCb. Maybe explicitly declare it as C-style function ptr. The cons is that guide is tricky: there is a gap between dynamic network filter config and static network filter config.
Do things similar to http filters: pass configs by reference instead of shared_ptr #14737 but in the opposite direction: If any network filter wants to share anything among filter objects, use shared_ptr and eliminate the c++ references

mattklein123 · 2021-01-19T21:12:39Z

Do things similar to #14737 but in the opposite direction: If any network filter wants to share anything among filter objects, use shared_ptr and eliminate the c++ references

I haven't been tracking this convo, but I just merged the above PR. I would be fine with effectively inverting it, but we should be consistent either way.

lambdai · 2021-01-19T22:33:24Z

Thanks, @rgs1. The life time of factory callback lambda is unclear to me. How do we know when it is safe to delete it? We need some sort of ownership from the stream back to lambda?

Previously Envoy destroys all connections before Envoy destroys the associated FilterFactoryCb

…xy#14711)" This reverts commit 72db81d. Per discussion in envoyproxy#14717 and via Slack, we'll come up with a different approach since using a std::function to keep state presents a few challenges. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

…#14755) This reverts commit 72db81d. Per discussion in #14717 and via Slack, we'll come up with a different approach since using a std::function to keep state presents a few challenges. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

kyessenov · 2021-01-21T20:31:11Z

@lambdai Given that we reverted the use of functions to own the config, can you take a look again without that assumption?

@lambdai

Signed-off-by: qinggniq <livewithblank@gmail.com> add auth helper Signed-off-by: qinggniq <livewithblank@gmail.com> refactor mysql server greeting codec Signed-off-by: qinggniq <livewithblank@gmail.com> codec encode Signed-off-by: qinggniq <livewithblank@gmail.com> add unit test for mysql greeting codec Signed-off-by: qinggniq <livewithblank@gmail.com> complete login resp codec Signed-off-by: qinggniq <livewithblank@gmail.com> add login message test Signed-off-by: qinggniq <livewithblank@gmail.com> feat link error Signed-off-by: qinggniq <livewithblank@gmail.com> refactor mysql login resp Signed-off-by: qinggniq <livewithblank@gmail.com> pass all codec test Signed-off-by: qinggniq <livewithblank@gmail.com> only contain codec change Signed-off-by: qinggniq <livewithblank@gmail.com> remove header Signed-off-by: qinggniq <livewithblank@gmail.com> http: expose encoded headers/trailers via callbacks (envoyproxy#14544) In order to support upstream filters passing ownership of headers and then being able to reference them after the fact, expose a HTTP filter function that allows reading the header maps back. Signed-off-by: Snow Pettersen <snowp@lyft.com> Implement request header processing in ext_proc (envoyproxy#14385) Send request headers to the server and apply header mutations based on the response. The rest of the protocol is still ignored. Signed-off-by: Gregory Brail <gregbrail@google.com> 1.17.0 release (envoyproxy#14624) Signed-off-by: Matt Klein <mklein@lyft.com> kick off v1.18.0 (envoyproxy#14637) Signed-off-by: Matt Klein <mklein@lyft.com> filter manager: drop assert (envoyproxy#14633) Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com> docs: update ext_proc docs to reflect implementation status (envoyproxy#14636) Signed-off-by: Gregory Brail <gregbrail@google.com> [tls] Expose ServerContextImpl::selectTlsContext (envoyproxy#14592) Signed-off-by: Chad Retz <chad.retz@stackpath.com> http: support creating filters with match tree (envoyproxy#14430) Adds support for wrapping a HTTP filter with an ExtensionWithMatcher proto to create the filters with an associated match tree. Under the hood this makes use of a wrapper filter factory that manages creating the match tree and adding it to the FM alongside the associated filter. Also includes some code to register factories for input/actions, allowing them to be referenced in the proto configuration. Signed-off-by: Snow Pettersen <snowp@lyft.com> fix empty connection debug logs (envoyproxy#14666) Fixes envoyproxy#14661 Signed-off-by: Rama Chavali <rama.rao@salesforce.com> tcp_proxy: ignore transfer encoding in HTTP/1.1 CONNECT responses (envoyproxy#14623) Commit Message: Ignore the transfer encoding header in CONNECT responses Additional Description: NONE Risk Level: low Testing: integration test Docs Changes: NONE Release Notes: https://github.com/irozzo-1A/envoy/blob/ignore-transfer-encoding/docs/root/version_history/current.rst#new-features Platform Specific Features: NONE Fixes envoyproxy#11308 Signed-off-by: Iacopo Rozzo <iacopo@kubermatic.com> ci: fix docs tag build (envoyproxy#14653) Signed-off-by: Lizan Zhou <lizan@tetrate.io> HTTP health checker: handle GOAWAY from HTTP2 upstreams (envoyproxy#13599) Makes the HTTP health checker handle GOAWAY properly. When the NO_ERROR code is received, any in flight request will be allowed to complete, at which time the connection will be closed and a new connection created on the next interval. GOAWAY frames with codes other than NO_ERROR are treated as a health check failure, and immediately close the connection. Signed-off-by: Michael Puncel <mpuncel@squareup.com> upstream: clean up feature parsing code (envoyproxy#14629) Fixing a perfectly safe and fairly terrible version merge in the ALPN pr the "refactor all upstream config" PRs. the original code created the new options for new config, and parseFeatures handled parsing features from either the new options, or the old config. I decided that was too complicated, changed the code to always create the new options struct and forgot to clean up parseFeatures to assume the presence of the new options struct and remove handling things the old style way. Risk Level: low (clean up inaccessible code) Testing: added one extra unit test just because Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> upstream: force a full rebuild on host weight changes (envoyproxy#14569) This will allow us to build load balancers that pre-compute data structures based on host weights (for example using weighted queues), to work around some of the deficiencies of EDF scheduling. This behavior can be temporarily disabled by setting the envoy.reloadable_features.upstream_host_weight_change_causes_rebuild feature flag to false. Fixes envoyproxy#14360 Signed-off-by: Matt Klein <mklein@lyft.com> access log: add support for command formatter extensions (envoyproxy#14512) Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com> test: improving dynamic_forward_proxy coverage (envoyproxy#14672) Risk Level: n/a (test only) Signed-off-by: Alyssa Wilk <alyssar@chromium.org> access_logs: removing disallow_unbounded_access_logs (envoyproxy#14677) Signed-off-by: Alyssa Wilk <alyssar@chromium.org> wasm: replace the obsolete contents in wasm-cc's README with docs link (envoyproxy#14628) Signed-off-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com> grpc-json-transcoder: support root path (envoyproxy#14585) Signed-off-by: Xuyang Tao <taoxuy@google.com> ecds: add config source for network filter configs (envoyproxy#14674) Signed-off-by: Kuat Yessenov <kuat@google.com> fix comment for parameters end_stream of decodeData/encodeData. (envoyproxy#14620) Signed-off-by: wangfakang <fakangwang@gmail.com> [fuzz] Fix bugs in HPACK fuzz test (envoyproxy#14638) - Use after free because nghttp2_nv object has pointers to the underlying strings and copying them resulted in a use after free when the copy was used after the original was destroyed - Fixed sorting issues and tested leading/trailing whitespace headers (I can no longer reproduce an issue I saw where a null byte appeared after decoding whitespace, maybe the former fix fixed this) Risk Level: Low Testing: Added regression tests and cases for whitespace headers Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28880 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28869 Signed-off-by: Asra Ali <asraa@google.com> v3 packages updates for omit_canary_hosts proto (envoyproxy#14117) Risk Level: LOW Testing: unit ( proto_format and docs ) part of envoyproxy#12841 Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com> streaminfo/mocks: delay filter_state_ dereference (envoyproxy#14612) By dereferencing filter_state_ in the constructor, any test that sets filter_state_ will dereference an invalid pointer. This may not be a common use-case, but it came up when writing some microbenchmarks for a custom filter where I needed to reset the FilterState on each iteration of the benchmark. Signed-off-by: Brian Wolfe <brian.wolfe@airbnb.com> http: support passing match result action to filter (envoyproxy#14462) Adds support for passing through a match action from a match tree to the associated HTTP filter. Some care has to be taken here around dual filters, so we introduce an abstraction that moves handling HttpMatchingData updates and applying the match result into a FilterMatchState object that is shared between all filter wrappers for a given filter. This should also avoid having to match twice for dual filters: the match result is shared for both filters, instead of both of them having to independently arrive at it with the same data. Signed-off-by: Snow Pettersen <snowp@lyft.com> refactor: use unitfloat in more places (envoyproxy#14396) Commit Message: Use UnitFloat in place of float in more locations Additional Description: UnitFloat represents a floating point value that is guaranteed to be in the range [0, 1]. Use it in place of floats that also have the same expectation in OverloadActionState and connection listeners. This PR introduces no functional changes. Risk Level: low Testing: ran affected tests Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Alex Konradi <akonradi@google.com> [tls] add missing built in cipher stat names (envoyproxy#14676) * add missing ciphers Signed-off-by: Asra Ali <asraa@google.com> docs: adding coverage walkthroguh (envoyproxy#14688) Risk Level: n/a Testing: n/a Docs Changes: adding developer docs Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> local ratelimit: Add descriptor support in HTTP Local Rate Limiting (envoyproxy#14588) Signed-off-by: Kuat Yessenov <kuat@google.com> Co-authored-by: gargnupur <gargnupur@google.com> http: prefetch for upstreams (envoyproxy#14143) Commit Message: Adding predictive prefetch (useful mainly for HTTP/1.1 and TCP proxying) and uncommenting prefetch config. Additional Description: Risk Level: low (mostly config guarded) Testing: unit, integration tests Docs Changes: APIs unhidden Release Notes: inline Fixes envoyproxy#2755 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> docs: Give a hint to specify type_url instead (envoyproxy#14562) Signed-off-by: Dhi Aurrahman <dio@rockybars.com> Remove flaky_on_windows tag from proxy_filter_integration_test (envoyproxy#14680) Testing: Ran proxy_filter_integration_test thousands of times Signed-off-by: Randy Miller <rmiller14@gmail.com> upstream: Fix moving EDS hosts between priorities. (envoyproxy#14483) At present if health checks are enabled and passing then moving an EDS host from P0->P1 is a NOOP, and P1->P0 results in an abort. In the first case: * P0 processing treats A as being removed because it's not in P0's list of endpoints anymore. * P0 marks A's existing Host as PENDING_DYNAMIC_REMOVAL. It marks A as having been updated in this config apply. * P1 skips over A because it is marked as updated in this update cycle already. In the second case: * P0 updates the priority on the existing Host. It is appended to the vector of added hosts. * P1 marks A's existing Host as PENDING_DYNAMIC_REMOVAL. It does adjust the removed host vector as the host is still pending removal. * A's Host is now in both priorities and is PENDING_DYNAMIC_REMOVAL. This is wrong, and would cause problems later but doesn't have a chance to because: * onClusterMemberUpdate fires with A's existing Host in the added vector (remember it wasn't removed from P1!) * HealthChecker attempts to create a new health check session on A, which results in an abort from the destructor of the already existing one. This was masked in tests by the tests enabling ignore_health_on_host_removal. We fix this by passing in the set of addresses that appear in the endpoint update. If a host being considered for removal appears in this set, and it isn't being duplicated into the current priority as a result of a health check address change, then we assume it's being moved and will immediately remove it. To simplify the case where a host's health check address is being changed AND it is being moved between priorities we always apply priority moves in place before we attempt any other modifications. This means such a case is treated as a separate priority move, followed by the health check address change. fixes envoyproxy#11517 Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com> examples: Add TLS SNI sandbox (envoyproxy#13975) Signed-off-by: Ryan Northey <ryan@synca.io> [utility]: Change behavior of main thread verification utility (envoyproxy#14660) Currently, the isMainThread function can only be called during the lifetime of thread local instance because the singleton that store main thread id is initialized in the constructor of tls instance and cleared in the destructor of tls instance. Change the utility so that outside the lifetime of tls instance, the function return true by default because everything is in main thread when threading is off. Risk Level: low Testing: change unit to reflect change of behavior. Signed-off-by: chaoqin-li1123 <chaoqin@uchicago.edu> Windows build: Add repository cache to CI (envoyproxy#14678) Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> ext-proc: Support "immediate_response" options for request headers (envoyproxy#14652) This lets ext_proc servers return an immediate HTTP response (such as to indicate an error) in response to a request_headers message. Signed-off-by: Gregory Brail <gregbrail@google.com> stats: convert tag extractor regexs to Re2 (envoyproxy#14519) Risk Level: high, the regexes are updated to match more specific patterns. Testing: unit tests Fixes envoyproxy#14439 Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com> hcm: removing envoy.reloadable_features.early_errors_via_hcm envoyproxy#14641 (envoyproxy#14684) Risk Level: Low (removal of deprecated disabled code) Testing: n/a Docs Changes: n/a Release Notes: inline Fixes envoyproxy#14641 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> test: Fix O(1/32k) flakiness in H2 flood tests that disable writes based on source port of outgoing connections. (envoyproxy#14695) It is possible for the kernel to assign the same source port to both the client connection used by the test framework to connect to the Envoy and the Envoy's client connection to the upstream. When the source port is reused by both connections, the test client times out while trying to send the request because disabling write on the upstream connection also disabled writes on the test's client connection. Signed-off-by: Antonio Vicente <avd@google.com> tls: add missing stats for signature algorithms. (envoyproxy#14703) While there, refresh supported cipher suites and add more warnings. Signed-off-by: Piotr Sikora <piotrsikora@google.com> connection: tighten network connection buffer limits (envoyproxy#14333) Signed-off-by: Antonio Vicente <avd@google.com> xdstp: LDS glob collection support. (envoyproxy#14311) This patch introduces support for LDS xdstp:// collection URLs for glob collections over ADS. Context parameters are currently computed from node and resource URLs. Followup PRs will add support for other collection types (CDS, SRDS), non-ADS, provide dynamic context parameter update, extend support to singleton resources and then other xdstp:// features (list collections, redirects, alternatives, etc.) Part of envoyproxy#11264. Risk level: Low (opt-in) Testing: ADS integration test added. Various unit tests following implementation. Signed-off-by: Harvey Tuch <htuch@google.com> listener manager: avoid unique -> shared conversion (envoyproxy#14693) buildFilterChainInternal() returns a shared_ptr, so let's make that instead of unique_ptr. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com> proto: re-implement RepeatedPtrUtil::hash(). (envoyproxy#14701) This changes RepeatedPtrUtil::hash() implementation to match MessageUtil::hash(), which was re-implemented in envoyproxy#8231. Reported by Tomoaki Fujii (Google). Signed-off-by: Piotr Sikora <piotrsikora@google.com> tcp: setting nodelay on all connections (envoyproxy#14574) This should have minimal effect, new server side connections had no-delay, codecs set no-delay, and upstream pools set no-delay. Traffic not using the tcp connection pool may be affected as well as raw use of the TCP client. Risk Level: Medium (data plane) Testing: new unit tests Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.always_nodelay Signed-off-by: Alyssa Wilk <alyssar@chromium.org> test: Add multiheader TE + Content-Length test (envoyproxy#14686) Signed-off-by: Yan Avlasov <yavlasov@google.com> http2: Flip the upstream H2 frame flood and abuse checks to ON by default (envoyproxy#14443) Signed-off-by: Yan Avlasov <yavlasov@google.com> Fix the emsdk patching. (envoyproxy#14673) If the patch fails, because of `|| true`, bazel continues the build. Signed-off-by: Jonh Wendell <jonh.wendell@redhat.com> test: print test parameters meaningfully (envoyproxy#14604) Signed-off-by: Alex Konradi <akonradi@google.com> Migrate v2 thrift_filter to v3 api and corresponding docs changes. (envoyproxy#13885) part of envoyproxy#12841 Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com> http: reinstating prior connect timeout behavior (envoyproxy#14685) Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Fix typo (envoyproxy#14716) Signed-off-by: Hu Shuai <hus.fnst@cn.fujitsu.com> master -> main (envoyproxy#14729) Various fixes Signed-off-by: Matt Klein <mklein@lyft.com> readme: fix logo URL (envoyproxy#14733) Signed-off-by: Matt Klein <mklein@lyft.com> Bump nghttp2 to 1.42.0 (envoyproxy#14730) - Drops nghttp2 PR1468 patch - Requires bazel_external_cmake to support copts, defines to drop the rest Risk Level: low Testing: CI Fixes envoyproxy#1417 Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Pick up current bazel-build-tools tag (envoyproxy#14734) Signed-off-by: William A Rowe Jr <wrowe@vmware.com> access-logger: support request/response headers size (envoyproxy#14692) Add following command operator in access logger %REQUEST_HEADER_BYTES% %RESPONSE_HEADER_BYTES% %RESPONSE_TRAILER_BYTES% Risk Level: Low Testing: unit test Docs Changes: done Release Notes: done Signed-off-by: Xuyang Tao <taoxuy@google.com> dynamic_forward_proxy: envoy.reloadable_features.enable_dns_cache_circuit_breakers deprecation (envoyproxy#14683) * dynamic_forward_proxy: deprecation Signed-off-by: Shikugawa <rei@tetrate.io> Add support for google::protobuf::ListValue formatting (envoyproxy#14518) Signed-off-by: Itamar Kaminski <itamark@google.com> tls: improve TLS handshake/read/write error log (envoyproxy#14600) Signed-off-by: Shikugawa <Shikugawa@gmail.com> config: switch from std::set to absl::flat_hash_set for resource names. (envoyproxy#14739) This was a cleanup deferred from the review of envoyproxy#14311. The idea is to switch to the more efficient unordered absl::flat_hash_set across the resource subscription code base. Internally, we still use std::set (and even explicitly sort in the http_subscription_impl) to avoid changing any wire ordering. It seems desirable to preserve this for two reasons: (1) this derisks this PR as an internal-only change and (2) having deterministic wire ordering makes debug of xDS issues somewhat easier. Risk level: Low Testing: Updated tests. Signed-off-by: Harvey Tuch <htuch@google.com> network filters: avoid unnecessary std::shared_ptrs (envoyproxy#14711) While debugging a crash in: envoyproxy#13592 I ended up discussing with @lambdai and @mattklein123 whether network filters can hold references to things owned by their corresponding FactoryFilterCb. The answer is yes and the HCM and some other notable filters already use references instead of std::shared_ptrs. So let's consistently do this everywhere to avoid someone else asking this same question in the future. Plus, it's always nice to create fewer std::shared_ptrs. Follow-up on: envoyproxy#8633 Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com> docs: Updated version history with 1.13.8 release notes. (envoyproxy#14742) Signed-off-by: Christoph Pakulski <christoph@tetrate.io> Dispatcher: keeps a stack of tracked objects. (envoyproxy#14573) Dispatcher will now keep a stack of tracked objects; on crash it'll "unwind" and have those objects dump their state. Moreover, it'll invoke fatal actions with the tracked objects. This allows us to dump more information during crash. See related PR: envoyproxy#14509 Will follow up with another PR dumping information at the codec/parser level. Signed-off-by: Kevin Baichoo <kbaichoo@google.com> bootstrap-extensions: fix a crash on http callout (envoyproxy#14478) Currently when the ServerFactoryContext is passed to bootstrap extensions, it is only partially initialized. Specifically, attempting to access the cluster manager will cause a nullptr access (and hence a crash) This PR splits the creation and initialized to 2 seperate fucntions. Early creation is required to not break the `default_socket_interface` feature. Once created, the extension will receive the ServerFactoryContext in a different callback (the newly added `serverInitialized`), once they are fully initialized. Commit Message: Fix a crash that happens when bootstrap extensions perform http calls. Additional Description: Risk Level: Low (small bug-fix) Testing: Unit tests updated; tested manually with the changes as well. Docs Changes: N/A Release Notes: N/A Fixes envoyproxy#14420 Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com> overload: create scaled timers via the dispatcher (envoyproxy#14679) Refactor the existing pathway for creating scaled Timer objects away from the ThreadLocalOverloadState and into the Dispatcher interface. This allows scaled timers to be created without plumbing through a bunch of extra state. Signed-off-by: Alex Konradi <akonradi@google.com> http: removing nvoy.reloadable_features.fix_upgrade_response envoyproxy#14643 (envoyproxy#14706) Risk Level: Low (removing deprecated disabled code) Testing: n/a Docs Changes: n/a Release Notes: inline Fixes envoyproxy#14643 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> http: removing envoy.reloadable_features.fixed_connection_close (envoyproxy#14705) Risk Level: Low (removing deprecated guarded code) Testing: n/a Docs Changes: n/a Release Notes: inline Fixes envoyproxy#14645 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Revert "network filters: avoid unnecessary std::shared_ptrs (envoyproxy#14711)" (envoyproxy#14755) This reverts commit 72db81d. Per discussion in envoyproxy#14717 and via Slack, we'll come up with a different approach since using a std::function to keep state presents a few challenges. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com> Clarify Consecutive Gateway Failure docs (envoyproxy#14738) It was initially unclear to me that when split_external_local_origin_errors is in the default setting of false that local origin failures will be counted as Consecutive Gateway Failures. It is clear above that they are counted by the Consecutive 5xx detection type but since I had that disabled I was surprised to find them counted in Consecutive Gateway Failure. I think the logic makes sense though so just attempting to clarify the docs here. Signed-off-by: Matthew Mead-Briggs <mmb@yelp.com> thrift proxy: fix crash when using payload_passthrough (envoyproxy#14723) We started seeing crashes triggered by ConnectionManager::passthroughEnabled() once we enabled `payload_passthrough`. That code assumes that there will _always_ be an active RPC. However, this is not true after a local response has been sent (e.g.: no healthy upstream, no cluster, no route, etc.). Risk Level: low Testing: unit tests added Doc Changes: n/a Release Notes: n/a Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com> thrift proxy: add comments explaining local replies (envoyproxy#14754) Risk Level: low Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com> wasm: update V8 to v8.9.255.6. (envoyproxy#14764) Signed-off-by: Piotr Sikora <piotrsikora@google.com> Request headers to add (envoyproxy#14747) Being consistent about treating Host: and :authority the same way in Envoy header modification. Risk Level: Medium (changes allowed modifiable headers) Testing: new unit tests Docs Changes: yes Release Notes: inline Signed-off-by: Alyssa Wilk <alyssar@chromium.org> tls: update BoringSSL to fbbf8781 (4324). (envoyproxy#14763) Signed-off-by: Piotr Sikora <piotrsikora@google.com> docs: change getting started order (envoyproxy#14774) Sandboxes are more relevant to new users than the other sections. Signed-off-by: Matt Klein <mklein@lyft.com> oauth2: set accept header on access_token request (envoyproxy#14538) Co-authored-by: Dhi Aurrahman <dio@tetrate.io> Signed-off-by: Richard Patel <me@terorie.dev> router: Remove envoy.reloadable_features.consume_all_retry_headers (envoyproxy#14662) This patch removes the envoy.reloadable_features.consume_all_retry_headers runtime flag. Signed-off-by: Martin Matusiak <numerodix@gmail.com> docs: API review checklist (envoyproxy#14399) * API review checklist Signed-off-by: Mark D. Roth <roth@google.com> [docs] Add guidance on ENVOY_BUG in STYLE.md (envoyproxy#14575) * Add guidanceon ENVOY_BUG and macro usage to STYLE.md Signed-off-by: Asra Ali <asraa@google.com> filters: Add test/server:filter_config_test (envoyproxy#14746) As part of envoyproxy#14470, I'll be modifying the base filter interface to include an overridable dependencies() function. This is prep work. Risk Level: Low (test only) Doc Change: n/a Release Notes: n/a Signed-off-by: Auni Ahsan <auni@google.com> dns: removing envoy.reloadable_features.fix_wildcard_matching envoyproxy#14644 (envoyproxy#14768) Signed-off-by: Alyssa Wilk <alyssar@chromium.org> test: FakeUpstream threading fixes (envoyproxy#14526) Signed-off-by: Antonio Vicente <avd@google.com> server: add FIPS mode statistic indicating FIPS compliance (envoyproxy#14719) Signed-off-by: Ravindra Akella <rakella@rakella-ltm.internal.salesforce.com> Add error_state to all config dump resources (envoyproxy#14689) Store the NACKed resource in each resources Risk Level: None Fixes: envoyproxy#14431 Signed-off-by: Lidi Zheng <lidiz@google.com> docs: fix two typos in jwt_authn_filter (envoyproxy#14796) Signed-off-by: Lukasz Jernas <lukasz.jernas@allegro.pl> tcp: adding logs and debug checks (envoyproxy#14771) Adding some logs and one ENVOY_BUG around the new TCP pool. Signed-off-by: Alyssa Wilk <alyssar@chromium.org> google_grpc: attempt to reduce lock contention between completionThread() and onCompletedOps() (envoyproxy#14777) Holding a stream's lock while running handleOpCompletion can result in the completion queue having to wait until the lock is released before adding messages on that stream to completed_ops_. In cases where the completion queue is shared across multiple gRPC streams, delivery of new messages on all streams is blocked until the lock held by the first stream while executing onCompletedOps. Signed-off-by: Antonio Vicente <avd@google.com> filters: Add dependencies.proto (envoyproxy#14750) Introduces the FilterDependency proto. This isn't quite an extension, but it's a common proto to be used by all filter extensions. Risk Level: Low (proto addition only) Signed-off-by: Auni Ahsan <auni@google.com> tools: Syncing api/BUILD file to generated_api_shadow (envoyproxy#14792) After chatting with @akonradi on Slack, it seems the generated_api_shadow/BUILD file was not being updated by proto_format since PR envoyproxy#9719. This PR copies the api/BUILD file to generated_api_shadow. Risk Level: Low (relevant for development) Signed-off-by: Adi Suissa-Peleg <adip@google.com> Add debug log for slow config updates for GRPC subscriptions (envoyproxy#14343) Risk Level: Low Testing: Docs Changes: N/A Release Notes: N/A Signed-off-by: Adam Schaub <adamsjob@google.com> oauth2 filter: Make OAuth scopes configurable. (envoyproxy#14168) New optional parameter 'auth_scopes' added to the filter. The default value is 'user' (if not provided) to avoid breaking changes to users updating to the latest version. Signed-off-by: andreyprezotto <andreypp@gmail.com> Co-authored-by: Nitin Goyal <nitingoyal.dev@gmail.com> upstream: Optimize LoadStatsReporter::startLoadReportPeriod implementation (envoyproxy#14803) cm_.clusters() is not O(1) in part due to it creating maps and returning by value. This means that startLoadReportPeriod was effectively O(n**2) on number of clusters since cm_.clusters() is called for every active cluster. Risk Level: low, functional no-op Testing: Existing tests. We may want a benchmark. Signed-off-by: Antonio Vicente <avd@google.com> ext_proc: Implement response path for headers only (envoyproxy#14713) Implement header processing on the response path by sending the response_headers message to the processor and handling the result. Also update the docs in the .proto file. Signed-off-by: Gregory Brail <gregbrail@google.com> reformat code Signed-off-by: qinggniq <livewithblank@gmail.com>

lambdai

Generally LGTM!
My only concern is the condition of validation exception throws during listener update. I think listener will be in an inconsistent state. In theory, this bad default configuration should rollback the listener

lambdai · 2021-01-28T19:34:36Z

source/common/filter/config_discovery_impl.h

+    if (!config_source.apply_default_config_without_warming()) {
+      factory_context.initManager().add(subscription->initTarget());
+    }


There is another factory_context.initManager().add(provider->initTarget()); 5 lines below at the reverse considition. Cound you colocate them?

The order matters I believe since there is an object constructed in the middle.

source/common/filter/config_discovery_impl.h

lambdai · 2021-01-28T20:52:20Z

source/common/filter/config_discovery_impl.h

+    }
+    // Apply default config if requested.
+    if (config_source.has_default_config()) {
+      provider->validateConfig(config_source.default_config());


Can you clarify where the exception is caught? I have the concern that provider is destroyed and the target within is deemed as completed. If it's this target is the only one in init manager, the initmanager will be accidentally notified as `all targets are completed"

Not sure if there is a test case covers that "the single invalid config is the only target"

This whole thing runs within xDS path on main, so exceptions are normal here. An invalid update will trigger a retry loop within xDS, and after some deadline and invalid config is accepted. The invalid config will acts as no filters.

kyessenov · 2021-01-28T22:05:03Z

Sorry, I'm no longer able to work on this. @mathetake feel free to pick it up.

lambdai · 2021-02-11T18:59:05Z

I revoke my request for changes since the issue is addressed by #14934

We can fix in the follow up PRs

lambdai

Since my concern is describe in another issue and the code is generally migrated from existing code. Fine for me

lambdai · 2021-02-11T19:04:09Z

source/common/filter/config_discovery_impl.h

+                       bool is_terminal) const;
+
+private:
+  absl::flat_hash_map<std::string, std::weak_ptr<FilterConfigSubscription>> subscriptions_;


Prefer manage provider to manage subscription to address #14934. see

envoy/source/common/router/rds_impl.h

Line 260 in 1d1b708

absl::node_hash_map<uint64_t, std::weak_ptr<RdsRouteConfigProviderImpl>>

snowp

Somewhat hard to review the large diff but overall looks good though. Is there anyone actively working on this at this moment? Needs a CI fix

/wait-any

snowp · 2021-02-17T17:33:18Z

include/envoy/config/extension_config_provider.h

+template <class FactoryCallback>
+class ExtensionConfigProvider : public ExtensionConfigProviderBase {
+public:
+  virtual ~ExtensionConfigProvider() = default;


You don't need this since you already have a virtual base class

snowp · 2021-02-17T17:35:05Z

include/envoy/config/extension_config_provider.h

+ * the extension configuration discovery service. Dynamically updated extension
+ * configurations may share subscriptions across extension config providers.


I know this is an old comment but do you think you could elaborate on what the last sentence here means? It's not immediately clear to me what it's talking about

snowp · 2021-02-17T17:37:50Z

source/common/config/utility.h

@@ -323,6 +323,20 @@ class Utility {
    return type;
  }

+  /**
+   * Get and check a factory from the registry by type URL.


by typed config instead? This line makes me think that the function takes a type url as parameter

mandarjog · 2021-03-10T02:34:22Z

@kyessenov are you picking this back up soon?

kyessenov · 2021-03-10T17:37:26Z

@mandarjog this will need to be redone again since there are many existing and upcoming merge conflicts.

github-actions · 2021-04-11T00:03:36Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2021-04-18T04:02:23Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Additional Description: Part of #14696 (comment). The goal is to templatize DynamicFilterConfigProviderImpl while having DynamicFilterConfigProviderImplBase, FilterConfigProviderManagerImplBase, and FilterConfigSubscription agnostic to HTTP vs. network filters. This is a step in that direction, following the approach of #14717. Because extra validation was added since that PR, the filter factory object must be created before onConfigUpdate in addition to during Risk Level: Low Testing: Existing (refactoring) Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A #14696 Signed-off-by: Taylor Barrella <tabarr@google.com>

kyessenov added 3 commits January 14, 2021 20:05

initial implementation

c1ec16b

Signed-off-by: Kuat Yessenov <kuat@google.com>

Merge remote-tracking branch 'upstream/master' into ecds_network_filters

5ca2fbe

more meta programming

9ab9fb5

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch assigned lambdai Jan 15, 2021

try another approach with compilers

3f096d9

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov force-pushed the ecds_network_filters branch from 080a7de to 3f096d9 Compare January 15, 2021 19:14

kyessenov added 3 commits January 15, 2021 12:55

continue refactor

5848aa1

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix tests

3a76bf6

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix stat prefix

171bae7

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov marked this pull request as ready for review January 15, 2021 22:23

kyessenov requested review from alyssawilk, ggreenway and mattklein123 as code owners January 15, 2021 22:23

Base automatically changed from master to main January 15, 2021 23:02

kyessenov changed the title ~~[WIP] ecds: network filter support~~ ecds: network filter support Jan 16, 2021

lambdai reviewed Jan 18, 2021

View reviewed changes

lambdai suggested changes Jan 18, 2021

View reviewed changes

rgs1 mentioned this pull request Jan 19, 2021

Revert "network filters: avoid unnecessary std::shared_ptrs (#14711)" #14755

Merged

lambdai suggested changes Jan 28, 2021

View reviewed changes

lambdai approved these changes Feb 11, 2021

View reviewed changes

lambdai removed their assignment Feb 11, 2021

mattklein123 assigned mattklein123 and snowp Feb 14, 2021

snowp suggested changes Feb 18, 2021

View reviewed changes

repokitteh-read-only bot added the waiting:any label Feb 18, 2021

repokitteh-read-only bot removed the waiting:any label Mar 10, 2021

mattklein123 added the waiting label Mar 11, 2021

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 11, 2021

github-actions bot closed this Apr 18, 2021

This was referenced Aug 12, 2021

ecds: add support for network filters #14696

Closed

ecds: create shared base class for DynamicFilterConfigProviderImpl #17735

Merged

tbarrella mentioned this pull request Sep 9, 2021

ecds: make onConfigUpdate generic over filter type #18061

Merged

		* the extension configuration discovery service. Dynamically updated extension
		* configurations may share subscriptions across extension config providers.

ecds: network filter support #14717

ecds: network filter support #14717

Conversation

kyessenov commented Jan 15, 2021 • edited Loading

htuch commented Jan 15, 2021

kyessenov commented Jan 15, 2021

kyessenov commented Jan 15, 2021

kyessenov commented Jan 16, 2021

htuch commented Jan 17, 2021

lambdai commented Jan 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Jan 18, 2021

kyessenov commented Jan 18, 2021

rgs1 commented Jan 18, 2021

lambdai commented Jan 19, 2021

kyessenov commented Jan 19, 2021

lambdai commented Jan 19, 2021

mattklein123 commented Jan 19, 2021

lambdai commented Jan 19, 2021

kyessenov commented Jan 21, 2021

lambdai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Jan 28, 2021

lambdai commented Feb 11, 2021

lambdai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandarjog commented Mar 10, 2021

kyessenov commented Mar 10, 2021

github-actions bot commented Apr 11, 2021

github-actions bot commented Apr 18, 2021

kyessenov commented Jan 15, 2021 •

edited

Loading