Merge lp:~dimitern/juju-core/382-api-provisioner-machine-nics into lp:~go-bot/juju-core/trunk

[Dimiter Naydenov]

api/provisioner: Allow adding networks and NICs

This adds two new Provisioner API calls:
 * AddNetwork (exposed as st.AddNetworks bulk call on
   the client-side provisioner API)
 * AddNetworkInterface (exposed in the client-side API
   as machine.AddNetworkInterfaces bulk call)

These will be used to set the networks/interfaces that
will be configured by MAAS at provisioning time.

Next, we'll make the necessary changes to StartInstance()
in general (and in provider/maas specifically) to return
what networks will the machine start with.

https://codereview.appspot.com/84570043/

R=gz

api/provisioner: Allow adding networks and NICs

This adds two new Provisioner API calls:
 * AddNetwork (exposed as st.AddNetworks bulk call on
   the client-side provisioner API)
 * AddNetworkInterface (exposed in the client-side API
   as machine.AddNetworkInterfaces bulk call)

These will be used to set the networks/interfaces that
will be configured by MAAS at provisioning time.

Next, we'll make the necessary changes to StartInstance()
in general (and in provider/maas specifically) to return
what networks will the machine start with.

https://codereview.appspot.com/84570043/

[Dimiter Naydenov (dimitern)]

Reviewers: mp+214272_code.launchpad.net,

Message:
Please take a look.

Description:
api/provisioner: Allow adding networks and NICs

This adds two new Provisioner API calls:
  * AddNetwork (exposed as st.AddNetworks bulk call on
    the client-side provisioner API)
  * AddNetworkInterface (exposed in the client-side API
    as machine.AddNetworkInterfaces bulk call)

These will be used to set the networks/interfaces that
will be configured by MAAS at provisioning time.

Next, we'll make the necessary changes to StartInstance()
in general (and in provider/maas specifically) to return
what networks will the machine start with.

https://code.launchpad.net/~dimitern/juju-core/382-api-provisioner-machine-nics/+merge/214272

Requires:
https://code.launchpad.net/~dimitern/juju-core/381-state-machine-nics/+merge/213796

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/84570043/

Affected files (+322, -4 lines):
   A [revision details]
   M state/api/params/internal.go
   M state/api/provisioner/machine.go
   M state/api/provisioner/provisioner.go
   M state/api/provisioner/provisioner_test.go
   M state/apiserver/provisioner/provisioner.go
   M state/apiserver/provisioner/provisioner_test.go

[Martin Packman (gz)]

LGTM. I am a little panicked around future-proofing though, I think
these api structs will want to look a little different, or maybe quite a
bit different, when we add other clouds and better networking support.
We seem to be reserving some pretty generic API names 'AddNetworks" and
will probably want a number of revisions. We may get to several -Ex
suffixes or something.

