environs/agent: add APIInfo fields

environs/agent: add APIInfo fields

This will enable the machine agent to
connect to the API server.

https://code.launchpad.net/~rogpeppe/juju-core/188-agent-mongo-addr/+merge/143286

(do not edit description out of merge proposal)

[rog, 2013-01-15 12:06:03]

Please take a look.

[niemeyer, 2013-01-15 12:21:15]

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go#newc...
environs/agent/agent.go:37: OldAPIPassword string
Is "Old" the same as what used to be "initial"? Why the change? FWIW, it feels a
bit awkward to have an "old" password which is actually not old, and a current
password that will be pretty much always there without ever getting old.

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go#newc...
environs/agent/agent.go:55: // We use json rather than yaml because it deals
with
Doesn't feel like good motivation. We can fix that, or even easily code around
it here. A file format cannot be easily changed down the road, though. Can we
please stick to the well established convention instead?

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go#newc...
environs/agent/agent.go:124: func (c0 *Conf) Write() error {
Would be better to have the variable name below changed instead of having to
wonder why this is c0 when reading the method signature.

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go#newc...
environs/agent/agent.go:129: // don't needlessly duplicate the CA certificate.
I don't get the actual motivation behind this, but I may be missing context.
What's the real reason why this is not being written down? "needlessly" writing
a configuration setting to the configuration file needs some explanation, as
otherwise there's no real reason to save those few bytes.

[rog, 2013-01-15 13:02:56]

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go#newc...
environs/agent/agent.go:37: OldAPIPassword string
On 2013/01/15 12:21:15, niemeyer wrote:
> Is "Old" the same as what used to be "initial"? Why the change? FWIW, it feels
a
> bit awkward to have an "old" password which is actually not old, and a current
> password that will be pretty much always there without ever getting old.

"Old" was at the suggestion of william, who observed that it's trivial to get
password changing functionality by changing the value of the "initial password"
field. if we do that, then "initial" is a misnomer.

i'm happy to change it back though, although you might want to talk to william
first.

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go#newc...
environs/agent/agent.go:55: // We use json rather than yaml because it deals
with
On 2013/01/15 12:21:15, niemeyer wrote:
> Doesn't feel like good motivation. We can fix that, or even easily code around
> it here. A file format cannot be easily changed down the road, though. Can we
> please stick to the well established convention instead?

One alternative I considered was to use string for certificates and keys
throughout, but then there's an impedance mismatch with the core go crypto
primitives, which expect []byte.

To code around it here, we'll need to duplicate the State.Info and API.Info
structs here, AFAICS which leads to more code and more things to get wrong when
they change.

On the other hand a base-64 encoding of an already base-64-encoded value is not
very efficient, so what we've got here isn't great either.

Another alternative would be to define CACert as a value that implements
goyaml.Getter and goyaml.Setter; or similar for api.Info and state.Info.

What do you think?

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go#newc...
environs/agent/agent.go:124: func (c0 *Conf) Write() error {
On 2013/01/15 12:21:15, niemeyer wrote:
> Would be better to have the variable name below changed instead of having to
> wonder why this is c0 when reading the method signature.

will do.

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go#newc...
environs/agent/agent.go:129: // don't needlessly duplicate the CA certificate.
On 2013/01/15 12:21:15, niemeyer wrote:
> I don't get the actual motivation behind this, but I may be missing context.
> What's the real reason why this is not being written down? "needlessly"
writing
> a configuration setting to the configuration file needs some explanation, as
> otherwise there's no real reason to save those few bytes.

the motivation is that i'm concerned about exceeding the 16K cloud-init limit.
we're not far off already (10K AFAIR) and the certificate is a significant part
of that)

[niemeyer, 2013-01-15 17:19:58]

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go#newc...
environs/agent/agent.go:37: OldAPIPassword string
On 2013/01/15 13:02:56, rog wrote:
> On 2013/01/15 12:21:15, niemeyer wrote:
> > Is "Old" the same as what used to be "initial"? Why the change? FWIW, it
feels
> a
> > bit awkward to have an "old" password which is actually not old, and a
current
> > password that will be pretty much always there without ever getting old.
> 
> "Old" was at the suggestion of william, who observed that it's trivial to get
> password changing functionality by changing the value of the "initial
password"
> field. if we do that, then "initial" is a misnomer.
> 
> i'm happy to change it back though, although you might want to talk to william
> first.

I see. That sounds good.

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go#newc...
environs/agent/agent.go:55: // We use json rather than yaml because it deals
with
On 2013/01/15 13:02:56, rog wrote:
> On 2013/01/15 12:21:15, niemeyer wrote:
> > Doesn't feel like good motivation. We can fix that, or even easily code
around
> > it here. A file format cannot be easily changed down the road, though. Can
we
> > please stick to the well established convention instead?
> 
> One alternative I considered was to use string for certificates and keys
> throughout, but then there's an impedance mismatch with the core go crypto
> primitives, which expect []byte.
> 
> To code around it here, we'll need to duplicate the State.Info and API.Info
> structs here, AFAICS which leads to more code and more things to get wrong
when
> they change.
> 
> On the other hand a base-64 encoding of an already base-64-encoded value is
not
> very efficient, so what we've got here isn't great either.
> 
> Another alternative would be to define CACert as a value that implements
> goyaml.Getter and goyaml.Setter; or similar for api.Info and state.Info.
> 
> What do you think?

goyaml ought to marshal []byte as base64 by itself, using the !!binary tag:
http://yaml.org/type/binary.html

I suggest using plain yaml for now with goyaml, and down the road we just fix
goyaml in a compatible way so that it does the right thing and converts to
base64 transparently when saving.

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go#newc...
environs/agent/agent.go:129: // don't needlessly duplicate the CA certificate.
On 2013/01/15 13:02:56, rog wrote:
> On 2013/01/15 12:21:15, niemeyer wrote:
> > I don't get the actual motivation behind this, but I may be missing context.
> > What's the real reason why this is not being written down? "needlessly"
> writing
> > a configuration setting to the configuration file needs some explanation, as
> > otherwise there's no real reason to save those few bytes.
> 
> the motivation is that i'm concerned about exceeding the 16K cloud-init limit.
> we're not far off already (10K AFAIR) and the certificate is a significant
part
> of that)

