grpc access log: verify cluster exists and is static#2360
Conversation
Signed-off-by: Matt Klein <mklein@lyft.com>
| // TODO(htuch): Support Google gRPC client. | ||
| const auto cluster_name = proto_config.common_config().grpc_service().envoy_grpc().cluster_name(); | ||
| auto cluster = context.clusterManager().get(cluster_name); | ||
| if (cluster == nullptr || cluster->info()->addedViaApi()) { |
There was a problem hiding this comment.
what is the difference between a static cluster vs a cluster added by api?
There was a problem hiding this comment.
Static cluster is one defined in the config. A cluster added by api would be a cluster added via CDS.
|
Discussed this with @htuch and I think we will move this check into the base AsyncClient for now as I think we want this in all existing use cases. |
|
Hmm. I just looked and this is not so easy, as we don't create the AsyncClient in this case until we get into TLS code, unlike many of the existing main thread use cases. I think the best I can do is potentially make a utility function for this check? @htuch WDYT? |
|
We may have a more fundamental issue here. Are we relying on client
creation to be cheap and potentially reuse an existing connection? The
Google gRPC client creates a new channel each time it instantiates, I'm
wondering if this violates this. Otoh, maybe it does what Envoy does
internally with connection pooling.
…On Sat, Jan 13, 2018 at 6:01 PM Matt Klein ***@***.***> wrote:
Hmm. I just looked and this is not so easy, as we don't create the
AsyncClient in this case until we get into TLS code, unlike many of the
existing main thread use cases. I think the best I can do is potentially
make a utility function for this check? @htuch <https://github.com/htuch>
WDYT?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2360 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKaLv8akkwWI6AnSRFsZWM4Qol9NiKpNks5tKTXkgaJpZM4Rc11y>
.
|
In general, yes, but not in the case of access log where we are more careful about TLS data structures and will keep the client around for a long time. There are other cases (rate limit is one for sure) where we create a new client on every request. This is not optimal but works OK with the Envoy gRPC client. |
|
@htuch I'm inclined to just merge this and we can reconcile the error handling in a follow up PR? WDYT? |
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
OK, let's do this. I think we're going to have to iterate a bit here; the initial Google gRPC implementation will be sub-optimal for rate limit for sure, we'll be missing some checks for Envoy gRPC where they should be as well. Keep in mind that there are two in-flight PRs that will be adding new gRPC client users.
|
@mattklein123 @htuch i was thinking of doing similar validation for metrics service. Should I do the same or wait till it is finalised on how this should be done generically? |
|
I have a PR that I will post soon that consolidates allmthis checking, so
no worries, thanks.
…On Sun, Jan 14, 2018 at 11:24 PM ramaraochavali ***@***.***> wrote:
@mattklein123 <https://github.com/mattklein123> @htuch
<https://github.com/htuch> i was thinking of doing similar validation for
metrics service. Should I do the same or wait till it is finalised on how
this should be done generically?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2360 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKaLv0m72Vw0I7pqwFxmASf6aRQXKIteks5tKtMTgaJpZM4Rc11y>
.
|
|
Cool. Thanks |
|
@htuch did you push this static cluster validation? |
|
@ramaraochavali yes, this was done in #2393. |
|
@htuch Great. Thanks. |
6 participants
Risk Level: Low
Testing: Unit
Docs Changes: N/A
Release Notes: N/A