https://codereview.appspot.com/84570043/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/84570043/diff/1/state/api/params/internal.go#newcode398
state/api/params/internal.go:398: NetworkName string
This will probably be a non-name id for clouds other than MAAS.
Shouldn't matter apart from naming.

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/machine.go
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/machine.go#newcode85
state/api/provisioner/machine.go:85: if interfaces[i].MachineTag !=
m.tag {
Why the conditional?

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisioner_test.go
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisioner_test.go#newcode359
state/api/provisioner/provisioner_test.go:359: c.Assert(net.Name(),
gc.Equals, expectParams.Name)
Can use c.Check for these two after the err Assert.

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisioner_test.go#newcode412
state/api/provisioner/provisioner_test.go:412: // Ensure only the first
error is reported.
This feels like a seperate testcase really, the current test is > one
screen high which is a bit of a personal bugbear of mine. Sharing setup
steps is not that hard.

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/provisioner_test.go
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/provisioner_test.go#newcode828
state/apiserver/provisioner/provisioner_test.go:828:
c.Assert(net.Name(), gc.Equals, "vlan42")
Can use c.Check, and below.

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/provisioner_test.go#newcode839
state/apiserver/provisioner/provisioner_test.go:839: // Test it fails
with a non-environment-manager.
This feels like a seperate testcase.

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/provisioner_test.go#newcode850
state/apiserver/provisioner/provisioner_test.go:850: func (s
*withoutStateServerSuite) TestAddNetworkInterface(c *gc.C) {
Giant complicated test again...

https://codereview.appspot.com/84570043/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/84570043/diff/1/[revision%20details]
File [revision details] (right):

https://codereview.appspot.com/84570043/diff/1/[revision%20details]#newcode1
[revision details]:1: Old revision:
<email address hidden>
> LGTM. I am a little panicked around future-proofing though, I
> think these api structs will want to look a little different,
> or maybe quite a bit different, when we add other clouds and
> better networking support. We seem to be reserving some
> pretty generic API names 'AddNetworks" and will probably want
> a number of revisions. We may get to several -Ex suffixes or
> something.

Structs can evolve and we can phase-out API calls and add new ones with
extended functionality. This is a general problem with the API w/o
versioning.

https://codereview.appspot.com/84570043/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/84570043/diff/1/state/api/params/internal.go#newcode398
state/api/params/internal.go:398: NetworkName string
On 2014/04/04 15:40:02, gz wrote:
> This will probably be a non-name id for clouds other than MAAS.
Shouldn't matter
> apart from naming.

We can add fields as needed later (and "name" can also be a UUID if
needed :)

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/machine.go
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/machine.go#newcode85
state/api/provisioner/machine.go:85: if interfaces[i].MachineTag !=
m.tag {
On 2014/04/04 15:40:02, gz wrote:
> Why the conditional?

To make sure we can only add NICs to the current machine (in case it's
empty or different). There are tests for that in state/api/provisioner.

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisioner_test.go
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisioner_test.go#newcode359
state/api/provisioner/provisioner_test.go:359: c.Assert(net.Name(),
gc.Equals, expectParams.Name)
On 2014/04/04 15:40:02, gz wrote:
> Can use c.Check for these two after the err Assert.

Done.

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisioner_test.go#newcode412
state/api/provisioner/provisioner_test.go:412: // Ensure only the first
error is reported.
On 2014/04/04 15:40:02, gz wrote:
> This feels like a seperate testcase really, the current test is > one
screen
> high which is a bit of a personal bugbear of mine. Sharing setup steps
is not
> that hard.

Moved to a separate test case.

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/provisioner_test.go
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/provisioner_test.go#newcode828
state/apiserver/provisioner/provisioner_test.go:828:
c.Assert(net.Name(), gc.Equals, "vlan42")
On 2014/04/04 15:40:02, gz wrote:
> Can use c.Check, and below.

Done.

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/provis...

[William Reade (fwereade)]

I'm a bit disappointed that we're making this so chatty, when ISTM that
we could pack all this into SetProvisioned, but... well, deadlines are
tight and internal APIs are much easier to change in future. Let it
stand, I guess :).

https://codereview.appspot.com/84570043/

=== modified file 'state/api/params/internal.go'
--- state/api/params/internal.go	2014-03-31 11:59:16 +0000
+++ state/api/params/internal.go	2014-04-04 16:33:28 +0000
@@ -378,6 +378,32 @@
 	Results []NetworkResult
 }
 
+// NetworkParams holds a single network definition.
+type NetworkParams struct {
+	Name    string
+	CIDR    string
+	VLANTag int
+}
+
+// AddNetworkParams holds the parameters for making an AddNetwork call.
+type AddNetworkParams struct {
+	Networks []NetworkParams
+}
+
+// NetworkInterfaceParams holds a single network interface definition.
+type NetworkInterfaceParams struct {
+	MACAddress    string
+	MachineTag    string
+	InterfaceName string
+	NetworkName   string
+}
+
+// AddNetworkInterfaceParams holds the parameters for making an
+// AddNetworkInterface call.
+type AddNetworkInterfaceParams struct {
+	Interfaces []NetworkInterfaceParams
+}
+
 // AgentGetEntitiesResults holds the results of a
 // agent.API.GetEntities call.
 type AgentGetEntitiesResults struct {

=== modified file 'state/api/provisioner/machine.go'
--- state/api/provisioner/machine.go	2014-04-04 09:08:42 +0000
+++ state/api/provisioner/machine.go	2014-04-04 16:33:28 +0000
@@ -74,6 +74,34 @@
 	return result.IncludeNetworks, result.ExcludeNetworks, nil
 }
 
+// AddNetworkInterfaces creates one or more network interfaces each on
+// an existing network and bound to the machine, which must not be
+// provisioned yet. MachineTag inside interfaces params is always set
+// to the current machine's tag. If any operation fails, the first
+// error is returned.
+func (m *Machine) AddNetworkInterfaces(interfaces []params.NetworkInterfaceParams) error {
+	var results params.ErrorResults
+	for i, _ := range interfaces {
+		if interfaces[i].MachineTag != m.tag {
+			interfaces[i].MachineTag = m.tag
+		}
+	}
+	args := params.AddNetworkInterfaceParams{Interfaces: interfaces}
+	err := m.st.call("AddNetworkInterface", args, &results)
+	if err != nil {
+		return err
+	}
+	if n := len(results.Results); n != len(interfaces) {
+		return fmt.Errorf("expected %d result(s), got %d", len(interfaces), n)
+	}
+	for _, result := range results.Results {
+		if err := result.Error; err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // SetStatus sets the status of the machine.
 func (m *Machine) SetStatus(status params.Status, info string, data params.StatusData) error {
 	var result params.ErrorResults

=== modified file 'state/api/provisioner/provisioner.go'
--- state/api/provisioner/provisioner.go	2014-03-26 03:15:41 +0000
+++ state/api/provisioner/provisioner.go	2014-04-04 16:33:28 +0000
@@ -143,3 +143,23 @@
 	}
 	return machines, results.Results, nil
 }
+
+// AddNetworks creates one or more networks with the given parameters.
+// If any operation fails, the first error is returned.
+func (st *State) AddNetworks(networks []params.NetworkParams) error {
+	var results params.ErrorResults
+	args := params.AddNetworkParams{Networks: networks}
+	err := st.call("AddNetwork", args, &results)
+	if err != nil {
+		return err
+	}
+	if n := len(results.Results); n != len(networks) {
+		return fmt.Errorf("expected %d result(s), got %d", len(networks), n)
+	}
+	for _, result := range results.Results {
+		if err := result.Error; err != nil {
+			return err
+		}
+	}
+	return nil
+}

=== modified file 'state/api/provisioner/provisioner_test.go'
--- state/api/provisioner/provisioner_test.go	2014-04-04 09:08:42 +0000
+++ state/api/provisioner/provisioner_test.go	2014-04-04 16:33:28 +0000
@@ -13,6 +13,7 @@
 	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/juju/testing"
+	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/api/params"
@@ -316,8 +317,8 @@
 	c.Assert(cons, gc.DeepEquals, constraints.Value{})
 }
 
-func (s *provisionerSuite) TestNetworks(c *gc.C) {
-	// Create a fresh machine with some networks.
+func (s *provisionerSuite) TestRequestedNetworks(c *gc.C) {
+	// Create a fresh machine with some requested networks.
 	template := state.MachineTemplate{
 		Series:          "quantal",
 		Jobs:            []state.MachineJob{state.JobHostUnits},
@@ -343,6 +344,97 @@
 	c.Assert(excludeNetworks, gc.HasLen, 0)
 }
 
+func (s *provisionerSuite) TestAddNetworks(c *gc.C) {
+	args := []params.NetworkParams{
+		{Name: "vlan42", CIDR: "0.1.2.0/24", VLANTag: 42},
+		{Name: "net1", CIDR: "0.2.1.0/24", VLANTag: 0},
+	}
+	err := s.provisioner.AddNetworks(args)
+	c.Assert(err, gc.IsNil)
+
+	assertNetwork := func(name string, expectParams params.NetworkParams) {
+		net, err := s.State.Network(name)
+		c.Assert(err, gc.IsNil)
+		c.Check(net.CIDR(), gc.Equals, expectParams.CIDR)
+		c.Check(net.Name(), gc.Equals, expectParams.Name)
+		c.Check(net.VLANTag(), gc.Equals, expectParams.VLANTag)
+	}
+	assertNetwork("vlan42", args[0])
+	assertNetwork("net1", args[1])
+
+	// Test the first error is returned.
+	args = []params.NetworkParams{
+		{Name: "net2", CIDR: "0.2.2.0/24", VLANTag: 0},
+		{Name: "", CIDR: "0.1.2.0/24", VLANTag: 0},
+		{Name: "net2", CIDR: "0.2.2.0/24", VLANTag: -1},
+	}
+	err = s.provisioner.AddNetworks(args)
+	c.Assert(err, gc.ErrorMatches, `cannot add network "": name must be not empty`)
+
+	assertNetwork("net2", args[0])
+}
+
+func (s *provisionerSuite) addMachineAndNetworks(c *gc.C) (*state.Machine, *provisioner.Machine) {
+	err := s.provisioner.AddNetworks([]params.NetworkParams{
+		{Name: "vlan42", CIDR: "0.1.2.0/24", VLANTag: 42},
+		{Name: "net1", CIDR: "0.2.1.0/24", VLANTag: 0},
+	})
+	c.Assert(err, gc.IsNil)
+	machine, err := s.State.AddMachine("quantal", state.JobHostUnits)
+	c.Assert(err, gc.IsNil)
+	apiMachine, err := s.provisioner.Machine(machine.Tag())
+	c.Assert(err, gc.IsNil)
+	return machine, apiMachine
+}
+
+func (s *provisionerSuite) TestMachineAddNetworkInterfaces(c *gc.C) {
+	machine, apiMachine := s.addMachineAndNetworks(c)
+
+	args := []params.NetworkInterfaceParams{
+		{"aa:bb:cc:dd:ee:f0", machine.Tag(), "eth0", "net1"},
+		{"aa:bb:cc:dd:ee:f1", "", "eth0", "net1"},             // tag filled in when empty
+		{"aa:bb:cc:dd:ee:f2", "machine-42", "eth2", "vlan42"}, // tag overwritten
+	}
+	err := apiMachine.AddNetworkInterfaces(args)
+	c.Assert(err, gc.IsNil)
+
+	// Check the interfaces are there.
+	ifaces, err := machine.NetworkInterfaces()
+	c.Assert(err, gc.IsNil)
+	c.Assert(ifaces, gc.HasLen, len(args))
+	actual := make([]params.NetworkInterfaceParams, len(args))
+	for i, iface := range ifaces {
+		actual[i] = params.NetworkInterfaceParams{
+			MACAddress:    iface.MACAddress(),
+			MachineTag:    names.MachineTag(iface.MachineId()),
+			InterfaceName: iface.InterfaceName(),
+			NetworkName:   iface.NetworkName(),
+		}
+	}
+	c.Assert(actual, jc.SameContents, args)
+}
+
+func (s *provisionerSuite) TestMachineAddNetworkInterfacesReportsFirstError(c *gc.C) {
+	machine, apiMachine := s.addMachineAndNetworks(c)
+
+	// Ensure only the first error is reported.
+	args := []params.NetworkInterfaceParams{
+		{"aa:bb:cc:dd:ee:f3", "", "eth3", "net1"},
+		{"aa:bb:cc:dd:ee:f4", "", "eth0", "missing"},
+		{"invalid", "", "eth42", "net1"},
+	}
+	err := apiMachine.AddNetworkInterfaces(args)
+	c.Assert(err, gc.ErrorMatches, `cannot add network interface to machine 1: network "missing" not found`)
+
+	ifaces, err := machine.NetworkInterfaces()
+	c.Assert(err, gc.IsNil)
+	c.Assert(ifaces, gc.HasLen, 1)
+	c.Assert(ifaces[0].MachineId(), gc.Equals, machine.Id())
+	c.Assert(ifaces[0].MACAddress(), gc.Equals, args[0].MACAddress)
+	c.Assert(ifaces[0].InterfaceName(), gc.Equals, args[0].InterfaceName)
+	c.Assert(ifaces[0].NetworkName(), gc.Equals, args[0].NetworkName)
+}
+
 func (s *provisionerSuite) TestWatchContainers(c *gc.C) {
 	apiMachine, err := s.provisioner.Machine(s.machine.Tag())
 	c.Assert(err, gc.IsNil)

=== modified file 'state/apiserver/provisioner/provisioner.go'
--- state/apiserver/provisioner/provisioner.go	2014-04-04 09:08:42 +0000
+++ state/apiserver/provisioner/provisioner.go	2014-04-04 16:33:28 +0000
@@ -432,6 +432,42 @@
 	return result, nil
 }
 
+// AddNetwork creates one or more new networks with the given parameters.
+// Only the environment manager can add networks.
+func (p *ProvisionerAPI) AddNetwork(args params.AddNetworkParams) (params.ErrorResults, error) {
+	result := params.ErrorResults{
+		Results: make([]params.ErrorResult, len(args.Networks)),
+	}
+	if !p.authorizer.AuthEnvironManager() {
+		return result, common.ErrPerm
+	}
+	for i, arg := range args.Networks {
+		_, err := p.st.AddNetwork(arg.Name, arg.CIDR, arg.VLANTag)
+		result.Results[i].Error = common.ServerError(err)
+	}
+	return result, nil
+}
+
+// AddNetworkInterface creates one or more new network interfaces with
+// the given parameters.
+func (p *ProvisionerAPI) AddNetworkInterface(args params.AddNetworkInterfaceParams) (params.ErrorResults, error) {
+	result := params.ErrorResults{
+		Results: make([]params.ErrorResult, len(args.Interfaces)),
+	}
+	canAccess, err := p.getAuthFunc()
+	if err != nil {
+		return result, err
+	}
+	for i, arg := range args.Interfaces {
+		machine, err := p.getMachine(canAccess, arg.MachineTag)
+		if err == nil {
+			_, err = machine.AddNetworkInterface(arg.MACAddress, arg.InterfaceName, arg.NetworkName)
+		}
+		result.Results[i].Error = common.ServerError(err)
+	}
+	return result, nil
+}
+
 // SetProvisioned sets the provider specific machine id, nonce and
 // metadata for each given machine. Once set, the instance id cannot
 // be changed.

=== modified file 'state/apiserver/provisioner/provisioner_test.go'
--- state/apiserver/provisioner/provisioner_test.go	2014-04-04 09:08:42 +0000
+++ state/apiserver/provisioner/provisioner_test.go	2014-04-04 16:33:28 +0000
@@ -15,6 +15,7 @@
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/juju/osenv"
 	"launchpad.net/juju-core/juju/testing"
+	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/apiserver/common"
@@ -758,8 +759,8 @@
 	})
 }
 
