firewaller: integrated global mode

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

(do not edit description out of merge proposal)

[TheMue, 2012-10-17 15:58:55]

Please take a look.

[niemeyer, 2012-10-18 00:54:13]

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

[TheMue, 2012-10-18 16:40:34]

Please take a look.

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...
worker/firewaller/firewaller.go:195: m, err := machined.machine()
On 2012/10/18 00:54:13, niemeyer wrote:
> 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.

Done.

[niemeyer, 2012-10-19 00:48:28]

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...
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...
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...
worker/firewaller/firewaller.go:192: // so close the port globally.
Same.

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

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller...
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...
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...
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...
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.

[TheMue, 2012-10-19 13:22:47]

Please take a look.

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...
worker/firewaller/firewaller.go:177: // flushGlobalPorts opens and closes ports
global in the environment.
On 2012/10/19 00:48:28, niemeyer wrote:
> // 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.

Done.

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:183: // The port is not already open.
On 2012/10/19 00:48:28, niemeyer wrote:
> Please drop. Code is shorter and obvious.

Done.

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:192: // so close the port globally.
On 2012/10/19 00:48:28, niemeyer wrote:
> Same.

Done.

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

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller...
worker/firewaller/firewaller_test.go:563: // Check that all expected ports are
open in environment.
On 2012/10/19 00:48:28, niemeyer wrote:
> Please drop comment ("assertEnvironPorts" is clear).

Done.

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller...
worker/firewaller/firewaller_test.go:566: // Check that closing a multiple used
port on one machine lets it untouched.
On 2012/10/19 00:48:28, niemeyer wrote:
> // Closing a port opened by a different unit won't touch the environment.

Done.

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller...
worker/firewaller/firewaller_test.go:571: // Check that closing a single used
port is closed.
On 2012/10/19 00:48:28, niemeyer wrote:
> // Closing a port used just once changes the environment.

Done.

https://codereview.appspot.com/6713054/diff/5001/worker/firewaller/firewaller...
worker/firewaller/firewaller_test.go:576: // Check that closing the last usage
of a port closes it globally.
On 2012/10/19 00:48:28, niemeyer wrote:
> // Closing the last port also modifies the environment.

Done.

[niemeyer, 2012-10-19 15:11:21]

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

https://codereview.appspot.com/6713054/diff/10002/worker/firewaller/firewalle...
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.

[TheMue, 2012-10-19 15:24:41]

On 2012/10/19 15:11:21, niemeyer wrote:
>
https://codereview.appspot.com/6713054/diff/10002/worker/firewaller/firewalle...
> File worker/firewaller/firewaller.go (right):
> 
>
https://codereview.appspot.com/6713054/diff/10002/worker/firewaller/firewalle...
> 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.

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 (opening real
only new ones, otherwise init the counter and removed the flag that the port
already has been open). The second propose removed it to handle this topic in a
separate CL (see changed description).

[niemeyer, 2012-10-19 16:47:31]

On Fri, Oct 19, 2012 at 12:24 PM,  <themue@gmail.com> 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.

[TheMue, 2012-10-22 12:15:17]

Please take a look.

[niemeyer, 2012-10-22 14:55:35]

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

https://codereview.appspot.com/6713054/diff/14001/worker/firewaller/firewalle...
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?

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

On 2012/10/22 14:55:35, niemeyer wrote:
>
https://codereview.appspot.com/6713054/diff/14001/worker/firewaller/firewalle...
> File worker/firewaller/firewaller.go (right):
> 
>
https://codereview.appspot.com/6713054/diff/14001/worker/firewaller/firewalle...
> 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?

Aargh, sorry, shit. :( Mixed two ideas I had in mind so that the counters have
the right value when all units had been gathered. Will fix and re-propose it.

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

Please take a look.

[niemeyer, 2012-10-29 09:28:06]

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...
worker/firewaller/firewaller.go:30: globalPortsCount map[state.Port]int
s/PortsFlag/PortOpen/
s/PortsCount/PortRef/

[TheMue, 2012-10-29 10:23:26]

*** Submitted:

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.

R=
CC=
https://codereview.appspot.com/6713054

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...
worker/firewaller/firewaller.go:30: globalPortsCount map[state.Port]int
On 2012/10/29 09:28:07, niemeyer wrote:
> s/PortsFlag/PortOpen/
> s/PortsCount/PortRef/

Done.