Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: create upstream error when pass host is node and nodes without port #2421

Merged
merged 3 commits into from
May 19, 2022
Merged

fix: create upstream error when pass host is node and nodes without port #2421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
88ad21d
fix: cerate upstream error
jwrookie Apr 11, 2022
2058b92
Update api/internal/core/entity/format.go
jwrookie Apr 12, 2022
f73c75c
fix: review
jwrookie Apr 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ github.com/hashicorp/logutils v1.0.0/go.mod h1:QIAnNjmIWmVIIkWDTG1z5v++HQmx9WQRO
github.com/hashicorp/mdns v1.0.0/go.mod h1:tL+uN++7HEJ6SQLQ2/p+z2pH24WQKWjBPkE0mNTz8vQ=
github.com/hashicorp/memberlist v0.1.3/go.mod h1:ajVTdAv/9Im8oMAAj5G31PhhMCZJV2pPBoIllUwCN7I=
github.com/hashicorp/serf v0.8.2/go.mod h1:6hOLApaqBFA1NXqRQAsxw9QxuDEvNxSQRwA/JwenrHc=
github.com/hpcloud/tail v1.0.0 h1:nfCOvKYfkgYP8hkirhJocXT2+zOD8yUNjXaWfTlyFKI=
github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU=
github.com/hudl/fargo v1.3.0/go.mod h1:y3CKSmjA+wD2gak7sUSXTAoopbhU08POFhmITJgmKTg=
github.com/ianlancetaylor/demangle v0.0.0-20181102032728-5e5cf60278f6/go.mod h1:aSSvb/t6k1mPoxDqO4vJh6VOCGPwU4O0C2/Eqndh1Sc=
Expand Down Expand Up @@ -357,7 +358,9 @@ github.com/oklog/oklog v0.3.2/go.mod h1:FCV+B7mhrz4o+ueLpx+KqkyXRGMWOYEvfiXtdGtb
github.com/oklog/run v1.0.0/go.mod h1:dlhp/R75TPv97u0XWUtDeV/lRKWPKSdTuV0TZvrmrQA=
github.com/olekukonko/tablewriter v0.0.0-20170122224234-a0225b3f23b5/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo=
github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
github.com/onsi/ginkgo v1.7.0 h1:WSHQ+IS43OoUrWtD1/bbclrwK8TTH5hzp+umCiuxHgs=
github.com/onsi/ginkgo v1.7.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
github.com/onsi/gomega v1.4.3 h1:RE1xgDvH7imwFD45h+u2SgIfERHlS2yNG4DObb5BSKU=
github.com/onsi/gomega v1.4.3/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY=
github.com/op/go-logging v0.0.0-20160315200505-970db520ece7/go.mod h1:HzydrMdWErDVzsI23lYNej1Htcns9BCg93Dk0bBINWk=
github.com/opentracing-contrib/go-observer v0.0.0-20170622124052-a52f23424492/go.mod h1:Ngi6UdF0k5OKD5t5wlmGhe/EDKPoUM3BXZSSfIuJbis=
Expand Down Expand Up @@ -878,13 +881,15 @@ gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 h1:YR8cESwS4TdDjEe65xsg0ogR
gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/cheggaaa/pb.v1 v1.0.25/go.mod h1:V/YB90LKu/1FcN3WVnfiiE5oMCibMjukxqG/qStrOgw=
gopkg.in/errgo.v2 v2.1.0/go.mod h1:hNsd1EY+bozCKY1Ytp96fpM3vjJbqLJn88ws8XvfDNI=
gopkg.in/fsnotify.v1 v1.4.7 h1:xOHLXZwVvI9hhs+cLKq5+I5onOuwQLhQwiu63xxlHs4=
gopkg.in/fsnotify.v1 v1.4.7/go.mod h1:Tz8NjZHkW78fSQdbUxIjBTcgA1z1m8ZHf0WmKUhAMys=
gopkg.in/gcfg.v1 v1.2.3/go.mod h1:yesOnuUOFQAhST5vPY4nbZsb/huCgGGXlipJsBn0b3o=
gopkg.in/go-playground/assert.v1 v1.2.1/go.mod h1:9RXL0bg/zibRAgZUYszZSwO/z8Y/a8bDuhia5mkpMnE=
gopkg.in/go-playground/validator.v9 v9.29.1/go.mod h1:+c9/zcJMFNgbLvly1L1V+PpxWdVbfP1avr/N00E2vyQ=
gopkg.in/ini.v1 v1.62.0 h1:duBzk771uxoUuOlyRLkHsygud9+5lrlGjdFBb4mSKDU=
gopkg.in/ini.v1 v1.62.0/go.mod h1:pNLf8WUiyNEtQjuu5G5vTm06TEv9tsIgeAvK8hOrP4k=
gopkg.in/resty.v1 v1.12.0/go.mod h1:mDo4pnntr5jdWRML875a/NmxYqAlA73dVijT2AXvQQo=
gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 h1:uRGJdciOHaEIrze2W8Q3AKkepLTh2hOroT7a+7czfdQ=
gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7/go.mod h1:dt/ZhP58zS4L8KSrWDmTeBkI65Dw0HsyUHuEVlX15mw=
gopkg.in/warnings.v0 v0.1.2/go.mod h1:jksf8JmL6Qr/oQM2OXTHunEvvTAsrWBLb6OOjuVWRNI=
gopkg.in/yaml.v2 v2.0.0-20170812160011-eb3733d160e7/go.mod h1:JAlM8MvJe8wmxCU4Bli9HhUf9+ttbYbLASfIpnQbh74=
Expand Down
16 changes: 11 additions & 5 deletions api/internal/core/entity/format.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it appropriate to use port 0 by default? Should port 80 be used by default? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apisix-dashboard/api/internal/core/entity/entity.go

