Merge lp:~wallyworld/juju-core/machiner-access-for-uniter into lp:~go-bot/juju-core/trunk

[Ian Booth]

Allow assigned units read access to machines

This branch allows unit agents to use the machiner
API to get a machine object for their assigned
machine. The server side api looks up the machine
life and the necessary auth function is modified
to allow assigned units as well as the machine
owner.

The implementation is done as a helper function
because a downstream branch also uses it. This
work will be used by the unit agent upgrade worker.

https://codereview.appspot.com/60920043/

[Ian Booth (wallyworld)]

Reviewers: mp+205290_code.launchpad.net,

Message:
Please take a look.

Description:
Allow assigned units read access to machines

This branch allows unit agents to use the machiner
API to get a machine object for their assigned
machine. The server side api looks up the machine
life and the necessary auth function is modified
to allow assigned units as well as the machine
owner.

The implementation is done as a helper function
because a downstream branch also uses it. This
work will be used by the unit agent upgrade worker.

https://code.launchpad.net/~wallyworld/juju-core/machiner-access-for-uniter/+merge/205290

(do not edit description out of merge proposal)

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

Affected files (+224, -21 lines):
   A [revision details]
   A state/apiserver/common/auth.go
   M state/apiserver/machine/common_test.go
   M state/apiserver/machine/machiner.go
   M state/apiserver/machine/machiner_test.go

[Dimiter Naydenov (dimitern)]

Looks great so far, I have just a few suggestions.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/common/auth.go
File state/apiserver/common/auth.go (right):

https://codereview.appspot.com/60920043/diff/1/state/apiserver/common/auth.go#newcode13
state/apiserver/common/auth.go:13: // OwnerOrDeployedUnitAuthFunc
provides an AuthFunc which returns true if the caller
I still think this ought to be in the Authorizer interfaces, and should
be called AuthOwnerOrDeployedUnit. Then, you can implement it in
apiserver/root.go, as the other auth funcs, and it could be simplified
as well (no need to call GetAuthTag and GetAuthEntity, because you have
direct access to both).

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machiner_test.go
File state/apiserver/machine/machiner_test.go (right):

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machiner_test.go#newcode215
state/apiserver/machine/machiner_test.go:215: // Test that a unit agent
has read access to the assigned machine for it's unit.
Please put this suite in a separate file, like uniter_test.go.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machiner_test.go#newcode309
state/apiserver/machine/machiner_test.go:309: {Tag: "machine-1"},
I'd add "machine-0" and "machine-42" to the list of entities, just to be
extra careful and check for ErrPerm.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machiner_test.go#newcode334
state/apiserver/machine/machiner_test.go:334: {Tag: "machine-1",
Addresses: addresses},
ditto

https://codereview.appspot.com/60920043/

[lp:~wallyworld/juju-core/machiner-access-for-uniter]

2305. By Ian Booth on 2014-02-11

Merge trunk

2306. By Ian Booth on 2014-02-11

Refactor some tests

[Ian Booth (wallyworld)]

Please take a look.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/common/auth.go
File state/apiserver/common/auth.go (right):

https://codereview.appspot.com/60920043/diff/1/state/apiserver/common/auth.go#newcode13
state/apiserver/common/auth.go:13: // OwnerOrDeployedUnitAuthFunc
provides an AuthFunc which returns true if the caller
On 2014/02/10 08:41:29, dimitern wrote:
> I still think this ought to be in the Authorizer interfaces, and
should be
> called AuthOwnerOrDeployedUnit. Then, you can implement it in
apiserver/root.go,
> as the other auth funcs, and it could be simplified as well (no need
to call
> GetAuthTag and GetAuthEntity, because you have direct access to both).

