Merge lp:~dimitern/juju-core/146-apiprovisioner-addresses into lp:~go-bot/juju-core/trunk

[Dimiter Naydenov]

api/provisioner: Addresses/CACert methods added

Like the deployer API, the provisioner API also
needs to provider StateAddresses(), APIAddresses(),
and CACert(), so we can use the up-to-date (not
localhost) addresses in NewAPIAuthenticator().

This is needed, because the addresses we get
from agent config are incorrect (use localhost),
and without this CL neither containers nor
machines can't connect to state/API server
at provisioning.

And because the same code is already in deployer,
I followed a suggestion to factor it out into
apiserver/common/addresses.

https://codereview.appspot.com/13963043/

R=fwereade

api/provisioner: Addresses/CACert methods added

Like the deployer API, the provisioner API also
needs to provider StateAddresses(), APIAddresses(),
and CACert(), so we can use the up-to-date (not
localhost) addresses in NewAPIAuthenticator().

This is needed, because the addresses we get
from agent config are incorrect (use localhost),
and without this CL neither containers nor
machines can't connect to state/API server
at provisioning.

And because the same code is already in deployer,
I followed a suggestion to factor it out into
apiserver/common/addresses.

https://codereview.appspot.com/13963043/

[Dimiter Naydenov (dimitern)]

Reviewers: mp+187719_code.launchpad.net,

Message:
Please take a look.

Description:
api/provisioner: Addresses/CACert methods added

Like the deployer API, the provisioner API also
needs to provider StateAddresses(), APIAddresses(),
and CACert(), so we can use the up-to-date (not
localhost) addresses in NewAPIAuthenticator().

This is needed, because the addresses we get
from agent config are incorrect (use localhost),
and without this CL neither containers nor
machines can't connect to state/API server
at provisioning.

https://code.launchpad.net/~dimitern/juju-core/146-apiprovisioner-addresses/+merge/187719

(do not edit description out of merge proposal)

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

Affected files (+167, -11 lines):
   A [revision details]
   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
   M worker/provisioner/authentication.go
   M worker/provisioner/provisioner.go

[William Reade (fwereade)]

If the copy/paste is necessary to get this in time for 1.15, LGTM with
an immediate followup to deduplicate. Much rather see it in this branch
though.

https://codereview.appspot.com/13963043/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/13963043/

[William Reade (fwereade)]

LGTM, thanks.

https://codereview.appspot.com/13963043/diff/6001/state/apiserver/common/addresses.go
File state/apiserver/common/addresses.go (right):

https://codereview.appspot.com/13963043/diff/6001/state/apiserver/common/addresses.go#newcode21
state/apiserver/common/addresses.go:21: // PasswordChanger implements a
common set of methods for getting
PasswordChanger? ;p

https://codereview.appspot.com/13963043/diff/6001/state/apiserver/common/addresses.go#newcode22
state/apiserver/common/addresses.go:22: // state and API addresses, as
well as the CA certificated, used to
"state and API server addresses, and the CA certificate used to
authenticate them."?

https://codereview.appspot.com/13963043/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/13963043/diff/6001/state/apiserver/common/addresses.go
File state/apiserver/common/addresses.go (right):

https://codereview.appspot.com/13963043/diff/6001/state/apiserver/common/addresses.go#newcode21
state/apiserver/common/addresses.go:21: // PasswordChanger implements a
common set of methods for getting
On 2013/09/26 13:28:35, fwereade wrote:
> PasswordChanger? ;p

oops :) Fixed.

https://codereview.appspot.com/13963043/diff/6001/state/apiserver/common/addresses.go#newcode22
state/apiserver/common/addresses.go:22: // state and API addresses, as
well as the CA certificated, used to
On 2013/09/26 13:28:35, fwereade wrote:
> "state and API server addresses, and the CA certificate used to
authenticate
> them."?

Done.

https://codereview.appspot.com/13963043/

=== modified file 'agent/agent.go'
--- agent/agent.go	2013-09-25 16:18:20 +0000
+++ agent/agent.go	2013-09-26 13:42:07 +0000
@@ -87,12 +87,6 @@
 	// api connections.
 	PasswordHash() string
 
