Series support

[William Reade]

On Wed, 2013-03-13 at 14:18 +1300, Tim Penhey wrote:
> Hi William,
> 
> tl;dr - proposal and questions repeated at the end of the email.
> 
> Note: I've tagged all series tasks with "series" for nice filtering.

Lovely, thanks.

> As mostly expected, I start looking at this work and feel blocked by
> questions :-)  This email is a little rambly...

Sorry, so's the response. We should talk :).

> I'll see if I can enumerate enough of them to get started effectively.
> Part of this is kinda like a pre-implementation check, as I have a few
> ideas which may or may not make real sense.
> 
> The first card I have assigned looks like two very separate branches:
>   * add series to environs interface (instance launching, includes
> bootstrap)
>     - add series to envrons.Bootstrap
>     - taking tools out of the interface
> 
> Now I started looking at this again, and it almost makes sense, although
> I am confused by what is meant by "environs interface".  Does it mean:
> add a function "Series() string" to the interface environs.Environ
> (which would seem weird)? Or does it mean that we add it to the
> environs.config?  Which already seems to have a DefaultSeries().

1) state.Tools should never be used in the Environ interface; this is
the core problem.
2) Series may actually be irrelevant to bootstrap: I can't see an
overriding reason to add an extra path that overrides DefaultSeries (at
this stage it's just extra work for very marginal benefit).
3) StartInstance certainly requires a series args.
4) Both Bootstrap and StartInstance certainly require Constraints args.

> so... environs.Bootstrap takes the following params:
>   environ Environ
>   uploadTools bool
>   writeCertAndKey func(environName string, cert, key []byte) error
> 
> First thing I suggest is that we take the write function out, and make
> it an explicit thing in the bootstrap command itself (in
> cmd/juju/bootstrap).

+1 -- all we should need to do is ensure that we have a CACert and key
available before we even try to bootstrap. We can generate a server
cert/key *much* later in the process; I don't see any justification for
generating them early and passing them around everywhere.

> This cleans up all the call-sites which reimplement panicWrite (6 times!).

sgtm (/sigh)

> Which leads to the first question:
>   Given that we panic in environs.Bootstrap if there is a CA private key
> but no certificate, what do we expect the user to do with this
> information?  Also interesting to note that the function  currently
> thinks it is fine if there is a CA Cert but no CA private key - is this
> valid?

Panicking sounds crazy (I'd missed that), as does not just requiring
that both exist. Roger, did I miss something here?

> Next, looking back at the suggested work, it says to take upload tools
> out of the Bootstrap params.  Where do we move it to?  Is it a function
> on the environment itself instead of a weird parameter to the
> environs.Bootstrap function.  This would have no impact on the actual
> command function except it would do something like:
> 
>   if c.UploadTools {
>     environ.UploadTools()
>   }
>   return environs.Bootstrap(environ, c.Series)

I don't think UploadTools needs anything except an Environ interface, so
I'd rather make it a free function; and I have no strong feelings as to
whether it should be inside Bootstrap or not. (Well, I don't myself
believe juju itself should be concerned with tools upload at all, but if
it's included I don't really mind whether it's part of the common
environs.Bootstrap logic or whether it's called by BootstrapCommand.)

> So... back to the series param, if we have a string param to
> environs.Bootstrap, I'm assuming we should allow the bootstrap command
> CLI to allow setting it.  And we pass through the requested series
> through, and it will end up with the provider looking to see if it is
> empty, and if so, use the default-series?

Yeah, if we decide it's worthwhile overriding default-series at all. IMO
default-series is probably good enough as it is, and skipping that saves
us a bit of work, so, meh.

> I think I've talked (or typed) myself into a reasonable understanding,
> and summarised below:
> 
> 
> Work Summary:

Responses repeated/clarified below.

> * Move cert creation and writing to the bootstrap command

*CA* cert creation and writing, yes. State server certs don't need to be
written locally at all, and I have no idea why they're dirtying up the
bootstrap args at the moment.

> * environs.Bootstrap errors out if no CA Cert defined or no CA Key defined

+1

> * Add "UploadTools" to the Environ interface, and remove the parameter
> from Bootstrap function.

-0 -- the write change above actually renders the common Bootstrap
function worthless, and I'm not sure that's a particularly great
direction. I think I'm leaning towards keeping it in
environs.Bootstrap(); it is after all common business logic.

> * Add a "series string" parameter to environs.Bootstrap, which then gets
> passed through to Environ.Bootstrap.

-0 -- now I've thought about it more, I'm not really seeing the value of
an extra way to specify the series of the bootstrap machine.

> Questions:
> 
> * Is it valid to have a CA Cert but no CA private key?

Don't think so.

> * If we make an UploadTools method on environs.Environ, should it take a
> series parameter?

It can't at the moment: if we stick with the tools-per-series thing, we
can only build for the current series anyway.

> * Should environs.Environ.Bootstrap be:
> 	Bootstrap(series string, stateServerCert, stateServerKey []byte) error
> or
> 	Bootstrap(stateServerCert, stateServerKey []byte, series string) error

Honestly? I think it should be `Bootstrap() error`. Series can come from
default-series, and the state server cert/key can be generated from the
CA cert/key when we actually need it. This may need to come later.

> Sound sane?

Parts sound sane, parts need a face-to-face chat, I think. I'll come
online for a G+ late tonight, and pop out for breakfast now to
compensate ;).

Cheers
William

> Tim