do not add previous hosts predicate when consistent hashing is enabled (

#50759)

Signed-off-by: Rama Chavali <[email protected]>

pilot/pkg/networking/core/listener_waypoint.go

@@ -576,7 +576,7 @@ func (lb *ListenerBuilder) routeDestination(out *route.Route, in *networking.HTT
  policy = lb.push.Mesh.GetDefaultHttpRetryPolicy()
  }
  action := &route.RouteAction{
- RetryPolicy: retry.ConvertPolicy(policy),
+ RetryPolicy: retry.ConvertPolicy(policy, false),
  }
 
  // Configure timeouts specified by Virtual Service if they are provided, otherwise set it to defaults.

pilot/pkg/networking/core/route/retry/retry.go

@@ -32,21 +32,32 @@ var defaultRetryPriorityTypedConfig = protoconv.MessageToAny(buildPreviousPriori
 
 // DefaultPolicy gets a copy of the default retry policy.
 func DefaultPolicy() *route.RetryPolicy {
+ policy := defaultPolicy()
+ policy.RetryHostPredicate = []*route.RetryPolicy_RetryHostPredicate{
+ // to configure retries to prefer hosts that haven’t been attempted already,
+ // the builtin `envoy.retry_host_predicates.previous_hosts` predicate can be used.
+ xdsfilters.RetryPreviousHosts,
+ }
+ return policy
+}
+
+func defaultPolicy() *route.RetryPolicy {
  policy := route.RetryPolicy{
  NumRetries: &wrappers.UInt32Value{Value: 2},
  RetryOn: "connect-failure,refused-stream,unavailable,cancelled,retriable-status-codes",
  RetriableStatusCodes: []uint32{http.StatusServiceUnavailable},
- RetryHostPredicate: []*route.RetryPolicy_RetryHostPredicate{
- // to configure retries to prefer hosts that haven’t been attempted already,
- // the builtin `envoy.retry_host_predicates.previous_hosts` predicate can be used.
- xdsfilters.RetryPreviousHosts,
- },
  // TODO: allow this to be configured via API.
  HostSelectionRetryMaxAttempts: 5,
  }
  return &policy
 }
 
+// DefaultConsistentHashPolicy gets a copy of the default retry policy without previous host predicate.
+// When Consistent Hashing is enabled, we don't want to use other hosts during retries.
+func DefaultConsistentHashPolicy() *route.RetryPolicy {
+ return defaultPolicy()
+}
+
 // ConvertPolicy converts the given Istio retry policy to an Envoy policy.
 //
 // If in is nil, DefaultPolicy is returned.
@@ -61,10 +72,16 @@ func DefaultPolicy() *route.RetryPolicy {
 // is appended when encountering parts that are valid HTTP status codes.
 //
 // - PerTryTimeout: set from in.PerTryTimeout (if specified)
-func ConvertPolicy(in *networking.HTTPRetry) *route.RetryPolicy {
+func ConvertPolicy(in *networking.HTTPRetry, hashPolicy bool) *route.RetryPolicy {
+ var out *route.RetryPolicy
+ if hashPolicy {
+ out = DefaultConsistentHashPolicy()
+ } else {
+ out = DefaultPolicy()
+ }
  if in == nil {
  // No policy was set, use a default.
- return DefaultPolicy()
+ return out
  }
 
  if in.Attempts <= 0 {
@@ -73,7 +90,6 @@ func ConvertPolicy(in *networking.HTTPRetry) *route.RetryPolicy {
  }
 
  // A policy was specified. Start with the default and override with user-provided fields where appropriate.
- out := DefaultPolicy()
  out.NumRetries = &wrappers.UInt32Value{Value: uint32(in.Attempts)}
 
  if in.RetryOn != "" {

pilot/pkg/networking/core/route/retry/retry_test.go

@@ -218,7 +218,7 @@ func TestRetry(t *testing.T) {
  for _, tc := range testCases {
  t.Run(tc.name, func(t *testing.T) {
  g := NewWithT(t)
- policy := retry.ConvertPolicy(tc.route.Retries)
+ policy := retry.ConvertPolicy(tc.route.Retries, false)
  if tc.assertFunc != nil {
  tc.assertFunc(g, policy)
  }

pilot/pkg/networking/core/route/route.go

@@ -553,14 +553,7 @@ func applyHTTPRouteDestination(
  listenerPort int,
  hashByDestination DestinationHashMap,
 ) []host.Name {
- policy := in.Retries
- if policy == nil {
- // No VS policy set, use mesh defaults
- policy = mesh.GetDefaultHttpRetryPolicy()
- }
- action := &route.RouteAction{
- RetryPolicy: retry.ConvertPolicy(policy),
- }
+ action := &route.RouteAction{}
 
  setTimeout(action, in.Timeout, node)
 
@@ -622,8 +615,16 @@ func applyHTTPRouteDestination(
  }
 
  var hostnames []host.Name
+ policy := in.Retries
+ if policy == nil {
+ // No VS policy set, use mesh defaults
+ policy = mesh.GetDefaultHttpRetryPolicy()
+ }
+ consistentHash := false
  if len(in.Route) == 1 {
  hostnames = append(hostnames, processDestination(in.Route[0], serviceRegistry, listenerPort, hashByDestination, out, action))
+ hash := hashByDestination[in.Route[0]]
+ consistentHash = hash != nil
  } else {
  weighted := make([]*route.WeightedCluster_ClusterWeight, 0)
  for _, dst := range in.Route {
@@ -641,6 +642,7 @@ func applyHTTPRouteDestination(
  },
  }
  }
+ action.RetryPolicy = retry.ConvertPolicy(policy, consistentHash)
  return hostnames
 }
 
@@ -1290,7 +1292,7 @@ func setTimeout(action *route.RouteAction, vsTimeout *durationpb.Duration, node
 func BuildDefaultHTTPOutboundRoute(clusterName string, operation string, mesh *meshconfig.MeshConfig) *route.Route {
  out := buildDefaultHTTPRoute(clusterName, operation)
  // Add a default retry policy for outbound routes.
- out.GetRoute().RetryPolicy = retry.ConvertPolicy(mesh.GetDefaultHttpRetryPolicy())
+ out.GetRoute().RetryPolicy = retry.ConvertPolicy(mesh.GetDefaultHttpRetryPolicy(), false)
  setTimeout(out.GetRoute(), nil, nil)
  return out
 }

pilot/pkg/networking/core/route/route_test.go

@@ -270,6 +270,7 @@ func TestBuildHTTPRoutes(t *testing.T) {
  g.Expect(routes[1].StatPrefix).To(Equal(""))
  g.Expect(routes[2].GetMatch().GetPrefix()).To(Equal("/bar"))
  g.Expect(routes[2].StatPrefix).To(Equal(""))
+ g.Expect(len(routes[0].GetRoute().GetRetryPolicy().RetryHostPredicate)).To(Equal(1))
  })
 
  t.Run("for virtual service with exact matching on JWT claims", func(t *testing.T) {
@@ -513,6 +514,7 @@ func TestBuildHTTPRoutes(t *testing.T) {
  },
  }
  g.Expect(routes[0].GetRoute().GetHashPolicy()).To(ConsistOf(hashPolicy))
+ g.Expect(len(routes[0].GetRoute().GetRetryPolicy().RetryHostPredicate)).To(Equal(0))
  })
 
  t.Run("for virtual service with query param based ring hash", func(t *testing.T) {
@@ -560,6 +562,7 @@ func TestBuildHTTPRoutes(t *testing.T) {
  },
  }
  g.Expect(routes[0].GetRoute().GetHashPolicy()).To(ConsistOf(hashPolicy))
+ g.Expect(len(routes[0].GetRoute().GetRetryPolicy().RetryHostPredicate)).To(Equal(0))
  })
 
  t.Run("for virtual service with subsets with ring hash", func(t *testing.T) {