cds: Add "auto_http2" option. by jrajahalme · Pull Request #399 · envoyproxy/data-plane-api

jrajahalme · 2018-01-06T00:58:56Z

Currently clusters can not open both HTTP1.1 and HTTP2 upstream
connections at the same time. When the new cluster option
"auto_http2" is set to "true", the cluster must open an HTTP2 upstream
connection if the downstream connection is HTTP2, and an HTTP1.1
upstream connection if the downstream connection is HTTP1.1. This option
is to have no effect if there is no corresponding downstream
connection.

This functionality removes the need to operate multiple clusters and
routing rules for them when the backends accept both HTTP1.1 and HTTP2
connections, and when the choice of the HTTP protocol is significant,
as with gRPC.

Signed-off-by: Jarno Rajahalme jarno@covalent.io

Currently clusters can not open both HTTP1.1 and HTTP2 upstream connections at the same time. When the new cluster option "auto_http2" is set to "true", the cluster must open an HTTP2 upstream connection if the downstream connection is HTTP2, and an HTTP1.1 upstream connection if the downstream connection is HTTP1.1. This option is to have no effect if there is no corresponding downstream connection. This functionality removes the need to operate multiple clusters and routing rules for them when the backends accept both HTTP1.1 and HTTP2 connections, and when the choice of the HTTP protocol is significant, as with gRPC. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-01-06T00:59:41Z

I have the corresponding Envoy PR ready to go after this is merged.

Remove over-enthusiastic spaces. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123 · 2018-01-06T01:33:42Z

I'm a little suspicious this is a common use case. Why wouldn't most users want to upgrade their incoming HTTP/1.1 connections to HTTP/2 if the backend supports it?

Also, like the other PR, if you have the code already written do you mind posting it so that we can look at the code at the same time? Thank you!

mattklein123 · 2018-01-06T01:35:50Z

api/cds.proto

+  // Cluster can operate both HTTP1.1 and HTTP2 connections and the protocol
+  // is chosen based on the downstream protocol, if any. In case there is no
+  // downstream connection, this option will be ignored.
+  bool auto_http2 = 26;


Assuming we do end up taking this upstream, I think we should make this field more specific, such as infer_http2_from_downstream_connection or something like that. I also wonder if this should go inside the Http2ProtocolOptions message.

The semantics looks complicated and the variable name is too terse, as @mattklein123 suggested.

Can we make this behavior automatic instead of a specific option? Every boolean option essentially double the testing matrix. It adds more work to monitoring and support as well. W need to justify the value before adding another option.

Unfortunately I don't think this can be the default behavior.

