diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 868ad4c95..7bd734e10 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -25,6 +25,7 @@ jobs: build_test: name: Build and Test runs-on: ubuntu-22.04 + timeout-minutes: 45 steps: - uses: actions/checkout@v4 with: diff --git a/internal/db/controller_test.go b/internal/db/controller_test.go index f5f06a02b..99bdc5eb9 100644 --- a/internal/db/controller_test.go +++ b/internal/db/controller_test.go @@ -4,14 +4,11 @@ package db_test import ( "context" - "database/sql" "testing" - "time" qt "github.com/frankban/quicktest" "github.com/google/go-cmp/cmp/cmpopts" jujuparams "github.com/juju/juju/rpc/params" - "github.com/juju/juju/state" "github.com/canonical/jimm/internal/db" "github.com/canonical/jimm/internal/dbmodel" @@ -72,7 +69,6 @@ func (s *dbSuite) TestGetController(c *qt.C) { controller := dbmodel.Controller{ Name: "test-controller", UUID: "00000000-0000-0000-0000-0000-0000000000001", - Models: []dbmodel.Model{}, CloudName: "test-cloud", } err = s.Database.AddController(context.Background(), &controller) @@ -102,105 +98,6 @@ func (s *dbSuite) TestGetController(c *qt.C) { c.Assert(eError.Code, qt.Equals, errors.CodeNotFound) } -func (s *dbSuite) TestGetControllerWithModels(c *qt.C) { - err := s.Database.Migrate(context.Background(), true) - c.Assert(err, qt.Equals, nil) - - cloud := dbmodel.Cloud{ - Name: "test-cloud", - Type: "test-provider", - Regions: []dbmodel.CloudRegion{{ - Name: "test-region", - }}, - } - c.Assert(s.Database.DB.Create(&cloud).Error, qt.IsNil) - - controller := dbmodel.Controller{ - Name: "test-controller", - UUID: "00000000-0000-0000-0000-0000-0000000000001", - Models: []dbmodel.Model{}, - CloudName: "test-cloud", - CloudRegion: "test-region", - } - u, err := dbmodel.NewIdentity("bob@canonical.com") - c.Assert(err, qt.IsNil) - c.Assert(s.Database.DB.Create(&u).Error, qt.IsNil) - - cred := dbmodel.CloudCredential{ - Name: "test-cred", - Cloud: cloud, - Owner: *u, - AuthType: "empty", - } - c.Assert(s.Database.DB.Create(&cred).Error, qt.IsNil) - - err = s.Database.AddController(context.Background(), &controller) - c.Assert(err, qt.Equals, nil) - - models := []dbmodel.Model{{ - Name: "test-model-1", - UUID: sql.NullString{ - String: "00000001-0000-0000-0000-0000-000000000001", - Valid: true, - }, - Owner: *u, - Controller: controller, - CloudRegion: cloud.Regions[0], - CloudCredential: cred, - Type: "iaas", - IsController: true, - DefaultSeries: "warty", - Life: state.Alive.String(), - Status: dbmodel.Status{ - Status: "available", - Since: sql.NullTime{ - Time: time.Now(), - Valid: true, - }, - }, - SLA: dbmodel.SLA{ - Level: "unsupported", - }, - }, { - Name: "test-model-2", - UUID: sql.NullString{ - String: "00000001-0000-0000-0000-0000-000000000002", - Valid: true, - }, - Owner: *u, - Controller: controller, - CloudRegion: cloud.Regions[0], - CloudCredential: cred, - Type: "iaas", - IsController: false, - DefaultSeries: "warty", - Life: state.Alive.String(), - Status: dbmodel.Status{ - Status: "available", - Since: sql.NullTime{ - Time: time.Now(), - Valid: true, - }, - }, - SLA: dbmodel.SLA{ - Level: "unsupported", - }, - }} - for _, m := range models { - c.Assert(s.Database.DB.Create(&m).Error, qt.IsNil) - } - - dbController := dbmodel.Controller{ - UUID: controller.UUID, - } - err = s.Database.GetController(context.Background(), &dbController) - c.Assert(err, qt.Equals, nil) - controller.Models = []dbmodel.Model{ - models[0], - } - c.Assert(dbController, qt.CmpEquals(cmpopts.IgnoreFields(dbmodel.Controller{}, "Models"), cmpopts.EquateEmpty()), controller) -} - func TestForEachControllerUnconfiguredDatabase(t *testing.T) { c := qt.New(t) @@ -289,7 +186,6 @@ func (s *dbSuite) TestUpdateController(c *qt.C) { UUID: "00000000-0000-0000-0000-0000-0000000000001", CloudName: "test-cloud", CloudRegion: "test-region", - Models: []dbmodel.Model{}, } err = s.Database.AddController(context.Background(), &controller) c.Assert(err, qt.Equals, nil) @@ -336,7 +232,6 @@ func (s *dbSuite) TestDeleteController(c *qt.C) { Name: "test-controller", UUID: "00000000-0000-0000-0000-0000-0000000000001", CloudName: "test-cloud", - Models: []dbmodel.Model{}, } err = s.Database.AddController(context.Background(), &controller) c.Assert(err, qt.Equals, nil) diff --git a/internal/db/model.go b/internal/db/model.go index 68dcd5bab..3ed5a5ccc 100644 --- a/internal/db/model.go +++ b/internal/db/model.go @@ -181,3 +181,19 @@ func preloadModel(prefix string, db *gorm.DB) *gorm.DB { return db } + +// GetModelsByController retrieves a list of models hosted on the specified controller. +// Note that because we do not preload here, foreign key references will be empty. +func (d *Database) GetModelsByController(ctx context.Context, ctl dbmodel.Controller) ([]dbmodel.Model, error) { + const op = errors.Op("db.GetModelsByController") + + if err := d.ready(); err != nil { + return nil, errors.E(op, err) + } + var models []dbmodel.Model + db := d.DB.WithContext(ctx) + if err := db.Model(ctl).Association("Models").Find(&models); err != nil { + return nil, errors.E(op, dbError(err)) + } + return models, nil +} diff --git a/internal/db/model_test.go b/internal/db/model_test.go index b45878b6b..1f4848cca 100644 --- a/internal/db/model_test.go +++ b/internal/db/model_test.go @@ -7,6 +7,7 @@ import ( "database/sql" "sort" "testing" + "time" qt "github.com/frankban/quicktest" "github.com/juju/juju/state" @@ -57,7 +58,6 @@ func (s *dbSuite) TestAddModel(c *qt.C) { UUID: "00000000-0000-0000-0000-0000-0000000000001", CloudName: "test-cloud", CloudRegion: "test-region", - Models: []dbmodel.Model{}, } err = s.Database.AddController(context.Background(), &controller) c.Assert(err, qt.Equals, nil) @@ -127,7 +127,6 @@ func (s *dbSuite) TestGetModel(c *qt.C) { controller := dbmodel.Controller{ Name: "test-controller", UUID: "00000000-0000-0000-0000-0000-0000000000001", - Models: []dbmodel.Model{}, CloudName: "test-cloud", CloudRegion: "test-region", } @@ -231,7 +230,6 @@ func (s *dbSuite) TestUpdateModel(c *qt.C) { UUID: "00000000-0000-0000-0000-0000-0000000000001", CloudName: "test-cloud", CloudRegion: "test-region", - Models: []dbmodel.Model{}, } err = s.Database.AddController(context.Background(), &controller) c.Assert(err, qt.Equals, nil) @@ -308,7 +306,6 @@ func (s *dbSuite) TestDeleteModel(c *qt.C) { UUID: "00000000-0000-0000-0000-0000-0000000000001", CloudName: "test-cloud", CloudRegion: "test-region", - Models: []dbmodel.Model{}, } err = s.Database.AddController(context.Background(), &controller) c.Assert(err, qt.Equals, nil) @@ -641,3 +638,102 @@ func (s *dbSuite) TestGetModelsByUUID(c *qt.C) { c.Check(models[2].UUID.String, qt.Equals, "00000002-0000-0000-0000-000000000003") c.Check(models[2].Controller.Name, qt.Not(qt.Equals), "") } + +func (s *dbSuite) TestGetModelsByController(c *qt.C) { + err := s.Database.Migrate(context.Background(), true) + c.Assert(err, qt.Equals, nil) + + cloud := dbmodel.Cloud{ + Name: "test-cloud", + Type: "test-provider", + Regions: []dbmodel.CloudRegion{{ + Name: "test-region", + }}, + } + c.Assert(s.Database.DB.Create(&cloud).Error, qt.IsNil) + + controller := dbmodel.Controller{ + Name: "test-controller", + UUID: "00000000-0000-0000-0000-0000-0000000000001", + CloudName: "test-cloud", + CloudRegion: "test-region", + } + u, err := dbmodel.NewIdentity("bob@canonical.com") + c.Assert(err, qt.IsNil) + c.Assert(s.Database.DB.Create(&u).Error, qt.IsNil) + + cred := dbmodel.CloudCredential{ + Name: "test-cred", + Cloud: cloud, + Owner: *u, + AuthType: "empty", + } + c.Assert(s.Database.DB.Create(&cred).Error, qt.IsNil) + + err = s.Database.AddController(context.Background(), &controller) + c.Assert(err, qt.Equals, nil) + + models := []dbmodel.Model{{ + Name: "test-model-1", + UUID: sql.NullString{ + String: "00000001-0000-0000-0000-0000-000000000001", + Valid: true, + }, + Owner: *u, + Controller: controller, + CloudRegion: cloud.Regions[0], + CloudCredential: cred, + Type: "iaas", + IsController: true, + DefaultSeries: "focal", + Life: state.Alive.String(), + Status: dbmodel.Status{ + Status: "available", + Since: sql.NullTime{ + Time: time.Now(), + Valid: true, + }, + }, + SLA: dbmodel.SLA{ + Level: "unsupported", + }, + }, { + Name: "test-model-2", + UUID: sql.NullString{ + String: "00000001-0000-0000-0000-0000-000000000002", + Valid: true, + }, + Owner: *u, + Controller: controller, + CloudRegion: cloud.Regions[0], + CloudCredential: cred, + Type: "iaas", + IsController: false, + DefaultSeries: "focal", + Life: state.Alive.String(), + Status: dbmodel.Status{ + Status: "available", + Since: sql.NullTime{ + Time: time.Now(), + Valid: true, + }, + }, + SLA: dbmodel.SLA{ + Level: "unsupported", + }, + }} + for _, m := range models { + c.Assert(s.Database.DB.Create(&m).Error, qt.IsNil) + } + foundModels, err := s.Database.GetModelsByController(context.Background(), controller) + foundModelNames := []string{} + for _, m := range foundModels { + foundModelNames = append(foundModelNames, m.Name) + } + modelNames := []string{} + for _, m := range models { + modelNames = append(modelNames, m.Name) + } + c.Assert(err, qt.IsNil) + c.Assert(foundModelNames, qt.DeepEquals, modelNames) +} diff --git a/internal/dbmodel/cloud_test.go b/internal/dbmodel/cloud_test.go index 7d1351ec3..3f0d174bf 100644 --- a/internal/dbmodel/cloud_test.go +++ b/internal/dbmodel/cloud_test.go @@ -353,13 +353,17 @@ func TestCloudRegionControllers(t *testing.T) { c.Assert(err, qt.IsNil) c.Check(crcps, qt.HasLen, 2) c.Check(crcps[0].Controller, qt.DeepEquals, dbmodel.Controller{ - Model: ctl2.Model, + ID: ctl2.ID, + CreatedAt: ctl2.CreatedAt, + UpdatedAt: ctl2.UpdatedAt, Name: ctl2.Name, CloudName: ctl2.CloudName, CloudRegion: ctl2.CloudRegion, }) c.Check(crcps[1].Controller, qt.DeepEquals, dbmodel.Controller{ - Model: ctl1.Model, + ID: ctl1.ID, + CreatedAt: ctl1.CreatedAt, + UpdatedAt: ctl1.UpdatedAt, Name: ctl1.Name, CloudName: ctl1.CloudName, CloudRegion: ctl1.CloudRegion, @@ -370,13 +374,17 @@ func TestCloudRegionControllers(t *testing.T) { c.Assert(err, qt.IsNil) c.Check(crcps, qt.HasLen, 2) c.Check(crcps[0].Controller, qt.DeepEquals, dbmodel.Controller{ - Model: ctl1.Model, + ID: ctl1.ID, + CreatedAt: ctl1.CreatedAt, + UpdatedAt: ctl1.UpdatedAt, Name: ctl1.Name, CloudName: ctl1.CloudName, CloudRegion: ctl1.CloudRegion, }) c.Check(crcps[1].Controller, qt.DeepEquals, dbmodel.Controller{ - Model: ctl2.Model, + ID: ctl2.ID, + CreatedAt: ctl2.CreatedAt, + UpdatedAt: ctl2.UpdatedAt, Name: ctl2.Name, CloudName: ctl2.CloudName, CloudRegion: ctl2.CloudRegion, diff --git a/internal/dbmodel/controller.go b/internal/dbmodel/controller.go index 96e148151..d88963ff4 100644 --- a/internal/dbmodel/controller.go +++ b/internal/dbmodel/controller.go @@ -6,6 +6,7 @@ import ( "database/sql" "net" "strconv" + "time" jujuparams "github.com/juju/juju/rpc/params" "github.com/juju/names/v5" @@ -17,7 +18,10 @@ import ( // A controller represents a juju controller which is hosting models // within the JAAS system. type Controller struct { - gorm.Model + // Note that we do not use gorm.Model to avoid the use of soft-deletes. + ID uint `gorm:"primarykey"` + CreatedAt time.Time + UpdatedAt time.Time // Name is the name given to this controller. Name string `gorm:"not null;uniqueIndex"` diff --git a/internal/dbmodel/sql/postgres/1_9.sql b/internal/dbmodel/sql/postgres/1_9.sql new file mode 100644 index 000000000..d43ad6591 --- /dev/null +++ b/internal/dbmodel/sql/postgres/1_9.sql @@ -0,0 +1,6 @@ +-- 1_9.sql is a migration that deletes soft-deleted controllers and +-- drops the deleted_at column from the controllers table. +DELETE FROM controllers WHERE deleted_at IS NOT null; +ALTER TABLE controllers DROP COLUMN deleted_at; + +UPDATE versions SET major=1, minor=9 WHERE component='jimmdb'; diff --git a/internal/dbmodel/version.go b/internal/dbmodel/version.go index dc772ead4..1649904f4 100644 --- a/internal/dbmodel/version.go +++ b/internal/dbmodel/version.go @@ -20,7 +20,7 @@ const ( // Minor is the minor version of the model described in the dbmodel // package. It should be incremented for any change made to the // database model from database model in a released JIMM. - Minor = 8 + Minor = 9 ) type Version struct { diff --git a/internal/jimm/jimm.go b/internal/jimm/jimm.go index a52096f0d..211978dce 100644 --- a/internal/jimm/jimm.go +++ b/internal/jimm/jimm.go @@ -527,8 +527,12 @@ func (j *JIMM) RemoveController(ctx context.Context, user *openfga.User, control return errors.E(errors.CodeStillAlive, "controller is still alive") } + models, err := db.GetModelsByController(ctx, c) + if err != nil { + return err + } // Delete its models first. - for _, model := range c.Models { + for _, model := range models { err := db.DeleteModel(ctx, &model) if err != nil { return err diff --git a/internal/jimm/jimm_test.go b/internal/jimm/jimm_test.go index f2d447ea6..3d6223f7b 100644 --- a/internal/jimm/jimm_test.go +++ b/internal/jimm/jimm_test.go @@ -468,7 +468,6 @@ func TestRemoveController(t *testing.T) { env := jimmtest.ParseEnvironment(c, removeControllerTestEnv) env.PopulateDB(c, j.Database) - // env.PopulateDBAndPermissions(c, j.ResourceTag(), j.Database, client) dbUser := env.User(test.user).DBObject(c, j.Database) user := openfga.NewUser(&dbUser, nil) @@ -500,6 +499,92 @@ func TestRemoveController(t *testing.T) { } } +const removeAndAddControllerTestEnv = `clouds: +- name: test-cloud + type: test-provider + regions: + - name: test-cloud-region +cloud-credentials: +- owner: alice@canonical.com + name: cred-1 + cloud: test-cloud +users: +- username: alice@canonical.com + controller-access: superuser +- username: bob@canonical.com + controller-access: login +- username: eve@canonical.com + controller-access: "no-access" +controllers: +- name: controller-1 + uuid: 00000001-0000-0000-0000-000000000001 + cloud: test-cloud + region: test-cloud-region +models: +- name: model-1 + type: iaas + uuid: 00000002-0000-0000-0000-000000000001 + controller: controller-1 + default-series: warty + cloud: test-cloud + region: test-cloud-region + cloud-credential: cred-1 + owner: alice@canonical.com + life: alive + status: + status: available + info: "OK!" + since: 2020-02-20T20:02:20Z + users: + - user: alice@canonical.com + access: admin + - user: bob@canonical.com + access: write + - user: charlie@canonical.com + access: read + sla: + level: unsupported + agent-version: 1.2.3 +` + +func TestRemoveAndAddController(t *testing.T) { + c := qt.New(t) + ctx := context.Background() + now := time.Now().UTC().Round(time.Millisecond) + + j := &jimm.JIMM{ + UUID: uuid.NewString(), + Database: db.Database{ + DB: jimmtest.PostgresDB(c, func() time.Time { return now }), + }, + } + + err := j.Database.Migrate(ctx, false) + c.Assert(err, qt.IsNil) + + env := jimmtest.ParseEnvironment(c, removeAndAddControllerTestEnv) + env.PopulateDB(c, j.Database) + controller := env.Controllers[0] + + dbUser := env.User("alice@canonical.com").DBObject(c, j.Database) + user := openfga.NewUser(&dbUser, nil) + user.JimmAdmin = true + + err = j.RemoveController(ctx, user, "controller-1", true) + c.Assert(err, qt.Equals, nil) + ctls, err := j.ListControllers(ctx, user) + c.Assert(err, qt.Equals, nil) + c.Assert(len(ctls), qt.Equals, 0) + // Recreate the controller. + ctlDbObject := controller.DBObject(c, j.Database) + ctlDbObject.ID = 0 + err = j.Database.AddController(ctx, &ctlDbObject) + c.Assert(err, qt.Equals, nil) + ctls, err = j.ListControllers(ctx, user) + c.Assert(err, qt.Equals, nil) + c.Assert(len(ctls), qt.Equals, 1) +} + const fullModelStatusTestEnv = `clouds: - name: test-cloud type: test-provider diff --git a/internal/jimm/watcher.go b/internal/jimm/watcher.go index be6f6d403..3ba56ae2b 100644 --- a/internal/jimm/watcher.go +++ b/internal/jimm/watcher.go @@ -309,6 +309,7 @@ func (w *Watcher) watchController(ctx context.Context, ctl *dbmodel.Controller) continue } if v.changed { + v.changed = false // Update changed model. err := w.Database.Transaction(func(tx *db.Database) error { m := dbmodel.Model{ diff --git a/internal/jimmtest/cmp.go b/internal/jimmtest/cmp.go index 404bef2fb..6bdbe1554 100644 --- a/internal/jimmtest/cmp.go +++ b/internal/jimmtest/cmp.go @@ -45,7 +45,7 @@ var DBObjectEquals = qt.CmpEquals( cmpopts.IgnoreFields(dbmodel.CloudCredential{}, "CloudName", "OwnerIdentityName"), cmpopts.IgnoreFields(dbmodel.CloudRegion{}, "CloudName"), cmpopts.IgnoreFields(dbmodel.CloudRegionControllerPriority{}, "CloudRegionID", "ControllerID"), - cmpopts.IgnoreFields(dbmodel.Controller{}, "ID"), + cmpopts.IgnoreFields(dbmodel.Controller{}, "ID", "UpdatedAt", "CreatedAt"), cmpopts.IgnoreFields(dbmodel.Model{}, "ID", "CreatedAt", "UpdatedAt", "OwnerIdentityName", "ControllerID", "CloudRegionID", "CloudCredentialID"), ) diff --git a/internal/jujuapi/jimm_test.go b/internal/jujuapi/jimm_test.go index ed10b0a1c..c626af755 100644 --- a/internal/jujuapi/jimm_test.go +++ b/internal/jujuapi/jimm_test.go @@ -186,6 +186,32 @@ func (s *jimmSuite) TestAddController(c *gc.C) { c.Assert(jujuparams.IsBadRequest(err), gc.Equals, true) } +func (s *jimmSuite) TestRemoveAndAddController(c *gc.C) { + conn := s.open(c, nil, "alice") + defer conn.Close() + client := api.NewClient(conn) + + info := s.APIInfo(c) + + acr := apiparams.AddControllerRequest{ + UUID: info.ControllerUUID, + Name: "controller-2", + APIAddresses: info.Addrs, + CACertificate: info.CACert, + Username: info.Tag.Id(), + Password: info.Password, + } + + ci, err := client.AddController(&acr) + c.Assert(err, gc.Equals, nil) + _, err = client.RemoveController(&apiparams.RemoveControllerRequest{Name: acr.Name, Force: true}) + c.Assert(err, gc.Equals, nil) + ciNew, err := client.AddController(&acr) + c.Assert(err, gc.Equals, nil) + c.Assert(ci, gc.DeepEquals, ciNew) + +} + func (s *jimmSuite) TestAddControllerCustomTLSHostname(c *gc.C) { conn := s.open(c, nil, "alice") defer conn.Close()