Merge lp:~dave-cheney/juju-core/go-cmd-jujud-firewaller into lp:~juju/juju-core/trunk

[Dave Cheney]

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://codereview.appspot.com/6333067/

[William Reade (fwereade)]

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#newcode28
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?

https://codereview.appspot.com/6333067/

[Gustavo Niemeyer (niemeyer)]

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.

https://codereview.appspot.com/6333067/

[lp:~dave-cheney/juju-core/go-cmd-jujud-firewaller]

269. By Dave Cheney on 2012-07-12

merge from trunk

270. By Dave Cheney on 2012-07-12

merge from trunk

271. By Dave Cheney on 2012-07-12

responding to review feedback

272. By Dave Cheney on 2012-07-12

responding to review feedback

273. By Dave Cheney on 2012-07-12

final tweaks

[William Reade (fwereade)]

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?

https://codereview.appspot.com/6333067/

[Gustavo Niemeyer (niemeyer)]

This was abandoned by Dave, in favor of Frank's recent work.

Unmerged revisions

273. By Dave Cheney on 2012-07-12

final tweaks

272. By Dave Cheney on 2012-07-12

responding to review feedback

271. By Dave Cheney on 2012-07-12

responding to review feedback

270. By Dave Cheney on 2012-07-12

merge from trunk

269. By Dave Cheney on 2012-07-12

merge from trunk

268. By Dave Cheney on 2012-07-11

merge from trunk

267. By Dave Cheney on 2012-07-09

gofmt

266. By Dave Cheney on 2012-07-09

responding to review feedback

265. By Dave Cheney on 2012-07-09

merge from trunk

264. By Dave Cheney on 2012-06-26

tweaks

=== added directory 'worker/firewaller'
=== added file 'worker/firewaller/export_test.go'
--- worker/firewaller/export_test.go	1970-01-01 00:00:00 +0000
+++ worker/firewaller/export_test.go	2012-07-12 23:19:42 +0000
@@ -0,0 +1,7 @@
+package firewaller
+
+// exported so we can manually close the Firewallers underlying
+// state connection.
+func (f *Firewaller) CloseState() error {
+	return f.st.Close()
+}

=== added file 'worker/firewaller/firewaller.go'
--- worker/firewaller/firewaller.go	1970-01-01 00:00:00 +0000
+++ worker/firewaller/firewaller.go	2012-07-12 23:19:42 +0000
@@ -0,0 +1,72 @@
+package firewaller
+
+import (
+	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/log"
+	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/watcher"
+	"launchpad.net/tomb"
+)
+
+type Firewaller struct {
+	st      *state.State
+	info    *state.Info
+	environ environs.Environ
+	tomb    tomb.Tomb
+}
+
+// NewFirewaller returns a Firewaller.
+func NewFirewaller(st *state.State) (*Firewaller, error) {
+	f := &Firewaller{
+		st: st,
+	}
+	go f.loop()
+	return f, nil
+}
+
+func (f *Firewaller) loop() {
+	defer f.tomb.Done()
+	defer f.st.Close()
+	environWatcher := f.st.WatchEnvironConfig()
+	defer watcher.Stop(environWatcher, &f.tomb)
+
+refreshState:
+	for {
+		select {
+		case <-f.tomb.Dying():
+			return
+		case config, ok := <-environWatcher.Changes():
+			if !ok {
+				f.tomb.Kill(watcher.MustErr(environWatcher))
+				return
+			}
+			var err error
+			f.environ, err = environs.NewEnviron(config.Map())
+			if err != nil {
+				log.Printf("firewaller loaded invalid environment configuration: %v", err)
+				continue
+			}
+			log.Printf("firewaller loaded new environment configuration")
+			break refreshState
+		}
+	}
+
+	// TODO watch all machines for open/close port actions
+	for {
+		select {
+		case <-f.tomb.Dying():
+			return
+		}
+	}
+}
+
+// Wait waits for the Firewaller to exit.
+func (f *Firewaller) Wait() error {
+	return f.tomb.Wait()
+}
+
+// Stop stops the Firewaller and returns any error encountered while stopping.
+func (f *Firewaller) Stop() error {
+	f.tomb.Kill(nil)
+	return f.tomb.Wait()
+}

