Rework domain/controllerconfig package

The controller config package in domain was wrong. We removed required
fields from the controller config and we coerced on the way in and on the
way out, each with different outcomes.

The following fixes that by not validating on the way in, instead we just
serialize (encode) all the config values to strings. Then on the way out
we just coerce them when required. This ensures that the controller config
map always has the right values at the right time.

Author: SimonRichardson

apiserver/facades/agent/agent/agent.go

@@ -24,8 +24,8 @@ import (
 	"github.com/juju/juju/state"
 )
 
-// ControllerConfigGetter is the interface that gets ControllerConfig form DB.
-type ControllerConfigGetter interface {
+// ControllerConfigService is the interface that gets ControllerConfig form DB.
+type ControllerConfigService interface {
 	ControllerConfig(context.Context) (controller.Config, error)
 }
 
@@ -37,19 +37,19 @@ type AgentAPI struct {
 	*common.ControllerConfigAPI
 	cloudspec.CloudSpecer
 
-	credentialService      CredentialService
-	controllerConfigGetter ControllerConfigGetter
-	st                     *state.State
-	auth                   facade.Authorizer
-	resources              facade.Resources
+	credentialService       CredentialService
+	controllerConfigService ControllerConfigService
+	st                      *state.State
+	auth                    facade.Authorizer
+	resources               facade.Resources
 }
 
 // NewAgentAPI returns an agent API facade.
 func NewAgentAPI(
 	auth facade.Authorizer,
 	resources facade.Resources,
 	st *state.State,
-	controllerConfigGetter ControllerConfigGetter,
+	controllerConfigService ControllerConfigService,
 	externalController common.ExternalControllerService,
 	credentialService common.CredentialService,
 ) (*AgentAPI, error) {
@@ -77,11 +77,11 @@ func NewAgentAPI(
 			cloudspec.MakeCloudSpecCredentialContentWatcherForModel(st, credentialService),
 			common.AuthFuncForTag(model.ModelTag()),
 		),
-		credentialService:      credentialService,
-		controllerConfigGetter: controllerConfigGetter,
-		st:                     st,
-		auth:                   auth,
-		resources:              resources,
+		credentialService:       credentialService,
+		controllerConfigService: controllerConfigService,
+		st:                      st,
+		auth:                    auth,
+		resources:               resources,
 	}, nil
 }
 
@@ -137,7 +137,7 @@ func (api *AgentAPI) StateServingInfo(ctx context.Context) (result params.StateS
 		return params.StateServingInfo{}, errors.Trace(err)
 	}
 	// ControllerAPIPort comes from the controller config.
-	config, err := api.controllerConfigGetter.ControllerConfig(ctx)
+	config, err := api.controllerConfigService.ControllerConfig(ctx)
 	if err != nil {
 		return params.StateServingInfo{}, errors.Trace(err)
 	}

controller/config_test.go

@@ -9,7 +9,6 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	stdtesting "testing"
 	"time"
 
 	"github.com/juju/collections/set"
@@ -25,10 +24,6 @@ import (
 	"github.com/juju/juju/testing"
 )
 
-func Test(t *stdtesting.T) {
-	gc.TestingT(t)
-}
-
 type ConfigSuite struct {
 	testing.FakeJujuXDGDataHomeSuite
 }

controller/package_test.go

@@ -0,0 +1,14 @@
+// Copyright 2023 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package controller_test
+
+import (
+	stdtesting "testing"
+
+	gc "gopkg.in/check.v1"
+)
+
+func Test(t *stdtesting.T) {
+	gc.TestingT(t)
+}

controller/serialize.go

