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.

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

bugfix: fix can't set macaddress for endpoint #2222

Merged
merged 1 commit into from
Sep 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 14 additions & 3 deletions daemon/mgr/network.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/alibaba/pouch/pkg/errtypes"
"github.com/alibaba/pouch/pkg/meta"
"github.com/alibaba/pouch/pkg/randomid"
"github.com/alibaba/pouch/pkg/utils"

"github.com/docker/go-connections/nat"
"github.com/docker/libnetwork"
Expand Down Expand Up @@ -579,12 +580,22 @@ func endpointOptions(n libnetwork.Network, endpoint *types.Endpoint) ([]libnetwo
}
}

// generate genric endpoint options
genericOption := options.Generic{}
createOptions = append(createOptions, libnetwork.EndpointOptionGeneric(genericOption))

if len(endpoint.GenericParams) > 0 {
createOptions = append(createOptions, libnetwork.EndpointOptionGeneric(endpoint.GenericParams))
genericOption, _ = utils.MergeMap(genericOption, endpoint.GenericParams)
}

if n.Name() == endpoint.NetworkMode && endpoint.MacAddress != "" {
mac, err := net.ParseMAC(endpoint.MacAddress)
if err != nil {
return nil, err
}
genericOption, _ = utils.MergeMap(genericOption, options.Generic{netlabel.MacAddress: mac})
Copy link
Contributor

Choose a reason for hiding this comment

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

we already have Merge funtion in utils.go, can we use it ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It can't to merge the map as I want, I don't know and I don't want to knon what's wrong with it. 🐶

logrus.Debugf("generate endpoint macaddress: (%s)", endpoint.MacAddress)
}
createOptions = append(createOptions, libnetwork.EndpointOptionGeneric(genericOption))

if endpoint.DisableResolver {
createOptions = append(createOptions, libnetwork.CreateOptionDisableResolution())
}
Expand Down
21 changes: 21 additions & 0 deletions pkg/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -330,3 +330,24 @@ func StringSliceEqual(s1, s2 []string) bool {

return true
}

// MergeMap merges the m2 into m1, if it has the same keys, m2 will overwrite m1.
func MergeMap(m1 map[string]interface{}, m2 map[string]interface{}) (map[string]interface{}, error) {
if m1 == nil && m2 == nil {
return nil, fmt.Errorf("all of maps are nil")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does it return an error? I think it is quite normal to merge two nil map.

I think it is incorrect to return err. Instead, the upper caller could judge whether the returned value is nil to return error or something.

MergeMap is a general function and fundamental, it should not add some business logic in the implementation. For me, judging both of them nil is business logic.

}

if m1 == nil {
return m2, nil
}

if m2 == nil {
return m1, nil
}

for k, v := range m2 {
m1[k] = v
}

return m1, nil
}
32 changes: 32 additions & 0 deletions pkg/utils/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -544,3 +544,35 @@ func TestStringSliceEqual(t *testing.T) {
}
}
}

func TestMergeMap(t *testing.T) {
type Expect struct {
err error
key string
value interface{}
}
tests := []struct {
m1 map[string]interface{}
m2 map[string]interface{}
expect Expect
}{
{nil, nil, Expect{fmt.Errorf("all of maps are nil"), "", nil}},
{nil, map[string]interface{}{"a": "a"}, Expect{nil, "a", "a"}},
{map[string]interface{}{"a": "a"}, nil, Expect{nil, "a", "a"}},
{map[string]interface{}{"a": "a"}, map[string]interface{}{"a": "b"}, Expect{nil, "a", "b"}},
{map[string]interface{}{"a": "a"}, map[string]interface{}{"b": "b"}, Expect{nil, "b", "b"}},
}

for _, test := range tests {
m3, err := MergeMap(test.m1, test.m2)
if err != nil {
if test.expect.err.Error() != err.Error() {
t.Fatalf("MergeMap(%v, %v) expected: %v, but got %v", test.m1, test.m2, test.expect.err, err)
}
} else {
if m3[test.expect.key] != test.expect.value {
t.Fatalf("MergeMap(%v, %v) expected: %v, but got %v", test.m1, test.m2, test.expect.value, m3[test.expect.key])
}
}
}
}
48 changes: 48 additions & 0 deletions test/api_container_create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,15 @@ package main
import (
"net/url"
"sort"
"strings"

"github.com/alibaba/pouch/apis/types"
"github.com/alibaba/pouch/test/command"
"github.com/alibaba/pouch/test/environment"
"github.com/alibaba/pouch/test/request"

"github.com/go-check/check"
"github.com/gotestyourself/gotestyourself/icmd"
)

// APIContainerCreateSuite is the test suite for container create API.
Expand Down Expand Up @@ -231,3 +234,48 @@ func (suite *APIContainerCreateSuite) TestCreateNvidiaConfig(c *check.C) {

DelContainerForceMultyTime(c, cname)
}

func (suite *APIContainerCreateSuite) TestCreateWithMacAddress(c *check.C) {
cname := "TestCreateWithMacAddress"
mac := "00:16:3e:02:00:b7"
q := url.Values{}
q.Add("name", cname)
query := request.WithQuery(q)

obj := map[string]interface{}{
"Entrypoint": []string{"top"},
"Image": busyboxImage,
"MacAddress": mac,
"NetworkMode": "bridge",
}
body := request.WithJSONBody(obj)

resp, err := request.Post("/containers/create", query, body)
c.Assert(err, check.IsNil)
CheckRespStatus(c, resp, 201)

q = url.Values{}
q.Add("detachKeys", "EOF")
query = request.WithQuery(q)

resp, err = request.Post("/containers/"+cname+"/start", query)
c.Assert(err, check.IsNil)
CheckRespStatus(c, resp, 204)

ret := command.PouchRun("exec", cname, "ip", "addr", "show", "eth0")
ret.Assert(c, icmd.Success)

found := false
for _, line := range strings.Split(ret.Stdout(), "\n") {
if strings.Contains(line, "link/ether") {
if mac == strings.Fields(line)[1] {
found = true
break
}
}
}

c.Assert(found, check.Equals, true)

DelContainerForceMultyTime(c, cname)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should use defer to delete container to avoid legacy container for other tests.

}