=== added file 'worker/firewaller/firewaller_test.go'
--- worker/firewaller/firewaller_test.go	1970-01-01 00:00:00 +0000
+++ worker/firewaller/firewaller_test.go	2012-07-12 23:19:42 +0000
@@ -0,0 +1,115 @@
+package firewaller_test
+
+import (
+	. "launchpad.net/gocheck"
+	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/environs/dummy"
+	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/testing"
+	coretesting "launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/worker/firewaller"
+	stdtesting "testing"
+)
+
+func TestPackage(t *stdtesting.T) {
+	coretesting.ZkTestPackage(t)
+}
+
+type FirewallerSuite struct {
+	coretesting.LoggingSuite
+	testing.StateSuite
+	op <-chan dummy.Operation
+}
+
+func (s *FirewallerSuite) SetUpTest(c *C) {
+	// Create the operations channel with more than enough space
+	// for those tests that don't listen on it.
+	op := make(chan dummy.Operation, 500)
+	dummy.Listen(op)
+	s.op = op
+
+	env, err := environs.NewEnviron(map[string]interface{}{
+		"type":      "dummy",
+		"zookeeper": true,
+		"name":      "testing",
+	})
+	c.Assert(err, IsNil)
+	err = env.Bootstrap(false)
+	c.Assert(err, IsNil)
+
+	// Sanity check
+	info, err := env.StateInfo()
+	c.Assert(err, IsNil)
+	c.Assert(info, DeepEquals, s.StateInfo(c))
+
+	s.StateSuite.SetUpTest(c)
+}
+
+func (s *FirewallerSuite) TearDownTest(c *C) {
+	dummy.Reset()
+	s.StateSuite.TearDownTest(c)
+	s.LoggingSuite.TearDownTest(c)
+}
+
+var _ = Suite(&FirewallerSuite{})
+
+// invalidateEnvironment alters the environment configuration
+// so the ConfigNode returned from the watcher will not pass
+// validation.
+func (s *FirewallerSuite) invalidateEnvironment() error {
+	env, err := s.State.EnvironConfig()
+	if err != nil {
+		return err
+	}
+	env.Set("name", 1)
+	_, err = env.Write()
+	return err
+}
+
+// fixEnvironment undoes the work of invalidateEnvironment.
+func (s *FirewallerSuite) fixEnvironment() error {
+	env, err := s.State.EnvironConfig()
+	if err != nil {
+		return err
+	}
+	env.Set("name", "testing")
+	_, err = env.Write()
+	return err
+}
+
+func (s *FirewallerSuite) newState(c *C) *state.State {
+	st, err := state.Open(s.StateInfo(c))
+	c.Assert(err, IsNil)
+	return st
+}
+
+func stopFirewaller(c *C, f *firewaller.Firewaller) {
+	c.Assert(f.Stop(), IsNil)
+}
+
+func (s *FirewallerSuite) TestFirewallerStartStop(c *C) {
+	f, err := firewaller.NewFirewaller(s.newState(c))
+	c.Assert(err, IsNil)
+	stopFirewaller(c, f)
+}
+
+func (s *FirewallerSuite) TestFirewallerEnvironmentChange(c *C) {
+	f, err := firewaller.NewFirewaller(s.newState(c))
+	c.Assert(err, IsNil)
+	defer stopFirewaller(c, f)
+	err = s.invalidateEnvironment()
+	c.Assert(err, IsNil)
+	err = s.fixEnvironment()
+	c.Assert(err, IsNil)
+}
+
+func (s *FirewallerSuite) TestFirewallerStopOnStateClose(c *C) {
+	f, err := firewaller.NewFirewaller(s.newState(c))
+	c.Assert(err, IsNil)
+
+	f.CloseState()
+
+	// must use Check to avoid leaking firewallers.
+	c.Check(f.Wait(), ErrorMatches, ".* zookeeper is closing")
+	c.Assert(f.Stop(), ErrorMatches, ".* zookeeper is closing")
+}