http: making http upstreams pluggable by alyssawilk · Pull Request #11327 · envoyproxy/envoy

alyssawilk · 2020-05-26T20:08:55Z

Mainly moving code, but finishes up the series of pluggable upstream PRs.

Additional Description: This unhides the configurable extension point to the cluster for selecting a connection pool and creating an upstream, which can be used for custom business logic in upstream creation.

Risk Level: medium (router refactor)
Testing: with prior PRs
Docs Changes: inline with APIs
Release Notes: added

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

At a high level this looks great. I left some comments. I would suggest we flesh out some config before going further? wdyt?

/wait

mattklein123 · 2020-05-27T22:11:14Z

+  // Called to cancel a call to newStream. Returns true if a newStream request
+  // was canceled, false otherwise.
+  virtual bool cancelAnyPendingRequest() PURE;


From an API perspective I would recommend having newStream() return a handle which can be cancelled? I think that would be cleaner and more future proof.

I think if you're OK with it I'll do one PR just moving the APIs out and cleaning them up, then a follow-up doing factory and moving code around. I think if we do both in the same PR it's hard to see what's changing.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only · 2020-06-02T16:13:03Z

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

🐱

Caused by: #11327 was synchronize by alyssawilk.

see: more, trace.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2020-06-02T16:22:28Z

@htuch @mattklein123 just for the cluster config file

@mattklein123 I think this is what we agreed on for a strawman on slack - lmk if I didn't understaodn
@htuch for a second opinion - as Matt said the cluster config is a bit of a mess
@lambdai for "does this work for you"

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lambdai · 2020-06-02T20:45:33Z

  return Http::FilterHeadersStatus::StopIteration;
 }

+// TODO(alyssawilk) create from cluster config.


How could a Router::GenericConnPool created from cluster config?
The cluster doesn't even know if a filter requires Router::GenericConnPool, TcpProxy::GenericConnPool, or a WeirdConnPool.

My understanding the GenericConnPool is generic to the impl of ConnPool, but there is no such a generic conn pool interface for HttpConnManager, TcpProxy and Redis. Each above protocol has its individual GenericConnPool.
It might be more straighforward if you rename GenericConnPool to RouterGenericConnPool in below line

Did the factory todo - take a look and let me know if you have further questions

I think I was confused when I assume Router::GenericConnPool is going to replace the GenericConnPool in tcp proxy(the network filter, not the bool flag).
When I get rid of the assumption the code is clear. Thanks!

mattklein123

At a high level this LGTM, though I think we need more comments and docs. Do you mind doing that first just to make sure we agree on the end user config/docs? Thank you!

/wait

mattklein123 · 2020-06-03T15:23:47Z

+    // Required. The name of the upstream type
+    string name = 1;


You can use TypedExtensionConfig for this message and probably just remove UpstreamType

mattklein123 · 2020-06-03T15:25:04Z

+  // Optional customization and configuration of upstream type.
+  // By default, Envoy will create an upstream of type envoy.filters.upstreams.http.default
+  // The default upstream will create a envoy.filters.upstreams.http.tcp upstream
+  // if the request is a CONNECT request and connect termination is configured.
+  // Otherwise it will be envoy.filters.upstreams.http.http, with the protocol
+  // based on `http2_protocol_options`.


I think this is still going to confuse people, so I think it might be worth spelling this out even more, especially the last part about being based on http2_protocol_options. This is already a point of huge confusion.

mattklein123 · 2020-06-03T15:26:50Z

+  // if the request is a CONNECT request and connect termination is configured.
+  // Otherwise it will be envoy.filters.upstreams.http.http, with the protocol
+  // based on `http2_protocol_options`.
+  UpstreamType upstream_type = 48;


In terms of field name, I'm torn between being more specific here versus keeping this generic because I think ultimately we would want to use that for tcp_proxy also? Should the docs be more clear about which filters this applies to currently? This is what I was talking about re: cluster.proto being a mess. There are a huge amount of concepts convolved into one place and it's very confusing trying to understand what fields are used by what. I'm not saying we can fix this without a major version bump / major deprecation spree, but I think we probably need a lot more docs for this type of thing.

I think we can reuse the proto for tcp proxy, but we'd have to use the different type if that's allowable. Basically for tcp proxy, it'd have to be a envoy.filters.upstreams.tcp since the interfaces are all different.
I'll comment that for today, this only works for http upstreams but is designed to be extended to custom tcp upstreams as well. SG?

Right I agree that is how it would have to work, we just need to make sure it's super clearly documented, both here and probably also in the filter docs.

So the plan is

UpstreamType could be reused by tcp_proxy, and

Another UpstreamType typed field, e.g. tcp_proxy_upstream_type = 49 can be added.
That make perfect sense to me and my other questions are resolved.

