Skip to content

Commit

Permalink
fix: better handle model imports
Browse files Browse the repository at this point in the history 
Importing a model owned by a local Juju user requires the use of the --switch-owner CLI flag to change the model owner in jimm to a new external user. This change doesn't propagate to the Juju controller so there needs to be a lot of complex logic to ensure we replace all results containing the old user with the new one.

That was not being done and as a result, things like listing application offers were not working. This change allows models created by local Juju users to keep their model owner tag in JIMM. Although JIMM normally deals with external users, there is no reason it can't also hold local users for cases of model imports.

To handle the cloud-credential of an imported model where JIMM is not aware of the local user's credentials, we allow the value of the cloud-credential in JIMM to be nil. This works out because we fetch model info from the controller anyway.
  • Loading branch information
@kian99
kian99 committed Jan 27, 2025
1 parent d97f72e commit b03285b
Show file tree
Hide file tree
Showing 19 changed files with 110 additions and 202 deletions.
4 changes: 2 additions & 2 deletions cmd/jimmctl/cmd/relation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ func initializeEnvironment(c *gc.C, ctx context.Context, db *db.Database, u dbmo
OwnerIdentityName: u.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
}
err = db.AddModel(ctx, &model)
c.Assert(err, gc.IsNil)
Expand Down Expand Up @@ -539,7 +539,7 @@ func (s *relationSuite) TestCheckRelationViaSuperuser(c *gc.C) {
OwnerIdentityName: u.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
Life: "alive",
}

Expand Down
4 changes: 2 additions & 2 deletions internal/db/applicationoffer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func initTestEnvironment(c *qt.C, db *db.Database) testEnvironment {
Owner: env.u,
Controller: env.controller,
CloudRegion: env.cloud.Regions[0],
CloudCredential: env.cred,
CloudCredential: &env.cred,
Life: state.Alive.String(),
}
c.Assert(db.DB.Create(&env.model).Error, qt.IsNil)
Expand All @@ -94,7 +94,7 @@ func initTestEnvironment(c *qt.C, db *db.Database) testEnvironment {
Owner: env.u,
Controller: env.controller,
CloudRegion: env.cloud.Regions[0],
CloudCredential: env.cred,
CloudCredential: &env.cred,
Life: state.Alive.String(),
}
c.Assert(db.DB.Create(&env.model1).Error, qt.IsNil)
Expand Down
20 changes: 10 additions & 10 deletions internal/db/model_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func (s *dbSuite) TestAddModel(c *qt.C) {
OwnerIdentityName: u.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
Life: state.Alive.String(),
}
m1 := model
Expand Down Expand Up @@ -135,8 +135,8 @@ func (s *dbSuite) TestGetModel(c *qt.C) {
Controller: controller,
CloudRegionID: cloud.Regions[0].ID,
CloudRegion: cloud.Regions[0],
CloudCredentialID: cred.ID,
CloudCredential: cred,
CloudCredentialID: &cred.ID,
CloudCredential: &cred,
Life: state.Alive.String(),
}
model.CloudCredential.Cloud = dbmodel.Cloud{}
Expand Down Expand Up @@ -220,7 +220,7 @@ func (s *dbSuite) TestUpdateModel(c *qt.C) {
OwnerIdentityName: i.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
Life: state.Alive.String(),
}
err = s.Database.AddModel(context.Background(), &model)
Expand Down Expand Up @@ -288,7 +288,7 @@ func (s *dbSuite) TestDeleteModel(c *qt.C) {
ControllerID: controller.ID,
Controller: controller,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
Life: state.Alive.String(),
}

Expand Down Expand Up @@ -362,7 +362,7 @@ func (s *dbSuite) TestGetModelsUsingCredential(c *qt.C) {
OwnerIdentityName: i.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred1.ID,
CloudCredentialID: &cred1.ID,
Life: state.Alive.String(),
}
err = s.Database.AddModel(context.Background(), &model1)
Expand All @@ -377,7 +377,7 @@ func (s *dbSuite) TestGetModelsUsingCredential(c *qt.C) {
OwnerIdentityName: i.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred2.ID,
CloudCredentialID: &cred2.ID,
Life: state.Alive.String(),
}
err = s.Database.AddModel(context.Background(), &model2)
Expand All @@ -398,7 +398,7 @@ func (s *dbSuite) TestGetModelsUsingCredential(c *qt.C) {
ControllerID: controller.ID,
Controller: controller,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred1.ID,
CloudCredentialID: &cred1.ID,
Life: state.Alive.String(),
}})

