cmd/jujud: add firewaller outline

cmd/jujud: add firewaller outline

This proposal adds a skeletal firewaller service to complete the
provisioning agent. The firewaller only reacts to changes in the
environment configuration at the moment because it needs the following
(unwritten) pieces:

* the ability to watch all services 
* an interface into firewalling services in the environs.Environ interface

https://code.launchpad.net/~dave-cheney/juju-core/go-cmd-jujud-firewaller/+merge/112007

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-06-26 06:07:42]

Please take a look.

[fwereade, 2012-06-26 15:58:56]

A few thoughts/questions...

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go
File cmd/jujud/firewall.go (right):

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go#newcode13
cmd/jujud/firewall.go:13: type Firewaller struct {
A comment would be good, but maybe it's redundant at this stage. ("Firewaller
doesn't do anything yet"? ;))

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go#newcode24
cmd/jujud/firewall.go:24: st, err := state.Open(info)
Why do we need a whole new *State for a single component? It feels somewhat
wasteful at best, and at worst actively dangerous (surely, given that 2 separate
connections will not necessarily have remotely consistent views of state, any
non-zk-mediated data flow between components with separate connections is
potentially problematic...).

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go#newcode38
cmd/jujud/firewall.go:38: defer f.st.Close()
defer f.environWatcher.Stop()

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go#newcode50
cmd/jujud/firewall.go:50: return
Equivalent of `f.tomb.Kill(mustErr(f.environWatcher))` (see state/watcher.go)
would I think be better.

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall_test.go
File cmd/jujud/firewall_test.go (right):

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall_test.go#n...
cmd/jujud/firewall_test.go:28: _, err = env.Write()
I don't see what this is actually testing... surely we should at least check the
log messages?

[niemeyer, 2012-06-27 00:11:12]

Looks like a nice bootstrap.

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go
File cmd/jujud/firewall.go (right):

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go#newcode24
cmd/jujud/firewall.go:24: st, err := state.Open(info)
On 2012/06/26 15:58:56, fwereade wrote:
> Why do we need a whole new *State for a single component? It feels somewhat
> wasteful at best, and at worst actively dangerous (surely, given that 2
separate
> connections will not necessarily have remotely consistent views of state, any
> non-zk-mediated data flow between components with separate connections is
> potentially problematic...).

Agreed. It seems sensible for the firewaller to get a *State instead.

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go#newcode53
cmd/jujud/firewall.go:53: f.environ, err = environs.NewEnviron(config.Map())
Why do we need to watch the environment in the firewaller? Sorry if it's obvious
and I'm missing it.

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go#newcode55
cmd/jujud/firewall.go:55: log.Printf("firewaller loaded invalid environment
configuration: %v", err)
If this happens f.environ would be broken at this point (nil), and thus anything
else that depends on it would be dead.

[dave_cheney.net, 2012-07-11 05:00:22]

Please take a look.

[dave_cheney.net, 2012-07-12 00:07:28]

Please take a look.

[dave_cheney.net, 2012-07-12 23:18:25]

Please take a look.

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go
File cmd/jujud/firewall.go (right):

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go#newcode24
cmd/jujud/firewall.go:24: st, err := state.Open(info)
On 2012/06/27 00:11:12, niemeyer wrote:
> On 2012/06/26 15:58:56, fwereade wrote:
> > Why do we need a whole new *State for a single component? It feels somewhat
> > wasteful at best, and at worst actively dangerous (surely, given that 2
> separate
> > connections will not necessarily have remotely consistent views of state,
any
> > non-zk-mediated data flow between components with separate connections is
> > potentially problematic...).
> 
> Agreed. It seems sensible for the firewaller to get a *State instead.

Done. I will also change the provisioner to expose it's current state object.

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go#newcode38
cmd/jujud/firewall.go:38: defer f.st.Close()
On 2012/06/26 15:58:56, fwereade wrote:
> defer f.environWatcher.Stop()

Done.

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go#newcode50
cmd/jujud/firewall.go:50: return
On 2012/06/26 15:58:56, fwereade wrote:
> Equivalent of `f.tomb.Kill(mustErr(f.environWatcher))` (see state/watcher.go)
> would I think be better.

Done.

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go#newcode53
cmd/jujud/firewall.go:53: f.environ, err = environs.NewEnviron(config.Map())
On 2012/06/27 00:11:12, niemeyer wrote:
> Why do we need to watch the environment in the firewaller? Sorry if it's
obvious
> and I'm missing it.

Because the plan is the Environ will provide the generic interface for open port
/ close port.

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go#newcode55
cmd/jujud/firewall.go:55: log.Printf("firewaller loaded invalid environment
configuration: %v", err)
On 2012/06/27 00:11:12, niemeyer wrote:
> If this happens f.environ would be broken at this point (nil), and thus
anything
> else that depends on it would be dead.

Done.

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go#newcode55
cmd/jujud/firewall.go:55: log.Printf("firewaller loaded invalid environment
configuration: %v", err)
On 2012/06/27 00:11:12, niemeyer wrote:
> If this happens f.environ would be broken at this point (nil), and thus
anything
> else that depends on it would be dead.

Done.

[fwereade, 2012-07-16 13:53:18]

Not entirely keen on having multiple environs in the PA still, for similar
resons to not wanting multiple States. Thoughts?

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go
File cmd/jujud/firewall.go (right):

https://codereview.appspot.com/6333067/diff/2001/cmd/jujud/firewall.go#newcode53
cmd/jujud/firewall.go:53: f.environ, err = environs.NewEnviron(config.Map())
On 2012/07/12 23:18:26, dfc wrote:
> On 2012/06/27 00:11:12, niemeyer wrote:
> > Why do we need to watch the environment in the firewaller? Sorry if it's
> obvious
> > and I'm missing it.
> 
> Because the plan is the Environ will provide the generic interface for open
port
> / close port.

Hmm, I'd been vaguely expecting the PA itself to manage the environ-watching and
make it available to other thing -- do we absolutely have to have multiple
Environs in the PA?

[dave_cheney.net, 2012-07-17 06:23:51]

*** ABANDONED ***

Frank has taken this ball and run with it.