Merge lp:~axwalk/juju-core/lp1262764-manual-storage-setconfig into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

provider/manual: set storage in SetConfig

We currently rely on EnableBootstrapStorage to
specify the storage mechanism to use for bootstrap
and sync-tools. This is insufficient in practice,
causing panics such as those in lp:1262764.

Instead, we inject a boolean attribute into the
environment config that is stored in state; this
attribute is false in the .jenv file, and true
in state. We use its value to decide whether to
use sshstorage (external) or httpstorage (internal).

A followup will eliminate BootstrapStorager.

Fixes lp:1262764

https://codereview.appspot.com/61480043/

provider/manual: set storage in SetConfig

We currently rely on EnableBootstrapStorage to
specify the storage mechanism to use for bootstrap
and sync-tools. This is insufficient in practice,
causing panics such as those in lp:1262764.

Instead, we inject a boolean attribute into the
environment config that is stored in state; this
attribute is false in the .jenv file, and true
in state. We use its value to decide whether to
use sshstorage (external) or httpstorage (internal).

A followup will eliminate BootstrapStorager.

Fixes lp:1262764

https://codereview.appspot.com/61480043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+205530_code.launchpad.net,

Message:
Please take a look.

Description:
provider/manual: set storage in SetConfig

We currently rely on EnableBootstrapStorage to
specify the storage mechanism to use for bootstrap
and sync-tools. This is insufficient in practice,
causing panics such as those in lp:1262764.

Instead, we inject a boolean attribute into the
environment config that is stored in state; this
attribute is false in the .jenv file, and true
in state. We use its value to decide whether to
use sshstorage (external) or httpstorage (internal).

A followup will eliminate BootstrapStorager.

Fixes lp:1262764

https://code.launchpad.net/~axwalk/juju-core/lp1262764-manual-storage-setconfig/+merge/205530

(do not edit description out of merge proposal)

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

Affected files (+128, -66 lines):
   A [revision details]
   M provider/manual/config.go
   M provider/manual/config_test.go
   M provider/manual/environ.go
   M provider/manual/environ_test.go
   M provider/manual/export_test.go
   M provider/manual/provider.go
   M provider/manual/provider_test.go
   M provider/manual/suite_test.go

[Dimiter Naydenov (dimitern)]

I'm not so sure about the flag name vs. what's its meaning. See below.

Other than that, it looks solid.

https://codereview.appspot.com/61480043/diff/1/provider/manual/config.go
File provider/manual/config.go (right):

https://codereview.appspot.com/61480043/diff/1/provider/manual/config.go#newcode22
provider/manual/config.go:22: "bootstrapped": schema.Bool(),
This looks a bit dubious - if the purpose of this flag (as seen later in
tests) is to make a decision which storage to use, then why not call it
"use-httpstorage" or something? That way, for older environments it will
be "false" by default, and we get the same logic, don't we?

https://codereview.appspot.com/61480043/

[Andrew Wilkins (axwalk)]

On 2014/02/10 09:28:06, dimitern wrote:
> I'm not so sure about the flag name vs. what's its meaning. See below.

> Other than that, it looks solid.

https://codereview.appspot.com/61480043/diff/1/provider/manual/config.go
> File provider/manual/config.go (right):

https://codereview.appspot.com/61480043/diff/1/provider/manual/config.go#newcode22
> provider/manual/config.go:22: "bootstrapped": schema.Bool(),
> This looks a bit dubious - if the purpose of this flag (as seen later
in tests)
> is to make a decision which storage to use, then why not call it
> "use-httpstorage" or something? That way, for older environments it
will be
> "false" by default, and we get the same logic, don't we?

Good idea, thanks. I'll update that tomorrow.

https://codereview.appspot.com/61480043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/61480043/

[Dimiter Naydenov (dimitern)]

Thanks! LGTM.

https://codereview.appspot.com/61480043/

