Merge lp:~themue/juju-core/go-environs-mode-errors into lp:~juju/juju-core/trunk

[Frank Mueller]

environs: added error in case of illegal mode

Changed OpenPorts/ClosePorts/Ports in dummy and ec2 to
check if the correct firewall mode is set. That is "global"
for operations on the environ and "instance" for operations
on instances.

https://codereview.appspot.com/6715048/

[Gustavo Niemeyer (niemeyer)]

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

https://codereview.appspot.com/6715048/diff/1/environs/dummy/environs.go#newcode494
environs/dummy/environs.go:494: env: e,
Doesn't look right. One environment state may be linked to from many
environment instances, and one instance is associated to one state.

Please have a look at the code you reverted to see how to do that
properly.

https://codereview.appspot.com/6715048/

[Gustavo Niemeyer (niemeyer)]

LGTM either way. It's not worth spending more time on this.

https://codereview.appspot.com/6715048/diff/5001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6715048/diff/5001/environs/dummy/environs.go#newcode498
environs/dummy/environs.go:498: firewallMode: e.Config().FirewallMode(),
That's not what was being done. The firewall mode is the same for all
instances, and all environments. Here is the diff for reference:

http://paste.ubuntu.com/1288133/

https://codereview.appspot.com/6715048/

=== modified file 'environs/dummy/environs.go'
--- environs/dummy/environs.go	2012-10-16 17:27:11 +0000
+++ environs/dummy/environs.go	2012-10-18 11:21:20 +0000
@@ -491,10 +491,11 @@
 		return nil, fmt.Errorf("cannot find image for %s-%s", tools.Series, tools.Arch)
 	}
 	i := &instance{
-		state:     e.state,
-		id:        fmt.Sprintf("%s-%d", e.state.name, e.state.maxId),
-		ports:     make(map[state.Port]bool),
-		machineId: machineId,
+		state:        e.state,
+		id:           fmt.Sprintf("%s-%d", e.state.name, e.state.maxId),
+		ports:        make(map[state.Port]bool),
+		machineId:    machineId,
+		firewallMode: e.Config().FirewallMode(),
 	}
 	e.state.insts[i.id] = i
 	e.state.maxId++