I thought about this more and I would suggest actually making this an enum, with the enum default being the current behavior (effectively, "select based on codec options presence). The reason for this is eventually we will want to support HTTP/1 -> HTTP/2 upgrade and this could be a future enum value. cc @rshriram

jrajahalme · 2018-01-06T16:48:02Z

On 5 Jan 2018, at 17.33, Matt Klein ***@***.***> wrote: I'm a little suspicious this is a common use case. Why wouldn't most users want to upgrade their incoming HTTP/1.1 connections to HTTP/2 if the backend supports it?

The use case we need this for is with the original destination cluster, where we have no idea if the backends support HTTP2 or not, as there is no a-priori knowledge about the actual backends. Our actual use case is with gRPC where both client and server are using HTTP2, but there is no requirement that other non-gRPC services must use HTTP2. This should also be applicable in any other scenario where Envoy is deployed as a transparent proxy, e.g., with iptables REDIRECT rules, where it may be impractical to deduce application semantics and encode that into Envoy configuration at runtime.

Also, like the other PR, if you have the code already written do you mind posting it so that we can look at the code at the same time? Thank you!

OK. It will not compile without the API change but I guess that is initially OK. Jarno

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#399 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGN124u-NVRFl48M5-pKy041_3edRP_6ks5tHs13gaJpZM4RVHtZ>.

mattklein123 · 2018-01-07T00:54:23Z

@jrajahalme you can just point the proxy PR to your fork of data-plane-api temporarily and it will compile/pass tests.

htuch · 2018-01-07T12:45:24Z

Thanks for explaining the use case, seems legit. Some comments:

We've had a long standing position that the APIs are not intended to be particularly elegant for human generation, i.e. if generating multiple cluster/routes mechanically achieves the same result, why not just do that? It makes the API more minimal and pushes work to offline config generation tools rather than into the proxy. If there is a semantic effect that can't be achieved this way, then sure, I think this makes a lot of sense to bake into the API.
What are we going to do about protocol_options in Cluster? Today these are a oneof, we should have both protocols options for H1 and H2 with this option. I think we can remove the oneof qualifier in a wire compatible way, that should probably be done in this PR (and related in envoy main repo).

jrajahalme · 2018-01-08T18:19:34Z

On Jan 7, 2018, at 4:45 AM, htuch ***@***.***> wrote: What are we going to do about protocol_options in Cluster? Today these are a oneof, we should have both protocols options for H1 and H2 with this option. I think we can remove the oneof qualifier in a wire compatible way, that should probably be done in this PR (and related in envoy main repo).

Removing the oneof qualifier would allow this feature be turned on whenever both H1 and H2 options are present. This would not require further API changes. Jarno

Remove the oneof qualifier from protocol options. If multiple options are present, then the cluster can use multiple upstream protocols. If both HTTP1.1 and HTTP2 options are present, then the cluster will choose the upstream protocol based on the downstream connection. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123 · 2018-01-08T19:16:31Z

Yeah I think we need to remove the oneof. We will need to do this in the future anyway to support HTTP/1 -> HTTP/2 upgrade.

jrajahalme · 2018-01-08T19:20:20Z

On Jan 8, 2018, at 11:16 AM, Matt Klein ***@***.***> wrote: Yeah I think we need to remove the oneof. We will need to do this in the future anyway to support HTTP/1 -> HTTP/2 upgrade.

Do you think we can deduce the auto http2 feature I proposed from the presence of both H1 and H2 options, or should I add the enum for it? Jarno

mattklein123 · 2018-01-08T19:23:57Z

@jrajahalme friendly request to please use the GH UI to respond to comments. It's a lot easier to keep track of comments threads vs. email.

Do you think we can deduce the auto http2 feature I proposed from the presence of both H1 and H2 options, or should I add the enum for it?

I think we could, but IMO it becomes very confusing. I would be in favor of just adding the enum now. It's clearer and easier to document.

jrajahalme · 2018-01-08T19:34:16Z

On Jan 8, 2018, at 11:23 AM, Matt Klein ***@***.***> wrote: @jrajahalme <https://github.com/jrajahalme> friendly request to please use the GH UI to respond to comments. It's a lot easier to keep track of comments threads vs. email. Do you think we can deduce the auto http2 feature I proposed from the presence of both H1 and H2 options, or should I add the enum for it? I think we could, but IMO it becomes very confusing. I would be in favor of just adding the enum now. It's clearer and easier to document.

OK. How about removing/deprecating GrpcProtocolOptions now? It has not been implemented yet, and only consists of Http2ProtocolOptions? Actual gRPC options can be added later if we ever need any? Jarno

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#399 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGN126xCVxsSk-lUzbE1GOhWiTeQRV8Lks5tImtPgaJpZM4RVHtZ>.

jrajahalme · 2018-01-08T19:42:34Z

On Jan 8, 2018, at 11:34 AM, jrajahalme ***@***.***> wrote: > On Jan 8, 2018, at 11:23 AM, Matt Klein ***@***.***> wrote: > > @jrajahalme <https://github.com/jrajahalme> friendly request to please use the GH UI to respond to comments. It's a lot easier to keep track of comments threads vs. email. > > Do you think we can deduce the auto http2 feature I proposed from the presence of both H1 and H2 options, or should I add the enum for it? > > I think we could, but IMO it becomes very confusing. I would be in favor of just adding the enum now. It's clearer and easier to document. > > OK. How about removing/deprecating GrpcProtocolOptions now? It has not been implemented yet, and only consists of Http2ProtocolOptions? Actual gRPC options can be added later if we ever need any?

While a cluster could so far operate either HTTP1.1 or HTTP2 upstream connections, it is unclear to me if a cluster having HTTP protocol options specified could also be used by a TCP proxy? TcpProtocolOptions being mutually exclusive with HTTP protocol options would indicate not, but I have never tried this so I don’t know the current behavior. Jarno

jrajahalme · 2018-01-08T19:48:22Z

On Jan 8, 2018, at 11:16 AM, Matt Klein ***@***.***> wrote: Yeah I think we need to remove the oneof. We will need to do this in the future anyway to support HTTP/1 -> HTTP/2 upgrade.

How do you see the 1->2 upgrade working? Is it different from the current behavior of forwarding downstream HTTP/1 connections to upstream HTTP/2 connections whenever the chosen cluster is configured for HTTP/2? Jarno

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#399 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGN124ZFJIwur7rEJNquBrLAYPZqOMXDks5tImmRgaJpZM4RVHtZ>.

Add a new enum ClusterProtocolSelection and a corresponding 'protocol_selection' option. Default value enforces backwards compatible behavior of exclusively using HTTP/1 or HTTP/2 depending on the protocol options present. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

…on'. Use HTTP1.1 or HTTP2, depending on which one is used on the downstream connection. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

htuch

Did we have any resolution on whether this PR is just syntactic sugar (to reduce the amount of duplicated cluster definitions) or if there is a fundamental needs in terms of API expressiveness?

htuch · 2018-01-08T20:20:20Z

api/cds.proto

+    // If http2_protocol_options are present, HTTP2 will be used, otherwise HTTP1.1 will be used.
+    EXCLUSIVE_AS_CONFIGURED = 0;
+    // Use HTTP1.1 or HTTP2, depending on which one is used on the downstream connection.
+    USE_DOWNSTREAM_PROTO = 1;


Prefer USE_DOWNSTREAM_PROTOCOL to avoid confusion with protobufs (I read it as "use the protobuf supplied from downstream to specify which protocol to use" at first glance)

Good point, done :-)

