state: add CACert to Info.

state: add CACert to Info.

We will need to know the CA certificate when connecting to the state.

We don't make it mandatory when connecting to the state yet, because
that requires quite a few more changes, and this is a reasonable stepping
stone along the way.

https://code.launchpad.net/~rogpeppe/juju-core/149-state-info-rootcert/+merge/134642

Requires: https://code.launchpad.net/~rogpeppe/juju-core/146-cmd-bootstrap-use-juju/+merge/134437

(do not edit description out of merge proposal)

[rog, 2012-11-16 12:06:01]

Please take a look.

[jameinel, 2012-11-18 09:56:18]

LGTM
I'm not sure about the specifics for whether adding RootCertPEM to StateInfo is
the best way forward, but provided that it is, the changes all look good.

One small comment about 'rootCert' because the name doesn't seem to be quite
enough description.

https://codereview.appspot.com/6855054/diff/4001/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/6855054/diff/4001/cmd/jujud/agent.go#newcode203
cmd/jujud/agent.go:203: 
Should the TODO(rog) go here, instead of the individual entries?

Should we call this something different than just 'rootCert'? At the very least,
some sort of comment about what this actually is certifying. It certainly is
hard to tell from the armored text. :)

[niemeyer, 2012-11-19 12:50:10]

https://codereview.appspot.com/6855054/diff/4001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6855054/diff/4001/environs/ec2/ec2.go#newcode251
environs/ec2/ec2.go:251: // TODO(rog) add RootCertPEM from bootstrap arguments.
Shouldn't we have this in the environment configuration? We don't need to change
the bootstrap arguments if so, and already have it in context.

https://codereview.appspot.com/6855054/diff/4001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6855054/diff/4001/environs/interface.go#newcod...
environs/interface.go:140: // empty.
Why would it be empty? Don't we have it in the environment configuration?