That doesn't work so well. The OwnerOrDeployedUnitAuthFunc func is a
little involved, and also needs to be implemented properly on the
FakeAuthorizer. It's better to have a common helper function and pass in
the authorizer to use, allowing the OwnerOrDeployedUnitAuthFunc to use
the primitives offered by the respective authorisers. As an aside, there
a similarly complex bit of auth logic in deployer which could also be
pulled out as a separate function.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machiner_test.go
File state/apiserver/machine/machiner_test.go (right):

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machiner_test.go#newcode215
state/apiserver/machine/machiner_test.go:215: // Test that a unit agent
has read access to the assigned machine for it's unit.
On 2014/02/10 08:41:29, dimitern wrote:
> Please put this suite in a separate file, like uniter_test.go.

Done.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machiner_test.go#newcode309
state/apiserver/machine/machiner_test.go:309: {Tag: "machine-1"},
On 2014/02/10 08:41:29, dimitern wrote:
> I'd add "machine-0" and "machine-42" to the list of entities, just to
be extra
> careful and check for ErrPerm.

Done.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machiner_test.go#newcode309
state/apiserver/machine/machiner_test.go:309: {Tag: "machine-1"},
On 2014/02/10 08:41:29, dimitern wrote:
> I'd add "machine-0" and "machine-42" to the list of entities, just to
be extra
> careful and check for ErrPerm.

Done.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machiner_test.go#newcode334
state/apiserver/machine/machiner_test.go:334: {Tag: "machine-1",
Addresses: addresses},
On 2014/02/10 08:41:29, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/60920043/diff/1/state/apiserver/machine/machiner_test.go#newcode334
state/apiserver/machine/machiner_test.go:334: {Tag: "machine-1",
Addresses: addresses},
On 2014/02/10 08:41:29, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/60920043/

[Dimiter Naydenov (dimitern)]

The diff is screwed, but otherwise LGTM assuming you made what you said.

https://codereview.appspot.com/60920043/

[William Reade (fwereade)]

On 2014/02/11 09:53:47, dimitern wrote:
> The diff is screwed, but otherwise LGTM assuming you made what you
said.

as discussed live, the upgrader doesn't need changes so not lgtm

https://codereview.appspot.com/60920043/

Unmerged revisions

2306. By Ian Booth on 2014-02-11

Refactor some tests

2305. By Ian Booth on 2014-02-11

Merge trunk

2304. By Ian Booth on 2014-02-07

Unit agents have read access to machines on which their units are deployed

=== added file 'state/apiserver/common/auth.go'
--- state/apiserver/common/auth.go	1970-01-01 00:00:00 +0000
+++ state/apiserver/common/auth.go	2014-02-11 01:14:25 +0000
@@ -0,0 +1,38 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package common
+
+import (
+	"fmt"
+
+	"launchpad.net/juju-core/names"
+	"launchpad.net/juju-core/state"
+)
+
+// OwnerOrDeployedUnitAuthFunc provides an AuthFunc which returns true if the caller
+// is the machine owner or a unit agent for a unit which is deployed on the machine.
+func OwnerOrDeployedUnitAuthFunc(authorizer Authorizer) func() (AuthFunc, error) {
+	return func() (AuthFunc, error) {
+		tag := authorizer.GetAuthTag()
+		kind, _, err := names.ParseTag(tag, "")
+		if err != nil {
+			return nil, err
+		}
+		if kind == names.MachineTagKind {
+			return authorizer.AuthOwner, nil
+		}
+		unit, ok := authorizer.GetAuthEntity().(*state.Unit)
+		if !ok {
+			return nil, fmt.Errorf("%v is not a unit", authorizer.GetAuthTag())
+		}
+		id, err := unit.AssignedMachineId()
+		if err != nil {
+			return nil, err
+		}
+		return func(tag string) bool {
+			assignedMachineTag := names.MachineTag(id)
+			return tag == assignedMachineTag
+		}, nil
+	}
+}

=== modified file 'state/apiserver/machine/common_test.go'
--- state/apiserver/machine/common_test.go	2014-01-22 19:28:08 +0000
+++ state/apiserver/machine/common_test.go	2014-02-11 01:14:25 +0000
@@ -7,6 +7,8 @@
 
 	"launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/apiserver/common"
