Merge lp:~themue/juju-core/go-ec2-security-group-config into lp:~juju/juju-core/trunk

[Frank Mueller]

ec2: added security group configuration

Added a configuration option to enable and disable the
usage of one security group per machine in EC2. Currently
only the configuration change is not used, only hints
in ec2.go.

https://codereview.appspot.com/6596051/

[Gustavo Niemeyer (niemeyer)]

This is going in the right direction, but this problem is not specific
to EC2, so we can take another step forward and have it ready for the
next providers. I suggest introducing:

     firewall-mode: default / global

With an internal API such as:

     func (c *Config) FirewallMode() FirewallMode { ... }

     type FirewallMode string

     const (
             FwDefault FirewallMode = "default"
             FwGlobal FirewallMode = "global"
     )

https://codereview.appspot.com/6596051/

[Gustavo Niemeyer (niemeyer)]

LGTM, but please add a test where indicated.

https://codereview.appspot.com/6596051/diff/3001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6596051/diff/3001/environs/config/config.go#newcode10
environs/config/config.go:10: // per machine of global.
// FirewallMode defines the way in which the environment
// handles opening and closing of firewall ports.

https://codereview.appspot.com/6596051/diff/3001/environs/config/config.go#newcode13
environs/config/config.go:13: const (
// FwDefault is the environment-specific default mode.

https://codereview.appspot.com/6596051/diff/3001/environs/config/config.go#newcode14
environs/config/config.go:14: FwDefault FirewallMode = "default"
<empty line>
// FwGlobal requests the use of a single firewall group for all
machines.
// When ports are opened for one machine, all machines will have the
same
// port opened.

https://codereview.appspot.com/6596051/diff/3001/environs/config/config_test.go
File environs/config/config_test.go (right):

https://codereview.appspot.com/6596051/diff/3001/environs/config/config_test.go#newcode133
environs/config/config_test.go:133: },
We need a test here.

https://codereview.appspot.com/6596051/

[Gustavo Niemeyer (niemeyer)]

LGTM

https://codereview.appspot.com/6596051/diff/3002/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6596051/diff/3002/environs/config/config.go#newcode78
environs/config/config.go:78: return nil, fmt.Errorf("invalid firewall
mode %q in environment configuration", firewallMode)
Good catch.

This should follow the existing convention above:

"invalid firewall mode in environment configuration: %q"

https://codereview.appspot.com/6596051/

=== modified file 'environs/config/config.go'
--- environs/config/config.go	2012-09-21 11:39:56 +0000
+++ environs/config/config.go	2012-10-02 15:34:21 +0000
@@ -6,6 +6,20 @@
 	"launchpad.net/juju-core/version"
 )
 
+// FirewallMode defines the way in which the environment
+// handles opening and closing of firewall ports.
+type FirewallMode string
+
+const (
+	// FwDefault is the environment-specific default mode. 
+	FwDefault FirewallMode = "default"
+
+	// FwGlobal requests the use of a single firewall group for all machines.
+	// When ports are opened for one machine, all machines will have the same
+	// port opened.
+	FwGlobal FirewallMode = "global"
+)
+
 // Config holds an immutable environment configuration.
 type Config struct {
 	m, t map[string]interface{}
@@ -58,6 +72,12 @@
 		}
 	}
 
+	// Check firewall mode.
+	firewallMode := FirewallMode(c.m["firewall-mode"].(string))
+	if firewallMode != FwDefault && firewallMode != FwGlobal {
+		return nil, fmt.Errorf("invalid firewall mode %q in environment configuration", firewallMode)
+	}
+
 	// Copy unknown attributes onto the type-specific map.
 	for k, v := range attrs {
 		if _, ok := fields[k]; !ok {
@@ -87,6 +107,12 @@
 	return c.m["authorized-keys"].(string)
 }
 
+// FirewallMode returns whether the firewall should
+// manage ports per machine or global.
+func (c *Config) FirewallMode() FirewallMode {
+	return FirewallMode(c.m["firewall-mode"].(string))
+}
+
 // AgentVersion returns the proposed version number for the agent tools.
 // It returns the zero version if unset.
 func (c *Config) AgentVersion() version.Number {
@@ -143,6 +169,7 @@
 	"default-series":       schema.String(),
 	"authorized-keys":      schema.String(),
 	"authorized-keys-path": schema.String(),
+	"firewall-mode":        schema.String(),
 	"agent-version":        schema.String(),
 	"development":          schema.Bool(),
 }
@@ -151,6 +178,7 @@
 	"default-series":       version.Current.Series,
 	"authorized-keys":      "",
 	"authorized-keys-path": "",
+	"firewall-mode":        FwDefault,
 	"agent-version":        schema.Omit,
 	"development":          false,
 }

