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 2022/04/11 02:55:27 UTC
[GitHub] [apisix-dashboard] jwrookie opened a new pull request, #2421: fix: cerate upstream error when pass host is node and nodes without port
jwrookie opened a new pull request, #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421
Signed-off-by: Wei Jiang <ma...@gmail.com>
Please answer these questions before submitting a pull request, **or your PR will get closed**.
**Why submit this pull request?**
- [x] Bugfix
- [ ] New feature provided
- [ ] Improve performance
- [ ] Backport patches
**What changes will this PR take into?**
Create upstream should succeed when **pass_host** is `node` and **nodes** without `port`
**Related issues**
fix/resolve #2409
**Checklist:**
- [x] Did you explain what problem does this PR solve? Or what new features have been added?
- [x] Have you added corresponding test cases?
- [ ] Have you modified the corresponding document?
- [x] Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] membphis commented on pull request #2421: fix: cerate upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
membphis commented on PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#issuecomment-1095761274
typo in your title
<img width="1145" alt="image" src="https://user-images.githubusercontent.com/6814606/162858395-168dfb2d-8bf3-41b1-93f5-abb82007085a.png">
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] jwrookie commented on a diff in pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
jwrookie commented on code in PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#discussion_r849268728
##########
api/test/e2enew/upstream/upstream_test.go:
##########
@@ -88,6 +88,63 @@ var _ = ginkgo.Describe("Upstream", func() {
ExpectStatus: http.StatusOK,
})
})
+ ginkgo.It("create upstream3 success when pass host is 'node' and nodes without port", func() {
+ ginkgo.By("create upstream3", func() {
+ createUpstreamBody := make(map[string]interface{})
+ createUpstreamBody["name"] = "upstream3"
+ createUpstreamBody["nodes"] = map[string]float64{base.UpstreamIp: 100}
Review Comment:
Value(100) is weight, not port
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] codecov-commenter commented on pull request #2421: fix: cerate upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#issuecomment-1094491496
# [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/2421?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2421](https://codecov.io/gh/apache/apisix-dashboard/pull/2421?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ab496ff) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/6f4200ce56316fd5f9e20c175064d9ff0a46e973?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6f4200c) will **decrease** coverage by `21.11%`.
> The diff coverage is `58.33%`.
> :exclamation: Current head ab496ff differs from pull request most recent head 88ad21d. Consider uploading reports for the commit 88ad21d to get more accurate results
```diff
@@ Coverage Diff @@
## master #2421 +/- ##
===========================================
- Coverage 70.04% 48.93% -21.12%
===========================================
Files 189 42 -147
Lines 7441 3276 -4165
Branches 828 0 -828
===========================================
- Hits 5212 1603 -3609
+ Misses 1923 1469 -454
+ Partials 306 204 -102
```
| Flag | Coverage Δ | |
|---|---|---|
| backend-e2e-test | `?` | |
| backend-e2e-test-ginkgo | `?` | |
| backend-unit-test | `48.93% <58.33%> (+0.30%)` | :arrow_up: |
| frontend-e2e-test | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/2421?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [api/internal/core/store/validate.go](https://codecov.io/gh/apache/apisix-dashboard/pull/2421/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvdmFsaWRhdGUuZ28=) | `57.21% <0.00%> (-13.50%)` | :arrow_down: |
| [api/internal/core/entity/format.go](https://codecov.io/gh/apache/apisix-dashboard/pull/2421/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXBpL2ludGVybmFsL2NvcmUvZW50aXR5L2Zvcm1hdC5nbw==) | `87.27% <100.00%> (+19.96%)` | :arrow_up: |
| [api/main.go](https://codecov.io/gh/apache/apisix-dashboard/pull/2421/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXBpL21haW4uZ28=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [api/internal/utils/version.go](https://codecov.io/gh/apache/apisix-dashboard/pull/2421/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXBpL2ludGVybmFsL3V0aWxzL3ZlcnNpb24uZ28=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [api/internal/filter/request\_id.go](https://codecov.io/gh/apache/apisix-dashboard/pull/2421/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXBpL2ludGVybmFsL2ZpbHRlci9yZXF1ZXN0X2lkLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [api/internal/core/entity/entity.go](https://codecov.io/gh/apache/apisix-dashboard/pull/2421/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXBpL2ludGVybmFsL2NvcmUvZW50aXR5L2VudGl0eS5nbw==) | `0.00% <0.00%> (-90.91%)` | :arrow_down: |
| [api/internal/core/store/storehub.go](https://codecov.io/gh/apache/apisix-dashboard/pull/2421/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmVodWIuZ28=) | `0.00% <0.00%> (-72.92%)` | :arrow_down: |
| [api/internal/handler/proto/proto.go](https://codecov.io/gh/apache/apisix-dashboard/pull/2421/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvcHJvdG8vcHJvdG8uZ28=) | `0.00% <0.00%> (-67.26%)` | :arrow_down: |
| [api/internal/utils/closer.go](https://codecov.io/gh/apache/apisix-dashboard/pull/2421/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXBpL2ludGVybmFsL3V0aWxzL2Nsb3Nlci5nbw==) | `0.00% <0.00%> (-66.67%)` | :arrow_down: |
| [api/internal/filter/schema.go](https://codecov.io/gh/apache/apisix-dashboard/pull/2421/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXBpL2ludGVybmFsL2ZpbHRlci9zY2hlbWEuZ28=) | `0.00% <0.00%> (-56.00%)` | :arrow_down: |
| ... and [175 more](https://codecov.io/gh/apache/apisix-dashboard/pull/2421/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/2421?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/2421?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6f4200c...88ad21d](https://codecov.io/gh/apache/apisix-dashboard/pull/2421?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] nic-chen commented on a diff in pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
nic-chen commented on code in PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#discussion_r849265521
##########
api/test/e2enew/upstream/upstream_test.go:
##########
@@ -88,6 +88,63 @@ var _ = ginkgo.Describe("Upstream", func() {
ExpectStatus: http.StatusOK,
})
})
+ ginkgo.It("create upstream3 success when pass host is 'node' and nodes without port", func() {
+ ginkgo.By("create upstream3", func() {
+ createUpstreamBody := make(map[string]interface{})
+ createUpstreamBody["name"] = "upstream3"
+ createUpstreamBody["nodes"] = map[string]float64{base.UpstreamIp: 100}
Review Comment:
node for the test case still has port?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on a diff in pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
starsz commented on code in PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#discussion_r847862261
##########
api/test/e2enew/upstream/upstream_test.go:
##########
@@ -88,6 +88,24 @@ var _ = ginkgo.Describe("Upstream", func() {
ExpectStatus: http.StatusOK,
})
})
+ ginkgo.It("create upstream3 success when pass host is 'node' and nodes without port", func() {
+ createUpstreamBody := make(map[string]interface{})
+ createUpstreamBody["name"] = "upstream3"
+ createUpstreamBody["nodes"] = map[string]float64{base.UpstreamIp: 100}
+ createUpstreamBody["type"] = "roundrobin"
+ createUpstreamBody["pass_host"] = "node"
+
+ _createUpstreamBody, err := json.Marshal(createUpstreamBody)
+ gomega.Expect(err).To(gomega.BeNil())
+ base.RunTestCase(base.HttpTestCase{
+ Object: base.ManagerApiExpect(),
+ Method: http.MethodPut,
+ Path: "/apisix/admin/upstreams/3",
+ Body: string(_createUpstreamBody),
+ Headers: map[string]string{"Authorization": base.GetToken()},
+ ExpectStatus: http.StatusOK,
+ })
+ })
Review Comment:
Can you add test to check the upstream is work ?
##########
api/internal/core/entity/format.go:
##########
@@ -17,17 +17,23 @@
package entity
import (
- "net"
+ "errors"
"strconv"
+ "strings"
"github.com/apisix/manager-api/internal/log"
)
func mapKV2Node(key string, val float64) (*Node, error) {
- host, port, err := net.SplitHostPort(key)
- if err != nil {
- log.Errorf("split host port fail: %s", err)
- return nil, err
+ hp := strings.Split(key, ":")
+ host := hp[0]
+ // according to APISIX upstream nodes policy, port is optional
+ port := "0"
+
+ if len(hp) > 2 {
+ return nil, errors.New("address typo error")
Review Comment:
```suggestion
return nil, errors.New("invalid upstream node")
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] jwrookie commented on pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
jwrookie commented on PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#issuecomment-1095764412
> typo in your title
done, thanks for your checking!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on a diff in pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#discussion_r874379186
##########
api/internal/core/entity/format.go:
##########
@@ -17,17 +17,23 @@
package entity
import (
- "net"
+ "errors"
"strconv"
+ "strings"
"github.com/apisix/manager-api/internal/log"
)
func mapKV2Node(key string, val float64) (*Node, error) {
- host, port, err := net.SplitHostPort(key)
- if err != nil {
- log.Errorf("split host port fail: %s", err)
- return nil, err
+ hp := strings.Split(key, ":")
+ host := hp[0]
+ // according to APISIX upstream nodes policy, port is optional
+ port := "0"
Review Comment:
When there is no port in host, port `0` will be used by default, which I think is wrong. For HTTP it should be `80` and for HTTPS it should be `443`, so I think it should be set to 80 by default.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] nic-chen commented on a diff in pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
nic-chen commented on code in PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#discussion_r847873820
##########
api/test/e2enew/upstream/upstream_test.go:
##########
@@ -88,6 +88,24 @@ var _ = ginkgo.Describe("Upstream", func() {
ExpectStatus: http.StatusOK,
})
})
+ ginkgo.It("create upstream3 success when pass host is 'node' and nodes without port", func() {
+ createUpstreamBody := make(map[string]interface{})
+ createUpstreamBody["name"] = "upstream3"
+ createUpstreamBody["nodes"] = map[string]float64{base.UpstreamIp: 100}
+ createUpstreamBody["type"] = "roundrobin"
+ createUpstreamBody["pass_host"] = "node"
+
+ _createUpstreamBody, err := json.Marshal(createUpstreamBody)
+ gomega.Expect(err).To(gomega.BeNil())
+ base.RunTestCase(base.HttpTestCase{
+ Object: base.ManagerApiExpect(),
+ Method: http.MethodPut,
+ Path: "/apisix/admin/upstreams/3",
+ Body: string(_createUpstreamBody),
+ Headers: map[string]string{"Authorization": base.GetToken()},
+ ExpectStatus: http.StatusOK,
+ })
+ })
Review Comment:
need a test case for DP
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] jwrookie commented on a diff in pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
jwrookie commented on code in PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#discussion_r847894195
##########
api/test/e2enew/upstream/upstream_test.go:
##########
@@ -88,6 +88,24 @@ var _ = ginkgo.Describe("Upstream", func() {
ExpectStatus: http.StatusOK,
})
})
+ ginkgo.It("create upstream3 success when pass host is 'node' and nodes without port", func() {
+ createUpstreamBody := make(map[string]interface{})
+ createUpstreamBody["name"] = "upstream3"
+ createUpstreamBody["nodes"] = map[string]float64{base.UpstreamIp: 100}
+ createUpstreamBody["type"] = "roundrobin"
+ createUpstreamBody["pass_host"] = "node"
+
+ _createUpstreamBody, err := json.Marshal(createUpstreamBody)
+ gomega.Expect(err).To(gomega.BeNil())
+ base.RunTestCase(base.HttpTestCase{
+ Object: base.ManagerApiExpect(),
+ Method: http.MethodPut,
+ Path: "/apisix/admin/upstreams/3",
+ Body: string(_createUpstreamBody),
+ Headers: map[string]string{"Authorization": base.GetToken()},
+ ExpectStatus: http.StatusOK,
+ })
+ })
Review Comment:
Good idea. it is done now
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on a diff in pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#discussion_r874345460
##########
api/internal/core/entity/format.go:
##########
@@ -17,17 +17,23 @@
package entity
import (
- "net"
+ "errors"
"strconv"
+ "strings"
"github.com/apisix/manager-api/internal/log"
)
func mapKV2Node(key string, val float64) (*Node, error) {
- host, port, err := net.SplitHostPort(key)
- if err != nil {
- log.Errorf("split host port fail: %s", err)
- return nil, err
+ hp := strings.Split(key, ":")
+ host := hp[0]
+ // according to APISIX upstream nodes policy, port is optional
+ port := "0"
Review Comment:
Is it appropriate to use port 0 by default? Should port 80 be used by default? 🤔
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 merged pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
bzp2010 merged PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] nic-chen commented on a diff in pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
nic-chen commented on code in PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#discussion_r849279164
##########
api/test/e2enew/upstream/upstream_test.go:
##########
@@ -88,6 +88,63 @@ var _ = ginkgo.Describe("Upstream", func() {
ExpectStatus: http.StatusOK,
})
})
+ ginkgo.It("create upstream3 success when pass host is 'node' and nodes without port", func() {
+ ginkgo.By("create upstream3", func() {
+ createUpstreamBody := make(map[string]interface{})
+ createUpstreamBody["name"] = "upstream3"
+ createUpstreamBody["nodes"] = map[string]float64{base.UpstreamIp: 100}
Review Comment:
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] jwrookie commented on pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
jwrookie commented on PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#issuecomment-1118341942
@bzp2010 have a check, thanks!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] jwrookie commented on a diff in pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
jwrookie commented on code in PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#discussion_r874349477
##########
api/internal/core/entity/format.go:
##########
@@ -17,17 +17,23 @@
package entity
import (
- "net"
+ "errors"
"strconv"
+ "strings"
"github.com/apisix/manager-api/internal/log"
)
func mapKV2Node(key string, val float64) (*Node, error) {
- host, port, err := net.SplitHostPort(key)
- if err != nil {
- log.Errorf("split host port fail: %s", err)
- return nil, err
+ hp := strings.Split(key, ":")
+ host := hp[0]
+ // according to APISIX upstream nodes policy, port is optional
+ port := "0"
Review Comment:
https://github.com/apache/apisix-dashboard/blob/92f46d9d2fa8438659131eedecf9cab4c87b0f9d/api/internal/core/entity/entity.go#L99-L105
We cannot give default values to port, the value of 0 is assigned because the zero value is ignored during `json.Marshal`
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on a diff in pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#discussion_r874454810
##########
api/internal/core/entity/format.go:
##########
@@ -17,17 +17,23 @@
package entity
import (
- "net"
+ "errors"
"strconv"
+ "strings"
"github.com/apisix/manager-api/internal/log"
)
func mapKV2Node(key string, val float64) (*Node, error) {
- host, port, err := net.SplitHostPort(key)
- if err != nil {
- log.Errorf("split host port fail: %s", err)
- return nil, err
+ hp := strings.Split(key, ":")
+ host := hp[0]
+ // according to APISIX upstream nodes policy, port is optional
+ port := "0"
Review Comment:
So to put it another way, if the port is 0, the JSON will not contain the port field when it is encoded in JSON? The `port` is required.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] jwrookie commented on a diff in pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
jwrookie commented on code in PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#discussion_r874390535
##########
api/internal/core/entity/format.go:
##########
@@ -17,17 +17,23 @@
package entity
import (
- "net"
+ "errors"
"strconv"
+ "strings"
"github.com/apisix/manager-api/internal/log"
)
func mapKV2Node(key string, val float64) (*Node, error) {
- host, port, err := net.SplitHostPort(key)
- if err != nil {
- log.Errorf("split host port fail: %s", err)
- return nil, err
+ hp := strings.Split(key, ":")
+ host := hp[0]
+ // according to APISIX upstream nodes policy, port is optional
+ port := "0"
Review Comment:
Port is not required, when using https and not configured port it will cause problems, if we do not configure the port there is no such problem
If 80 is given, you need to determine the schema type, when it is https and then change the port default back to 443. I think the first one is more convenient, what do you think?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] jwrookie commented on a diff in pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
jwrookie commented on code in PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#discussion_r874847969
##########
api/internal/core/entity/format.go:
##########
@@ -17,17 +17,23 @@
package entity
import (
- "net"
+ "errors"
"strconv"
+ "strings"
"github.com/apisix/manager-api/internal/log"
)
func mapKV2Node(key string, val float64) (*Node, error) {
- host, port, err := net.SplitHostPort(key)
- if err != nil {
- log.Errorf("split host port fail: %s", err)
- return nil, err
+ hp := strings.Split(key, ":")
+ host := hp[0]
+ // according to APISIX upstream nodes policy, port is optional
+ port := "0"
Review Comment:
<img width="765" alt="image" src="https://user-images.githubusercontent.com/41732283/168827306-1e6b5118-8a1b-4475-aa83-908b8b187f64.png">
Nodes has two sets of policies
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] jwrookie commented on pull request #2421: fix: create upstream error when pass host is node and nodes without port
Posted by GitBox <gi...@apache.org>.
jwrookie commented on PR #2421:
URL: https://github.com/apache/apisix-dashboard/pull/2421#issuecomment-1097524981
> test failed @jwrookie
Start the depend error, pls rerun it thanks
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org