htuch · 2018-01-08T20:22:58Z

api/cds.proto

+  Http2ProtocolOptions http2_protocol_options = 14;
+
+  // [#not-implemented-hide:]
+  GrpcProtocolOptions grpc_protocol_options = 15;


This is something we probably want medium-long term, so should keep. There are gRPC specific options, like how much to buffer for gRPC retries, that could belong here (I think; can't remember where we left that particular one @alyssawilk @mattklein123).

Like my other comment, I would personally kill this until it's needed later. As it's no longer mutually exclusive, GrpcProtocolOptions having an embedded Http2ProtocolOptions I don't think makes sense?

I tend to lean towards treating gRPC as H2 until we have a concrete reason not to (which we probably will, eventually) and add it back then

+1 (but still suspicious we'll end up wanting it at some point).

htuch · 2018-01-08T20:23:47Z

api/cds.proto

+
+  enum ClusterProtocolSelection {
+    // Cluster can only operate on one of the possible upstream protocols (HTTP1.1, HTTP2).
+    // If http2_protocol_options are present, HTTP2 will be used, otherwise HTTP1.1 will be used.


Are there any load balancing implications when using mixed upstream protocols?

Current Envoy implementation assumes pooled connections are all either HTTP/1 or HTTP/2, as open connections are reused without any version checks. My implementation expands the containers currently used for connection pools to factor in the protocol version into the pool selection.

In the default mode the cluster there is no change, so no impact.
In the 'USE_DOWNSTREAM_PROTOCOL' each request is load balanced on the set of hosts as before, and the connection pooling extension I mentioned above makes sure that v1&2 connections are not mixed up. With this I don't see any load balancing impact.

htuch · 2018-01-08T20:25:55Z

api/cds.proto

-    GrpcProtocolOptions grpc_protocol_options = 15;
-  }
+  // [#not-implemented-hide:]
+  TcpProtocolOptions tcp_protocol_options = 12;


IDK re: TCP proxying being mutually exclusive with HTTP for a cluster. Opening this comment thread for folks to respond.

Technically, I think this was put in place for low level TCP options which are common to TCP proxy, HTTP proxy, etc. Personally, I would kill this until it's needed and add it back later.

mattklein123 · 2018-01-08T21:01:08Z

How do you see the 1->2 upgrade working? Is it different from the current behavior of forwarding downstream HTTP/1 connections to upstream HTTP/2 connections whenever the chosen cluster is configured for HTTP/2?

Like actual HTTP/1.1 -> HTTP/2 upgrade as an option: https://tools.ietf.org/html/rfc7540#section-3.2

Better avoid 'proto' when not meaning protobufs. Suggested-by: Harvey Tuch <htuch@google.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Now that the protocol options are no longer mutually exclusive, HTTP2 options can also be used for gRPC. We can add these back in when needed. The old code points remain unused. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-01-08T21:58:56Z

On Jan 8, 2018, at 1:01 PM, Matt Klein ***@***.***> wrote: How do you see the 1->2 upgrade working? Is it different from the current behavior of forwarding downstream HTTP/1 connections to upstream HTTP/2 connections whenever the chosen cluster is configured for HTTP/2? Like actual HTTP/1.1 -> HTTP/2 upgrade as an option: https://tools.ietf.org/html/rfc7540#section-3.2 <https://tools.ietf.org/html/rfc7540#section-3.2>

Thanks for the pointer. So this would apply when we don’t yet know if the upstream server supports HTTP2. My implementation separates the connection pools for HTTP/1 and HTTP/2, so any connection that is upgraded from 1 to 2 should be replaced to the HTTP/2 pool after such an upgrade. Jarno

mattklein123 · 2018-01-08T22:01:53Z

Thanks for the pointer. So this would apply when we don’t yet know if the upstream server supports HTTP2. My implementation separates the connection pools for HTTP/1 and HTTP/2, so any connection that is upgraded from 1 to 2 should be replaced to the HTTP/2 pool after such an upgrade.

I suspect the implementation of upgrade would probably use a new UpgradingConnectionPool type, but we can worry about that later. I think from the API standpoint everything we are doing here will allow us to seamlessly implement it at some later point.

mattklein123 · 2018-01-09T00:57:32Z

@jrajahalme was talking with @htuch and he suggested that if we had a route matching rule that could match on HTTP protocol this might not be needed. (You could have duplicate clusters, one for HTTP/1 and one for HTTP/2). Would that work?

jrajahalme · 2018-01-09T01:20:22Z

On Jan 8, 2018, at 4:57 PM, Matt Klein ***@***.***> wrote: @jrajahalme <https://github.com/jrajahalme> was talking with @htuch <https://github.com/htuch> and he suggested that if we had a route matching rule that could match on HTTP protocol this might not be needed. (You could have duplicate clusters, one for HTTP/1 and one for HTTP/2). Would that work?

That would double the number of routes which could be bad for performance. Let’s see what you think of the PR once I get it out. Jarno

jrajahalme · 2018-01-09T01:44:58Z

Posted the corresponding Envoy PR: envoyproxy/envoy#2328. Some new tests may still be needed, but the approach should be good for review.

mattklein123 · 2018-01-09T05:48:01Z

That would double the number of routes which could be bad for performance. Let’s see what you think of the PR once I get it out.

How many routes do you have in your setup? We are talking about a single static check when looping over a list (I think we are likely talking sub-microsecond).

htuch

envoyproxy/envoy#2328 seems a small enough change, so I don't have a huge objection to proceeding with the current PR. Would just like to make sure we fully understand the alternatives considered.

mattklein123 · 2018-01-09T17:20:38Z

envoyproxy/envoy#2328 seems a small enough change, so I don't have a huge objection to proceeding with the current PR. Would just like to make sure we fully understand the alternatives considered.

It is a small change, but I'm still not convinced it's a common enough use-case that it's worth the complexity, especially when I think routing on protocol is a trivial workaround. Do we think the extra complexity here is justified?

jrajahalme · 2018-01-09T17:37:35Z

On Jan 9, 2018, at 9:20 AM, Matt Klein ***@***.***> wrote: envoyproxy/envoy#2328 <envoyproxy/envoy#2328> seems a small enough change, so I don't have a huge objection to proceeding with the current PR. Would just like to make sure we fully understand the alternatives considered. It is a small change, but I'm still not convinced it's a common enough use-case that it's worth the complexity, especially when I think routing on protocol is a trivial workaround. Do we think the extra complexity here is justified?

IMO most of envoyproxy/envoy#2328 <envoyproxy/envoy#2328> should be reusable towards HTTP 1->2 upgrade you mentioned earlier, don’t you think? Jarno

mattklein123 · 2018-01-09T17:45:36Z

IMO most of envoyproxy/envoy#2328 <envoyproxy/envoy#2328> should be reusable towards HTTP 1->2 upgrade you mentioned earlier, don’t you think?

Some of it maybe, but if we implement upgrade it will be an entirely new connection pool implementation, so really only the config parts.

Ultimately, I don't feel super strongly about this, but I'm not convinced this feature is really necessary or useful in the general case. I would like to hear some opinions from the other maintainers. cc @envoyproxy/maintainers

jrajahalme · 2018-01-09T17:48:57Z

On Jan 8, 2018, at 9:48 PM, Matt Klein ***@***.***> wrote: That would double the number of routes which could be bad for performance. Let’s see what you think of the PR once I get it out. How many routes do you have in your setup? We are talking about a single static check when looping over a list (I think we are likely talking sub-microsecond).

I might be wrong, but if we make the downstream protocol matchable, we’ll need to split every routing rule in two, one matching on HTTP/1 and the other matching on HTTP/2, additionally to whatever those rules were already matching on, each forwarding to a similarly split (or doubled) clusters, one for HTTP/1 and the other for HTTP/2, respectively. As the route matching logic chooses the first matching route we can’t separate routing for the protocol and the rest (method, path, headers, etc.) into separate routing stages. In our application we don’t have direct control of the routing rules, as we create them from application level policy description that we do not control. In some instances the number to routes will be small, in others it can be big. I’d rather not double the configuration state (routes, clusters) and the execution time in Envoy routing. Jarno

mattklein123 · 2018-01-09T17:50:57Z

In our application we don’t have direct control of the routing rules, as we create them from application level policy description that we do not control. In some instances the number to routes will be small, in others it can be big. I’d rather not double the configuration state (routes, clusters) and the execution time in Envoy routing.

OK that's fair. I had assumed that you were basically using catch-all routes so the number of routes would increase from 1 to 2. :)

jrajahalme · 2018-01-09T17:59:19Z

On Jan 9, 2018, at 9:45 AM, Matt Klein ***@***.***> wrote: IMO most of envoyproxy/envoy#2328 <envoyproxy/envoy#2328> <envoyproxy/envoy#2328> should be reusable towards HTTP 1->2 upgrade you mentioned earlier, don’t you think? Some of it maybe, but if we implement upgrade it will be an entirely new connection pool implementation, so really only the config parts. Ultimately, I don't feel super strongly about this, but I'm not convinced this feature is really necessary or useful in the general case. I would like to hear some opinions from the other maintainers. cc @envoyproxy/maintainers <https://github.com/orgs/envoyproxy/teams/maintainers>

While other maintainers are looking at this, I’d like to bring up one side to this that may be significant to other as well. In our application we are using technique similar to iptables REDIRECT to transparently move traffic to Envoy, and restoring the original destination address (&port) of the connection before forwarding the connection from Envoy to the original destination, using the original destination cluster. Enabling Envoy to use the same HTTP protocol in the outgoing connections as was used in the incoming connection further helps in this “transparency”, as we don’t need to understand the application semantics regarding HTTP protocol version and encode that to Envoy routing and cluster configuration. Jarno

mattklein123

@jrajahalme I discussed with the other maintainers and I think we are all good moving forward on this. Thank you for taking the time to discuss it with us. I have a few small comments but otherwise LGTM.

mattklein123 · 2018-01-09T20:30:51Z

api/cds.proto


-  oneof protocol_options {
-    // [#not-implemented-hide:]
-    TcpProtocolOptions tcp_protocol_options = 12;


@wora @htuch for locked protos, when we delete a field, should we do something like:
// deleted = 12;
or something? Just curious what the norm is here. I think something like that would help me.

I added these comments for both 12 and 15.

mattklein123 · 2018-01-09T20:32:01Z

api/cds.proto

+
+  enum ClusterProtocolSelection {
+    // Cluster can only operate on one of the possible upstream protocols (HTTP1.1, HTTP2).
+    // If http2_protocol_options are present, HTTP2 will be used, otherwise HTTP1.1 will be used.


Please :ref: link http2_protocol_options

Requested-by: Matt Klein <mklein@lyft.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123

LGTM. Will defer to @htuch for merge and @wora for any further comments.

wora

Overall, it looks good.

wora · 2018-01-10T12:35:03Z

api/cds.proto

+    // Cluster can only operate on one of the possible upstream protocols (HTTP1.1, HTTP2).
+    // If :ref:`http2_protocol_options <envoy_api_field_Cluster.http2_protocol_options>` are
+    // present, HTTP2 will be used, otherwise HTTP1.1 will be used.
+    EXCLUSIVE_AS_CONFIGURED = 0;


USE_CONFIGURED_PROTOCOL sounds better.

Please avoid subtle concept like exclusive unless it is really needed.

Done. Thanks for the review!

wora · 2018-01-10T12:36:16Z

api/cds.proto

+  // connections to happen over plain text.
+  Http2ProtocolOptions http2_protocol_options = 14;
+
+  // deleted = 15;


Use proto language syntax as follows. If you are not familiar, please see proto spec.

reserved 13, 15;

Thanks, used the reserved keyword for 12 and 15 separately.

Requested-by: Hong Zhang <wora2000@gmail.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

cds: Fix spacing.

8b90cc7

Remove over-enthusiastic spaces. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123 reviewed Jan 6, 2018

View reviewed changes

jrajahalme added 2 commits January 8, 2018 11:53

cds: Add 'USE_DOWNSTREAM_PROTO' enum value to 'ClusterProtocolSelecti…

d2a491c

…on'. Use HTTP1.1 or HTTP2, depending on which one is used on the downstream connection. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

htuch reviewed Jan 8, 2018

View reviewed changes

jrajahalme added 2 commits January 8, 2018 13:24

cds: Spell out PROCOCOL.

cc5c6f6

Better avoid 'proto' when not meaning protobufs. Suggested-by: Harvey Tuch <htuch@google.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

cds: Drop unimplemented TCP and gRPC protocol options.

5f59143

Now that the protocol options are no longer mutually exclusive, HTTP2 options can also be used for gRPC. We can add these back in when needed. The old code points remain unused. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme mentioned this pull request Jan 9, 2018

cluster_manager: Add support for USE_DOWNSTREAM_PROTOCOL. envoyproxy/envoy#2328

Merged

htuch reviewed Jan 9, 2018

View reviewed changes

mattklein123 reviewed Jan 9, 2018

View reviewed changes

jrajahalme added 3 commits January 9, 2018 13:16

cds: Link comment to http2_protocol_options.

a4fa3e9

Requested-by: Matt Klein <mklein@lyft.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

cds: Add comments for deleted fields.

1c5328b

Requested-by: Matt Klein <mklein@lyft.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

cds: Fix spacing.

fe29d04

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123 previously approved these changes Jan 9, 2018

View reviewed changes

wora suggested changes Jan 10, 2018

View reviewed changes

cds: Review fixes.

2df3018

Requested-by: Hong Zhang <wora2000@gmail.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme dismissed mattklein123’s stale review via 2df3018 January 10, 2018 18:14

htuch approved these changes Jan 10, 2018

View reviewed changes

mattklein123 approved these changes Jan 10, 2018

View reviewed changes

mattklein123 merged commit a4b80d6 into envoyproxy:master Jan 10, 2018

Conversation

jrajahalme commented Jan 6, 2018

Uh oh!

jrajahalme commented Jan 6, 2018

Uh oh!

mattklein123 commented Jan 6, 2018

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!

jrajahalme commented Jan 6, 2018 via email

Uh oh!

mattklein123 commented Jan 7, 2018

Uh oh!

htuch commented Jan 7, 2018

Uh oh!

jrajahalme commented Jan 8, 2018 via email

Uh oh!

mattklein123 commented Jan 8, 2018

Uh oh!

jrajahalme commented Jan 8, 2018 via email

Uh oh!

mattklein123 commented Jan 8, 2018

Uh oh!

jrajahalme commented Jan 8, 2018 via email

Uh oh!

jrajahalme commented Jan 8, 2018 via email

Uh oh!

jrajahalme commented Jan 8, 2018 via email

Uh oh!

htuch 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!

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 commented Jan 8, 2018

Uh oh!

jrajahalme commented Jan 8, 2018 via email

Uh oh!

mattklein123 commented Jan 8, 2018

Uh oh!

mattklein123 commented Jan 9, 2018

Uh oh!

jrajahalme commented Jan 9, 2018 via email

Uh oh!

jrajahalme commented Jan 9, 2018

Uh oh!

mattklein123 commented Jan 9, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jan 9, 2018