Merge pull request juju#17660 from barrettj12/ctrl-modelid-ctx

juju#17660

<!-- Why this change is needed and what it does. -->

This PR fixes a chicken-and-egg problem in setting up the apiserver. Previously we were given just a service factory getter. We needed to pass in the controller model UUID to obtain the service factory (for the controller model). However, we needed the service factory to get the controller model UUID.

To get around this, we were using a nasty hack where we passed in the controller namespace (not UUID). This worked because the controller service factory methods don't depend on the UUID passed in. (In fact, any string would have worked). Then we could obtain the controller UUID from this "false" service factory, and pass it back in to the service factory getter to get the "real" service factory. Something like this:
```go
// the arg here should be a model UUID, but we don't have this yet
fakeServiceFactory := serviceFactoryGetter.FactoryForModel(database.ControllerNS)
controllerModel, err := fakeServiceFactory.Model().ControllerModel(ctx)
if err != nil { ... }
realServiceFactory := serviceFactoryGetter.FactoryForModel(controllerModel.UUID.String())
...
```

To fix this, we add the `controllerModelID` as a field to `apiserver.ServerConfig` and `api.sharedServerContext`. Then, we will always have the controller model UUID when we need it to pass in to the service factory getter.

To nudge developers in the right direction, I've changed the arg type of `ServiceFactoryGetter.FactoryForModel` to `core/model.UUID`:
```go
type ServiceFactoryGetter interface {
 // FactoryForModel returns a ServiceFactory for the given model.
 FactoryForModel(modelID model.UUID) ServiceFactory
}
```
This should reduce the temptation to pass in a namespace or fake value that is not a valid model UUID.

