Merge lp:~themue/juju-core/go-firewaller-added-global-mode into lp:~juju/juju-core/trunk

[Frank Mueller]

firewaller: integrated global mode

Firewaller now recognizes the global mode. It dies a
port usage counting usage counting and opens each
used port only once and closes it after the last using unit
has gone. The handling of ports in case of a restart has
been removed from this CL to a later one.

https://codereview.appspot.com/6713054/

[Gustavo Niemeyer (niemeyer)]

Needs some love.

https://codereview.appspot.com/6713054/diff/1/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6713054/diff/1/worker/firewaller/firewaller.go#newcode195
worker/firewaller/firewaller.go:195: m, err := machined.machine()
Why do we need the machine, and the instance? It's feeling somewhat
hackish. If it's in global mode, it should be handed off to a method
that deals with global mode *only* in some sensible point, rather than
interleaving unnecessary logic like that.

https://codereview.appspot.com/6713054/diff/1/worker/firewaller/firewaller.go#newcode252
worker/firewaller/firewaller.go:252: if fw.initialPorts[port] {
It's not clear to me how initialPorts is working. What if there are open
ports that should not be? What if there are initialPorts that were
closed and need to be re-opened?

The overall approach is asking for some tasteful consideration.

https://codereview.appspot.com/6713054/

[Gustavo Niemeyer (niemeyer)]

Much better thanks.

Still, what about ports previously opened?

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller.go#newcode177
worker/firewaller/firewaller.go:177: // flushGlobalPorts opens and
closes ports global in the environment.
// flushGlobalPorts opens and closes global ports in the environment.
// It keeps a reference count for ports so that only 0-to-1 and 1-to-0
events
// modify the environment.

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller.go#newcode183
worker/firewaller/firewaller.go:183: // The port is not already open.
Please drop. Code is shorter and obvious.

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller.go#newcode192
worker/firewaller/firewaller.go:192: // so close the port globally.
Same.

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller_test.go
File worker/firewaller/firewaller_test.go (right):

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller_test.go#newcode563
worker/firewaller/firewaller_test.go:563: // Check that all expected
ports are open in environment.
Please drop comment ("assertEnvironPorts" is clear).

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller_test.go#newcode566
worker/firewaller/firewaller_test.go:566: // Check that closing a
multiple used port on one machine lets it untouched.
// Closing a port opened by a different unit won't touch the
environment.

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller_test.go#newcode571
worker/firewaller/firewaller_test.go:571: // Check that closing a single
used port is closed.
// Closing a port used just once changes the environment.

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller_test.go#newcode576
worker/firewaller/firewaller_test.go:576: // Check that closing the last
usage of a port closes it globally.
// Closing the last port also modifies the environment.

https://codereview.appspot.com/6713054/

[Gustavo Niemeyer (niemeyer)]

https://codereview.appspot.com/6713054/diff/10002/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6713054/diff/10002/worker/firewaller/firewaller.go#newcode70
worker/firewaller/firewaller.go:70: err = fw.environ.ClosePorts(ports)
Frank, what happens if you close ports when stuff is running?

Some out-of-the-box thinking is needed. If we have to discuss trivial
stuff repeatedly, there's an inflection point where it's easier to
implement than to review.

https://codereview.appspot.com/6713054/

[Gustavo Niemeyer (niemeyer)]

On Fri, Oct 19, 2012 at 12:24 PM, <email address hidden> wrote:
> If it's trivial and you already have a concept in mind I don't see
> please let me know, I really appreciate it.
(...)
> I removed my initial concept which relied on the retrieved initial open
> ports from the provider and still does a correct port counting later

Your initial concept had the problems I described. Your new concept
has the problems I described. Both are issues I consider obvious
(closing ports that should be open while services are running!?).

What you have is:

1) Ports that are known open in the provider
2) Simple ways to compute what you need open
3) Simple ways to close and open at will

I cannot break down this problem for you further without implementing
the solution myself, which gets back to the point I raised: there's an
inflection point where reviews are counterproductive if compared to
implementing it myself and asking other people to review it.

Please think about the problem. If you cannot solve it, let's talk
next week and see what kind of problem you'd appreciate working on
instead.

[Gustavo Niemeyer (niemeyer)]

https://codereview.appspot.com/6713054/diff/14001/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6713054/diff/14001/worker/firewaller/firewaller.go#newcode191
worker/firewaller/firewaller.go:191: // Reset port counter, will be set
during gathering of the units.
Why? Doesn't that mean we'll run OpenPort on the environment for every
single port that we just confirmed is open?