-func (s *withoutStateServerSuite) TestNetworks(c *gc.C) {
-	// Add a machine with some networks.
+func (s *withoutStateServerSuite) TestRequestedNetworks(c *gc.C) {
+	// Add a machine with some requested networks.
 	template := state.MachineTemplate{
 		Series:          "quantal",
 		Jobs:            []state.MachineJob{state.JobHostUnits},
@@ -798,6 +799,137 @@
 	})
 }
 
+func (s *withoutStateServerSuite) TestAddNetwork(c *gc.C) {
+	args := params.AddNetworkParams{Networks: []params.NetworkParams{
+		{Name: "vlan42", CIDR: "0.1.2.0/24", VLANTag: 42},
+		{Name: "net1", CIDR: "0.2.1.0/24", VLANTag: 0},
+		{Name: "", CIDR: "0.1.3.0/24", VLANTag: 0},
+		{Name: "net2", CIDR: "invalid", VLANTag: 0},
+		{Name: "net1", CIDR: "0.1.4.0/24", VLANTag: 0},
+		{Name: "net2", CIDR: "0.1.5.0/24", VLANTag: -1},
+	}}
+	result, err := s.provisioner.AddNetwork(args)
+	c.Assert(err, gc.IsNil)
+	prefix := "cannot add network %q: "
+	c.Assert(result, jc.DeepEquals, params.ErrorResults{
+		Results: []params.ErrorResult{
+			{Error: nil},
+			{Error: nil},
+			{Error: apiservertesting.PrefixedError(
+				fmt.Sprintf(prefix, ""),
+				"name must be not empty")},
+			{Error: apiservertesting.PrefixedError(
+				fmt.Sprintf(prefix, "net2"),
+				"invalid CIDR address: invalid")},
+			{Error: apiservertesting.PrefixedError(
+				fmt.Sprintf(prefix, "net1"),
+				"already exists")},
+			{Error: apiservertesting.PrefixedError(
+				fmt.Sprintf(prefix, "net2"),
+				"invalid VLAN tag -1: must be between 0 and 4094")},
+		}})
+
+	// Check add networks are there and failed ones are not.
+	net, err := s.State.Network("vlan42")
+	c.Assert(err, gc.IsNil)
+	c.Check(net.Name(), gc.Equals, "vlan42")
+	c.Check(net.CIDR(), gc.Equals, "0.1.2.0/24")
+	c.Check(net.VLANTag(), gc.Equals, 42)
+	net, err = s.State.Network("net1")
+	c.Assert(err, gc.IsNil)
+	c.Check(net.Name(), gc.Equals, "net1")
+	c.Check(net.CIDR(), gc.Equals, "0.2.1.0/24")
+	c.Check(net.VLANTag(), gc.Equals, 0)
+	_, err = s.State.Network("net2")
+	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
+}
+
+func (s *withoutStateServerSuite) TestAddNetworkFailsWithNonEnvironManager(c *gc.C) {
+	anAuthorizer := s.authorizer
+	anAuthorizer.MachineAgent = true
+	anAuthorizer.EnvironManager = false
+	aProvisioner, err := provisioner.NewProvisionerAPI(s.State, s.resources, anAuthorizer)
+	c.Assert(err, gc.IsNil)
+	c.Assert(aProvisioner, gc.NotNil)
+	_, err = aProvisioner.AddNetwork(params.AddNetworkParams{})
+	c.Assert(err, gc.ErrorMatches, "permission denied")
+}
+
+func assertInterfaces(c *gc.C, machine *state.Machine, expect ...params.NetworkInterfaceParams) {
+	ifaces, err := machine.NetworkInterfaces()
+	c.Assert(err, gc.IsNil)
+	c.Assert(ifaces, gc.HasLen, len(expect))
+	actual := make([]params.NetworkInterfaceParams, len(expect))
+	for i, iface := range ifaces {
+		actual[i] = params.NetworkInterfaceParams{
+			MACAddress:    iface.MACAddress(),
+			MachineTag:    names.MachineTag(iface.MachineId()),
+			InterfaceName: iface.InterfaceName(),
+			NetworkName:   iface.NetworkName(),
+		}
+	}
+	c.Assert(actual, jc.SameContents, expect)
+}
+
+func (s *withoutStateServerSuite) TestAddNetworkInterface(c *gc.C) {
+	// Add a few networks first.
+	_, err := s.State.AddNetwork("net1", "0.1.2.0/24", 0)
+	c.Assert(err, gc.IsNil)
+	_, err = s.State.AddNetwork("vlan42", "0.2.1.0/24", 42)
+	c.Assert(err, gc.IsNil)
+
+	args := params.AddNetworkInterfaceParams{Interfaces: []params.NetworkInterfaceParams{
+		{"aa:bb:cc:dd:ee:f0", s.machines[0].Tag(), "eth0", "net1"},
+		{"aa:bb:cc:dd:ee:f1", s.machines[1].Tag(), "eth0", "net1"},
+		{"aa:bb:cc:dd:ee:f2", s.machines[1].Tag(), "eth1", "vlan42"},
+		{"invalid", s.machines[1].Tag(), "eth1", "vlan42"},
+		{"aa:bb:cc:dd:ee:ff", s.machines[1].Tag(), "", "net1"},
+		{"aa:bb:cc:dd:ee:ff", s.machines[1].Tag(), "eth1", "invalid"},
+		{"aa:bb:cc:dd:ee:ff", s.machines[1].Tag(), "eth1", ""},
+		{"aa:bb:cc:dd:ee:f1", s.machines[1].Tag(), "eth2", "net1"},
+		{"aa:bb:cc:dd:ee:f1", s.machines[2].Tag(), "eth0", "net1"},
+		{"aa:bb:cc:dd:ee:f1", "machine-42", "eth0", "net1"},
+		{"aa:bb:cc:dd:ee:f1", "unit-foo-42", "eth0", "net1"},
+	}}
+	err = s.machines[2].SetProvisioned("i-am", "fake_nonce", nil)
+	c.Assert(err, gc.IsNil)
+
+	prefix := "cannot add network interface to machine %s: "
+	prefixM1 := fmt.Sprintf(prefix, s.machines[1].Id())
+	result, err := s.provisioner.AddNetworkInterface(args)
+	c.Assert(err, gc.IsNil)
+	c.Assert(result, jc.DeepEquals, params.ErrorResults{
+		Results: []params.ErrorResult{
+			{Error: nil},
+			{Error: nil},
+			{Error: nil},
+			{Error: apiservertesting.PrefixedError(
+				prefixM1, "invalid MAC address: invalid")},
+			{Error: apiservertesting.PrefixedError(
+				prefixM1, "interface name must be not empty")},
+			{Error: apiservertesting.PrefixedError(
+				prefixM1, `network "invalid" not found`)},
+			{Error: apiservertesting.PrefixedError(
+				prefixM1, `network "" not found`)},
+			{Error: apiservertesting.PrefixedError(
+				prefixM1,
+				`interface with MAC address "aa:bb:cc:dd:ee:f1" already exists`),
+			},
+			{Error: apiservertesting.PrefixedError(
+				fmt.Sprintf(prefix, s.machines[2].Id()),
+				`machine already provisioned: dynamic network interfaces not currently supported`,
+			)},
+			{Error: apiservertesting.NotFoundError("machine 42")},
+			{Error: apiservertesting.ErrUnauthorized},
+		},
+	})
+
+	// Now check the added interfaces are there.
+	assertInterfaces(c, s.machines[0], args.Interfaces[0])
+	assertInterfaces(c, s.machines[1], args.Interfaces[1], args.Interfaces[2])
+	assertInterfaces(c, s.machines[2]) // None
+}
+
 func (s *withoutStateServerSuite) TestSetProvisioned(c *gc.C) {
 	// Provision machine 0 first.
 	hwChars := instance.MustParseHardware("arch=i386", "mem=4G")

=== modified file 'state/apiserver/testing/errors.go'
--- state/apiserver/testing/errors.go	2014-01-21 10:05:20 +0000
+++ state/apiserver/testing/errors.go	2014-04-04 16:33:28 +0000
@@ -41,3 +41,7 @@
 		Code:    "",
 	}
 }
+
+func PrefixedError(prefix, message string) *params.Error {
+	return ServerError(prefix + message)
+}