@@ -0,0 +1,37 @@
+// Copyright 2023 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package controller
+
+import (
+	"fmt"
+	"time"
+
+	"github.com/juju/errors"
+)
+
+// EncodeToString encodes the given controller config into a map of strings.
+func EncodeToString(cfg Config) (map[string]string, error) {
+	result := make(map[string]string, len(cfg))
+	for key, v := range cfg {
+		switch v := v.(type) {
+		case string:
+			result[key] = v
+		case bool:
+			result[key] = fmt.Sprintf("%t", v)
+		case int, int8, int16, int32, int64:
+			result[key] = fmt.Sprintf("%d", v)
+		case uint, uint8, uint16, uint32, uint64:
+			result[key] = fmt.Sprintf("%d", v)
+		case float32, float64:
+			result[key] = fmt.Sprintf("%f", v)
+		case time.Duration:
+			result[key] = v.String()
+		case time.Time:
+			result[key] = v.Format(time.RFC3339Nano)
+		default:
+			return nil, errors.Errorf("unable to serialize controller config key %q: unknown type %T", key, v)
+		}
+	}
+	return result, nil
+}

controller/serialize_test.go

@@ -0,0 +1,43 @@
+// Copyright 2023 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package controller_test
+
+import (
+	"github.com/juju/testing"
+	jc "github.com/juju/testing/checkers"
+	gc "gopkg.in/check.v1"
+
+	"github.com/juju/juju/controller"
+	jujutesting "github.com/juju/juju/testing"
+)
+
+type EncodeToStringSuite struct {
+	testing.IsolationSuite
+}
+
+var _ = gc.Suite(&EncodeToStringSuite{})
+
+func (s *EncodeToStringSuite) TestEncodeToString(c *gc.C) {
+	cfg := jujutesting.FakeControllerConfig()
+
+	encoded, err := controller.EncodeToString(cfg)
+	c.Assert(err, jc.ErrorIsNil)
+
+	c.Assert(encoded, gc.DeepEquals, map[string]string{
+		"controller-uuid":           jujutesting.ControllerTag.Id(),
+		"ca-cert":                   jujutesting.CACert,
+		"state-port":                "1234",
+		"api-port":                  "17777",
+		"set-numa-control-policy":   "false",
+		"model-logfile-max-backups": "1",
+		"model-logfile-max-size":    "1M",
+		"model-logs-size":           "1M",
+		"max-txn-log-size":          "10M",
+		"auditing-enabled":          "false",
+		"audit-log-capture-args":    "true",
+		"audit-log-max-size":        "200M",
+		"audit-log-max-backups":     "5",
+		"query-tracing-threshold":   "1s",
+	})
+}

domain/controllerconfig/bootstrap/bootstrap.go

