core: integration with Envoy's API listener #616

junr03 · 2019-12-19T22:07:20Z

Description: this PR integrates Envoy's ApiListener to replace Envoy Mobile's use of the AsyncClient. Doing so gives Envoy Mobile access to all L7 facilities provided by Envoy's Http Connection Manager; including but no limited to the L7 filters.
Risk Level: HIGH! This PR changes the underlying code for Envoy Mobile's core functionality
Testing: unit tests updated for the dispatcher, integration tests, and e2e tests with the example apps. Additionally, the ApiListener code has been unit and integration tested in Envoy.

Fixes #543

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2019-12-19T22:07:44Z

FYI @rebello95 @buildbreaker

Signed-off-by: Jose Nino <jnino@lyft.com>

library/common/config_template.cc

Signed-off-by: Jose Nino <jnino@lyft.com>

Description: this PR introduces the initial implementation of an Api Listener based on the proto configuration merged in #8170. Notably, this PR introduces the ability to add only _one_ api listener via _bootstrap config only_. This decision was made in order to iterate into more complex setups (multiple listeners, LDS supplied listeners) in subsequent PRs. Moreover, the API listener is created in the context of Envoy's main thread not worker threads. A first use of this Api Listener can be seen in envoyproxy/envoy-mobile#616. Risk Level: low, only used in Envoy Mobile. The risk here is about building something generally useful and flexible. Note however that a couple of things were rejiggered in the HCM. Testing: unit and integration tests. Additional testing in https://github.com/lyft/envoy-mobile. Docs Changes: Added inline comments and TODOs. Proto documentation is up-to-date. Release Notes: similar to doc changes. Signed-off-by: Jose Nino <jnino@lyft.com>

Description: this PR introduces the initial implementation of an Api Listener based on the proto configuration merged in envoyproxy/envoy#8170. Notably, this PR introduces the ability to add only _one_ api listener via _bootstrap config only_. This decision was made in order to iterate into more complex setups (multiple listeners, LDS supplied listeners) in subsequent PRs. Moreover, the API listener is created in the context of Envoy's main thread not worker threads. A first use of this Api Listener can be seen in envoyproxy/envoy-mobile#616. Risk Level: low, only used in Envoy Mobile. The risk here is about building something generally useful and flexible. Note however that a couple of things were rejiggered in the HCM. Testing: unit and integration tests. Additional testing in https://github.com/lyft/envoy-mobile. Docs Changes: Added inline comments and TODOs. Proto documentation is up-to-date. Release Notes: similar to doc changes. Signed-off-by: Jose Nino <jnino@lyft.com> Mirrored from https://github.com/envoyproxy/envoy @ 9b6260fcf6ee1299744b8e5c76c1e6d9d36f7c89

Signed-off-by: Jose Nino <jnino@lyft.com>

stale · 2020-01-21T01:31:44Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

Signed-off-by: Jose Nino <jnino@lyft.com>

library/common/config_template.cc

library/common/http/dispatcher.cc

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2020-01-29T03:02:51Z

cc @mattklein123 in case you want to take a look as well.

goaway · 2020-01-29T17:13:51Z

library/common/engine.cc

-          http_dispatcher_->ready(server->dispatcher(), server->clusterManager());
+          auto api_listener = server->listenerManager().apiListener()->get().http();
+          ASSERT(api_listener.has_value());
+          http_dispatcher_->ready(server->dispatcher(), api_listener.value());


Is this additional delay (beyond construction) still necessary? We're no longer waiting on an initial round of DNS resolution and the filter dynamic forwarding filter contains built in buffering logic while we do wait. If we're unsure, maybe insert a TODO to follow up.

Still suggest adding a TODO and/or issue for this so we still have visibility to follow up.

library/common/http/dispatcher.cc

library/common/config_template.cc

library/objective-c/EnvoyHTTPStreamImpl.m

Signed-off-by: Jose Nino <jnino@lyft.com>

test/common/thread/lock_guard_test.cc

Signed-off-by: Jose Nino <jnino@lyft.com>

.github/workflows/core.yml

