listener: first implementation of an Api Listener by junr03 · Pull Request #9516 · envoyproxy/envoy

junr03 · 2019-12-28T21:25:35Z

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

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

repokitteh-read-only · 2019-12-28T21:25:40Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9516 was opened by junr03.

see: more, trace.

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

source/common/http/conn_manager_impl.cc

source/extensions/filters/network/http_connection_manager/config.h

…-listener

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

junr03 · 2020-01-16T18:44:29Z

@mattklein123 merged!

mattklein123 · 2020-01-16T21:22:57Z

/retest

repokitteh-read-only · 2020-01-16T21:23:02Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #9516 (comment) was created by @mattklein123.

see: more, trace.

mattklein123 · 2020-01-16T21:23:13Z

/azp run envoy-linux

azure-pipelines · 2020-01-16T21:23:22Z

Azure Pipelines successfully started running 1 pipeline(s).

mattklein123

Thanks, LGTM with a few remaining comments.

/wait

include/envoy/server/api_listener.h

include/envoy/server/listener_manager.h

source/common/http/conn_manager_impl.cc

source/server/api_listener_impl.cc

mattklein123 · 2020-01-16T21:44:46Z

test/integration/api_listener_integration_test.cc

+class ApiListenerIntegrationTest : public BaseIntegrationTest,
+                                   public testing::TestWithParam<Network::Address::IpVersion> {


Can this potentially be based on the HTTP integration tests? I think you could just use that with a config modifier that replaces the listener contents with an API listener?

I didn't want to grab all the unnecessary codec_client stuff. I did clean up the config part to DRY it up.

test/integration/api_listener_integration_test.cc

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

junr03 · 2020-01-17T02:30:37Z

@mattklein123 updated! Thanks for all the comments :)

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

goaway

Looks great from my end. Nice work @junr03!

mattklein123

Awesome work, thanks!

Description: noticed in #9516 that we did not have a convention around aliasing absl::optional<std::reference_wrapper<T>>. This PR proposes one. Risk Level: low. compiles. Testing: compilation and test suite ran. Docs Changes: added the convention in STYLE.md Signed-off-by: Jose Nino <jnino@lyft.com>

Description: this PR integrates [Envoy's ApiListener](envoyproxy/envoy#9516) 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>

Description: this PR integrates [Envoy's ApiListener](#9516) 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> Signed-off-by: JP Simard <jp@jpsim.com>

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

wip: get an api listener at all costs

d182cbe

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

fmt build

3c36ef8

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

building and worksgit add source/! 🎉

d91b112

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

initial clean up. Builds, tested

c6f2d8a

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

Merge branch 'master' into api-listener

31b8ecd

fmt

cfa81d8

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

back to singleton. ApiListener and ApiListenerHandle interfaces

538d73c

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

fmt

ad81348

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

comments

e617918

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

comments split up

efe5cff

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

move read callbacks outside of the lambda

2760a21

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

comment

1a57cf0

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

fmt

d74abb6

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

change api listener to implemented

5684186

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

junr03 requested review from alyssawilk and mattklein123 as code owners December 28, 2019 21:25

repokitteh-read-only bot added the api label Dec 28, 2019

junr03 assigned goaway and mattklein123 Dec 28, 2019

fixes

1427f3e

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

goaway reviewed Dec 30, 2019

View reviewed changes

source/common/http/conn_manager_impl.cc Outdated Show resolved Hide resolved

goaway reviewed Dec 30, 2019

View reviewed changes

source/extensions/filters/network/http_connection_manager/config.h Outdated Show resolved Hide resolved

Jose Nino added 7 commits January 2, 2020 11:20

Merge branch 'master' of https://github.com/envoyproxy/envoy into api…

06d0043

…-listener

fmt

3dd0b13

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

fmt, with clang-format 9

9f6094b

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

does this work with proto formatting?

65e3a71

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

change to implemented

c9f429b

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

Merge branch 'master' into api-listener

c8f9658

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

Merge branch 'master' into api-listener

9647159

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

Merge branch 'master' into api-listener

516b71b

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

repokitteh-read-only bot removed the waiting label Jan 16, 2020

mattklein123 requested changes Jan 16, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 16, 2020

junr03 mentioned this pull request Jan 17, 2020

protobuf: update pertinent sites to anyConvertAndValidate #9712

Closed

comments

835adbb

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

repokitteh-read-only bot removed the waiting label Jan 17, 2020

nits

77a5570

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

Merge branch 'master' into api-listener

7442f86

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

goaway approved these changes Jan 17, 2020

View reviewed changes

mattklein123 approved these changes Jan 17, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jan 17, 2020

junr03 merged commit 9b6260f into envoyproxy:master Jan 17, 2020

This was referenced Jan 17, 2020

style: convention for optional reference wrapper type alias #9725

Merged

proto: use anyConvertAndValidate where applicable #9726

Merged

core: integration with Envoy's API listener envoyproxy/envoy-mobile#616

Merged

		class ApiListenerIntegrationTest : public BaseIntegrationTest,
		public testing::TestWithParam<Network::Address::IpVersion> {

Conversation

junr03 commented Dec 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Dec 28, 2019

Uh oh!

Uh oh!

Uh oh!

junr03 commented Jan 16, 2020

Uh oh!

mattklein123 commented Jan 16, 2020

Uh oh!

repokitteh-read-only bot commented Jan 16, 2020

Uh oh!

mattklein123 commented Jan 16, 2020

Uh oh!

azure-pipelines bot commented Jan 16, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

junr03 Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

junr03 commented Jan 17, 2020

Uh oh!

goaway left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

junr03 commented Dec 28, 2019 •

edited

Loading