@@ -18,25 +18,16 @@ import (
 // into the database.
 func InsertInitialControllerConfig(cfg controller.Config) func(context.Context, database.TxnRunner) error {
 	return func(ctx context.Context, db database.TxnRunner) error {
-		fields, _, err := controller.ConfigSchema.ValidationSchema()
+		values, err := controller.EncodeToString(cfg)
 		if err != nil {
 			return errors.Trace(err)
 		}
 
-		values := make(map[string]any)
-		for k, v := range cfg {
-			if field, ok := fields[k]; ok {
-				if v, err = field.Coerce(v, []string{k}); err != nil {
-					return errors.Trace(err)
-				}
-			}
-			values[k] = v
-		}
+		query := "INSERT INTO controller_config (key, value) VALUES (?, ?)"
 
 		return errors.Trace(db.StdTxn(ctx, func(ctx context.Context, tx *sql.Tx) error {
 			for k, v := range values {
-				q := "INSERT INTO controller_config (key, value) VALUES (?, ?)"
-				if _, err := tx.ExecContext(ctx, q, k, v); err != nil {
+				if _, err := tx.ExecContext(ctx, query, k, v); err != nil {
 					if jujudatabase.IsErrConstraintPrimaryKey(errors.Cause(err)) {
 						return errors.AlreadyExistsf("controller configuration key %q", k)
 					}

domain/controllerconfig/controllerconfig_test.go

@@ -0,0 +1,89 @@
+// Copyright 2023 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package controllerconfig
+
+import (
+	ctx "context"
+	"time"
+
+	"github.com/juju/collections/set"
+	jc "github.com/juju/testing/checkers"
+	gc "gopkg.in/check.v1"
+
+	"github.com/juju/juju/controller"
+	"github.com/juju/juju/domain/controllerconfig/bootstrap"
+	"github.com/juju/juju/domain/controllerconfig/service"
+	domainstate "github.com/juju/juju/domain/controllerconfig/state"
+	schematesting "github.com/juju/juju/domain/schema/testing"
+	jujutesting "github.com/juju/juju/testing"
+)
+
+type controllerconfigSuite struct {
+	schematesting.ControllerSuite
+}
+
+var _ = gc.Suite(&controllerconfigSuite{})
+
+func (s *controllerconfigSuite) TestControllerConfigRoundTrips(c *gc.C) {
+	st := domainstate.NewState(s.TxnRunnerFactory())
+	srv := service.NewService(st, nil)
+
+	cfgMap := map[string]any{
+		controller.AuditingEnabled:        true,
+		controller.AuditLogCaptureArgs:    false,
+		controller.AuditLogMaxBackups:     10,
+		controller.PublicDNSAddress:       "controller.test.com:1234",
+		controller.APIPortOpenDelay:       "100ms",
+		controller.MigrationMinionWaitMax: "101ms",
+		controller.PruneTxnSleepTime:      "102ms",
+		controller.QueryTracingThreshold:  "103ms",
+		controller.MaxDebugLogDuration:    "104ms",
+	}
+	cfgIn, err := controller.NewConfig(
+		jujutesting.ControllerTag.Id(),
+		jujutesting.CACert,
+		cfgMap,
+	)
+	c.Assert(err, jc.ErrorIsNil)
+
+	err = bootstrap.InsertInitialControllerConfig(cfgIn)(ctx.Background(), s.TxnRunner())
+	c.Assert(err, jc.ErrorIsNil)
+
+	cfgOut, err := srv.ControllerConfig(ctx.Background())
+	c.Assert(err, jc.ErrorIsNil)
+
+	selected := filterConfig(cfgOut)
+
+	err = srv.UpdateControllerConfig(ctx.Background(), selected, nil)
+	c.Assert(err, jc.ErrorIsNil)
+
+	cfgOut, err = srv.ControllerConfig(ctx.Background())
+	c.Assert(err, jc.ErrorIsNil)
+
+	c.Assert(cfgOut.AuditingEnabled(), jc.IsTrue)
+	c.Assert(cfgOut.AuditLogCaptureArgs(), jc.IsFalse)
+	c.Assert(cfgOut.AuditLogMaxBackups(), gc.Equals, 10)
+	c.Assert(cfgOut.PublicDNSAddress(), gc.Equals, "controller.test.com:1234")
+	c.Assert(cfgOut.APIPortOpenDelay(), gc.Equals, 100*time.Millisecond)
+	c.Assert(cfgOut.MigrationMinionWaitMax(), gc.Equals, 101*time.Millisecond)
+	c.Assert(cfgOut.PruneTxnSleepTime(), gc.Equals, 102*time.Millisecond)
+	c.Assert(cfgOut.QueryTracingThreshold(), gc.Equals, 103*time.Millisecond)
+	c.Assert(cfgOut.MaxDebugLogDuration(), gc.Equals, 104*time.Millisecond)
+}
+
+func keys(m map[string]any) set.Strings {
+	var result []string
+	for k := range m {
+		result = append(result, k)
+	}
+	return set.NewStrings(result...)
+}
+
+func filterConfig(m map[string]any) map[string]any {
+	k := keys(m).Difference(controller.AllowedUpdateConfigAttributes)
+	for _, key := range k.Values() {
+		delete(m, key)
+	}
+	return m
+}

domain/controllerconfig/package_test.go

@@ -0,0 +1,14 @@
+// Copyright 2023 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package controllerconfig
+
+import (
+	"testing"
+
+	gc "gopkg.in/check.v1"
+)
+
+func TestPackage(t *testing.T) {
+	gc.TestingT(t)
+}