-	// StateAddresses returns all known state server addresses.
-	StateAddresses() []string
-
-	// APIAddresses returns all known API server addresses.
-	APIAddresses() []string
-
 	// APIServerDetails returns the details needed to run an API server.
 	APIServerDetails() (port int, cert, key []byte)
 
@@ -311,18 +305,6 @@
 	}
 }
 
-func (c *configInternal) StateAddresses() []string {
-	addresses := make([]string, len(c.stateDetails.addresses))
-	copy(addresses, c.stateDetails.addresses)
-	return addresses
-}
-
-func (c *configInternal) APIAddresses() []string {
-	addresses := make([]string, len(c.apiDetails.addresses))
-	copy(addresses, c.apiDetails.addresses)
-	return addresses
-}
-
 func (c *configInternal) APIServerDetails() (port int, cert, key []byte) {
 	return c.apiPort, c.stateServerCert, c.stateServerKey
 }

=== modified file 'state/api/provisioner/provisioner.go'
--- state/api/provisioner/provisioner.go	2013-09-19 12:10:31 +0000
+++ state/api/provisioner/provisioner.go	2013-09-26 13:42:07 +0000
@@ -101,3 +101,33 @@
 	w := watcher.NewStringsWatcher(st.caller, result)
 	return w, nil
 }
+
+// StateAddresses returns the list of addresses used to connect to the state.
+func (st *State) StateAddresses() ([]string, error) {
+	var result params.StringsResult
+	err := st.caller.Call("Provisioner", "", "StateAddresses", nil, &result)
+	if err != nil {
+		return nil, err
+	}
+	return result.Result, nil
+}
+
+// APIAddresses returns the list of addresses used to connect to the API.
+func (st *State) APIAddresses() ([]string, error) {
+	var result params.StringsResult
+	err := st.caller.Call("Provisioner", "", "APIAddresses", nil, &result)
+	if err != nil {
+		return nil, err
+	}
+	return result.Result, nil
+}
+
+// CACert returns the certificate used to validate the state connection.
+func (st *State) CACert() ([]byte, error) {
+	var result params.BytesResult
+	err := st.caller.Call("Provisioner", "", "CACert", nil, &result)
+	if err != nil {
+		return nil, err
+	}
+	return result.Result, nil
+}

=== modified file 'state/api/provisioner/provisioner_test.go'
--- state/api/provisioner/provisioner_test.go	2013-09-19 12:10:31 +0000
+++ state/api/provisioner/provisioner_test.go	2013-09-26 13:42:07 +0000
@@ -337,3 +337,26 @@
 	statetesting.AssertStop(c, w)
 	wc.AssertClosed()
 }
+
+func (s *provisionerSuite) TestStateAddresses(c *gc.C) {
+	stateAddresses, err := s.State.Addresses()
+	c.Assert(err, gc.IsNil)
+
+	addresses, err := s.provisioner.StateAddresses()
+	c.Assert(err, gc.IsNil)
+	c.Assert(addresses, gc.DeepEquals, stateAddresses)
+}
+
+func (s *provisionerSuite) TestAPIAddresses(c *gc.C) {
+	apiInfo := s.APIInfo(c)
+
+	addresses, err := s.provisioner.APIAddresses()
+	c.Assert(err, gc.IsNil)
+	c.Assert(addresses, gc.DeepEquals, apiInfo.Addrs)
+}
+
+func (s *provisionerSuite) TestCACert(c *gc.C) {
+	caCert, err := s.provisioner.CACert()
+	c.Assert(err, gc.IsNil)
+	c.Assert(caCert, gc.DeepEquals, s.State.CACert())
+}

