Merge lp:~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0 into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

Make bootstrap machine jobs configurable

This addresses a long-standing TODO in
cmd/jujud to making bootstrap machine
jobs configurable. If agent.BootstrapJobs
is present in the agent configuration
key/value map, it will be used instead
of the defaults (the existing hard-coded
values).

The local provider is updated to set
agent.BootstrapJobs to be {JobManageEnviron}.

Fixes lp:1273933

https://codereview.appspot.com/60310044/

Make bootstrap machine jobs configurable

This addresses a long-standing TODO in
cmd/jujud to making bootstrap machine
jobs configurable. If agent.BootstrapJobs
is present in the agent configuration
key/value map, it will be used instead
of the defaults (the existing hard-coded
values).

The local provider is updated to set
agent.BootstrapJobs to be {JobManageEnviron}.

Fixes lp:1273933

https://codereview.appspot.com/60310044/

[Andrew Wilkins (axwalk)]

Reviewers: mp+204829_code.launchpad.net,

Message:
Please take a look.

Description:
Make bootstrap machine jobs configurable

This addresses a long-standing TODO in
cmd/jujud to making bootstrap machine
jobs configurable. If agent.BootstrapJobs
is present in the agent configuration
key/value map, it will be used instead
of the defaults (the existing hard-coded
values).

The local provider is updated to set
agent.BootstrapJobs to be {JobManageEnviron}.

Fixes lp:1273933

https://code.launchpad.net/~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0/+merge/204829

(do not edit description out of merge proposal)

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

Affected files (+84, -17 lines):
   A [revision details]
   M agent/agent.go
   M agent/bootstrap.go
   M agent/bootstrap_test.go
   M cmd/jujud/bootstrap.go
   M cmd/jujud/bootstrap_test.go
   M provider/local/environ.go
   M provider/local/environ_test.go

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/60310044/

[Roger Peppe (rogpeppe)]

I just noticed this in the code. It seems somewhat odd that this
is in the string attributes, rather than getting a field for itself.

Could you perhaps elucidate on the reasons why this is
the right approach?

On 10 February 2014 05:48, Ian Booth <email address hidden> wrote:
> LGTM
>
> https://codereview.appspot.com/60310044/
>
> --
> https://code.launchpad.net/~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0/+merge/204829
> Your team juju hackers is requested to review the proposed merge of lp:~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0 into lp:juju-core.

[Andrew Wilkins (axwalk)]

On Wed, Feb 19, 2014 at 9:18 PM, Roger Peppe <email address hidden>wrote:

> I just noticed this in the code. It seems somewhat odd that this
> is in the string attributes, rather than getting a field for itself.
>
> Could you perhaps elucidate on the reasons why this is
> the right approach?

My rationale was: this is only used for the bootstrap agent, so making it a
field that is then common to all agents only confusing.
It would be better if we had agent-type specific config structures.

On 10 February 2014 05:48, Ian Booth <email address hidden> wrote:
> > LGTM
> >
> > https://codereview.appspot.com/60310044/
> >
> > --
> >
> https://code.launchpad.net/~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0/+merge/204829
> > Your team juju hackers is requested to review the proposed merge of
> lp:~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0 into
> lp:juju-core.
>
> --
>
> https://code.launchpad.net/~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0/+merge/204829
> You are the owner of
> lp:~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0.
>

[Roger Peppe (rogpeppe)]

Ok, that's useful to know, even if I don't entirely
agree with the rationale (I think it's ok to have optional
fields, and marshalling json inside yaml seems kind
of wrong).

For the record, we actually have a need for this field
for all agents, so I think we might promote it to a
first-class field and call it Jobs rather than BootstrapJobs.

