Merge pull request juju#16659 from tlm/model-config-api

juju#16659

With this change we are introducing the ability to bootstrap cloud model defaults at agent bootstrap. This is needed as the Juju client sends a set of cloud defaults to agent bootstrap for persistence as part of the running controller.

We only save these defaults for the controller cloud and they will only ever be used for models that use the same cloud.

This PR does not:
- Do model cloud region defaults yet.

## Checklist

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing
- ~[ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~
- ~[ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~

## QA steps

This code is currently not being used in a read only context so we only need to make sure that a successful bootstrap can take place at the moment. It is the same code path for lxd and k8's so bootstrap to either cloud and confirm through the debug logs that no errors occur.

## Documentation changes

N/A

## Links

**Jira card:** JUJU-5112

agent/agentbootstrap/bootstrap.go

@@ -243,6 +243,7 @@ func (b *AgentBootstrap) Initialize(ctx stdcontext.Context) (_ *state.Controller
 			ccbootstrap.InsertInitialControllerConfig(stateParams.ControllerConfig),
 			cloudbootstrap.InsertCloud(stateParams.ControllerCloud),
 			credbootstrap.InsertCredential(credential.IdFromTag(cloudCredTag), cloudCred),
+			cloudbootstrap.SetCloudDefaults(stateParams.ControllerCloud.Name, stateParams.ControllerInheritedConfig),
 			modelbootstrap.CreateModel(controllerUUID, controllerModelArgs),
 		),
 		database.BootstrapModelConcern(controllerUUID,

domain/cloud/bootstrap/bootstrap.go

@@ -6,13 +6,15 @@ package bootstrap
 import (
 	"context"
 	"database/sql"
+	"fmt"
 
 	"github.com/juju/errors"
 	"github.com/juju/utils/v3"
 
 	"github.com/juju/juju/cloud"
 	"github.com/juju/juju/core/database"
 	"github.com/juju/juju/domain/cloud/state"
+	modelconfigservice "github.com/juju/juju/domain/modelconfig/service"
 )
 
 // InsertCloud inserts the initial cloud during bootstrap.
@@ -30,3 +32,29 @@ func InsertCloud(cloud cloud.Cloud) func(context.Context, database.TxnRunner) er
 		}))
 	}
 }
+
+// SetCloudDefaults is responsible for setting a previously inserted cloud's
+// default config values that will be used as part of the default values
+// supplied to a models config. If no cloud exists for the specified name an
+// error satisfying [github.com/juju/juju/domain/cloud/errors.NotFound] will be
+// returned.
+func SetCloudDefaults(
+	cloudName string,
+	defaults map[string]any,
+) func(context.Context, database.TxnRunner) error {
+	return func(ctx context.Context, db database.TxnRunner) error {
+		strDefaults, err := modelconfigservice.CoerceConfigForStorage(defaults)
+		if err != nil {
+			return fmt.Errorf("coercing cloud %q default values for storage: %w", cloudName, err)
+		}
+
+		err = db.StdTxn(ctx, func(ctx context.Context, tx *sql.Tx) error {
+			return state.SetCloudDefaults(ctx, tx, cloudName, strDefaults)
+		})
+
+		if err != nil {
+			return fmt.Errorf("setting cloud %q bootstrap defaults: %w", cloudName, err)
+		}
+		return nil
+	}
+}

domain/cloud/bootstrap/bootstrap_test.go

@@ -10,6 +10,8 @@ import (
 	gc "gopkg.in/check.v1"
 
 	"github.com/juju/juju/cloud"
+	clouderrors "github.com/juju/juju/domain/cloud/errors"
+	"github.com/juju/juju/domain/cloud/state"
 	schematesting "github.com/juju/juju/domain/schema/testing"
 )
 
@@ -29,3 +31,87 @@ func (s *bootstrapSuite) TestInsertCloud(c *gc.C) {
 	c.Assert(row.Scan(&name), jc.ErrorIsNil)
 	c.Assert(name, gc.Equals, "cirrus")
 }
