CSS-5100 Remove SQLite (#1108)

* Use Postgres as test database Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Remove SQLite support Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Delete SQLite migration files Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Use `JIMM_TEST_PGXDSN` env var for tests Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Update godoc replacing SQLite with Postgres Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Update `JIMM_TEST_PGXDSN` env var Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Remove SQLite/dqlite errors Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Delete SQLite/dqlite tests files Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Make `postgresSuite` tests mandatory to run Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Apply `go mod tidy` to remove SQLite related modules Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Drop lower-casing schema name Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Add `CreateEmptyDatabase` helper function Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Use `jimmtest.CreateEmptyDatabase` in suite set-up Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Use double-quote to reference names with upper-case chars Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Use lower-case database/schema names Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Fix typo Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Refactor toward creating new databases from template Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Close connection after creating new database Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Add missing DSN in JIMM service params Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Add `Close` method to `Database` type Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Close database connection at tear down Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Fix error message Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Close database connection on test clean-up/tear-down Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Separate functions to create empty and templated databases Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Avoid name collision when truncating long database names Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Make unique names for template databases Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Fix typo in comment Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Add `fsync=false` option to improve db performance Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Disable 'full_page_writes' to improve testing performance Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Avoid name collision when only casing is different Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Change expected error regexp to PG equivalent Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Set `time.Now` to precision ms at service level Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Override unprovided nowFunc with ms precision to match service setup Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Limit `time.Now()` precision Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Wrap `nowFunc` to truncate returned time precision Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Limit `time.Now()` precision Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Limit `time.Now()` precision Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Remove extra blank line Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Improve error message when dropping a database fails Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Fix vault test with applying an env Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Fix test setup with creating required test clouds Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Fix checking for context cancellation Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Fix potential slice ordering issue for deep-equal assertion Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Increase test job timeout to 1h Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Compose `jimmSuite` from `JujuConnSuite` instead of `JIMMSuite` Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> * Rename 'MemoryDB` function to `PostgresDB` Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> --------- Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com> Co-authored-by: Ales Stimec <ales.stimec@canonical.com>
canonical · Jan 2, 2024 · be79a3c · be79a3c
1 parent 67090f7
commit be79a3c
Show file tree

Hide file tree

Showing 49 changed files with 469 additions and 598 deletions.
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
@@ -45,7 +45,7 @@ jobs:
       - name: Start test environment
         run: docker compose up -d
       - name: Build and Test
-        run: go test -mod readonly ./... -timeout 30m -cover
+        run: go test -mod readonly ./... -timeout 1h -cover
         env:
           JIMM_DSN: postgresql://jimm:jimm@localhost:5432/jimm
           JIMM_TEST_PGXDSN: postgresql://jimm:jimm@localhost:5432/jimm

diff --git a/cmd/jimmctl/cmd/importcloudcredentials_test.go b/cmd/jimmctl/cmd/importcloudcredentials_test.go
@@ -43,8 +43,22 @@ const creds = `{
 }`
 
 func (s *importCloudCredentialsSuite) TestImportCloudCredentials(c *gc.C) {
+	err := s.JIMM.Database.AddCloud(context.Background(), &dbmodel.Cloud{
+		Name:    "aws",
+		Type:    "kubernetes",
+		Regions: []dbmodel.CloudRegion{{Name: "default", CloudName: "test-cloud"}},
+	})
+	c.Assert(err, gc.IsNil)
+
+	err = s.JIMM.Database.AddCloud(context.Background(), &dbmodel.Cloud{
+		Name:    "gce",
+		Type:    "kubernetes",
+		Regions: []dbmodel.CloudRegion{{Name: "default", CloudName: "test-cloud"}},
+	})
+	c.Assert(err, gc.IsNil)
+
 	tmpfile := filepath.Join(c.MkDir(), "test.json")
-	err := os.WriteFile(tmpfile, []byte(creds), 0660)
+	err = os.WriteFile(tmpfile, []byte(creds), 0660)
 	c.Assert(err, gc.IsNil)
 
 	// alice is superuser

diff --git a/cmd/jimmctl/cmd/jimmsuite_test.go b/cmd/jimmctl/cmd/jimmsuite_test.go
@@ -6,17 +6,18 @@ import (
 	"bytes"
 	"context"
 	"encoding/pem"
-	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"time"
 
+	cofga "github.com/canonical/ofga"
 	"github.com/go-macaroon-bakery/macaroon-bakery/v3/bakery"
 	"github.com/go-macaroon-bakery/macaroon-bakery/v3/httpbakery"
 	"github.com/go-macaroon-bakery/macaroon-bakery/v3/httpbakery/agent"
 	"github.com/juju/juju/api"
 	"github.com/juju/juju/core/network"
+	corejujutesting "github.com/juju/juju/juju/testing"
 	jjclient "github.com/juju/juju/jujuclient"
 	jujuparams "github.com/juju/juju/rpc/params"
 	"github.com/juju/names/v4"
@@ -31,17 +32,9 @@ import (
 	ofganames "github.com/canonical/jimm/internal/openfga/names"
 )
 
-type gcTester struct {
-	*gc.C
-}
-
-func (t *gcTester) Name() string {
-	return t.C.TestName()
-}
-
 type jimmSuite struct {
 	jimmtest.CandidSuite
-	jimmtest.JujuSuite
+	corejujutesting.JujuConnSuite
 
 	Params      service.Params
 	HTTP        *httptest.Server
@@ -50,6 +43,10 @@ type jimmSuite struct {
 	ClientStore func() *jjclient.MemStore
 	JIMM        *jimm.JIMM
 	cancel      context.CancelFunc
+
+	OFGAClient  *openfga.OFGAClient
+	COFGAClient *cofga.Client
+	COFGAParams *cofga.OpenFGAParams
 }
 
 func (s *jimmSuite) SetUpTest(c *gc.C) {
@@ -74,7 +71,7 @@ func (s *jimmSuite) SetUpTest(c *gc.C) {
 		CandidURL:        s.Candid.URL.String(),
 		CandidPublicKey:  s.CandidPublicKey,
 		ControllerAdmins: []string{"admin"},
-		DSN:              fmt.Sprintf("file:%s?mode=memory&cache=shared", c.TestName()),
+		DSN:              jimmtest.CreateEmptyDatabase(&jimmtest.GocheckTester{c}),
 		OpenFGAParams: service.OpenFGAParams{
 			Scheme:    cofgaParams.Scheme,
 			Host:      cofgaParams.Host,
@@ -159,6 +156,9 @@ func (s *jimmSuite) TearDownTest(c *gc.C) {
 	if s.HTTP != nil {
 		s.HTTP.Close()
 	}
+	if err := s.JIMM.Database.Close(); err != nil {
+		c.Logf("failed to close database connections at tear down: %s", err)
+	}
 	s.CandidSuite.TearDownTest(c)
 	s.JujuConnSuite.TearDownTest(c)
 }

diff --git a/cmd/jimmctl/cmd/relation_test.go b/cmd/jimmctl/cmd/relation_test.go
@@ -8,6 +8,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"sort"
 	"strings"
 	"time"
 
@@ -232,6 +233,9 @@ func (s *relationSuite) TestRemoveRelationViaFileSuperuser(c *gc.C) {
 	c.Assert(ct, gc.Equals, "")
 	c.Logf("existing relations %v", tuples)
 	// Only two relations exist.
+	sort.Slice(tuples, func(i, j int) bool {
+		return tuples[i].Object.ID < tuples[j].Object.ID
+	})
 	c.Assert(tuples, gc.DeepEquals, []openfga.Tuple{{
 		Object:   ofganames.ConvertTag(names.NewUserTag("admin")),
 		Relation: ofganames.AdministratorRelation,

diff --git a/docker-compose.yaml b/docker-compose.yaml
@@ -62,7 +62,7 @@ services:
       JIMM_DNS_NAME: "jimm.localhost"
       JIMM_WATCH_CONTROLLERS: ""
       JIMM_LISTEN_ADDR: "0.0.0.0:80"
-      JIMM_TEST_PGXDSN: ""
+      JIMM_TEST_PGXDSN: "postgresql://jimm:jimm@db/jimm"
       JIMM_JWT_EXPIRY: 30s
       JIMM_ENABLE_JWKS_ROTATOR: "1"
       JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS: "1"
@@ -111,6 +111,9 @@ services:
       POSTGRES_DB: jimm
       POSTGRES_USER: jimm
       POSTGRES_PASSWORD: jimm
+    # Since it's mainly used for testing purposes, it's okay to set fsync=off for
+    # improved performance.
+    command: -c fsync=off -c full_page_writes=off
     healthcheck:
       test: [ "CMD-SHELL", "pg_isready -U jimm" ]
       interval: 5s

diff --git a/go.mod b/go.mod
@@ -4,7 +4,7 @@ go 1.21.3
 
 require (
 	github.com/canonical/candid v1.12.2
-	github.com/canonical/go-dqlite v1.20.0
+	github.com/canonical/go-dqlite v1.20.0 // indirect
 	github.com/canonical/go-service v1.0.0
 	github.com/frankban/quicktest v1.14.6
 	github.com/go-macaroon-bakery/macaroon-bakery/v3 v3.0.1
@@ -29,7 +29,7 @@ require (
 	github.com/juju/version v0.0.0-20210303051006-2015802527a8
 	github.com/juju/version/v2 v2.0.1
 	github.com/juju/zaputil v0.0.0-20190326175239-ef53049637ac
-	github.com/mattn/go-sqlite3 v2.0.3+incompatible
+	github.com/mattn/go-sqlite3 v2.0.3+incompatible // indirect
 	github.com/openfga/go-sdk v0.2.2
 	github.com/prometheus/client_golang v1.14.0
 	github.com/rogpeppe/fastuuid v1.2.0
@@ -40,7 +40,6 @@ require (
 	gopkg.in/macaroon-bakery.v2 v2.3.0
 	gopkg.in/macaroon.v2 v2.1.0
 	gorm.io/driver/postgres v1.0.5
-	gorm.io/driver/sqlite v1.1.4-0.20201029040614-e1caf3738eb9
 	gorm.io/gorm v1.20.6
 	sigs.k8s.io/yaml v1.3.0
 )

diff --git a/go.sum b/go.sum
@@ -2628,10 +2628,7 @@ gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
 gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
 gorm.io/driver/postgres v1.0.5 h1:raX6ezL/ciUmaYTvOq48jq1GE95aMC0CmxQYbxQ4Ufw=
 gorm.io/driver/postgres v1.0.5/go.mod h1:qrD92UurYzNctBMVCJ8C3VQEjffEuphycXtxOudXNCA=
-gorm.io/driver/sqlite v1.1.4-0.20201029040614-e1caf3738eb9 h1:Tc5uLcWg0znXtGa3auYGM7Q4Y3kM1Xby9pHkEUFXA9I=
-gorm.io/driver/sqlite v1.1.4-0.20201029040614-e1caf3738eb9/go.mod h1:UVVgiOxsQDg6fNiHcNA1fnvw4EFmq98C6c2GNcEkj78=
 gorm.io/gorm v1.20.4/go.mod h1:0HFTzE/SqkGTzK6TlDPPQbAYCluiVvhzoA1+aVyzenw=
-gorm.io/gorm v1.20.5/go.mod h1:0HFTzE/SqkGTzK6TlDPPQbAYCluiVvhzoA1+aVyzenw=
 gorm.io/gorm v1.20.6 h1:qa7tC1WcU+DBI/ZKMxvXy1FcrlGsvxlaKufHrT2qQ08=
 gorm.io/gorm v1.20.6/go.mod h1:0HFTzE/SqkGTzK6TlDPPQbAYCluiVvhzoA1+aVyzenw=
 gotest.tools v2.2.0+incompatible h1:VsBPFP1AI068pPrMxtb/S8Zkgf9xEmTLJjfM+P5UIEo=

diff --git a/internal/db/db.go b/internal/db/db.go
@@ -114,6 +114,18 @@ func (d *Database) ready() error {
 	return nil
 }
 
+// Close closes open connections to the underlying database backend.
+func (d *Database) Close() error {
+	sqlDB, err := d.DB.DB()
+	if err != nil {
+		return errors.E(err, "failed to get the internal DB object")
+	}
+	if err := sqlDB.Close(); err != nil {
+		return errors.E(err, "failed to close database connection")
+	}
+	return nil
+}
+
 // Now returns the current time as a valid sql.NullTime. The time that is
 // returned is in UTC and is truncated to milliseconds which is the
 // resolution supported on all databases.

diff --git a/internal/db/dqlite_stub_test.go b/internal/db/dqlite_stub_test.go
diff --git a/internal/db/dqlite_test.go b/internal/db/dqlite_test.go
diff --git a/internal/db/errors.go b/internal/db/errors.go
@@ -3,9 +3,7 @@
 package db
 
 import (
-	dqlitedriver "github.com/canonical/go-dqlite/driver"
 	"github.com/jackc/pgconn"
-	sqlite3 "github.com/mattn/go-sqlite3"
 	"gorm.io/gorm"
 
 	"github.com/canonical/jimm/internal/errors"
@@ -24,15 +22,6 @@ func dbError(err error) error {
 		code = errors.CodeNotFound
 	}
 	switch e := err.(type) {
-	case sqlite3.Error:
-		if e.ExtendedCode == sqlite3.ErrConstraintUnique {
-			code = errors.CodeAlreadyExists
-		}
-		if e.Code == sqlite3.ErrLocked {
-			code = errors.CodeDatabaseLocked
-		}
-	case dqlitedriver.Error:
-		// TODO(mhilton) work out how to decode dqlite errors.
 	case *pgconn.PgError:
 		if e.Code == pgUniqueViolation {
 			code = errors.CodeAlreadyExists

diff --git a/internal/db/pgx_test.go b/internal/db/pgx_test.go
@@ -34,11 +34,8 @@ type postgresSuite struct {
 }
 
 func (s *postgresSuite) Init(c *qt.C) {
-	dsn := os.Getenv("JIMM_TEST_PGXDSN")
-	if dsn == "" {
-		c.Skip("postgresql not configured")
-		return
-	}
+	dsn, exists := os.LookupEnv("JIMM_TEST_PGXDSN")
+	c.Assert(exists, qt.IsTrue, qt.Commentf("env var JIMM_TEST_PGXDSN is not assigned"))
 
 	connCfg, err := pgx.ParseConfig(dsn)
 	c.Assert(err, qt.IsNil)

diff --git a/internal/db/sqlite_test.go b/internal/db/sqlite_test.go
diff --git a/internal/db/usermodeldefaults_test.go b/internal/db/usermodeldefaults_test.go
@@ -110,7 +110,7 @@ func TestSetUserModelDefaults(t *testing.T) {
 			return testConfig{
 				user:          &user,
 				defaults:      defaults,
-				expectedError: `FOREIGN KEY constraint failed`,
+				expectedError: `.*violates foreign key constraint.*`,
 			}
 		},
 	}, {
@@ -139,7 +139,7 @@ func TestSetUserModelDefaults(t *testing.T) {
 		c.Run(test.about, func(c *qt.C) {
 			j := &jimm.JIMM{
 				Database: db.Database{
-					DB: jimmtest.MemoryDB(c, func() time.Time { return now }),
+					DB: jimmtest.PostgresDB(c, func() time.Time { return now }),
 				},
 			}
 			err := j.Database.Migrate(ctx, true)

diff --git a/internal/dbmodel/audit_test.go b/internal/dbmodel/audit_test.go
@@ -23,7 +23,7 @@ func TestAuditLogEntry(t *testing.T) {
 	c.Assert(err, qt.IsNil)
 
 	ale := dbmodel.AuditLogEntry{
-		Time:           time.Now(),
+		Time:           time.Now().Truncate(time.Second),
 		ConversationId: "1234",
 		MessageId:      9876,
 		FacadeName:     "JIMM",
@@ -50,7 +50,7 @@ func TestToAPIAuditEvent(t *testing.T) {
 	c.Assert(err, qt.IsNil)
 
 	ale := dbmodel.AuditLogEntry{
-		Time:           time.Now(),
+		Time:           time.Now().Truncate(time.Second),
 		ConversationId: "1234",
 		MessageId:      9876,
 		FacadeName:     "JIMM",

diff --git a/internal/dbmodel/cloud_test.go b/internal/dbmodel/cloud_test.go
@@ -73,7 +73,7 @@ func TestCloud(t *testing.T) {
 		Name: "test-cloud",
 	}
 	result = db.Create(&cl4)
-	c.Check(result.Error, qt.ErrorMatches, `UNIQUE constraint failed: clouds.name`)
+	c.Check(result.Error, qt.ErrorMatches, `.*violates unique constraint "clouds_name_key".*`)
 }
 
 func TestToJujuCloud(t *testing.T) {
@@ -412,7 +412,7 @@ func TestReuseDeletedCloudName(t *testing.T) {
 		Name: "test-cloud",
 	}
 	result = db.Create(&cl2)
-	c.Check(result.Error, qt.ErrorMatches, `UNIQUE constraint failed: clouds.name`)
+	c.Check(result.Error, qt.ErrorMatches, `.*violates unique constraint "clouds_name_key".*`)
 
 	result = db.Delete(&cl1)
 	c.Assert(result.Error, qt.IsNil)