diff --git a/docs/operation/operation.md b/docs/operation/operation.md index 3c4b74bc79..2052387e9f 100644 --- 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 index 2de0161d01..cc5061954a 100644 --- 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 index ea3cc79463..51780a4f5d 100644 --- 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) {