That's writing an agent configuration to disk. How's cloud-init size playing a
role here?

This kind of note should also be part of the comment here, btw. As it is one has
to guess where that kind of logic comes from.

[fwereade, 2013-01-16 08:42:52]

LGTM modulo certificate-sharing clarity in Write.

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/7085062/diff/2001/environs/agent/agent.go#newc...
environs/agent/agent.go:129: // don't needlessly duplicate the CA certificate.
On 2013/01/15 17:19:59, niemeyer wrote:
> On 2013/01/15 13:02:56, rog wrote:
> > On 2013/01/15 12:21:15, niemeyer wrote:
> > > I don't get the actual motivation behind this, but I may be missing
context.
> > > What's the real reason why this is not being written down? "needlessly"
> > writing
> > > a configuration setting to the configuration file needs some explanation,
as
> > > otherwise there's no real reason to save those few bytes.
> > 
> > the motivation is that i'm concerned about exceeding the 16K cloud-init
limit.
> > we're not far off already (10K AFAIR) and the certificate is a significant
> part
> > of that)
> 
> That's writing an agent configuration to disk. How's cloud-init size playing a
> role here?
> 
> This kind of note should also be part of the comment here, btw. As it is one
has
> to guess where that kind of logic comes from.

Agree that the motivation in *this* context remains unclear, even if the goal is
sane and noble.

[rog, 2013-01-16 15:17:12]

Please take a look.

[fwereade, 2013-01-16 15:23:24]

LGTM, all the more so now it's smaller ;)

[rog, 2013-01-16 15:38:33]

Please take a look.

[niemeyer, 2013-01-18 14:33:22]

LGTM

[rog, 2013-01-18 14:42:14]

*** Submitted:

environs/agent: add APIInfo fields

This will enable the machine agent to
connect to the API server.

R=niemeyer, fwereade
CC=
https://codereview.appspot.com/7085062