diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 43ca6e3b3..d25b0ed09 100644 --- a/.github/workflows/ci.yaml +++ b/.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 diff --git a/Makefile b/Makefile index ca5f2e072..52e0460e1 100644 --- a/Makefile +++ b/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)/... diff --git a/api/params/params.go b/api/params/params.go index 918c2812a..ceb5a5f2d 100644 --- a/api/params/params.go +++ b/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 diff --git a/cmd/jimmctl/cmd/importmodel.go b/cmd/jimmctl/cmd/importmodel.go index beb492e09..881593722 100644 --- a/cmd/jimmctl/cmd/importmodel.go +++ b/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 + jimmctl import-model --owner ` // 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) { diff --git a/cmd/jimmctl/cmd/importmodel_test.go b/cmd/jimmctl/cmd/importmodel_test.go index 63d866951..8ff99b516 100644 --- a/cmd/jimmctl/cmd/importmodel_test.go +++ b/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) { diff --git a/internal/dbmodel/model.go b/internal/dbmodel/model.go index 40ef720e0..795b2aa3d 100644 --- a/internal/dbmodel/model.go +++ b/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 diff --git a/internal/jimm/controller.go b/internal/jimm/controller.go index 554efd005..41527ea43 100644 --- a/internal/jimm/controller.go +++ b/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) diff --git a/internal/jimm/controller_test.go b/internal/jimm/controller_test.go index d5ee364d6..5db84f37e 100644 --- a/internal/jimm/controller_test.go +++ b/internal/jimm/controller_test.go @@ -609,6 +609,7 @@ func TestImportModel(t *testing.T) { controllerName string modelUUID string modelInfo func(context.Context, *jujuparams.ModelInfo) error + newOwner string expectedModel dbmodel.Model expectedError string deltas []jujuparams.Delta @@ -616,6 +617,7 @@ func TestImportModel(t *testing.T) { about: "model imported", user: "alice@external", controllerName: "test-controller", + newOwner: "", modelUUID: "00000002-0000-0000-0000-000000000001", modelInfo: func(_ context.Context, info *jujuparams.ModelInfo) error { info.Name = "test-model" @@ -771,19 +773,117 @@ func TestImportModel(t *testing.T) { Access: "read", }}, }, + }, { + about: "model from local user imported", + user: "alice@external", + controllerName: "test-controller", + newOwner: "alice@external", + modelUUID: "00000002-0000-0000-0000-000000000001", + modelInfo: func(_ context.Context, info *jujuparams.ModelInfo) error { + info.Name = "test-model" + info.Type = "test-type" + info.UUID = "00000002-0000-0000-0000-000000000001" + info.ControllerUUID = "00000001-0000-0000-0000-000000000001" + info.DefaultSeries = "test-series" + info.CloudTag = names.NewCloudTag("test-cloud").String() + info.CloudRegion = "test-region" + info.CloudCredentialTag = names.NewCloudCredentialTag("test-cloud/local-user/test-credential").String() + info.CloudCredentialValidity = &trueValue + info.OwnerTag = names.NewUserTag("local-user").String() + info.Life = life.Alive + info.Status = jujuparams.EntityStatus{ + Status: status.Status("available"), + Info: "test-info", + Since: &now, + } + info.Users = []jujuparams.ModelUserInfo{{ + UserName: "local-user", + Access: jujuparams.ModelAdminAccess, + }, { + UserName: "another-user", + Access: jujuparams.ModelReadAccess, + }} + info.Machines = []jujuparams.ModelMachineInfo{{ + Id: "test-machine", + DisplayName: "Test machine", + Status: "test-status", + Message: "test-message", + }} + info.SLA = &jujuparams.ModelSLAInfo{ + Level: "essential", + Owner: "local-user", + } + info.AgentVersion = newVersion("2.1.0") + return nil + }, + expectedModel: dbmodel.Model{ + Name: "test-model", + UUID: sql.NullString{ + String: "00000002-0000-0000-0000-000000000001", + Valid: true, + }, + Owner: dbmodel.User{ + Username: "alice@external", + DisplayName: "Alice", + ControllerAccess: "superuser", + }, + Controller: dbmodel.Controller{ + Name: "test-controller", + UUID: "00000001-0000-0000-0000-000000000001", + CloudName: "test-cloud", + CloudRegion: "test-region-1", + AgentVersion: "3.2.1", + }, + CloudRegion: dbmodel.CloudRegion{ + Cloud: dbmodel.Cloud{ + Name: "test-cloud", + Type: "test", + }, + Name: "test-region", + }, + CloudCredential: dbmodel.CloudCredential{ + Name: "test-credential", + }, + Type: "test-type", + DefaultSeries: "test-series", + Life: "alive", + Status: dbmodel.Status{ + Status: "available", + Info: "test-info", + Since: sql.NullTime{ + Valid: true, + Time: now, + }, + Version: "2.1.0", + }, + SLA: dbmodel.SLA{ + Level: "essential", + Owner: "local-user", + }, + Users: []dbmodel.UserModelAccess{{ + User: dbmodel.User{ + Username: "alice@external", + DisplayName: "Alice", + ControllerAccess: "superuser", + }, + Access: "admin", + }}, + }, }, { about: "model not found", user: "alice@external", controllerName: "test-controller", + newOwner: "", modelUUID: "00000002-0000-0000-0000-000000000001", modelInfo: func(_ context.Context, info *jujuparams.ModelInfo) error { return errors.E(errors.CodeNotFound, "model not found") }, expectedError: "model not found", }, { - about: "cloud credentials not found", + about: "fail import from local user without newOwner flag", user: "alice@external", controllerName: "test-controller", + newOwner: "", modelUUID: "00000002-0000-0000-0000-000000000001", modelInfo: func(_ context.Context, info *jujuparams.ModelInfo) error { info.Name = "test-model" @@ -795,14 +895,35 @@ func TestImportModel(t *testing.T) { info.CloudRegion = "test-region" info.CloudCredentialTag = names.NewCloudCredentialTag("test-cloud/alice@external/unknown-credential").String() info.CloudCredentialValidity = &trueValue + info.OwnerTag = names.NewUserTag("local-user").String() + return nil + }, + expectedError: `cannot import model from local user, try --owner to switch the model owner`, + }, { + about: "cloud credentials not found", + user: "alice@external", + controllerName: "test-controller", + newOwner: "", + modelUUID: "00000002-0000-0000-0000-000000000001", + modelInfo: func(_ context.Context, info *jujuparams.ModelInfo) error { + info.Name = "test-model" + info.Type = "test-type" + info.UUID = "00000002-0000-0000-0000-000000000001" + info.ControllerUUID = "00000001-0000-0000-0000-000000000001" + info.DefaultSeries = "test-series" + info.CloudTag = names.NewCloudTag("invalid-cloud").String() + info.CloudRegion = "test-region" + info.CloudCredentialTag = names.NewCloudCredentialTag("invalid-cloud/alice@external/unknown-credential").String() + info.CloudCredentialValidity = &trueValue info.OwnerTag = names.NewUserTag("alice@external").String() return nil }, - expectedError: `cloudcredential "test-cloud/alice@external/unknown-credential" not found`, + expectedError: `Failed to find cloud credential for user alice@external on cloud invalid-cloud`, }, { about: "cloud region not found", user: "alice@external", controllerName: "test-controller", + newOwner: "", modelUUID: "00000002-0000-0000-0000-000000000001", modelInfo: func(_ context.Context, info *jujuparams.ModelInfo) error { info.Name = "test-model" @@ -822,6 +943,7 @@ func TestImportModel(t *testing.T) { about: "not allowed if not superuser", user: "bob@external", controllerName: "test-controller", + newOwner: "", modelUUID: "00000002-0000-0000-0000-000000000001", modelInfo: func(_ context.Context, info *jujuparams.ModelInfo) error { info.Name = "test-model" @@ -841,6 +963,7 @@ func TestImportModel(t *testing.T) { about: "model already exists", user: "alice@external", controllerName: "test-controller", + newOwner: "", modelUUID: "00000002-0000-0000-0000-000000000002", modelInfo: func(_ context.Context, info *jujuparams.ModelInfo) error { info.Name = "model-1" @@ -861,6 +984,9 @@ func TestImportModel(t *testing.T) { for _, test := range tests { c.Run(test.about, func(c *qt.C) { api := &jimmtest.API{ + GrantModelAccess_: func(ctx context.Context, mt names.ModelTag, ut names.UserTag, uap jujuparams.UserAccessPermission) error { + return nil + }, ModelInfo_: test.modelInfo, ModelWatcherNext_: func(ctx context.Context, id string) ([]jujuparams.Delta, error) { if id != test.about { @@ -895,7 +1021,7 @@ func TestImportModel(t *testing.T) { env.PopulateDB(c, j.Database) user := env.User(test.user).DBObject(c, j.Database) - err = j.ImportModel(ctx, &user, test.controllerName, names.NewModelTag(test.modelUUID)) + err = j.ImportModel(ctx, &user, test.controllerName, names.NewModelTag(test.modelUUID), test.newOwner) if test.expectedError == "" { c.Assert(err, qt.IsNil) diff --git a/internal/jujuapi/jimm.go b/internal/jujuapi/jimm.go index 4bd3566c5..296f6c634 100644 --- a/internal/jujuapi/jimm.go +++ b/internal/jujuapi/jimm.go @@ -425,7 +425,7 @@ func (r *controllerRoot) ImportModel(ctx context.Context, req apiparams.ImportMo return errors.E(op, err, errors.CodeBadRequest) } - err = r.jimm.ImportModel(ctx, r.user, req.Controller, mt) + err = r.jimm.ImportModel(ctx, r.user, req.Controller, mt, req.Owner) if err != nil { return errors.E(op, err) } diff --git a/internal/jujuapi/jimm_test.go b/internal/jujuapi/jimm_test.go index d24c51d6a..f24a8096b 100644 --- a/internal/jujuapi/jimm_test.go +++ b/internal/jujuapi/jimm_test.go @@ -508,6 +508,7 @@ func (s *jimmSuite) TestImportModel(c *gc.C) { req := apiparams.ImportModelRequest{ Controller: "controller-1", ModelTag: s.Model2.Tag().String(), + Owner: "", } err = conn.APICall("JIMM", 3, "", "ImportModel", &req, nil) c.Assert(err, gc.ErrorMatches, `unauthorized \(unauthorized access\)`) diff --git a/local/candid/config.yaml b/local/candid/config.yaml index ed8d20ef8..53c53dcfc 100644 --- a/local/candid/config.yaml +++ b/local/candid/config.yaml @@ -26,5 +26,12 @@ identity-providers: groups: - group1 - group2 + joe: + name: Joe user (non superuser) + email: joe.user@example.com + password: joe + groups: + - group1 + - group2 hidden: false - require-mfa: false \ No newline at end of file + require-mfa: false