Drive-bys:
- move apiserver worker services to `service.go`
- change some instances of `modelUUID string` to `modelID model.UUID`

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [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

<!-- Describe steps to verify that the change works. -->

Bootstrap Juju and run `smoke` tests.

Check that the GitHub actions pass below.

## Links

<!-- Link to all relevant specification, documentation, bug, issue or JIRA card. -->

**Jira card:** JUJU-6323

apiserver/admin.go

@@ -20,7 +20,6 @@ import (
 	"github.com/juju/juju/apiserver/facade"
 	"github.com/juju/juju/apiserver/observer"
 	"github.com/juju/juju/core/auditlog"
-	"github.com/juju/juju/core/database"
 	"github.com/juju/juju/core/network"
 	"github.com/juju/juju/core/permission"
 	"github.com/juju/juju/core/pinger"
@@ -463,7 +462,7 @@ func (a *admin) checkUserPermissions(authInfo authentication.AuthInfo, controlle
 		// The best option might be to round up all of these special case
 		// "everyone@external" checks and push them into either the permission
 		// delegator or the service itself.
-		accessService := a.srv.shared.serviceFactoryGetter.FactoryForModel(database.ControllerNS).Access()
+		accessService := a.srv.shared.serviceFactoryGetter.FactoryForModel(a.srv.shared.controllerModelID).Access()
 		everyoneGroupUser, err := accessService.ReadUserAccessForTarget(
 			context.TODO(),
 			common.EveryoneTagName,

apiserver/apiserver.go

@@ -46,6 +46,7 @@ import (
 	"github.com/juju/juju/core/database"
 	"github.com/juju/juju/core/lease"
 	corelogger "github.com/juju/juju/core/logger"
+	"github.com/juju/juju/core/model"
 	"github.com/juju/juju/core/objectstore"
 	"github.com/juju/juju/core/presence"
 	"github.com/juju/juju/core/resources"
@@ -164,6 +165,9 @@ type ServerConfig struct {
 	// ControllerUUID is the controller unique identifier.
 	ControllerUUID string
 
+	// ControllerModelID is the ID for the controller model.
+	ControllerModelID model.UUID
+
 	// LocalMacaroonAuthenticator is the request authenticator used for verifying
 	// local user macaroons.
 	LocalMacaroonAuthenticator macaroon.LocalMacaroonAuthenticator
@@ -266,6 +270,9 @@ func (c ServerConfig) Validate() error {
 	if c.ControllerUUID == "" {
 		return errors.NotValidf("missing ControllerUUID")
 	}
+	if c.ControllerModelID == "" {
+		return errors.NotValidf("missing ControllerModelID")
+	}
 	if c.LocalMacaroonAuthenticator == nil {
 		return errors.NotValidf("missing local macaroon authenticator")
 	}
@@ -337,7 +344,7 @@ func NewServer(ctx context.Context, cfg ServerConfig) (*Server, error) {
 const readyTimeout = time.Second * 30
 
 func newServer(ctx context.Context, cfg ServerConfig) (_ *Server, err error) {
-	controllerServiceFactory := cfg.ServiceFactoryGetter.FactoryForModel(database.ControllerNS)
+	controllerServiceFactory := cfg.ServiceFactoryGetter.FactoryForModel(cfg.ControllerModelID)
 	controllerConfigService := controllerServiceFactory.ControllerConfig()
 	controllerConfig, err := controllerConfigService.ControllerConfig(ctx)
 	if err != nil {
@@ -358,6 +365,7 @@ func newServer(ctx context.Context, cfg ServerConfig) (_ *Server, err error) {
 		presence:             cfg.Presence,
 		leaseManager:         cfg.LeaseManager,
 		controllerUUID:       cfg.ControllerUUID,
+		controllerModelID:    cfg.ControllerModelID,
 		controllerConfig:     controllerConfig,
 		logger:               internallogger.GetLogger("juju.apiserver"),
 		charmhubHTTPClient:   cfg.CharmhubHTTPClient,
@@ -1107,7 +1115,7 @@ func (srv *Server) apiHandler(w http.ResponseWriter, req *http.Request) {
 		if err := srv.serveConn(
 			srv.tomb.Context(req.Context()),
 			conn,
-			modelUUID,
+			model.UUID(modelUUID),
 			connectionID,
 			apiObserver,
 			req.Host,
@@ -1120,7 +1128,7 @@ func (srv *Server) apiHandler(w http.ResponseWriter, req *http.Request) {
 func (srv *Server) serveConn(
 	ctx context.Context,
 	wsConn *websocket.Conn,
-	modelUUID string,
+	modelID model.UUID,
 	connectionID uint64,
 	apiObserver observer.Observer,
 	host string,
@@ -1129,33 +1137,27 @@ func (srv *Server) serveConn(
 	recorderFactory := observer.NewRecorderFactory(apiObserver, nil, observer.NoCaptureArgs)
 	conn := rpc.NewConn(codec, recorderFactory)
 
-	modelService := srv.shared.serviceFactoryGetter.FactoryForModel(database.ControllerNS).Model()
-
 	// Note that we don't overwrite modelUUID here because
 	// newAPIHandler treats an empty modelUUID as signifying
 	// the API version used.
-	resolvedModelUUID := modelUUID
-	if modelUUID == "" {
-		info, err := modelService.ControllerModel(ctx)
-		if err != nil {
-			return errors.Trace(err)
-		}
-		resolvedModelUUID = info.UUID.String()
+	resolvedModelID := modelID
+	if resolvedModelID == "" {
+		resolvedModelID = srv.shared.controllerModelID
 	}
 
 	tracer, err := srv.shared.tracerGetter.GetTracer(
 		ctx,
-		coretrace.Namespace("apiserver", resolvedModelUUID),
+		coretrace.Namespace("apiserver", resolvedModelID.String()),
 	)
 	if err != nil {
-		logger.Infof("failed to get tracer for model %q: %v", modelUUID, err)
+		logger.Infof("failed to get tracer for model %q: %v", resolvedModelID, err)
 		tracer = coretrace.NoopTracer{}
 	}
 
 	// Grab the object store for the model.
-	objectStore, err := srv.shared.objectStoreGetter.GetObjectStore(ctx, resolvedModelUUID)
+	objectStore, err := srv.shared.objectStoreGetter.GetObjectStore(ctx, resolvedModelID.String())
 	if err != nil {
-		return errors.Annotatef(err, "getting object store for model %q", resolvedModelUUID)
+		return errors.Annotatef(err, "getting object store for model %q", resolvedModelID)
 	}
 
 	// Grab the object store for the controller, this is primarily used for
@@ -1165,10 +1167,10 @@ func (srv *Server) serveConn(
 		return errors.Annotatef(err, "getting controller object store")
 	}
 
-	serviceFactory := srv.shared.serviceFactoryGetter.FactoryForModel(resolvedModelUUID)
+	serviceFactory := srv.shared.serviceFactoryGetter.FactoryForModel(resolvedModelID)
 
 	var handler *apiHandler
-	st, err := srv.shared.statePool.Get(resolvedModelUUID)
+	st, err := srv.shared.statePool.Get(resolvedModelID.String())
 	if err == nil {
 		defer st.Release()
 		handler, err = newAPIHandler(
@@ -1182,13 +1184,13 @@ func (srv *Server) serveConn(
 			objectStore,
 			srv.shared.objectStoreGetter,
 			controllerObjectStore,
-			modelUUID,
+			modelID,
 			connectionID,
 			host,
 		)
 	}
 	if errors.Is(err, errors.NotFound) {
-		err = fmt.Errorf("%w: %q", apiservererrors.UnknownModelError, resolvedModelUUID)
+		err = fmt.Errorf("%w: %q", apiservererrors.UnknownModelError, resolvedModelID)
 	}
 
 	if err != nil {

apiserver/embeddedcli.go

@@ -21,7 +21,6 @@ import (
 	apiservererrors "github.com/juju/juju/apiserver/errors"
 	"github.com/juju/juju/apiserver/httpcontext"
 	"github.com/juju/juju/apiserver/websocket"
-	"github.com/juju/juju/core/database"
 	corelogger "github.com/juju/juju/core/logger"
 	"github.com/juju/juju/core/model"
 	"github.com/juju/juju/internal/featureflag"
@@ -161,7 +160,7 @@ func (h *embeddedCLIHandler) runEmbeddedCommands(
 	// TODO (stickupkid): This is actually terrible. We should refactor
 	// this out, so we can just pass an interface the handler, that hides
 	// all of this nonsense.
-	controllerServiceFactory := h.ctxt.srv.shared.serviceFactoryGetter.FactoryForModel(database.ControllerNS)
+	controllerServiceFactory := h.ctxt.srv.shared.serviceFactoryGetter.FactoryForModel(h.ctxt.srv.shared.controllerModelID)
 	controllerConfigService := controllerServiceFactory.ControllerConfig()
 
 	// Make a pipe to stream the stdout/stderr of the commands.

apiserver/export_test.go

@@ -20,6 +20,7 @@ import (
 	"github.com/juju/juju/apiserver/facade"
 	"github.com/juju/juju/apiserver/stateauthenticator"
 	"github.com/juju/juju/controller"
+	"github.com/juju/juju/core/model"
 	"github.com/juju/juju/core/objectstore"
 	"github.com/juju/juju/core/permission"
 	coretrace "github.com/juju/juju/core/trace"
@@ -140,15 +141,15 @@ func TestingAPIHandler(c *gc.C, pool *state.StatePool, st *state.State, sf servi
 		},
 		tag: names.NewMachineTag("0"),
 	}
-	h, err := newAPIHandler(context.Background(), srv, st, nil, sf, nil, coretrace.NoopTracer{}, nil, nil, nil, st.ModelUUID(), 6543, "testing.invalid:1234")
+	h, err := newAPIHandler(context.Background(), srv, st, nil, sf, nil, coretrace.NoopTracer{}, nil, nil, nil, model.UUID(st.ModelUUID()), 6543, "testing.invalid:1234")
 	c.Assert(err, jc.ErrorIsNil)
 
 	return h, h.Resources()
 }
 
 type StubServiceFactoryGetter struct{}
 
-func (s *StubServiceFactoryGetter) FactoryForModel(string) servicefactory.ServiceFactory {
+func (s *StubServiceFactoryGetter) FactoryForModel(model.UUID) servicefactory.ServiceFactory {
 	return nil
 }
 

apiserver/facades/agent/provisioner/provisioner_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/juju/juju/controller"
 	"github.com/juju/juju/core/constraints"
 	"github.com/juju/juju/core/instance"
+	"github.com/juju/juju/core/model"
 	"github.com/juju/juju/core/network"
 	"github.com/juju/juju/core/status"
 	"github.com/juju/juju/environs/config"
@@ -1304,7 +1305,7 @@ func (s *withoutControllerSuite) TestSetInstanceInfo(c *gc.C) {
 	serviceFactoryGetter := s.ServiceFactoryGetter(c)
 
 	st := s.ControllerModel(c).State()
-	storageService := serviceFactoryGetter.FactoryForModel(st.ModelUUID()).Storage(registry)
+	storageService := serviceFactoryGetter.FactoryForModel(model.UUID(st.ModelUUID())).Storage(registry)
 	err := storageService.CreateStoragePool(context.Background(), "static-pool", "static", map[string]any{"foo": "bar"})
 	c.Assert(err, jc.ErrorIsNil)
 	err = s.ControllerModel(c).UpdateModelConfig(s.ConfigSchemaSourceGetter(c), map[string]any{

apiserver/facades/agent/provisioner/provisioninginfo_test.go

@@ -36,7 +36,7 @@ func (s *withoutControllerSuite) TestProvisioningInfoWithStorage(c *gc.C) {
 	serviceFactoryGetter := s.ServiceFactoryGetter(c)
 
 	st := s.ControllerModel(c).State()
-	storageService := serviceFactoryGetter.FactoryForModel(st.ModelUUID()).Storage(registry)
+	storageService := serviceFactoryGetter.FactoryForModel(model.UUID(st.ModelUUID())).Storage(registry)
 	err := storageService.CreateStoragePool(context.Background(), "static-pool", "static", map[string]any{"foo": "bar"})
 	c.Assert(err, jc.ErrorIsNil)
 
@@ -140,7 +140,7 @@ func (s *withoutControllerSuite) TestProvisioningInfoRootDiskVolume(c *gc.C) {
 	serviceFactoryGetter := s.ServiceFactoryGetter(c)
 
 	st := s.ControllerModel(c).State()
-	storageService := serviceFactoryGetter.FactoryForModel(st.ModelUUID()).Storage(registry)
+	storageService := serviceFactoryGetter.FactoryForModel(model.UUID(st.ModelUUID())).Storage(registry)
 	err := storageService.CreateStoragePool(context.Background(), "static-pool", "static", map[string]any{"foo": "bar"})
 	c.Assert(err, jc.ErrorIsNil)
 	template := state.MachineTemplate{

apiserver/facades/agent/storageprovisioner/storageprovisioner_caas_test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/juju/juju/caas/kubernetes/provider"
 	k8stesting "github.com/juju/juju/caas/kubernetes/provider/testing"
 	"github.com/juju/juju/core/life"
+	"github.com/juju/juju/core/model"
 	loggertesting "github.com/juju/juju/internal/logger/testing"
 	"github.com/juju/juju/rpc/params"
 	"github.com/juju/juju/state"
@@ -61,7 +62,7 @@ func (s *caasProvisionerSuite) SetUpTest(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 	registry := stateenvirons.NewStorageProviderRegistry(broker)
 	serviceFactoryGetter := s.ServiceFactoryGetter(c)
-	storageService := serviceFactoryGetter.FactoryForModel(s.st.ModelUUID()).Storage(registry)
+	storageService := serviceFactoryGetter.FactoryForModel(model.UUID(s.st.ModelUUID())).Storage(registry)
 
 	s.authorizer = &apiservertesting.FakeAuthorizer{
 		Tag:        names.NewMachineTag("0"),

apiserver/facades/agent/storageprovisioner/storageprovisioner_iaas_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/juju/juju/core/blockdevice"
 	"github.com/juju/juju/core/instance"
 	"github.com/juju/juju/core/life"
+	"github.com/juju/juju/core/model"
 	"github.com/juju/juju/core/objectstore"
 	"github.com/juju/juju/core/watcher/watchertest"
 	"github.com/juju/juju/environs"
@@ -70,7 +71,7 @@ func (s *iaasProvisionerSuite) newApi(c *gc.C, blockDeviceService storageprovisi
 	registry := stateenvirons.NewStorageProviderRegistry(env)
 	s.st = s.ControllerModel(c).State()
 	serviceFactoryGetter := s.ServiceFactoryGetter(c)
-	storageService := serviceFactoryGetter.FactoryForModel(s.st.ModelUUID()).Storage(registry)
+	storageService := serviceFactoryGetter.FactoryForModel(model.UUID(s.st.ModelUUID())).Storage(registry)
 
 	s.authorizer = &apiservertesting.FakeAuthorizer{
 		Tag:        names.NewMachineTag("0"),

apiserver/facades/client/application/application_test.go

@@ -85,7 +85,7 @@ func (s *applicationSuite) makeAPI(c *gc.C) *application.APIBase {
 	st := s.ControllerModel(c).State()
 	storageAccess, err := application.GetStorageState(st)
 	c.Assert(err, jc.ErrorIsNil)
-	model, err := st.Model()
+	m, err := st.Model()
 	c.Assert(err, jc.ErrorIsNil)
 	blockChecker := common.NewBlockChecker(st)
 
@@ -101,7 +101,7 @@ func (s *applicationSuite) makeAPI(c *gc.C) *application.APIBase {
 
 	registry := stateenvirons.NewStorageProviderRegistry(env)
 	serviceFactoryGetter := s.ServiceFactoryGetter(c)
-	storageService := serviceFactoryGetter.FactoryForModel(st.ModelUUID()).Storage(registry)
+	storageService := serviceFactoryGetter.FactoryForModel(model.UUID(st.ModelUUID())).Storage(registry)
 	api, err := application.NewAPIBase(
 		application.GetState(st, env),
 		nil,
@@ -111,7 +111,7 @@ func (s *applicationSuite) makeAPI(c *gc.C) *application.APIBase {
 		nil,
 		nil,
 		blockChecker,
-		application.GetModel(model),
+		application.GetModel(m),
 		serviceFactory.Cloud(),
 		serviceFactory.Credential(),
 		serviceFactory.Machine(),

apiserver/facades/client/controller/controller_test.go

@@ -86,7 +86,7 @@ func (s *controllerSuite) SetUpTest(c *gc.C) {
 	s.StateSuite.ControllerConfig = controllerCfg
 	s.StateSuite.SetUpTest(c)
 	s.ServiceFactorySuite.SetUpTest(c)
-	jujujujutesting.SeedDatabase(c, s.ControllerSuite.TxnRunner(), s.ServiceFactoryGetter(c)(s.ControllerModelUUID.String()), controllerCfg)
+	jujujujutesting.SeedDatabase(c, s.ControllerSuite.TxnRunner(), s.ServiceFactoryGetter(c)(s.ControllerModelUUID), controllerCfg)
 
 	s.hub = pubsub.NewStructuredHub(nil)
 

apiserver/introspection.go

@@ -11,7 +11,6 @@ import (
 	"github.com/juju/names/v5"
 
 	"github.com/juju/juju/apiserver/common"
-	"github.com/juju/juju/core/database"
 	"github.com/juju/juju/core/permission"
 	"github.com/juju/juju/rpc/params"
 )
@@ -45,7 +44,7 @@ func (h introspectionHandler) checkAuth(r *http.Request) error {
 	// or "read" access on the controller model, can
 	// access these endpoints.
 
-	accessService := h.ctx.srv.shared.serviceFactoryGetter.FactoryForModel(database.ControllerNS).Access()
+	accessService := h.ctx.srv.shared.serviceFactoryGetter.FactoryForModel(h.ctx.srv.shared.controllerModelID).Access()
 
 	userPermission := func(subject names.UserTag, target names.Tag) (permission.Access, error) {
 		var pID permission.ID

apiserver/registration.go

@@ -19,6 +19,7 @@ import (
 	coremacaroon "github.com/juju/juju/core/macaroon"
 	usererrors "github.com/juju/juju/domain/access/errors"
 	"github.com/juju/juju/environs"
+	"github.com/juju/juju/internal/servicefactory"
 	"github.com/juju/juju/rpc/params"
 	"github.com/juju/juju/state"
 	"github.com/juju/juju/state/stateenvirons"
@@ -51,7 +52,8 @@ func (h *registerUserHandler) ServeHTTP(w http.ResponseWriter, req *http.Request
 
 	// TODO (stickupkid): Remove this nonsense, we should be able to get the
 	// service factory from the handler.
-	serviceFactory := h.ctxt.srv.shared.serviceFactoryGetter.FactoryForModel(st.ModelUUID())
+	var serviceFactory servicefactory.ControllerServiceFactory
+	serviceFactory = h.ctxt.srv.shared.serviceFactoryGetter.FactoryForModel(h.ctxt.srv.shared.controllerModelID)
 	userTag, response, err := h.processPost(
 		req,
 		st.State,

apiserver/root.go

@@ -137,7 +137,7 @@ func newAPIHandler(
 	objectStore objectstore.ObjectStore,
 	objectStoreGetter objectstore.ObjectStoreGetter,
 	controllerObjectStore objectstore.ObjectStore,
-	modelUUID string,
+	modelID model.UUID,
 	connectionID uint64,
 	serverHost string,
 ) (*apiHandler, error) {
@@ -174,7 +174,7 @@ func newAPIHandler(
 		watcherRegistry:       registry,
 		shared:                srv.shared,
 		rpcConn:               rpcConn,
-		modelUUID:             modelUUID,
+		modelUUID:             modelID.String(),
 		connectionID:          connectionID,
 		serverHost:            serverHost,
 	}
@@ -985,7 +985,7 @@ func (ctx *facadeContext) migrationScope(modelUUID string) modelmigration.Scope
 // ServiceFactoryForModel returns the services factory for a given
 // model uuid.
 func (ctx *facadeContext) ServiceFactoryForModel(uuid model.UUID) servicefactory.ServiceFactory {
-	return ctx.r.serviceFactoryGetter.FactoryForModel(uuid.String())
+	return ctx.r.serviceFactoryGetter.FactoryForModel(uuid)
 }
 
 // ObjectStoreForModel returns the object store for a given model uuid.