goaway · 2020-01-30T20:46:28Z

library/common/http/dispatcher.cc

+  // means that instead of efficiently queuing the deletes for one event in the event loop, all
+  // deletes here get queued up as individual posts.
+  TS_UNCHECKED_READ(event_dispatcher_)->post([direct_stream]() -> void {
+    ENVOY_LOG(debug, "[S{}] deferred deletion of stream", direct_stream->stream_handle_);


Just want to be sure that the optimizer won't optimize away this capture of direct_stream - even if ENVOY_LOG is compiled away. (Even if this would definitively/arguably be an optimizer bug, I've seen this sort of thing happen before.)

I really enjoy your eye for things that might end up being subtle bugs in the future. https://stackoverflow.com/a/12718425 and the comments below state that an explicit capture like above will not be optimized out even if the variable is not used in the lambda body.

library/common/thread/lock_guard.h

goaway

Great work. Thanks @junr03!

junr03 · 2020-01-30T21:11:09Z

Alright team @goaway @buildbreaker @rebello95 going to merge this. However we can't bump iOS until we fix envoyproxy/envoy#9875

As of #616, this is no longer used or necessary since it's handled by the router. Resolves #643. Signed-off-by: Michael Rebello <me@michaelrebello.com>

As of envoyproxy/envoy-mobile#616, this is no longer used or necessary since it's handled by the router. Resolves envoyproxy/envoy-mobile#643. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>

Jose Nino added 4 commits December 17, 2019 10:08

wip: headers, data, trailers working and unit tested

4e4ace3

Signed-off-by: Jose Nino <jnino@lyft.com>

wire up reset, and track stream state for cleanup

20d3e1e

Signed-off-by: Jose Nino <jnino@lyft.com>

terminal states updated. Missing tests.

8418ee9

Signed-off-by: Jose Nino <jnino@lyft.com>

cleanup

56914a0

Signed-off-by: Jose Nino <jnino@lyft.com>

Jose Nino added 5 commits December 23, 2019 14:53

update types and config

62ebd14

Signed-off-by: Jose Nino <jnino@lyft.com>

wip integration with envoy and dynamic forward proxy

fcb0870

Signed-off-by: Jose Nino <jnino@lyft.com>

use named listener

76aee26

Signed-off-by: Jose Nino <jnino@lyft.com>

api handle

f069c82

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

f38e77c

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 mentioned this pull request Dec 28, 2019

listener: first implementation of an Api Listener envoyproxy/envoy#9516

Merged

goaway reviewed Dec 30, 2019

View reviewed changes

library/common/config_template.cc Show resolved Hide resolved

Jose Nino added 5 commits January 7, 2020 10:24

updated interfaces to keep up with envoy changes

cec541f

Signed-off-by: Jose Nino <jnino@lyft.com>

first pass at closed state pushed down to the core

9b8b5e2

Signed-off-by: Jose Nino <jnino@lyft.com>

Merge branch 'master' into hcm-integration

1ab5cd6

Signed-off-by: Jose Nino <jnino@lyft.com>

always zero-init

cf69083

Signed-off-by: Jose Nino <jnino@lyft.com>

v3 types

85d1acf

Signed-off-by: Jose Nino <jnino@lyft.com>

Jose Nino added 2 commits January 17, 2020 15:42

Merge branch 'master' into hcm-integration

3cd3a1a

Signed-off-by: Jose Nino <jnino@lyft.com>

added fixme before merge

c9721d9

Signed-off-by: Jose Nino <jnino@lyft.com>

stale bot added the stale label Jan 21, 2020

update after api listener PR merged upstream

e131965

Signed-off-by: Jose Nino <jnino@lyft.com>

stale bot removed the stale label Jan 21, 2020

fmt

f97b8b1

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 marked this pull request as ready for review January 21, 2020 17:45

goaway reviewed Jan 21, 2020

View reviewed changes

library/common/config_template.cc Outdated Show resolved Hide resolved

goaway reviewed Jan 21, 2020

View reviewed changes

library/common/http/dispatcher.cc Show resolved Hide resolved

goaway reviewed Jan 21, 2020

View reviewed changes

library/common/http/dispatcher.cc Outdated Show resolved Hide resolved

comment clarity

191105a

Signed-off-by: Jose Nino <jnino@lyft.com>

goaway reviewed Jan 29, 2020

View reviewed changes

library/common/http/dispatcher.cc Show resolved Hide resolved

rebello95 reviewed Jan 29, 2020

View reviewed changes

library/common/config_template.cc Show resolved Hide resolved

rebello95 reviewed Jan 29, 2020

View reviewed changes

library/objective-c/EnvoyHTTPStreamImpl.m Show resolved Hide resolved

junr03 mentioned this pull request Jan 30, 2020

core: improve handling of race between local stream cancellation and non-terminal encoding #647

Open

Jose Nino added 2 commits January 29, 2020 23:41

dispatch lock for race between cancellation and non-terminal encodings

f3d8ff2

Signed-off-by: Jose Nino <jnino@lyft.com>

definition

ed88f67

Signed-off-by: Jose Nino <jnino@lyft.com>

rebello95 reviewed Jan 30, 2020

View reviewed changes

test/common/thread/lock_guard_test.cc Show resolved Hide resolved

rebello95 reviewed Jan 30, 2020

View reviewed changes

test/common/thread/lock_guard_test.cc Show resolved Hide resolved

increase timeout for perf build

8af70cf

Signed-off-by: Jose Nino <jnino@lyft.com>

buildbreaker reviewed Jan 30, 2020

View reviewed changes

.github/workflows/core.yml Show resolved Hide resolved

goaway reviewed Jan 30, 2020

View reviewed changes

library/common/thread/lock_guard.h Show resolved Hide resolved

goaway approved these changes Jan 30, 2020

View reviewed changes

junr03 merged commit 38710da into master Jan 30, 2020

junr03 deleted the hcm-integration branch January 30, 2020 21:11

junr03 mentioned this pull request Feb 4, 2020

config: support dynamic_forward_proxy #433

Closed

junr03 mentioned this pull request Feb 13, 2020

core: sigabort in histogram code #688

Closed

rebello95 added a commit that referenced this pull request Feb 14, 2020

core: remove envoy_stream_options

0b1a8fa

As of #616, this is no longer used or necessary since it's handled by the router. Resolves #643. Signed-off-by: Michael Rebello <me@michaelrebello.com>

rebello95 mentioned this pull request Feb 14, 2020

core: remove envoy_stream_options #689

Merged

rebello95 added a commit that referenced this pull request Feb 19, 2020

core: remove envoy_stream_options (#689)

2223fcc

As of #616, this is no longer used or necessary since it's handled by the router. Resolves #643. Signed-off-by: Michael Rebello <me@michaelrebello.com>

junr03 mentioned this pull request Mar 19, 2020

callbacks: only ever surface one final callback #592

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: integration with Envoy's API listener #616

core: integration with Envoy's API listener #616

junr03 commented Dec 19, 2019 •

edited

Loading

junr03 commented Dec 19, 2019

stale bot commented Jan 21, 2020

junr03 commented Jan 29, 2020

goaway Jan 29, 2020

goaway Jan 30, 2020

goaway Jan 30, 2020

junr03 Jan 30, 2020

goaway left a comment

junr03 commented Jan 30, 2020

core: integration with Envoy's API listener #616

core: integration with Envoy's API listener #616

Conversation

junr03 commented Dec 19, 2019 • edited Loading

junr03 commented Dec 19, 2019

stale bot commented Jan 21, 2020

junr03 commented Jan 29, 2020

goaway Jan 29, 2020

Choose a reason for hiding this comment

goaway Jan 30, 2020

Choose a reason for hiding this comment

goaway Jan 30, 2020

Choose a reason for hiding this comment

junr03 Jan 30, 2020

Choose a reason for hiding this comment

goaway left a comment

Choose a reason for hiding this comment

junr03 commented Jan 30, 2020

junr03 commented Dec 19, 2019 •

edited

Loading