apiserver: Extract common.EnvironWatcher mixin

apiserver: Extract common.EnvironWatcher mixin

These two methods of the ProvisionerAPI and the
upcoming FirewallerAPI are common, so they're
extracted in a common.EnvironWatcher mixin:
* WatchForEnvironConfigChanges()
* EnvironConfig()

As a side-effect, added a TODO and a tech-dept
bug about UniterAPI.GetOwnerTag: no doc comment
and no permissions check implemented (#1270795).

This is the first of a sequence of CLs which will
end with a complete server-side API for the 
firewaller.

https://code.launchpad.net/~dimitern/juju-core/232-apiserver-common-environwatcher/+merge/202343

(do not edit description out of merge proposal)

[dimitern, 2014-01-20 17:02:43]

Please take a look.

[rog, 2014-01-21 10:44:50]

Looks great in general with a few suggestion.
I'm not that keen on the "not available" attributes
though.

https://codereview.appspot.com/54620043/diff/1/state/api/uniter/service.go
File state/api/uniter/service.go (right):

https://codereview.appspot.com/54620043/diff/1/state/api/uniter/service.go#ne...
state/api/uniter/service.go:131: // Add a doc comment here.
You could just add a doc comment here :-)

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
File state/apiserver/common/environwatcher.go (right):

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher.go:13: // EnvironWatcher implements two
common method for use by various
s/method/methods/

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher.go:24: // determine current permissions.
// Active watchers will be stored in the provided Resources.
?

and perhaps document the fact that the expected
tag is "" not the environment tag?

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher.go:34: // WatchForEnvironConfigChanges
returns a NotifyWatcher to observe
s/to observe/that observes/ ?

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher.go:85: // This needs to change so we won't
return anything to
How is that not covered by the canReadSecrets permission check?

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher.go:94: allAttrs[k] = "not available"
That doesn't seem right to me - I think it would probably be better just to
delete the attributes, rather than replacing them with an arbitrary value that
might be of the wrong type.

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
File state/apiserver/common/environwatcher_test.go (right):

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher_test.go:20: type environWatcherSuite
struct{}
please embed LoggingSuite.

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher_test.go:101: "state-id":     "1", //
needed by the dummy provider to signal an environment is prepared.
I don't understand this - how are these tests interacting with the dummy
provider?

https://codereview.appspot.com/54620043/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/54620043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:70: getCanWatch := func()
(common.AuthFunc, error) {
Just a thought - you could define a function in commmon/interfaces.go to make
this code simpler, say:

// AuthAlways returns an authentication function
// that always returns the given permission.
func AuthAlways(ok bool) func() (common.AuthFunc, error) {
	return func() {
		return func(tag string) bool {
			return ok
		}
	}, nil
}


then you could do:

getCanWatch := common.AuthAlways(true)
getCanReadSecrets := common.AuthAlways(authorizer.AuthEnvironManager())

[dimitern, 2014-01-21 11:17:30]

Please take a look.

https://codereview.appspot.com/54620043/diff/1/state/api/uniter/service.go
File state/api/uniter/service.go (right):

https://codereview.appspot.com/54620043/diff/1/state/api/uniter/service.go#ne...
state/api/uniter/service.go:131: // Add a doc comment here.
On 2014/01/21 10:44:51, rog wrote:
> You could just add a doc comment here :-)

Yes, but it's out of scope for this CL.

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
File state/apiserver/common/environwatcher.go (right):

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher.go:13: // EnvironWatcher implements two
common method for use by various
On 2014/01/21 10:44:51, rog wrote:
> s/method/methods/

Done.

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher.go:24: // determine current permissions.
On 2014/01/21 10:44:51, rog wrote:
> // Active watchers will be stored in the provided Resources.
> ?
> 
> and perhaps document the fact that the expected
> tag is "" not the environment tag?

Done.

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher.go:34: // WatchForEnvironConfigChanges
returns a NotifyWatcher to observe
On 2014/01/21 10:44:51, rog wrote:
> s/to observe/that observes/ ?

Done.

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher.go:85: // This needs to change so we won't
return anything to
On 2014/01/21 10:44:51, rog wrote:
> How is that not covered by the canReadSecrets permission check?

#1231384 is a general enhancement tech-dept bug, which (once fixed) will make
the masking of secret attributes irrelevant (the lxc/kvm provisioners won't have
access to them).

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher.go:94: allAttrs[k] = "not available"
On 2014/01/21 10:44:51, rog wrote:
> That doesn't seem right to me - I think it would probably be better just to
> delete the attributes, rather than replacing them with an arbitrary value that
> might be of the wrong type.