=== added file 'state/apiserver/common/addresses.go'
--- state/apiserver/common/addresses.go	1970-01-01 00:00:00 +0000
+++ state/apiserver/common/addresses.go	2013-09-26 13:42:07 +0000
@@ -0,0 +1,92 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package common
+
+import (
+	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/api"
+	"launchpad.net/juju-core/state/api/params"
+)
+
+// EnvironConfigAndCertGetter defines EnvironConfig and CACert
+// methods.
+type EnvironConfigAndCertGetter interface {
+	EnvironConfig() (*config.Config, error)
+	CACert() []byte
+}
+
+// Addresser implements a common set of methods for getting state and
+// API server addresses, and the CA certificate used to authenticate
+// them.
+type Addresser struct {
+	st EnvironConfigAndCertGetter
+}
+
+// NewAddresser returns a new Addresser.
+func NewAddresser(st EnvironConfigAndCertGetter) *Addresser {
+	return &Addresser{st}
+}
+
+// getEnvironStateInfo returns the state and API connection
+// information from the state and the environment.
+//
+// TODO(dimitern): Remove this once we have a way to get state/API
+// public addresses from state.
+// BUG(lp:1205371): This is temporary, until the Addresser worker
+// lands and we can take the addresses of all machines with
+// JobManageState.
+func (a *Addresser) getEnvironStateInfo() (*state.Info, *api.Info, error) {
+	cfg, err := a.st.EnvironConfig()
+	if err != nil {
+		return nil, nil, err
+	}
+	env, err := environs.New(cfg)
+	if err != nil {
+		return nil, nil, err
+	}
+	return env.StateInfo()
+}
+
+// StateAddresses returns the list of addresses used to connect to the state.
+//
+// TODO(dimitern): Remove this once we have a way to get state/API
+// public addresses from state.
+// BUG(lp:1205371): This is temporary, until the Addresser worker
+// lands and we can take the addresses of all machines with
+// JobManageState.
+func (a *Addresser) StateAddresses() (params.StringsResult, error) {
+	stateInfo, _, err := a.getEnvironStateInfo()
+	if err != nil {
+		return params.StringsResult{}, err
+	}
+	return params.StringsResult{
+		Result: stateInfo.Addrs,
+	}, nil
+}
+
+// APIAddresses returns the list of addresses used to connect to the API.
+//
+// TODO(dimitern): Remove this once we have a way to get state/API
+// public addresses from state.
+// BUG(lp:1205371): This is temporary, until the Addresser worker
+// lands and we can take the addresses of all machines with
+// JobManageState.
+func (a *Addresser) APIAddresses() (params.StringsResult, error) {
+	_, apiInfo, err := a.getEnvironStateInfo()
+	if err != nil {
+		return params.StringsResult{}, err
+	}
+	return params.StringsResult{
+		Result: apiInfo.Addrs,
+	}, nil
+}
+
+// CACert returns the certificate used to validate the state connection.
+func (a *Addresser) CACert() params.BytesResult {
+	return params.BytesResult{
+		Result: a.st.CACert(),
+	}
+}

=== added file 'state/apiserver/common/addresses_test.go'
--- state/apiserver/common/addresses_test.go	1970-01-01 00:00:00 +0000
+++ state/apiserver/common/addresses_test.go	2013-09-26 13:42:07 +0000
@@ -0,0 +1,45 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package common_test
+
+import (
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/juju/testing"
+	"launchpad.net/juju-core/state/apiserver/common"
+)
+
+type addresserSuite struct {
+	testing.JujuConnSuite
+	addresser *common.Addresser
+}
+
+var _ = gc.Suite(&addresserSuite{})
+
+func (s *addresserSuite) SetUpTest(c *gc.C) {
+	s.JujuConnSuite.SetUpTest(c)
+	s.addresser = common.NewAddresser(s.State)
+}
+
+func (s *addresserSuite) TestStateAddresses(c *gc.C) {
+	stateAddresses, err := s.State.Addresses()
+	c.Assert(err, gc.IsNil)
+
+	result, err := s.addresser.StateAddresses()
+	c.Assert(err, gc.IsNil)
+	c.Assert(result.Result, gc.DeepEquals, stateAddresses)
+}
+
+func (s *addresserSuite) TestAPIAddresses(c *gc.C) {
+	apiInfo := s.APIInfo(c)
+
+	result, err := s.addresser.APIAddresses()
+	c.Assert(err, gc.IsNil)
+	c.Assert(result.Result, gc.DeepEquals, apiInfo.Addrs)
+}
+
+func (s *addresserSuite) TestCACert(c *gc.C) {
+	result := s.addresser.CACert()
+	c.Assert(result.Result, gc.DeepEquals, s.State.CACert())
+}