https://codereview.appspot.com/6713054/

[Gustavo Niemeyer (niemeyer)]

LGTM, assuming that this works with real usage.

https://codereview.appspot.com/6713054/diff/9002/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6713054/diff/9002/worker/firewaller/firewaller.go#newcode30
worker/firewaller/firewaller.go:30: globalPortsCount map[state.Port]int
s/PortsFlag/PortOpen/
s/PortsCount/PortRef/

https://codereview.appspot.com/6713054/

=== modified file 'worker/firewaller/firewaller.go'
--- worker/firewaller/firewaller.go	2012-10-01 12:29:05 +0000
+++ worker/firewaller/firewaller.go	2012-10-22 15:47:21 +0000
@@ -3,6 +3,7 @@
 import (
 	"fmt"
 	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/log"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/watcher"
@@ -13,17 +14,20 @@
 // Firewaller watches the state for ports opened or closed
 // and reflects those changes onto the backing environment.
 type Firewaller struct {
-	tomb            tomb.Tomb
-	st              *state.State
-	environ         environs.Environ
-	environWatcher  *state.EnvironConfigWatcher
-	machinesWatcher *state.MachinesWatcher
-	machineds       map[int]*machineData
-	unitsChange     chan *unitsChange
-	unitds          map[string]*unitData
-	portsChange     chan *portsChange
-	serviceds       map[string]*serviceData
-	exposedChange   chan *exposedChange
+	tomb             tomb.Tomb
+	st               *state.State
+	environ          environs.Environ
+	environWatcher   *state.EnvironConfigWatcher
+	machinesWatcher  *state.MachinesWatcher
+	machineds        map[int]*machineData
+	unitsChange      chan *unitsChange
+	unitds           map[string]*unitData
+	portsChange      chan *portsChange
+	serviceds        map[string]*serviceData
+	exposedChange    chan *exposedChange
+	globalMode       bool
+	globalPortsFlag  map[state.Port]bool
+	globalPortsCount map[state.Port]int
 }
 
 // NewFirewaller returns a new Firewaller.
@@ -54,6 +58,11 @@
 	if err != nil {
 		return err
 	}
+	if fw.environ.Config().FirewallMode() == config.FwGlobal {
+		if err := fw.initGlobalMode(); err != nil {
+			return err
+		}
+	}
 	for {
 		select {
 		case <-fw.tomb.Dying():
@@ -129,6 +138,61 @@
 	panic("not reached")
 }
 
+// initGlobalMode retrieves the ports that need to be open globally,
+// opens them and also closes not needed open ports left from an
+// earlier run.
+func (fw *Firewaller) initGlobalMode() error {
+	fw.globalMode = true
+	fw.globalPortsFlag = make(map[state.Port]bool)
+	fw.globalPortsCount = make(map[state.Port]int)
+	initialPorts, err := fw.environ.Ports()
+	if err != nil {
+		return err
+	}
+	// Retrieve ports to be open from state.
+	services, err := fw.st.AllServices()
+	if err != nil {
+		return err
+	}
+	for _, service := range services {
+		if !service.IsExposed() {
+			continue
+		}
+		units, err := service.AllUnits()
+		if err != nil {
+			return err
+		}
+		for _, unit := range units {
+			ports := unit.OpenedPorts()
+			for _, port := range ports {
+				fw.globalPortsFlag[port] = true
+			}
+		}
+	}
+	openedPorts := []state.Port{}
+	for port := range fw.globalPortsFlag {
+		openedPorts = append(openedPorts, port)
+	}
+	// Check which ports to open or to close.
+	toOpen := diff(openedPorts, initialPorts)
+	toClose := diff(initialPorts, openedPorts)
+	if len(toOpen) > 0 {
+		if err := fw.environ.OpenPorts(toOpen); err != nil {
+			return err
+		}
+		state.SortPorts(toOpen)
+		log.Printf("firewaller: initially opened ports %v in environment", toOpen)
+	}
+	if len(toClose) > 0 {
+		if err := fw.environ.ClosePorts(toClose); err != nil {
+			return err
+		}
+		state.SortPorts(toClose)
+		log.Printf("firewaller: initially closed ports %v in environment", toClose)
+	}
+	return nil
+}
+
 // flushUnits opens and closes ports for the passed unit data.
 func (fw *Firewaller) flushUnits(unitds []*unitData) error {
 	machineds := map[int]*machineData{}
@@ -161,7 +225,55 @@
 	toOpen := diff(want, machined.ports)
 	toClose := diff(machined.ports, want)
 	machined.ports = want
-
+	if fw.globalMode {
+		return fw.flushGlobalPorts(toOpen, toClose)
+	}
+	return fw.flushInstancePorts(machined, toOpen, toClose)
+}
+
+// flushGlobalPorts opens and closes global ports in the environment.
+// It keeps a reference count for ports so that only 0-to-1 and 1-to-0 events
+// modify the environment.
+func (fw *Firewaller) flushGlobalPorts(rawOpen, rawClose []state.Port) error {
+	// Filter which ports are really to open or close.
+	var toOpen, toClose []state.Port
+	for _, port := range rawOpen {
+		if !fw.globalPortsFlag[port] {
+			toOpen = append(toOpen, port)
+			fw.globalPortsFlag[port] = true
+		}
+		fw.globalPortsCount[port]++
+	}
+	for _, port := range rawClose {
+		fw.globalPortsCount[port]--
+		if fw.globalPortsCount[port] == 0 {
+			toClose = append(toClose, port)
+			delete(fw.globalPortsFlag, port)
+			delete(fw.globalPortsCount, port)
+		}
+	}
+	// Open and close the ports.
+	if len(toOpen) > 0 {
+		if err := fw.environ.OpenPorts(toOpen); err != nil {
+			// TODO(mue) Add local retry logic.
+			return err
+		}
+		state.SortPorts(toOpen)
+		log.Printf("firewaller: opened ports %v in environment", toOpen)
+	}
+	if len(toClose) > 0 {
+		if err := fw.environ.ClosePorts(toClose); err != nil {
+			// TODO(mue) Add local retry logic.
+			return err
+		}
+		state.SortPorts(toClose)
+		log.Printf("firewaller: closed ports %v in environment", toClose)
+	}
+	return nil
+}
+
+// flushGlobalPorts opens and closes ports global on the machine.
+func (fw *Firewaller) flushInstancePorts(machined *machineData, toOpen, toClose []state.Port) error {
 	// If there's nothing to do, do nothing.
 	// This is important because when a machine is first created,
 	// it will have no instance id but also no open ports -
@@ -169,7 +281,6 @@
 	if len(toOpen) == 0 && len(toClose) == 0 {
 		return nil
 	}
-	// Open and close the ports.
 	m, err := machined.machine()
 	if state.IsNotFound(err) {
 		return nil
@@ -185,9 +296,9 @@
 	if err != nil {
 		return err
 	}
+	// Open and close the ports.
 	if len(toOpen) > 0 {
-		err = instances[0].OpenPorts(machined.id, toOpen)
-		if err != nil {
+		if err := instances[0].OpenPorts(machined.id, toOpen); err != nil {
 			// TODO(mue) Add local retry logic.
 			return err
 		}
@@ -195,8 +306,7 @@
 		log.Printf("firewaller: opened ports %v on machine %d", toOpen, machined.id)
 	}
 	if len(toClose) > 0 {
-		err = instances[0].ClosePorts(machined.id, toClose)
-		if err != nil {
+		if err := instances[0].ClosePorts(machined.id, toClose); err != nil {
 			// TODO(mue) Add local retry logic.
 			return err
 		}
 
=== modified file 'worker/firewaller/firewaller_test.go'
--- worker/firewaller/firewaller_test.go	2012-10-09 08:18:40 +0000
+++ worker/firewaller/firewaller_test.go	2012-10-22 15:47:21 +0000
@@ -3,6 +3,7 @@
 import (
 	. "launchpad.net/gocheck"
 	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/environs/dummy"
 	"launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/state"
@@ -49,6 +50,32 @@
 	panic("unreachable")
 }
 
+// assertEnvironPorts retrieves the open ports of environment and compares them
+// to the expected. 
+func (s *FirewallerSuite) assertEnvironPorts(c *C, expected []state.Port) {
+	s.State.StartSync()
+	start := time.Now()
+	for {
+		got, err := s.Conn.Environ.Ports()
+		if err != nil {
+			c.Fatal(err)
+			return
+		}
+		state.SortPorts(got)
+		state.SortPorts(expected)
+		if reflect.DeepEqual(got, expected) {
+			c.Succeed()
+			return
+		}
+		if time.Since(start) > 5*time.Second {
+			c.Fatalf("timed out: expected %q; got %q", expected, got)
+			return
+		}
+		time.Sleep(50 * time.Millisecond)
+	}
+	panic("unreachable")
+}
+
 var _ = Suite(&FirewallerSuite{})
 
 func (s *FirewallerSuite) SetUpTest(c *C) {
@@ -72,6 +99,32 @@
 	return u, m
 }
 
+func (s *FirewallerSuite) setGlobalMode(c *C) func(*C) {
+	oldConfig := s.Conn.Environ.Config()
+	restore := func(rc *C) {
+		attrs := oldConfig.AllAttrs()
+		attrs["admin-secret"] = ""
+		oldConfig, err := oldConfig.Apply(attrs)
+		rc.Assert(err, IsNil)
+		err = s.Conn.Environ.SetConfig(oldConfig)
+		rc.Assert(err, IsNil)
+		err = s.State.SetEnvironConfig(oldConfig)
+		rc.Assert(err, IsNil)
+	}
+
+	attrs := s.Conn.Environ.Config().AllAttrs()
+	attrs["firewall-mode"] = config.FwGlobal
+	attrs["admin-secret"] = ""
+	newConfig, err := s.Conn.Environ.Config().Apply(attrs)
+	c.Assert(err, IsNil)
+	err = s.Conn.Environ.SetConfig(newConfig)
+	c.Assert(err, IsNil)
+	err = s.State.SetEnvironConfig(newConfig)
+	c.Assert(err, IsNil)
+
+	return restore
+}
+
 // startInstance starts a new instance for the given machine.
 func (s *FirewallerSuite) startInstance(c *C, m *state.Machine) environs.Instance {
 	inst, err := s.Conn.Environ.StartInstance(m.Id(), testing.InvalidStateInfo(m.Id()), nil)
@@ -482,3 +535,184 @@
 	err = s.State.RemoveMachine(m.Id())
 	c.Assert(err, IsNil)
 }
+
+func (s *FirewallerSuite) TestGlobalMode(c *C) {
+	// Change configuration.
+	restore := s.setGlobalMode(c)
+	defer restore(c)
+
+	// Start firewall and open ports.
+	fw := firewaller.NewFirewaller(s.State)
+	defer func() { c.Assert(fw.Stop(), IsNil) }()
+
+	svc1, err := s.State.AddService("wordpress", s.charm)
+	c.Assert(err, IsNil)
+	err = svc1.SetExposed()
+	c.Assert(err, IsNil)
+
+	u1, m1 := s.addUnit(c, svc1)
+	s.startInstance(c, m1)
+	err = u1.OpenPort("tcp", 80)
+	c.Assert(err, IsNil)
+	err = u1.OpenPort("tcp", 8080)
+	c.Assert(err, IsNil)
+
+	svc2, err := s.State.AddService("moinmoin", s.charm)
+	c.Assert(err, IsNil)
+	err = svc2.SetExposed()
+	c.Assert(err, IsNil)
+
+	u2, m2 := s.addUnit(c, svc2)
+	s.startInstance(c, m2)
+	err = u2.OpenPort("tcp", 80)
+	c.Assert(err, IsNil)
+
+	s.assertEnvironPorts(c, []state.Port{{"tcp", 80}, {"tcp", 8080}})
+
+	// Closing a port opened by a different unit won't touch the environment.
+	err = u1.ClosePort("tcp", 80)
+	c.Assert(err, IsNil)
+	s.assertEnvironPorts(c, []state.Port{{"tcp", 80}, {"tcp", 8080}})
+
+	// Closing a port used just once changes the environment.
+	err = u1.ClosePort("tcp", 8080)
+	c.Assert(err, IsNil)
+	s.assertEnvironPorts(c, []state.Port{{"tcp", 80}})
+
+	// Closing the last port also modifies the environment.
+	err = u2.ClosePort("tcp", 80)
+	c.Assert(err, IsNil)
+	s.assertEnvironPorts(c, nil)
+}
+
+func (s *FirewallerSuite) TestGlobalModeRestart(c *C) {
+	// Change configuration.
+	restore := s.setGlobalMode(c)
+	defer restore(c)
+
+	// Start firewall and open ports.
+	fw := firewaller.NewFirewaller(s.State)
+
+	svc, err := s.State.AddService("wordpress", s.charm)
+	c.Assert(err, IsNil)
+	err = svc.SetExposed()
+	c.Assert(err, IsNil)
+
+	u, m := s.addUnit(c, svc)
+	s.startInstance(c, m)
+	err = u.OpenPort("tcp", 80)
+	c.Assert(err, IsNil)
+	err = u.OpenPort("tcp", 8080)
+	c.Assert(err, IsNil)
+
+	s.assertEnvironPorts(c, []state.Port{{"tcp", 80}, {"tcp", 8080}})
+
+	// Stop firewall and close one and open a different port.
+	err = fw.Stop()
+	c.Assert(err, IsNil)
+
+	err = u.ClosePort("tcp", 8080)
+	c.Assert(err, IsNil)
+	err = u.OpenPort("tcp", 8888)
+	c.Assert(err, IsNil)
+
+	// Start firewall and check port.
+	fw = firewaller.NewFirewaller(s.State)
+	defer func() { c.Assert(fw.Stop(), IsNil) }()
+
+	s.assertEnvironPorts(c, []state.Port{{"tcp", 80}, {"tcp", 8888}})
+}
+
+func (s *FirewallerSuite) TestGlobalModeRestartUnexposedService(c *C) {
+	// Change configuration.
+	restore := s.setGlobalMode(c)
+	defer restore(c)
+
+	// Start firewall and open ports.
+	fw := firewaller.NewFirewaller(s.State)
+
+	svc, err := s.State.AddService("wordpress", s.charm)
+	c.Assert(err, IsNil)
+	err = svc.SetExposed()
+	c.Assert(err, IsNil)
+
+	u, m := s.addUnit(c, svc)
+	s.startInstance(c, m)
+	err = u.OpenPort("tcp", 80)
+	c.Assert(err, IsNil)
+	err = u.OpenPort("tcp", 8080)
+	c.Assert(err, IsNil)
+
+	s.assertEnvironPorts(c, []state.Port{{"tcp", 80}, {"tcp", 8080}})
+
+	// Stop firewall and clear exposed flag on service.
+	err = fw.Stop()
+	c.Assert(err, IsNil)
+
+	err = svc.ClearExposed()
+	c.Assert(err, IsNil)
+
+	// Start firewall and check port.
+	fw = firewaller.NewFirewaller(s.State)
+	defer func() { c.Assert(fw.Stop(), IsNil) }()
+
+	s.assertEnvironPorts(c, nil)
+}
+
+func (s *FirewallerSuite) TestGlobalModeRestartPortCount(c *C) {
+	// Change configuration.
+	restore := s.setGlobalMode(c)
+	defer restore(c)
+
+	// Start firewall and open ports.
+	fw := firewaller.NewFirewaller(s.State)
+
+	svc1, err := s.State.AddService("wordpress", s.charm)
+	c.Assert(err, IsNil)
+	err = svc1.SetExposed()
+	c.Assert(err, IsNil)
+
+	u1, m1 := s.addUnit(c, svc1)
+	s.startInstance(c, m1)
+	err = u1.OpenPort("tcp", 80)
+	c.Assert(err, IsNil)
+	err = u1.OpenPort("tcp", 8080)
+	c.Assert(err, IsNil)
+
+	s.assertEnvironPorts(c, []state.Port{{"tcp", 80}, {"tcp", 8080}})
+
+	// Stop firewall and add another service using the port.
+	err = fw.Stop()
+	c.Assert(err, IsNil)
+
+	svc2, err := s.State.AddService("moinmoin", s.charm)
+	c.Assert(err, IsNil)
+	err = svc2.SetExposed()
+	c.Assert(err, IsNil)
+
+	u2, m2 := s.addUnit(c, svc2)
+	s.startInstance(c, m2)
+	err = u2.OpenPort("tcp", 80)
+	c.Assert(err, IsNil)
+
+	// Start firewall and check port.
+	fw = firewaller.NewFirewaller(s.State)
+	defer func() { c.Assert(fw.Stop(), IsNil) }()
+
+	s.assertEnvironPorts(c, []state.Port{{"tcp", 80}, {"tcp", 8080}})
+
+	// Closing a port opened by a different unit won't touch the environment.
+	err = u1.ClosePort("tcp", 80)
+	c.Assert(err, IsNil)
+	s.assertEnvironPorts(c, []state.Port{{"tcp", 80}, {"tcp", 8080}})
+
+	// Closing a port used just once changes the environment.
+	err = u1.ClosePort("tcp", 8080)
+	c.Assert(err, IsNil)
+	s.assertEnvironPorts(c, []state.Port{{"tcp", 80}})
+
+	// Closing the last port also modifies the environment.
+	err = u2.ClosePort("tcp", 80)
+	c.Assert(err, IsNil)
+	s.assertEnvironPorts(c, nil)
+}