add support for sourceip hashing in tcp proxy #35014
Conversation
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
|
We need to update the API docs as well. Also add an integration test to https://github.com/istio/istio/blob/f2909db45681292f4acaf550d83cc92f2be47828/tests/integration/pilot/common/routing.go#L1409. The other thing is backwards compatibility. I think we are probably ok here, but it could represent a behavioral change for some users |
| return buildOutboundNetworkFiltersWithSingleDestination(push, node, statPrefix, clusterName, port) | ||
| destRule := push.DestinationRule(node, service) | ||
| destinationRule := CastDestinationRule(destRule) | ||
| return buildOutboundNetworkFiltersWithSingleDestination(push, node, statPrefix, clusterName, port, destinationRule) | ||
| } | ||
| return buildOutboundNetworkFiltersWithWeightedClusters(node, routes, push, port, configMeta) |
There was a problem hiding this comment.
Doesn't it need src ip hash?
There was a problem hiding this comment.
I do not know if it makes sense for weighted clusters - Does it? We build a single TCP Proxy and there are multiple upstream cluster. I am not sure how we can decide for which cluster sourceIP needs to be used - we certainly can not enforce for all?
There was a problem hiding this comment.
Don't we support it for HTTP? We should be consistent at least I think
There was a problem hiding this comment.
I just looked at envoy code and it picks the cluster based on weights first applies the hash - so we should be able to do it here as well. Let me try
| } | ||
|
|
||
| idleTimeout, err := time.ParseDuration(node.Metadata.IdleTimeout) | ||
| if err == nil { | ||
| tcpProxy.IdleTimeout = durationpb.New(idleTimeout) | ||
| } | ||
|
|
||
| // If destinationrule has consistent hash source ip set, use it for tcp proxy. | ||
| if destinationRule != nil && destinationRule.GetTrafficPolicy().GetLoadBalancer().GetConsistentHash() != nil && | ||
| destinationRule.GetTrafficPolicy().GetLoadBalancer().GetConsistentHash().GetUseSourceIp() { |
There was a problem hiding this comment.
You should consider subset hash here
There was a problem hiding this comment.
Yes. I will add
Yes. Planning to update API docs once this is merged. Will add an integration test |
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
|
@hzxuzhonghu @howardjohn any more comments on this? |
| } | ||
|
|
||
| idleTimeout, err := time.ParseDuration(node.Metadata.IdleTimeout) | ||
| if err == nil { | ||
| tcpProxy.IdleTimeout = durationpb.New(idleTimeout) | ||
| } | ||
|
|
||
| if destinationRule != nil { |
There was a problem hiding this comment.
| if destinationRule != nil { | |
| if destinationRule.GetTrafficPolicy().GetLoadBalancer().GetConsistentHash().GetUseSourceIp() { |
protobuf adds nil checks into each Get call
There was a problem hiding this comment.
nevermind it doesn't quite work due to subsets. But we can simplify L85-L86
| return buildOutboundNetworkFiltersWithSingleDestination(push, node, statPrefix, clusterName, port) | ||
| destRule := push.DestinationRule(node, service) | ||
| destinationRule := CastDestinationRule(destRule) | ||
| return buildOutboundNetworkFiltersWithSingleDestination(push, node, statPrefix, clusterName, port, destinationRule) | ||
| } | ||
| return buildOutboundNetworkFiltersWithWeightedClusters(node, routes, push, port, configMeta) |
There was a problem hiding this comment.
Don't we support it for HTTP? We should be consistent at least I think
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
|
/test unit-tests_istio |
|
/test integ-security-multicluster-tests_istio |
Fixes #33558
Please check any characteristics that apply to this pull request.