On 19 February 2014 14:03, Andrew Wilkins <email address hidden> wrote:
> On Wed, Feb 19, 2014 at 9:18 PM, Roger Peppe <email address hidden>wrote:
>
>> I just noticed this in the code. It seems somewhat odd that this
>> is in the string attributes, rather than getting a field for itself.
>>
>> Could you perhaps elucidate on the reasons why this is
>> the right approach?
>
>
> My rationale was: this is only used for the bootstrap agent, so making it a
> field that is then common to all agents only confusing.
> It would be better if we had agent-type specific config structures.
>
> On 10 February 2014 05:48, Ian Booth <email address hidden> wrote:
>> > LGTM
>> >
>> > https://codereview.appspot.com/60310044/
>> >
>> > --
>> >
>> https://code.launchpad.net/~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0/+merge/204829
>> > Your team juju hackers is requested to review the proposed merge of
>> lp:~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0 into
>> lp:juju-core.
>>
>> --
>>
>> https://code.launchpad.net/~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0/+merge/204829
>> You are the owner of
>> lp:~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0.
>>
>
> --
> https://code.launchpad.net/~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0/+merge/204829
> Your team juju hackers is requested to review the proposed merge of lp:~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0 into lp:juju-core.

[Andrew Wilkins (axwalk)]

On Wed, Feb 19, 2014 at 11:24 PM, Roger Peppe <email address hidden>wrote:

> Ok, that's useful to know, even if I don't entirely
> agree with the rationale (I think it's ok to have optional
> fields, and marshalling json inside yaml seems kind
> of wrong).
>
> For the record, we actually have a need for this field
> for all agents, so I think we might promote it to a
> first-class field and call it Jobs rather than BootstrapJobs.

Cool. I will see if I can squeeze it in before 1.18 so we don't have yet
another backwards compatibility issue.

> On 19 February 2014 14:03, Andrew Wilkins <email address hidden>
> wrote:
> > On Wed, Feb 19, 2014 at 9:18 PM, Roger Peppe <<email address hidden>
> >wrote:
> >
> >> I just noticed this in the code. It seems somewhat odd that this
> >> is in the string attributes, rather than getting a field for itself.
> >>
> >> Could you perhaps elucidate on the reasons why this is
> >> the right approach?
> >
> >
> > My rationale was: this is only used for the bootstrap agent, so making
> it a
> > field that is then common to all agents only confusing.
> > It would be better if we had agent-type specific config structures.
> >
> > On 10 February 2014 05:48, Ian Booth <email address hidden> wrote:
> >> > LGTM
> >> >
> >> > https://codereview.appspot.com/60310044/
> >> >
> >> > --
> >> >
> >>
> https://code.launchpad.net/~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0/+merge/204829
> >> > Your team juju hackers is requested to review the proposed merge of
> >> lp:~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0 into
> >> lp:juju-core.
> >>
> >> --
> >>
> >>
> https://code.launchpad.net/~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0/+merge/204829
> >> You are the owner of
> >> lp:~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0.
> >>
> >
> > --
> >
> https://code.launchpad.net/~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0/+merge/204829
> > Your team juju hackers is requested to review the proposed merge of
> lp:~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0 into
> lp:juju-core.
>
> --
>
> https://code.launchpad.net/~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0/+merge/204829
> You are the owner of
> lp:~axwalk/juju-core/lp1273933-local-disallow-hostunits-machine-0.
>

=== modified file 'agent/agent.go'
--- agent/agent.go	2014-02-04 19:31:25 +0000
+++ agent/agent.go	2014-02-05 08:05:31 +0000
@@ -31,6 +31,7 @@
 	AgentServiceName = "AGENT_SERVICE_NAME"
 	MongoServiceName = "MONGO_SERVICE_NAME"
 	RsyslogConfPath  = "RSYSLOG_CONF_PATH"
+	BootstrapJobs    = "BOOTSTRAP_JOBS"
 )
 
 // The Config interface is the sole way that the agent gets access to the

=== modified file 'agent/bootstrap.go'
--- agent/bootstrap.go	2014-01-23 20:30:35 +0000
+++ agent/bootstrap.go	2014-02-05 08:05:31 +0000
@@ -4,6 +4,7 @@
 package agent
 
 import (
+	"encoding/json"
 	"fmt"
 
 	"launchpad.net/juju-core/constraints"
@@ -31,6 +32,21 @@
 	InitializeState(envCfg *config.Config, machineCfg BootstrapMachineConfig, timeout state.DialOpts) (*state.State, *state.Machine, error)
 }
 
