config: support determine terminal filter at runtime #15803

qinggniq · 2021-04-01T09:33:17Z

Currently we validate terminal filter before getting real protobuf message, now envoy can determine a filter whether is a terminal filter base on the protobuf message.

Commit Message: config: support determine terminal filter at runtime
Additional Description:
Risk Level: Low
Testing: TBD
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: qinggniq <[email protected]>

Signed-off-by: qinggniq <[email protected]> add a test Signed-off-by: qinggniq <[email protected]> add test Signed-off-by: qinggniq <[email protected]> add test Signed-off-by: qinggniq <[email protected]>

Signed-off-by: qinggniq <[email protected]>

Signed-off-by: qinggniq <[email protected]> fix typo Signed-off-by: qinggniq <[email protected]>

Signed-off-by: qinggniq <[email protected]>

snowp

Thanks this makes sense, just some questions around how we'll handle the dynamic case

snowp · 2021-04-01T21:02:33Z

include/envoy/filter/http/filter_config_provider.h

@@ -32,11 +32,14 @@ class FilterConfigProviderManager {
   * @param filter_config_name the filter config resource name.
   * @param factory_context is the context to use for the filter config provider.
   * @param stat_prefix supplies the stat_prefix to use for the provider stats.
+   * @param last_filter_in_current_config indicate the filter position at filter chain


maybe something like indicates whether this filter is the last filter in the configured chain

snowp · 2021-04-01T21:03:34Z

include/envoy/filter/http/filter_config_provider.h

   */
  virtual FilterConfigProviderPtr createDynamicFilterConfigProvider(
      const envoy::config::core::v3::ExtensionConfigSource& config_source,
      const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context,
-      const std::string& stat_prefix) PURE;
+      const std::string& stat_prefix, bool last_filter_in_current_config,


maybe just last_filter? or last_filter_in_filter_chain? not sure what "current" is referring to

last_filter_in_filter_chain better.

snowp · 2021-04-01T21:04:23Z

source/common/filter/http/filter_config_discovery_impl.cc

@@ -64,6 +69,12 @@ void DynamicFilterConfigProviderImpl::onConfigUpdate(Envoy::Http::FilterFactoryC
        this->current_config_ = config;
      });
 }
+void DynamicFilterConfigProviderImpl::validateTermianlFilter(const std::string& name,


typo in name

snowp · 2021-04-01T21:04:56Z

source/common/filter/http/filter_config_discovery_impl.cc

@@ -106,7 +117,7 @@ void FilterConfigSubscription::onConfigUpdate(
    throw EnvoyException(fmt::format(
        "Unexpected number of resources in ExtensionConfigDS response: {}", resources.size()));
  }
-  const auto& filter_config = dynamic_cast<const envoy::config::core::v3::TypedExtensionConfig&>(
+  auto& filter_config = dynamic_cast<const envoy::config::core::v3::TypedExtensionConfig&>(


why remove the const here? we cast to a const ref

sorry, did not notice.

snowp · 2021-04-01T21:07:29Z

source/common/filter/http/filter_config_discovery_impl.h

@@ -121,6 +129,7 @@ class FilterConfigSubscription
  absl::optional<Envoy::Http::FilterFactoryCb> last_config_{absl::nullopt};
  std::string last_type_url_;
  std::string last_version_info_;
+  envoy::config::core::v3::TypedExtensionConfig last_filter_config_;


I think we intentionally avoid storing the entire filter config, can we just track whether the last filter passed the terminal filter check?

Yes, we only need it_terminal_filter

Signed-off-by: qinggniq <[email protected]>

snowp · 2021-04-05T18:06:33Z

Can you merge main to resolve conflicts and look at getting CI green? Thanks

Signed-off-by: qinggniq <[email protected]>

qinggniq · 2021-04-06T10:22:25Z

@snowp I think the remain ci flakes is due to not update https://github.com/envoyproxy/envoy-filter-example, should I give a pr to that repo?

snowp

Thanks, just a couple comments

CI seems to still be broken

/wait

snowp · 2021-04-08T18:26:20Z

test/common/filter/http/filter_config_discovery_impl_test.cc

@@ -353,7 +355,6 @@ TEST_F(FilterConfigDiscoveryImplTest, DualProviders) {
 TEST_F(FilterConfigDiscoveryImplTest, DualProvidersInvalid) {
  InSequence s;
  setup();
-  auto provider2 = createProvider("foo", true, false);


is this intentional?

By mistake.

snowp · 2021-04-08T18:27:34Z

test/integration/extension_discovery_integration_test.cc

@@ -600,6 +608,28 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithoutDefault) {
  EXPECT_EQ("500", response->headers().getStatusValue());
 }

+// test when terminal filter config not put at last config position


nit: proper sentence, proper casing + punctuation

snowp · 2021-04-08T18:28:23Z

test/integration/filters/set_response_code_filter_config.proto

@@ -8,4 +8,5 @@ message SetResponseCodeFilterConfig {
  string prefix = 1;
  uint32 code = 2 [(validate.rules).uint32 = {lt: 600 gte: 200}];
  string body = 3;
+  bool is_terminal_filter = 4;


I would do this in a different test filter instead of overloading the existing one

Signed-off-by: qinggniq <[email protected]>

snowp

Thanks this LGTM. Can you check CI? I think the asan/tsan ones are legit

snowp · 2021-04-14T17:50:53Z

Oh actually no those are the filter example ones you mentioned, let me look into how we can fix this

snowp · 2021-04-14T19:04:58Z

@qinggniq Can you make a PR to envoy-filter-example and point the SHA in ci/filter_example_setup.sh to the commit in the PR? That should let this PR test against the envoy-filter-example PR

Signed-off-by: qinggniq <[email protected]>

qinggniq · 2021-04-15T06:39:28Z

/retest

repokitteh-read-only · 2021-04-15T06:39:32Z

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

🐱

Caused by: a #15803 (comment) was created by @qinggniq.

see: more, trace.

Signed-off-by: qinggniq <[email protected]>

qinggniq · 2021-04-15T08:09:40Z

The failures of ci asan and tsan seem due to #15972 (comment).

Signed-off-by: qinggniq <[email protected]>

qinggniq · 2021-04-16T07:37:57Z

/retest

qinggniq · 2021-04-16T07:38:00Z

/retest

repokitteh-read-only · 2021-04-16T07:38:01Z

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

🐱

Caused by: a #15803 (comment) was created by @qinggniq.

see: more, trace.

repokitteh-read-only · 2021-04-16T07:38:04Z

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

🐱

Caused by: a #15803 (comment) was created by @qinggniq.

see: more, trace.

qinggniq · 2021-04-16T09:13:44Z

@snowp all cis are green, waiting for envoyproxy/envoy-filter-example#144 merged. I will change the commit SHA and URL at ci/setup_example_filter.sh to the latest commit.

snowp · 2021-04-19T14:38:59Z

I merged the filter example PR, can you update this to use the new commit and we'll get this landed? Thanks

Signed-off-by: qinggniq <[email protected]>

snowp

Thanks!

Currently we validate terminal filter before getting real protobuf message, now envoy can determine a filter whether is a terminal filter base on the protobuf message. Signed-off-by: qinggniq <[email protected]> Signed-off-by: Gokul Nair <[email protected]>

qinggniq and others added 6 commits March 30, 2021 17:38

imporve doc

a33db77

Signed-off-by: qinggniq <[email protected]>

Update README.md

e93cfb4

Signed-off-by: qinggniq <[email protected]>

fix typo

418554b

Signed-off-by: qinggniq <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into main

ed05ce5

Signed-off-by: qinggniq <[email protected]>

change signature of isTermianlFilter

bcf50b2

Signed-off-by: qinggniq <[email protected]> add a test Signed-off-by: qinggniq <[email protected]> add test Signed-off-by: qinggniq <[email protected]> add test Signed-off-by: qinggniq <[email protected]>

remove code

e89113f

Signed-off-by: qinggniq <[email protected]>

qinggniq requested review from alyssawilk, ggreenway, htuch, lizan, mattklein123, snowp and zuercher as code owners April 1, 2021 09:33

qinggniq added 2 commits April 1, 2021 18:32

cover more cases to validate config

3bebde5

Signed-off-by: qinggniq <[email protected]> fix typo Signed-off-by: qinggniq <[email protected]>

fix test flakes

18bf6ed

Signed-off-by: qinggniq <[email protected]>

mattklein123 assigned snowp Apr 1, 2021

snowp suggested changes Apr 1, 2021

View reviewed changes

qinggniq added 4 commits April 2, 2021 13:28

fix build flakes

3d81b83

Signed-off-by: qinggniq <[email protected]>

fix typo

55dba70

Signed-off-by: qinggniq <[email protected]>

fix build flakes

af0ecf2

Signed-off-by: qinggniq <[email protected]>

fix: prevent Werror=overloaded-virtual warnig

7b2214d

Signed-off-by: qinggniq <[email protected]>

qinggniq added 3 commits April 6, 2021 11:13

merge from master

f5c5371

Signed-off-by: qinggniq <[email protected]>

fix test flakes

a6dee8f

Signed-off-by: qinggniq <[email protected]>

refactor the signature of isTerminalFilter

0e56490

Signed-off-by: qinggniq <[email protected]>

snowp suggested changes Apr 8, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 8, 2021

qinggniq added 2 commits April 9, 2021 11:46

fix extension_discovery test

e7075fa

Signed-off-by: qinggniq <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into main

c98dd0e

Signed-off-by: qinggniq <[email protected]>

qinggniq requested a review from snowp April 14, 2021 09:05

snowp previously approved these changes Apr 14, 2021

View reviewed changes

qinggniq mentioned this pull request Apr 15, 2021

config: support determine terminal filter at runtime envoyproxy/envoy-filter-example#144

Merged

fix: envoy-filter-example sha

f3dde43

Signed-off-by: qinggniq <[email protected]>

qinggniq dismissed snowp’s stale review via f3dde43 April 15, 2021 02:43

fix: change envoy-filter-exmaple url

dbff93b

Signed-off-by: qinggniq <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into main

953f9b2

Signed-off-by: qinggniq <[email protected]>

qinggniq mentioned this pull request Apr 15, 2021

test: making assert required #15972

Merged

qinggniq added 3 commits April 16, 2021 11:21

merge from upstream

ff8a04b

Signed-off-by: qinggniq <[email protected]>

fix: change sha to latest

3f9afe6

Signed-off-by: qinggniq <[email protected]>

fix: build flakes

23aade7

Signed-off-by: qinggniq <[email protected]>

snowp added the waiting label Apr 19, 2021

repokitteh-read-only bot removed the waiting label Apr 19, 2021

fix: change envoy-filter-example sha to latest commit

af1a9b2

Signed-off-by: qinggniq <[email protected]>

qinggniq force-pushed the main branch from 3cc387a to af1a9b2 Compare April 19, 2021 15:11

snowp approved these changes Apr 19, 2021

View reviewed changes

snowp merged commit e1935de into envoyproxy:main Apr 19, 2021

qinggniq deleted the main branch April 20, 2021 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: support determine terminal filter at runtime #15803

config: support determine terminal filter at runtime #15803

qinggniq commented Apr 1, 2021 •

edited

Loading

snowp left a comment

snowp Apr 1, 2021

qinggniq Apr 2, 2021

snowp Apr 1, 2021

qinggniq Apr 2, 2021

snowp Apr 1, 2021

qinggniq Apr 2, 2021

snowp Apr 1, 2021

qinggniq Apr 2, 2021

snowp Apr 1, 2021

qinggniq Apr 2, 2021

snowp commented Apr 5, 2021

qinggniq commented Apr 6, 2021

snowp left a comment

snowp Apr 8, 2021

qinggniq Apr 9, 2021

snowp Apr 8, 2021

qinggniq Apr 9, 2021

snowp Apr 8, 2021

qinggniq Apr 9, 2021

snowp left a comment

snowp commented Apr 14, 2021

snowp commented Apr 14, 2021

qinggniq commented Apr 15, 2021

repokitteh-read-only bot commented Apr 15, 2021

qinggniq commented Apr 15, 2021

qinggniq commented Apr 16, 2021

qinggniq commented Apr 16, 2021

repokitteh-read-only bot commented Apr 16, 2021

repokitteh-read-only bot commented Apr 16, 2021

qinggniq commented Apr 16, 2021

snowp commented Apr 19, 2021

snowp left a comment

config: support determine terminal filter at runtime #15803

config: support determine terminal filter at runtime #15803

Conversation

qinggniq commented Apr 1, 2021 • edited Loading

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

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

snowp commented Apr 5, 2021

qinggniq commented Apr 6, 2021

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

Choose a reason for hiding this 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

snowp commented Apr 14, 2021

snowp commented Apr 14, 2021

qinggniq commented Apr 15, 2021

repokitteh-read-only bot commented Apr 15, 2021

qinggniq commented Apr 15, 2021

qinggniq commented Apr 16, 2021

qinggniq commented Apr 16, 2021

repokitteh-read-only bot commented Apr 16, 2021

repokitteh-read-only bot commented Apr 16, 2021

qinggniq commented Apr 16, 2021

snowp commented Apr 19, 2021

snowp left a comment

Choose a reason for hiding this comment

qinggniq commented Apr 1, 2021 •

edited

Loading