+
+// TestSetCloudDefaultsNoExist is check that if we try and set cloud defaults
+// for a cloud that doesn't exist we get a [clouderrors.NotFound] error back
+func (s *bootstrapSuite) TestSetCloudDefaultsNoExist(c *gc.C) {
+	set := SetCloudDefaults("noexist", map[string]any{
+		"HTTP_PROXY": "[2001:0DB8::1]:80",
+	})
+
+	err := set(context.Background(), s.TxnRunner())
+	c.Check(err, jc.ErrorIs, clouderrors.NotFound)
+
+	var count int
+	row := s.DB().QueryRow("SELECT count(*) FROM cloud_defaults")
+	err = row.Scan(&count)
+	c.Check(err, jc.ErrorIsNil)
+	c.Check(count, gc.Equals, 0)
+}
+
+// TestSetCloudDefaults is testing the happy path for setting cloud defaults.
+func (s *bootstrapSuite) TestSetCloudDefaults(c *gc.C) {
+	cld := cloud.Cloud{
+		Name:      "cirrus",
+		Type:      "ec2",
+		AuthTypes: cloud.AuthTypes{cloud.UserPassAuthType},
+	}
+	err := InsertCloud(cld)(context.Background(), s.TxnRunner())
+	c.Check(err, jc.ErrorIsNil)
+
+	set := SetCloudDefaults("cirrus", map[string]any{
+		"HTTP_PROXY": "[2001:0DB8::1]:80",
+	})
+
+	err = set(context.Background(), s.TxnRunner())
+	c.Check(err, jc.ErrorIsNil)
+
+	st := state.NewState(s.TxnRunnerFactory())
+	defaults, err := st.CloudDefaults(context.Background(), "cirrus")
+	c.Check(err, jc.ErrorIsNil)
+	c.Check(defaults, jc.DeepEquals, map[string]string{
+		"HTTP_PROXY": "[2001:0DB8::1]:80",
+	})
+}
+
+// TestSetCloudDefaultsOverrides is testing that repeated calls to
+// [SetCloudDefaults] overrides existing cloud defaults that have been set.
+func (s *bootstrapSuite) TestSetCloudDefaultsOverides(c *gc.C) {
+	cld := cloud.Cloud{
+		Name:      "cirrus",
+		Type:      "ec2",
+		AuthTypes: cloud.AuthTypes{cloud.UserPassAuthType},
+	}
+	err := InsertCloud(cld)(context.Background(), s.TxnRunner())
+	c.Check(err, jc.ErrorIsNil)
+
+	set := SetCloudDefaults("cirrus", map[string]any{
+		"HTTP_PROXY": "[2001:0DB8::1]:80",
+	})
+
+	err = set(context.Background(), s.TxnRunner())
+	c.Check(err, jc.ErrorIsNil)
+
+	st := state.NewState(s.TxnRunnerFactory())
+	defaults, err := st.CloudDefaults(context.Background(), "cirrus")
+	c.Check(err, jc.ErrorIsNil)
+	c.Check(defaults, jc.DeepEquals, map[string]string{
+		"HTTP_PROXY": "[2001:0DB8::1]:80",
+	})
+
+	// Second time around
+
+	set = SetCloudDefaults("cirrus", map[string]any{
+		"foo": "bar",
+	})
+
+	err = set(context.Background(), s.TxnRunner())
+	c.Check(err, jc.ErrorIsNil)
+
+	st = state.NewState(s.TxnRunnerFactory())
+	defaults, err = st.CloudDefaults(context.Background(), "cirrus")
+	c.Check(err, jc.ErrorIsNil)
+	c.Check(defaults, jc.DeepEquals, map[string]string{
+		"foo": "bar",
+	})
+}

domain/cloud/errors/errors.go

@@ -0,0 +1,14 @@
+// Copyright 2023 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package errors
+
+import (
+	"github.com/juju/errors"
+)
+
+const (
+	// NotFound describes an error that occurs when the cloud being operated on
+	// does not exist.
+	NotFound = errors.ConstError("cloud not found")
+)

domain/cloud/state/state.go

@@ -17,6 +17,7 @@ import (
 	coredatabase "github.com/juju/juju/core/database"
 	"github.com/juju/juju/core/watcher"
 	"github.com/juju/juju/domain"
+	clouderrors "github.com/juju/juju/domain/cloud/errors"
 	"github.com/juju/juju/domain/model"
 	"github.com/juju/juju/internal/database"
 )