https://codereview.appspot.com/6855054/diff/4001/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/6855054/diff/4001/state/export_test.go#newcode24
state/export_test.go:24: func TestingStateInfo() *Info {
Why is this here? There's nothing being exported.

https://codereview.appspot.com/6855054/diff/4001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/6855054/diff/4001/state/open.go#newcode32
state/open.go:32: RootCertPEM []byte
// CACert holds the CA certificate that will be used
// to validate the state server's certificate.
CACert []byte

https://codereview.appspot.com/6855054/diff/4001/testing/cert.go
File testing/cert.go (right):

https://codereview.appspot.com/6855054/diff/4001/testing/cert.go#newcode26
testing/cert.go:26: RootCertPEMBytes = []byte(RootCertPEM)
Can we please either define everything as bytes or everything as strings, and
avoid all these little helpers? I'd prefer to read more in the test than to have
to come here and figure what all the little variations mean.

[rog, 2012-11-19 13:23:42]

https://codereview.appspot.com/6855054/diff/4001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6855054/diff/4001/environs/ec2/ec2.go#newcode251
environs/ec2/ec2.go:251: // TODO(rog) add RootCertPEM from bootstrap arguments.
On 2012/11/19 12:50:10, niemeyer wrote:
> Shouldn't we have this in the environment configuration? We don't need to
change
> the bootstrap arguments if so, and already have it in context.

Yes. I proposed this CL before we had decided to put the root certificate in the
environment configuration however. Will change the TODO.

https://codereview.appspot.com/6855054/diff/4001/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/6855054/diff/4001/state/export_test.go#newcode24
state/export_test.go:24: func TestingStateInfo() *Info {
On 2012/11/19 12:50:10, niemeyer wrote:
> Why is this here? There's nothing being exported.

It's here so both internal and external tests can use it. If you'd prefer, I can
define it both internally and externally. Or I could put it in its own file. Or
choose one of the internal test files to put it in. Any one will do, I don't
mind.

https://codereview.appspot.com/6855054/diff/4001/testing/cert.go
File testing/cert.go (right):

https://codereview.appspot.com/6855054/diff/4001/testing/cert.go#newcode26
testing/cert.go:26: RootCertPEMBytes = []byte(RootCertPEM)
On 2012/11/19 12:50:10, niemeyer wrote:
> Can we please either define everything as bytes or everything as strings, and
> avoid all these little helpers? I'd prefer to read more in the test than to
have
> to come here and figure what all the little variations mean.

will do

[rog, 2012-11-20 14:35:32]

Please take a look.

https://codereview.appspot.com/6855054/diff/4001/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/6855054/diff/4001/state/export_test.go#newcode24
state/export_test.go:24: func TestingStateInfo() *Info {
On 2012/11/19 13:23:43, rog wrote:
> On 2012/11/19 12:50:10, niemeyer wrote:
> > Why is this here? There's nothing being exported.
> 
> It's here so both internal and external tests can use it. If you'd prefer, I
can
> define it both internally and externally. Or I could put it in its own file.
Or
> choose one of the internal test files to put it in. Any one will do, I don't
> mind.

I've left it here for the time being, pending a reply.

[niemeyer, 2012-11-20 22:26:19]

https://codereview.appspot.com/6855054/diff/4001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6855054/diff/4001/environs/interface.go#newcod...
environs/interface.go:140: // empty.
On 2012/11/19 12:50:10, niemeyer wrote:
> Why would it be empty? Don't we have it in the environment configuration?

Unhandled comment.

https://codereview.appspot.com/6855054/diff/4001/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/6855054/diff/4001/state/export_test.go#newcode24
state/export_test.go:24: func TestingStateInfo() *Info {
On 2012/11/19 13:23:43, rog wrote:
> On 2012/11/19 12:50:10, niemeyer wrote:
> > Why is this here? There's nothing being exported.
> 
> It's here so both internal and external tests can use it. If you'd prefer, I
can
> define it both internally and externally. Or I could put it in its own file.
Or
> choose one of the internal test files to put it in. Any one will do, I don't
> mind.

There's a reason why this file is called "export", and I'd prefer to respect it.
Right above the use of this function in settings_test.go seems like a pretty
reasonable place to have it.

https://codereview.appspot.com/6855054/diff/4001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/6855054/diff/4001/state/open.go#newcode32
state/open.go:32: RootCertPEM []byte
On 2012/11/19 12:50:10, niemeyer wrote:
> // CACert holds the CA certificate that will be used
> // to validate the state server's certificate.
> CACert []byte

Comment is unhandled.

[rog, 2012-11-21 00:22:55]

Assuming these issues are addressed (yes, the StateInfo method can just
return the root cert from the config), does this look good to you?
On Nov 20, 2012 10:26 PM, <n13m3y3r@gmail.com> wrote:

>
>
https://codereview.appspot.**com/6855054/diff/4001/**environs/interface.go<ht...
> File environs/interface.go (right):
>
> https://codereview.appspot.**com/6855054/diff/4001/**
>
environs/interface.go#**newcode140<https://codereview.appspot.com/6855054/diff/4001/environs/interface.go#newcode140>
> environs/interface.go:140: // empty.
> On 2012/11/19 12:50:10, niemeyer wrote:
>
>> Why would it be empty? Don't we have it in the environment
>>
> configuration?
>
> Unhandled comment.
>
>
https://codereview.appspot.**com/6855054/diff/4001/state/**export_test.go<htt...
> File state/export_test.go (right):
>
> https://codereview.appspot.**com/6855054/diff/4001/state/**
>
export_test.go#newcode24<https://codereview.appspot.com/6855054/diff/4001/state/export_test.go#newcode24>
> state/export_test.go:24: func TestingStateInfo() *Info {
> On 2012/11/19 13:23:43, rog wrote:
>
>> On 2012/11/19 12:50:10, niemeyer wrote:
>> > Why is this here? There's nothing being exported.
>>
>
>  It's here so both internal and external tests can use it. If you'd
>>
> prefer, I can
>
>> define it both internally and externally. Or I could put it in its own
>>
> file. Or
>
>> choose one of the internal test files to put it in. Any one will do, I
>>
> don't
>
>> mind.
>>
>
> There's a reason why this file is called "export", and I'd prefer to
> respect it. Right above the use of this function in settings_test.go
> seems like a pretty reasonable place to have it.
>
>
https://codereview.appspot.**com/6855054/diff/4001/state/**open.go<https://co...
> File state/open.go (right):
>
> https://codereview.appspot.**com/6855054/diff/4001/state/**
>
open.go#newcode32<https://codereview.appspot.com/6855054/diff/4001/state/open.go#newcode32>
> state/open.go:32: RootCertPEM []byte
> On 2012/11/19 12:50:10, niemeyer wrote:
>
>> // CACert holds the CA certificate that will be used
>> // to validate the state server's certificate.
>> CACert []byte
>>
>
> Comment is unhandled.
>
>
https://codereview.appspot.**com/6855054/<https://codereview.appspot.com/6855...
>

[rog, 2012-11-23 11:12:51]

Please take a look.

https://codereview.appspot.com/6855054/diff/4001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6855054/diff/4001/environs/interface.go#newcod...
environs/interface.go:140: // empty.
On 2012/11/20 22:26:19, niemeyer wrote:
> On 2012/11/19 12:50:10, niemeyer wrote:
> > Why would it be empty? Don't we have it in the environment configuration?
> 
> Unhandled comment.

Done.

https://codereview.appspot.com/6855054/diff/4001/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/6855054/diff/4001/state/export_test.go#newcode24
state/export_test.go:24: func TestingStateInfo() *Info {
On 2012/11/20 22:26:19, niemeyer wrote:
> On 2012/11/19 13:23:43, rog wrote:
> > On 2012/11/19 12:50:10, niemeyer wrote:
> > > Why is this here? There's nothing being exported.
> > 
> > It's here so both internal and external tests can use it. If you'd prefer, I
> can
> > define it both internally and externally. Or I could put it in its own file.
> Or
> > choose one of the internal test files to put it in. Any one will do, I don't
> > mind.
> 
> There's a reason why this file is called "export", and I'd prefer to respect
it.
> Right above the use of this function in settings_test.go seems like a pretty
> reasonable place to have it.

Done.

https://codereview.appspot.com/6855054/diff/4001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/6855054/diff/4001/state/open.go#newcode32
state/open.go:32: RootCertPEM []byte
On 2012/11/20 22:26:19, niemeyer wrote:
> On 2012/11/19 12:50:10, niemeyer wrote:
> > // CACert holds the CA certificate that will be used
> > // to validate the state server's certificate.
> > CACert []byte
> 
> Comment is unhandled.

Done.

[rog, 2012-11-23 11:18:27]

Please take a look.

[rog, 2012-11-23 11:23:39]

Please take a look.

[niemeyer, 2012-11-26 19:59:02]

LGTM. I suggest simplifying the field name, though:

https://codereview.appspot.com/6855054/diff/8020/state/open.go
File state/open.go (right):

https://codereview.appspot.com/6855054/diff/8020/state/open.go#newcode32
state/open.go:32: CACertPEM []byte
This can be CACert (no PEM), I think. The PEM was a disambiguation trick due to
the very long prefix we were using. I think it's fine to get rid of it now as
you suggested (and easy). As a plus it matches the configuration.

We do have to document the fact it's in PEM format, in that case.

[rog, 2012-11-27 10:17:28]

*** Submitted:

state: add CACert to Info.

We will need to know the CA certificate when connecting to the state.

We don't make it mandatory when connecting to the state yet, because
that requires quite a few more changes, and this is a reasonable stepping
stone along the way.

R=john.meinel, niemeyer
CC=
https://codereview.appspot.com/6855054