=== modified file 'environs/config/config_test.go'
--- environs/config/config_test.go	2012-09-06 09:29:52 +0000
+++ environs/config/config_test.go	2012-10-02 15:34:21 +0000
@@ -130,6 +130,30 @@
 			"name": "",
 		},
 		"empty name in environment configuration",
+	}, {
+		// Default firewall mode.
+		attrs{
+			"type":          "my-type",
+			"name":          "my-name",
+			"firewall-mode": config.FwDefault,
+		},
+		"",
+	}, {
+		// Global firewall mode.
+		attrs{
+			"type":          "my-type",
+			"name":          "my-name",
+			"firewall-mode": config.FwGlobal,
+		},
+		"",
+	}, {
+		// Illegal firewall mode.
+		attrs{
+			"type":          "my-type",
+			"name":          "my-name",
+			"firewall-mode": "illegal",
+		},
+		"invalid firewall mode .* in environment configuration",
 	},
 }
 
@@ -213,6 +237,7 @@
 		"type":            "my-type",
 		"name":            "my-name",
 		"authorized-keys": "my-keys",
+		"firewall-mode":   string(config.FwDefault),
 		"default-series":  version.Current.Series,
 		"unknown":         "my-unknown",
 	}

=== modified file 'environs/ec2/config_test.go'
--- environs/ec2/config_test.go	2012-07-26 15:54:58 +0000
+++ environs/ec2/config_test.go	2012-10-02 15:34:21 +0000
@@ -235,7 +235,7 @@
 
 func (s *ConfigSuite) TestConfig(c *C) {
 	for i, t := range configTests {
-		c.Logf("test %d: %q", i, t.config)
+		c.Logf("test %d: %v", i, t.config)
 		t.check(c)
 	}
 }

=== modified file 'environs/ec2/ec2.go'
--- environs/ec2/ec2.go	2012-09-28 13:25:35 +0000
+++ environs/ec2/ec2.go	2012-10-02 15:34:21 +0000
@@ -566,6 +566,7 @@
 		return nil
 	}
 	// Give permissions for anyone to access the given ports.
+	// TODO(mue) Choose group depending on inst.e.Config().FirewallMode().
 	ipPerms := portsToIPPerms(ports)
 	g := ec2.SecurityGroup{Name: inst.e.machineGroupName(machineId)}
 	_, err := inst.e.ec2().AuthorizeSecurityGroup(g, ipPerms)
@@ -599,6 +600,7 @@
 	// Revoke permissions for anyone to access the given ports.
 	// Note that ec2 allows the revocation of permissions that aren't
 	// granted, so this is naturally idempotent.
+	// TODO(mue) Choose group depending on inst.e.Config().FirewallMode().
 	g := ec2.SecurityGroup{Name: inst.e.machineGroupName(machineId)}
 	_, err := inst.e.ec2().RevokeSecurityGroup(g, portsToIPPerms(ports))
 	if err != nil {
@@ -622,6 +624,7 @@
 }
 
 func (inst *instance) Ports(machineId int) (ports []state.Port, err error) {
+	// TODO(mue) Choose group depending on inst.e.Config().FirewallMode().
 	g := ec2.SecurityGroup{Name: inst.e.machineGroupName(machineId)}
 	resp, err := inst.e.ec2().SecurityGroups([]ec2.SecurityGroup{g}, nil)
 	if err != nil {
@@ -685,6 +688,8 @@
 	if err != nil {
 		return nil, err
 	}
+	// TODO(mue) Ensure machine group only if e.Config().FirewallMode()
+	// is config.FwDefault.
 	jujuMachineGroup, err := e.ensureGroup(e.machineGroupName(machineId), nil)
 	if err != nil {
 		return nil, err