Lines 99 to 105 in 92f46d9

type Node struct {
Host string `json:"host,omitempty"`
Port int `json:"port,omitempty"`
Weight int `json:"weight"`
Metadata interface{} `json:"metadata,omitempty"`
Priority int `json:"priority,omitempty"`
}

We cannot give default values to port, the value of 0 is assigned because the zero value is ignored during json.Marshal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image

image


if len(hp) > 2 {
return nil, errors.New("invalid upstream node")
} else if len(hp) == 2 {
port = hp[1]
}

portInt, err := strconv.Atoi(port)
Expand Down
64 changes: 64 additions & 0 deletions api/internal/core/entity/format_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,17 @@ func TestNodesFormat_no_nodes(t *testing.T) {
assert.Contains(t, jsonStr, `null`)
}

func TestNodesFormat_nodes_without_port(t *testing.T) {
nodes := map[string]float64{"127.0.0.1": 0}
// nodes format
formattedNodes := NodesFormat(nodes)

// json encode for client
res, err := json.Marshal(formattedNodes)
assert.Nil(t, err)
assert.Equal(t, res, []byte(`[{"host":"127.0.0.1","weight":0}]`))
}

func Test_Idle_Timeout_nil_and_zero(t *testing.T) {
ukp0 := UpstreamKeepalivePool{}
// Unmarshal from zero value
Expand Down Expand Up @@ -229,3 +240,56 @@ func TestUpstream_nil_and_zero_retries(t *testing.T) {
assert.Nil(t, err)
assert.Equal(t, string(marshaledNull), `{}`)
}

func TestMapKV2Node(t *testing.T) {
testCases := []struct {
name string
key string
value float64
wantErr bool
errMessage string
wantRes *Node
}{
{
name: "invalid upstream node",
key: "127.0.0.1:0:0",
wantErr: true,
errMessage: "invalid upstream node",
},
{
name: "when address contains port convert should succeed",
key: "127.0.0.1:8080",
value: 100,
wantErr: false,
wantRes: &Node{
Host: "127.0.0.1",
Port: 8080,
Weight: 100,
},
},
{
name: "when address without port convert should succeed",
key: "127.0.0.1",
wantErr: false,
wantRes: &Node{
Host: "127.0.0.1",
Port: 0,
Weight: 0,
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
got, err := mapKV2Node(tc.key, tc.value)
if tc.wantErr {
assert.NotNil(t, err)
assert.Contains(t, err.Error(), tc.errMessage)
return
}

assert.Nil(t, err)
assert.Equal(t, tc.wantRes, got)
})
}
}
7 changes: 5 additions & 2 deletions api/internal/core/store/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,11 @@ func checkUpstream(upstream *entity.UpstreamDef) error {
}

if upstream.PassHost == "node" && upstream.Nodes != nil {
if nodes := entity.NodesFormat(upstream.Nodes); len(nodes.([]*entity.Node)) != 1 {
return fmt.Errorf("only support single node for `node` mode currently")
nodes, ok := entity.NodesFormat(upstream.Nodes).([]*entity.Node)
if !ok {
return fmt.Errorf("upstrams nodes not support value %v when `pass_host` is `node`", nodes)
} else if len(nodes) != 1 {
return fmt.Errorf("only support single node for `node` mode currentlywhen `pass_host` is `node`")
}
}

Expand Down
69 changes: 69 additions & 0 deletions api/internal/handler/upstream/upstream_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,75 @@ func TestUpstream_Create(t *testing.T) {
},
wantErr: nil,
},
{
caseDesc: "when nodes address without port and pass host is node, create should succeed",
getCalled: true,
giveInput: &entity.Upstream{
BaseInfo: entity.BaseInfo{
ID: "u1",
},
UpstreamDef: entity.UpstreamDef{
Name: "upstream1",
Timeout: &entity.Timeout{
Connect: 15,
Send: 15,
Read: 15,
},
Key: "server_addr",
Nodes: map[string]float64{"127.0.0.1": 100},
PassHost: "node",
},
},
giveRet: &entity.Upstream{
BaseInfo: entity.BaseInfo{
ID: "u1",
},
UpstreamDef: entity.UpstreamDef{
Name: "upstream1",
Timeout: &entity.Timeout{
Connect: 15,
Send: 15,
Read: 15,
},
Key: "server_addr",
Nodes: map[string]float64{"127.0.0.1": 100},
PassHost: "node",
},
},
wantInput: &entity.Upstream{
BaseInfo: entity.BaseInfo{
ID: "u1",
},
UpstreamDef: entity.UpstreamDef{
Name: "upstream1",
Timeout: &entity.Timeout{
Connect: 15,
Send: 15,
Read: 15,
},
Key: "server_addr",
Nodes: map[string]float64{"127.0.0.1": 100},
PassHost: "node",
},
},
wantRet: &entity.Upstream{
BaseInfo: entity.BaseInfo{
ID: "u1",
},
UpstreamDef: entity.UpstreamDef{
Name: "upstream1",
Timeout: &entity.Timeout{
Connect: 15,
Send: 15,
Read: 15,
},
Key: "server_addr",
Nodes: map[string]float64{"127.0.0.1": 100},
PassHost: "node",
},
},
wantErr: nil,
},
{
caseDesc: "create failed, create return error",
getCalled: true,
Expand Down
66 changes: 66 additions & 0 deletions api/test/e2enew/upstream/upstream_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
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,
})
})