=== modified file 'provider/manual/config.go'
--- provider/manual/config.go	2014-01-29 09:18:45 +0000
+++ provider/manual/config.go	2014-02-10 12:33:23 +0000
@@ -19,11 +19,13 @@
 		"storage-listen-ip": schema.String(),
 		"storage-port":      schema.Int(),
 		"storage-auth-key":  schema.String(),
+		"use-sshstorage":    schema.Bool(),
 	}
 	configDefaults = schema.Defaults{
 		"bootstrap-user":    "",
 		"storage-listen-ip": "",
 		"storage-port":      defaultStoragePort,
+		"use-sshstorage":    true,
 	}
 )
 
@@ -36,6 +38,14 @@
 	return &environConfig{Config: config, attrs: attrs}
 }
 
+func (c *environConfig) useSSHStorage() bool {
+	// Prior to 1.17.3, the use-sshstorage attribute
+	// did not exist. We take non-existence to be
+	// equivalent to false.
+	useSSHStorage, _ := c.attrs["use-sshstorage"].(bool)
+	return useSSHStorage
+}
+
 func (c *environConfig) bootstrapHost() string {
 	return c.attrs["bootstrap-host"].(string)
 }

=== modified file 'provider/manual/config_test.go'
--- provider/manual/config_test.go	2014-01-31 05:38:42 +0000
+++ provider/manual/config_test.go	2014-02-10 12:33:23 +0000
@@ -11,6 +11,7 @@
 
 	"launchpad.net/juju-core/environs/config"
 	coretesting "launchpad.net/juju-core/testing"
+	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
 )
 
@@ -26,6 +27,9 @@
 		"type":             "manual",
 		"bootstrap-host":   "hostname",
 		"storage-auth-key": "whatever",
+		// Not strictly necessary, but simplifies testing by disabling
+		// ssh storage by default.
+		"use-sshstorage": false,
 		// While the ca-cert bits aren't entirely minimal, they avoid the need
 		// to set up a fake home.
 		"ca-cert":        coretesting.CACert,
@@ -120,3 +124,16 @@
 	c.Assert(testConfig.storageAddr(), gc.Equals, "hostname:1234")
 	c.Assert(testConfig.storageListenAddr(), gc.Equals, "10.0.0.123:1234")
 }
+
+func (s *configSuite) TestStorageCompat(c *gc.C) {
+	// Older environment configurations will not have the
+	// use-sshstorage attribute. We treat them as if they
+	// have use-sshstorage=false.
+	values := MinimalConfigValues()
+	delete(values, "use-sshstorage")
+	cfg, err := config.New(config.UseDefaults, values)
+	c.Assert(err, gc.IsNil)
+	envConfig := newEnvironConfig(cfg, values)
+	c.Assert(err, gc.IsNil)
+	c.Assert(envConfig.useSSHStorage(), jc.IsFalse)
+}

=== modified file 'provider/manual/environ.go'
--- provider/manual/environ.go	2014-01-29 09:18:45 +0000
+++ provider/manual/environ.go	2014-02-10 12:33:23 +0000
@@ -51,12 +51,11 @@
 var logger = loggo.GetLogger("juju.provider.manual")
 
 type manualEnviron struct {
-	cfg                   *environConfig
-	cfgmutex              sync.Mutex
-	bootstrapStorage      storage.Storage
-	bootstrapStorageMutex sync.Mutex
-	ubuntuUserInited      bool
-	ubuntuUserInitMutex   sync.Mutex
+	cfg                 *environConfig
+	cfgmutex            sync.Mutex
+	storage             storage.Storage
+	ubuntuUserInited    bool
+	ubuntuUserInitMutex sync.Mutex
 }
 
 var _ environs.BootstrapStorager = (*manualEnviron)(nil)
@@ -115,6 +114,14 @@
 	if err := e.ensureBootstrapUbuntuUser(ctx); err != nil {
 		return err
 	}
