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 <[email protected]> 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: ¬Empty, + }, + } + notNegativeIntegerSchema = spec.Schema{ + SchemaProps: spec.SchemaProps{ + Type: []string{"integer"}, + Minimum: ¬Negative, + }, + } + notNegativeNumberSchema = spec.Schema{ + SchemaProps: spec.SchemaProps{ + Type: []string{"number"}, + Minimum: ¬Negative, + }, + } + hasPropertiesSchema = spec.Schema{ + SchemaProps: spec.SchemaProps{ + Type: []string{"object"}, + MinProperties: ¬Empty, + }, + } + 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: ¬Empty, - }, - }, - "matches": { + "name": notEmptyStringSchema, + "apiPath": hasPropertiesSchema, + "headers": hasPropertiesSchema, + "method": { SchemaProps: spec.SchemaProps{ Type: []string{"array"}, MinItems: ¬Empty, 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: ¬Empty, - }, - }, - "apiPath": { - SchemaProps: spec.SchemaProps{ - Type: []string{"string"}, - MinLength: ¬Empty, - }, - }, - "method": { - SchemaProps: spec.SchemaProps{ - Type: []string{"array"}, - MinItems: ¬Empty, - 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: ¬Empty, - }, - }, - }, + Type: []string{"string"}, + Enum: []interface{}{"GET", "POST", "DELETE", "PUT", "PATCH"}, }, }, }, }, }, }, + }, + } + matchesSchema = spec.Schema{ + SchemaProps: spec.SchemaProps{ + Type: []string{"array"}, + MinItems: ¬Empty, + 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: ¬Empty, Properties: map[string]spec.Schema{ - "maxAttempts": { - SchemaProps: spec.SchemaProps{ - Type: []string{"integer"}, - Minimum: ¬Negative, - }, - }, - "retryOnSame": { - SchemaProps: spec.SchemaProps{ - Type: []string{"integer"}, - Minimum: ¬Negative, - }, - }, + "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: ¬Negative, - }, - }, - }, }, } 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: ¬Empty, - }, - }, + "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: ¬Negative, - }, - }, + "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: ¬Negative, - }, - }, + "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: ¬Negative, - }, - }, + "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: ¬Negative, - }, - }, + "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) -}
