Merge lp:~axwalk/juju-core/null-provider-customsources into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

provider/null: define simplestreams tools source

This is somewhat involved, as there's some fixes
required (and one drive-by in sshstorage). The
change to use GetToolsSources is simple enough,
and matches what the local provider does.

The Environ override struct in cmd/juju/bootstrap.go
had to so that the optional Environ interfaces were
still implemented. Overriding like that loses the
additional interfaces.

To do the above, environs.BootstrapStorager has been
changed to have a method that tells the environ to
use bootstrap storage, rather than returning a
bootstrap storage. The null provider has changed to
match, of course, as well as cmd/juju.

https://codereview.appspot.com/14011046/

provider/null: define simplestreams tools source

This is somewhat involved, as there's some fixes
required (and one drive-by in sshstorage). The
change to use GetToolsSources is simple enough,
and matches what the local provider does.

The Environ override struct in cmd/juju/bootstrap.go
had to so that the optional Environ interfaces were
still implemented. Overriding like that loses the
additional interfaces.

To do the above, environs.BootstrapStorager has been
changed to have a method that tells the environ to
use bootstrap storage, rather than returning a
bootstrap storage. The null provider has changed to
match, of course, as well as cmd/juju.

https://codereview.appspot.com/14011046/

[Andrew Wilkins (axwalk)]

Reviewers: mp+187978_code.launchpad.net,

Message:
Please take a look.

Description:
provider/null: define simplestreams tools source

This is somewhat involved, as there's some fixes
required (and one drive-by in sshstorage). The
change to use GetToolsSources is simple enough,
and matches what the local provider does.

The Environ override struct in cmd/juju/bootstrap.go
had to so that the optional Environ interfaces were
still implemented. Overriding like that loses the
additional interfaces.

To do the above, environs.BootstrapStorager has been
changed to have a method that tells the environ to
use bootstrap storage, rather than returning a
bootstrap storage. The null provider has changed to
match, of course, as well as cmd/juju.

https://code.launchpad.net/~axwalk/juju-core/null-provider-customsources/+merge/187978

(do not edit description out of merge proposal)

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

Affected files (+66, -32 lines):
   A [revision details]
   M cmd/juju/bootstrap.go
   M environs/httpstorage/storage.go
   M environs/interface.go
   M environs/manual/bootstrap.go
   M environs/sshstorage/storage.go
   M provider/null/environ.go
   M provider/null/environ_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130927021656-yolgm6vq7sjrlf6b
+New revision: <email address hidden>

Index: environs/interface.go
=== modified file 'environs/interface.go'
--- environs/interface.go 2013-09-26 09:18:36 +0000
+++ environs/interface.go 2013-09-27 05:19:30 +0000
@@ -69,15 +69,18 @@
   PublicStorage() storage.StorageReader
  }

-// BootstrapStorager is an interface that returns a environs.Storage that
may
-// be used before the bootstrap machine agent has been provisioned.
+// BootstrapStorager is an interface through which an Environ may be
+// instructed to use a special "bootstrap storage". Bootstrap storage
+// is one that may be used before the bootstrap machine agent has been
+// provisioned.
  //
-// This is useful for environments where the storage is managed by the
machine
-// agent once bootstrapped.
+// This is useful for environments where the storage is managed by the
+// machine agent once bootstrapped.
  type BootstrapStorager interface {
- // BootstrapStorager returns an environs.Storage that may be used while
- // bootstrapping a machine.
- BootstrapStorage() (storage.Storage, error)
+ // EnableBootstrapStorage enables or disables bootstrap storage,
+ // depending on the flag, and returns the previous state and an
+ // error if enablement failed.
+ EnableBootstrapStorage(enable bool) (wasEnabled bool, err error)
  }

  // ConfigGetter implements access to an environments configuration.

Index: cmd/juju/bootstrap.go
=== modified file 'cmd/juju/bootstrap.go'
--- cmd/juju/bootstrap.go 2013-09-25 06:22:54 +0000
+++ cmd/juju/bootstrap.go 2013-09-27 05:19:30 +0000
@@ -18,7 +18,6 @@
   "launchpad.net/juju-core/environs/bootstrap"
   "launchpad.net/juju-core/environs/config"
   "launchpad.net/juju-core/envir...

[William Reade (fwereade)]

