Tune default outlier detection parameters for remote endpoints

admiral/pkg/clusters/handler.go

@@ -3,6 +3,7 @@ package clusters
 import (
 	"bytes"
 	"fmt"
+	"net"
 	"reflect"
 	"sort"
 	"strings"
@@ -61,20 +62,15 @@ func getIstioResourceName(host string, suffix string) string {
 	return strings.ToLower(host) + suffix
 }
 
-func getDestinationRule(host string, locality string, gtpTrafficPolicy *model.TrafficPolicy) *v1alpha32.DestinationRule {
+func getDestinationRule(se *v1alpha32.ServiceEntry, locality string, gtpTrafficPolicy *model.TrafficPolicy) *v1alpha32.DestinationRule {
 	var dr = &v1alpha32.DestinationRule{}
-	dr.Host = host
+	dr.Host = se.Hosts[0]
 	dr.TrafficPolicy = &v1alpha32.TrafficPolicy{Tls: &v1alpha32.TLSSettings{Mode: v1alpha32.TLSSettings_ISTIO_MUTUAL}}
 	processGtp := true
 	if len(locality) == 0 {
-		log.Warnf(LogErrFormat, "Process", "GlobalTrafficPolicy", host, "", "Skipping gtp processing, locality of the cluster nodes cannot be determined. Is this minikube?")
+		log.Warnf(LogErrFormat, "Process", "GlobalTrafficPolicy", dr.Host, "", "Skipping gtp processing, locality of the cluster nodes cannot be determined. Is this minikube?")
 		processGtp = false
 	}
-	outlierDetection := &v1alpha32.OutlierDetection{
-		BaseEjectionTime:      &types.Duration{Seconds: 300},
-		Consecutive_5XxErrors: &types.UInt32Value{Value: uint32(10)},
-		Interval:              &types.Duration{Seconds: 60},
-	}
 	if gtpTrafficPolicy != nil && processGtp {
 		var loadBalancerSettings = &v1alpha32.LoadBalancerSettings{
 			LbPolicy: &v1alpha32.LoadBalancerSettings_Simple{Simple: v1alpha32.LoadBalancerSettings_ROUND_ROBIN},
@@ -103,10 +99,33 @@ func getDestinationRule(host string, locality string, gtpTrafficPolicy *model.Tr
 			dr.TrafficPolicy.LoadBalancer = loadBalancerSettings
 		}
 	}
-	dr.TrafficPolicy.OutlierDetection = outlierDetection
+	dr.TrafficPolicy.OutlierDetection = getOutlierDetection(se, locality, gtpTrafficPolicy)
 	return dr
 }
 
+func getOutlierDetection(se *v1alpha32.ServiceEntry, locality string, gtpTrafficPolicy *model.TrafficPolicy) *v1alpha32.OutlierDetection {
+
+	outlierDetection := &v1alpha32.OutlierDetection{
+		BaseEjectionTime:      &types.Duration{Seconds: 300},
+		ConsecutiveGatewayErrors: &types.UInt32Value{Value: uint32(50)},
+		Interval:              &types.Duration{Seconds: 60},
+	}
+
+	//Scenario 1: Only one endpoint present and is local service (ends in svc.cluster.local) - no outlier detection (optimize this for headless services in future?)
+	if len(se.Endpoints) == 1 && (strings.Contains(se.Endpoints[0].Address, common.DotLocalDomainSuffix) || net.ParseIP(se.Endpoints[0].Address).To4() != nil) {
+		return nil
+	} else if len(se.Endpoints) == 1 {
+		//Scenario 2: Only one endpoint present and is remote - outlier detection with 34% ejection (protection against zone specific issues)
+		outlierDetection.MaxEjectionPercent = 34
+	} else {
+		//Scenario 3: Two endpoints present each with different locality and both remote - outlier detection with 100% ejection
+		//Scenario 4: Two endpoints present each with different locality with one local and other remote - outlier detection with 100% ejection
+		//for service entries with more than 2 endpoints eject 100% to failover to other endpoint within or outside the same region
+		outlierDetection.MaxEjectionPercent = 100
+	}
+	return outlierDetection
+}
+
 func (se *ServiceEntryHandler) Added(obj *v1alpha3.ServiceEntry) {
 	if IgnoreIstioResource(obj.Spec.ExportTo, obj.Annotations, obj.Namespace) {
 		log.Infof(LogFormat, "Add", "ServiceEntry", obj.Name, se.ClusterID, "Skipping resource from namespace="+obj.Namespace)

admiral/pkg/clusters/handler_test.go

@@ -176,10 +176,16 @@ func TestGetDestinationRule(t *testing.T) {
 	//Do setup here
 	outlierDetection := &v1alpha3.OutlierDetection{
 		BaseEjectionTime:      &types.Duration{Seconds: 300},
-		Consecutive_5XxErrors: &types.UInt32Value{Value: 10},
-		Interval:              &types.Duration{Seconds: 60}}
+		ConsecutiveGatewayErrors: &types.UInt32Value{Value: 50},
+		Interval:              &types.Duration{Seconds: 60},
+		MaxEjectionPercent: 100,
+	}
 	mTLS := &v1alpha3.TrafficPolicy{Tls: &v1alpha3.TLSSettings{Mode: v1alpha3.TLSSettings_ISTIO_MUTUAL}, OutlierDetection: outlierDetection}
 
+
+	se := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"},Endpoints:[]*v1alpha3.ServiceEntry_Endpoint{
+		{Address: "east.com", Locality: "us-east-2"}, {Address: "west.com", Locality: "us-west-2"},
+	}}
 	noGtpDr := v1alpha3.DestinationRule{
 		Host:          "qa.myservice.global",
 		TrafficPolicy: mTLS,
@@ -243,35 +249,35 @@ func TestGetDestinationRule(t *testing.T) {
 	//Struct of test case info. Name is required.
 	testCases := []struct {
 		name            string
-		host            string
+		se              *v1alpha3.ServiceEntry
 		locality        string
 		gtpPolicy       *model.TrafficPolicy
 		destinationRule *v1alpha3.DestinationRule
 	}{
 		{
 			name:            "Should handle a nil GTP",
-			host:            "qa.myservice.global",
+			se:              se,
 			locality:        "uswest2",
 			gtpPolicy:       nil,
 			destinationRule: &noGtpDr,
 		},
 		{
 			name:            "Should return default DR with empty locality",
-			host:            "qa.myservice.global",
+			se:              se,
 			locality:        "",
 			gtpPolicy:       failoverGTPPolicy,
 			destinationRule: &noGtpDr,
 		},
 		{
 			name:            "Should handle a topology GTP",
-			host:            "qa.myservice.global",
+			se:              se,
 			locality:        "uswest2",
 			gtpPolicy:       topologyGTPPolicy,
 			destinationRule: &basicGtpDr,
 		},
 		{
 			name:            "Should handle a failover GTP",
-			host:            "qa.myservice.global",
+			se:              se,
 			locality:        "uswest2",
 			gtpPolicy:       failoverGTPPolicy,
 			destinationRule: &failoverGtpDr,
@@ -281,14 +287,105 @@ func TestGetDestinationRule(t *testing.T) {
 	//Run the test for every provided case
 	for _, c := range testCases {
 		t.Run(c.name, func(t *testing.T) {
-			result := getDestinationRule(c.host, c.locality, c.gtpPolicy)
+			result := getDestinationRule(c.se, c.locality, c.gtpPolicy)
 			if !cmp.Equal(result, c.destinationRule) {
 				t.Fatalf("DestinationRule Mismatch. Diff: %v", cmp.Diff(result, c.destinationRule))
 			}
 		})
 	}
 }
 
+func TestGetOutlierDetection(t *testing.T) {
+	//Do setup here
+	outlierDetection := &v1alpha3.OutlierDetection{
+		BaseEjectionTime:      &types.Duration{Seconds: 300},
+		ConsecutiveGatewayErrors: &types.UInt32Value{Value: 50},
+		Interval:              &types.Duration{Seconds: 60},
+		MaxEjectionPercent: 100,
+	}
+
+	outlierDetectionOneHostRemote := &v1alpha3.OutlierDetection{
+		BaseEjectionTime:      &types.Duration{Seconds: 300},
+		ConsecutiveGatewayErrors: &types.UInt32Value{Value: 50},
+		Interval:              &types.Duration{Seconds: 60},
+		MaxEjectionPercent: 34,
+	}
+
+	topologyGTPPolicy := &model.TrafficPolicy{
+		LbType: model.TrafficPolicy_TOPOLOGY,
+		Target: []*model.TrafficGroup{
+			{
+				Region: "us-west-2",
+				Weight: 100,
+			},
+		},
+	}
+
+	se := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"},Endpoints:[]*v1alpha3.ServiceEntry_Endpoint{
+		{Address: "east.com", Locality: "us-east-2"}, {Address: "west.com", Locality: "us-west-2"},
+	}}
+
+	seOneHostRemote := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"},Endpoints:[]*v1alpha3.ServiceEntry_Endpoint{
+		{Address: "east.com", Locality: "us-east-2"},
+	}}
+
+	seOneHostLocal := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"},Endpoints:[]*v1alpha3.ServiceEntry_Endpoint{
+		{Address: "hello.ns.svc.cluster.local", Locality: "us-east-2"},
+	}}
+
+	seOneHostRemoteIp := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"},Endpoints:[]*v1alpha3.ServiceEntry_Endpoint{
+		{Address: "95.45.25.34", Locality: "us-east-2"},
+	}}
+
+	//Struct of test case info. Name is required.
+	testCases := []struct {
+		name            string
+		se              *v1alpha3.ServiceEntry
+		locality        string
+		gtpPolicy       *model.TrafficPolicy
+		outlierDetection *v1alpha3.OutlierDetection
+	}{
+		{
+			name:            "Should return nil for cluster local only endpoint",
+			se:              seOneHostLocal,
+			locality:        "uswest2",
+			gtpPolicy:       topologyGTPPolicy,
+			outlierDetection: nil,
+		},
+		{
+			name:            "Should return nil for one IP endpoint",
+			se:              seOneHostRemoteIp,
+			locality:        "uswest2",
+			gtpPolicy:       topologyGTPPolicy,
+			outlierDetection: nil,
+		},
+		{
+			name:            "Should return 34% ejection for remote endpoint with one entry",
+			se:              seOneHostRemote,
+			locality:        "uswest2",
+			gtpPolicy:       topologyGTPPolicy,
+			outlierDetection: outlierDetectionOneHostRemote,
+		},
+		{
+			name:            "Should return 100% ejection for two remote endpoints",
+			se:              se,
+			locality:        "uswest2",
+			gtpPolicy:       topologyGTPPolicy,
+			outlierDetection: outlierDetection,
+		},
+	}
+
+	//Run the test for every provided case
+	for _, c := range testCases {
+		t.Run(c.name, func(t *testing.T) {
+			result := getOutlierDetection(c.se, c.locality, c.gtpPolicy)
+			if !cmp.Equal(result, c.outlierDetection) {
+				t.Fatalf("OutlierDetection Mismatch. Diff: %v", cmp.Diff(result, c.outlierDetection))
+			}
+		})
+	}
+}
+
 func TestHandleVirtualServiceEvent(t *testing.T) {
 	tooManyHosts := v1alpha32.VirtualService{
 		Spec: v1alpha3.VirtualService{

admiral/pkg/clusters/serviceentry.go

@@ -469,7 +469,7 @@ func createSeAndDrSetFromGtp(env, region string, se *networking.ServiceEntry, gl
 			var seDr = &SeDrTuple{
 				DrName:          drName,
 				SeName:          seName,
-				DestinationRule: getDestinationRule(host, region, gtpTrafficPolicy),
+				DestinationRule: getDestinationRule(modifiedSe, region, gtpTrafficPolicy),
 				ServiceEntry:    modifiedSe,
 			}
 			seDrSet[host] = seDr
@@ -480,7 +480,7 @@ func createSeAndDrSetFromGtp(env, region string, se *networking.ServiceEntry, gl
 		var seDr = &SeDrTuple{
 			DrName:          defaultDrName,
 			SeName:          defaultSeName,
-			DestinationRule: getDestinationRule(se.Hosts[0], region, nil),
+			DestinationRule: getDestinationRule(se, region, nil),
 			ServiceEntry:    se,
 		}
 		seDrSet[se.Hosts[0]] = seDr