ginkgo.By("create route using the upstream3", func() {
base.RunTestCase(base.HttpTestCase{
Object: base.ManagerApiExpect(),
Method: http.MethodPut,
Path: "/apisix/admin/routes/1",
Body: `{
"name": "route1",
"uri": "/hello",
"upstream_id": "3"
}`,
Headers: map[string]string{"Authorization": base.GetToken()},
ExpectStatus: http.StatusOK,
Sleep: base.SleepTime,
})
})

ginkgo.By("hit the route just created", func() {
base.RunTestCase(base.HttpTestCase{
Object: base.APISIXExpect(),
Method: http.MethodGet,
Path: "/hello",
ExpectStatus: http.StatusOK,
ExpectBody: "hello",
Sleep: base.SleepTime,
})
})

ginkgo.By("delete route", func() {
base.RunTestCase(base.HttpTestCase{
Object: base.ManagerApiExpect(),
Method: http.MethodDelete,
Path: "/apisix/admin/routes/1",
Headers: map[string]string{"Authorization": base.GetToken()},
ExpectStatus: http.StatusOK,
})
})
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add test to check the upstream is work ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need a test case for DP

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. it is done now

ginkgo.It("create upstream failed, name existed", func() {
createUpstreamBody := make(map[string]interface{})
createUpstreamBody["name"] = "upstream2"
Expand Down Expand Up @@ -252,6 +309,15 @@ var _ = ginkgo.Describe("Upstream", func() {
ExpectStatus: http.StatusOK,
})
})
ginkgo.It("delete upstream3", func() {
base.RunTestCase(base.HttpTestCase{
Object: base.ManagerApiExpect(),
Method: http.MethodDelete,
Path: "/apisix/admin/upstreams/3",
Headers: map[string]string{"Authorization": base.GetToken()},
ExpectStatus: http.StatusOK,
})
})
ginkgo.It("hit the route just deleted", func() {
base.RunTestCase(base.HttpTestCase{
Object: base.APISIXExpect(),
Expand Down