Merge lp:~thumper/juju-core/provisioner-auth-provider into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Add an AuthenticationProvider interface.

To be used by the provisioner. Very simple implementation now to mirror what
is currently happening. We can add caching and re-getting the information as
we desire.

https://codereview.appspot.com/10478043/

Add an AuthenticationProvider interface.

To be used by the provisioner. Very simple implementation now to mirror what
is currently happening. We can add caching and re-getting the information as
we desire.

https://codereview.appspot.com/10478043/

[Tim Penhey (thumper)]

Reviewers: mp+171006_code.launchpad.net,

Message:
Please take a look.

Description:
Add an AuthenticationProvider interface.

To be used by the provisioner. Very simple implementation now to mirror
what
is currently happening. We can add caching and re-getting the
information as
we desire.

https://code.launchpad.net/~thumper/juju-core/provisioner-auth-provider/+merge/171006

Requires:
https://code.launchpad.net/~thumper/juju-core/lxc-broker/+merge/170713

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   A worker/provisioner/authentication.go
   M worker/provisioner/provisioner.go
   M worker/provisioner/provisioner_task.go

[Tim Penhey (thumper)]

https://codereview.appspot.com/10478043/diff/1/worker/provisioner/authentication.go
File worker/provisioner/authentication.go (right):

https://codereview.appspot.com/10478043/diff/1/worker/provisioner/authentication.go#newcode15
worker/provisioner/authentication.go:15: // AuthenticationProvider
defines the single function that the provisioner
On 2013/06/24 06:30:32, dfc wrote:
> /s/function/method, only types with receivers can fulfil interfaces

Done.

https://codereview.appspot.com/10478043/diff/1/worker/provisioner/authentication.go#newcode27
worker/provisioner/authentication.go:27: return &simpleAuth{stateInfo,
apiInfo}, nil
On 2013/06/24 06:30:32, dfc wrote:
> return &simpleAuth{stateInfo, apiInfo}, err

> In the unlikely case of an error, the heap allocation of the
simpleAuth won't be
> noticable.

I thought it was idiomatic go to make sure that if you have an error,
the other return params are nil or zero initialized.

https://codereview.appspot.com/10478043/diff/1/worker/provisioner/authentication.go#newcode41
worker/provisioner/authentication.go:41: return nil, nil,
fmt.Errorf("cannot set password for machine %v: %v", machine, err)
On 2013/06/24 06:30:32, dfc wrote:
> cannot set mongo password ? there must be a million different
username/password
> combinations, I don't think it hurts to give a hint which is wrong.

Done.

https://codereview.appspot.com/10478043/diff/1/worker/provisioner/authentication.go#newcode43
worker/provisioner/authentication.go:43: stateInfo := *auth.stateInfo
On 2013/06/24 06:30:32, dfc wrote:
> stateInfo := &auth.StateInfo {
> Tag: machine.Tag(),
> Password: password,
> }

> etc

However we are copying some of the fields from stateInfo. This is the
same code that was elsewhere, just moved here.

https://codereview.appspot.com/10478043/

[Ian Booth (wallyworld)]

LGTM. If Dave is happy, I'm happy

https://codereview.appspot.com/10478043/

=== added file 'worker/provisioner/authentication.go'
--- worker/provisioner/authentication.go	1970-01-01 00:00:00 +0000
+++ worker/provisioner/authentication.go	2013-06-25 22:13:27 +0000
@@ -0,0 +1,50 @@
+// Copyright 2012, 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package provisioner
+
+import (
+	"fmt"
+
+	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/api"
+	"launchpad.net/juju-core/utils"
+)
+
+// AuthenticationProvider defines the single method that the provisioner
+// task needs to set up authentication for a machine.
+type AuthenticationProvider interface {
+	SetupAuthentication(*state.Machine) (*state.Info, *api.Info, error)
+}
+
+// NewSimpleAuthenticator gets the state and api info once from the environ.
+func NewSimpleAuthenticator(environ environs.Environ) (AuthenticationProvider, error) {
+	stateInfo, apiInfo, err := environ.StateInfo()
+	if err != nil {
+		return nil, err
+	}
+	return &simpleAuth{stateInfo, apiInfo}, nil
+}
+
+type simpleAuth struct {
+	stateInfo *state.Info
+	apiInfo   *api.Info
+}
+
+func (auth *simpleAuth) SetupAuthentication(machine *state.Machine) (*state.Info, *api.Info, error) {
+	password, err := utils.RandomPassword()
+	if err != nil {
+		return nil, nil, fmt.Errorf("cannot make password for machine %v: %v", machine, err)
+	}
+	if err := machine.SetMongoPassword(password); err != nil {
+		return nil, nil, fmt.Errorf("cannot set mongo password for machine %v: %v", machine, err)
+	}
+	stateInfo := *auth.stateInfo
+	stateInfo.Tag = machine.Tag()
+	stateInfo.Password = password
+	apiInfo := *auth.apiInfo
+	apiInfo.Tag = machine.Tag()
+	apiInfo.Password = password
+	return &stateInfo, &apiInfo, nil
+}

