ecds: make onConfigUpdate generic over filter type by tbarrella · Pull Request #18061 · envoyproxy/envoy

tbarrella · 2021-09-09T22:59:52Z

Commit Message:
ecds: make onConfigUpdate generic over filter type

Signed-off-by: Taylor Barrella tabarr@google.com

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>

zuercher · 2021-09-10T19:06:15Z

/assign-from @envoyproxy/first-pass-reviewers

repokitteh-read-only · 2021-09-10T19:06:19Z

@envoyproxy/first-pass-reviewers assignee is @adisuissa

🐱

Caused by: a #18061 (comment) was created by @zuercher.

see: more, trace.

source/common/filter/config_discovery_impl.h

Signed-off-by: Taylor Barrella <tabarr@google.com>

adisuissa

Overall refactor LGTM, thanks!
Left a couple of comments.
/wait

source/common/filter/config_discovery_impl.h

lambdai · 2021-09-16T05:05:26Z

source/common/filter/config_discovery_impl.h

+    const absl::optional<Envoy::Http::FilterFactoryCb> default_config =
+        default_configuration_ ? absl::make_optional(factory_cb_(default_configuration_.value()))
+                               : absl::nullopt;


Is this default_config need to be constructed on every onConfigRemoved?
We should be able to save the factory_cb_(default_configuration_.value())

I'd prefer not to save this extra field on top of the proto just for optimization. The proto already has to be saved for applyDefaultConfiguration()

lambdai · 2021-09-16T05:12:01Z

source/common/filter/config_discovery_impl.h

