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

[Frank Mueller]

firewaller: added port counter for global mode

For the global mode the firewaller now counts the
number of port usages and only opens a port once and
closes it when its usage is closed the last time.
Also found that the dummy provider not yet handles
the global mode (one test failed) and fixed that.

https://codereview.appspot.com/6635043/

[Roger Peppe (rogpeppe)]

looks good. a few remarks below.

https://codereview.appspot.com/6635043/diff/1/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6635043/diff/1/environs/dummy/environs.go#newcode590
environs/dummy/environs.go:590: if inst.defaultFirewall() {
i think this would be clearer (and easier to change if we ever add
modes) as:

switch inst.e.Config().FirewallMode() {
case config.FwDefault:
    ....
case config.FwGlobal:
    ....
default:
    panic("unknown mode")
}

FWIW, i think config.FwDefault is not a great name. FwPerInstance would
make it clearer what the default behaviour is intended to be.

https://codereview.appspot.com/6635043/diff/1/environs/dummy/environs.go#newcode618
environs/dummy/environs.go:618: if inst.defaultFirewall() {
ditto

https://codereview.appspot.com/6635043/diff/1/environs/dummy/environs.go#newcode640
environs/dummy/environs.go:640: if inst.defaultFirewall() {
ditto

https://codereview.appspot.com/6635043/diff/1/environs/dummy/environs.go#newcode656
environs/dummy/environs.go:656: func (inst *instance) defaultFirewall()
bool {
i'm not sure this method pulls its weight.

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

https://codereview.appspot.com/6635043/diff/1/worker/firewaller/firewaller.go#newcode214
worker/firewaller/firewaller.go:214: // already open (for opening) and
which are still needed (for closing).
// filterGlobalPorts returns the ports that actually need
// opening and closing, given the ports that have been opened
// and closed on a particular machine.

?

https://codereview.appspot.com/6635043/diff/1/worker/firewaller/firewaller.go#newcode220
worker/firewaller/firewaller.go:220: openOut = []state.Port{}
d

https://codereview.appspot.com/6635043/diff/1/worker/firewaller/firewaller.go#newcode221
worker/firewaller/firewaller.go:221: closeOut = []state.Port{}
d

https://codereview.appspot.com/6635043/diff/1/worker/firewaller/firewaller.go#newcode224
worker/firewaller/firewaller.go:224: // Open only the first one.
// The port is not already open.

?

https://codereview.appspot.com/6635043/diff/1/worker/firewaller/firewaller.go#newcode230
worker/firewaller/firewaller.go:230: if fw.globalPorts[port] == 1 {
i'd prefer to see this after the decrement, so it's obvious we're
looking for zero.

https://codereview.appspot.com/6635043/diff/1/worker/firewaller/firewaller.go#newcode231
worker/firewaller/firewaller.go:231: // Close only the last one.
// The last reference to the port is gone,
// so close the port globally.

https://codereview.appspot.com/6635043/

[Gustavo Niemeyer (niemeyer)]

As we talked online, the fix in dummy should be split out from this CL.
Also, can we talk a bit about the firewaller change? I don't understand
how the proposed change will solve the described issues.

https://codereview.appspot.com/6635043/

[Gustavo Niemeyer (niemeyer)]

https://codereview.appspot.com/6635043/diff/1/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6635043/diff/1/environs/dummy/environs.go#newcode590
environs/dummy/environs.go:590: if inst.defaultFirewall() {
On 2012/10/08 16:10:36, rog wrote:
> i think this would be clearer (and easier to change if we ever add
modes) as:

> switch inst.e.Config().FirewallMode() {
> case config.FwDefault:
> ....
> case config.FwGlobal:
> ....
> default:
> panic("unknown mode")
> }

> FWIW, i think config.FwDefault is not a great name. FwPerInstance
would make it
> clearer what the default behaviour is intended to be.

The reason for FwDefault is that different environments may only be able
to support a given kind of firewall. PerInstance, for example, is not
entirely true. It's per security group, and that mode is unsupported by
MaaS.

https://codereview.appspot.com/6635043/

[Gustavo Niemeyer (niemeyer)]

We can't check for "default" within the firewaller, since the default
for different environments may be different (which is the reason why we
name it "default" rather than the actual behavior). Also, we have to
consider how to handle firewall restarts. The way it currently works,
depending on a full-blown initialization of all the firewall ports,
isn't going to scale (100k open/close ports on every restart.. sounds
bad).

https://codereview.appspot.com/6635043/

Unmerged revisions

628. By Frank Mueller on 2012-10-08

firewaller: added port counter for global mode

627. By Roger Peppe on 2012-10-05

trivial: add PasswordHash and RandomBytes

R=niemeyer
CC=
https://codereview.appspot.com/6615047

626. By Roger Peppe on 2012-10-05

cmd/jujud: make bootstrap-state accept password

This makes bootstrap-state accept all the same flags
as the agents (including --data-dir which it doesn't use;
but it seems to me that it may well use it in the future).

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6623047

625. By Roger Peppe on 2012-10-05

state: make SetAdminPassword("") idempotent

R=TheMue, niemeyer
CC=
https://codereview.appspot.com/6622047

624. By Frank Mueller on 2012-10-05

ec2: integrated firewall mode configuration

After the adding of a configuration switch for default
or global firewall mode this behavior is now integrated
in EC2. If the mode is "global", then instead of a group
per machine one group "juju-<name>-global" is created
additionally to the juju group. All machines share this
global group then and opening and closing ports on it
so effect all machines.

R=niemeyer
CC=
https://codereview.appspot.com/6589073

623. By Dave Cheney on 2012-10-05

cmd: add tests for --format=nnn

Add checks for --format=nnn flag parsing.

This will break for people who have not updated gnuflag to the latest version, this is a good thing.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6601056

622. By William Reade on 2012-10-04

uniter: integrate filter type

By tweaking the filter slightly, to provide more carefully-tailored events,
the uniter Modes have been radically simplified.

Note that state.Unit was previously mis-specified, and is now fixed: it's
fine (in fact, potentially necessary) to resolve errors when the unit is
Dying.

Also note that there are roughly 50% more full Uniter tests than the
previous branch, but they run in roughly the same amount of time. Win!
But! This is actually a significant slowdown, because I fixed a bunch of
happy-path 500ms waits in the uniter tests. It's true that this variant does
more unnecessary work in the service of simpler Mode code, and I haven't been
able to find any obvious hotspots (apart from the pre-existing one) so if
reviewers would bear subtle performance implications in mind I would be most
grateful :).

R=rog, niemeyer
CC=
https://codereview.appspot.com/6588053

621. By Dave Cheney on 2012-10-04

cmd/jujuc: config-get: return default values

As discussed on IRC, this proposal is not the solution to the complex issue of versioned configuration settings. It is not even a start. However with this patch in place I am able to get charms running on ec2 and fix integration issues.

R=niemeyer
CC=
https://codereview.appspot.com/6591080

620. By William Reade on 2012-10-04

jujuc: move from cmd/jujuc/server

as discussed on IRC

R=niemeyer
CC=
https://codereview.appspot.com/6607043

619. By Roger Peppe on 2012-10-04

environs/config: add admin-secret attribute

R=niemeyer
CC=
https://codereview.appspot.com/6587085

=== modified file 'environs/dummy/environs.go'
--- environs/dummy/environs.go	2012-09-21 11:45:08 +0000
+++ environs/dummy/environs.go	2012-10-08 15:45:24 +0000
@@ -107,6 +107,7 @@
 	maxId         int // maximum instance id allocated so far.
 	insts         map[string]*instance
 	ports         map[int]map[state.Port]bool
+	globalPorts   map[state.Port]bool
 	bootstrapped  bool
 	storageDelay  time.Duration
 	storage       *storage
@@ -175,10 +176,11 @@
 // storage requests.
 func newState(name string, ops chan<- Operation) *environState {
 	s := &environState{
-		name:  name,
-		ops:   ops,
-		insts: make(map[string]*instance),
-		ports: make(map[int]map[state.Port]bool),
+		name:        name,
+		ops:         ops,
+		insts:       make(map[string]*instance),
+		ports:       make(map[int]map[state.Port]bool),
+		globalPorts: make(map[state.Port]bool),
 	}
 	s.storage = newStorage(s, "/"+name+"/private")
 	s.publicStorage = newStorage(s, "/"+name+"/public")
@@ -472,6 +474,7 @@
 		return nil, fmt.Errorf("cannot find image for %s-%s", tools.Series, tools.Arch)
 	}
 	i := &instance{
+		e:         e,
 		state:     e.state,
 		id:        fmt.Sprintf("%s-%d", e.state.name, e.state.maxId),
 		ports:     make(map[state.Port]bool),
@@ -550,6 +553,7 @@
 }
 
 type instance struct {
+	e         *environ
 	state     *environState
 	ports     map[state.Port]bool
 	id        string
@@ -583,8 +587,17 @@
 		InstanceId: inst.Id(),
 		Ports:      ports,
 	}
-	for _, p := range ports {
-		inst.ports[p] = true
+	if inst.defaultFirewall() {
+		// Default: Open per machine.
+		for _, p := range ports {
+			inst.ports[p] = true
+		}
+
+	} else {
+		// Global: Open globally.
+		for _, p := range ports {
+			inst.state.globalPorts[p] = true
+		}
 	}
 	return nil
 }
@@ -602,8 +615,17 @@
 		InstanceId: inst.Id(),
 		Ports:      ports,
 	}
-	for _, p := range ports {
-		delete(inst.ports, p)
+	if inst.defaultFirewall() {
+		// Default: Close per machine.
+		for _, p := range ports {
+			delete(inst.ports, p)
+		}
+
+	} else {
+		// Global: Close globally.
+		for _, p := range ports {
+			delete(inst.state.globalPorts, p)
+		}
 	}
 	return nil
 }
@@ -615,13 +637,26 @@
 	}
 	inst.state.mu.Lock()
 	defer inst.state.mu.Unlock()
-	for p := range inst.ports {
-		ports = append(ports, p)
+	if inst.defaultFirewall() {
+		// Default: Return local ports.
+		for p := range inst.ports {
+			ports = append(ports, p)
+		}
+	} else {
+		// Global: Return global ports.
+		for p := range inst.state.globalPorts {
+			ports = append(ports, p)
+		}
 	}
 	state.SortPorts(ports)
 	return
 }
 
+// defaultFirewall tests, if the firewall mode is default.
+func (inst *instance) defaultFirewall() bool {
+	return inst.e.Config().FirewallMode() == config.FwDefault
+}
+
 // providerDelay controls the delay before dummy responds.
 // non empty values in JUJU_DUMMY_DELAY will be parsed as 
 // time.Durations into this value.

=== modified file 'worker/firewaller/firewaller.go'
--- worker/firewaller/firewaller.go	2012-10-01 12:29:05 +0000
+++ worker/firewaller/firewaller.go	2012-10-08 15:45:24 +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"
@@ -22,6 +23,7 @@
 	unitsChange     chan *unitsChange
 	unitds          map[string]*unitData
 	portsChange     chan *portsChange
+	globalPorts     map[state.Port]int
 	serviceds       map[string]*serviceData
 	exposedChange   chan *exposedChange
 }
@@ -36,6 +38,7 @@
 		unitsChange:     make(chan *unitsChange),
 		unitds:          make(map[string]*unitData),
 		portsChange:     make(chan *portsChange),
+		globalPorts:     make(map[state.Port]int),
 		serviceds:       make(map[string]*serviceData),
 		exposedChange:   make(chan *exposedChange),
 	}
