Merge pull request #5887 from wallyworld/model-default-config

When adding a model, fork and store default config attributes

environs.Config is refactored so that default values are available for all relevant configuration attributes, not just select values. This means that when creating a model, all config attributes, even the default ones are forked and stored. This means that:
* juju model-config correctly shows where config originates from 
* models migrate and retain all their config and are not subject to new Juju versions setting different defaults
* set-model-config does not print a bogus error


(Review request: http://reviews.vapour.ws/r/5321/)

agent/agentbootstrap/bootstrap_test.go

@@ -222,7 +222,9 @@ LXC_BRIDGE="ignored"`[1:])
 	newModelCfg, err := st.ModelConfig()
 	c.Assert(err, jc.ErrorIsNil)
 	// Add in the cloud attributes.
-	expectedAttrs := modelCfg.AllAttrs()
+	expectedCfg, err := config.New(config.UseDefaults, modelAttrs)
+	c.Assert(err, jc.ErrorIsNil)
+	expectedAttrs := expectedCfg.AllAttrs()
 	expectedAttrs["apt-mirror"] = "http://mirror"
 	c.Assert(newModelCfg.AllAttrs(), jc.DeepEquals, expectedAttrs)
 

apiserver/common/modelmanagerinterface.go

@@ -32,6 +32,7 @@ type ModelManagerBackend interface {
 	IsControllerAdministrator(user names.UserTag) (bool, error)
 	NewModel(state.ModelArgs) (Model, ModelManagerBackend, error)
 
+	ComposeNewModelConfig(modelAttr map[string]interface{}) (map[string]interface{}, error)
 	ControllerModel() (Model, error)
 	ControllerConfig() (controller.Config, error)
 	ForModel(tag names.ModelTag) (ModelManagerBackend, error)

apiserver/common/modelwatcher_test.go

@@ -126,7 +126,7 @@ func testingEnvConfig(c *gc.C) *config.Config {
 		bootstrap.PrepareParams{
 			ControllerConfig: testing.FakeControllerConfig(),
 			ControllerName:   "dummycontroller",
-			BaseConfig:       dummy.SampleConfig(),
+			ModelConfig:      dummy.SampleConfig(),
 			Cloud:            dummy.SampleCloudSpec(),
 			AdminSecret:      "admin-secret",
 		},

apiserver/modelmanager/modelinfo_test.go

@@ -273,6 +273,16 @@ func (st *mockState) ControllerModel() (common.Model, error) {
 	return st.controllerModel, st.NextErr()
 }
 
+func (st *mockState) ComposeNewModelConfig(modelAttr map[string]interface{}) (map[string]interface{}, error) {
+	st.MethodCall(st, "ComposeNewModelConfig")
+	attr := make(map[string]interface{})
+	for attrName, val := range modelAttr {
+		attr[attrName] = val
+	}
+	attr["something"] = "value"
+	return attr, st.NextErr()
+}
+
 func (st *mockState) ControllerConfig() (controller.Config, error) {
 	st.MethodCall(st, "ControllerConfig")
 	return controller.Config{

apiserver/modelmanager/modelmanager.go

@@ -150,6 +150,9 @@ func (mm *ModelManagerAPI) newModelConfig(
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
+	if joint, err = mm.state.ComposeNewModelConfig(joint); err != nil {
+		return nil, errors.Trace(err)
+	}
 	creator := modelmanager.ModelConfigCreator{
 		FindTools: func(n version.Number) (tools.List, error) {
 			result, err := mm.toolsFinder.FindTools(params.FindToolsParams{

apiserver/modelmanager/modelmanager_test.go

@@ -28,7 +28,6 @@ import (
 	jujuversion "github.com/juju/juju/version"
 	// Register the providers for the field check test
 	"github.com/juju/juju/apiserver/common"
-	"github.com/juju/juju/controller"
 	_ "github.com/juju/juju/provider/azure"
 	"github.com/juju/juju/provider/dummy"
 	_ "github.com/juju/juju/provider/ec2"
@@ -129,6 +128,7 @@ func (s *modelManagerSuite) TestCreateModelArgs(c *gc.C) {
 		"ControllerModel",
 		"CloudCredentials",
 		"ControllerConfig",
+		"ComposeNewModelConfig",
 		"NewModel",
 		"ForModel",
 		"Model",
@@ -140,7 +140,7 @@ func (s *modelManagerSuite) TestCreateModelArgs(c *gc.C) {
 	// We cannot predict the UUID, because it's generated,
 	// so we just extract it and ensure that it's not the
 	// same as the controller UUID.
-	newModelArgs := s.st.Calls()[5].Args[0].(state.ModelArgs)
+	newModelArgs := s.st.Calls()[6].Args[0].(state.ModelArgs)
 	uuid := newModelArgs.Config.UUID()
 	c.Assert(uuid, gc.Not(gc.Equals), s.st.controllerModel.cfg.UUID())
 
@@ -153,13 +153,9 @@ func (s *modelManagerSuite) TestCreateModelArgs(c *gc.C) {
 		"controller":    false,
 		"broken":        "",
 		"secret":        "pork",
+		"something":     "value",
 	})
 	c.Assert(err, jc.ErrorIsNil)
-	// TODO(wallyworld) - we need to separate controller and model schemas
-	// Remove any remaining controller attributes from the env config.
-	cfg, err = cfg.Remove(controller.ControllerOnlyConfigAttributes)
-	c.Assert(err, jc.ErrorIsNil)
-
 	c.Assert(newModelArgs, jc.DeepEquals, state.ModelArgs{
 		Owner:           names.NewUserTag("admin@local"),
 		CloudName:       "some-cloud",
@@ -177,7 +173,7 @@ func (s *modelManagerSuite) TestCreateModelDefaultRegion(c *gc.C) {
 	_, err := s.api.CreateModel(args)
 	c.Assert(err, jc.ErrorIsNil)
 
-	newModelArgs := s.st.Calls()[5].Args[0].(state.ModelArgs)
+	newModelArgs := s.st.Calls()[6].Args[0].(state.ModelArgs)
 	c.Assert(newModelArgs.CloudRegion, gc.Equals, "some-region")
 }
 
@@ -198,7 +194,7 @@ func (s *modelManagerSuite) testCreateModelDefaultCredentialAdmin(c *gc.C, owner
 	_, err := s.api.CreateModel(args)
 	c.Assert(err, jc.ErrorIsNil)
 
-	newModelArgs := s.st.Calls()[5].Args[0].(state.ModelArgs)
+	newModelArgs := s.st.Calls()[6].Args[0].(state.ModelArgs)
 	c.Assert(newModelArgs.CloudCredential, gc.Equals, "some-credential")
 }
 
@@ -210,7 +206,7 @@ func (s *modelManagerSuite) TestCreateModelEmptyCredentialNonAdmin(c *gc.C) {
 	_, err := s.api.CreateModel(args)
 	c.Assert(err, jc.ErrorIsNil)
 
-	newModelArgs := s.st.Calls()[5].Args[0].(state.ModelArgs)
+	newModelArgs := s.st.Calls()[6].Args[0].(state.ModelArgs)
 	c.Assert(newModelArgs.CloudCredential, gc.Equals, "")
 }
 

cmd/juju/commands/bootstrap.go

@@ -462,9 +462,6 @@ func (c *bootstrapCommand) Run(ctx *cmd.Context) (resultErr error) {
 		"name":         bootstrap.ControllerModelName,
 		config.UUIDKey: controllerUUID.String(),
 	}
-	for k, v := range cloud.Config {
-		modelConfigAttrs[k] = v
-	}
 	userConfigAttrs, err := c.config.ReadAttrs(ctx)
 	if err != nil {
 		return errors.Trace(err)
@@ -474,6 +471,20 @@ func (c *bootstrapCommand) Run(ctx *cmd.Context) (resultErr error) {
 	}
 	bootstrapConfigAttrs := make(map[string]interface{})
 	controllerConfigAttrs := make(map[string]interface{})
+	// Based on the attribute names in clouds.yaml, create
+	// a map of shared config for all models on this cloud.
+	inheritedControllerAttrs := make(map[string]interface{})
+	for k, v := range cloud.Config {
+		switch {
+		case bootstrap.IsBootstrapAttribute(k):
+			bootstrapConfigAttrs[k] = v
+			continue
+		case controller.ControllerOnlyAttribute(k):
+			controllerConfigAttrs[k] = v
+			continue
+		}
+		inheritedControllerAttrs[k] = v
+	}
 	for k, v := range modelConfigAttrs {
 		switch {
 		case bootstrap.IsBootstrapAttribute(k):
@@ -528,10 +539,26 @@ func (c *bootstrapCommand) Run(ctx *cmd.Context) (resultErr error) {
 		}
 	}()
 
+	bootstrapModelConfig := make(map[string]interface{})
+	for k, v := range inheritedControllerAttrs {
+		bootstrapModelConfig[k] = v
+	}
+	for k, v := range modelConfigAttrs {
+		bootstrapModelConfig[k] = v
+	}
+	// Add in any default attribute values if not already
+	// specified, making the recorded bootstrap config
+	// immutable to changes in Juju.
+	for k, v := range config.ConfigDefaults() {
+		if _, ok := bootstrapModelConfig[k]; !ok {
+			bootstrapModelConfig[k] = v
+		}
+	}
+
 	environ, err := bootstrapPrepare(
 		modelcmd.BootstrapContext(ctx), store,
 		bootstrap.PrepareParams{
-			BaseConfig:       modelConfigAttrs,
+			ModelConfig:      bootstrapModelConfig,
 			ControllerConfig: controllerConfig,
 			ControllerName:   c.controllerName,
 			Cloud: environs.CloudSpec{
@@ -629,6 +656,9 @@ to clean up the model.`[1:])
 		"name":         c.hostedModelName,
 		config.UUIDKey: hostedModelUUID.String(),
 	}
