Merge lp:~wallyworld/juju-core/instance-default-secgroup into lp:~go-bot/juju-core/trunk

[Ian Booth]

New openstack use-default-secgroup option

Openstack environments can be configured to start
instances with the "default" security group assigned.
The new use-default-secgroup config option (default false)
is used to enable this behaviour.

https://codereview.appspot.com/44770044/

New openstack use-default-secgroup option

Openstack environments can be configured to start
instances with the "default" security group assigned.
The new use-default-secgroup config option (default false)
is used to enable this behaviour.

https://codereview.appspot.com/44770044/

[Ian Booth (wallyworld)]

Reviewers: mp+199929_code.launchpad.net,

Message:
Please take a look.

Description:
New openstack use-default-secgroup option

Openstack environments can be configured to start
instances with the "default" security group assigned.
The new use-default-secgroup config option (default false)
is used to enable this behaviour.

https://code.launchpad.net/~wallyworld/juju-core/instance-default-secgroup/+merge/199929

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/44770044/

Affected files (+106, -23 lines):
   A [revision details]
   M cmd/juju/help_topics.go
   M dependencies.tsv
   M provider/openstack/config.go
   M provider/openstack/config_test.go
   M provider/openstack/live_test.go
   M provider/openstack/provider.go

[William Reade (fwereade)]

I'm just ignorant, I'm sure, but I don't see what we gain by making this
configurable. What do we lose by just *always* adding the default
security group?

https://codereview.appspot.com/44770044/diff/1/cmd/juju/help_topics.go
File cmd/juju/help_topics.go (right):

https://codereview.appspot.com/44770044/diff/1/cmd/juju/help_topics.go#newcode99
cmd/juju/help_topics.go:99: # use-default-secgroup: false
Thanks for documenting this, but I feel little the wiser.

https://codereview.appspot.com/44770044/

[Martin Packman (gz)]

I added a comment to bug 1129720 about the general approach.

I agree with William that adding more config here isn't really
desirable, but just putting the default group on all machines juju
creates is a step back in our general isolation level. The
per-environment security group is a much cleaner place to add custom
rules, even if we don't supply a juju command for doing that (though we
could).

Code itself looks fine.

https://codereview.appspot.com/44770044/

[William Reade (fwereade)]

On 2014/01/09 12:56:35, gz wrote:
> I added a comment to bug 1129720 about the general approach.

> I agree with William that adding more config here isn't really
desirable, but
> just putting the default group on all machines juju creates is a step
back in
> our general isolation level. The per-environment security group is a
much
> cleaner place to add custom rules, even if we don't supply a juju
command for
> doing that (though we could).

> Code itself looks fine.

I dunno, I think there's quite a neat separation between the default
security group -- with whatever cloud-specific rules, considered
appropriate by the administrator, that are kinda outside our purview --
and our own config. What's the use case for *not* including the default
security group? Nobody's needed it before, sure; but the default
behaviour doesn't look harmful, and if that's been changed it seems
reasonable to assume it's been done for a reason. Right?

Sorry for the delay, Ian, I had an all-day power cut -- can we chat
about this tomorrow morning? I'm fine with the code, indeed, but I feel
like including that security group is the correct default, and I'd be
happiest of all if we just skipped the config, but I'll accept pretty
much any plausible use case that requires its falsity as justification
for including the setting.

https://codereview.appspot.com/44770044/

[William Reade (fwereade)]

On 2014/01/09 19:48:46, fwereade wrote:
> On 2014/01/09 12:56:35, gz wrote:
> > I added a comment to bug 1129720 about the general approach.
> >
> > I agree with William that adding more config here isn't really
desirable, but
> > just putting the default group on all machines juju creates is a
step back in
> > our general isolation level. The per-environment security group is a
much
> > cleaner place to add custom rules, even if we don't supply a juju
command for
> > doing that (though we could).
> >
> > Code itself looks fine.