=== modified file 'state/apiserver/common/common_test.go'
--- state/apiserver/common/common_test.go	2013-08-16 15:25:46 +0000
+++ state/apiserver/common/common_test.go	2013-09-26 13:42:07 +0000
@@ -10,10 +10,11 @@
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/state/apiserver/common"
+	coretesting "launchpad.net/juju-core/testing"
 )
 
 func TestAll(t *stdtesting.T) {
-	gc.TestingT(t)
+	coretesting.MgoTestPackage(t)
 }
 
 type commonSuite struct{}

=== modified file 'state/apiserver/deployer/deployer.go'
--- state/apiserver/deployer/deployer.go	2013-09-17 12:17:13 +0000
+++ state/apiserver/deployer/deployer.go	2013-09-26 13:42:07 +0000
@@ -6,10 +6,8 @@
 import (
 	"fmt"
 
-	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state"
-	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/apiserver/common"
 	"launchpad.net/juju-core/state/watcher"
@@ -20,6 +18,7 @@
 	*common.Remover
 	*common.PasswordChanger
 	*common.LifeGetter
+	*common.Addresser
 
 	st         *state.State
 	resources  *common.Resources
@@ -76,6 +75,7 @@
 		Remover:         common.NewRemover(st, true, getAuthFunc),
 		PasswordChanger: common.NewPasswordChanger(st, getAuthFunc),
 		LifeGetter:      common.NewLifeGetter(st, getAuthFunc),
+		Addresser:       common.NewAddresser(st),
 		st:              st,
 		resources:       resources,
 		authorizer:      authorizer,
@@ -120,64 +120,3 @@
 	}
 	return result, nil
 }
-
-// getEnvironStateInfo returns the state and API connection
-// information from the state and the environment.
-//
-// TODO(dimitern): Remove this once we have a way to get state/API
-// public addresses from state.
-// BUG(lp:1205371): This is temporary, until the Addresser worker
-// lands and we can take the addresses of all machines with
-// JobManageState.
-func (d *DeployerAPI) getEnvironStateInfo() (*state.Info, *api.Info, error) {
-	cfg, err := d.st.EnvironConfig()
-	if err != nil {
-		return nil, nil, err
-	}
-	env, err := environs.New(cfg)
-	if err != nil {
-		return nil, nil, err
-	}
-	return env.StateInfo()
-}
-
-// StateAddresses returns the list of addresses used to connect to the state.
-//
-// TODO(dimitern): Remove this once we have a way to get state/API
-// public addresses from state.
-// BUG(lp:1205371): This is temporary, until the Addresser worker
-// lands and we can take the addresses of all machines with
-// JobManageState.
-func (d *DeployerAPI) StateAddresses() (params.StringsResult, error) {
-	stateInfo, _, err := d.getEnvironStateInfo()
-	if err != nil {
-		return params.StringsResult{}, err
-	}
-	return params.StringsResult{
-		Result: stateInfo.Addrs,
-	}, nil
-}
-
-// APIAddresses returns the list of addresses used to connect to the API.
-//
-// TODO(dimitern): Remove this once we have a way to get state/API
-// public addresses from state.
-// BUG(lp:1205371): This is temporary, until the Addresser worker
-// lands and we can take the addresses of all machines with
-// JobManageState.
-func (d *DeployerAPI) APIAddresses() (params.StringsResult, error) {
-	_, apiInfo, err := d.getEnvironStateInfo()
-	if err != nil {
-		return params.StringsResult{}, err
-	}
-	return params.StringsResult{
-		Result: apiInfo.Addrs,
-	}, nil
-}
-
-// CACert returns the certificate used to validate the state connection.
-func (d *DeployerAPI) CACert() params.BytesResult {
-	return params.BytesResult{
-		Result: d.st.CACert(),
-	}
-}

=== modified file 'state/apiserver/provisioner/provisioner.go'
--- state/apiserver/provisioner/provisioner.go	2013-09-19 14:25:23 +0000
+++ state/apiserver/provisioner/provisioner.go	2013-09-26 13:42:07 +0000
@@ -22,6 +22,7 @@
 	*common.DeadEnsurer
 	*common.PasswordChanger
 	*common.LifeGetter
+	*common.Addresser
 
 	st          *state.State
 	resources   *common.Resources