+	for k, v := range inheritedControllerAttrs {
+		hostedModelConfig[k] = v
+	}
 
 	// We copy across any user supplied attributes to the hosted model config.
 	// But only if the attributes have not been removed from the controller
@@ -646,15 +676,6 @@ to clean up the model.`[1:])
 	delete(hostedModelConfig, config.AuthorizedKeysKey)
 	delete(hostedModelConfig, config.AgentVersionKey)
 
-	// Based on the attribute names in clouds.yaml, create
-	// a map of shared config for all models on this cloud.
-	inheritedControllerAttrs := make(map[string]interface{})
-	for k := range cloud.Config {
-		if v, ok := controllerModelConfigAttrs[k]; ok {
-			inheritedControllerAttrs[k] = v
-		}
-	}
-
 	// Check whether the Juju GUI must be installed in the controller.
 	// Leaving this value empty means no GUI will be installed.
 	var guiDataSourceBaseURL string

cmd/juju/commands/bootstrap_test.go

@@ -30,6 +30,7 @@ import (
 	"github.com/juju/juju/constraints"
 	"github.com/juju/juju/environs"
 	"github.com/juju/juju/environs/bootstrap"
+	"github.com/juju/juju/environs/config"
 	"github.com/juju/juju/environs/filestorage"
 	"github.com/juju/juju/environs/gui"
 	"github.com/juju/juju/environs/imagemetadata"
@@ -242,12 +243,18 @@ func (s *BootstrapSuite) run(c *gc.C, test bootstrapTest) testing.Restorer {
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(bootstrapConfig.Cloud, gc.Equals, "dummy")
 	c.Assert(bootstrapConfig.Credential, gc.Equals, "")
-	c.Assert(bootstrapConfig.Config, jc.DeepEquals, map[string]interface{}{
+	expected := map[string]interface{}{
 		"name":            bootstrap.ControllerModelName,
 		"type":            "dummy",
 		"default-series":  "raring",
 		"authorized-keys": "public auth key\n",
-	})
+	}
+	for k, v := range config.ConfigDefaults() {
+		if _, ok := expected[k]; !ok {
+			expected[k] = v
+		}
+	}
+	c.Assert(bootstrapConfig.Config, jc.DeepEquals, expected)
 
 	return restore
 }
@@ -737,7 +744,7 @@ func (s *BootstrapSuite) TestAutoSyncLocalSource(c *gc.C) {
 	// are automatically synchronized.
 	_, err := coretesting.RunCommand(
 		c, s.newBootstrapCommand(), "--metadata-source", sourceDir,
-		"devcontroller", "dummy-cloud/region-1",
+		"devcontroller", "dummy-cloud/region-1", "--config", "default-series=trusty",
 	)
 	c.Assert(err, jc.ErrorIsNil)
 

cmd/juju/model/get.go

@@ -88,6 +88,14 @@ func (c *getCommand) getAPI() (GetModelAPI, error) {
 	return modelconfig.NewClient(api), nil
 }
 
+func (c *getCommand) isModelAttrbute(attr string) bool {
+	switch attr {
+	case config.NameKey, config.TypeKey, config.UUIDKey:
+		return true
+	}
+	return false
+}
+
 func (c *getCommand) Run(ctx *cmd.Context) error {
 	client, err := c.getAPI()
 	if err != nil {
@@ -100,6 +108,14 @@ func (c *getCommand) Run(ctx *cmd.Context) error {
 		return err
 	}
 
+	for attrName := range attrs {
+		// We don't want model attributes included, these are available
+		// via show-model.
+		if c.isModelAttrbute(attrName) {
+			delete(attrs, attrName)
+		}
+	}
+
 	if c.key != "" {
 		if value, found := attrs[c.key]; found {
 			out, err := cmd.FormatSmart(value.Value)

cmd/juju/model/get_test.go

@@ -37,19 +37,19 @@ func (s *GetSuite) TestInit(c *gc.C) {
 }
 
 func (s *GetSuite) TestSingleValue(c *gc.C) {
-	context, err := s.run(c, "name")
+	context, err := s.run(c, "special")
 	c.Assert(err, jc.ErrorIsNil)
 
 	output := strings.TrimSpace(testing.Stdout(context))
-	c.Assert(output, gc.Equals, "test-model")
+	c.Assert(output, gc.Equals, "special value")
 }
 
 func (s *GetSuite) TestSingleValueJSON(c *gc.C) {
-	context, err := s.run(c, "--format=json", "name")
+	context, err := s.run(c, "--format=json", "special")
 	c.Assert(err, jc.ErrorIsNil)
 
 	output := strings.TrimSpace(testing.Stdout(context))
-	c.Assert(output, gc.Equals, "test-model")
+	c.Assert(output, gc.Equals, "special value")
 }
 
 func (s *GetSuite) TestAllValuesYAML(c *gc.C) {
@@ -58,9 +58,6 @@ func (s *GetSuite) TestAllValuesYAML(c *gc.C) {
 
 	output := strings.TrimSpace(testing.Stdout(context))
 	expected := "" +
-		"name:\n" +
-		"  value: test-model\n" +
-		"  source: model\n" +
 		"running:\n" +
 		"  value: true\n" +
 		"  source: model\n" +
@@ -75,7 +72,7 @@ func (s *GetSuite) TestAllValuesJSON(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 
 	output := strings.TrimSpace(testing.Stdout(context))
-	expected := `{"name":{"Value":"test-model","Source":"model"},"running":{"Value":true,"Source":"model"},"special":{"Value":"special value","Source":"model"}}`
+	expected := `{"running":{"Value":true,"Source":"model"},"special":{"Value":"special value","Source":"model"}}`
 	c.Assert(output, gc.Equals, expected)
 }
 
@@ -86,7 +83,6 @@ func (s *GetSuite) TestAllValuesTabular(c *gc.C) {
 	output := strings.TrimSpace(testing.Stdout(context))
 	expected := "" +
 		"ATTRIBUTE  FROM   VALUE\n" +
-		"name       model  test-model\n" +
 		"running    model  True\n" +
 		"special    model  special value"
 	c.Assert(output, gc.Equals, expected)

cmd/modelcmd/base.go

@@ -406,7 +406,7 @@ func (g bootstrapConfigGetter) getBootstrapConfigParams(controllerName string) (
 	}
 	bootstrapConfig.Config[config.UUIDKey] = controllerDetails.ControllerUUID
 
-	cfg, err := config.New(config.UseDefaults, bootstrapConfig.Config)
+	cfg, err := config.New(config.NoDefaults, bootstrapConfig.Config)
 	if err != nil {
 		return nil, nil, errors.Trace(err)
 	}

cmd/plugins/juju-metadata/toolsmetadata_test.go

@@ -57,7 +57,7 @@ func (s *ToolsMetadataSuite) SetUpTest(c *gc.C) {
 		bootstrap.PrepareParams{
 			ControllerConfig: coretesting.FakeControllerConfig(),
 			ControllerName:   cfg.Name(),
-			BaseConfig:       cfg.AllAttrs(),
+			ModelConfig:      cfg.AllAttrs(),
 			Cloud:            dummy.SampleCloudSpec(),
 			AdminSecret:      "admin-secret",
 		},

controller/modelmanager/createmodel.go

@@ -11,7 +11,6 @@ import (
 	"github.com/juju/utils"
 	"github.com/juju/version"
 
-	"github.com/juju/juju/controller"
 	"github.com/juju/juju/environs"
 	"github.com/juju/juju/environs/config"
 	"github.com/juju/juju/tools"
@@ -94,17 +93,11 @@ func (c ModelConfigCreator) NewModelConfig(
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
-	attrs = cfg.AllAttrs()
 
-	// TODO(wallyworld) - we need to separate controller and model schemas
-	for _, attr := range controller.ControllerOnlyConfigAttributes {
-		if _, ok := attrs[attr]; ok {
-			return nil, errors.Errorf("unexpected controller attribute %q in model config", attr)
-		}
-	}
 	// Any values that would normally be copied from the controller
 	// config can also be defined, but if they differ from the controller
 	// values, an error is returned.
+	attrs = cfg.AllAttrs()
 	for _, field := range restrictedFields {
 		if value, ok := attrs[field]; ok {
 			if serverValue := baseAttrs[field]; value != serverValue {
@@ -205,13 +198,6 @@ func finalizeConfig(isAdmin bool, controllerUUID string, controllerModelCfg *con
 		return nil, errors.Annotate(err, "creating config from values failed")
 	}
 
-	// TODO(wallyworld) - we need to separate controller and model schemas
-	// Remove any remaining controller attributes from the env config.
-	cfg, err = cfg.Remove(controller.ControllerOnlyConfigAttributes)
-	if err != nil {
-		return nil, errors.Annotate(err, "cannot remove controller attributes")
-	}
-
 	cfg, err = provider.PrepareForCreateEnvironment(controllerUUID, cfg)
 	if err != nil {
 		return nil, errors.Trace(err)

environs/bootstrap/bootstrap.go

@@ -93,7 +93,7 @@ type BootstrapParams struct {
 	ControllerConfig controller.Config
 
 	// ControllerInheritedConfig is the set of config attributes to be shared
-	// across all models in the same controller on the bootstrap cloud.
+	// across all models in the same controller.
 	ControllerInheritedConfig map[string]interface{}
 
 	// HostedModelConfig is the set of config attributes to be overlaid