> I dunno, I think there's quite a neat separation between the default
security
> group -- with whatever cloud-specific rules, considered appropriate by
the
> administrator, that are kinda outside our purview -- and our own
config. What's
> the use case for *not* including the default security group? Nobody's
needed it
> before, sure; but the default behaviour doesn't look harmful, and if
that's been
> changed it seems reasonable to assume it's been done for a reason.
Right?

> Sorry for the delay, Ian, I had an all-day power cut -- can we chat
about this
> tomorrow morning? I'm fine with the code, indeed, but I feel like
including that
> security group is the correct default, and I'd be happiest of all if
we just
> skipped the config, but I'll accept pretty much any plausible use case
that
> requires its falsity as justification for including the setting.

LGTM after discussion -- we've been fine without it so far, so let's
stick with the existing default. Thanks for bearing with me.

https://codereview.appspot.com/44770044/

[Go Bot (go-bot)]

The attempt to merge lp:~wallyworld/juju-core/instance-default-secgroup into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.212s
ok launchpad.net/juju-core/agent/tools 0.258s
ok launchpad.net/juju-core/bzr 7.119s
ok launchpad.net/juju-core/cert 3.131s
ok launchpad.net/juju-core/charm 0.550s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.032s
ok launchpad.net/juju-core/cloudinit/sshinit 1.181s
ok launchpad.net/juju-core/cmd 0.209s
ok launchpad.net/juju-core/cmd/charm-admin 0.840s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 176.230s
ok launchpad.net/juju-core/cmd/jujud 56.073s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 2.879s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.028s
ok launchpad.net/juju-core/container 0.036s
ok launchpad.net/juju-core/container/factory 0.050s
ok launchpad.net/juju-core/container/kvm 0.277s
ok launchpad.net/juju-core/container/kvm/mock 0.050s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.296s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.335s
ok launchpad.net/juju-core/environs 3.171s
ok launchpad.net/juju-core/environs/bootstrap 4.567s
ok launchpad.net/juju-core/environs/cloudinit 0.534s
ok launchpad.net/juju-core/environs/config 1.111s
ok launchpad.net/juju-core/environs/configstore 0.041s
ok launchpad.net/juju-core/environs/filestorage 0.032s
ok launchpad.net/juju-core/environs/httpstorage 0.942s
ok launchpad.net/juju-core/environs/imagemetadata 0.593s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.057s
ok launchpad.net/juju-core/environs/jujutest 0.260s
ok launchpad.net/juju-core/environs/manual 11.019s
ok launchpad.net/juju-core/environs/simplestreams 0.362s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.188s
ok launchpad.net/juju-core/environs/storage 1.195s
ok launchpad.net/juju-core/environs/sync 30.984s
ok launchpad.net/juju-core/environs/testing 0.199s
ok launchpad.net/juju-core/environs/tools 6.719s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.016s
ok launchpad.net/juju-core/instance 0.023s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 16.000s
ok launchpad.net/juju-core/juju/osenv 0.018s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.027s
ok launchpad.net/juju-core/log/syslog 0.022s
ok launchpad.net/juju-core/names 0.033s
? launchpad.net/juju-core/provider [no test files]
? launchpad.net/juj...

[Go Bot (go-bot)]