And UpstreamType or the updated TypedExtensionConfig works for tcp proxy case wrt proto api. Thanks!

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

Thanks this config LGTM at a high level. Do you want to finish up the change and then we can review further?

/wait

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2020-06-03T17:23:39Z

I suspect I need some more unit tests on this, but I think it's ready for another pass - will check out coverage build and work on that in parallel.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Part 3 of envoyproxy#11327, using the new configuration to create the upstream connection pool. Risk Level: Medium (router refactor, intended as no-op) Testing: new unit tests Docs Changes: n/a Release Notes: pending final PR Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

…11554) split out from envoyproxy#11327 There's a bit of transitive ugliness: declaring the extensions requires security posture, requires stub build files, requires codeowners before the code move, but it'll be pretty short lived. Risk Level: Low (mostly only APIs) Testing: n/a Docs Changes: some of the new docs Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

Moving the Generic class definitions into the include file, in preparation for pluggable upstreams split out from envoyproxy#11327 Risk Level: Low (mostly only header changes) Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

Part 3 of envoyproxy#11327, using the new configuration to create the upstream connection pool. Risk Level: Medium (router refactor, intended as no-op) Testing: new unit tests Docs Changes: n/a Release Notes: pending final PR Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

…11554) split out from envoyproxy#11327 There's a bit of transitive ugliness: declaring the extensions requires security posture, requires stub build files, requires codeowners before the code move, but it'll be pretty short lived. Risk Level: Low (mostly only APIs) Testing: n/a Docs Changes: some of the new docs Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Moving the Generic class definitions into the include file, in preparation for pluggable upstreams split out from envoyproxy#11327 Risk Level: Low (mostly only header changes) Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Part 3 of envoyproxy#11327, using the new configuration to create the upstream connection pool. Risk Level: Medium (router refactor, intended as no-op) Testing: new unit tests Docs Changes: n/a Release Notes: pending final PR Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

This includes enhancing the script failures to leave breadcrumbs for devs who miss the new docs. Risk Level: n/a (tooling / docs) Testing: manual testing removing files from envoyproxy#11327 Docs Changes: yes Release Notes: no Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Mainly moving code, but finishes up the series of pluggable upstream PRs. Additional Description: This unhides the configurable extension point to the cluster for selecting a connection pool and creating an upstream, which can be used for custom business logic in upstream creation. Risk Level: medium (router refactor) Testing: with prior PRs Docs Changes: inline with APIs Release Notes: added Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

…11554) split out from envoyproxy#11327 There's a bit of transitive ugliness: declaring the extensions requires security posture, requires stub build files, requires codeowners before the code move, but it'll be pretty short lived. Risk Level: Low (mostly only APIs) Testing: n/a Docs Changes: some of the new docs Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

Moving the Generic class definitions into the include file, in preparation for pluggable upstreams split out from envoyproxy#11327 Risk Level: Low (mostly only header changes) Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

http: WIP making things pluggable

5610ef9

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 self-assigned this May 26, 2020

mattklein123 requested changes May 27, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label May 27, 2020

Merge branch 'master' into pluggable

d5af278

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk mentioned this pull request Jun 2, 2020

http: refactoring http and tcp upstreams #11349

Merged

alyssawilk added 3 commits June 2, 2020 10:12

working in the other PR

8efa065

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

adding default type

0927471

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'master' into pluggable

1cbbec1

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only Bot added api and removed waiting labels Jun 2, 2020

cleanup

f6dea5c

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

API comments

30b7487

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lambdai reviewed Jun 2, 2020

View reviewed changes

Comment thread api/envoy/config/cluster/v3/cluster.proto Outdated

lambdai reviewed Jun 2, 2020

View reviewed changes

mattklein123 requested changes Jun 3, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label Jun 3, 2020

reviewer comments

84721ab

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only Bot removed the waiting label Jun 3, 2020

mattklein123 reviewed Jun 3, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label Jun 3, 2020

fixing build, FIXMEs

88ab5d5

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only Bot removed the waiting label Jun 3, 2020

lambdai reviewed Jun 3, 2020

View reviewed changes

Comment thread source/extensions/upstreams/http/default/config.cc Outdated

alyssawilk added 2 commits June 3, 2020 13:40

speeling

1160059

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

g to #

528df2e

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk marked this pull request as ready for review June 3, 2020 18:10

mattklein123 approved these changes Jun 22, 2020

View reviewed changes

alyssawilk merged commit 8a9d615 into envoyproxy:master Jun 23, 2020

Conversation

alyssawilk commented May 26, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Jun 2, 2020

Uh oh!

alyssawilk commented Jun 2, 2020

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!

mattklein123 left a comment

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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alyssawilk commented May 26, 2020 •

edited

Loading

alyssawilk commented Jun 3, 2020 •

edited

Loading