+// MarshalBootstrapJobs may be used to marshal a set of
+// machine jobs for the bootstrap agent, to be added
+// into the agent configuration.
+func MarshalBootstrapJobs(jobs ...state.MachineJob) (string, error) {
+	b, err := json.Marshal(jobs)
+	return string(b), err
+}
+
+// UnmarshalBootstrapJobs unmarshals a set of machine
+// jobs marshalled with MarshalBootstrapJobs.
+func UnmarshalBootstrapJobs(s string) (jobs []state.MachineJob, err error) {
+	err = json.Unmarshal([]byte(s), &jobs)
+	return jobs, err
+}
+
 // BootstrapMachineConfig holds configuration information
 // to attach to the bootstrap machine.
 type BootstrapMachineConfig struct {

=== modified file 'agent/bootstrap_test.go'
--- agent/bootstrap_test.go	2013-12-13 19:15:26 +0000
+++ agent/bootstrap_test.go	2014-02-05 08:05:31 +0000
@@ -162,3 +162,19 @@
 	_, err = st.Machine("0")
 	c.Assert(err, gc.IsNil)
 }
+
+func (s *bootstrapSuite) TestMarshalUnmarshalBootstrapJobs(c *gc.C) {
+	jobs := [][]state.MachineJob{
+		{},
+		{state.JobHostUnits},
+		{state.JobManageEnviron},
+		{state.JobHostUnits, state.JobManageEnviron},
+	}
+	for _, jobs := range jobs {
+		marshalled, err := agent.MarshalBootstrapJobs(jobs...)
+		c.Assert(err, gc.IsNil)
+		unmarshalled, err := agent.UnmarshalBootstrapJobs(marshalled)
+		c.Assert(err, gc.IsNil)
+		c.Assert(unmarshalled, gc.DeepEquals, jobs)
+	}
+}

=== modified file 'cmd/jujud/bootstrap.go'
--- cmd/jujud/bootstrap.go	2014-01-22 19:28:08 +0000
+++ cmd/jujud/bootstrap.go	2014-02-05 08:05:31 +0000
@@ -74,23 +74,19 @@
 	if err != nil {
 		return err
 	}
-	// TODO(fwereade): we need to be able to customize machine jobs,
-	// not just hardcode these values; in particular, JobHostUnits
-	// on a machine, like this one, that is running JobManageEnviron
-	// (not to mention the actual state server itself...) will allow
-	// a malicious or compromised unit to trivially access to the
-	// user's environment credentials. However, given that this point
-	// is currently moot (see Upgrader in this package), the pseudo-
-	// local provider mode (in which everything is deployed with
-	// `--to 0`) offers enough value to enough people that
-	// JobHostUnits is currently always enabled. This will one day
-	// have to change, but it's strictly less important than fixing
-	// Upgrader, and it's a capability we'll always want to have
-	// available for the aforementioned use case.
+	// agent.BootstrapJobs is an optional field in the agent
+	// config, and was introduced after 1.17.2. We default to
+	// allowing units on machine-0 if missing.
 	jobs := []state.MachineJob{
 		state.JobManageEnviron,
 		state.JobHostUnits,
 	}
+	if bootstrapJobs := c.Conf.config.Value(agent.BootstrapJobs); bootstrapJobs != "" {
+		jobs, err = agent.UnmarshalBootstrapJobs(bootstrapJobs)
+		if err != nil {
+			return err
+		}
+	}
 	var characteristics instance.HardwareCharacteristics
 	if len(bsState.Characteristics) > 0 {
 		characteristics = bsState.Characteristics[0]

=== modified file 'cmd/jujud/bootstrap_test.go'
--- cmd/jujud/bootstrap_test.go	2014-01-22 19:28:08 +0000
+++ cmd/jujud/bootstrap_test.go	2014-02-05 08:05:31 +0000
@@ -164,7 +164,7 @@
 	return &v
 }
 
-func (s *BootstrapSuite) TestMachinerWorkers(c *gc.C) {
+func (s *BootstrapSuite) TestDefaultMachineJobs(c *gc.C) {
 	_, cmd, err := s.initBootstrapCommand(c, "--env-config", testConfig)
 	c.Assert(err, gc.IsNil)
 	err = cmd.Run(nil)
@@ -184,6 +184,29 @@
 	})
 }
 