Good direction, but a few notes:

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go
File provider/null/environ.go (left):

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#oldcode43
provider/null/environ.go:43: }
It's quite nice to do "var _" type asserts by the definition of the
type, given that we seem to be growing optional interfaces at a
reasonable rate. Compiled documentation, if you like, that stops you
having to read all the methods.

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#newcode126
provider/null/environ.go:126: func (e *nullEnviron)
EnableBootstrapStorage(enable bool) (wasEnabled bool, err error) {
We should be locking here, shouldn't we? Even if it makes no practical
difference today, environs should be goroutine-safe.

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#newcode153
provider/null/environ.go:153:
storage.NewStorageSimpleStreamsDataSource(e.Storage(),
storage.BaseToolsPath),
This is starting to look awfully familiar. No action obviously right
today, but it feels like a bit of a smell.

https://codereview.appspot.com/14011046/diff/1/provider/null/environ_test.go
File provider/null/environ_test.go (right):

https://codereview.appspot.com/14011046/diff/1/provider/null/environ_test.go#newcode96
provider/null/environ_test.go:96: }
I don't see any tests for EnableBootstrapStorage.

https://codereview.appspot.com/14011046/

[Roger Peppe (rogpeppe)]

I never did like the optional Environ interfaces, and now I have another
reason - they break encapsulation.
My preferred solution here would be to remove all those interfaces so we
have One True Environ Interface, and leave the code as it was before.

Perhaps there's another way of solving this issue without adding the
fairly icky EnableBootstrapStorage. Could we make the provider use its
bootstrap storage automatically if it detects that it has not been
bootstrapped? Then we could lose all the externally visible
BootstrapStorage stuff, perhaps.

Looks reasonable in general; some comments and suggestions below (the
sshstorage ones should be addressed in a separate CL).

https://codereview.appspot.com/14011046/diff/1/environs/manual/bootstrap.go
File environs/manual/bootstrap.go (right):