-        [config = default_configuration_](OptRef<ThreadLocalConfig> tls) { tls->config_ = config; },
-        [this, applied_on_all_threads]() {
+        [config = default_config](OptRef<ThreadLocalConfig> tls) { tls->config_ = config; },
+        [this, default_config, applied_on_all_threads]() {


I am skeptical if the captured this could survive when the main_callback is invoked.
Any theory to support this will outlive?

I'm not sure, although is this out of the scope of this PR?

yeah I'm skeptical too - the same thing might happen as grpc access loggers (currently being fixed).

@mathetake can you share a reference here for my benefit? Thanks.

#18081 and #18067 - basically I am worried about the case where DynamicFilterConfigProviderImpl has been deleted before this callback would be called on all threads. Then current_config_ (which I imagine is used in applied_on_all_threads below) would be invalid address by the time when the callback is called.

Yeah, this seems a plausible concern. I'm tempted to merge this though if @tbarrella wants to take this to a followup PR, since I don't see anything in this PR that makes the existing situation worse.

Signed-off-by: Taylor Barrella <tabarr@google.com>

tbarrella · 2021-09-16T20:52:27Z

@adisuissa please let me know your thoughts on copy elision vs. unique_ptr

Signed-off-by: Taylor Barrella <tabarr@google.com>

tbarrella · 2021-09-20T19:16:21Z

/retest

repokitteh-read-only · 2021-09-20T19:16:25Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18061 (comment) was created by @tbarrella.

see: more, trace.

tbarrella · 2021-09-20T19:49:34Z

/retest

repokitteh-read-only · 2021-09-20T19:49:38Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18061 (comment) was created by @tbarrella.

see: more, trace.

htuch

Makes sense, would be good to have @kyessenov take a pass if possible as well.
/wait

envoy/config/extension_config_provider.h

source/common/filter/config_discovery_impl.h

htuch · 2021-09-22T02:30:25Z

source/common/filter/config_discovery_impl.h

-        [config = default_configuration_](OptRef<ThreadLocalConfig> tls) { tls->config_ = config; },
-        [this, applied_on_all_threads]() {
+        [config = default_config](OptRef<ThreadLocalConfig> tls) { tls->config_ = config; },
+        [this, default_config, applied_on_all_threads]() {


@mathetake can you share a reference here for my benefit? Thanks.

Signed-off-by: Taylor Barrella <tabarr@google.com>

htuch

LGTM modulo comment thread on cleanup state; I think we can take that to a different PR.

htuch · 2021-09-26T16:46:40Z

source/common/filter/config_discovery_impl.h

-        [config = default_configuration_](OptRef<ThreadLocalConfig> tls) { tls->config_ = config; },
-        [this, applied_on_all_threads]() {
+        [config = default_config](OptRef<ThreadLocalConfig> tls) { tls->config_ = config; },
+        [this, default_config, applied_on_all_threads]() {


Yeah, this seems a plausible concern. I'm tempted to merge this though if @tbarrella wants to take this to a followup PR, since I don't see anything in this PR that makes the existing situation worse.

tbarrella · 2021-10-01T18:06:44Z

/retest

repokitteh-read-only · 2021-10-01T18:06:48Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18061 (comment) was created by @tbarrella.

see: more, trace.

source/common/filter/config_discovery_impl.cc

kyessenov · 2021-10-01T20:03:22Z

Sorry, I was OoO but have time to review this now.

Signed-off-by: Taylor Barrella <tabarr@google.com>

source/common/filter/config_discovery_impl.h

kyessenov

Thanks, I think this captures and addresses by comment.

tbarrella · 2021-10-20T17:13:56Z

Thank you for the reviews! Anything else that needs to be done to merge?

tbarrella · 2021-10-25T22:56:54Z

Sorry, @adisuissa @htuch does this look good?

htuch · 2021-10-27T04:24:39Z

All my concerns are addressed. @adisuissa any further comments before merging?

adisuissa

LGTM.

* main: (221 commits) deps: Bump `protobuf` -> 3.19.0 (envoyproxy#18471) tooling: auto-assign dependency shephards (envoyproxy#18794) clang-tidy: Return from diff fun if empty diff (envoyproxy#18815) repokitteh: Block PRs pending deps approval (envoyproxy#18814) deps: Bump `org_llvm_llvm` -> 12.0.1, `com_github_wavm_wavm` -> 9ffd3e2 (envoyproxy#18747) dns resolvers: add All lookup mode (envoyproxy#18464) doc: fix link formatting for TLS session_timeout (envoyproxy#18790) ext_authz: Set response flag and code details to UAEX when denied (envoyproxy#18740) socket options: add support for directly creating ipv4/ipv6 pairs (envoyproxy#18769) ecds: make onConfigUpdate generic over filter type (envoyproxy#18061) bazel: update CMake instructions in EXTERNAL_DEPS.md (envoyproxy#18799) upstream: fix typo in comment (envoyproxy#18798) runtime: removing envoy.reloadable_features.grpc_json_transcoder_adhere_to_buffer_limits (envoyproxy#18696) bazel: Add CC=clang to clang configuration (envoyproxy#18732) fix error request id in the dubbbo local reply (envoyproxy#18741) event: assert the case of both read and closed event registered (envoyproxy#18265) tcp proxy connect tunneling: improved testing (envoyproxy#18784) deps: Bump `protoc-gen-validate` -> 0.6.2 (envoyproxy#18742) deps: Bump `rules_pkg` -> ad57589 (envoyproxy#18746) bazel: copy .bazelversion for envoy filter examples (envoyproxy#18730) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

tbarrella added 2 commits September 9, 2021 22:40

ecds: make onConfigUpdate generic over filter type

c72c022

Signed-off-by: Taylor Barrella <tabarr@google.com>

fix segfault

0a3540d

Signed-off-by: Taylor Barrella <tabarr@google.com>

repokitteh-read-only bot assigned adisuissa Sep 10, 2021

kyessenov reviewed Sep 10, 2021

View reviewed changes

source/common/filter/config_discovery_impl.h Outdated Show resolved Hide resolved

tbarrella added 2 commits September 10, 2021 21:01

fix capture

fba80d3

Signed-off-by: Taylor Barrella <tabarr@google.com>

undo comment wrapping

3c93afc

Signed-off-by: Taylor Barrella <tabarr@google.com>

adisuissa reviewed Sep 15, 2021

View reviewed changes

source/common/filter/config_discovery_impl.h Outdated Show resolved Hide resolved

source/common/filter/config_discovery_impl.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Sep 15, 2021

lambdai reviewed Sep 16, 2021

View reviewed changes

tbarrella added 2 commits September 16, 2021 20:30

pass reference

8ede15f

Signed-off-by: Taylor Barrella <tabarr@google.com>

* instead of .value() (https://abseil.io/tips/181)

37f849c

Signed-off-by: Taylor Barrella <tabarr@google.com>

repokitteh-read-only bot removed the waiting label Sep 16, 2021

alyssawilk assigned htuch Sep 20, 2021

use reference

ad0735a

Signed-off-by: Taylor Barrella <tabarr@google.com>

tbarrella requested a review from adisuissa September 20, 2021 17:43

htuch suggested changes Sep 22, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 22, 2021

tbarrella added 2 commits September 22, 2021 17:27

comment wording

fd28cda

Signed-off-by: Taylor Barrella <tabarr@google.com>

rename variable

1d81ce5

Signed-off-by: Taylor Barrella <tabarr@google.com>

repokitteh-read-only bot removed the waiting label Sep 22, 2021

tbarrella requested review from htuch and kyessenov September 22, 2021 18:08

htuch reviewed Sep 26, 2021

View reviewed changes

tbarrella requested a review from htuch October 1, 2021 18:07

kyessenov reviewed Oct 1, 2021

View reviewed changes

source/common/filter/config_discovery_impl.cc Outdated Show resolved Hide resolved

source/common/filter/config_discovery_impl.cc Show resolved Hide resolved

mattklein123 added the waiting label Oct 6, 2021

respond to review

cfc933c

Signed-off-by: Taylor Barrella <tabarr@google.com>

repokitteh-read-only bot removed the waiting label Oct 12, 2021

tbarrella requested a review from kyessenov October 12, 2021 19:47

tbarrella added 2 commits October 18, 2021 18:45

remove unnecessary validation

0c6b08f

Signed-off-by: Taylor Barrella <tabarr@google.com>

pass message instead of typed_config

6141581

Signed-off-by: Taylor Barrella <tabarr@google.com>

kyessenov reviewed Oct 19, 2021

View reviewed changes

source/common/filter/config_discovery_impl.h Show resolved Hide resolved

kyessenov reviewed Oct 19, 2021

View reviewed changes

adisuissa approved these changes Oct 28, 2021

View reviewed changes

htuch approved these changes Oct 28, 2021

View reviewed changes

htuch merged commit 4a2b01f into envoyproxy:main Oct 28, 2021

tbarrella deleted the ecds2 branch October 28, 2021 17:58

Conversation

tbarrella commented Sep 9, 2021

Uh oh!

zuercher commented Sep 10, 2021

Uh oh!

repokitteh-read-only bot commented Sep 10, 2021

Uh oh!

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbarrella commented Sep 16, 2021

Uh oh!

tbarrella commented Sep 20, 2021

Uh oh!

repokitteh-read-only bot commented Sep 20, 2021

Uh oh!

tbarrella commented Sep 20, 2021

Uh oh!

repokitteh-read-only bot commented Sep 20, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbarrella commented Oct 1, 2021

Uh oh!

repokitteh-read-only bot commented Oct 1, 2021

Uh oh!

Uh oh!

Uh oh!

kyessenov commented Oct 1, 2021

Uh oh!

Uh oh!

kyessenov left a comment

Choose a reason for hiding this comment

Uh oh!

tbarrella commented Oct 20, 2021

Uh oh!

tbarrella commented Oct 25, 2021

Uh oh!

htuch commented Oct 27, 2021

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants