network filters: add `injectDataToFilterChain(data, end_stream)` method to network filter callbacks by yskopets · Pull Request #6750 · envoyproxy/envoy

yskopets · 2019-04-29T23:36:00Z

Description: Similarly to HTTP use case, add injectDataToFilterChain(data, end_stream) method
to network filter callbacks (see #6640).

This method should be used in advanced cases in which a filter needs full control over
how subsequent filters view data.

Using this method allows a filter to buffer data (or not) and then periodically inject data
to subsequent filters, indicating end_stream at an appropriate time.

Example use cases include:

low bandwidth simulation at L4 (see Slow network / local rate limit HTTP filter #5942)
piping request/response data through an out-of-process filter (see Add ability to execute Filters over gRPC to mutate payload. #5209)

Risk Level: Medium
Testing: unit tests, integration tests
Docs Changes: N/A
Release Notes: N/A
Fixes #6640

…od to network filter callbacks Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

mattklein123

In general this looks great, thanks for taking this on. A few high level comments. Also, can you please work on an integration test maybe using tcp_proxy as well as a fake integration test filter that does injection? We have other test filter examples in the code base now. Thank you!

/wait

source/common/network/filter_manager_impl.h

source/extensions/filters/network/mongo_proxy/proxy.h

source/common/network/filter_manager_impl.h

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

yskopets · 2019-05-03T18:50:02Z

@mattklein123 Hi Matt!
I've addressed all comments. Please take another look.

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

mattklein123

Thanks this is really impressive work. Good stuff and thanks a ton for adding the integration tests. Can you merge master to fix CI. Also, please run the new integration tests with --runs_per_test=1000 to take a look for flakes. The timing/throttling tests are particularly susceptible to flakes. Thank you!

/wait

include/envoy/network/filter.h

source/common/network/filter_manager_impl.h

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

…ilterChain` and `injectWriteDataToFilterChain` Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

…ta-into-network-filter-chain Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

yskopets · 2019-05-06T23:05:23Z

@mattklein123 I'm running tests on my Mac using the following command:

bazel test --jobs=8 --runs_per_test=1000 //test/integration:filter_manager_integration_test

occasionally, 1-2 test runs fail with

[test/integration/http_integration.cc:313] assert failure: result. Details: Timed out waiting for new connection.

which must be caused by

uint64_t HttpIntegrationTest::waitForNextUpstreamRequest(const std::vector<uint64_t>& upstream_indices) {
...
      result = fake_upstreams_[upstream_index]->waitForHttpConnection(
          *dispatcher_, fake_upstream_connection_, TestUtility::DefaultTimeout,
          max_request_headers_kb_);
...
}

what should I do?

mattklein123 · 2019-05-06T23:07:47Z

@yskopets you will need to debug the race condition that is causing that to happen. @alyssawilk is our integration test wizard and she has a guide that is helpful here: https://github.com/envoyproxy/envoy/tree/master/test/integration#reproducing-test-flakes

/wait

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

…ta-into-network-filter-chain Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

yskopets · 2019-05-07T23:08:55Z

@mattklein123 Hi Matt,

Sorry, I don't see where the issue might be.

I don't think it's caused by timer events in a test filter.

When I run the same way one of the existing tests, I observe exactly the same behaviour - 1-2 test runs fail.

E.g.

bazel test --jobs=8 --runs_per_test=1000 //test/integration:http2_integration_test

[critical][assert] [test/integration/http_integration.cc:313] assert failure: result. Details: Timed out waiting for new connection.

mattklein123 · 2019-05-07T23:19:14Z

When I run the same way one of the existing tests, I observe exactly the same behaviour - 1-2 test runs fail.

OK that's fine. I'm mostly just interested in whether your tests are flaking. You can also just run your test specifically in a loop using --gtest_filter inside a --test_arg bazel CLI option. I will take another pass tomorrow.

yskopets · 2019-05-07T23:23:29Z

Thank you!

lizan

Great work!

Risk Level: Low

I would at least rate this PR as Med risk or more, the change is large and involving the core of connection management.

test/integration/filter_manager_integration_test.cc

mattklein123

Thanks, very impressive work. Can you merge master to pick up CI flake fixes?

/wait

mattklein123 · 2019-05-08T23:00:46Z

I would at least rate this PR as Med risk or more, the change is large and involving the core of connection management.

Agreed, good call out @lizan.

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

…ta-into-network-filter-chain Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

yskopets · 2019-05-09T10:29:31Z

@lizan @mattklein123 I've changed risk level to "Medium" (since it's mostly a refactoring and majority of the new code are tests).

Let me know if I should update it to "High".

mattklein123

Thank you!

* master: (88 commits) upstream: Null-deref on TCP health checker if setsockopt fails (envoyproxy#6793) ci: switch macOS CI to azure pipelines (envoyproxy#6889) os syscalls lib: break apart syscalls used for hot restart (envoyproxy#6880) Kafka codec: precompute request size before serialization, so we do n… (envoyproxy#6862) upstream: move static and strict_dns clusters to dedicated files (envoyproxy#6886) Rollforward of api: Add total_issued_requests to Upstream Locality and Endpoint Stats. (envoyproxy#6692) (envoyproxy#6784) fix explicit constructor in copy-initialization (envoyproxy#6884) stats: use tag iterator rather than constructing the tag-array and searching that. (envoyproxy#6853) common: use unscoped build target in generate_version_linkstamp (envoyproxy#6877) Addendum to envoyproxy#6778 (envoyproxy#6882) ci: add minimum Linux build for Azure Pipelines (envoyproxy#6881) grpc: utilities for inter-converting grpc::ByteBuffer and Buffer::Instance. (envoyproxy#6732) upstream: allow excluding hosts from lb calculations until initial health check (envoyproxy#6794) stats: prevent unused counters from leaking across hot restart (envoyproxy#6850) network filters: add `injectDataToFilterChain(data, end_stream)` method to network filter callbacks (envoyproxy#6750) delete things that snuck back in (envoyproxy#6873) config: scoped rds (2b): support delta APIs in ConfigProvider framework (envoyproxy#6781) string == string! (envoyproxy#6868) config: add mssing imports to delta_subscription_state (envoyproxy#6869) protobuf: add missing default case to enum (envoyproxy#6870) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

network filters: add injectDataToFilterChain(data, end_stream) meth…

6c4f749

…od to network filter callbacks Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

yskopets requested review from mattklein123 and snowp as code owners April 29, 2019 23:36

mattklein123 self-assigned this Apr 29, 2019

Kick CI

4c5869c

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

mattklein123 requested changes May 1, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 1, 2019

yskopets added 2 commits May 2, 2019 08:48

code review: delete explicit destructors

e74299b

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

code review: make initializeWriteFilterCallbacks() optional

e6c9bba

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

repokitteh-read-only bot removed the waiting label May 2, 2019

mattklein123 added the waiting label May 2, 2019

repokitteh-read-only bot removed the waiting label May 3, 2019

yskopets added 2 commits May 3, 2019 22:08

code review: add integration tests

5775765

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

code review: rework FilterManagerCallbacks into FilterManagerConnection

d8a984e

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

yskopets force-pushed the feature/inject-data-into-network-filter-chain branch from 5e0e599 to d8a984e Compare May 3, 2019 20:09

yskopets added 2 commits May 6, 2019 12:46

code review: fix typo

6879c6c

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

code review: add more test checks

1c51559

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

mattklein123 requested changes May 6, 2019

View reviewed changes

include/envoy/network/filter.h Outdated Show resolved Hide resolved

include/envoy/network/filter.h Outdated Show resolved Hide resolved

source/common/network/filter_manager_impl.h Show resolved Hide resolved

source/common/network/filter_manager_impl.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label May 6, 2019

yskopets added 5 commits May 7, 2019 00:35

code review: fix code style

8494988

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

code review: remove redundant destructors

baded41

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

code review: rename injectDataToFilterChain into `injectReadDataToF…

2e3bd9b

…ilterChain` and `injectWriteDataToFilterChain` Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

code review: remove BufferSource that has become obsolete

4a21e3d

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

Merge remote-tracking branch 'upstream/master' into feature/inject-da…

b3bcdf1

…ta-into-network-filter-chain Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

repokitteh-read-only bot removed the waiting label May 6, 2019

repokitteh-read-only bot added the waiting label May 6, 2019

Kick CI

f4f9f7a

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

Kick CI

341f60d

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

repokitteh-read-only bot removed the waiting label May 7, 2019

Merge remote-tracking branch 'upstream/master' into feature/inject-da…

ed27bc3

…ta-into-network-filter-chain Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

mattklein123 added the waiting label May 7, 2019

test: make auxiliary test filters configurable (to address flakiness)

8659d7b

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

repokitteh-read-only bot removed the waiting label May 7, 2019

lizan reviewed May 8, 2019

View reviewed changes

test/integration/filter_manager_integration_test.cc Outdated Show resolved Hide resolved

mattklein123 reviewed May 8, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 8, 2019

yskopets added 2 commits May 9, 2019 12:13

code review: avoid "static initialization order 'fiasco' (problem)"

2ec0791

Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

Merge remote-tracking branch 'upstream/master' into feature/inject-da…

ff0d9b0

…ta-into-network-filter-chain Signed-off-by: Yaroslav Skopets <y.skopets@gmail.com>

repokitteh-read-only bot removed the waiting label May 9, 2019

mattklein123 approved these changes May 9, 2019

View reviewed changes

mattklein123 merged commit 72129db into envoyproxy:master May 9, 2019

mattklein123 mentioned this pull request May 10, 2019

Allow filters to write through downstream filters only #3343

Closed

Conversation

yskopets commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yskopets commented May 3, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yskopets commented May 6, 2019

Uh oh!

mattklein123 commented May 6, 2019

Uh oh!

yskopets commented May 7, 2019

Uh oh!

mattklein123 commented May 7, 2019

Uh oh!

yskopets commented May 7, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented May 8, 2019

Uh oh!

yskopets commented May 9, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yskopets commented Apr 29, 2019 •

edited

Loading