[servicecomb-service-center] 01/01: [fix]failed to create match-group policy (#1335)

[li...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

littlecui pushed a commit to branch mod
in repository https://gitbox.apache.org/repos/asf/servicecomb-service-center.git

commit 4684e466a980d9513c1136650576dc9cad76da3e
Author: little-cui <su...@qq.com>
AuthorDate: Tue Aug 23 11:19:49 2022 +0800

    [fix]failed to create match-group policy (#1335)
    
    * [fix]failed to create match-group policy
    
    * [fix]failed to create match-group policy
---
 server/ext/policy/builtin.go        | 157 ++++++++++++++----------------------
 server/ext/policy/builtin_test.go   |   5 +-
 server/resource/gov/gov_resource.go |  12 +--
 server/service/grc/policy.go        |   5 +-
 server/service/grc/validate.go      |  13 ---
 5 files changed, 65 insertions(+), 127 deletions(-)

diff --git a/server/ext/policy/builtin.go b/server/ext/policy/builtin.go
index b32c34ab..36720b0c 100644
--- a/server/ext/policy/builtin.go
+++ b/server/ext/policy/builtin.go
@@ -23,86 +23,82 @@ import (
 )
 
 var (
-	notEmpty         = int64(1)
-	notNegative      = float64(0)
-	matchGroupSchema = &spec.Schema{
+	notEmpty             = int64(1)
+	notNegative          = float64(0)
+	notEmptyStringSchema = spec.Schema{
+		SchemaProps: spec.SchemaProps{
+			Type:      []string{"string"},
+			MinLength: &notEmpty,
+		},
+	}
+	notNegativeIntegerSchema = spec.Schema{
+		SchemaProps: spec.SchemaProps{
+			Type:    []string{"integer"},
+			Minimum: &notNegative,
+		},
+	}
+	notNegativeNumberSchema = spec.Schema{
+		SchemaProps: spec.SchemaProps{
+			Type:    []string{"number"},
+			Minimum: &notNegative,
+		},
+	}
+	hasPropertiesSchema = spec.Schema{
+		SchemaProps: spec.SchemaProps{
+			Type:          []string{"object"},
+			MinProperties: &notEmpty,
+		},
+	}
+	matchSchema = spec.Schema{
 		SchemaProps: spec.SchemaProps{
 			Type:     []string{"object"},
-			Required: []string{"matches", "alias"},
+			Required: []string{"name"},
 			Properties: map[string]spec.Schema{
-				"alias": {
-					SchemaProps: spec.SchemaProps{
-						Type:      []string{"string"},
-						MinLength: &notEmpty,
-					},
-				},
-				"matches": {
+				"name":    notEmptyStringSchema,
+				"apiPath": hasPropertiesSchema,
+				"headers": hasPropertiesSchema,
+				"method": {
 					SchemaProps: spec.SchemaProps{
 						Type:     []string{"array"},
 						MinItems: &notEmpty,
 						Items: &spec.SchemaOrArray{
 							Schema: &spec.Schema{
 								SchemaProps: spec.SchemaProps{
-									Type:     []string{"object"},
-									Required: []string{"name"},
-									Properties: map[string]spec.Schema{
-										"name": {
-											SchemaProps: spec.SchemaProps{
-												Type:      []string{"string"},
-												MinLength: &notEmpty,
-											},
-										},
-										"apiPath": {
-											SchemaProps: spec.SchemaProps{
-												Type:      []string{"string"},
-												MinLength: &notEmpty,
-											},
-										},
-										"method": {
-											SchemaProps: spec.SchemaProps{
-												Type:     []string{"array"},
-												MinItems: &notEmpty,
-												Items: &spec.SchemaOrArray{
-													Schema: &spec.Schema{
-														SchemaProps: spec.SchemaProps{
-															Type: []string{"string"},
-															Enum: []interface{}{"GET", "POST", "DELETE", "PUT", "PATCH"},
-														},
-													},
-												},
-											},
-										},
-										"headers": {
-											SchemaProps: spec.SchemaProps{
-												Type:          []string{"object"},
-												MinProperties: &notEmpty,
-											},
-										},
-									},
+									Type: []string{"string"},
+									Enum: []interface{}{"GET", "POST", "DELETE", "PUT", "PATCH"},
 								},
 							},
 						},
 					},
 				},
 			},
+		},
+	}
+	matchesSchema = spec.Schema{
+		SchemaProps: spec.SchemaProps{
+			Type:     []string{"array"},
+			MinItems: &notEmpty,
+			Items: &spec.SchemaOrArray{
+				Schema: &matchSchema,
+			},
+		},
+	}
+	matchGroupSchema = &spec.Schema{
+		SchemaProps: spec.SchemaProps{
+			Type:     []string{"object"},
+			Required: []string{"matches", "alias"},
+			Properties: map[string]spec.Schema{
+				"alias":   notEmptyStringSchema,
+				"matches": matchesSchema,
+			},
 		}}
 	retrySchema = &spec.Schema{
 		SchemaProps: spec.SchemaProps{
 			Type:          []string{"object"},
 			MinProperties: &notEmpty,
 			Properties: map[string]spec.Schema{
-				"maxAttempts": {
-					SchemaProps: spec.SchemaProps{
-						Type:    []string{"integer"},
-						Minimum: &notNegative,
-					},
-				},
-				"retryOnSame": {
-					SchemaProps: spec.SchemaProps{
-						Type:    []string{"integer"},
-						Minimum: &notNegative,
-					},
-				},
+				"maxAttempts": notNegativeIntegerSchema,
+				"retryOnSame": notNegativeIntegerSchema,
 			},
 		},
 	}
@@ -110,14 +106,6 @@ var (
 		SchemaProps: spec.SchemaProps{
 			Type:     []string{"object"},
 			Required: []string{"rate"},
-			Properties: map[string]spec.Schema{
-				"rate": {
-					SchemaProps: spec.SchemaProps{
-						Type:    []string{"number"},
-						Minimum: &notNegative,
-					},
-				},
-			},
 		},
 	}
 	loadbalanceSchema = &spec.Schema{
@@ -125,12 +113,7 @@ var (
 			Type:     []string{"object"},
 			Required: []string{"rule"},
 			Properties: map[string]spec.Schema{
-				"rule": {
-					SchemaProps: spec.SchemaProps{
-						Type:      []string{"string"},
-						MinLength: &notEmpty,
-					},
-				},
+				"rule": notEmptyStringSchema,
 			},
 		}}
 	circuitBreakerSchema = &spec.Schema{
@@ -138,12 +121,7 @@ var (
 			Type:     []string{"object"},
 			Required: []string{"minimumNumberOfCalls"},
 			Properties: map[string]spec.Schema{
-				"minimumNumberOfCalls": {
-					SchemaProps: spec.SchemaProps{
-						Type:    []string{"integer"},
-						Minimum: &notNegative,
-					},
-				},
+				"minimumNumberOfCalls": notNegativeIntegerSchema,
 			},
 		}}
 	instanceIsolationSchema = &spec.Schema{
@@ -151,12 +129,7 @@ var (
 			Type:     []string{"object"},
 			Required: []string{"minimumNumberOfCalls"},
 			Properties: map[string]spec.Schema{
-				"minimumNumberOfCalls": {
-					SchemaProps: spec.SchemaProps{
-						Type:    []string{"integer"},
-						Minimum: &notNegative,
-					},
-				},
+				"minimumNumberOfCalls": notNegativeIntegerSchema,
 			},
 		}}
 	faultInjectionSchema = &spec.Schema{
@@ -164,12 +137,7 @@ var (
 			Type:     []string{"object"},
 			Required: []string{"percentage"},
 			Properties: map[string]spec.Schema{
-				"percentage": {
-					SchemaProps: spec.SchemaProps{
-						Type:    []string{"number"},
-						Minimum: &notNegative,
-					},
-				},
+				"percentage": notNegativeNumberSchema,
 			},
 		}}
 	bulkheadSchema = &spec.Schema{
@@ -177,12 +145,7 @@ var (
 			Type:     []string{"object"},
 			Required: []string{"maxConcurrentCalls"},
 			Properties: map[string]spec.Schema{
-				"maxConcurrentCalls": {
-					SchemaProps: spec.SchemaProps{
-						Type:    []string{"integer"},
-						Minimum: &notNegative,
-					},
-				},
+				"maxConcurrentCalls": notNegativeIntegerSchema,
 			},
 		}}
 )
diff --git a/server/ext/policy/builtin_test.go b/server/ext/policy/builtin_test.go
index c0136edf..24f523d2 100644
--- a/server/ext/policy/builtin_test.go
+++ b/server/ext/policy/builtin_test.go
@@ -68,7 +68,7 @@ func TestValidatePolicySpec(t *testing.T) {
 			map[string]interface{}{"name": "1", "apiPath": ""},
 		}, "alias": "1"}}, true},
 		{kindMatchGroup, args{kind: kindMatchGroup, spec: map[string]interface{}{"matches": []interface{}{
-			map[string]interface{}{"name": "1", "apiPath": "1"},
+			map[string]interface{}{"name": "1", "apiPath": map[string]interface{}{"prefix": "/"}},
 		}, "alias": "1"}}, false},
 		{kindMatchGroup, args{kind: kindMatchGroup, spec: map[string]interface{}{"matches": []interface{}{
 			map[string]interface{}{"name": "1", "headers": ""},
@@ -98,10 +98,9 @@ func TestValidatePolicySpec(t *testing.T) {
 
 		{kindRateLimiting, args{kind: kindRateLimiting, spec: ""}, true},
 		{kindRateLimiting, args{kind: kindRateLimiting, spec: map[string]interface{}{}}, true},
-		{kindRateLimiting, args{kind: kindRateLimiting, spec: map[string]interface{}{"rate": -1}}, true},
 		{kindRateLimiting, args{kind: kindRateLimiting, spec: map[string]interface{}{"rate": 1}}, false},
 		{kindRateLimiting, args{kind: kindRateLimiting, spec: map[string]interface{}{"rate": 0.5}}, false},
-		{kindRateLimiting, args{kind: kindRateLimiting, spec: map[string]interface{}{"rate": "1"}}, true},
+		{kindRateLimiting, args{kind: kindRateLimiting, spec: map[string]interface{}{"rate": "1"}}, false},
 
 		{kindLoadbalance, args{kind: kindLoadbalance, spec: ""}, true},
 		{kindLoadbalance, args{kind: kindLoadbalance, spec: map[string]interface{}{}}, true},
diff --git a/server/resource/gov/gov_resource.go b/server/resource/gov/gov_resource.go
index 3c639565..bf16fa09 100644
--- a/server/resource/gov/gov_resource.go
+++ b/server/resource/gov/gov_resource.go
@@ -70,11 +70,6 @@ func (t *Governance) Create(w http.ResponseWriter, r *http.Request) {
 	}
 	id, err := grc.Create(r.Context(), kind, project, p)
 	if err != nil {
-		if _, ok := err.(*grc.ErrIllegalItem); ok {
-			log.Error("", err)
-			rest.WriteError(w, discovery.ErrInvalidParams, err.Error())
-			return
-		}
 		processError(w, err, "create gov data err")
 		return
 	}
@@ -102,18 +97,13 @@ func (t *Governance) Put(w http.ResponseWriter, r *http.Request) {
 	}
 	err = grc.ValidatePolicySpec(kind, p.Spec)
 	if err != nil {
-		log.Error("validate policy err", err)
+		log.Error(fmt.Sprintf("validate policy [%s] err", kind), err)
 		rest.WriteError(w, discovery.ErrInvalidParams, err.Error())
 		return
 	}
 	log.Info(fmt.Sprintf("update %v", &p))
 	err = grc.Update(r.Context(), kind, id, project, p)
 	if err != nil {
-		if _, ok := err.(*grc.ErrIllegalItem); ok {
-			log.Error("", err)
-			rest.WriteError(w, discovery.ErrInvalidParams, err.Error())
-			return
-		}
 		processError(w, err, "put gov err")
 		return
 	}
diff --git a/server/service/grc/policy.go b/server/service/grc/policy.go
index c22e6639..201f9a2f 100644
--- a/server/service/grc/policy.go
+++ b/server/service/grc/policy.go
@@ -19,7 +19,6 @@
 package grc
 
 import (
-	"errors"
 	"fmt"
 	"strings"
 
@@ -48,7 +47,7 @@ func ValidatePolicySpec(kind string, spec interface{}) error {
 	schema, ok := policySchemas[kind]
 	if !ok {
 		log.Warn(fmt.Sprintf("can not recognize policy %s", kind))
-		return &ErrIllegalItem{"not support kind yet", kind}
+		return fmt.Errorf("not support kind[%s] yet", kind)
 	}
 	validator := validate.NewSchemaValidator(schema, nil, kind, strfmt.Default)
 	errs := validator.Validate(spec).Errors
@@ -57,7 +56,7 @@ func ValidatePolicySpec(kind string, spec interface{}) error {
 		for _, err := range errs {
 			str = append(str, err.Error())
 		}
-		return errors.New(strings.Join(str, "; "))
+		return fmt.Errorf("illegal policy[%s] spec, msg: %s", kind, strings.Join(str, "; "))
 	}
 	return nil
 }
diff --git a/server/service/grc/validate.go b/server/service/grc/validate.go
index 26ed0870..06e39fd2 100644
--- a/server/service/grc/validate.go
+++ b/server/service/grc/validate.go
@@ -17,10 +17,6 @@
 
 package grc
 
-import (
-	"fmt"
-)
-
 const (
 	KeyPrefix       = "servicecomb."
 	KindMatchGroup  = "match-group"
@@ -31,12 +27,3 @@ const (
 	KeyEnvironment  = "environment"
 	EnvAll          = "all"
 )
-
-type ErrIllegalItem struct {
-	err string
-	val interface{}
-}
-
-func (e *ErrIllegalItem) Error() string {
-	return fmt.Sprintf("illegal item : %v , msg: %s", e.val, e.err)
-}