@@ -160,6 +163,7 @@
 	}
 	toOpen := diff(want, machined.ports)
 	toClose := diff(machined.ports, want)
+	toOpen, toClose = fw.filterGlobalPorts(toOpen, toClose)
 	machined.ports = want
 
 	// If there's nothing to do, do nothing.
@@ -206,6 +210,33 @@
 	return nil
 }
 
+// filterGlobalPorts checks in case of the global firewall mode, which ports are
+// already open (for opening) and which are still needed (for closing).
+func (fw *Firewaller) filterGlobalPorts(openIn, closeIn []state.Port) (openOut, closeOut []state.Port) {
+	if fw.environ.Config().FirewallMode() == config.FwDefault {
+		return openIn, closeIn
+	}
+	// Global mode, so filter and count.
+	openOut = []state.Port{}
+	closeOut = []state.Port{}
+	for _, port := range openIn {
+		if fw.globalPorts[port] == 0 {
+			// Open only the first one.
+			openOut = append(openOut, port)
+		}
+		fw.globalPorts[port]++
+	}
+	for _, port := range closeIn {
+		if fw.globalPorts[port] == 1 {
+			// Close only the last one.
+			closeOut = append(closeOut, port)
+			delete(fw.globalPorts, port)
+		}
+		fw.globalPorts[port]--
+	}
+	return
+}
+
 // machineLifeChanged starts watching new machines when the firewaller
 // is starting, or when new machines come to life, and stops watching
 // machines that are dying.

