feat: tweak remove-controller logic (#1893)

* feat: tweak remove-controller logic

* test: update integration test

Author: kian99

docs/howto/manage-juju-controllers.md

@@ -177,6 +177,9 @@ These options include:
 1. Use the `jaas` plugin to first unregister the controller then use the `juju` CLI to destroy it.
 2. Use the `jaas` plugin destroy the controller via an API call to JIMM, allowing for greater automation.
 
+Removing a controller can only be done once the controller is not hosting
+any models known to JIMM. Migrate or destroy these models before destroying the controller.
+
 ````{dropdown} Unregister and destroy
 
 Switch to the JIMM controller and unregister your controller from JIMM:
@@ -196,8 +199,6 @@ juju destroy-controller mycontroller
 ````{dropdown} JIMM destroy
 Switch to the JIMM controller and destroy the controller.
 
-Note that this command will return an error if the controller is still hosting
-any models. Migrate or destroy these models before destroying the controller.
 ```text
 juju switch jimm
 juju jaas destroy-controller mycontroller

internal/jimm/juju/jimm.go

@@ -130,18 +130,16 @@ func (j *JujuManager) RemoveController(ctx context.Context, user *openfga.User,
 			return err
 		}
 
-		// if c.UnavailableSince is valid, then we can delete is
-		// if c.UnavailableSince is no valid, then we can't delete is
-		// if force is true, we can always delete is
-		if !force && !c.UnavailableSince.Valid {
-			return errors.E(errors.CodeStillAlive, "controller is still alive")
-		}
-
 		models, err := db.GetModelsByController(ctx, c)
 		if err != nil {
 			return err
 		}
-		// Delete its models first.
+		if len(models) > 0 && !force {
+			return errors.E(errors.CodeStillAlive, "controller still has models")
+		}
+
+		// Remove all models associated with the controller. If force is false,
+		// we can only reach here with an empty list of models.
 		for _, model := range models {
 			err := db.DeleteModel(ctx, &model)
 			if err != nil {

internal/jimm/juju/jimm_test.go

@@ -231,7 +231,7 @@ func TestSetControllerDeprecated(t *testing.T) {
 	}
 }
 
-const removeControllerTestEnv = `clouds:
+const removeControllerWithModelsTestEnv = `clouds:
 - name: test-cloud
   type: test-provider
   regions:
@@ -270,54 +270,64 @@ models:
     access: read
 `
 
+const removeControllerWithoutModelsTestEnv = `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
+controllers:
+- name: controller-1
+  uuid: 00000001-0000-0000-0000-000000000001
+  cloud: test-cloud
+  region: test-cloud-region
+`
+
 func TestRemoveController(t *testing.T) {
 	c := qt.New(t)
 
 	ctx := context.Background()
 
 	tests := []struct {
-		about            string
-		user             string
-		unavailableSince *time.Time
-		force            bool
-		expectedError    string
+		about         string
+		user          string
+		force         bool
+		env           string
+		expectedError string
 	}{{
-		about:            "remove an unavailable controller",
-		user:             "alice@canonical.com",
-		unavailableSince: &now,
-		force:            true,
+		about: "remove a controller without models",
+		user:  "alice@canonical.com",
+		force: true,
+		env:   removeControllerWithoutModelsTestEnv,
 	}, {
-		about: "remove a live controller with force",
+		about: "remove a controller with models with force",
 		user:  "alice@canonical.com",
 		force: true,
+		env:   removeControllerWithModelsTestEnv,
 	}, {
-		about:         "error when removing a live controller",
+		about:         "error when removing a controller with models",
 		user:          "alice@canonical.com",
 		force:         false,
-		expectedError: "controller is still alive",
+		expectedError: "controller still has models",
+		env:           removeControllerWithModelsTestEnv,
 	}}
 
 	for _, test := range tests {
 		c.Run(test.about, func(c *qt.C) {
 			j := newTestJujuManager(c, nil)
 
-			env := jimmtest.ParseEnvironment(c, removeControllerTestEnv)
+			env := jimmtest.ParseEnvironment(c, test.env)
 			env.PopulateDB(c, j.Database)
 
 			dbUser := env.User(test.user).DBObject(c, j.Database)
 			user := openfga.NewUser(&dbUser, nil)
 
-			if test.unavailableSince != nil {
-				// make the controller unavailable
-				controller := env.Controller("controller-1").DBObject(c, j.Database)
-				controller.UnavailableSince = sql.NullTime{
-					Valid: true,
-					Time:  *test.unavailableSince,
-				}
-				err := j.Database.UpdateController(ctx, &controller)
-				c.Assert(err, qt.Equals, nil)
-			}
-
 			err := j.RemoveController(ctx, user, "controller-1", test.force)
 			if test.expectedError != "" {
 				c.Assert(err, qt.ErrorMatches, test.expectedError)

testing/jimm_test.go

@@ -361,6 +361,7 @@ func TestAddControllerCustomTLSHostname(t *testing.T) {
 func TestRemoveController(t *testing.T) {
 	c := qt.New(t)
 	s := jimmtest.SetupJimmWithControllers(c)
+	model := s.CreateModelForBob(c)
 
 	conn := s.Open(c, nil, "alice", nil)
 	defer conn.Close()
@@ -371,8 +372,9 @@ func TestRemoveController(t *testing.T) {
 	_, err := client.RemoveController(&apiparams.RemoveControllerRequest{
 		Name: name,
 	})
-	c.Check(err, qt.ErrorMatches, `controller is still alive \(still alive\)`)
+	c.Check(err, qt.ErrorMatches, `controller still has models.*`)
 	c.Check(jujuparams.ErrCode(err), qt.Equals, apiparams.CodeStillAlive)
+	s.DestroyModelAndDeleteFromDatabase(c, model.ResourceTag())
 
 	conn2 := s.Open(c, nil, "bob", nil)
 	defer conn2.Close()
@@ -385,8 +387,7 @@ func TestRemoveController(t *testing.T) {
 	c.Check(jujuparams.ErrCode(err), qt.Equals, jujuparams.CodeUnauthorized)
 
 	ci, err := client.RemoveController(&apiparams.RemoveControllerRequest{
-		Name:  name,
-		Force: true,
+		Name: name,
 	})
 	c.Assert(err, qt.Equals, nil)
 	ciExpected := apiparams.ControllerInfo{