firewaller: added port counter for global mode

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://code.launchpad.net/~themue/juju-core/go-firewaller-global-mode/+merge/128529

(do not edit description out of merge proposal)

[TheMue, 2012-10-08 15:45:22]

Please take a look.

[rog, 2012-10-08 16:10:36]

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#newc...
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#newc...
environs/dummy/environs.go:618: if inst.defaultFirewall() {
ditto

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

https://codereview.appspot.com/6635043/diff/1/environs/dummy/environs.go#newc...
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...
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...
worker/firewaller/firewaller.go:220: openOut = []state.Port{}
d

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

https://codereview.appspot.com/6635043/diff/1/worker/firewaller/firewaller.go...
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...
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...
worker/firewaller/firewaller.go:231: // Close only the last one.
// The last reference to the port is gone,
// so close the port globally.

[niemeyer, 2012-10-08 16:33:11]

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.

[niemeyer, 2012-10-08 16:44:24]

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#newc...
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.

[niemeyer, 2012-10-08 17:39:01]

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).