Expand Down Expand Up @@ -622,7 +622,7 @@ func (s *dbSuite) TestGetModelsByController(c *qt.C) {
Owner: *u,
Controller: controller,
CloudRegion: cloud.Regions[0],
CloudCredential: cred,
CloudCredential: &cred,
Life: state.Alive.String(),
}, {
Name: "test-model-2",
Expand All @@ -633,7 +633,7 @@ func (s *dbSuite) TestGetModelsByController(c *qt.C) {
Owner: *u,
Controller: controller,
CloudRegion: cloud.Regions[0],
CloudCredential: cred,
CloudCredential: &cred,
Life: state.Alive.String(),
}}
for _, m := range models {
Expand Down
2 changes: 1 addition & 1 deletion internal/db/resource_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func SetupDB(c *qt.C, database *db.Database) (dbmodel.Model, dbmodel.Controller,
OwnerIdentityName: u.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
Life: state.Alive.String(),
}
err = database.AddModel(context.Background(), &model)
Expand Down
2 changes: 1 addition & 1 deletion internal/dbmodel/applicationoffer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func TestApplicationOfferUniqueConstraint(t *testing.T) {
Owner: u,
Controller: ctl,
CloudRegion: cl.Regions[0],
CloudCredential: cred,
CloudCredential: &cred,
Life: state.Alive.String(),
}
c.Assert(db.Create(&m).Error, qt.IsNil)
Expand Down
4 changes: 2 additions & 2 deletions internal/dbmodel/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func TestControllerModels(t *testing.T) {
Owner: u1,
Controller: ctl,
CloudRegion: cl.Regions[0],
CloudCredential: cred,
CloudCredential: &cred,
}
c.Assert(db.Create(&m1).Error, qt.IsNil)
u2, err := dbmodel.NewIdentity("[email protected]")
Expand All @@ -103,7 +103,7 @@ func TestControllerModels(t *testing.T) {
Owner: *u2,
Controller: ctl,
CloudRegion: cl.Regions[0],
CloudCredential: cred,
CloudCredential: &cred,
}
c.Assert(db.Create(&m2).Error, qt.IsNil)

Expand Down
22 changes: 10 additions & 12 deletions internal/dbmodel/model.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ type Model struct {
CloudRegion CloudRegion

// CloudCredential is the credential used with the model.
CloudCredentialID uint
CloudCredential CloudCredential `gorm:"foreignkey:CloudCredentialID;references:ID"`
CloudCredentialID *uint
CloudCredential *CloudCredential `gorm:"foreignkey:CloudCredentialID;references:ID"`

// Life holds the life status of the model.
Life string
Expand Down Expand Up @@ -102,15 +102,11 @@ func (m *Model) FromJujuModelInfo(info jujuparams.ModelInfo) error {
}
m.CloudRegion.Cloud.Name = ct.Id()
}
if info.CloudCredentialTag != "" {
cct, err := names.ParseCloudCredentialTag(info.CloudCredentialTag)
if err != nil {
return errors.E(err)
}
m.CloudCredential.Name = cct.Name()
m.CloudCredential.CloudName = cct.Cloud().Id()
m.CloudCredential.Owner.Name = cct.Owner().Id()
}
// Note that we ignore the cloud-credential from
// the controller here because in all cases JIMM
// either knows the credential used (because it
// created the model) or it doesn't have access
// to the credential and doesn't care (imported models).

return nil
}
Expand All @@ -134,6 +130,8 @@ func (m Model) ToJujuModel() jujuparams.Model {
// It uses the info from the controller and JIMM's db to fill the jujuparams.ModelSummary.
// maskingControllerUUID is used to mask the controllerUUID with the JIMM's one.
// access is the user access level got from JIMM.
// This function indicates which fields JIMM is authoritative and which can come directly
// from Juju.
func (m Model) MergeModelSummaryFromController(modelSummaryFromController *jujuparams.ModelSummary, maskingControllerUUID string, access jujuparams.UserAccessPermission) jujuparams.ModelSummary {
if modelSummaryFromController == nil {
modelSummaryFromController = &jujuparams.ModelSummary{}
Expand All @@ -148,7 +146,7 @@ func (m Model) MergeModelSummaryFromController(modelSummaryFromController *jujup
modelSummaryFromController.ProviderType = m.CloudRegion.Cloud.Type
modelSummaryFromController.CloudTag = m.CloudRegion.Cloud.Tag().String()
modelSummaryFromController.CloudRegion = m.CloudRegion.Name
modelSummaryFromController.CloudCredentialTag = m.CloudCredential.Tag().String()
// modelSummaryFromController.CloudCredentialTag = m.CloudCredential.Tag().String()
modelSummaryFromController.OwnerTag = m.Owner.Tag().String()
modelSummaryFromController.Life = life.Value(m.Life)
modelSummaryFromController.UserAccess = access
Expand Down
20 changes: 8 additions & 12 deletions internal/dbmodel/model_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func TestRecreateDeletedModel(t *testing.T) {
Name: "test-1",
Controller: ctl,
CloudRegion: cl.Regions[0],
CloudCredential: cred,
CloudCredential: &cred,
}
c.Assert(db.Create(&m1).Error, qt.IsNil)

Expand All @@ -55,7 +55,7 @@ func TestRecreateDeletedModel(t *testing.T) {
Name: "test-1",
Controller: ctl,
CloudRegion: cl.Regions[0],
CloudCredential: cred,
CloudCredential: &cred,
}
c.Check(db.Create(&m2).Error, qt.ErrorMatches, `.*violates unique constraint "unique_model_names".*`)

Expand All @@ -78,7 +78,7 @@ func TestModel(t *testing.T) {
Owner: u,
Controller: ctl,
CloudRegion: cl.Regions[0],
CloudCredential: cred,
CloudCredential: &cred,
Life: state.Alive.String(),
}
c.Assert(db.Create(&m).Error, qt.IsNil)
Expand Down Expand Up @@ -131,7 +131,7 @@ func TestModelUniqueConstraint(t *testing.T) {
Owner: u,
Controller: ctl1,
CloudRegion: cl1.Regions[0],
CloudCredential: cred1,
CloudCredential: &cred1,
Life: state.Alive.String(),
}
c.Assert(db.Create(&m1).Error, qt.IsNil)
Expand All @@ -145,7 +145,7 @@ func TestModelUniqueConstraint(t *testing.T) {
Owner: u,
Controller: ctl2,
CloudRegion: cl2.Regions[0],
CloudCredential: cred2,
CloudCredential: &cred2,
Life: state.Alive.String(),
}
c.Assert(db.Create(&m2).Error, qt.ErrorMatches, `ERROR: duplicate key value violates unique constraint .*`)
Expand Down Expand Up @@ -178,7 +178,7 @@ func TestToJujuModel(t *testing.T) {
Owner: u,
Controller: ctl,
CloudRegion: cl.Regions[0],
CloudCredential: cred,
CloudCredential: &cred,
Life: state.Alive.String(),
}
m.CloudRegion.Cloud = cl
Expand All @@ -205,7 +205,7 @@ func TestToJujuModelSummary(t *testing.T) {
Owner: u,
Controller: ctl,
CloudRegion: cl.Regions[0],
CloudCredential: cred,
CloudCredential: &cred,
Life: state.Alive.String(),
}
m.CloudRegion.Cloud = cl
Expand Down Expand Up @@ -376,11 +376,7 @@ func TestModelFromJujuModelInfo(t *testing.T) {
Name: "test-cloud",
},
},
CloudCredential: dbmodel.CloudCredential{
Name: "test-cred",
CloudName: "test-cloud",
Owner: *i,
},
CloudCredential: nil,
OwnerIdentityName: "[email protected]",
Life: state.Alive.String(),
})
Expand Down
20 changes: 10 additions & 10 deletions internal/jimm/applicationoffer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ var initializeEnvironment = func(c *qt.C, ctx context.Context, db *db.Database,
OwnerIdentityName: u.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
}
err = db.AddModel(ctx, &model)
c.Assert(err, qt.IsNil)
Expand Down Expand Up @@ -252,7 +252,7 @@ func TestGetApplicationOfferConsumeDetails(t *testing.T) {
OwnerIdentityName: u.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
}
err = db.AddModel(ctx, &model)
c.Assert(err, qt.IsNil)
Expand Down Expand Up @@ -541,7 +541,7 @@ func TestGetApplicationOffer(t *testing.T) {
OwnerIdentityName: u.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
}
err = j.Database.AddModel(ctx, &model)
c.Assert(err, qt.IsNil)
Expand Down Expand Up @@ -750,7 +750,7 @@ func TestOffer(t *testing.T) {
OwnerIdentityName: u.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
}
err = db.AddModel(ctx, &model)
c.Assert(err, qt.IsNil)
Expand Down Expand Up @@ -840,7 +840,7 @@ func TestOffer(t *testing.T) {
OwnerIdentityName: u.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
}
err = db.AddModel(ctx, &model)
c.Assert(err, qt.IsNil)
Expand Down Expand Up @@ -960,7 +960,7 @@ func TestOffer(t *testing.T) {
OwnerIdentityName: u.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
}
err = db.AddModel(ctx, &model)
c.Assert(err, qt.IsNil)
Expand Down Expand Up @@ -1052,7 +1052,7 @@ func TestOffer(t *testing.T) {
OwnerIdentityName: u.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
}
err = db.AddModel(ctx, &model)
c.Assert(err, qt.IsNil)
Expand Down Expand Up @@ -1139,7 +1139,7 @@ func TestOffer(t *testing.T) {
OwnerIdentityName: u.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
}
err = db.AddModel(ctx, &model)
c.Assert(err, qt.IsNil)
Expand Down Expand Up @@ -1226,7 +1226,7 @@ func TestOffer(t *testing.T) {
OwnerIdentityName: u.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
}
err = db.AddModel(ctx, &model)
c.Assert(err, qt.IsNil)
Expand Down Expand Up @@ -1345,7 +1345,7 @@ func TestOfferAssertOpenFGARelationsExist(t *testing.T) {
OwnerIdentityName: u.Name,
ControllerID: controller.ID,
CloudRegionID: cloud.Regions[0].ID,
CloudCredentialID: cred.ID,
CloudCredentialID: &cred.ID,
}
err = db.AddModel(ctx, &model)
c.Assert(err, qt.IsNil)
Expand Down
Loading

0 comments on commit b03285b

Please sign in to comment.