Make min-drop-probability of PHC optional (#3067)

The default value (0.0) makes some sense and is equivalent to how PHC was working before. Signed-off-by: Roman Zavodskikh <[email protected]> Co-authored-by: Roman Zavodskikh <[email protected]>
zalando · May 7, 2024 · 75c404b · 75c404b
1 parent f15ffba
commit 75c404b
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 20 deletions.
diff --git a/docs/operation/operation.md b/docs/operation/operation.md
@@ -907,13 +907,17 @@ in previous period are, the stronger reduction is) amount of requests compared t
 
 Having this, we expect less requests to fail because a lot of them would be sent to endpoints that seem to be healthy instead.
 
-To enable this feature, you need to provide `-passive-health-check` option having all forementioned parameters
-(`period`, `min-requests`, `min-drop-probability`, `max-drop-probability`) defined,
-for instance: `-passive-health-check=period=1s,min-requests=10,min-drop-probability=0.05,max-drop-probability=0.9`.
+To enable this feature, you need to provide `-passive-health-check` option having forementioned parameters
+(`period`, `min-requests`, `min-drop-probability`, `max-drop-probability`) defined.
+`period`, `min-requests`, `max-drop-probability` are required parameters, it is not possible for PHC to be enabled without
+them explicitly defined by user. `min-drop-probability` is implicitly defined as `0.0` if not explicitly set by user.
+Valid examples of `-passive-health-check` are:
 
-You need to define all parameters on your side, there are no defaults, and skipper will not run if PHC params are passed only partially.
++ `-passive-health-check=period=1s,min-requests=10,min-drop-probability=0.05,max-drop-probability=0.9`
++ `-passive-health-check=period=1s,min-requests=10,max-drop-probability=0.9`
 
-However, Skipper will run without this feature, if no `-passive-health-check` is provided at all.
+If `-passive-health-check` option is provided, but some required parameters are not defined, Skipper will not start.
+Skipper will run without this feature, if no `-passive-health-check` is provided at all.
 
 The parameters of `-passive-health-check` option are:
 

diff --git a/proxy/proxy.go b/proxy/proxy.go
@@ -21,6 +21,7 @@ import (
 	"time"
 	"unicode/utf8"
 
+	"golang.org/x/exp/maps"
 	"golang.org/x/time/rate"
 
 	ot "github.com/opentracing/opentracing-go"
@@ -169,10 +170,16 @@ func InitPassiveHealthChecker(o map[string]string) (bool, *PassiveHealthCheck, e
 	}
 
 	result := &PassiveHealthCheck{}
-	keysInitialized := make(map[string]struct{})
+	requiredParams := map[string]struct{}{
+		"period":               {},
+		"max-drop-probability": {},
+		"min-requests":         {},
+	}
 
 	for key, value := range o {
+		delete(requiredParams, key)
 		switch key {
+		/* required parameters */
 		case "period":
 			period, err := time.ParseDuration(value)
 			if err != nil {
@@ -191,15 +198,6 @@ func InitPassiveHealthChecker(o map[string]string) (bool, *PassiveHealthCheck, e
 				return false, nil, fmt.Errorf("passive health check: invalid minRequests value: %s", value)
 			}
 			result.MinRequests = int64(minRequests)
-		case "min-drop-probability":
-			minDropProbability, err := strconv.ParseFloat(value, 64)
-			if err != nil {
-				return false, nil, fmt.Errorf("passive health check: invalid minDropProbability value: %s", value)
-			}
-			if minDropProbability < 0 || minDropProbability > 1 {
-				return false, nil, fmt.Errorf("passive health check: invalid minDropProbability value: %s", value)
-			}
-			result.MinDropProbability = minDropProbability
 		case "max-drop-probability":
 			maxDropProbability, err := strconv.ParseFloat(value, 64)
 			if err != nil {
@@ -209,15 +207,24 @@ func InitPassiveHealthChecker(o map[string]string) (bool, *PassiveHealthCheck, e
 				return false, nil, fmt.Errorf("passive health check: invalid maxDropProbability value: %s", value)
 			}
 			result.MaxDropProbability = maxDropProbability
+
+		/* optional parameters */
+		case "min-drop-probability":
+			minDropProbability, err := strconv.ParseFloat(value, 64)
+			if err != nil {
+				return false, nil, fmt.Errorf("passive health check: invalid minDropProbability value: %s", value)
+			}
+			if minDropProbability < 0 || minDropProbability > 1 {
+				return false, nil, fmt.Errorf("passive health check: invalid minDropProbability value: %s", value)
+			}
+			result.MinDropProbability = minDropProbability
 		default:
 			return false, nil, fmt.Errorf("passive health check: invalid parameter: key=%s,value=%s", key, value)
 		}
-
-		keysInitialized[key] = struct{}{}
 	}
 
-	if len(keysInitialized) != 4 {
-		return false, nil, fmt.Errorf("passive health check: missing required parameters")
+	if len(requiredParams) != 0 {
+		return false, nil, fmt.Errorf("passive health check: missing required parameters %+v", maps.Keys(requiredParams))
 	}
 	if result.MinDropProbability >= result.MaxDropProbability {
 		return false, nil, fmt.Errorf("passive health check: minDropProbability should be less than maxDropProbability")

diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go
@@ -2452,7 +2452,23 @@ func TestInitPassiveHealthChecker(t *testing.T) {
 			},
 			expectedEnabled: false,
 			expectedParams:  nil,
-			expectedError:   fmt.Errorf("passive health check: missing required parameters"),
+			expectedError:   fmt.Errorf("passive health check: missing required parameters [max-drop-probability]"),
+		},
+		{
+			inputArg: map[string]string{
+				"period":               "1m",
+				"min-requests":         "10",
+				"max-drop-probability": "0.9",
+				/* using default max-drop-probability */
+			},
+			expectedEnabled: true,
+			expectedParams: &PassiveHealthCheck{
+				Period:             1 * time.Minute,
+				MinRequests:        10,
+				MaxDropProbability: 0.9,
+				MinDropProbability: 0.0,
+			},
+			expectedError: nil,
 		},
 	} {
 		t.Run(fmt.Sprintf("case_%d", i), func(t *testing.T) {