+	// Set "use-sshstorage" to false, so agents know not to use sshstorage.
+	cfg, err := e.Config().Apply(map[string]interface{}{"use-sshstorage": false})
+	if err != nil {
+		return err
+	}
+	if err := e.SetConfig(cfg); err != nil {
+		return err
+	}
 	envConfig := e.envConfig()
 	host := envConfig.bootstrapHost()
 	hc, series, err := manual.DetectSeriesAndHardwareCharacteristics(host)
@@ -147,6 +154,35 @@
 	if err != nil {
 		return err
 	}
+	// Set storage. If "use-sshstorage" is true then use the SSH storage.
+	// Otherwise, use HTTP storage.
+	//
+	// We don't change storage once it's been set. Storage parameters
+	// are fixed at bootstrap time, and it is not possible to change
+	// them.
+	if e.storage == nil {
+		var stor storage.Storage
+		if envConfig.useSSHStorage() {
+			storageDir := e.StorageDir()
+			storageTmpdir := path.Join(dataDir, storageTmpSubdir)
+			stor, err = newSSHStorage("ubuntu@"+e.cfg.bootstrapHost(), storageDir, storageTmpdir)
+			if err != nil {
+				return fmt.Errorf("initialising SSH storage failed: %v", err)
+			}
+		} else {
+			caCertPEM, ok := envConfig.CACert()
+			if !ok {
+				// should not be possible to validate base config
+				return fmt.Errorf("ca-cert not set")
+			}
+			authkey := envConfig.storageAuthKey()
+			stor, err = httpstorage.ClientTLS(envConfig.storageAddr(), caCertPEM, authkey)
+			if err != nil {
+				return fmt.Errorf("initialising HTTPS storage failed: %v", err)
+			}
+		}
+		e.storage = stor
+	}
 	e.cfg = envConfig
 	return nil
 }
@@ -184,23 +220,7 @@
 
 // Implements environs.BootstrapStorager.
 func (e *manualEnviron) EnableBootstrapStorage(ctx environs.BootstrapContext) error {
-	e.bootstrapStorageMutex.Lock()
-	defer e.bootstrapStorageMutex.Unlock()
-	if e.bootstrapStorage != nil {
-		return nil
-	}
-	if err := e.ensureBootstrapUbuntuUser(ctx); err != nil {
-		return err
-	}
-	cfg := e.envConfig()
-	storageDir := e.StorageDir()
-	storageTmpdir := path.Join(dataDir, storageTmpSubdir)
-	bootstrapStorage, err := newSSHStorage("ubuntu@"+cfg.bootstrapHost(), storageDir, storageTmpdir)
-	if err != nil {
-		return err
-	}
-	e.bootstrapStorage = bootstrapStorage
-	return nil
+	return e.ensureBootstrapUbuntuUser(ctx)
 }
 
 // GetToolsSources returns a list of sources which are
@@ -213,24 +233,9 @@
 }
 
 func (e *manualEnviron) Storage() storage.Storage {
-	e.bootstrapStorageMutex.Lock()
-	defer e.bootstrapStorageMutex.Unlock()
-	if e.bootstrapStorage != nil {
-		return e.bootstrapStorage
-	}
-	caCertPEM, authkey := e.StorageCACert(), e.StorageAuthKey()
-	if caCertPEM != nil && authkey != "" {
-		storage, err := httpstorage.ClientTLS(e.envConfig().storageAddr(), caCertPEM, authkey)
-		if err != nil {
-			// Should be impossible, since ca-cert will always be validated.
-			logger.Errorf("initialising HTTPS storage failed: %v", err)
-		} else {
-			return storage
-		}
-	} else {
-		logger.Errorf("missing CA cert or auth-key")
-	}
-	return nil
+	e.cfgmutex.Lock()
+	defer e.cfgmutex.Unlock()
+	return e.storage
 }
 
 var runSSHCommand = func(host string, command []string) (stderr string, err error) {

=== modified file 'provider/manual/environ_test.go'
--- provider/manual/environ_test.go	2014-01-31 05:38:42 +0000
+++ provider/manual/environ_test.go	2014-02-10 12:33:23 +0000
@@ -26,11 +26,16 @@
 	env *manualEnviron
 }
 