The attempt to merge lp:~wallyworld/juju-core/instance-default-secgroup into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.229s
ok launchpad.net/juju-core/agent/tools 0.284s
ok launchpad.net/juju-core/bzr 7.167s
ok launchpad.net/juju-core/cert 3.144s
ok launchpad.net/juju-core/charm 0.595s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.038s
ok launchpad.net/juju-core/cloudinit/sshinit 1.090s
ok launchpad.net/juju-core/cmd 0.249s
ok launchpad.net/juju-core/cmd/charm-admin 0.834s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 175.648s
ok launchpad.net/juju-core/cmd/jujud 55.463s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 2.734s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.040s
ok launchpad.net/juju-core/container 0.040s
ok launchpad.net/juju-core/container/factory 0.049s
ok launchpad.net/juju-core/container/kvm 0.257s
ok launchpad.net/juju-core/container/kvm/mock 0.036s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.471s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.284s
ok launchpad.net/juju-core/environs 3.059s
ok launchpad.net/juju-core/environs/bootstrap 4.566s
ok launchpad.net/juju-core/environs/cloudinit 0.572s
ok launchpad.net/juju-core/environs/config 1.084s
ok launchpad.net/juju-core/environs/configstore 0.042s
ok launchpad.net/juju-core/environs/filestorage 0.033s
ok launchpad.net/juju-core/environs/httpstorage 1.026s
ok launchpad.net/juju-core/environs/imagemetadata 0.544s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.061s
ok launchpad.net/juju-core/environs/jujutest 0.256s
ok launchpad.net/juju-core/environs/manual 9.481s
ok launchpad.net/juju-core/environs/simplestreams 0.353s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.225s
ok launchpad.net/juju-core/environs/storage 1.205s
ok launchpad.net/juju-core/environs/sync 30.806s
ok launchpad.net/juju-core/environs/testing 0.236s
ok launchpad.net/juju-core/environs/tools 6.802s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.016s
ok launchpad.net/juju-core/instance 0.023s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 15.854s
ok launchpad.net/juju-core/juju/osenv 0.018s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.015s
ok launchpad.net/juju-core/log/syslog 0.024s
ok launchpad.net/juju-core/names 0.026s
? launchpad.net/juju-core/provider [no test files]
? launchpad.net/juju...

=== modified file 'cmd/juju/help_topics.go'
--- cmd/juju/help_topics.go	2013-10-25 22:24:35 +0000
+++ cmd/juju/help_topics.go	2013-12-23 00:50:44 +0000
@@ -94,6 +94,9 @@
     # give the nodes a public IP address. Some installations assign public
     # IP addresses by default without requiring a floating IP address.
     # use-floating-ip: false
+    # Specifies whether new machine instances should have the "default"
+    # Openstack security group assigned.
+    # use-default-secgroup: false
     admin-secret: 13850d1b9786065cadd0f477e8c97cd3
     # Globally unique swift bucket name
     control-bucket: juju-fd6ab8d02393af742bfbe8b9629707ee

=== modified file 'dependencies.tsv'
--- dependencies.tsv	2013-12-04 15:34:47 +0000
+++ dependencies.tsv	2013-12-23 00:50:44 +0000
@@ -6,7 +6,7 @@
 launchpad.net/gocheck	bzr	[e-mail]-20130302024745-6ikofwq2c03h7giu	85
 launchpad.net/golxc	bzr	[e-mail]-20130617094614-jelomk87defuetol	5
 launchpad.net/gomaasapi	bzr	[e-mail]-20131017011445-m1hmr0ap14osd7li	47
-launchpad.net/goose	bzr	tarmac-20131101034733-zbirap24k6k8wjlg	110
+launchpad.net/goose	bzr	tarmac-20131223000240-y2igxjt3kznam7vi	114
 launchpad.net/goyaml	bzr	[e-mail]-20131114120802-abe042syx64z2m7s	50
 launchpad.net/gwacl	bzr	tarmac-20131031081035-b33m6fyrrdiuf408	229
 launchpad.net/loggo	bzr	[e-mail]-20130729043644-ym0cscir1cist4sm	39

