state/api(server)/deployer: AssignedMachineTag

state/api(server)/deployer: AssignedMachineTag

Last yet not implemented API call needed by the
deployer worker. Next in line is the actual
migration from state calls to API calls in the
deployer.

https://code.launchpad.net/~dimitern/juju-core/077-deployer-uses-api/+merge/176360

(do not edit description out of merge proposal)

[dimitern, 2013-07-23 11:54:43]

Please take a look.

[mue, 2013-07-23 12:05:16]

LGTM

[rog, 2013-07-23 12:13:45]

good direction, but I think the call semantics could use some refinement.

https://codereview.appspot.com/11713043/diff/1/state/api/deployer/unit.go
File state/api/deployer/unit.go (right):

https://codereview.appspot.com/11713043/diff/1/state/api/deployer/unit.go#new...
state/api/deployer/unit.go:70: // SetPassword changes the unit's password.
s/changes/sets/

it might not change.

https://codereview.appspot.com/11713043/diff/1/state/api/deployer/unit.go#new...
state/api/deployer/unit.go:99: if len(result.Results) > 0 &&
result.Results[0].Error != nil {
this isn't robust if the server returns no results and no error.

if len(result.Results) > 0 {
    return result.Results[0].Tag, result.Results[0].Error.Err()
}
return "", fmt.Errorf("no results found")

?

https://codereview.appspot.com/11713043/diff/1/state/apiserver/deployer/deplo...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/11713043/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:109: func (d *DeployerAPI)
AssignedMachineTag(args params.Entities) (params.EntityResults, error) {
the semantics of this call seem a bit odd to me.
as it is, the returned machine tag contains no information
(it must be the tag of the calling machine agent)

how about something like this, perhaps?

func (d *DeployerAPI) IsResponsibleForUnit(args params.Entities)
(params.BoolResults, error)

for a given unit it would return whether the caller is
responsible for that unit. we then don't have to play
games with ErrPerm meaning "you are not responsible for
this unit".

that would seem to map better to the way the call is
used inside worker/deployer.

https://codereview.appspot.com/11713043/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:114: err := common.ErrPerm
d

https://codereview.appspot.com/11713043/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:115: var unit *state.Unit
d

https://codereview.appspot.com/11713043/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:117: unit, err = d.st.Unit(unitName)
s/=/:=

[dimitern, 2013-07-23 12:46:10]

Please take a look.

https://codereview.appspot.com/11713043/diff/1/state/api/deployer/unit.go
File state/api/deployer/unit.go (right):

https://codereview.appspot.com/11713043/diff/1/state/api/deployer/unit.go#new...
state/api/deployer/unit.go:70: // SetPassword changes the unit's password.
On 2013/07/23 12:13:45, rog wrote:
> s/changes/sets/
> 
> it might not change.

Done.

https://codereview.appspot.com/11713043/diff/1/state/api/deployer/unit.go#new...
state/api/deployer/unit.go:99: if len(result.Results) > 0 &&
result.Results[0].Error != nil {
On 2013/07/23 12:13:45, rog wrote:
> this isn't robust if the server returns no results and no error.
> 
> if len(result.Results) > 0 {
>     return result.Results[0].Tag, result.Results[0].Error.Err()
> }
> return "", fmt.Errorf("no results found")
> 
> ?

As agreed online, will change the code to check for a single result and return
"expected one result, got %d", like the other similar methods.

Will also call this CanDeploy -> BoolResults, error.

https://codereview.appspot.com/11713043/diff/1/state/apiserver/deployer/deplo...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/11713043/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:109: func (d *DeployerAPI)
AssignedMachineTag(args params.Entities) (params.EntityResults, error) {
On 2013/07/23 12:13:45, rog wrote:
> the semantics of this call seem a bit odd to me.
> as it is, the returned machine tag contains no information
> (it must be the tag of the calling machine agent)
> 
> how about something like this, perhaps?
> 
> func (d *DeployerAPI) IsResponsibleForUnit(args params.Entities)
> (params.BoolResults, error)
> 
> for a given unit it would return whether the caller is
> responsible for that unit. we then don't have to play
> games with ErrPerm meaning "you are not responsible for
> this unit".
> 
> that would seem to map better to the way the call is
> used inside worker/deployer.

Good idea! Actually that was the original intent. Will do.

[rog, 2013-07-23 14:10:11]

a couple more comments

https://codereview.appspot.com/11713043/diff/8001/state/api/deployer/unit.go
File state/api/deployer/unit.go (right):

https://codereview.appspot.com/11713043/diff/8001/state/api/deployer/unit.go#...
state/api/deployer/unit.go:89: // CanDeploy returns is the currently
authenticated entity (a machine
s/returns is/reports whether/

https://codereview.appspot.com/11713043/diff/8001/state/apiserver/deployer/de...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/11713043/diff/8001/state/apiserver/deployer/de...
state/apiserver/deployer/deployer.go:118: if err == nil {
if err == nil {
     result.Results[i].Result =
d.authorizer.AuthOwner(state.MachineTag(machineId))
}

has less conditional logic to follow.

https://codereview.appspot.com/11713043/diff/8001/state/apiserver/deployer/de...
state/apiserver/deployer/deployer.go:127: if !result.Results[i].Result {
i think just returning false is fine here. it seems odd to get a
permission-denied error when asking about a unit that you've just been told
about.

if there's a security flaw here, it's that any machine agent can use this call
to probe whether a given unit exists.

as discussed online, i suggest that we return an error if we get an
non-not-found error from Unit; otherwise return true iff the assigned machine
matches.

[dimitern, 2013-07-23 15:48:00]

Please take a look.

[dimitern, 2013-07-23 15:50:29]

Please take a look.

https://codereview.appspot.com/11713043/diff/8001/state/api/deployer/unit.go
File state/api/deployer/unit.go (right):

https://codereview.appspot.com/11713043/diff/8001/state/api/deployer/unit.go#...
state/api/deployer/unit.go:89: // CanDeploy returns is the currently
authenticated entity (a machine
On 2013/07/23 14:10:12, rog wrote:
> s/returns is/reports whether/

Done.

https://codereview.appspot.com/11713043/diff/8001/state/apiserver/deployer/de...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/11713043/diff/8001/state/apiserver/deployer/de...
state/apiserver/deployer/deployer.go:118: if err == nil {
On 2013/07/23 14:10:12, rog wrote:
> if err == nil {
>      result.Results[i].Result =
> d.authorizer.AuthOwner(state.MachineTag(machineId))
> }
> 
> has less conditional logic to follow.

Done as discussed.

https://codereview.appspot.com/11713043/diff/8001/state/apiserver/deployer/de...
state/apiserver/deployer/deployer.go:127: if !result.Results[i].Result {
On 2013/07/23 14:10:12, rog wrote:
> i think just returning false is fine here. it seems odd to get a
> permission-denied error when asking about a unit that you've just been told
> about.
> 
> if there's a security flaw here, it's that any machine agent can use this call
> to probe whether a given unit exists.
> 
> as discussed online, i suggest that we return an error if we get an
> non-not-found error from Unit; otherwise return true iff the assigned machine
> matches.

As discussed.

[rog, 2013-07-23 15:57:12]

LGTM with a couple of trivial suggestions below

https://codereview.appspot.com/11713043/diff/18001/state/api/deployer/unit.go
File state/api/deployer/unit.go (right):

https://codereview.appspot.com/11713043/diff/18001/state/api/deployer/unit.go...
state/api/deployer/unit.go:102: }
possible suggestion:

return result.Results[0].Result, result.Results[0].Error.Err()

https://codereview.appspot.com/11713043/diff/18001/state/apiserver/deployer/d...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/11713043/diff/18001/state/apiserver/deployer/d...
state/apiserver/deployer/deployer.go:132: // This means we got some other error
from state.
// This means the unit wasn't assigned to the machine agent
// or it wasn't found. In both cases we just return false so
// as not to leak information about the existence of a unit
// to a potentially rogue machine agent.

?

[dimitern, 2013-07-23 16:11:30]

Please take a look.

https://codereview.appspot.com/11713043/diff/18001/state/api/deployer/unit.go
File state/api/deployer/unit.go (right):

https://codereview.appspot.com/11713043/diff/18001/state/api/deployer/unit.go...
state/api/deployer/unit.go:102: }
On 2013/07/23 15:57:12, rog wrote:
> possible suggestion:
> 
> return result.Results[0].Result, result.Results[0].Error.Err()

params.Error doesn't have Err() method (yet). Will add in a separate CL, after I
finish the deployer API migration.

https://codereview.appspot.com/11713043/diff/18001/state/apiserver/deployer/d...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/11713043/diff/18001/state/apiserver/deployer/d...
state/apiserver/deployer/deployer.go:132: // This means we got some other error
from state.
On 2013/07/23 15:57:12, rog wrote:
> // This means the unit wasn't assigned to the machine agent
> // or it wasn't found. In both cases we just return false so
> // as not to leak information about the existence of a unit
> // to a potentially rogue machine agent.
> 
> ?

Done.