=== modified file 'worker/provisioner/provisioner.go'
--- worker/provisioner/provisioner.go	2013-06-25 22:13:27 +0000
+++ worker/provisioner/provisioner.go	2013-06-25 22:13:27 +0000
@@ -66,10 +66,7 @@
 		return err
 	}
 
-	// Get a new StateInfo from the environment: the one used to
-	// launch the agent may refer to localhost, which will be
-	// unhelpful when attempting to run an agent on a new machine.
-	stateInfo, apiInfo, err := p.environ.StateInfo()
+	auth, err := NewSimpleAuthenticator(p.environ)
 	if err != nil {
 		return err
 	}
@@ -81,12 +78,12 @@
 	machinesWatcher := p.st.WatchEnvironMachines()
 	environmentBroker := newEnvironBroker(p.environ)
 	environmentProvisioner := NewProvisionerTask(
+		"environ provisioner for machine "+p.machineId,
 		p.machineId,
 		p.st,
 		machinesWatcher,
 		environmentBroker,
-		stateInfo,
-		apiInfo)
+		auth)
 	defer watcher.Stop(environmentProvisioner, &p.tomb)
 
 	for {

=== modified file 'worker/provisioner/provisioner_task.go'
--- worker/provisioner/provisioner_task.go	2013-06-25 22:13:27 +0000
+++ worker/provisioner/provisioner_task.go	2013-06-25 22:13:27 +0000
@@ -9,7 +9,6 @@
 	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/state"
-	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/watcher"
 	"launchpad.net/juju-core/utils"
@@ -22,6 +21,7 @@
 	Stop() error
 	Dying() <-chan struct{}
 	Err() error
+	String() string
 }
 
 type Watcher interface {
@@ -35,20 +35,20 @@
 }
 
 func NewProvisionerTask(
+	name string,
 	machineId string,
 	machineGetter MachineGetter,
 	watcher Watcher,
 	broker Broker,
-	stateInfo *state.Info,
-	apiInfo *api.Info,
+	auth AuthenticationProvider,
 ) ProvisionerTask {
 	task := &provisionerTask{
+		name:           name,
 		machineId:      machineId,
 		machineGetter:  machineGetter,
 		machineWatcher: watcher,
 		broker:         broker,
-		stateInfo:      stateInfo,
-		apiInfo:        apiInfo,
+		auth:           auth,
 		machines:       make(map[string]*state.Machine),
 	}
 	go func() {
@@ -59,13 +59,13 @@
 }
 
 type provisionerTask struct {
+	name           string
 	machineId      string
 	machineGetter  MachineGetter
 	machineWatcher Watcher
 	broker         Broker
 	tomb           tomb.Tomb
-	stateInfo      *state.Info
-	apiInfo        *api.Info
+	auth           AuthenticationProvider
 
 	// instance id -> instance
 	instances map[instance.Id]instance.Instance
@@ -96,6 +96,10 @@
 	return task.tomb.Err()
 }
 
+func (task *provisionerTask) String() string {
+	return task.name
+}
+
 func (task *provisionerTask) loop() error {
 	logger.Infof("Starting up provisioner task %s", task.machineId)
 	defer watcher.Stop(task.machineWatcher, &task.tomb)
@@ -300,11 +304,7 @@
 }
 
 func (task *provisionerTask) startMachine(machine *state.Machine) error {
-	// TODO(dfc) the state.Info passed to environ.StartInstance remains contentious
-	// however as the PA only knows one state.Info, and that info is used by MAs and
-	// UAs to locate the state for this environment, it is logical to use the same
-	// state.Info as the PA.
-	stateInfo, apiInfo, err := task.setupAuthentication(machine)
+	stateInfo, apiInfo, err := task.auth.SetupAuthentication(machine)
 	if err != nil {
 		logger.Errorf("failed to setup authentication: %v", err)
 		return err
@@ -353,20 +353,3 @@
 	logger.Infof("started machine %s as instance %s", machine, inst.Id())
 	return nil
 }
-
-func (task *provisionerTask) setupAuthentication(machine *state.Machine) (*state.Info, *api.Info, error) {
-	password, err := utils.RandomPassword()
-	if err != nil {
-		return nil, nil, fmt.Errorf("cannot make password for machine %v: %v", machine, err)
-	}
-	if err := machine.SetMongoPassword(password); err != nil {
-		return nil, nil, fmt.Errorf("cannot set password for machine %v: %v", machine, err)
-	}
-	stateInfo := *task.stateInfo
-	stateInfo.Tag = machine.Tag()
-	stateInfo.Password = password
-	apiInfo := *task.apiInfo
-	apiInfo.Tag = machine.Tag()
-	apiInfo.Password = password
-	return &stateInfo, &apiInfo, nil
-}