=== modified file 'provider/openstack/config.go'
--- provider/openstack/config.go	2013-10-24 00:20:59 +0000
+++ provider/openstack/config.go	2013-12-23 00:50:44 +0000
@@ -14,28 +14,30 @@
 )
 
 var configFields = schema.Fields{
-	"username":        schema.String(),
-	"password":        schema.String(),
-	"tenant-name":     schema.String(),
-	"auth-url":        schema.String(),
-	"auth-mode":       schema.String(),
-	"access-key":      schema.String(),
-	"secret-key":      schema.String(),
-	"region":          schema.String(),
-	"control-bucket":  schema.String(),
-	"use-floating-ip": schema.Bool(),
+	"username":             schema.String(),
+	"password":             schema.String(),
+	"tenant-name":          schema.String(),
+	"auth-url":             schema.String(),
+	"auth-mode":            schema.String(),
+	"access-key":           schema.String(),
+	"secret-key":           schema.String(),
+	"region":               schema.String(),
+	"control-bucket":       schema.String(),
+	"use-floating-ip":      schema.Bool(),
+	"use-default-secgroup": schema.Bool(),
 }
 var configDefaults = schema.Defaults{
-	"username":        "",
-	"password":        "",
-	"tenant-name":     "",
-	"auth-url":        "",
-	"auth-mode":       string(AuthUserPass),
-	"access-key":      "",
-	"secret-key":      "",
-	"region":          "",
-	"control-bucket":  "",
-	"use-floating-ip": false,
+	"username":             "",
+	"password":             "",
+	"tenant-name":          "",
+	"auth-url":             "",
+	"auth-mode":            string(AuthUserPass),
+	"access-key":           "",
+	"secret-key":           "",
+	"region":               "",
+	"control-bucket":       "",
+	"use-floating-ip":      false,
+	"use-default-secgroup": false,
 }
 
 type environConfig struct {
@@ -83,6 +85,10 @@
 	return c.attrs["use-floating-ip"].(bool)
 }
 
