Conversation
gargnupur
force-pushed
the
nup_tcp_cluster_name
branch
from
901bf67 to
27df3f7
Compare
|
@jacob-delgado, @kyessenov , @mandarjog , @douglas-reid : This is an attempt to fix istio/istio#21566 |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
At a high level I think this is an OK direction but I left some comments. Thank you!
There was a problem hiding this comment.
I don't mind the idea of storing a route inside the stream info, but this will need to be a shared_ptr to a const object to avoid potential lifetime issues around RDS, etc.
source/common/tcp_proxy/tcp_proxy.h
Outdated
There was a problem hiding this comment.
I don't think this part makes sense, as we have tons of not implemented APIs for TCP. I would recommend introducing a common base class that might be shared between TCP and HTTP routes (and maybe other procols in the future).
There was a problem hiding this comment.
I think FilterChain is a more appropriate abstraction for HTTP route IMHO.
|
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! |
stale
bot
added
stale
gargnupur
force-pushed
the
nup_tcp_cluster_name
branch
from
070ec03 to
2a01203
Compare
Signed-off-by: gargnupur <gargnupur@google.com> Refactor RouteEntry to use a new base class UpstreamEndpointInfo to be shared between HTTP and TCP Signed-off-by: gargnupur <gargnupur@google.com> Change to use shared ptr router Signed-off-by: gargnupur <gargnupur@google.com> Remove upstreamendpoint info added in a separate file Signed-off-by: gargnupur <gargnupur@google.com> Fix lint Signed-off-by: gargnupur <gargnupur@google.com>
gargnupur
force-pushed
the
nup_tcp_cluster_name
branch
from
b6f38ab to
1a95e49
Compare
Signed-off-by: gargnupur <gargnupur@google.com>
Signed-off-by: gargnupur <gargnupur@google.com>
|
@mattklein123 , @lizan , @zuercher, @alyssawilk : This is ready for initial review. /cc @kyessenov , @mandarjog |
|
@mattklein123 , @lizan , @kyessenov , @mandarjog : I feel this PR is not worth the refactoring needed to make TCP and HTTP share UpstreamEndPointInfo(clustername, metadatamatchcriteria) in their Route classes. It would be just simpler to add cluster_name as a property in streaminfo.. created a PR for that.. #10432 Please let me know what you guys think? |
|
@gargnupur I agree. I think adding upstream cluster info to stream info is easier and is more generally useful. |
|
|
||
| const std::string& cluster_name = route_ ? route_->clusterName() : EMPTY_STRING; | ||
| getStreamInfo().setUpstreamEndpointInfo(route_); | ||
| // getStreamInfo().setRouteEntry(std::dynamic_pointer_cast<const Router::RouteEntry>(route_)); |
There was a problem hiding this comment.
yes.. will remove.. I had hoped this would work but it obviously doesn't ..
and because of which we have to add 2 new functions for UpstreamEndpointInfo..
@kyessenov : Upstream ClusterInfo is also structured for HTTP only, thus cluster_name seemed better to me.. |
I commented in the other PR but I think we should snap the ClusterInfo shared_ptr, but let's discuss in the other PR. I'm fine with closing this one for now if we all agree on the other approach. |
5 participants
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Add RouteEntry To TCP. This is so that we can fetch cluster_name for TCP unhealthy host connections.
Ref: istio/istio#21566
Description: Add RouteEntry To TCP
Risk Level: Low
Testing: Will Add Test
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]