=== modified file 'worker/firewaller/firewaller_test.go'
--- worker/firewaller/firewaller_test.go	2012-09-28 17:40:29 +0000
+++ worker/firewaller/firewaller_test.go	2012-10-08 15:45:24 +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"
@@ -482,3 +483,74 @@
 	err = s.State.RemoveMachine(m.Id())
 	c.Assert(err, IsNil)
 }
+
+func (s *FirewallerSuite) TestGlobalMode(c *C) {
+	// Change configuration.
+	oldConfig := s.Conn.Environ.Config()
+	defer func() {
+		err := s.Conn.Environ.SetConfig(oldConfig)
+		c.Assert(err, IsNil)
+		err = s.State.SetEnvironConfig(oldConfig)
+		c.Assert(err, IsNil)
+	}()
+
+	attrs := s.Conn.Environ.Config().AllAttrs()
+	attrs["firewall-mode"] = config.FwGlobal
+	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)
+
+	// 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)
+	inst1 := 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)
+	inst2 := s.startInstance(c, m2)
+	err = u2.OpenPort("tcp", 80)
+	c.Assert(err, IsNil)
+
+	// Check that both machines have the same open ports.
+	s.assertPorts(c, inst1, m1.Id(), []state.Port{{"tcp", 80}, {"tcp", 8080}})
+	s.assertPorts(c, inst2, m2.Id(), []state.Port{{"tcp", 80}, {"tcp", 8080}})
+
+	// Check that closing a multiple used port on one machine lets it untouched.
+	err = u1.ClosePort("tcp", 80)
+	c.Assert(err, IsNil)
+
+	s.assertPorts(c, inst1, m1.Id(), []state.Port{{"tcp", 80}, {"tcp", 8080}})
+	s.assertPorts(c, inst2, m2.Id(), []state.Port{{"tcp", 80}, {"tcp", 8080}})
+
+	// Check that closing a single port effects all machines.
+	err = u1.ClosePort("tcp", 8080)
+	c.Assert(err, IsNil)
+
+	s.assertPorts(c, inst1, m1.Id(), []state.Port{{"tcp", 80}})
+	s.assertPorts(c, inst2, m2.Id(), []state.Port{{"tcp", 80}})
+
+	// Check that closing the last usage of a port closes it globally.
+	err = u2.ClosePort("tcp", 80)
+	c.Assert(err, IsNil)
+
+	s.assertPorts(c, inst1, m1.Id(), nil)
+	s.assertPorts(c, inst2, m2.Id(), nil)
+}