+func (c *environConfig) useDefaultSecurityGroup() bool {
+	return c.attrs["use-default-secgroup"].(bool)
+}
+
 func (p environProvider) newConfig(cfg *config.Config) (*environConfig, error) {
 	valid, err := p.Validate(cfg, nil)
 	if err != nil {

=== modified file 'provider/openstack/config_test.go'
--- provider/openstack/config_test.go	2013-10-24 00:20:59 +0000
+++ provider/openstack/config_test.go	2013-12-23 00:50:44 +0000
@@ -57,6 +57,7 @@
 	controlBucket           string
 	toolsURL                string
 	useFloatingIP           bool
+	useDefaultSecurityGroup bool
 	username                string
 	password                string
 	tenantName              string
@@ -159,6 +160,7 @@
 		c.Assert(ecfg.FirewallMode(), gc.Equals, t.firewallMode)
 	}
 	c.Assert(ecfg.useFloatingIP(), gc.Equals, t.useFloatingIP)
+	c.Assert(ecfg.useDefaultSecurityGroup(), gc.Equals, t.useDefaultSecurityGroup)
 	// Default should be true
 	expectedHostnameVerification := true
 	if t.sslHostnameSet {
@@ -372,6 +374,16 @@
 		},
 		useFloatingIP: true,
 	}, {
+		summary: "default use default security group",
+		// Do not use default security group by default.
+		useDefaultSecurityGroup: false,
+	}, {
+		summary: "use default security group",
+		config: attrs{
+			"use-default-secgroup": true,
+		},
+		useDefaultSecurityGroup: true,
+	}, {
 		summary: "admin-secret given",
 		config: attrs{
 			"admin-secret": "Futumpsh",

=== modified file 'provider/openstack/live_test.go'
--- provider/openstack/live_test.go	2013-11-04 02:01:19 +0000
+++ provider/openstack/live_test.go	2013-12-23 00:50:44 +0000
@@ -15,9 +15,14 @@
 	"launchpad.net/goose/identity"
 	"launchpad.net/goose/nova"
 
+	"launchpad.net/juju-core/constraints"
+	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/environs/bootstrap"
+	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/environs/jujutest"
 	"launchpad.net/juju-core/environs/storage"
 	envtesting "launchpad.net/juju-core/environs/testing"
+	jujutesting "launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/provider/openstack"
 	coretesting "launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/testing/testbase"
@@ -211,3 +216,42 @@
 	sort.Strings(expectedRules)
 	c.Check(stringRules, gc.DeepEquals, expectedRules)
 }
+
+func (s *LiveTests) assertStartInstanceDefaultSecurityGroup(c *gc.C, useDefault bool) {
+	attrs := s.TestConfig.Merge(coretesting.Attrs{
+		"name":                 "sample-" + randomName(),
+		"control-bucket":       "juju-test-" + randomName(),
+		"use-default-secgroup": useDefault,
+	})
+	cfg, err := config.New(config.NoDefaults, attrs)
+	c.Assert(err, gc.IsNil)
+	// Set up a test environment.
+	env, err := environs.New(cfg)
+	c.Assert(err, gc.IsNil)
+	c.Assert(env, gc.NotNil)
+	defer env.Destroy()
+	// Bootstrap and start an instance.
+	err = bootstrap.Bootstrap(bootstrapContext(c), env, constraints.Value{})
+	c.Assert(err, gc.IsNil)
+	inst, _ := jujutesting.AssertStartInstance(c, env, "100")
+	// Check whether the instance has the default security group assigned.
+	novaClient := openstack.GetNovaClient(env)
+	groups, err := novaClient.GetServerSecurityGroups(string(inst.Id()))
+	c.Assert(err, gc.IsNil)
+	defaultGroupFound := false
+	for _, group := range groups {
+		if group.Name == "default" {
+			defaultGroupFound = true
+			break
+		}
+	}
+	c.Assert(defaultGroupFound, gc.Equals, useDefault)
+}
+
+func (s *LiveTests) TestStartInstanceWithDefaultSecurityGroup(c *gc.C) {
+	s.assertStartInstanceDefaultSecurityGroup(c, true)
+}
+
+func (s *LiveTests) TestStartInstanceWithoutDefaultSecurityGroup(c *gc.C) {
+	s.assertStartInstanceDefaultSecurityGroup(c, false)
+}

=== modified file 'provider/openstack/provider.go'
--- provider/openstack/provider.go	2013-12-19 09:15:40 +0000
+++ provider/openstack/provider.go	2013-12-23 00:50:44 +0000
@@ -71,6 +71,10 @@
     # addresses by default without requiring a floating IP address.
     # use-floating-ip: false
 
+    # use-default-secgroup specifies whether new machine instances should have the "default"
+    # Openstack security group assigned.
+    # use-default-secgroup: false
+
     # tools-metadata-url specifies the location of the Juju tools and metadata. It defaults to the
     # global public tools metadata location https://streams.canonical.com/tools.
     # tools-metadata-url:  https://you-tools-metadata-url
@@ -119,7 +123,11 @@
     # to give the nodes a public IP address. Some installations assign public IP
     # addresses by default without requiring a floating IP address.
     # use-floating-ip: false
-    
+
+    # use-default-secgroup specifies whether new machine instances should have the "default"
+    # Openstack security group assigned.
+    # use-default-secgroup: false
+
     # tenant-name holds the openstack tenant name. In HPCloud, this is 
     # synonymous with the project-name  It defaults to
     # the environment variable OS_TENANT_NAME.
@@ -1058,7 +1066,15 @@
 	if err != nil {
 		return nil, err
 	}
-	return []nova.SecurityGroup{jujuGroup, machineGroup}, nil
+	groups := []nova.SecurityGroup{jujuGroup, machineGroup}
+	if e.ecfg().useDefaultSecurityGroup() {
+		defaultGroup, err := e.nova().SecurityGroupByName("default")
+		if err != nil {
+			return nil, fmt.Errorf("loading default security group: %v", err)
+		}
+		groups = append(groups, *defaultGroup)
+	}
+	return groups, nil
 }
 
 // zeroGroup holds the zero security group.