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/02 08:01:15 UTC

[GitHub] [apisix-dashboard] nic-chen opened a new pull request #952: fix: hack to fix the checking of `remote_addrs`

nic-chen opened a new pull request #952:
URL: https://github.com/apache/apisix-dashboard/pull/952


   Please answer these questions before submitting a pull request
   
   - Why submit this pull request?
   - [x] Bugfix
   - [ ] New feature provided
   - [ ] Improve performance
   
   - Related issues
   
   related #940 
   
   we should not sync the json schema outside APISIX v2.1, so we need to hack to fix it.
   
   
   
   
   


----------------------------------------------------------------
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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #952: fix: hack to fix the checking of `remote_addrs`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #952:
URL: https://github.com/apache/apisix-dashboard/pull/952#discussion_r534049385



##########
File path: api/internal/core/store/validate_test.go
##########
@@ -247,5 +247,134 @@ func TestAPISIXJsonSchemaValidator_checkUpstream(t *testing.T) {
 	err = validator.Validate(route5)
 	assert.NotNil(t, err)
 	assert.EqualError(t, err, "schema validate failed: (root): Does not match pattern '^((uri|server_name|server_addr|request_uri|remote_port|remote_addr|query_string|host|hostname)|arg_[0-9a-zA-z_-]+)$'")
+}
+
+func TestAPISIXJsonSchemaValidator_Route_checkRemoteAddr(t *testing.T) {
+	tests := []struct {
+		caseDesc        string
+		giveContent     string
+		wantNewErr      error
+		wantValidateErr error
+	}{
+		{
+			caseDesc: "correct remote_addr",
+			giveContent: `{
+				"id": "1",
+				"uri": "/*",
+				"upstream": {
+					"nodes": [{
+						"host": "127.0.0.1",
+						"port": 8080,
+						"weight": 1
+					}],
+					"type": "roundrobin"
+				},
+				"remote_addr": "127.0.0.1"
+			}`,
+		},
+		{
+			caseDesc: "correct remote_addr (CIDR)",
+			giveContent: `{
+				"id": "1",
+				"uri": "/*",
+				"upstream": {
+					"nodes": [{
+						"host": "127.0.0.1",
+						"port": 8080,
+						"weight": 1
+					}],
+					"type": "roundrobin"
+				},
+				"remote_addr": "192.168.1.0/24"
+			}`,
+		},
+		{
+			caseDesc: "invalid remote_addr",
+			giveContent: `{
+				"id": "1",
+				"uri": "/*",
+				"upstream": {
+					"nodes": [{
+						"host": "127.0.0.1",
+						"port": 8080,
+						"weight": 1
+					}],
+					"type": "roundrobin"
+				},
+				"remote_addr": "127.0.0."
+			}`,
+			wantValidateErr: fmt.Errorf("schema validate failed: remote_addr: Must validate at least one schema (anyOf)\nremote_addr: Does not match pattern '^[0-9]{1,3}.[0-9]{1,3}.[0-9]{1,3}.[0-9]{1,3}$'"),
+		},
+		{
+			caseDesc: "correct remote_addrs",
+			giveContent: `{
+				"id": "1",
+				"uri": "/*",
+				"upstream": {
+					"nodes": [{
+						"host": "127.0.0.1",
+						"port": 8080,
+						"weight": 1
+					}],
+					"type": "roundrobin"
+				},
+				"remote_addrs": ["127.0.0.1", "192.0.0.0/8", "::1", "fe80::1/64"]
+			}`,
+		},

Review comment:
       yes, it is. if it cause err, test will fail.




----------------------------------------------------------------
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

[GitHub] [apisix-dashboard] nic-chen merged pull request #952: fix: hack to fix the checking of `remote_addrs`

Posted by GitBox <gi...@apache.org>.
nic-chen merged pull request #952:
URL: https://github.com/apache/apisix-dashboard/pull/952


   


----------------------------------------------------------------
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

[GitHub] [apisix-dashboard] codecov-io commented on pull request #952: fix: hack to fix the checking of `remote_addrs`

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #952:
URL: https://github.com/apache/apisix-dashboard/pull/952#issuecomment-737066048


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/952?src=pr&el=h1) Report
   > Merging [#952](https://codecov.io/gh/apache/apisix-dashboard/pull/952?src=pr&el=desc) (738bf9d) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/95f26122fee30d2fcbd18d98433b106c09e9bdca?el=desc) (95f2612) will **increase** coverage by `0.31%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/952/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/952?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #952      +/-   ##
   ==========================================
   + Coverage   43.10%   43.41%   +0.31%     
   ==========================================
     Files          18       18              
     Lines        1290     1299       +9     
   ==========================================
   + Hits          556      564       +8     
   - Misses        642      643       +1     
     Partials       92       92              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/952?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [api/internal/core/store/validate.go](https://codecov.io/gh/apache/apisix-dashboard/pull/952/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvdmFsaWRhdGUuZ28=) | `58.04% <100.00%> (+2.96%)` | :arrow_up: |
   | [api/internal/core/store/store.go](https://codecov.io/gh/apache/apisix-dashboard/pull/952/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmUuZ28=) | `78.57% <0.00%> (-0.65%)` | :arrow_down: |
   | [api/internal/utils/utils.go](https://codecov.io/gh/apache/apisix-dashboard/pull/952/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL3V0aWxzLmdv) | `51.42% <0.00%> (-0.19%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/952?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/952?src=pr&el=footer). Last update [95f2612...738bf9d](https://codecov.io/gh/apache/apisix-dashboard/pull/952?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [apisix-dashboard] gxthrj commented on a change in pull request #952: fix: hack to fix the checking of `remote_addrs`

Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #952:
URL: https://github.com/apache/apisix-dashboard/pull/952#discussion_r534047655



##########
File path: api/internal/core/store/validate_test.go
##########
@@ -247,5 +247,134 @@ func TestAPISIXJsonSchemaValidator_checkUpstream(t *testing.T) {
 	err = validator.Validate(route5)
 	assert.NotNil(t, err)
 	assert.EqualError(t, err, "schema validate failed: (root): Does not match pattern '^((uri|server_name|server_addr|request_uri|remote_port|remote_addr|query_string|host|hostname)|arg_[0-9a-zA-z_-]+)$'")
+}
+
+func TestAPISIXJsonSchemaValidator_Route_checkRemoteAddr(t *testing.T) {
+	tests := []struct {
+		caseDesc        string
+		giveContent     string
+		wantNewErr      error
+		wantValidateErr error
+	}{
+		{
+			caseDesc: "correct remote_addr",
+			giveContent: `{
+				"id": "1",
+				"uri": "/*",
+				"upstream": {
+					"nodes": [{
+						"host": "127.0.0.1",
+						"port": 8080,
+						"weight": 1
+					}],
+					"type": "roundrobin"
+				},
+				"remote_addr": "127.0.0.1"
+			}`,
+		},
+		{
+			caseDesc: "correct remote_addr (CIDR)",
+			giveContent: `{
+				"id": "1",
+				"uri": "/*",
+				"upstream": {
+					"nodes": [{
+						"host": "127.0.0.1",
+						"port": 8080,
+						"weight": 1
+					}],
+					"type": "roundrobin"
+				},
+				"remote_addr": "192.168.1.0/24"
+			}`,
+		},
+		{
+			caseDesc: "invalid remote_addr",
+			giveContent: `{
+				"id": "1",
+				"uri": "/*",
+				"upstream": {
+					"nodes": [{
+						"host": "127.0.0.1",
+						"port": 8080,
+						"weight": 1
+					}],
+					"type": "roundrobin"
+				},
+				"remote_addr": "127.0.0."
+			}`,
+			wantValidateErr: fmt.Errorf("schema validate failed: remote_addr: Must validate at least one schema (anyOf)\nremote_addr: Does not match pattern '^[0-9]{1,3}.[0-9]{1,3}.[0-9]{1,3}.[0-9]{1,3}$'"),
+		},
+		{
+			caseDesc: "correct remote_addrs",
+			giveContent: `{
+				"id": "1",
+				"uri": "/*",
+				"upstream": {
+					"nodes": [{
+						"host": "127.0.0.1",
+						"port": 8080,
+						"weight": 1
+					}],
+					"type": "roundrobin"
+				},
+				"remote_addrs": ["127.0.0.1", "192.0.0.0/8", "::1", "fe80::1/64"]
+			}`,
+		},

Review comment:
       There is no assert, is it default ok?




----------------------------------------------------------------
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

[GitHub] [apisix-dashboard] membphis commented on pull request #952: fix: hack to fix the checking of `remote_addrs`

Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #952:
URL: https://github.com/apache/apisix-dashboard/pull/952#issuecomment-737113630


   @gxthrj @moonming @ShiningRush please review this PR if you have time ^_^


----------------------------------------------------------------
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

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #952: fix: hack to fix the checking of `remote_addrs`

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #952:
URL: https://github.com/apache/apisix-dashboard/pull/952#discussion_r533968084



##########
File path: api/internal/core/store/validate_test.go
##########
@@ -247,5 +247,132 @@ func TestAPISIXJsonSchemaValidator_checkUpstream(t *testing.T) {
 	err = validator.Validate(route5)
 	assert.NotNil(t, err)
 	assert.EqualError(t, err, "schema validate failed: (root): Does not match pattern '^((uri|server_name|server_addr|request_uri|remote_port|remote_addr|query_string|host|hostname)|arg_[0-9a-zA-z_-]+)$'")
+}
 
+func TestAPISIXJsonSchemaValidator_Route_checkRemoteAddr(t *testing.T) {

Review comment:
       add test case: `"remote_addr": ""`




----------------------------------------------------------------
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

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #952: fix: hack to fix the checking of `remote_addrs`

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #952:
URL: https://github.com/apache/apisix-dashboard/pull/952#discussion_r533967586



##########
File path: api/internal/core/store/validate.go
##########
@@ -201,6 +210,9 @@ func checkConf(reqBody interface{}) error {
 		if err := checkUpstream(route.Upstream); err != nil {
 			return err
 		}
+		if err := checkRemoteAddr(route.RemoteAddrs); err != nil {

Review comment:
       need a todo comment: this is a temporary method, we'll drop it later

##########
File path: api/internal/core/store/validate_test.go
##########
@@ -247,5 +247,132 @@ func TestAPISIXJsonSchemaValidator_checkUpstream(t *testing.T) {
 	err = validator.Validate(route5)
 	assert.NotNil(t, err)
 	assert.EqualError(t, err, "schema validate failed: (root): Does not match pattern '^((uri|server_name|server_addr|request_uri|remote_port|remote_addr|query_string|host|hostname)|arg_[0-9a-zA-z_-]+)$'")
+}
 
+func TestAPISIXJsonSchemaValidator_Route_checkRemoteAddr(t *testing.T) {

Review comment:
       test case: `"remote_addr": ""`




----------------------------------------------------------------
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