Enable importing models created by local users (#1037)

* Add switch-owner option

* Ignore local users on the incoming model

* Tweaks to import logic

* Update setup-controller.sh

* Add new test

* Ensure old test follows previous behavior

* Test tweaks

* Tweaks removing unnecessary changes

* Update to add "owner" flag

* PR comments

* Test fixes

* Add TODO with jira ref

* Fixes

* PR suggestions

* Extra test and tweaks

* Switch to User.Tag()

.github/workflows/ci.yaml

@@ -40,7 +40,7 @@ jobs:
       - name: Start test environment
         run: docker compose up -d
       - name: Build and Test
-        run: go test -mod readonly ./...
+        run: go test -mod readonly ./... -coverprofile cover.out
         env:
           JIMM_DSN: postgresql://jimm:jimm@localhost:5432/jimm
           PGHOST: localhost

Makefile

@@ -34,7 +34,7 @@ build/server: version/commit.txt version/version.txt
 	go build -tags version ./cmd/jimmsrv
 
 check: version/commit.txt version/version.txt
-	go test -p 1 -timeout 30m -tags version $(PROJECT)/...
+	go test -timeout 30m $(PROJECT)/... -coverprofile cover.out
 
 install: version/commit.txt version/version.txt
 	go install -tags version $(INSTALL_FLAGS) -v $(PROJECT)/...

api/params/params.go

@@ -236,6 +236,10 @@ type ImportModelRequest struct {
 
 	// ModelTag is the tag of the model that is to be imported.
 	ModelTag string `json:"model-tag"`
+
+	// Owner specifies the new owner of the model after import.
+	// Can be empty to skip switching the owner.
+	Owner string `json:"owner"`
 }
 
 // CrossModelQueryRequest holds the parameters to perform a cross model query against

cmd/jimmctl/cmd/importmodel.go

@@ -4,6 +4,7 @@ package cmd
 
 import (
 	"github.com/juju/cmd/v3"
+	"github.com/juju/gnuflag"
 	jujuapi "github.com/juju/juju/api"
 	jujucmd "github.com/juju/juju/cmd"
 	"github.com/juju/juju/cmd/modelcmd"
@@ -18,8 +19,16 @@ import (
 const importModelCommandDoc = `
 	import-model imports a model running on a controller to jimm.
 
+	When importing, it is necessary for JIMM to contain cloud credentials
+	for the user against the cloud the incoming model resides.
+
+	The --owner command is necessary when importing a model created by a 
+	local user and will switch the model owner to the desired external user.
+	E.g. --owner my-user@external
+
 	Example:
 		jimmctl import-model <controller name> <model-uuid>
+		jimmctl import-model <controller name> <model-uuid> --owner <username>
 `
 
 // NewImportModelCommand returns a command to import a model.
@@ -50,6 +59,11 @@ func (c *importModelCommand) Info() *cmd.Info {
 	})
 }
 
+// SetFlags implements Command.SetFlags.
+func (c *importModelCommand) SetFlags(f *gnuflag.FlagSet) {
+	f.StringVar(&c.req.Owner, "owner", "", "switch the model owner to the desired user")
+}
+
 // Init implements the cmd.Command interface.
 func (c *importModelCommand) Init(args []string) error {
 	switch len(args) {

cmd/jimmctl/cmd/importmodel_test.go

@@ -44,6 +44,35 @@ func (s *importModelSuite) TestImportModelSuperuser(c *gc.C) {
 	err = s.JIMM.Database.GetModel(context.Background(), &model2)
 	c.Assert(err, gc.Equals, nil)
 	c.Check(model2.CreatedAt.After(model.CreatedAt), gc.Equals, true)
+	c.Check(model2.OwnerUsername, gc.Equals, "charlie@external")
+}
+
+func (s *importModelSuite) TestImportModelFromLocalUser(c *gc.C) {
+	s.AddController(c, "controller-1", s.APIInfo(c))
+	cct := names.NewCloudCredentialTag(jimmtest.TestCloudName + "/charlie@external/cred")
+	s.UpdateCloudCredential(c, cct, jujuparams.CloudCredential{AuthType: "empty"})
+	// Add credentials for Alice on the test cloud, they are needed for the Alice user to become the new model owner
+	cctAlice := names.NewCloudCredentialTag(jimmtest.TestCloudName + "/alice@external/cred")
+	s.UpdateCloudCredential(c, cctAlice, jujuparams.CloudCredential{AuthType: "empty"})
+	mt := s.AddModel(c, names.NewUserTag("charlie@external"), "model-2", names.NewCloudTag(jimmtest.TestCloudName), jimmtest.TestCloudRegionName, cct)
+	var model dbmodel.Model
+	model.SetTag(mt)
+	err := s.JIMM.Database.GetModel(context.Background(), &model)
+	c.Assert(err, gc.Equals, nil)
+	err = s.JIMM.Database.DeleteModel(context.Background(), &model)
+	c.Assert(err, gc.Equals, nil)
+
+	// alice is superuser
+	bClient := s.userBakeryClient("alice")
+	_, err = cmdtesting.RunCommand(c, cmd.NewImportModelCommandForTesting(s.ClientStore, bClient), "controller-1", mt.Id(), "--owner", "alice@external")
+	c.Assert(err, gc.IsNil)
+
+	var model2 dbmodel.Model
+	model2.SetTag(mt)
+	err = s.JIMM.Database.GetModel(context.Background(), &model2)
+	c.Assert(err, gc.Equals, nil)
+	c.Check(model2.CreatedAt.After(model.CreatedAt), gc.Equals, true)
+	c.Check(model2.OwnerUsername, gc.Equals, "alice@external")
 }
 
 func (s *importModelSuite) TestImportModelUnauthorized(c *gc.C) {

internal/dbmodel/model.go

@@ -105,6 +105,12 @@ func (m *Model) UserAccess(u *User) string {
 	return ""
 }
 
+// FromModelUpdate updates the model from the given ModelUpdate.
+func (m *Model) SwitchOwner(u *User) {
+	m.OwnerUsername = u.Username
+	m.Owner = *u
+}
+
 // FromJujuModelInfo converts jujuparams.ModelInfo into Model.
 func (m *Model) FromJujuModelInfo(info jujuparams.ModelInfo) error {
 	m.Name = info.Name

internal/jimm/controller.go

@@ -416,7 +416,7 @@ func (j *JIMM) GetControllerAccess(ctx context.Context, user *dbmodel.User, tag
 }
 
 // ImportModel imports model with the specified uuid from the controller.
-func (j *JIMM) ImportModel(ctx context.Context, u *dbmodel.User, controllerName string, modelTag names.ModelTag) error {
+func (j *JIMM) ImportModel(ctx context.Context, u *dbmodel.User, controllerName string, modelTag names.ModelTag, newOwner string) error {
 	const op = errors.Op("jimm.ImportModel")
 
 	ale := dbmodel.AuditLogEntry{
@@ -461,7 +461,6 @@ func (j *JIMM) ImportModel(ctx context.Context, u *dbmodel.User, controllerName
 	if err != nil {
 		return errors.E(op, err)
 	}
-
 	model := dbmodel.Model{}
 	// fill in data from model info
 	err = model.FromJujuModelInfo(modelInfo)
@@ -471,31 +470,65 @@ func (j *JIMM) ImportModel(ctx context.Context, u *dbmodel.User, controllerName
 	model.ControllerID = controller.ID
 	model.Controller = controller
 
-	// fetch the model owner user
-	ownerTag, err := names.ParseUserTag(modelInfo.OwnerTag)
+	var ownerString string
+	if newOwner != "" {
+		// Switch the model to be owned by the specified user.
+		ownerString = names.UserTagKind + "-" + newOwner
+	} else {
+		// Use the model owner user
+		ownerString = modelInfo.OwnerTag
+	}
+	ownerTag, err := names.ParseUserTag(ownerString)
 	if err != nil {
 		return errors.E(op, err)
 	}
-	owner := dbmodel.User{}
-	owner.SetTag(ownerTag)
-	err = j.Database.GetUser(ctx, &owner)
+	if ownerTag.IsLocal() {
+		return errors.E(op, "cannot import model from local user, try --owner to switch the model owner")
+	}
+	ownerUser := dbmodel.User{}
+	ownerUser.SetTag(ownerTag)
+	err = j.Database.GetUser(ctx, &ownerUser)
 	if err != nil {
 		return errors.E(op, err)
 	}
-	model.OwnerUsername = owner.Username
-	model.Owner = owner
+	model.SwitchOwner(&ownerUser)
+
+	ownerHasModelAccess := false
+	for _, user := range model.Users {
+		if user.User.Username == ownerUser.Username {
+			ownerHasModelAccess = true
+			break
+		}
+	}
+	if !ownerHasModelAccess {
+		zapctx.Debug(ctx, "User doesn't have model access, adding it")
+		// Ensure the current user gets access to the model
+		// This will be applied to JIMM's access table lower down.
+		model.Users = append(model.Users, dbmodel.UserModelAccess{User: *u, Access: string(jujuparams.ModelAdminAccess)})
+	}
+
+	// TODO(CSS-5458): Remove the below section on cloud credentials once we no longer persist the relation between
+	// cloud credentials and models
 
 	// fetch cloud credential used by the model
-	credentialTag, err := names.ParseCloudCredentialTag(modelInfo.CloudCredentialTag)
+	cloudTag, err := names.ParseCloudTag(modelInfo.CloudTag)
 	if err != nil {
-		return errors.E(op, err)
-	}
-	cloudCredential := dbmodel.CloudCredential{}
-	cloudCredential.SetTag(credentialTag)
-	err = j.Database.GetCloudCredential(ctx, &cloudCredential)
+		errors.E(op, err)
+	}
+	// Note that the model already has a cloud credential configured which it will use when deploying new
+	// applications. JIMM needs some cloud credential reference to be able to import the model so use any
+	// credential against the cloud the model is deployed against. Even using the correct cloud for the
+	// credential is not strictly necessary, but will help prevent the user think they can create new
+	// models on the incoming cloud.
+	allCredentials, err := j.Database.GetUserCloudCredentials(ctx, &ownerUser, cloudTag.Id())
 	if err != nil {
 		return errors.E(op, err)
 	}
+	if len(allCredentials) == 0 {
+		return errors.E(op, errors.CodeNotFound, fmt.Sprintf("Failed to find cloud credential for user %s on cloud %s", ownerUser.Username, cloudTag.Id()))
+	}
+	cloudCredential := allCredentials[0]
+
 	model.CloudCredentialID = cloudCredential.ID
 	model.CloudCredential = cloudCredential
 
@@ -505,6 +538,7 @@ func (j *JIMM) ImportModel(ctx context.Context, u *dbmodel.User, controllerName
 	}
 	err = j.Database.GetCloud(ctx, &cloud)
 	if err != nil {
+		zapctx.Error(ctx, "failed to get cloud", zap.String("cloud", cloud.Name))
 		return errors.E(op, err)
 	}
 
@@ -521,10 +555,24 @@ func (j *JIMM) ImportModel(ctx context.Context, u *dbmodel.User, controllerName
 		return errors.E(op, "cloud region not found")
 	}
 
+	var usersExcludingLocalUsers []dbmodel.UserModelAccess
+	for _, userAccess := range model.Users {
+		userTag, ok := userAccess.User.Tag().(names.UserTag)
+		if !ok {
+			zapctx.Error(ctx, "failed to extract user tag", zap.String("username", userAccess.User.Username))
+			continue
+		}
+		if userTag.IsLocal() {
+			// Don't propogate local users into JIMM.
+			continue
+		}
+		usersExcludingLocalUsers = append(usersExcludingLocalUsers, userAccess)
+	}
+	model.Users = usersExcludingLocalUsers
+
 	for i, userAccess := range model.Users {
 		u := userAccess.User
-		err = j.Database.GetUser(ctx, &u)
-		if err != nil {
+		if err = j.Database.GetUser(ctx, &u); err != nil {
 			return errors.E(op, err)
 		}
 		model.Users[i].Username = u.Username
@@ -539,6 +587,19 @@ func (j *JIMM) ImportModel(ctx context.Context, u *dbmodel.User, controllerName
 		return errors.E(op, err)
 	}
 
+	if !ownerHasModelAccess {
+		// Here we finally grant the model owner, access to the underlying model.
+		err = j.doModelAdmin(ctx, u, modelTag, func(m *dbmodel.Model, api API) error {
+			if err := api.GrantModelAccess(ctx, modelTag, ownerUser.Tag().(names.UserTag), jujuparams.ModelAdminAccess); err != nil {
+				return err
+			}
+			return nil
+		})
+		if err != nil {
+			return errors.E(op, err, fmt.Sprintf("Failed to grant user %s admin access on the model", ownerUser.Username))
+		}
+	}
+
 	modelAPI, err := j.dial(ctx, &controller, modelTag)
 	if err != nil {
 		return errors.E(op, err)