This is as agreed previously while I was doing the ProvisionerAPI. Leaving the
attributes in place rather than just deleting them helps the consumers
reconstruct a config.Config from the attributes, without having to worry about
missing and required values.

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
File state/apiserver/common/environwatcher_test.go (right):

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher_test.go:20: type environWatcherSuite
struct{}
On 2014/01/21 10:44:51, rog wrote:
> please embed LoggingSuite.

Will do. What's benefit by the way?

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher_test.go:101: "state-id":     "1", //
needed by the dummy provider to signal an environment is prepared.
On 2014/01/21 10:44:51, rog wrote:
> I don't understand this - how are these tests interacting with the dummy
> provider?

I need a supported provider config, which knows its own secret attributes, so I
can test the secrets masking.

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher_test.go:101: "state-id":     "1", //
needed by the dummy provider to signal an environment is prepared.
On 2014/01/21 10:44:51, rog wrote:
> I don't understand this - how are these tests interacting with the dummy
> provider?

Dummy provider knows its secret attributes, which I can check they're masked
when canReadSecrets returns false. Using just random config is not an option
(couldn't make it work, believe me, I tried).

https://codereview.appspot.com/54620043/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/54620043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:70: getCanWatch := func()
(common.AuthFunc, error) {
On 2014/01/21 10:44:51, rog wrote:
> Just a thought - you could define a function in commmon/interfaces.go to make
> this code simpler, say:
> 
> // AuthAlways returns an authentication function
> // that always returns the given permission.
> func AuthAlways(ok bool) func() (common.AuthFunc, error) {
> 	return func() {
> 		return func(tag string) bool {
> 			return ok
> 		}
> 	}, nil
> }
> 
> 
> then you could do:
> 
> getCanWatch := common.AuthAlways(true)
> getCanReadSecrets := common.AuthAlways(authorizer.AuthEnvironManager())

Good idea - done.

[rog, 2014-01-21 11:42:53]

I'm still not keen on the "not available" thing.
And I've added a suggestion for making the dummy configuration a little less
fragile.

https://codereview.appspot.com/54620043/diff/1/state/api/uniter/service.go
File state/api/uniter/service.go (right):

https://codereview.appspot.com/54620043/diff/1/state/api/uniter/service.go#ne...
state/api/uniter/service.go:131: // Add a doc comment here.
On 2014/01/21 11:17:30, dimitern wrote:
> On 2014/01/21 10:44:51, rog wrote:
> > You could just add a doc comment here :-)
> 
> Yes, but it's out of scope for this CL.

For this kind of drive-by fix, I don't mind much.

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
File state/apiserver/common/environwatcher.go (right):

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher.go:85: // This needs to change so we won't
return anything to
On 2014/01/21 11:17:30, dimitern wrote:
> On 2014/01/21 10:44:51, rog wrote:
> > How is that not covered by the canReadSecrets permission check?
> 
> #1231384 is a general enhancement tech-dept bug, which (once fixed) will make
> the masking of secret attributes irrelevant (the lxc/kvm provisioners won't
have
> access to them).

How about:

// TODO(dimitern) 2013-09-26 bug #1231384
// Delete this code when the above bug is fixed.

?

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher.go:94: allAttrs[k] = "not available"
On 2014/01/21 11:17:30, dimitern wrote:
> On 2014/01/21 10:44:51, rog wrote:
> > That doesn't seem right to me - I think it would probably be better just to
> > delete the attributes, rather than replacing them with an arbitrary value
that
> > might be of the wrong type.
> 
> This is as agreed previously while I was doing the ProvisionerAPI. Leaving the
> attributes in place rather than just deleting them helps the consumers
> reconstruct a config.Config from the attributes, without having to worry about
> missing and required values.

I don't buy it. There are no secret required attributes in config.Config
and an agent that tries to call environs.New without secrets *should*
fail (by doing the above, it might well succeed and create an Environ
with erroneous secret attributes).

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
File state/apiserver/common/environwatcher_test.go (right):

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher_test.go:20: type environWatcherSuite
struct{}
On 2014/01/21 11:17:30, dimitern wrote:
> On 2014/01/21 10:44:51, rog wrote:
> > please embed LoggingSuite.
> 
> Will do. What's benefit by the way?

It means that if someone's trying to debug the tests, logging
messages don't disappear into /dev/null.

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher_test.go:101: "state-id":     "1", //
needed by the dummy provider to signal an environment is prepared.
On 2014/01/21 11:17:30, dimitern wrote:
> On 2014/01/21 10:44:51, rog wrote:
> > I don't understand this - how are these tests interacting with the dummy
> > provider?
> 
> Dummy provider knows its secret attributes, which I can check they're masked
> when canReadSecrets returns false. Using just random config is not an option
> (couldn't make it work, believe me, I tried).

Ah, I see. Rather than doing the above, which seems quite fragile, I think it
would
be better to actually prepare the dummy env and use that config.

Something like this, perhaps:

cfg, err := config.New(config.NoDefaults, dummy.SampleConfig)
...
env, err := environs.Prepare(cfg, configstore.NewMemStorage())
...
testingEnvConfig = env.Config()

[dimitern, 2014-01-21 13:59:04]

Please take a look.

https://codereview.appspot.com/54620043/diff/1/state/api/uniter/service.go
File state/api/uniter/service.go (right):

https://codereview.appspot.com/54620043/diff/1/state/api/uniter/service.go#ne...
state/api/uniter/service.go:131: // Add a doc comment here.
On 2014/01/21 11:42:53, rog wrote:
> On 2014/01/21 11:17:30, dimitern wrote:
> > On 2014/01/21 10:44:51, rog wrote:
> > > You could just add a doc comment here :-)
> > 
> > Yes, but it's out of scope for this CL.
> 
> For this kind of drive-by fix, I don't mind much.

I'd prefer to keep both changes related to fixing bug 1270795 in a single CL, if
you don't mind.

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
File state/apiserver/common/environwatcher.go (right):

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher.go:85: // This needs to change so we won't
return anything to
On 2014/01/21 11:42:53, rog wrote:
> On 2014/01/21 11:17:30, dimitern wrote:
> > On 2014/01/21 10:44:51, rog wrote:
> > > How is that not covered by the canReadSecrets permission check?
> > 
> > #1231384 is a general enhancement tech-dept bug, which (once fixed) will
make
> > the masking of secret attributes irrelevant (the lxc/kvm provisioners won't
> have
> > access to them).
> 
> How about:
> 
> // TODO(dimitern) 2013-09-26 bug #1231384
> // Delete this code when the above bug is fixed.
> 
> ?

Done slightly differently, mentioning the 1.16 compatibility issue.

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher.go:94: allAttrs[k] = "not available"
On 2014/01/21 11:42:53, rog wrote:
> On 2014/01/21 11:17:30, dimitern wrote:
> > On 2014/01/21 10:44:51, rog wrote:
> > > That doesn't seem right to me - I think it would probably be better just
to
> > > delete the attributes, rather than replacing them with an arbitrary value
> that
> > > might be of the wrong type.
> > 
> > This is as agreed previously while I was doing the ProvisionerAPI. Leaving
the
> > attributes in place rather than just deleting them helps the consumers
> > reconstruct a config.Config from the attributes, without having to worry
about
> > missing and required values.
> 
> I don't buy it. There are no secret required attributes in config.Config
> and an agent that tries to call environs.New without secrets *should*
> fail (by doing the above, it might well succeed and create an Environ
> with erroneous secret attributes).

The secret attributes are supposed to be erroneous, if you're not allowed to see
them. As discussed, let's drop this for now and focus on it in a follow-up,
after the firewaller api chain of CL lands please.
I've updated the description of bug 1231384 to reflect the current code state,
but we can't fix it yet (and remove the workaround here) before we drop 1.16
compatibility.

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
File state/apiserver/common/environwatcher_test.go (right):

https://codereview.appspot.com/54620043/diff/1/state/apiserver/common/environ...
state/apiserver/common/environwatcher_test.go:101: "state-id":     "1", //
needed by the dummy provider to signal an environment is prepared.
On 2014/01/21 11:42:53, rog wrote:
> On 2014/01/21 11:17:30, dimitern wrote:
> > On 2014/01/21 10:44:51, rog wrote:
> > > I don't understand this - how are these tests interacting with the dummy
> > > provider?
> > 
> > Dummy provider knows its secret attributes, which I can check they're masked
> > when canReadSecrets returns false. Using just random config is not an option
> > (couldn't make it work, believe me, I tried).
> 
> Ah, I see. Rather than doing the above, which seems quite fragile, I think it
> would
> be better to actually prepare the dummy env and use that config.
> 
> Something like this, perhaps:
> 
> cfg, err := config.New(config.NoDefaults, dummy.SampleConfig)
> ...
> env, err := environs.Prepare(cfg, configstore.NewMemStorage())
> ...
> testingEnvConfig = env.Config()
> 

Done.

[rog, 2014-01-21 14:03:36]

LGTM, thanks.