@@ -567,6 +568,10 @@
 func (e *environ) OpenPorts(ports []state.Port) error {
 	e.state.mu.Lock()
 	defer e.state.mu.Unlock()
+	if e.Config().FirewallMode() != config.FwGlobal {
+		return fmt.Errorf("invalid firewall mode for opening ports on environment: %q",
+			e.Config().FirewallMode())
+	}
 	for _, p := range ports {
 		e.state.ports[p] = true
 	}
@@ -576,6 +581,10 @@
 func (e *environ) ClosePorts(ports []state.Port) error {
 	e.state.mu.Lock()
 	defer e.state.mu.Unlock()
+	if e.Config().FirewallMode() != config.FwGlobal {
+		return fmt.Errorf("invalid firewall mode for closing ports on environment: %q",
+			e.Config().FirewallMode())
+	}
 	for _, p := range ports {
 		delete(e.state.ports, p)
 	}
@@ -585,6 +594,10 @@
 func (e *environ) Ports() (ports []state.Port, err error) {
 	e.state.mu.Lock()
 	defer e.state.mu.Unlock()
+	if e.Config().FirewallMode() != config.FwGlobal {
+		return nil, fmt.Errorf("invalid firewall mode for retrieving ports from environment: %q",
+			e.Config().FirewallMode())
+	}
 	for p := range e.state.ports {
 		ports = append(ports, p)
 	}
@@ -597,10 +610,11 @@
 }
 
 type instance struct {
-	state     *environState
-	ports     map[state.Port]bool
-	id        string
-	machineId int
+	state        *environState
+	ports        map[state.Port]bool
+	id           string
+	machineId    int
+	firewallMode config.FirewallMode
 }
 
 func (inst *instance) Id() string {
@@ -619,6 +633,10 @@
 func (inst *instance) OpenPorts(machineId int, ports []state.Port) error {
 	defer delay()
 	log.Printf("openPorts %d, %#v", machineId, ports)
+	if inst.firewallMode != config.FwInstance {
+		return fmt.Errorf("invalid firewall mode for opening ports on instance: %q",
+			inst.firewallMode)
+	}
 	if inst.machineId != machineId {
 		panic(fmt.Errorf("OpenPorts with mismatched machine id, expected %d got %d", inst.machineId, machineId))
 	}
@@ -638,6 +656,10 @@
 
 func (inst *instance) ClosePorts(machineId int, ports []state.Port) error {
 	defer delay()
+	if inst.firewallMode != config.FwInstance {
+		return fmt.Errorf("invalid firewall mode for closing ports on instance: %q",
+			inst.firewallMode)
+	}
 	if inst.machineId != machineId {
 		panic(fmt.Errorf("ClosePorts with mismatched machine id, expected %d got %d", inst.machineId, machineId))
 	}
@@ -657,6 +679,10 @@
 
 func (inst *instance) Ports(machineId int) (ports []state.Port, err error) {
 	defer delay()
+	if inst.firewallMode != config.FwInstance {
+		return nil, fmt.Errorf("invalid firewall mode for retrieving ports from instance: %q",
+			inst.firewallMode)
+	}
 	if inst.machineId != machineId {
 		panic(fmt.Errorf("Ports with mismatched machine id, expected %d got %d", inst.machineId, machineId))
 	}
 
=== modified file 'environs/ec2/ec2.go'
--- environs/ec2/ec2.go	2012-10-16 17:27:11 +0000
+++ environs/ec2/ec2.go	2012-10-18 11:21:20 +0000
@@ -602,6 +602,10 @@
 }
 
 func (e *environ) OpenPorts(ports []state.Port) error {
+	if e.Config().FirewallMode() != config.FwGlobal {
+		return fmt.Errorf("invalid firewall mode for opening ports on environment: %q",
+			e.Config().FirewallMode())
+	}
 	if err := e.openPortsInGroup(e.globalGroupName(), ports); err != nil {
 		return err
 	}
@@ -610,6 +614,10 @@
 }
 
 func (e *environ) ClosePorts(ports []state.Port) error {
+	if e.Config().FirewallMode() != config.FwGlobal {
+		return fmt.Errorf("invalid firewall mode for closing ports on environment: %q",
+			e.Config().FirewallMode())
+	}
 	if err := e.closePortsInGroup(e.globalGroupName(), ports); err != nil {
 		return err
 	}
@@ -618,6 +626,10 @@
 }
 
 func (e *environ) Ports() ([]state.Port, error) {
+	if e.Config().FirewallMode() != config.FwGlobal {
+		return nil, fmt.Errorf("invalid firewall mode for retrieving ports from environment: %q",
+			e.Config().FirewallMode())
+	}
 	return e.portsInGroup(e.globalGroupName())
 }
 
@@ -669,6 +681,10 @@
 }
 
 func (inst *instance) OpenPorts(machineId int, ports []state.Port) error {
+	if inst.e.Config().FirewallMode() != config.FwInstance {
+		return fmt.Errorf("invalid firewall mode for opening ports on instance: %q",
+			inst.e.Config().FirewallMode())
+	}
 	name := inst.e.machineGroupName(machineId)
 	if err := inst.e.openPortsInGroup(name, ports); err != nil {
 		return err
@@ -678,6 +694,10 @@
 }
 
 func (inst *instance) ClosePorts(machineId int, ports []state.Port) error {
+	if inst.e.Config().FirewallMode() != config.FwInstance {
+		return fmt.Errorf("invalid firewall mode for closing ports on instance: %q",
+			inst.e.Config().FirewallMode())
+	}
 	name := inst.e.machineGroupName(machineId)
 	if err := inst.e.closePortsInGroup(name, ports); err != nil {
 		return err
@@ -687,6 +707,10 @@
 }
 
 func (inst *instance) Ports(machineId int) ([]state.Port, error) {
+	if inst.e.Config().FirewallMode() != config.FwInstance {
+		return nil, fmt.Errorf("invalid firewall mode for retrieving ports from instance: %q",
+			inst.e.Config().FirewallMode())
+	}
 	name := inst.e.machineGroupName(machineId)
 	return inst.e.portsInGroup(name)
 }
 
=== modified file 'environs/jujutest/livetests.go'
--- environs/jujutest/livetests.go	2012-10-15 15:30:23 +0000
+++ environs/jujutest/livetests.go	2012-10-18 11:21:20 +0000
@@ -211,6 +211,16 @@
 	c.Assert(err, IsNil)
 	ports, err = inst2.Ports(2)
 	c.Assert(ports, DeepEquals, []state.Port{{"tcp", 89}})
+
+	// Check errors when acting on environment.
+	err = t.Env.OpenPorts([]state.Port{{"tcp", 80}})
+	c.Assert(err, ErrorMatches, `invalid firewall mode for opening ports on environment: "instance"`)
+
+	err = t.Env.ClosePorts([]state.Port{{"tcp", 80}})
+	c.Assert(err, ErrorMatches, `invalid firewall mode for closing ports on environment: "instance"`)
+
+	_, err = t.Env.Ports()
+	c.Assert(err, ErrorMatches, `invalid firewall mode for retrieving ports from environment: "instance"`)
 }
 
 func (t *LiveTests) TestGlobalPorts(c *C) {
@@ -265,6 +275,16 @@
 	ports, err = t.Env.Ports()
 	c.Assert(err, IsNil)
 	c.Assert(ports, DeepEquals, []state.Port{{"tcp", 45}, {"tcp", 89}})
+
+	// Check errors when acting on instances.
+	err = inst1.OpenPorts(1, []state.Port{{"tcp", 80}})
+	c.Assert(err, ErrorMatches, `invalid firewall mode for opening ports on instance: "global"`)
+
+	err = inst1.ClosePorts(1, []state.Port{{"tcp", 80}})
+	c.Assert(err, ErrorMatches, `invalid firewall mode for closing ports on instance: "global"`)
+
+	_, err = inst1.Ports(1)
+	c.Assert(err, ErrorMatches, `invalid firewall mode for retrieving ports from instance: "global"`)
 }
 
 func (t *LiveTests) TestBootstrapMultiple(c *C) {