You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/12/19 16:20:47 UTC
[GitHub] [apisix-dashboard] juzhiyuan commented on a change in pull request #1057: feat: support global rules for Manager API
juzhiyuan commented on a change in pull request #1057:
URL: https://github.com/apache/apisix-dashboard/pull/1057#discussion_r546254524
##########
File path: api/internal/handler/global_rule/global_rule_test.go
##########
@@ -0,0 +1,338 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package global_rule
+
+import (
+ "context"
+ "fmt"
+ "net/http"
+ "net/http/httptest"
+ "testing"
+
+ "github.com/gin-gonic/gin"
+ "github.com/shiningrush/droplet"
+ "github.com/shiningrush/droplet/data"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/mock"
+
+ "github.com/apisix/manager-api/internal/core/entity"
+ "github.com/apisix/manager-api/internal/core/store"
+)
+
+func performRequest(r http.Handler, method, path string) *httptest.ResponseRecorder {
+ req := httptest.NewRequest(method, path, nil)
+ w := httptest.NewRecorder()
+ r.ServeHTTP(w, req)
+ return w
+}
+
+func TestHandler_ApplyRoute(t *testing.T) {
+ mStore := &store.MockInterface{}
+ giveRet := `{
+ "id": "test",
+ "plugins": {
+ "jwt-auth": {}
+ }
+ }`
+ mStore.On("Get", mock.Anything).Run(func(args mock.Arguments) {
+ assert.Equal(t, "test", args.Get(0))
+ }).Return(giveRet, nil)
+
+ h := Handler{globalRuleStore: mStore}
+ r := gin.New()
+ h.ApplyRoute(r)
+
+ w := performRequest(r, "GET", "/apisix/admin/global_rules/test")
+ assert.Equal(t, 200, w.Code)
Review comment:
I remember we will response 200 with code if an error occurred, it seems that the Get method relies on etcd go client?
##########
File path: api/test/e2e/global_rule_test.go
##########
@@ -0,0 +1,172 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package e2e
+
+import (
+ "net/http"
+ "testing"
+ "time"
+)
+
+func TestGlobalRule(t *testing.T) {
+ tests := []HttpTestCase{
+ {
+ caseDesc: "make sure the route doesn't exist",
+ Object: APISIXExpect(t),
+ Method: http.MethodGet,
+ Path: "/hello",
+ ExpectStatus: http.StatusNotFound,
+ ExpectBody: `{"error_msg":"404 Route Not Found"}`,
+ },
+ {
+ caseDesc: "create route",
+ Object: ManagerApiExpect(t),
+ Method: http.MethodPut,
+ Path: "/apisix/admin/routes/r1",
+ Body: `{
+ "uri": "/hello",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": [{
+ "host": "172.16.238.20",
+ "port": 1981,
+ "weight": 1
+ }]
+ }
+ }`,
+ Headers: map[string]string{"Authorization": token},
+ ExpectStatus: http.StatusOK,
+ },
+ {
+ caseDesc: "create global rule",
+ Object: ManagerApiExpect(t),
+ Path: "/apisix/admin/global_rules/1",
+ Method: http.MethodPut,
+ Body: `{
+ "id": "1",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 2,
+ "rejected_code": 503,
+ "key": "remote_addr"
+ }
+ }
+ }`,
+ Headers: map[string]string{"Authorization": token},
+ ExpectStatus: http.StatusOK,
+ },
+ {
+ caseDesc: "verify route that should not be limited",
+ Object: APISIXExpect(t),
+ Method: http.MethodGet,
+ Path: "/hello",
+ ExpectStatus: http.StatusOK,
+ ExpectBody: "hello world",
+ Sleep: sleepTime,
+ },
+ {
+ caseDesc: "verify route that should not be limited 2",
+ Object: APISIXExpect(t),
+ Method: http.MethodGet,
+ Path: "/hello",
+ ExpectStatus: http.StatusOK,
+ ExpectBody: "hello world",
Review comment:
`hello world` is from upstream?
##########
File path: api/internal/core/store/storehub.go
##########
@@ -145,6 +153,15 @@ func InitStores() error {
return err
}
+ err = InitStore(HubKeyGlobalRule, GenericStoreOption{
Review comment:
##########
File path: api/internal/handler/global_rule/global_rule_test.go
##########
@@ -0,0 +1,338 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package global_rule
+
+import (
+ "context"
+ "fmt"
+ "net/http"
+ "net/http/httptest"
+ "testing"
+
+ "github.com/gin-gonic/gin"
+ "github.com/shiningrush/droplet"
+ "github.com/shiningrush/droplet/data"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/mock"
+
+ "github.com/apisix/manager-api/internal/core/entity"
+ "github.com/apisix/manager-api/internal/core/store"
+)
+
+func performRequest(r http.Handler, method, path string) *httptest.ResponseRecorder {
+ req := httptest.NewRequest(method, path, nil)
+ w := httptest.NewRecorder()
+ r.ServeHTTP(w, req)
+ return w
+}
+
+func TestHandler_ApplyRoute(t *testing.T) {
+ mStore := &store.MockInterface{}
+ giveRet := `{
+ "id": "test",
+ "plugins": {
+ "jwt-auth": {}
+ }
+ }`
+ mStore.On("Get", mock.Anything).Run(func(args mock.Arguments) {
+ assert.Equal(t, "test", args.Get(0))
+ }).Return(giveRet, nil)
+
+ h := Handler{globalRuleStore: mStore}
+ r := gin.New()
+ h.ApplyRoute(r)
+
+ w := performRequest(r, "GET", "/apisix/admin/global_rules/test")
+ assert.Equal(t, 200, w.Code)
+}
+
+func TestHandler_Get(t *testing.T) {
+ tests := []struct {
+ caseDesc string
+ giveInput *GetInput
+ giveRet interface{}
+ giveErr error
+ wantErr error
+ wantGetKey string
+ wantRet interface{}
+ }{
+ {
+ caseDesc: "normal",
+ giveInput: &GetInput{ID: "test"},
+ wantGetKey: "test",
+ giveRet: `{
+ "id": "test",
+ "plugins": {
+ "jwt-auth": {}
+ }
+ }`,
+ wantRet: `{
+ "id": "test",
+ "plugins": {
+ "jwt-auth": {}
+ }
+ }`,
+ },
+ {
+ caseDesc: "store get failed",
+ giveInput: &GetInput{ID: "non-existent-key"},
+ wantGetKey: "non-existent-key",
+ giveErr: fmt.Errorf("data not found"),
+ wantErr: fmt.Errorf("data not found"),
+ wantRet: &data.SpecCodeResponse{
+ StatusCode: http.StatusNotFound,
+ },
+ },
+ }
+
+ for _, tc := range tests {
+ t.Run(tc.caseDesc, func(t *testing.T) {
+ getCalled := true
+ mStore := &store.MockInterface{}
+ mStore.On("Get", mock.Anything).Run(func(args mock.Arguments) {
+ getCalled = true
+ assert.Equal(t, tc.wantGetKey, args.Get(0))
+ }).Return(tc.giveRet, tc.giveErr)
+
+ h := Handler{globalRuleStore: mStore}
+ ctx := droplet.NewContext()
+ ctx.SetInput(tc.giveInput)
+ ret, err := h.Get(ctx)
+ assert.True(t, getCalled)
+ assert.Equal(t, tc.wantRet, ret)
+ assert.Equal(t, tc.wantErr, err)
+ })
+ }
+}
+
+func TestHandler_List(t *testing.T) {
+ tests := []struct {
+ caseDesc string
+ giveInput *ListInput
+ giveData []*entity.GlobalPlugins
+ giveErr error
+ wantErr error
+ wantInput store.ListInput
+ wantRet interface{}
+ }{
+ {
+ caseDesc: "list all condition",
+ giveInput: &ListInput{
+ Pagination: store.Pagination{
+ PageSize: 10,
+ PageNumber: 1,
+ },
+ },
+ wantInput: store.ListInput{
+ PageSize: 10,
+ PageNumber: 1,
+ },
+ giveData: []*entity.GlobalPlugins{
+ {ID: "global-rules-1"},
+ {ID: "global-rules-2"},
+ {ID: "global-rules-3"},
+ },
+ wantRet: &store.ListOutput{
+ Rows: []interface{}{
+ &entity.GlobalPlugins{ID: "global-rules-1"},
+ &entity.GlobalPlugins{ID: "global-rules-2"},
+ &entity.GlobalPlugins{ID: "global-rules-3"},
+ },
+ TotalSize: 3,
+ },
+ },
+ {
+ caseDesc: "store list failed",
+ giveInput: &ListInput{
+ Pagination: store.Pagination{
+ PageSize: 10,
+ PageNumber: 10,
+ },
+ },
+ wantInput: store.ListInput{
+ PageSize: 10,
+ PageNumber: 10,
+ },
+ giveData: []*entity.GlobalPlugins{},
+ giveErr: fmt.Errorf("list failed"),
Review comment:
Why not return empty array?
##########
File path: api/internal/handler/global_rule/global_rule_test.go
##########
@@ -0,0 +1,338 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package global_rule
+
+import (
+ "context"
+ "fmt"
+ "net/http"
+ "net/http/httptest"
+ "testing"
+
+ "github.com/gin-gonic/gin"
+ "github.com/shiningrush/droplet"
+ "github.com/shiningrush/droplet/data"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/mock"
+
+ "github.com/apisix/manager-api/internal/core/entity"
+ "github.com/apisix/manager-api/internal/core/store"
+)
+
+func performRequest(r http.Handler, method, path string) *httptest.ResponseRecorder {
+ req := httptest.NewRequest(method, path, nil)
+ w := httptest.NewRecorder()
+ r.ServeHTTP(w, req)
+ return w
+}
+
+func TestHandler_ApplyRoute(t *testing.T) {
+ mStore := &store.MockInterface{}
+ giveRet := `{
+ "id": "test",
+ "plugins": {
+ "jwt-auth": {}
+ }
+ }`
+ mStore.On("Get", mock.Anything).Run(func(args mock.Arguments) {
+ assert.Equal(t, "test", args.Get(0))
+ }).Return(giveRet, nil)
+
+ h := Handler{globalRuleStore: mStore}
+ r := gin.New()
+ h.ApplyRoute(r)
+
+ w := performRequest(r, "GET", "/apisix/admin/global_rules/test")
+ assert.Equal(t, 200, w.Code)
+}
+
+func TestHandler_Get(t *testing.T) {
+ tests := []struct {
+ caseDesc string
+ giveInput *GetInput
+ giveRet interface{}
+ giveErr error
+ wantErr error
+ wantGetKey string
+ wantRet interface{}
+ }{
+ {
+ caseDesc: "normal",
+ giveInput: &GetInput{ID: "test"},
+ wantGetKey: "test",
+ giveRet: `{
+ "id": "test",
+ "plugins": {
+ "jwt-auth": {}
+ }
+ }`,
+ wantRet: `{
+ "id": "test",
+ "plugins": {
+ "jwt-auth": {}
+ }
+ }`,
+ },
+ {
+ caseDesc: "store get failed",
+ giveInput: &GetInput{ID: "non-existent-key"},
+ wantGetKey: "non-existent-key",
+ giveErr: fmt.Errorf("data not found"),
+ wantErr: fmt.Errorf("data not found"),
+ wantRet: &data.SpecCodeResponse{
+ StatusCode: http.StatusNotFound,
+ },
+ },
+ }
+
+ for _, tc := range tests {
+ t.Run(tc.caseDesc, func(t *testing.T) {
+ getCalled := true
+ mStore := &store.MockInterface{}
+ mStore.On("Get", mock.Anything).Run(func(args mock.Arguments) {
+ getCalled = true
+ assert.Equal(t, tc.wantGetKey, args.Get(0))
+ }).Return(tc.giveRet, tc.giveErr)
+
+ h := Handler{globalRuleStore: mStore}
+ ctx := droplet.NewContext()
+ ctx.SetInput(tc.giveInput)
+ ret, err := h.Get(ctx)
+ assert.True(t, getCalled)
+ assert.Equal(t, tc.wantRet, ret)
+ assert.Equal(t, tc.wantErr, err)
+ })
+ }
+}
+
+func TestHandler_List(t *testing.T) {
+ tests := []struct {
+ caseDesc string
+ giveInput *ListInput
+ giveData []*entity.GlobalPlugins
+ giveErr error
+ wantErr error
+ wantInput store.ListInput
+ wantRet interface{}
+ }{
+ {
+ caseDesc: "list all condition",
+ giveInput: &ListInput{
+ Pagination: store.Pagination{
+ PageSize: 10,
+ PageNumber: 1,
+ },
+ },
+ wantInput: store.ListInput{
+ PageSize: 10,
+ PageNumber: 1,
+ },
+ giveData: []*entity.GlobalPlugins{
+ {ID: "global-rules-1"},
+ {ID: "global-rules-2"},
+ {ID: "global-rules-3"},
+ },
+ wantRet: &store.ListOutput{
+ Rows: []interface{}{
+ &entity.GlobalPlugins{ID: "global-rules-1"},
+ &entity.GlobalPlugins{ID: "global-rules-2"},
+ &entity.GlobalPlugins{ID: "global-rules-3"},
+ },
+ TotalSize: 3,
+ },
+ },
+ {
+ caseDesc: "store list failed",
+ giveInput: &ListInput{
+ Pagination: store.Pagination{
+ PageSize: 10,
+ PageNumber: 10,
+ },
+ },
+ wantInput: store.ListInput{
+ PageSize: 10,
+ PageNumber: 10,
+ },
+ giveData: []*entity.GlobalPlugins{},
+ giveErr: fmt.Errorf("list failed"),
+ wantErr: fmt.Errorf("list failed"),
+ },
+ }
+
+ for _, tc := range tests {
+ t.Run(tc.caseDesc, func(t *testing.T) {
+ getCalled := true
+ mStore := &store.MockInterface{}
+ mStore.On("List", mock.Anything).Run(func(args mock.Arguments) {
+ getCalled = true
+ input := args.Get(0).(store.ListInput)
+ assert.Equal(t, tc.wantInput.PageSize, input.PageSize)
+ assert.Equal(t, tc.wantInput.PageNumber, input.PageNumber)
+ }).Return(func(input store.ListInput) *store.ListOutput {
+ var returnData []interface{}
+ for _, c := range tc.giveData {
+ if input.Predicate == nil || input.Predicate(c) {
+ returnData = append(returnData, c)
+ }
+ }
+ return &store.ListOutput{
+ Rows: returnData,
+ TotalSize: len(returnData),
+ }
+ }, tc.giveErr)
+
+ h := Handler{globalRuleStore: mStore}
+ ctx := droplet.NewContext()
+ ctx.SetInput(tc.giveInput)
+ ret, err := h.List(ctx)
+ assert.True(t, getCalled)
+ assert.Equal(t, tc.wantRet, ret)
+ assert.Equal(t, tc.wantErr, err)
+ })
+ }
+}
+
+func TestHandler_Set(t *testing.T) {
+ tests := []struct {
+ caseDesc string
+ giveInput *entity.GlobalPlugins
+ giveCtx context.Context
+ giveErr error
+ wantErr error
+ wantInput *entity.GlobalPlugins
+ wantRet interface{}
+ wantCalled bool
+ }{
+ {
+ caseDesc: "normal",
+ giveInput: &entity.GlobalPlugins{
+ ID: "name",
+ Plugins: map[string]interface{}{
+ "jwt-auth": map[string]interface{}{},
+ },
+ },
+ giveCtx: context.WithValue(context.Background(), "test", "value"),
+ wantInput: &entity.GlobalPlugins{
+ ID: "name",
+ Plugins: map[string]interface{}{
+ "jwt-auth": map[string]interface{}{},
+ },
+ },
+ wantRet: nil,
+ wantCalled: true,
+ },
+ {
+ caseDesc: "store create failed",
+ giveInput: &entity.GlobalPlugins{
+ ID: "name",
+ Plugins: nil,
+ },
+ giveErr: fmt.Errorf("create failed"),
Review comment:
What error data will response to client?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org