@@ -53,36 +54,71 @@ func (st *State) ListClouds(ctx context.Context, name string) ([]cloud.Cloud, er
 // cloud has no defaults or the cloud does not exist a nil error is returned
 // with an empty defaults map.
 func (st *State) CloudDefaults(ctx context.Context, cloudName string) (map[string]string, error) {
-	defaults := map[string]string{}
-
 	db, err := st.DB()
 	if err != nil {
-		return defaults, errors.Trace(err)
-	}
-
+		return nil, fmt.Errorf("getting database for setting cloud %q defaults: %w", cloudName, err)
+	}
+
+	// This might look like an odd way to query for cloud defaults but by doing
+	// a left join onto the cloud table we are always guaranteed at least one
+	// row to be returned. This lets us confirm that a cloud actually exists
+	// for the name.
+	// The reason for going to so much effort for seeing if the cloud exists is
+	// so we can return an error if a cloud has been asked for that doesn't
+	// exist. This is important as it will let us potentially identify bad logic
+	// problems in Juju early where we have logic that might go off the rails
+	// with bad values that make their way down to state.
 	stmt := `
-SELECT  key, value
-FROM    cloud_defaults
-        INNER JOIN cloud
-            ON cloud_defaults.cloud_uuid = cloud.uuid
-WHERE   cloud.name = ?
+SELECT cloud_defaults.key,
+       cloud_defaults.value,
+       cloud.uuid
+FROM cloud
+LEFT JOIN cloud_defaults ON cloud.uuid = cloud_defaults.cloud_uuid
+WHERE cloud.name = ?
 `
 
-	return defaults, db.StdTxn(ctx, func(ctx context.Context, tx *sql.Tx) error {
+	rval := make(map[string]string)
+	err = db.StdTxn(ctx, func(ctx context.Context, tx *sql.Tx) error {
 		rows, err := tx.QueryContext(ctx, stmt, cloudName)
-		if err != nil {
-			return fmt.Errorf("fetching cloud %q defaults: %w", cloudName, err)
+		if errors.Is(err, sql.ErrNoRows) {
+			return fmt.Errorf("%w %q", clouderrors.NotFound, cloudName)
+		} else if err != nil {
+			return fmt.Errorf("getting cloud %q defaults: %w", cloudName, err)
 		}
+		defer func() { _ = rows.Close() }()
 
-		var key, value string
+		var (
+			cloudUUID  string
+			key, value sql.NullString
+		)
 		for rows.Next() {
-			if err := rows.Scan(&key, &value); err != nil {
-				return fmt.Errorf("compiling cloud %q defaults: %w", cloudName, stderrors.Join(err, rows.Close()))
+			if err := rows.Scan(&key, &value, &cloudUUID); err != nil {
+				return fmt.Errorf("reading cloud %q default: %w", cloudName, err)
 			}
-			defaults[key] = value
+			if !key.Valid {
+				// If the key is null it means there is no defaults set for the
+				// cloud. We can safely just continue because the next iteration
+				// of rows will return done.
+				continue
+			}
+			rval[key.String] = value.String
+		}
+
+		if err := rows.Err(); err != nil {
+			return fmt.Errorf("reading cloud %q defaults: %w", cloudName, err)
 		}
+		// If cloudUUID is the zero value it means no cloud exists for cloudName.
+		if cloudUUID == "" {
+			return fmt.Errorf("%w %q", clouderrors.NotFound, cloudName)
+		}
+
 		return nil
 	})
+
+	if err != nil {
+		return nil, err
+	}
+	return rval, nil
 }
 
 // UpdateCloudDefaults is responsible for updating default config values for a
@@ -816,3 +852,53 @@ func (st *State) WatchCloud(
 	result, err := getWatcher("cloud", uuid, changestream.All)
 	return result, errors.Annotatef(err, "watching cloud")
 }
+
+// SetCloudDefaults is responsible for removing any previously set cloud
+// default values and setting the new cloud defaults to use. If no defaults are
+// supplied to this function then the currently set cloud default values will be
+// removed and no further operations will be be
+// performed. If no cloud exists for the cloud name then an error satisfying
+// [clouderrors.NotFound] is returned.
+func SetCloudDefaults(
+	ctx context.Context,
+	tx *sql.Tx,
+	cloudName string,
+	defaults map[string]string,
+) error {
+	cloudUUIDStmt := "SELECT uuid FROM cloud WHERE name = ?"
+
+	var cloudUUID string
+	row := tx.QueryRowContext(ctx, cloudUUIDStmt, cloudName)
+	err := row.Scan(&cloudUUID)
+	if errors.Is(err, sql.ErrNoRows) {
+		return fmt.Errorf("%w %q", clouderrors.NotFound, cloudName)
+	} else if err != nil {
+		return fmt.Errorf("getting cloud %q uuid to set cloud model defaults: %w", cloudName, err)
+	}
+
+	deleteStmt := "DELETE FROM cloud_defaults WHERE cloud_defaults.cloud_uuid = ?"
+	_, err = tx.ExecContext(ctx, deleteStmt, cloudUUID)
+	if err != nil {
+		return fmt.Errorf("removing previously set cloud %q model defaults: %w", cloudName, err)
+	}
+
+	if len(defaults) == 0 {
+		return nil
+	}
+
+	bindStr, args := database.MapToMultiPlaceholderTransform(defaults, func(k, v string) []any {
+		return []any{cloudUUID, k, v}
+	})
+
+	insertStmt := fmt.Sprintf(
+		"INSERT INTO cloud_defaults (cloud_uuid, key, value) VALUES %s",
+		bindStr,
+	)
+
+	_, err = tx.ExecContext(ctx, insertStmt, args...)
+	if err != nil {
+		return fmt.Errorf("setting cloud %q model defaults: %w", cloudName, err)
+	}
+
+	return nil
+}