@@ -69,6 +70,7 @@
 		DeadEnsurer:     common.NewDeadEnsurer(st, getAuthFunc),
 		PasswordChanger: common.NewPasswordChanger(st, getAuthFunc),
 		LifeGetter:      common.NewLifeGetter(st, getAuthFunc),
+		Addresser:       common.NewAddresser(st),
 		st:              st,
 		resources:       resources,
 		authorizer:      authorizer,

=== modified file 'state/apiserver/provisioner/provisioner_test.go'
--- state/apiserver/provisioner/provisioner_test.go	2013-09-24 08:55:01 +0000
+++ state/apiserver/provisioner/provisioner_test.go	2013-09-26 13:42:07 +0000
@@ -621,3 +621,31 @@
 	c.Assert(err, gc.ErrorMatches, "permission denied")
 	c.Assert(result, gc.DeepEquals, params.StringsWatchResult{})
 }
+
+func (s *provisionerSuite) TestStateAddresses(c *gc.C) {
+	addresses, err := s.State.Addresses()
+	c.Assert(err, gc.IsNil)
+
+	result, err := s.provisioner.StateAddresses()
+	c.Assert(err, gc.IsNil)
+	c.Assert(result, gc.DeepEquals, params.StringsResult{
+		Result: addresses,
+	})
+}
+
+func (s *provisionerSuite) TestAPIAddresses(c *gc.C) {
+	apiInfo := s.APIInfo(c)
+
+	result, err := s.provisioner.APIAddresses()
+	c.Assert(err, gc.IsNil)
+	c.Assert(result, gc.DeepEquals, params.StringsResult{
+		Result: apiInfo.Addrs,
+	})
+}
+
+func (s *provisionerSuite) TestCACert(c *gc.C) {
+	result := s.provisioner.CACert()
+	c.Assert(result, gc.DeepEquals, params.BytesResult{
+		Result: s.State.CACert(),
+	})
+}

=== modified file 'worker/provisioner/authentication.go'
--- worker/provisioner/authentication.go	2013-09-25 16:18:20 +0000
+++ worker/provisioner/authentication.go	2013-09-26 13:42:07 +0000
@@ -6,10 +6,10 @@
 import (
 	"fmt"
 
-	"launchpad.net/juju-core/agent"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
+	apiprovisioner "launchpad.net/juju-core/state/api/provisioner"
 	"launchpad.net/juju-core/utils"
 )
 
@@ -35,18 +35,28 @@
 	return &simpleAuth{stateInfo, apiInfo}, nil
 }
 
-// NewAgentConfigAuthenticator gets the state and api info once from
-// the agent configuration.
-func NewAgentConfigAuthenticator(agentConfig agent.Config) (AuthenticationProvider, error) {
-	// TODO(dimitern) Take these from the API, like the deployer does,
-	// so we'll always have up-to-date addresses.
+// NewAPIAuthenticator gets the state and api info once from the
+// provisioner API.
+func NewAPIAuthenticator(st *apiprovisioner.State) (AuthenticationProvider, error) {
+	stateAddresses, err := st.StateAddresses()
+	if err != nil {
+		return nil, err
+	}
+	apiAddresses, err := st.APIAddresses()
+	if err != nil {
+		return nil, err
+	}
+	caCert, err := st.CACert()
+	if err != nil {
+		return nil, err
+	}
 	stateInfo := &state.Info{
-		Addrs:  agentConfig.StateAddresses(),
-		CACert: agentConfig.CACert(),
+		Addrs:  stateAddresses,
+		CACert: caCert,
 	}
 	apiInfo := &api.Info{
-		Addrs:  agentConfig.APIAddresses(),
-		CACert: agentConfig.CACert(),
+		Addrs:  apiAddresses,
+		CACert: caCert,
 	}
 	return &simpleAuth{stateInfo, apiInfo}, nil
 }

=== modified file 'worker/provisioner/provisioner.go'
--- worker/provisioner/provisioner.go	2013-09-25 16:18:20 +0000
+++ worker/provisioner/provisioner.go	2013-09-26 13:42:07 +0000
@@ -99,7 +99,7 @@
 		environConfigChanges = nil
 	}
 
-	auth, err := NewAgentConfigAuthenticator(p.agentConfig)
+	auth, err := NewAPIAuthenticator(p.st)
 	if err != nil {
 		return err
 	}