https://codereview.appspot.com/14011046/diff/1/environs/manual/bootstrap.go#newcode67
environs/manual/bootstrap.go:67: bootstrapStorage :=
args.Environ.Storage()
move this down to just before it's used, perhaps?

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go#newcode263
environs/sshstorage/storage.go:263: if _, err := io.ReadFull(r, buf);
err != nil {
I'm not very happy about this. Why are we reading in
the entire input when we can stream it?

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go#newcode267
environs/sshstorage/storage.go:267: tmpdir := utils.ShQuote(s.tmpdir)
Given that we're using these paths unquoted in many places,
I'd like to see a sanity check that they don't contain
any dubious characters.

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go#newcode272
environs/sshstorage/storage.go:272: "export TMPDIR=%s &&
TMPFILE=`mktemp` && ((%s && mv $TMPFILE %s) || rm -f $TMPFILE)",
What's TMPDIR used for?

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#newcode126
provider/null/environ.go:126: func (e *nullEnviron)
EnableBootstrapStorage(enable bool) (wasEnabled bool, err error) {
On 2013/09/27 09:47:33, fwereade wrote:
> We should be locking here, shouldn't we? Even if it makes no practical
> difference today, environs should be goroutine-safe.

+1

But also, why even have the option to disable bootstrap storage?
Will we ever need to do that? This method is used in one quite
specialised way - I don't see that we need to clutter its interface
too much. I'd lose the enable arg and the wasEnabled return value.

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#newcode133
provider/null/environ.go:133: return false, errors.New("bootstrap
storage already disabled")
Can't we just make this method idempotent?

https://codereview.appspot.com/14011046/

[Andrew Wilkins (axwalk)]

Thanks for the reviews.

rog: agreed, it would be *much* nicer if this could be determined
automatically. I don't have a solution to that, though.

I'll send another CL to fix some sshstorage things.

https://codereview.appspot.com/14011046/diff/1/environs/manual/bootstrap.go
File environs/manual/bootstrap.go (right):

https://codereview.appspot.com/14011046/diff/1/environs/manual/bootstrap.go#newcode67
environs/manual/bootstrap.go:67: bootstrapStorage :=
args.Environ.Storage()
On 2013/09/27 10:35:25, rog wrote:
> move this down to just before it's used, perhaps?

Done.

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go#newcode263
environs/sshstorage/storage.go:263: if _, err := io.ReadFull(r, buf);
err != nil {
On 2013/09/27 10:35:25, rog wrote:
> I'm not very happy about this. Why are we reading in
> the entire input when we can stream it?

Because we can't? There are multiple input streams to a long-lived bash
process, where the data input is interleaved with commands. If there's a
better way to do it, I'm happy to make changes.

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go#newcode267
environs/sshstorage/storage.go:267: tmpdir := utils.ShQuote(s.tmpdir)
On 2013/09/27 10:35:25, rog wrote:
> Given that we're using these paths unquoted in many places,
> I'd like to see a sanity check that they don't contain
> any dubious characters.

Where are we using them unquoted? I found one occurrence of s.remotepath
after adding a test (good call), but that doesn't equate to "in many
places". AFAICS, all other cases are ShQuoted.

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go#newcode272
environs/sshstorage/storage.go:272: "export TMPDIR=%s &&
TMPFILE=`mktemp` && ((%s && mv $TMPFILE %s) || rm -f $TMPFILE)",
On 2013/09/27 10:35:25, rog wrote:
> What's TMPDIR used for?

Just to influence mktemp. It was previously set elsewhere, and I just
failed to notice the obvious simplification. I'll update this to call
`mktemp --tmpdir=...`

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go
File provider/null/environ.go (left):

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#oldcode43
provider/null/environ.go:43: }
On 2013/09/27 09:47:33, fwereade wrote:
> It's quite nice to do "var _" type asserts by the definition of the
type, given
> that we seem to be growing optional interfaces at a reasonable rate.
Compiled
> documentation, if you like, that stops you having to read all the
methods.

Yeah, I was aware of the convention. As an experiment, I put the
assertion in a test, as it feels wrong to impose a package dependency
just for the assertion. Binary size won't change, but compilation speed
will take a hit.

But, point taken about documentation. I've added in here, and dropped
the assertion test. A short lived experiment ;)

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#...

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/14011046/

[Roger Peppe (rogpeppe)]

> rog: agreed, it would be *much* nicer if this could be determined
automatically.
> I don't have a solution to that, though.

One possibility might be to make the null provider storage fall back to
bootstrap storage if the regular storage is not available. We could make
the null provider Prepare ssh to the bootstrap host and start a stub
server (in some portable language, so we don't need to push tools
across) that simply returns an error, say "awaiting bootstrap" to any
request; then at bootstrap time we'd replace that by the real storage
server.

This would all be more simple, of course, if we stipulated that the null
provider always bootstrapped using the current machine for the bootstrap
machine. I imagine that's off the cards though.

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go#newcode263
environs/sshstorage/storage.go:263: if _, err := io.ReadFull(r, buf);
err != nil {
On 2013/09/29 16:05:25, axw1 wrote:
> On 2013/09/27 10:35:25, rog wrote:
> > I'm not very happy about this. Why are we reading in
> > the entire input when we can stream it?

> Because we can't? There are multiple input streams to a long-lived
bash process,
> where the data input is interleaved with commands. If there's a better
way to do
> it, I'm happy to make changes.

I'm probably missing something, but it seems to me that
the buffer is passed to s.run, then encoded to base64
(line 145), the result of which is used for its length (line 148),
and then written with Write (line 157).

It seems to me that you could pass in the reader and length directly,
create a base64.Encoder use base64.EncodedLen and io.Copy to
the encoder to stream the input.

The down side is if there happen to be less bytes than
advertised in the stream, your remote script will get out of sync.
You could use a more robust input reader than
"head -q -c 999". How about using a here document instead?
"base64 -d << '@EOF' | (%s)"

Then you could ignore the expected length and stream
the input, relying on the shell to find the end of it.

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go#newcode267
environs/sshstorage/storage.go:267: tmpdir := utils.ShQuote(s.tmpdir)
On 2013/09/29 16:05:25, axw1 wrote:
> On 2013/09/27 10:35:25, rog wrote:
> > Given that we're using these paths unquoted in many places,
> > I'd like to see a sanity check that they don't contain
> > any dubious characters.

> Where are we using them unquoted? I found one occurrence of
s.remotepath after
> adding a test (good call), but that doesn't equate to "in many
places". AFAICS,
> all other cases are ShQuoted.

I'm not sure what I was thinking when I wrote that comment :)

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go#newcode272
environs/sshstorage/storage.go:272: "export TMPDIR=%s &&
TMPFILE=`mktemp` && ((%s && mv $TMPFILE %s) || rm -f $TMPFILE)",
On 2013/09/29 16:05:25, axw1 wrote:
> On 2013/09/27 10:35:25, rog wrote:
> > What's TMPDIR used for?

> Just to influence mktemp. It was previously set elsewhere, and I just...

[Andrew Wilkins (axwalk)]

On 2013/09/30 13:31:08, rog wrote:
> > rog: agreed, it would be *much* nicer if this could be determined
> automatically.
> > I don't have a solution to that, though.

> One possibility might be to make the null provider storage fall back
to
> bootstrap storage if the regular storage is not available. We could
make the
> null provider Prepare ssh to the bootstrap host and start a stub
server (in some
> portable language, so we don't need to push tools across) that simply
returns an
> error, say "awaiting bootstrap" to any request; then at bootstrap time
we'd
> replace that by the real storage server.

I'll have to have a think about that some. I don't really like the idea
of requiring additional VMs/interpreters/etc. to support this, as we may
want to expand to other target OSs. Anyway, can we deal with this
outside of this CL? I can open a tech-debt bug to investigate it after
1.16.0 is out.

> This would all be more simple, of course, if we stipulated that the
null
> provider always bootstrapped using the current machine for the
bootstrap
> machine. I imagine that's off the cards though.

I think so. Having to connect to the bootstrap machine is (if only a
little) more onerous than necessary.

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go
> File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go#newcode263
> environs/sshstorage/storage.go:263: if _, err := io.ReadFull(r, buf);
err != nil
> {
> On 2013/09/29 16:05:25, axw1 wrote:
> > On 2013/09/27 10:35:25, rog wrote:
> > > I'm not very happy about this. Why are we reading in
> > > the entire input when we can stream it?
> >
> > Because we can't? There are multiple input streams to a long-lived
bash
> process,
> > where the data input is interleaved with commands. If there's a
better way to
> do
> > it, I'm happy to make changes.

> I'm probably missing something, but it seems to me that
> the buffer is passed to s.run, then encoded to base64
> (line 145), the result of which is used for its length (line 148),
> and then written with Write (line 157).

Now we're even; I don't know what *I* was thinking when I wrote what I
did ;)
Yes, I think you're right and we could just Write with a chained reader.
I will handle this in an update to
https://codereview.appspot.com/14102043/

> It seems to me that you could pass in the reader and length directly,
> create a base64.Encoder use base64.EncodedLen and io.Copy to
> the encoder to stream the input.

> The down side is if there happen to be less bytes than
> advertised in the stream, your remote script will get out of sync.
> You could use a more robust input reader than
> "head -q -c 999". How about using a here document instead?
> "base64 -d << '@EOF' | (%s)"

> Then you could ignore the expected length and stream
> the input, relying on the shell to find the end of it.

This is pretty similar to how I did it originally, though the input was
in a here-doc on the command line ;) I'm not familiar with that @EOF
syntax - will have to read up.

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go#newcode267
> environs/sshst...

[Mark Ramm (mark-ramm)]

> I'll have to have a think about that some. I don't really like the idea
> of requiring additional VMs/interpreters/etc. to support this, as we may
> want to expand to other target OSs. Anyway, can we deal with this
> outside of this CL? I can open a tech-debt bug to investigate it after
> 1.16.0 is out.

I don't think supporting bootstrap nodes running anything other than Ubuntu is an important feature at this time. Of course someday that may not be true -- but unlike unit machines, I don't see any compelling market forces for being OS agnostic on the bootstrap node.

That said, I'm generally a proponent of getting this landed, and creating a follow up task to handle any kind of "one true environment interface" style cleanup later. Just my opinion, and I will bow to the feelings of the group.

Side note -- have we considered using MongoDB and GridFS to provide a standard fallback storage provider everywhere that there's not "object storage" in the cloud already?

[Andrew Wilkins (axwalk)]

> I don't think supporting bootstrap nodes running anything other than Ubuntu is
> an important feature at this time. Of course someday that may not be true --
> but unlike unit machines, I don't see any compelling market forces for being
> OS agnostic on the bootstrap node.

Fair enough, I'll bear that in mind.

> That said, I'm generally a proponent of getting this landed, and creating a
> follow up task to handle any kind of "one true environment interface" style
> cleanup later. Just my opinion, and I will bow to the feelings of the group.
>
> Side note -- have we considered using MongoDB and GridFS to provide a standard
> fallback storage provider everywhere that there's not "object storage" in the
> cloud already?

I haven't, though it's known that the current storage solution is a bit weak, and won't stand up in an HA environment. This is just a first cut - we can do better. GridFS sounds like a pretty reasonable option.

[Tim Penhey (thumper)]

LGTM with one comment on base test suite.

https://codereview.appspot.com/14011046/diff/11001/provider/null/environ_test.go
File provider/null/environ_test.go (right):

https://codereview.appspot.com/14011046/diff/11001/provider/null/environ_test.go#newcode24
provider/null/environ_test.go:24: testbase.CleanupSuite
I'd recommend using the LoggingSuite just because it catches the logging
that may happen anywhere, and puts it in the test log, so it is visible
in the test failures.

https://codereview.appspot.com/14011046/

[Andrew Wilkins (axwalk)]

Thanks for the review.

https://codereview.appspot.com/14011046/diff/11001/provider/null/environ_test.go
File provider/null/environ_test.go (right):

https://codereview.appspot.com/14011046/diff/11001/provider/null/environ_test.go#newcode24
provider/null/environ_test.go:24: testbase.CleanupSuite
On 2013/10/02 04:26:49, thumper wrote:
> I'd recommend using the LoggingSuite just because it catches the
logging that
> may happen anywhere, and puts it in the test log, so it is visible in
the test
> failures.

Done.

https://codereview.appspot.com/14011046/

=== modified file 'cmd/juju/bootstrap.go'
--- cmd/juju/bootstrap.go	2013-09-30 16:23:40 +0000
+++ cmd/juju/bootstrap.go	2013-10-02 05:19:29 +0000
@@ -18,7 +18,6 @@
 	"launchpad.net/juju-core/environs/bootstrap"
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/environs/configstore"
-	"launchpad.net/juju-core/environs/storage"
 	"launchpad.net/juju-core/environs/sync"
 	envtools "launchpad.net/juju-core/environs/tools"
 	"launchpad.net/juju-core/errors"
@@ -75,11 +74,9 @@
 	// If the environment has a special bootstrap Storage, use it wherever
 	// we'd otherwise use environ.Storage.
 	if bs, ok := environ.(environs.BootstrapStorager); ok {
-		bootstrapStorage, err := bs.BootstrapStorage()
-		if err != nil {
-			return fmt.Errorf("failed to acquire bootstrap storage: %v", err)
+		if err := bs.EnableBootstrapStorage(); err != nil {
+			return fmt.Errorf("failed to enable bootstrap storage: %v", err)
 		}
-		environ = &bootstrapStorageEnviron{environ, bootstrapStorage}
 	}
 	// TODO (wallyworld): 2013-09-20 bug 1227931
 	// We can set a custom tools data source instead of doing an
@@ -230,12 +227,3 @@
 	}
 	return unique.Values()
 }
-
-type bootstrapStorageEnviron struct {
-	environs.Environ
-	bootstrapStorage storage.Storage
-}
-
-func (b *bootstrapStorageEnviron) Storage() storage.Storage {
-	return b.bootstrapStorage
-}

=== modified file 'environs/httpstorage/storage.go'
--- environs/httpstorage/storage.go	2013-09-23 10:05:53 +0000
+++ environs/httpstorage/storage.go	2013-10-02 05:19:29 +0000
@@ -141,6 +141,7 @@
 	if err != nil {
 		return err
 	}
+
 	// Here we wrap up the reader.  For some freaky unexplainable reason, the
 	// http library will call Close on the reader if it has a Close method
 	// available.  Since we sometimes reuse the reader, especially when

=== modified file 'environs/interface.go'
--- environs/interface.go	2013-09-27 08:14:36 +0000
+++ environs/interface.go	2013-10-02 05:19:29 +0000
@@ -64,15 +64,18 @@
 	PublicStorage() storage.StorageReader
 }
 
-// BootstrapStorager is an interface that returns a environs.Storage that may
-// be used before the bootstrap machine agent has been provisioned.
+// BootstrapStorager is an interface through which an Environ may be
+// instructed to use a special "bootstrap storage". Bootstrap storage
+// is one that may be used before the bootstrap machine agent has been
+// provisioned.
 //
-// This is useful for environments where the storage is managed by the machine
-// agent once bootstrapped.
+// This is useful for environments where the storage is managed by the
+// machine agent once bootstrapped.
 type BootstrapStorager interface {
-	// BootstrapStorager returns an environs.Storage that may be used while
-	// bootstrapping a machine.
-	BootstrapStorage() (storage.Storage, error)
+	// EnableBootstrapStorage enables bootstrap storage, returning an
+	// error if enablement failed. If nil is returned, then calling
+	// this again will have no effect and will return nil.
+	EnableBootstrapStorage() error
 }
 
 // ConfigGetter implements access to an environments configuration.

=== modified file 'environs/manual/bootstrap.go'
--- environs/manual/bootstrap.go	2013-09-30 19:40:06 +0000
+++ environs/manual/bootstrap.go	2013-10-02 05:19:29 +0000
@@ -24,7 +24,6 @@
 // manages its own local storage.
 type LocalStorageEnviron interface {
 	environs.Environ
-	environs.BootstrapStorager
 	localstorage.LocalStorageConfig
 }
 
@@ -65,11 +64,6 @@
 		return ErrProvisioned
 	}
 
