From 0cd6b15620601de3021abbca1e310120a566795d Mon Sep 17 00:00:00 2001
From: sodaRyCN <35725024+sodaRyCN@users.noreply.github.com>
Date: Tue, 29 Mar 2022 14:45:43 +0800
Subject: [PATCH] valid circuit-breaker filter spec by correlation (#551)

* valid circuit-breaker filter spec by correlation

* fix  circuit-breaker filter failure legality validate

* fix error message
---
 pkg/filter/circuitbreaker/circuitbreaker.go   | 30 ++++++-
 .../circuitbreaker/circuitbreaker_test.go     | 81 +++++++++++++++++++
 2 files changed, 107 insertions(+), 4 deletions(-)

diff --git a/pkg/filter/circuitbreaker/circuitbreaker.go b/pkg/filter/circuitbreaker/circuitbreaker.go
index 10c4d06919..3ff64a8033 100644
--- a/pkg/filter/circuitbreaker/circuitbreaker.go
+++ b/pkg/filter/circuitbreaker/circuitbreaker.go
@@ -49,8 +49,8 @@ type (
 	Policy struct {
 		Name                             string `yaml:"name" jsonschema:"required"`
 		SlidingWindowType                string `yaml:"slidingWindowType"  jsonschema:"omitempty,enum=COUNT_BASED,enum=TIME_BASED"`
-		FailureRateThreshold             uint8  `yaml:"failureRateThreshold" jsonschema:"omitempty,minimum=1,maximum=100"`
-		SlowCallRateThreshold            uint8  `yaml:"slowCallRateThreshold" jsonschema:"omitempty,minimum=1,maximum=100"`
+		FailureRateThreshold             uint8  `yaml:"failureRateThreshold" jsonschema:"omitempty,maximum=100"`
+		SlowCallRateThreshold            uint8  `yaml:"slowCallRateThreshold" jsonschema:"omitempty,maximum=100"`
 		CountingNetworkError             bool   `yaml:"countingNetworkError" jsonschema:"omitempty"`
 		SlidingWindowSize                uint32 `yaml:"slidingWindowSize" jsonschema:"omitempty,minimum=1"`
 		PermittedNumberOfCallsInHalfOpen uint32 `yaml:"permittedNumberOfCallsInHalfOpenState" jsonschema:"omitempty"`
@@ -87,8 +87,8 @@ type (
 	}
 )
 
-// Validate implements custom validation for Spec
-func (spec Spec) Validate() error {
+// check policy of url usage whether defined
+func (spec Spec) validateURLPoliciesUsage() error {
 URLLoop:
 	for _, u := range spec.URLs {
 		name := u.PolicyRef
@@ -108,6 +108,28 @@ URLLoop:
 	return nil
 }
 
+func (spec Spec) validatePoliciesSpec() error {
+	for _, p := range spec.Policies {
+		if p.FailureRateThreshold != 0 && len(p.FailureStatusCodes) == 0 && !p.CountingNetworkError {
+			return fmt.Errorf("policy '%s' has set failure threshold and countingNetworkError is false, but not set failure status code", p.Name)
+		}
+	}
+	return nil
+}
+
+// Validate implements custom validation for Spec
+func (spec Spec) Validate() error {
+	err := spec.validateURLPoliciesUsage()
+	if err != nil {
+		return err
+	}
+	err = spec.validatePoliciesSpec()
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
 func (url *URLRule) buildPolicy() *libcb.Policy {
 	policy := libcb.Policy{
 		FailureRateThreshold:             url.policy.FailureRateThreshold,
diff --git a/pkg/filter/circuitbreaker/circuitbreaker_test.go b/pkg/filter/circuitbreaker/circuitbreaker_test.go
index 94792ffcd0..2bdb58e34e 100644
--- a/pkg/filter/circuitbreaker/circuitbreaker_test.go
+++ b/pkg/filter/circuitbreaker/circuitbreaker_test.go
@@ -159,3 +159,84 @@ func TestBuildPolicy(t *testing.T) {
 		t.Error("wait duration in open is not 1m")
 	}
 }
+
+func TestValidate(t *testing.T) {
+	t.Run("invalidFailureCode", func(t *testing.T) {
+		const yamlSpec = `
+kind: CircuitBreaker
+name: circuitbreaker
+policies:
+- name: default
+  failureRateThreshold: 50
+  slidingWindowType: COUNT_BASED
+  slidingWindowSize: 10
+defaultPolicyRef: default
+urls:
+- methods: []
+  url:
+    exact: /circuitbreak
+    prefix:
+    regex:
+`
+		rawSpec := make(map[string]interface{})
+		yamltool.Unmarshal([]byte(yamlSpec), &rawSpec)
+
+		_, err := httppipeline.NewFilterSpec(rawSpec, nil)
+		if err == nil {
+			t.Errorf("set failure threshold and not set failure code, that did not fail")
+		}
+	})
+	t.Run("validFailureLegalityByCountingNetworkError", func(t *testing.T) {
+		const yamlSpec = `
+kind: CircuitBreaker
+name: circuitbreaker
+policies:
+- name: default
+  failureRateThreshold: 50
+  slidingWindowType: COUNT_BASED
+  slidingWindowSize: 10
+  countingNetworkError: true
+defaultPolicyRef: default
+urls:
+- methods: []
+  url:
+    exact: /circuitbreak
+    prefix:
+    regex:
+`
+		rawSpec := make(map[string]interface{})
+		yamltool.Unmarshal([]byte(yamlSpec), &rawSpec)
+
+		_, err := httppipeline.NewFilterSpec(rawSpec, nil)
+		if err != nil {
+			t.Errorf("set failure threshold and countingNetworkError is true, not set failure code, that is fail")
+		}
+	})
+
+	t.Run("validFailureLegalityByFailureStatusCodes", func(t *testing.T) {
+		const yamlSpec = `
+kind: CircuitBreaker
+name: circuitbreaker
+policies:
+- name: default
+  failureRateThreshold: 50
+  slidingWindowType: COUNT_BASED
+  slidingWindowSize: 10
+  failureStatusCodes: [500]
+defaultPolicyRef: default
+urls:
+- methods: []
+  url:
+    exact: /circuitbreak
+    prefix:
+    regex:
+`
+		rawSpec := make(map[string]interface{})
+		yamltool.Unmarshal([]byte(yamlSpec), &rawSpec)
+
+		_, err := httppipeline.NewFilterSpec(rawSpec, nil)
+		if err != nil {
+			t.Errorf("set failure threshold and countingNetworkError is true, not set failure code, that is fail")
+		}
+	})
+}