+type dummyStorage struct {
+	storage.Storage
+}
+
 var _ = gc.Suite(&environSuite{})
 
 func (s *environSuite) SetUpTest(c *gc.C) {
-	envConfig := getEnvironConfig(c, MinimalConfigValues())
-	s.env = &manualEnviron{cfg: envConfig}
+	env, err := manualProvider{}.Open(MinimalConfig(c))
+	c.Assert(err, gc.IsNil)
+	s.env = env.(*manualEnviron)
 }
 
 func (s *environSuite) TestSetConfig(c *gc.C) {
@@ -128,19 +133,7 @@
 	c.Assert(strings.Contains(url, "/tools"), jc.IsTrue)
 }
 
-type dummyStorage struct {
-	storage.Storage
-}
-
 func (s *environSuite) TestEnvironBootstrapStorager(c *gc.C) {
-	var newSSHStorageResult = struct {
-		stor storage.Storage
-		err  error
-	}{dummyStorage{}, errors.New("failed to get SSH storage")}
-	s.PatchValue(&newSSHStorage, func(sshHost, storageDir, storageTmpdir string) (storage.Storage, error) {
-		return newSSHStorageResult.stor, newSSHStorageResult.err
-	})
-
 	var initUbuntuResult error
 	s.PatchValue(&initUbuntuUser, func(host, user, authorizedKeys string, stdin io.Reader, stdout io.Writer) error {
 		return initUbuntuResult
@@ -150,17 +143,9 @@
 	initUbuntuResult = errors.New("failed to initialise ubuntu user")
 	c.Assert(s.env.EnableBootstrapStorage(ctx), gc.Equals, initUbuntuResult)
 	initUbuntuResult = nil
-	c.Assert(s.env.EnableBootstrapStorage(ctx), gc.Equals, newSSHStorageResult.err)
+	c.Assert(s.env.EnableBootstrapStorage(ctx), gc.IsNil)
 	// after the user is initialised once successfully,
 	// another attempt will not be made.
 	initUbuntuResult = errors.New("failed to initialise ubuntu user")
-	c.Assert(s.env.EnableBootstrapStorage(ctx), gc.Equals, newSSHStorageResult.err)
-
-	// after the bootstrap storage is initialised once successfully,
-	// another attempt will not be made.
-	backup := newSSHStorageResult.err
-	newSSHStorageResult.err = nil
-	c.Assert(s.env.EnableBootstrapStorage(ctx), gc.IsNil)
-	newSSHStorageResult.err = backup
 	c.Assert(s.env.EnableBootstrapStorage(ctx), gc.IsNil)
 }

=== modified file 'provider/manual/export_test.go'
--- provider/manual/export_test.go	2014-01-31 05:38:42 +0000
+++ provider/manual/export_test.go	2014-02-10 12:33:23 +0000
@@ -5,4 +5,5 @@
 
 var (
 	ProviderInstance = manualProvider{}
+	NewSSHStorage    = &newSSHStorage
 )

=== modified file 'provider/manual/provider.go'
--- provider/manual/provider.go	2014-02-10 03:29:45 +0000
+++ provider/manual/provider.go	2014-02-10 12:33:23 +0000
@@ -22,7 +22,7 @@
 var errNoBootstrapHost = errors.New("bootstrap-host must be specified")
 
 func (p manualProvider) Prepare(cfg *config.Config) (environs.Environ, error) {
-	if _, ok := cfg.UnknownAttrs()["storage-auth-key"].(string); !ok {
+	if _, ok := cfg.UnknownAttrs()["storage-auth-key"]; !ok {
 		uuid, err := utils.NewUUID()
 		if err != nil {
 			return nil, err
@@ -34,6 +34,9 @@
 			return nil, err
 		}
 	}
+	if use, ok := cfg.UnknownAttrs()["use-sshstorage"].(bool); ok && !use {
+		return nil, fmt.Errorf("use-sshstorage must not be specified")
+	}
 	return p.Open(cfg)
 }
 
@@ -46,7 +49,12 @@
 }
 
 func (p manualProvider) open(cfg *environConfig) (environs.Environ, error) {
-	return &manualEnviron{cfg: cfg}, nil
+	env := &manualEnviron{cfg: cfg}
+	// Need to call SetConfig to initialise storage.
+	if err := env.SetConfig(cfg.Config); err != nil {
+		return nil, err
+	}
+	return env, nil
 }
 
 func checkImmutableString(cfg, old *environConfig, key string) error {
@@ -88,6 +96,10 @@
 		if oldPort != newPort {
 			return nil, fmt.Errorf("cannot change storage-port from %q to %q", oldPort, newPort)
 		}
+		oldUseSSHStorage, newUseSSHStorage := oldEnvConfig.useSSHStorage(), envConfig.useSSHStorage()
+		if oldUseSSHStorage != newUseSSHStorage && newUseSSHStorage == true {
+			return nil, fmt.Errorf("cannot change use-sshstorage from %v to %v", oldUseSSHStorage, newUseSSHStorage)
+		}
 	}
 	return envConfig, nil
 }

=== modified file 'provider/manual/provider_test.go'
--- provider/manual/provider_test.go	2014-02-10 10:05:26 +0000
+++ provider/manual/provider_test.go	2014-02-10 12:33:23 +0000
@@ -4,10 +4,13 @@
 package manual_test
 
 import (
+	"fmt"
+
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/environs/storage"
 	"launchpad.net/juju-core/provider/manual"
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
@@ -22,6 +25,7 @@
 
 func (s *providerSuite) TestPrepare(c *gc.C) {
 	minimal := manual.MinimalConfigValues()
+	minimal["use-sshstorage"] = true
 	delete(minimal, "storage-auth-key")
 	testConfig, err := config.New(config.UseDefaults, minimal)
 	c.Assert(err, gc.IsNil)
@@ -32,6 +36,24 @@
 	c.Assert(key, jc.Satisfies, utils.IsValidUUIDString)
 }
 
+func (s *providerSuite) TestPrepareUseSSHStorage(c *gc.C) {
+	minimal := manual.MinimalConfigValues()
+	minimal["use-sshstorage"] = false
+	testConfig, err := config.New(config.UseDefaults, minimal)
+	c.Assert(err, gc.IsNil)
+	_, err = manual.ProviderInstance.Prepare(testConfig)
+	c.Assert(err, gc.ErrorMatches, "use-sshstorage must not be specified")
+
+	s.PatchValue(manual.NewSSHStorage, func(sshHost, storageDir, storageTmpdir string) (storage.Storage, error) {
+		return nil, fmt.Errorf("newSSHStorage failed")
+	})
+	minimal["use-sshstorage"] = true
+	testConfig, err = config.New(config.UseDefaults, minimal)
+	c.Assert(err, gc.IsNil)
+	_, err = manual.ProviderInstance.Prepare(testConfig)
+	c.Assert(err, gc.ErrorMatches, "initialising SSH storage failed: newSSHStorage failed")
+}
+
 func (s *providerSuite) TestNullAlias(c *gc.C) {
 	p, err := environs.Provider("manual")
 	c.Assert(p, gc.NotNil)

=== modified file 'provider/manual/suite_test.go'
--- provider/manual/suite_test.go	2014-01-31 05:38:42 +0000
+++ provider/manual/suite_test.go	2014-02-10 12:33:23 +0000
@@ -7,8 +7,15 @@
 	"testing"
 
 	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/environs/storage"
+	"launchpad.net/juju-core/provider/manual"
 )
 
 func Test(t *testing.T) {
+	// Prevent any use of ssh for storage.
+	*manual.NewSSHStorage = func(sshHost, storageDir, storageTmpdir string) (storage.Storage, error) {
+		return nil, nil
+	}
 	gc.TestingT(t)
 }