+	"launchpad.net/juju-core/state/apiserver/machine"
 	apiservertesting "launchpad.net/juju-core/state/apiserver/testing"
 	coretesting "launchpad.net/juju-core/testing"
 )
@@ -19,6 +21,8 @@
 	testing.JujuConnSuite
 
 	authorizer apiservertesting.FakeAuthorizer
+	resources  *common.Resources
+	machiner   *machine.MachinerAPI
 
 	machine0 *state.Machine
 	machine1 *state.Machine
@@ -34,11 +38,7 @@
 	s.machine1, err = s.State.AddMachine("quantal", state.JobHostUnits)
 	c.Assert(err, gc.IsNil)
 
-	// Create a FakeAuthorizer so we can check permissions,
-	// set up assuming machine 1 has logged in.
-	s.authorizer = apiservertesting.FakeAuthorizer{
-		Tag:          s.machine1.Tag(),
-		LoggedIn:     true,
-		MachineAgent: true,
-	}
+	// Create the resource registry separately to track invocations to
+	// Register.
+	s.resources = common.NewResources()
 }

=== modified file 'state/apiserver/machine/machiner.go'
--- state/apiserver/machine/machiner.go	2014-01-20 21:29:08 +0000
+++ state/apiserver/machine/machiner.go	2014-02-11 01:14:25 +0000
@@ -27,15 +27,13 @@
 
 // NewMachinerAPI creates a new instance of the Machiner API.
 func NewMachinerAPI(st *state.State, resources *common.Resources, authorizer common.Authorizer) (*MachinerAPI, error) {
-	if !authorizer.AuthMachineAgent() {
+	if !authorizer.AuthMachineAgent() && !authorizer.AuthUnitAgent() {
 		return nil, common.ErrPerm
 	}
 	getCanModify := func() (common.AuthFunc, error) {
 		return authorizer.AuthOwner, nil
 	}
-	getCanRead := func() (common.AuthFunc, error) {
-		return authorizer.AuthOwner, nil
-	}
+	getCanRead := common.OwnerOrDeployedUnitAuthFunc(authorizer)
 	return &MachinerAPI{
 		LifeGetter:         common.NewLifeGetter(st, getCanRead),
 		StatusSetter:       common.NewStatusSetter(st, getCanModify),

=== modified file 'state/apiserver/machine/machiner_test.go'
--- state/apiserver/machine/machiner_test.go	2014-01-20 21:00:43 +0000
+++ state/apiserver/machine/machiner_test.go	2014-02-11 01:14:25 +0000
@@ -9,7 +9,6 @@
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api/params"
-	"launchpad.net/juju-core/state/apiserver/common"
 	"launchpad.net/juju-core/state/apiserver/machine"
 	apiservertesting "launchpad.net/juju-core/state/apiserver/testing"
 	statetesting "launchpad.net/juju-core/state/testing"
@@ -17,9 +16,6 @@
 
 type machinerSuite struct {
 	commonSuite
-
-	resources *common.Resources
-	machiner  *machine.MachinerAPI
 }
 
 var _ = gc.Suite(&machinerSuite{})
@@ -27,18 +23,22 @@
 func (s *machinerSuite) SetUpTest(c *gc.C) {
 	s.commonSuite.SetUpTest(c)
 
-	// Create the resource registry separately to track invocations to
-	// Register.
-	s.resources = common.NewResources()
-
+	// Create a FakeAuthorizer so we can check permissions,
+	// set up assuming machine 1 has logged in.
+	s.authorizer = apiservertesting.FakeAuthorizer{
+		Tag:          s.machine1.Tag(),
+		LoggedIn:     true,
+		MachineAgent: true,
+	}
+	var err error
 	// Create a machiner API for machine 1.
-	machiner, err := machine.NewMachinerAPI(
+	s.machiner, err = machine.NewMachinerAPI(
 		s.State,
 		s.resources,
 		s.authorizer,
 	)
 	c.Assert(err, gc.IsNil)
-	s.machiner = machiner
+	c.Assert(s.machiner, gc.NotNil)
 }
 
 func (s *machinerSuite) TestMachinerFailsWithNonMachineAgentUser(c *gc.C) {

=== added file 'state/apiserver/machine/uniter_test.go'
--- state/apiserver/machine/uniter_test.go	1970-01-01 00:00:00 +0000
+++ state/apiserver/machine/uniter_test.go	2014-02-11 01:14:25 +0000
@@ -0,0 +1,188 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package machine_test
+
+import (
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/instance"
+	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/api/params"
+	"launchpad.net/juju-core/state/apiserver/machine"
+	apiservertesting "launchpad.net/juju-core/state/apiserver/testing"
+	statetesting "launchpad.net/juju-core/state/testing"
+)
+
+// Test that a unit agent has read access to the assigned machine for it's unit.
+type uniterSuite struct {
+	commonSuite
+}
+
+var _ = gc.Suite(&uniterSuite{})
+
+func (s *uniterSuite) SetUpTest(c *gc.C) {
+	s.commonSuite.SetUpTest(c)
+
+	// Deploy a unit to machine 1.
+	mysql := s.AddTestingService(c, "mysql", s.AddTestingCharm(c, "mysql"))
+	mysql0, err := mysql.AddUnit()
+	c.Assert(err, gc.IsNil)
+	err = mysql0.AssignToMachine(s.machine1)
+	c.Assert(err, gc.IsNil)
+	// Create another unit not on machine 1.
+	_, err = mysql.AddUnit()
+	c.Assert(err, gc.IsNil)
+
+	// Create a FakeAuthorizer so we can check permissions,
+	// set up assuming mysql/0 has logged in.
+	s.authorizer = apiservertesting.FakeAuthorizer{
+		LoggedIn:  true,
+		UnitAgent: true,
+		Tag:       mysql0.Tag(),
+		Entity:    mysql0,
+	}
+
+	// Create a machiner API for mysql/0.
+	s.machiner, err = machine.NewMachinerAPI(
+		s.State,
+		s.resources,
+		s.authorizer,
+	)
+	c.Assert(err, gc.IsNil)
+	c.Assert(s.machiner, gc.NotNil)
+}
+
+func (s *uniterSuite) TestMachinerFailsWithNonUnitAgentUser(c *gc.C) {
+	anAuthorizer := s.authorizer
+	anAuthorizer.UnitAgent = false
+	aMachiner, err := machine.NewMachinerAPI(s.State, s.resources, anAuthorizer)
+	c.Assert(err, gc.NotNil)
+	c.Assert(aMachiner, gc.IsNil)
+	c.Assert(err, gc.ErrorMatches, "permission denied")
+}
+
+func (s *uniterSuite) TestSetStatusFails(c *gc.C) {
+	err := s.machine1.SetStatus(params.StatusStarted, "foo", nil)
+	c.Assert(err, gc.IsNil)
+
+	args := params.SetStatus{
+		Entities: []params.SetEntityStatus{
+			{Tag: "machine-1", Status: params.StatusError, Info: "not really"},
+		}}
+	result, err := s.machiner.SetStatus(args)
+	c.Assert(err, gc.IsNil)
+	c.Assert(result, gc.DeepEquals, params.ErrorResults{
+		Results: []params.ErrorResult{
+			{apiservertesting.ErrUnauthorized},
+		},
+	})
+
+	// Verify machine 1 - no change.
+	status, info, _, err := s.machine1.Status()
+	c.Assert(err, gc.IsNil)
+	c.Assert(status, gc.Equals, params.StatusStarted)
+	c.Assert(info, gc.Equals, "foo")
+}
+
+func (s *uniterSuite) TestLife(c *gc.C) {
+	c.Assert(s.machine1.Life(), gc.Equals, state.Alive)
+
+	args := params.Entities{Entities: []params.Entity{
+		{Tag: "machine-1"},
+		{Tag: "machine-0"},
+		{Tag: "machine-42"},
+	}}
+	result, err := s.machiner.Life(args)
+	c.Assert(err, gc.IsNil)
+	c.Assert(result, gc.DeepEquals, params.LifeResults{
+		Results: []params.LifeResult{
+			{Life: "alive"},
+			{Error: apiservertesting.ErrUnauthorized},
+			{Error: apiservertesting.ErrUnauthorized},
+		},
+	})
+}
+
+func (s *uniterSuite) TestEnsureDeadFails(c *gc.C) {
+	c.Assert(s.machine1.Life(), gc.Equals, state.Alive)
+
+	args := params.Entities{Entities: []params.Entity{
+		{Tag: "machine-0"},
+		{Tag: "machine-1"},
+		{Tag: "machine-42"},
+	}}
+	result, err := s.machiner.EnsureDead(args)
+	c.Assert(err, gc.IsNil)
+	c.Assert(result, gc.DeepEquals, params.ErrorResults{
+		Results: []params.ErrorResult{
+			{apiservertesting.ErrUnauthorized},
+			{apiservertesting.ErrUnauthorized},
+			{apiservertesting.ErrUnauthorized},
+		},
+	})
+
+	err = s.machine1.Refresh()
+	c.Assert(err, gc.IsNil)
+	c.Assert(s.machine1.Life(), gc.Equals, state.Alive)
+}
+
+func (s *uniterSuite) TestSetMachineAddresses(c *gc.C) {
+	c.Assert(s.machine0.Addresses(), gc.HasLen, 0)
+	c.Assert(s.machine1.Addresses(), gc.HasLen, 0)
+
+	addresses := []instance.Address{
+		instance.NewAddress("127.0.0.1"),
+		instance.NewAddress("8.8.8.8"),
+	}
+
+	args := params.SetMachinesAddresses{MachineAddresses: []params.MachineAddresses{
+		{Tag: "machine-0", Addresses: addresses},
+		{Tag: "machine-1", Addresses: addresses},
+		{Tag: "machine-42", Addresses: addresses},
+	}}
+
+	result, err := s.machiner.SetMachineAddresses(args)
+	c.Assert(err, gc.IsNil)
+	c.Assert(result, gc.DeepEquals, params.ErrorResults{
+		Results: []params.ErrorResult{
+			{apiservertesting.ErrUnauthorized},
+			{apiservertesting.ErrUnauthorized},
+			{apiservertesting.ErrUnauthorized},
+		},
+	})
+
+	err = s.machine1.Refresh()
+	c.Assert(err, gc.IsNil)
+	c.Assert(s.machine1.MachineAddresses(), gc.HasLen, 0)
+}
+
+func (s *uniterSuite) TestWatch(c *gc.C) {
+	c.Assert(s.resources.Count(), gc.Equals, 0)
+
+	args := params.Entities{Entities: []params.Entity{
+		{Tag: "machine-1"},
+		{Tag: "machine-0"},
+		{Tag: "machine-42"},
+	}}
+	result, err := s.machiner.Watch(args)
+	c.Assert(err, gc.IsNil)
+	c.Assert(result, gc.DeepEquals, params.NotifyWatchResults{
+		Results: []params.NotifyWatchResult{
+			{NotifyWatcherId: "1"},
+			{Error: apiservertesting.ErrUnauthorized},
+			{Error: apiservertesting.ErrUnauthorized},
+		},
+	})
+
+	// Verify the resource was registered and stop when done
+	c.Assert(s.resources.Count(), gc.Equals, 1)
+	c.Assert(result.Results[0].NotifyWatcherId, gc.Equals, "1")
+	resource := s.resources.Get("1")
+	defer statetesting.AssertStop(c, resource)
+
+	// Check that the Watch has consumed the initial event ("returned" in
+	// the Watch call)
+	wc := statetesting.NewNotifyWatcherC(c, s.State, resource.(state.NotifyWatcher))
+	wc.AssertNoChange()
+}