EnvironTag from the API connection is accessible to all facades

zhatin · Sep 25, 2014 · 7ab5622 · 7ab5622
1 parent e9fc363
commit 7ab5622
Show file tree

Hide file tree

Showing 31 changed files with 139 additions and 100 deletions.
diff --git a/api/apiclient.go b/api/apiclient.go
@@ -94,9 +94,9 @@ type Info struct {
 	// only by the machine agent.
 	Nonce string `yaml:",omitempty"`
 
-	// Environ holds the environ tag for the environment we are trying to
-	// connect to.
-	EnvironTag names.Tag
+	// EnvironTag holds the environ tag for the environment we are
+	// trying to connect to.
+	EnvironTag names.EnvironTag
 }
 
 // DialOpts holds configuration parameters that control the
@@ -137,7 +137,7 @@ func Open(info *Info, opts DialOpts) (*State, error) {
 	pool.AddCert(xcert)
 
 	var environUUID string
-	if info.EnvironTag != nil {
+	if info.EnvironTag.Id() != "" {
 		environUUID = info.EnvironTag.Id()
 	}
 	// Dial all addresses at reasonable intervals.

diff --git a/api/apiclient_test.go b/api/apiclient_test.go
@@ -142,13 +142,13 @@ func (s *apiclientSuite) TestOpenPassesEnvironTag(c *gc.C) {
 	c.Check(err, gc.ErrorMatches, `unknown environment: "bad-tag"`)
 	c.Check(params.ErrCode(err), gc.Equals, params.CodeNotFound)
 	// Now set it to the right tag, and we should succeed.
-	info.EnvironTag = env.Tag()
+	info.EnvironTag = env.EnvironTag()
 	st, err := api.Open(info, api.DialOpts{})
 	c.Assert(err, gc.IsNil)
 	st.Close()
 	// Backwards compatibility, we should succeed if we do not set an
 	// environ tag
-	info.EnvironTag = nil
+	info.EnvironTag = names.NewEnvironTag("")
 	st, err = api.Open(info, api.DialOpts{})
 	c.Assert(err, gc.IsNil)
 	st.Close()

diff --git a/api/client_test.go b/api/client_test.go
@@ -306,7 +306,7 @@ func (s *clientSuite) TestDebugLogRootPath(c *gc.C) {
 
 	// If the server is old, we log at "/log"
 	info := s.APIInfo(c)
-	info.EnvironTag = nil
+	info.EnvironTag = names.NewEnvironTag("")
 	apistate, err := api.Open(info, api.DialOpts{})
 	c.Assert(err, gc.IsNil)
 	defer apistate.Close()
@@ -322,7 +322,7 @@ func (s *clientSuite) TestDebugLogAtUUIDLogPath(c *gc.C) {
 	environ, err := s.State.Environment()
 	c.Assert(err, gc.IsNil)
 	info := s.APIInfo(c)
-	info.EnvironTag = environ.Tag()
+	info.EnvironTag = environ.EnvironTag()
 	apistate, err := api.Open(info, api.DialOpts{})
 	c.Assert(err, gc.IsNil)
 	defer apistate.Close()
@@ -336,15 +336,15 @@ func (s *clientSuite) TestDebugLogAtUUIDLogPath(c *gc.C) {
 func (s *clientSuite) TestOpenUsesEnvironUUIDPaths(c *gc.C) {
 	info := s.APIInfo(c)
 	// Backwards compatibility, passing EnvironTag = "" should just work
-	info.EnvironTag = nil
+	info.EnvironTag = names.NewEnvironTag("")
 	apistate, err := api.Open(info, api.DialOpts{})
 	c.Assert(err, gc.IsNil)
 	apistate.Close()
 
 	// Passing in the correct environment UUID should also work
 	environ, err := s.State.Environment()
 	c.Assert(err, gc.IsNil)
-	info.EnvironTag = environ.Tag()
+	info.EnvironTag = environ.EnvironTag()
 	apistate, err = api.Open(info, api.DialOpts{})
 	c.Assert(err, gc.IsNil)
 	apistate.Close()

diff --git a/api/export_test.go b/api/export_test.go
@@ -23,7 +23,7 @@ func SetServerRoot(c *Client, root string) {
 	c.st.serverRoot = root
 }
 
-// SetEnvironTag patches the value of the environment tag.
+// PatchEnvironTag patches the value of the environment tag.
 // It returns a function that reverts the change.
 func PatchEnvironTag(st *State, envTag string) func() {
 	originalTag := st.environTag

diff --git a/api/highavailability/client.go b/api/highavailability/client.go
@@ -5,21 +5,29 @@ package highavailability
 
 import (
 	"github.com/juju/errors"
+	"github.com/juju/loggo"
+	"github.com/juju/names"
 
 	"github.com/juju/juju/api/base"
 	"github.com/juju/juju/apiserver/params"
 	"github.com/juju/juju/constraints"
 )
 
+var logger = loggo.GetLogger("juju.api.highavailability")
+
 // Client provides access to the high availability service, used to manage state servers.
 type Client struct {
 	base.ClientFacade
 	facade     base.FacadeCaller
-	environTag string
+	environTag names.EnvironTag
 }
 
 // NewClient returns a new HighAvailability client.
-func NewClient(caller base.APICallCloser, environTag string) *Client {
+func NewClient(caller base.APICallCloser) *Client {
+	environTag, err := caller.EnvironTag()
+	if err != nil {
+		logger.Errorf("ignoring invalid environment tag: %v", err)
+	}
 	frontend, backend := base.NewClientFacade(caller, "HighAvailability")
 	return &Client{ClientFacade: frontend, facade: backend, environTag: environTag}
 }
@@ -32,7 +40,7 @@ func (c *Client) EnsureAvailability(
 	var results params.StateServersChangeResults
 	arg := params.StateServersSpecs{
 		Specs: []params.StateServersSpec{{
-			EnvironTag:      c.environTag,
+			EnvironTag:      c.environTag.String(),
 			NumStateServers: numStateServers,
 			Constraints:     cons,
 			Series:          series,

diff --git a/api/highavailability/client_test.go b/api/highavailability/client_test.go
@@ -27,18 +27,6 @@ type clientSuite struct {
 
 var _ = gc.Suite(&clientSuite{})
 
-func (s *clientSuite) TestClientEnsureAvailabilityFailsBadEnvTag(c *gc.C) {
-	_, err := s.State.AddMachine("quantal", state.JobManageEnviron)
-	c.Assert(err, gc.IsNil)
-
-	emptyCons := constraints.Value{}
-	defaultSeries := ""
-	client := highavailability.NewClient(s.APIState, "bad-env-uuid")
-	_, err = client.EnsureAvailability(3, emptyCons, defaultSeries, nil)
-	c.Assert(err, gc.ErrorMatches,
-		`invalid environment tag: "bad-env-uuid" is not a valid tag`)
-}
-
 type Killer interface {
 	Kill() error
 }
@@ -67,8 +55,8 @@ func assertEnsureAvailability(c *gc.C, s *jujutesting.JujuConnSuite) {
 	defer assertKill(c, pingerA)
 
 	emptyCons := constraints.Value{}
-	result, err := highavailability.NewClient(
-		s.APIState, s.State.EnvironTag().String()).EnsureAvailability(3, emptyCons, "", nil)
+	client := highavailability.NewClient(s.APIState)
+	result, err := client.EnsureAvailability(3, emptyCons, "", nil)
 	c.Assert(err, gc.IsNil)
 
 	c.Assert(result.Maintained, gc.DeepEquals, []string{"machine-0"})
@@ -88,7 +76,7 @@ func (s *clientSuite) TestClientEnsureAvailability(c *gc.C) {
 }
 
 func (s *clientSuite) TestClientEnsureAvailabilityVersion(c *gc.C) {
-	client := highavailability.NewClient(s.APIState, s.State.EnvironTag().String())
+	client := highavailability.NewClient(s.APIState)
 	c.Assert(client.BestAPIVersion(), gc.Equals, 1)
 }
 
@@ -108,7 +96,7 @@ func (s *clientLegacySuite) TestEnsureAvailabilityLegacy(c *gc.C) {
 }
 
 func (s *clientLegacySuite) TestEnsureAvailabilityLegacyRejectsPlacement(c *gc.C) {
-	_, err := highavailability.NewClient(
-		s.APIState, s.State.EnvironTag().String()).EnsureAvailability(3, constraints.Value{}, "", []string{"machine"})
+	client := highavailability.NewClient(s.APIState)
+	_, err := client.EnsureAvailability(3, constraints.Value{}, "", []string{"machine"})
 	c.Assert(err, gc.ErrorMatches, "placement directives not supported with this version of Juju")
 }
diff --git a/api/state.go b/api/state.go
@@ -7,6 +7,7 @@ import (
 	"net"
 	"strconv"
 
+	"github.com/juju/errors"
 	"github.com/juju/names"
 
 	"github.com/juju/juju/api/agent"
@@ -132,18 +133,17 @@ func (st *State) Provisioner() *provisioner.State {
 
 // Uniter returns a version of the state that provides functionality
 // required by the uniter worker.
-func (st *State) Uniter() *uniter.State {
-	// TODO(dfc) yes, this can panic, we never checked before
-	unitTag := st.authTag.(names.UnitTag)
-	envTagString := ""
+func (st *State) Uniter() (*uniter.State, error) {
+	unitTag, ok := st.authTag.(names.UnitTag)
+	if !ok {
+		return nil, errors.Errorf("invalid authenticated unit tag %q", st.authTag)
+	}
 	envTag, err := st.EnvironTag()
 	if err != nil {
-		logger.Errorf("environ tag is invalid: %v", err)
-	} else {
-		envTagString = envTag.String()
+		logger.Warningf("ignoring invalid environ tag: %v", err)
 	}
-	charmsURL := uniter.CharmsURL(st.Addr(), envTagString)
-	return uniter.NewState(st, unitTag, charmsURL)
+	charmsURL := uniter.CharmsURL(st.Addr(), envTag)
+	return uniter.NewState(st, unitTag, charmsURL), nil
 }
 
 // Firewaller returns a version of the state that provides functionality

diff --git a/api/state_test.go b/api/state_test.go
@@ -87,7 +87,7 @@ func (s *stateSuite) TestLoginSetsEnvironTag(c *gc.C) {
 	// Now that we've logged in, EnvironTag should be updated correctly.
 	envTag, err = apistate.EnvironTag()
 	c.Check(err, gc.IsNil)
-	c.Check(envTag.String(), gc.Equals, env.Tag().String())
+	c.Check(envTag.String(), gc.Equals, env.EnvironTag().String())
 }
 
 func (s *stateSuite) TestLoginTracksFacadeVersions(c *gc.C) {

diff --git a/api/uniter/charm.go b/api/uniter/charm.go
@@ -79,14 +79,11 @@ func (c *Charm) ArchiveSha256() (string, error) {
 
 // CharmsURL takes an API server address and an optional environment
 // tag and constructs a base URL used for fetching charm archives.
-// If the environment tag is omitted or invalid, it will be ignored.
-func CharmsURL(apiAddr string, envTag string) *url.URL {
+// If the environment tag empty or invalid, it will be ignored.
+func CharmsURL(apiAddr string, envTag names.EnvironTag) *url.URL {
 	urlPath := "/"
-	if envTag != "" {
-		tag, err := names.ParseEnvironTag(envTag)
-		if err == nil {
-			urlPath = path.Join(urlPath, "environment", tag.Id())
-		}
+	if envTag.Id() != "" {
+		urlPath = path.Join(urlPath, "environment", envTag.Id())
 	}
 	urlPath = path.Join(urlPath, "charms")
 	return &url.URL{Scheme: "https", Host: apiAddr, Path: urlPath}

diff --git a/api/uniter/charm_test.go b/api/uniter/charm_test.go
@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"net/url"
 
+	"github.com/juju/names"
 	gc "launchpad.net/gocheck"
 
 	"github.com/juju/juju/api/uniter"
@@ -82,7 +83,12 @@ func (s *charmsURLSuite) TestCharmsURL(c *gc.C) {
 }
 
 func testCharmsURL(c *gc.C, addr, envTag, expected string) {
-	url := uniter.CharmsURL(addr, envTag)
+	tag, err := names.ParseEnvironTag(envTag)
+	if err != nil {
+		// If it's invalid, pretend it's not set at all.
+		tag = names.NewEnvironTag("")
+	}
+	url := uniter.CharmsURL(addr, tag)
 	if !c.Check(url, gc.NotNil) {
 		return
 	}

diff --git a/api/uniter/uniter_test.go b/api/uniter/uniter_test.go
@@ -59,7 +59,8 @@ func (s *uniterSuite) setUpTest(c *gc.C, addStateServer bool) {
 	s.st = s.OpenAPIAs(c, s.wordpressUnit.Tag(), password)
 
 	// Create the uniter API facade.
-	s.uniter = s.st.Uniter()
+	s.uniter, err = s.st.Uniter()
+	c.Assert(err, gc.IsNil)
 	c.Assert(s.uniter, gc.NotNil)
 }
 

diff --git a/apiserver/admin_test.go b/apiserver/admin_test.go
@@ -478,7 +478,7 @@ func (s *loginSuite) TestLoginReportsEnvironTag(c *gc.C) {
 	c.Assert(err, gc.IsNil)
 	env, err := s.State.Environment()
 	c.Assert(err, gc.IsNil)
-	c.Assert(result.EnvironTag, gc.Equals, env.Tag().String())
+	c.Assert(result.EnvironTag, gc.Equals, env.EnvironTag().String())
 }
 
 func (s *loginSuite) TestLoginValidationSuccess(c *gc.C) {
@@ -565,7 +565,7 @@ func (s *loginSuite) setupServerWithValidator(c *gc.C, validator apiserver.Login
 	info := &api.Info{
 		Tag:        nil,
 		Password:   "",
-		EnvironTag: env.Tag(),
+		EnvironTag: env.EnvironTag(),
 		Addrs:      []string{srv.Addr()},
 		CACert:     coretesting.CACert,
 	}

diff --git a/apiserver/highavailability/export_test.go b/apiserver/highavailability/export_test.go
diff --git a/apiserver/metricsender/package_test.go b/apiserver/metricsender/package_test.go
@@ -4,8 +4,9 @@
 package metricsender_test
 
 import (
-	"github.com/juju/juju/testing"
 	stdtesting "testing"
+
+	"github.com/juju/juju/testing"
 )
 
 func TestPackage(t *stdtesting.T) {

diff --git a/apiserver/metricsmanager/metricsmanager.go b/apiserver/metricsmanager/metricsmanager.go
@@ -8,6 +8,7 @@ package metricsmanager
 import (
 	"github.com/juju/errors"
 	"github.com/juju/loggo"
+	"github.com/juju/names"
 
 	"github.com/juju/juju/apiserver/common"
 	"github.com/juju/juju/apiserver/metricsender"
@@ -35,6 +36,8 @@ type MetricsManager interface {
 // implementation of the api end point.
 type MetricsManagerAPI struct {
 	state *state.State
+
+	accessEnviron common.GetAuthFunc
 }
 
 var _ MetricsManager = (*MetricsManagerAPI)(nil)
@@ -49,7 +52,20 @@ func NewMetricsManagerAPI(
 		return nil, common.ErrPerm
 	}
 
-	return &MetricsManagerAPI{state: st}, nil
+	// Allow access only to the current environment.
+	accessEnviron := func() (common.AuthFunc, error) {
+		return func(tag names.Tag) bool {
+			if tag == nil {
+				return false
+			}
+			return tag.String() == st.EnvironTag().String()
+		}, nil
+	}
+
+	return &MetricsManagerAPI{
+		state:         st,
+		accessEnviron: accessEnviron,
+	}, nil
 }
 
 // CleanupOldMetrics removes old metrics from the collection.
@@ -64,12 +80,21 @@ func (api *MetricsManagerAPI) CleanupOldMetrics(args params.Entities) (params.Er
 	if len(args.Entities) == 0 {
 		return result, nil
 	}
+	canAccess, err := api.accessEnviron()
+	if err != nil {
+		return result, err
+	}
 	for i, arg := range args.Entities {
-		if arg.Tag != api.state.EnvironTag().String() {
+		tag, err := names.ParseEnvironTag(arg.Tag)
+		if err != nil {
+			result.Results[i].Error = common.ServerError(common.ErrPerm)
+			continue
+		}
+		if !canAccess(tag) {
 			result.Results[i].Error = common.ServerError(common.ErrPerm)
 			continue
 		}
-		err := api.state.CleanupOldMetrics()
+		err = api.state.CleanupOldMetrics()
 		if err != nil {
 			err = errors.Annotate(err, "failed to cleanup old metrics")
 			result.Results[i].Error = common.ServerError(err)
@@ -83,16 +108,24 @@ func (api *MetricsManagerAPI) SendMetrics(args params.Entities) (params.ErrorRes
 	result := params.ErrorResults{
 		Results: make([]params.ErrorResult, len(args.Entities)),
 	}
-
 	if len(args.Entities) == 0 {
 		return result, nil
 	}
+	canAccess, err := api.accessEnviron()
+	if err != nil {
+		return result, err
+	}
 	for i, arg := range args.Entities {
-		if arg.Tag != api.state.EnvironTag().String() {
+		tag, err := names.ParseEnvironTag(arg.Tag)
+		if err != nil {
+			result.Results[i].Error = common.ServerError(common.ErrPerm)
+			continue
+		}
+		if !canAccess(tag) {
 			result.Results[i].Error = common.ServerError(common.ErrPerm)
 			continue
 		}
-		err := api.state.SendMetrics(sender, maxBatchesPerSend)
+		err = api.state.SendMetrics(sender, maxBatchesPerSend)
 		if err != nil {
 			err = errors.Annotate(err, "failed to send metrics")
 			result.Results[i].Error = common.ServerError(err)