Firewaller using APIV1 when available, otherwise uses APIV0 and legac…

…y code
zhatin · Sep 24, 2014 · 2179cef · 2179cef
1 parent 9ba3408
commit 2179cef
Show file tree

Hide file tree

Showing 22 changed files with 2,023 additions and 325 deletions.
diff --git a/api/apiclient.go b/api/apiclient.go
@@ -325,10 +325,9 @@ func (s *State) Addr() string {
 	return s.addr
 }
 
-// EnvironTag returns the Environment Tag describing the environment we are
-// connected to.
-func (s *State) EnvironTag() string {
-	return s.environTag
+// EnvironTag returns the tag of the environment we are connected to.
+func (s *State) EnvironTag() (names.EnvironTag, error) {
+	return names.ParseEnvironTag(s.environTag)
 }
 
 // APIHostPorts returns addresses that may be used to connect

diff --git a/api/base/caller.go b/api/base/caller.go
@@ -3,6 +3,10 @@
 
 package base
 
+import (
+	"github.com/juju/names"
+)
+
 // APICaller is implemented by the client-facing State object.
 type APICaller interface {
 	// APICall makes a call to the API server with the given object type,
@@ -13,6 +17,10 @@ type APICaller interface {
 	// BestFacadeVersion returns the newest version of 'objType' that this
 	// client can use with the current API server.
 	BestFacadeVersion(facade string) int
+
+	// EnvironTag returns the tag of the environment the client is
+	// connected to.
+	EnvironTag() (names.EnvironTag, error)
 }
 
 // FacadeCaller is a wrapper for the common paradigm that a given client just

diff --git a/api/client.go b/api/client.go
@@ -492,16 +492,12 @@ func (c *Client) EnvironmentInfo() (*EnvironmentInfo, error) {
 
 // EnvironmentUUID returns the environment UUID from the client connection.
 func (c *Client) EnvironmentUUID() string {
-	value := c.st.EnvironTag()
-	if value != "" {
-		tag, err := names.ParseEnvironTag(value)
-		if err != nil {
-			logger.Warningf("environ tag not an environ: %v", err)
-			return ""
-		}
-		return tag.Id()
+	tag, err := c.st.EnvironTag()
+	if err != nil {
+		logger.Warningf("environ tag not an environ: %v", err)
+		return ""
 	}
-	return ""
+	return tag.Id()
 }
 
 // ShareEnvironment allows the given users access to the environment.
@@ -817,14 +813,18 @@ func (c *Client) APIHostPorts() ([][]network.HostPort, error) {
 // This API is now on the HighAvailability facade.
 func (c *Client) EnsureAvailability(numStateServers int, cons constraints.Value, series string) (params.StateServersChanges, error) {
 	var results params.StateServersChangeResults
+	envTag, err := c.st.EnvironTag()
+	if err != nil {
+		return params.StateServersChanges{}, errors.Trace(err)
+	}
 	arg := params.StateServersSpecs{
 		Specs: []params.StateServersSpec{{
-			EnvironTag:      c.st.EnvironTag(),
+			EnvironTag:      envTag.String(),
 			NumStateServers: numStateServers,
 			Constraints:     cons,
 			Series:          series,
 		}}}
-	err := c.facade.FacadeCall("EnsureAvailability", arg, &results)
+	err = c.facade.FacadeCall("EnsureAvailability", arg, &results)
 	if err != nil {
 		return params.StateServersChanges{}, err
 	}

diff --git a/api/firewaller/firewaller.go b/api/firewaller/firewaller.go
@@ -21,48 +21,55 @@ type State struct {
 	*common.EnvironWatcher
 }
 
-// newStateV0 creates a new client-side Firewaller facade, version 0.
-func newStateV0(caller base.APICaller) *State {
-	facadeCaller := base.NewFacadeCallerForVersion(caller, firewallerFacade, 0)
+// newStateForVersion creates a new client-side Firewaller API facade
+// for the given version. If version is -1, the best facade version
+// among the ones supported by both the client and the server is
+// chosen.
+func newStateForVersion(caller base.APICaller, version int) *State {
+	var facadeCaller base.FacadeCaller
+	if version == -1 {
+		facadeCaller = base.NewFacadeCaller(caller, firewallerFacade)
+	} else {
+		facadeCaller = base.NewFacadeCallerForVersion(caller, firewallerFacade, version)
+	}
 	return &State{
 		facade:         facadeCaller,
 		EnvironWatcher: common.NewEnvironWatcher(facadeCaller),
 	}
 }
 
-// newStateV0 creates a new client-side Firewaller facade, version 1.
+// newStateV0 creates a new client-side Firewaller facade, version 0.
+func newStateV0(caller base.APICaller) *State {
+	return newStateForVersion(caller, 0)
+}
+
+// newStateV1 creates a new client-side Firewaller facade, version 1.
 func newStateV1(caller base.APICaller) *State {
-	facadeCaller := base.NewFacadeCallerForVersion(caller, firewallerFacade, 1)
-	return &State{
-		facade:         facadeCaller,
-		EnvironWatcher: common.NewEnvironWatcher(facadeCaller),
-	}
+	return newStateForVersion(caller, 1)
 }
 
 // newStateBestVersion creates a new client-side Firewaller facade
 // with the best API version supported by both the client and the
 // server.
-//
-// TODO(dimitern) Once the firewaller worker uses V1, make this
-// the default constructor.
 func newStateBestVersion(caller base.APICaller) *State {
-	facadeCaller := base.NewFacadeCaller(caller, firewallerFacade)
-	return &State{
-		facade:         facadeCaller,
-		EnvironWatcher: common.NewEnvironWatcher(facadeCaller),
-	}
+	return newStateForVersion(caller, -1)
 }
 
 // NewState creates a new client-side Firewaller facade.
 // Defined like this to allow patching during tests.
-var NewState = newStateV0
+var NewState = newStateBestVersion
 
 // BestAPIVersion returns the API version that we were able to
 // determine is supported by both the client and the API Server.
 func (st *State) BestAPIVersion() int {
 	return st.facade.BestAPIVersion()
 }
 
+// EnvironTag returns the current environment's tag.
+func (st *State) EnvironTag() (names.EnvironTag, error) {
+	return st.facade.RawAPICaller().EnvironTag()
+}
+
 // life requests the life cycle of the given entity from the server.
 func (st *State) life(tag names.Tag) (params.Life, error) {
 	return common.Life(st.facade, tag)
@@ -112,17 +119,21 @@ func (st *State) WatchEnvironMachines() (watcher.StringsWatcher, error) {
 }
 
 // WatchOpenedPorts returns a StringsWatcher that notifies of
-// changes to the opened ports for the given environment tag.
-func (st *State) WatchOpenedPorts(tag names.EnvironTag) (watcher.StringsWatcher, error) {
+// changes to the opened ports for the current environment.
+func (st *State) WatchOpenedPorts() (watcher.StringsWatcher, error) {
 	if st.BestAPIVersion() < 1 {
 		// WatchOpenedPorts() was introduced in FirewallerAPIV1.
 		return nil, errors.NotImplementedf("WatchOpenedPorts() (need V1+)")
 	}
+	envTag, err := st.EnvironTag()
+	if err != nil {
+		return nil, errors.Annotatef(err, "invalid environ tag")
+	}
 	var results params.StringsWatchResults
 	args := params.Entities{
-		Entities: []params.Entity{{Tag: tag.String()}},
+		Entities: []params.Entity{{Tag: envTag.String()}},
 	}
-	err := st.facade.FacadeCall("WatchOpenedPorts", args, &results)
+	err = st.facade.FacadeCall("WatchOpenedPorts", args, &results)
 	if err != nil {
 		return nil, err
 	}

diff --git a/api/firewaller/state_test.go b/api/firewaller/state_test.go
@@ -5,7 +5,6 @@ package firewaller_test
 
 import (
 	"github.com/juju/errors"
-	"github.com/juju/names"
 	jc "github.com/juju/testing/checkers"
 	gc "launchpad.net/gocheck"
 
@@ -67,7 +66,7 @@ func (s *stateSuite) TestWatchEnvironMachines(c *gc.C) {
 func (s *stateSuite) TestWatchOpenedPortsNotImplementedV0(c *gc.C) {
 	s.patchNewState(c, firewaller.NewStateV0)
 
-	w, err := s.firewaller.WatchOpenedPorts(s.APIInfo(c).EnvironTag.(names.EnvironTag))
+	w, err := s.firewaller.WatchOpenedPorts()
 	c.Assert(err, jc.Satisfies, errors.IsNotImplemented)
 	c.Assert(err, gc.ErrorMatches, `WatchOpenedPorts\(\) \(need V1\+\) not implemented`)
 	c.Assert(w, gc.IsNil)
@@ -82,7 +81,7 @@ func (s *stateSuite) TestWatchOpenedPortsV1(c *gc.C) {
 	err = s.units[2].OpenPort("udp", 4321)
 	c.Assert(err, gc.IsNil)
 
-	w, err := s.firewaller.WatchOpenedPorts(s.APIInfo(c).EnvironTag.(names.EnvironTag))
+	w, err := s.firewaller.WatchOpenedPorts()
 	c.Assert(err, gc.IsNil)
 	defer statetesting.AssertStop(c, w)
 	wc := statetesting.NewStringsWatcherC(c, s.BackingState, w)

diff --git a/api/firewaller/unit_test.go b/api/firewaller/unit_test.go
@@ -63,6 +63,8 @@ func (s *unitSuite) TestRefresh(c *gc.C) {
 }
 
 func (s *unitSuite) TestWatchV0(c *gc.C) {
+	s.patchNewState(c, firewaller.NewStateV0)
+
 	c.Assert(s.apiUnit.Life(), gc.Equals, params.Alive)
 
 	w, err := s.apiUnit.Watch()

diff --git a/api/metricsmanager/client.go b/api/metricsmanager/client.go
@@ -37,11 +37,15 @@ func NewClient(st *api.State) *Client {
 // CleanupOldMetrics looks for metrics that are 24 hours old (or older)
 // and have been sent. Any metrics it finds are deleted.
 func (c *Client) CleanupOldMetrics() error {
+	envTag, err := c.st.EnvironTag()
+	if err != nil {
+		return errors.Trace(err)
+	}
 	p := params.Entities{Entities: []params.Entity{
-		{c.st.EnvironTag()},
+		{envTag.String()},
 	}}
 	results := new(params.ErrorResults)
-	err := c.facade.FacadeCall("CleanupOldMetrics", p, results)
+	err = c.facade.FacadeCall("CleanupOldMetrics", p, results)
 	if err != nil {
 		return errors.Trace(err)
 	}
@@ -50,11 +54,15 @@ func (c *Client) CleanupOldMetrics() error {
 
 // SendMetrics will send any unsent metrics to the collection service.
 func (c *Client) SendMetrics() error {
+	envTag, err := c.st.EnvironTag()
+	if err != nil {
+		return errors.Trace(err)
+	}
 	p := params.Entities{Entities: []params.Entity{
-		{c.st.EnvironTag()},
+		{envTag.String()},
 	}}
 	results := new(params.ErrorResults)
-	err := c.facade.FacadeCall("SendMetrics", p, results)
+	err = c.facade.FacadeCall("SendMetrics", p, results)
 	if err != nil {
 		return errors.Trace(err)
 	}

diff --git a/api/state.go b/api/state.go
@@ -135,7 +135,14 @@ func (st *State) Provisioner() *provisioner.State {
 func (st *State) Uniter() *uniter.State {
 	// TODO(dfc) yes, this can panic, we never checked before
 	unitTag := st.authTag.(names.UnitTag)
-	charmsURL := uniter.CharmsURL(st.Addr(), st.EnvironTag())
+	envTagString := ""
+	envTag, err := st.EnvironTag()
+	if err != nil {
+		logger.Errorf("environ tag is invalid: %v", err)
+	} else {
+		envTagString = envTag.String()
+	}
+	charmsURL := uniter.CharmsURL(st.Addr(), envTagString)
 	return uniter.NewState(st, unitTag, charmsURL)
 }
 

diff --git a/api/state_test.go b/api/state_test.go
@@ -79,11 +79,15 @@ func (s *stateSuite) TestLoginSetsEnvironTag(c *gc.C) {
 	apistate, tag, password := s.OpenAPIWithoutLogin(c)
 	defer apistate.Close()
 	// We haven't called Login yet, so the EnvironTag shouldn't be set.
-	c.Check(apistate.EnvironTag(), gc.Equals, "")
+	envTag, err := apistate.EnvironTag()
+	c.Check(err, gc.ErrorMatches, `"" is not a valid tag`)
+	c.Check(envTag.String(), gc.Equals, "environment-")
 	err = apistate.Login(tag, password, "")
 	c.Assert(err, gc.IsNil)
 	// Now that we've logged in, EnvironTag should be updated correctly.
-	c.Check(apistate.EnvironTag(), gc.Equals, env.Tag().String())
+	envTag, err = apistate.EnvironTag()
+	c.Check(err, gc.IsNil)
+	c.Check(envTag.String(), gc.Equals, env.Tag().String())
 }
 
 func (s *stateSuite) TestLoginTracksFacadeVersions(c *gc.C) {

diff --git a/cmd/juju/ensureavailability.go b/cmd/juju/ensureavailability.go
@@ -168,7 +168,11 @@ func (c *EnsureAvailabilityCommand) Run(ctx *cmd.Context) error {
 		return errors.Annotate(err, "cannot get API connection")
 	}
 	var ensureAvailabilityResult params.StateServersChanges
-	haClient := highavailability.NewClient(root, root.EnvironTag())
+	envTag, err := root.EnvironTag()
+	if err != nil {
+		return errors.Annotatef(err, "cannot get environ tag")
+	}
+	haClient := highavailability.NewClient(root, envTag.String())
 	defer haClient.Close()
 	ensureAvailabilityResult, err = haClient.EnsureAvailability(c.NumStateServers, c.Constraints, c.Series, c.Placement)
 	if err != nil {

diff --git a/juju/api.go b/juju/api.go
@@ -33,7 +33,7 @@ var (
 type apiState interface {
 	Close() error
 	APIHostPorts() [][]network.HostPort
-	EnvironTag() string
+	EnvironTag() (names.EnvironTag, error)
 }
 
 type apiOpenFunc func(*api.Info, api.DialOpts) (apiState, error)
@@ -198,7 +198,11 @@ func newAPIFromStore(envName string, store configstore.Storage, apiOpen apiOpenF
 		}
 	}
 	// Update API addresses if they've changed. Error is non-fatal.
-	if localerr := cacheChangedAPIInfo(info, st.APIHostPorts(), st.EnvironTag()); localerr != nil {
+	envTag, err := st.EnvironTag()
+	if err != nil {
+		logger.Warningf("ignoring API connection environ tag: %v", err)
+	}
+	if localerr := cacheChangedAPIInfo(info, st.APIHostPorts(), envTag); localerr != nil {
 		logger.Warningf("cannot failed to cache API addresses: %v", localerr)
 	}
 	return st, nil
@@ -357,7 +361,7 @@ func cacheAPIInfo(info configstore.EnvironInfo, apiInfo *api.Info) (err error) {
 // cacheChangedAPIInfo updates the local environment settings (.jenv file)
 // with the provided API server addresses if they have changed. It will also
 // save the environment tag if it is available.
-func cacheChangedAPIInfo(info configstore.EnvironInfo, hostPorts [][]network.HostPort, newEnvironTag string) error {
+func cacheChangedAPIInfo(info configstore.EnvironInfo, hostPorts [][]network.HostPort, newEnvironTag names.EnvironTag) error {
 	var addrs []string
 	for _, serverHostPorts := range hostPorts {
 		for _, hostPort := range serverHostPorts {
@@ -371,15 +375,10 @@ func cacheChangedAPIInfo(info configstore.EnvironInfo, hostPorts [][]network.Hos
 	}
 	endpoint := info.APIEndpoint()
 	changed := false
-	if newEnvironTag != "" {
-		tag, err := names.ParseEnvironTag(newEnvironTag)
-		if err == nil {
-			if environUUID := tag.Id(); endpoint.EnvironUUID != environUUID {
-				changed = true
-				endpoint.EnvironUUID = environUUID
-			}
-		} else {
-			logger.Debugf("cannot parse environ tag: %v", err)
+	if newEnvironTag.Id() != "" {
+		if environUUID := newEnvironTag.Id(); endpoint.EnvironUUID != environUUID {
+			changed = true
+			endpoint.EnvironUUID = environUUID
 		}
 	}
 	if len(addrs) != 0 && addrsChanged(endpoint.Addresses, addrs) {

diff --git a/juju/api_test.go b/juju/api_test.go
@@ -834,7 +834,7 @@ func (s *CacheChangedAPISuite) TestAPIEndpointNotMachineLocalOrLinkLocal(c *gc.C
 	}
 
 	envTag := names.NewEnvironTag(fakeUUID)
-	err := juju.CacheChangedAPIInfo(info, hostPorts, envTag.String())
+	err := juju.CacheChangedAPIInfo(info, hostPorts, envTag)
 	c.Assert(err, gc.IsNil)
 
 	endpoint := info.APIEndpoint()

diff --git a/juju/mock_test.go b/juju/mock_test.go
@@ -1,6 +1,8 @@
 package juju_test
 
 import (
+	"github.com/juju/names"
+
 	"github.com/juju/juju/api"
 	"github.com/juju/juju/juju"
 	"github.com/juju/juju/network"
@@ -24,8 +26,8 @@ func (s *mockAPIState) APIHostPorts() [][]network.HostPort {
 	return s.apiHostPorts
 }
 
-func (s *mockAPIState) EnvironTag() string {
-	return s.environTag
+func (s *mockAPIState) EnvironTag() (names.EnvironTag, error) {
+	return names.ParseEnvironTag(s.environTag)
 }
 
 func panicAPIOpen(apiInfo *api.Info, opts api.DialOpts) (juju.APIState, error) {

diff --git a/state/ports_test.go b/state/ports_test.go
@@ -332,6 +332,10 @@ func (s *PortsDocSuite) TestWatchPorts(c *gc.C) {
 
 	defer statetesting.AssertStop(c, w)
 	wc := statetesting.NewStringsWatcherC(c, s.State, w)
+	// The first change we get is an empty one, as there are no ports
+	// opened yet and we need an initial event for the API watcher to
+	// work.
+	wc.AssertChange()
 	wc.AssertNoChange()
 
 	portRange := state.PortRange{