+func (s *BootstrapSuite) TestConfiguredMachineJobs(c *gc.C) {
+	agentConf, cmd, err := s.initBootstrapCommand(c, "--env-config", testConfig)
+	c.Assert(err, gc.IsNil)
+	bootstrapJobs, err := agent.MarshalBootstrapJobs(state.JobManageEnviron)
+	c.Assert(err, gc.IsNil)
+	agentConf.SetValue(agent.BootstrapJobs, bootstrapJobs)
+	err = agentConf.Write()
+	c.Assert(err, gc.IsNil)
+	err = cmd.Run(nil)
+	c.Assert(err, gc.IsNil)
+
+	st, err := state.Open(&state.Info{
+		Addrs:    []string{testing.MgoServer.Addr()},
+		CACert:   []byte(testing.CACert),
+		Password: testPasswordHash(),
+	}, state.DefaultDialOpts())
+	c.Assert(err, gc.IsNil)
+	defer st.Close()
+	m, err := st.Machine("0")
+	c.Assert(err, gc.IsNil)
+	c.Assert(m.Jobs(), gc.DeepEquals, []state.MachineJob{state.JobManageEnviron})
+}
+
 func testOpenState(c *gc.C, info *state.Info, expectErrType error) {
 	st, err := state.Open(info, state.DefaultDialOpts())
 	if st != nil {

=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2014-02-04 19:12:08 +0000
+++ provider/local/environ.go	2014-02-05 08:05:31 +0000
@@ -127,6 +127,11 @@
 		return err
 	}
 
+	bootstrapJobs, err := agent.MarshalBootstrapJobs(state.JobManageEnviron)
+	if err != nil {
+		return err
+	}
+
 	mcfg := environs.NewBootstrapMachineConfig(stateFileURL, privateKey)
 	mcfg.Tools = selectedTools[0]
 	mcfg.DataDir = env.config.rootDir()
@@ -137,9 +142,10 @@
 	mcfg.MachineAgentServiceName = env.machineAgentServiceName()
 	mcfg.MongoServiceName = env.mongoServiceName()
 	mcfg.AgentEnvironment = map[string]string{
-		agent.Namespace:   env.config.namespace(),
-		agent.StorageDir:  env.config.storageDir(),
-		agent.StorageAddr: env.config.storageAddr(),
+		agent.Namespace:     env.config.namespace(),
+		agent.StorageDir:    env.config.storageDir(),
+		agent.StorageAddr:   env.config.storageAddr(),
+		agent.BootstrapJobs: bootstrapJobs,
 	}
 	if err := environs.FinishMachineConfig(mcfg, env.Config(), cons); err != nil {
 		return err

=== modified file 'provider/local/environ_test.go'
--- provider/local/environ_test.go	2014-01-23 22:10:46 +0000
+++ provider/local/environ_test.go	2014-02-05 08:05:31 +0000
@@ -11,6 +11,7 @@
 
 	gc "launchpad.net/gocheck"
 
+	"launchpad.net/juju-core/agent"
 	coreCloudinit "launchpad.net/juju-core/cloudinit"
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
@@ -21,6 +22,7 @@
 	"launchpad.net/juju-core/environs/tools"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/provider/local"
+	"launchpad.net/juju-core/state"
 	coretesting "launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
 )
@@ -146,6 +148,11 @@
 		c.Assert(cloudcfg.AptUpdate(), jc.IsFalse)
 		c.Assert(cloudcfg.AptUpgrade(), jc.IsFalse)
 		c.Assert(cloudcfg.Packages(), gc.HasLen, 0)
+		c.Assert(mcfg.AgentEnvironment, gc.Not(gc.IsNil))
+		// local does not allow machine-0 to host units
+		bootstrapJobs, err := agent.UnmarshalBootstrapJobs(mcfg.AgentEnvironment[agent.BootstrapJobs])
+		c.Assert(err, gc.IsNil)
+		c.Assert(bootstrapJobs, gc.DeepEquals, []state.MachineJob{state.JobManageEnviron})
 		return nil
 	})
 	testConfig := minimalConfig(c)