-	bootstrapStorage, err := args.Environ.BootstrapStorage()
-	if err != nil {
-		return err
-	}
-
 	hc, series, err := detectSeriesAndHardwareCharacteristics(args.Host)
 	if err != nil {
 		return fmt.Errorf("error detecting hardware characteristics: %v", err)
@@ -87,6 +81,7 @@
 
 	// Store the state file. If provisioning fails, we'll remove the file.
 	logger.Infof("Saving bootstrap state file to bootstrap storage")
+	bootstrapStorage := args.Environ.Storage()
 	err = common.SaveState(
 		bootstrapStorage,
 		&common.BootstrapState{

=== modified file 'environs/manual/bootstrap_test.go'
--- environs/manual/bootstrap_test.go	2013-09-30 19:40:06 +0000
+++ environs/manual/bootstrap_test.go	2013-10-02 05:19:29 +0000
@@ -33,8 +33,8 @@
 	sharedStorageDir  string
 }
 
-func (e *localStorageEnviron) BootstrapStorage() (storage.Storage, error) {
-	return e.storage, nil
+func (e *localStorageEnviron) Storage() storage.Storage {
+	return e.storage
 }
 
 func (e *localStorageEnviron) StorageAddr() string {
@@ -86,7 +86,7 @@
 	err := Bootstrap(args)
 	c.Assert(err, gc.IsNil)
 
-	bootstrapState, err := common.LoadState(s.env.storage)
+	bootstrapState, err := common.LoadState(s.env.Storage())
 	c.Assert(err, gc.IsNil)
 	c.Assert(
 		bootstrapState.StateInstances,
@@ -117,7 +117,7 @@
 
 	// Since the script failed, the state file should have been
 	// removed from storage.
-	_, err = common.LoadState(s.env.storage)
+	_, err = common.LoadState(s.env.Storage())
 	c.Check(err, gc.Equals, environs.ErrNotBootstrapped)
 }
 

=== modified file 'environs/sshstorage/storage.go'
--- environs/sshstorage/storage.go	2013-09-24 01:55:30 +0000
+++ environs/sshstorage/storage.go	2013-10-02 05:19:29 +0000
@@ -260,7 +260,7 @@
 		return err
 	}
 	buf := make([]byte, length)
-	if _, err := r.Read(buf); err != nil {
+	if _, err := io.ReadFull(r, buf); err != nil {
 		return err
 	}
 	path = utils.ShQuote(path)

=== modified file 'provider/null/environ.go'
--- provider/null/environ.go	2013-09-30 19:40:06 +0000
+++ provider/null/environ.go	2013-10-02 05:19:29 +0000
@@ -14,8 +14,10 @@
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/environs/httpstorage"
 	"launchpad.net/juju-core/environs/manual"
+	"launchpad.net/juju-core/environs/simplestreams"
 	"launchpad.net/juju-core/environs/sshstorage"
 	"launchpad.net/juju-core/environs/storage"
+	envtools "launchpad.net/juju-core/environs/tools"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/provider/common"
 	"launchpad.net/juju-core/state"
@@ -38,10 +40,15 @@
 )
 
 type nullEnviron struct {
-	cfg      *environConfig
-	cfgmutex sync.Mutex
+	cfg                   *environConfig
+	cfgmutex              sync.Mutex
+	bootstrapStorage      *sshstorage.SSHStorage
+	bootstrapStorageMutex sync.Mutex
 }
 
+var _ environs.BootstrapStorager = (*nullEnviron)(nil)
+var _ envtools.SupportsCustomSources = (*nullEnviron)(nil)
+
 var errNoStartInstance = errors.New("null provider cannot start instances")
 var errNoStopInstance = errors.New("null provider cannot stop instances")
 
@@ -120,15 +127,39 @@
 	return instances, err
 }
 
-// Implements environs/bootstrap.BootstrapStorage.
-func (e *nullEnviron) BootstrapStorage() (storage.Storage, error) {
+// Implements environs.BootstrapStorager.
+func (e *nullEnviron) EnableBootstrapStorage() error {
+	e.bootstrapStorageMutex.Lock()
+	defer e.bootstrapStorageMutex.Unlock()
+	if e.bootstrapStorage != nil {
+		return nil
+	}
 	cfg := e.envConfig()
 	storageDir := e.StorageDir()
 	storageTmpdir := path.Join(dataDir, storageTmpSubdir)
-	return sshstorage.NewSSHStorage(cfg.sshHost(), storageDir, storageTmpdir)
+	bootstrapStorage, err := sshstorage.NewSSHStorage(cfg.sshHost(), storageDir, storageTmpdir)
+	if err != nil {
+		return err
+	}
+	e.bootstrapStorage = bootstrapStorage
+	return nil
+}
+
+// GetToolsSources returns a list of sources which are
+// used to search for simplestreams tools metadata.
+func (e *nullEnviron) GetToolsSources() ([]simplestreams.DataSource, error) {
+	// Add the simplestreams source off private storage.
+	return []simplestreams.DataSource{
+		storage.NewStorageSimpleStreamsDataSource(e.Storage(), storage.BaseToolsPath),
+	}, nil
 }
 
 func (e *nullEnviron) Storage() storage.Storage {
+	e.bootstrapStorageMutex.Lock()
+	defer e.bootstrapStorageMutex.Unlock()
+	if e.bootstrapStorage != nil {
+		return e.bootstrapStorage
+	}
 	return httpstorage.Client(e.envConfig().storageAddr())
 }
 

=== modified file 'provider/null/environ_test.go'
--- provider/null/environ_test.go	2013-09-26 09:02:09 +0000
+++ provider/null/environ_test.go	2013-10-02 05:19:29 +0000
@@ -4,14 +4,24 @@
 package null
 
 import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"strings"
+
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/manual"
+	"launchpad.net/juju-core/environs/sshstorage"
+	"launchpad.net/juju-core/environs/tools"
 	"launchpad.net/juju-core/instance"
+	jc "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/testing/testbase"
 )
 
 type environSuite struct {
+	testbase.LoggingSuite
 	env *nullEnviron
 }
 
@@ -79,3 +89,36 @@
 	c.Assert(s.env.SharedStorageAddr(), gc.Equals, "")
 	c.Assert(s.env.SharedStorageDir(), gc.Equals, "")
 }
+
+func (s *environSuite) TestEnvironSupportsCustomSources(c *gc.C) {
+	sources, err := tools.GetMetadataSources(s.env)
+	c.Assert(err, gc.IsNil)
+	c.Assert(len(sources), gc.Equals, 2)
+	url, err := sources[0].URL("")
+	c.Assert(err, gc.IsNil)
+	c.Assert(strings.Contains(url, "/tools"), jc.IsTrue)
+}
+
+func (s *environSuite) TestEnvironBootstrapStorager(c *gc.C) {
+	const sshScript = "#!/bin/sh\necho JUJU-RC: $RC"
+	bin := c.MkDir()
+	ssh := filepath.Join(bin, "ssh")
+	err := ioutil.WriteFile(ssh, []byte(sshScript), 0755)
+	c.Assert(err, gc.IsNil)
+	s.PatchEnvironment("PATH", bin+":"+os.Getenv("PATH"))
+
+	s.PatchEnvironment("RC", "99") // simulate ssh failure
+	err = s.env.EnableBootstrapStorage()
+	c.Assert(err, gc.ErrorMatches, "exit code 99")
+	c.Assert(s.env.Storage(), gc.Not(gc.FitsTypeOf), new(sshstorage.SSHStorage))
+
+	s.PatchEnvironment("RC", "0")
+	err = s.env.EnableBootstrapStorage()
+	c.Assert(err, gc.IsNil)
+	c.Assert(s.env.Storage(), gc.FitsTypeOf, new(sshstorage.SSHStorage))
+
+	// Check idempotency
+	err = s.env.EnableBootstrapStorage()
+	c.Assert(err, gc.IsNil)
+	c.Assert(s.env.Storage(), gc.FitsTypeOf, new(sshstorage.SSHStorage))
+}