-
Notifications
You must be signed in to change notification settings - Fork 879
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
Add negotiation process for driver scope #516
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,16 +28,40 @@ func newDriver(name string, client *plugins.Client) driverapi.Driver { | |
// plugin is activated. | ||
func Init(dc driverapi.DriverCallback) error { | ||
plugins.Handle(driverapi.NetworkPluginEndpointType, func(name string, client *plugins.Client) { | ||
c := driverapi.Capability{ | ||
Scope: driverapi.GlobalScope, | ||
// negotiate driver capability with client | ||
d := newDriver(name, client) | ||
c, err := d.(*driver).getCapabilities() | ||
if err != nil { | ||
log.Errorf("error getting capability for %s due to %v", name, err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you think it will be better to default the scope to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I noticed this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There have been other breaking changes, so I don't think changing the protocol is inherently a problem. You could argue that the robustness principle indicates that treating a 404 from the plugin as the default is reasonable. I'm not sure all plugins will return a 404 though -- from memory I don't think it's part of the plugin protocol spec, so there's a chance non-conforming plugins will return an error code, and be left in an unknown state. With that in mind I think it's better to break backwards compatibility and treat all non-conforming responses as errors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @squaremo. that make sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, @squaremo gives a really convincing reason 👍 |
||
return | ||
} | ||
if err := dc.RegisterDriver(name, newDriver(name, client), c); err != nil { | ||
if err = dc.RegisterDriver(name, d, *c); err != nil { | ||
log.Errorf("error registering driver for %s due to %v", name, err) | ||
} | ||
}) | ||
return nil | ||
} | ||
|
||
// Get capability from client | ||
func (d *driver) getCapabilities() (*driverapi.Capability, error) { | ||
var capResp api.GetCapabilityResponse | ||
if err := d.call("GetCapabilities", nil, &capResp); err != nil { | ||
return nil, err | ||
} | ||
|
||
c := &driverapi.Capability{} | ||
switch capResp.Scope { | ||
case "global": | ||
c.Scope = driverapi.GlobalScope | ||
case "local": | ||
c.Scope = driverapi.LocalScope | ||
default: | ||
return nil, fmt.Errorf("invalid capability: expecting 'local' or 'global', got %s", capResp.Scope) | ||
} | ||
|
||
return c, nil | ||
} | ||
|
||
// Config is not implemented for remote drivers, since it is assumed | ||
// to be supplied to the remote process out-of-band (e.g., as command | ||
// line arguments). | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -153,6 +153,91 @@ func (test *testEndpoint) AddStaticRoute(destination *net.IPNet, routeType int, | |
return nil | ||
} | ||
|
||
func TestGetEmptyCapabilities(t *testing.T) { | ||
var plugin = "test-net-driver-empty-cap" | ||
|
||
mux := http.NewServeMux() | ||
defer setupPlugin(t, plugin, mux)() | ||
|
||
handle(t, mux, "GetCapabilities", func(msg map[string]interface{}) interface{} { | ||
return map[string]interface{}{} | ||
}) | ||
|
||
p, err := plugins.Get(plugin, driverapi.NetworkPluginEndpointType) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
d := newDriver(plugin, p.Client) | ||
if d.Type() != plugin { | ||
t.Fatal("Driver type does not match that given") | ||
} | ||
|
||
_, err = d.(*driver).getCapabilities() | ||
if err == nil { | ||
t.Fatal("There should be error reported when get empty capability") | ||
} | ||
} | ||
|
||
func TestGetExtraCapabilities(t *testing.T) { | ||
var plugin = "test-net-driver-extra-cap" | ||
|
||
mux := http.NewServeMux() | ||
defer setupPlugin(t, plugin, mux)() | ||
|
||
handle(t, mux, "GetCapabilities", func(msg map[string]interface{}) interface{} { | ||
return map[string]interface{}{ | ||
"Scope": "local", | ||
"foo": "bar", | ||
} | ||
}) | ||
|
||
p, err := plugins.Get(plugin, driverapi.NetworkPluginEndpointType) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
d := newDriver(plugin, p.Client) | ||
if d.Type() != plugin { | ||
t.Fatal("Driver type does not match that given") | ||
} | ||
|
||
c, err := d.(*driver).getCapabilities() | ||
if err != nil { | ||
t.Fatal(err) | ||
} else if c.Scope != driverapi.LocalScope { | ||
t.Fatalf("get capability '%s', expecting 'local'", c.Scope) | ||
} | ||
} | ||
|
||
func TestGetInvalidCapabilities(t *testing.T) { | ||
var plugin = "test-net-driver-invalid-cap" | ||
|
||
mux := http.NewServeMux() | ||
defer setupPlugin(t, plugin, mux)() | ||
|
||
handle(t, mux, "GetCapabilities", func(msg map[string]interface{}) interface{} { | ||
return map[string]interface{}{ | ||
"Scope": "fake", | ||
} | ||
}) | ||
|
||
p, err := plugins.Get(plugin, driverapi.NetworkPluginEndpointType) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
d := newDriver(plugin, p.Client) | ||
if d.Type() != plugin { | ||
t.Fatal("Driver type does not match that given") | ||
} | ||
|
||
_, err = d.(*driver).getCapabilities() | ||
if err == nil { | ||
t.Fatal("There should be error reported when get invalid capability") | ||
} | ||
} | ||
|
||
func TestRemoteDriver(t *testing.T) { | ||
var plugin = "test-net-driver" | ||
|
||
|
@@ -177,6 +262,11 @@ func TestRemoteDriver(t *testing.T) { | |
|
||
var networkID string | ||
|
||
handle(t, mux, "GetCapabilities", func(msg map[string]interface{}) interface{} { | ||
return map[string]interface{}{ | ||
"Scope": "global", | ||
} | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for adding this test. in addition to this positive case, can you please add a couple of negative cases which am interested in seeing for backward compatibility ?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @WeiZhang555 My previous comment got overwritten due to the recent changes. thanks for adding this test. in addition to this positive case, can you please add a couple of negative cases which am interested in seeing for backward compatibility ?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All right, I can do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @WeiZhang555 appreciate it. |
||
handle(t, mux, "CreateNetwork", func(msg map[string]interface{}) interface{} { | ||
nid := msg["NetworkID"] | ||
var ok bool | ||
|
@@ -245,38 +335,45 @@ func TestRemoteDriver(t *testing.T) { | |
t.Fatal(err) | ||
} | ||
|
||
driver := newDriver(plugin, p.Client) | ||
if driver.Type() != plugin { | ||
d := newDriver(plugin, p.Client) | ||
if d.Type() != plugin { | ||
t.Fatal("Driver type does not match that given") | ||
} | ||
|
||
c, err := d.(*driver).getCapabilities() | ||
if err != nil { | ||
t.Fatal(err) | ||
} else if c.Scope != driverapi.GlobalScope { | ||
t.Fatalf("get capability '%s', expecting 'global'", c.Scope) | ||
} | ||
|
||
netID := "dummy-network" | ||
err = driver.CreateNetwork(netID, map[string]interface{}{}) | ||
err = d.CreateNetwork(netID, map[string]interface{}{}) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
endID := "dummy-endpoint" | ||
err = driver.CreateEndpoint(netID, endID, ep, map[string]interface{}{}) | ||
err = d.CreateEndpoint(netID, endID, ep, map[string]interface{}{}) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
joinOpts := map[string]interface{}{"foo": "fooValue"} | ||
err = driver.Join(netID, endID, "sandbox-key", ep, joinOpts) | ||
err = d.Join(netID, endID, "sandbox-key", ep, joinOpts) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
if _, err = driver.EndpointOperInfo(netID, endID); err != nil { | ||
if _, err = d.EndpointOperInfo(netID, endID); err != nil { | ||
t.Fatal(err) | ||
} | ||
if err = driver.Leave(netID, endID); err != nil { | ||
if err = d.Leave(netID, endID); err != nil { | ||
t.Fatal(err) | ||
} | ||
if err = driver.DeleteEndpoint(netID, endID); err != nil { | ||
if err = d.DeleteEndpoint(netID, endID); err != nil { | ||
t.Fatal(err) | ||
} | ||
if err = driver.DeleteNetwork(netID); err != nil { | ||
if err = d.DeleteNetwork(netID); err != nil { | ||
t.Fatal(err) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scope is the only capability that we are interested in.
Just want to make sure that when we add more capabilities, libnetwork must not fail in the future if the users are using an older version of plugin that might not have implemented the newly added capabilities. We just have to make sure that the
driver.call
function doesn't fail.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, got it. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to add a test case for this specifically ?
I expect more changes to the capability structure and want to ensure backward compatibility for any such changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just found that to make use of driver.call function, the GetCapabilityResponse need to contain
Response
, without this, I need to write some duplicate codes asdriver.call
.So I think maybe we'd better keep this although the error is not what we are interested in.