diff --git a/pkg/storage/unified/sql/db/dbimpl/dbEngine.go b/pkg/storage/unified/sql/db/dbimpl/dbEngine.go index d068a658481..3968cfc7839 100644 --- a/pkg/storage/unified/sql/db/dbimpl/dbEngine.go +++ b/pkg/storage/unified/sql/db/dbimpl/dbEngine.go @@ -7,23 +7,26 @@ import ( "time" "github.com/go-sql-driver/mysql" + "xorm.io/xorm" + "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/storage/unified/sql/db" - "xorm.io/xorm" ) -func getEngineMySQL(getter *sectionGetter, tracer tracing.Tracer) (*xorm.Engine, error) { +func getEngineMySQL(getter confGetter, tracer tracing.Tracer) (*xorm.Engine, error) { config := mysql.NewConfig() - config.User = getter.String("db_user") - config.Passwd = getter.String("db_pass") + config.User = getter.String("user") + // accept the core Grafana jargon of `password` as well, originally Unified + // Storage used `pass` + config.Passwd = cmp.Or(getter.String("pass"), getter.String("password")) config.Net = "tcp" - config.Addr = getter.String("db_host") - config.DBName = getter.String("db_name") + config.Addr = getter.String("host") + config.DBName = getter.String("name") config.Params = map[string]string{ // See: https://dev.mysql.com/doc/refman/en/sql-mode.html "@@SESSION.sql_mode": "ANSI", } - tls := getter.String("db_tls") + tls := getter.String("tls") if tls != "" { config.Params["tls"] = tls } @@ -39,8 +42,8 @@ func getEngineMySQL(getter *sectionGetter, tracer tracing.Tracer) (*xorm.Engine, //config.MultiStatements = true // TODO: do we want to support these? - // config.ServerPubKey = getter.String("db_server_pub_key") - // config.TLSConfig = getter.String("db_tls_config_name") + // config.ServerPubKey = getter.String("server_pub_key") + // config.TLSConfig = getter.String("tls_config_name") if err := getter.Err(); err != nil { return nil, fmt.Errorf("config error: %w", err) @@ -65,12 +68,14 @@ func getEngineMySQL(getter *sectionGetter, tracer tracing.Tracer) (*xorm.Engine, return engine, nil } -func getEnginePostgres(getter *sectionGetter, tracer tracing.Tracer) (*xorm.Engine, error) { +func getEnginePostgres(getter confGetter, tracer tracing.Tracer) (*xorm.Engine, error) { dsnKV := map[string]string{ - "user": getter.String("db_user"), - "password": getter.String("db_pass"), - "dbname": getter.String("db_name"), - "sslmode": cmp.Or(getter.String("db_sslmode"), "disable"), + "user": getter.String("user"), + // accept the core Grafana jargon of `password` as well, originally + // Unified Storage used `pass` + "password": cmp.Or(getter.String("pass"), getter.String("password")), + "dbname": getter.String("name"), + "sslmode": cmp.Or(getter.String("sslmode"), "disable"), } // TODO: probably interesting: @@ -88,7 +93,7 @@ func getEnginePostgres(getter *sectionGetter, tracer tracing.Tracer) (*xorm.Engi // More on Postgres connection string parameters: // https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING - hostport := getter.String("db_host") + hostport := getter.String("host") if err := getter.Err(); err != nil { return nil, fmt.Errorf("config error: %w", err) @@ -96,7 +101,7 @@ func getEnginePostgres(getter *sectionGetter, tracer tracing.Tracer) (*xorm.Engi host, port, err := splitHostPortDefault(hostport, "127.0.0.1", "5432") if err != nil { - return nil, fmt.Errorf("invalid db_host: %w", err) + return nil, fmt.Errorf("invalid host: %w", err) } dsnKV["host"] = host dsnKV["port"] = port diff --git a/pkg/storage/unified/sql/db/dbimpl/dbEngine_test.go b/pkg/storage/unified/sql/db/dbimpl/dbEngine_test.go index ab761b98744..659d3bd1bb6 100644 --- a/pkg/storage/unified/sql/db/dbimpl/dbEngine_test.go +++ b/pkg/storage/unified/sql/db/dbimpl/dbEngine_test.go @@ -6,22 +6,33 @@ import ( "github.com/stretchr/testify/assert" ) -func newValidMySQLGetter() *sectionGetter { - return newTestSectionGetter(map[string]string{ - "db_type": dbTypeMySQL, - "db_host": "/var/run/mysql.socket", - "db_name": "grafana", - "db_user": "user", - "db_password": "password", - }) +func newValidMySQLGetter(withKeyPrefix bool) confGetter { + var prefix string + if withKeyPrefix { + prefix = "db_" + } + return newTestConfGetter(map[string]string{ + prefix + "type": dbTypeMySQL, + prefix + "host": "/var/run/mysql.socket", + prefix + "name": "grafana", + prefix + "user": "user", + prefix + "password": "password", + }, prefix) } func TestGetEngineMySQLFromConfig(t *testing.T) { t.Parallel() - t.Run("happy path", func(t *testing.T) { + t.Run("happy path - with key prefix", func(t *testing.T) { t.Parallel() - engine, err := getEngineMySQL(newValidMySQLGetter(), nil) + engine, err := getEngineMySQL(newValidMySQLGetter(true), nil) + assert.NotNil(t, engine) + assert.NoError(t, err) + }) + + t.Run("happy path - without key prefix", func(t *testing.T) { + t.Parallel() + engine, err := getEngineMySQL(newValidMySQLGetter(false), nil) assert.NotNil(t, engine) assert.NoError(t, err) }) @@ -29,13 +40,13 @@ func TestGetEngineMySQLFromConfig(t *testing.T) { t.Run("invalid string", func(t *testing.T) { t.Parallel() - getter := newTestSectionGetter(map[string]string{ + getter := newTestConfGetter(map[string]string{ "db_type": dbTypeMySQL, "db_host": "/var/run/mysql.socket", "db_name": string(invalidUTF8ByteSequence), "db_user": "user", "db_password": "password", - }) + }, "db_") engine, err := getEngineMySQL(getter, nil) assert.Nil(t, engine) assert.Error(t, err) @@ -43,35 +54,46 @@ func TestGetEngineMySQLFromConfig(t *testing.T) { }) } -func newValidPostgresGetter() *sectionGetter { - return newTestSectionGetter(map[string]string{ - "db_type": dbTypePostgres, - "db_host": "localhost", - "db_name": "grafana", - "db_user": "user", - "db_password": "password", - }) +func newValidPostgresGetter(withKeyPrefix bool) confGetter { + var prefix string + if withKeyPrefix { + prefix = "db_" + } + return newTestConfGetter(map[string]string{ + prefix + "type": dbTypePostgres, + prefix + "host": "localhost", + prefix + "name": "grafana", + prefix + "user": "user", + prefix + "password": "password", + }, prefix) } func TestGetEnginePostgresFromConfig(t *testing.T) { t.Parallel() - t.Run("happy path", func(t *testing.T) { + t.Run("happy path - with key prefix", func(t *testing.T) { t.Parallel() - engine, err := getEnginePostgres(newValidPostgresGetter(), nil) + engine, err := getEnginePostgres(newValidPostgresGetter(true), nil) + assert.NotNil(t, engine) + assert.NoError(t, err) + }) + + t.Run("happy path - without key prefix", func(t *testing.T) { + t.Parallel() + engine, err := getEnginePostgres(newValidPostgresGetter(false), nil) assert.NotNil(t, engine) assert.NoError(t, err) }) t.Run("invalid string", func(t *testing.T) { t.Parallel() - getter := newTestSectionGetter(map[string]string{ + getter := newTestConfGetter(map[string]string{ "db_type": dbTypePostgres, "db_host": string(invalidUTF8ByteSequence), "db_name": "grafana", "db_user": "user", "db_password": "password", - }) + }, "db_") engine, err := getEnginePostgres(getter, nil) assert.Nil(t, engine) @@ -81,13 +103,13 @@ func TestGetEnginePostgresFromConfig(t *testing.T) { t.Run("invalid hostport", func(t *testing.T) { t.Parallel() - getter := newTestSectionGetter(map[string]string{ + getter := newTestConfGetter(map[string]string{ "db_type": dbTypePostgres, "db_host": "1:1:1", "db_name": "grafana", "db_user": "user", "db_password": "password", - }) + }, "db_") engine, err := getEnginePostgres(getter, nil) assert.Nil(t, engine) diff --git a/pkg/storage/unified/sql/db/dbimpl/dbimpl.go b/pkg/storage/unified/sql/db/dbimpl/dbimpl.go index b9498b87cd5..4756a1f4d56 100644 --- a/pkg/storage/unified/sql/db/dbimpl/dbimpl.go +++ b/pkg/storage/unified/sql/db/dbimpl/dbimpl.go @@ -2,6 +2,7 @@ package dbimpl import ( "context" + "errors" "fmt" "sync" @@ -12,7 +13,6 @@ import ( infraDB "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" - "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/storage/unified/sql/db" "github.com/grafana/grafana/pkg/storage/unified/sql/db/migrations" @@ -23,8 +23,17 @@ const ( dbTypePostgres = "postgres" ) -func ProvideResourceDB(grafanaDB infraDB.DB, cfg *setting.Cfg, features featuremgmt.FeatureToggles, tracer tracing.Tracer) (db.DBProvider, error) { - p, err := newResourceDBProvider(grafanaDB, cfg, features, tracer) +const grafanaDBInstrumentQueriesKey = "instrument_queries" + +var errGrafanaDBInstrumentedNotSupported = errors.New("the Resource API is " + + "attempting to leverage the database from core Grafana defined in the" + + " [database] INI section since a database configuration was not provided" + + " in the [resource_api] section. But we detected that the key `" + + grafanaDBInstrumentQueriesKey + "` is enabled in [database], and that" + + " setup is currently unsupported. Please, consider disabling that flag") + +func ProvideResourceDB(grafanaDB infraDB.DB, cfg *setting.Cfg, tracer tracing.Tracer) (db.DBProvider, error) { + p, err := newResourceDBProvider(grafanaDB, cfg, tracer) if err != nil { return nil, fmt.Errorf("provide Resource DB: %w", err) } @@ -54,41 +63,67 @@ type resourceDBProvider struct { logQueries bool } -func newResourceDBProvider(grafanaDB infraDB.DB, cfg *setting.Cfg, features featuremgmt.FeatureToggles, tracer tracing.Tracer) (p *resourceDBProvider, err error) { - // TODO: This should be renamed resource_api - getter := §ionGetter{ - DynamicSection: cfg.SectionWithEnvOverrides("resource_api"), - } +func newResourceDBProvider(grafanaDB infraDB.DB, cfg *setting.Cfg, tracer tracing.Tracer) (p *resourceDBProvider, err error) { + // Resource API has other configs in its section besides database ones, so + // we prefix them with "db_". We use the database config from core Grafana + // as fallback, and as it uses a dedicated INI section, then keys are not + // prefixed with "db_" + getter := newConfGetter(cfg.SectionWithEnvOverrides("resource_api"), "db_") + fallbackGetter := newConfGetter(cfg.SectionWithEnvOverrides("database"), "") p = &resourceDBProvider{ cfg: cfg, log: log.New("entity-db"), - logQueries: getter.Key("log_queries").MustBool(false), + logQueries: getter.Bool("log_queries"), migrateFunc: migrations.MigrateResourceStore, } - switch dbType := getter.Key("db_type").MustString(""); dbType { - case dbTypePostgres: + dbType := getter.String("type") + grafanaDBType := fallbackGetter.String("type") + switch { + // First try with the config in the "resource_api" section, which is + // specific to Unified Storage + case dbType == dbTypePostgres: p.registerMetrics = true p.engine, err = getEnginePostgres(getter, tracer) return p, err - case dbTypeMySQL: + case dbType == dbTypeMySQL: p.registerMetrics = true p.engine, err = getEngineMySQL(getter, tracer) return p, err - case "": + // TODO: add support for SQLite + + case dbType != "": + return p, fmt.Errorf("invalid db type specified: %s", dbType) + + // If we have an empty Resource API db config, try with the core Grafana + // database config + + case grafanaDBType == dbTypePostgres: + p.registerMetrics = true + p.engine, err = getEnginePostgres(fallbackGetter, tracer) + return p, err + + case grafanaDBType == dbTypeMySQL: + p.registerMetrics = true + p.engine, err = getEngineMySQL(fallbackGetter, tracer) + return p, err + + // TODO: add support for SQLite + + case grafanaDB != nil: // try to use the grafana db connection - if grafanaDB == nil { - return p, fmt.Errorf("no db connection provided") + + if fallbackGetter.Bool(grafanaDBInstrumentQueriesKey) { + return nil, errGrafanaDBInstrumentedNotSupported } p.engine = grafanaDB.GetEngine() return p, nil default: - // TODO: sqlite support - return p, fmt.Errorf("invalid db type specified: %s", dbType) + return p, fmt.Errorf("no db connection provided") } } @@ -102,7 +137,6 @@ func (p *resourceDBProvider) init(ctx context.Context) (db.DB, error) { _ = p.logQueries // TODO: configure SQL logging // TODO: change the migrator to use db.DB instead of xorm - // Skip migrations if feature flag is not enabled if p.migrateFunc != nil { err := p.migrateFunc(ctx, p.engine, p.cfg) if err != nil { diff --git a/pkg/storage/unified/sql/db/dbimpl/dbimpl_test.go b/pkg/storage/unified/sql/db/dbimpl/dbimpl_test.go new file mode 100644 index 00000000000..bb317509888 --- /dev/null +++ b/pkg/storage/unified/sql/db/dbimpl/dbimpl_test.go @@ -0,0 +1,159 @@ +package dbimpl + +import ( + "context" + "database/sql" + "net/http" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/trace" + traceNoop "go.opentelemetry.io/otel/trace/noop" + ini "gopkg.in/ini.v1" + + "github.com/grafana/grafana/pkg/bus" + infraDB "github.com/grafana/grafana/pkg/infra/db" + "github.com/grafana/grafana/pkg/infra/tracing" + "github.com/grafana/grafana/pkg/services/sqlstore" + "github.com/grafana/grafana/pkg/setting" +) + +type ( + // cfgSectionMap represents an INI section, mapping from an INI key to an + // INI value. + cfgSectionMap = map[string]string + // cfgMap is a map from INI section name to INI section contents. + cfgMap = map[string]cfgSectionMap +) + +// setupDBForGrafana modifies `m` in the following way: +// +// [database] +// type = sqlite3 +// path = unique-random-path +// +// After that, it initializes a temporary SQLite filesystem-backed database that +// is later deleted when the test finishes. +func setupDBForGrafana(t *testing.T, ctx context.Context, m cfgMap) { + dbSection, ok := m["database"] + if !ok { + dbSection = cfgSectionMap{} + m["database"] = dbSection + } + dbSection["type"] = "sqlite3" + dbSection["path"] = t.TempDir() + "/" + uuid.New().String() + + db, err := sql.Open("sqlite3", "file:"+dbSection["path"]) + require.NoError(t, err) + + _, err = db.ExecContext(ctx, ` + CREATE TABLE user ( + id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, + version INTEGER NOT NULL, + login TEXT NOT NULL, + email TEXT NOT NULL, + name TEXT NULL, + password TEXT NULL, + salt TEXT NULL, + rands TEXT NULL, + company TEXT NULL, + org_id INTEGER NOT NULL, + is_admin INTEGER NOT NULL, + email_verified INTEGER NULL, + theme TEXT NULL, + created DATETIME NOT NULL, + updated DATETIME NOT NULL, + help_flags1 INTEGER NOT NULL DEFAULT 0, + last_seen_at DATETIME NULL, + is_disabled INTEGER NOT NULL DEFAULT 0, + is_service_account BOOLEAN DEFAULT 0, + uid TEXT NULL + ); + CREATE TABLE org ( + id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, + version INTEGER NOT NULL, + name TEXT NOT NULL, + address1 TEXT NULL, + address2 TEXT NULL, + city TEXT NULL, + state TEXT NULL, + zip_code TEXT NULL, + country TEXT NULL, + billing_email TEXT NULL, + created DATETIME NOT NULL, + updated DATETIME NOT NULL + ); + CREATE TABLE org_user ( + id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, + org_id INTEGER NOT NULL, + user_id INTEGER NOT NULL, + role TEXT NOT NULL, + created DATETIME NOT NULL, + updated DATETIME NOT NULL + ); + `) + require.NoError(t, err) +} + +func newTestInfraDB(t *testing.T, m cfgMap) infraDB.DB { + t.Helper() + // nil migrations means no migrations + sqlstoreDB, err := sqlstore.ProvideService( + newCfgFromIniMap(t, m), // *setting.Cfg + featureTogglesNop{}, // featuremgmt.FeatureToggles + nil, // registry.DatabaseMigrator + nopBus{}, // github.com/grafana/grafana/pkg/bus.Bus + newNopTestGrafanaTracer(), + ) + require.NoError(t, err) + + return sqlstoreDB +} + +func newCfgFromIniMap(t *testing.T, m cfgMap) *setting.Cfg { + t.Helper() + cfg, err := setting.NewCfgFromINIFile(newTestINIFile(t, m)) + require.NoError(t, err) + return cfg +} + +func newTestINIFile(t *testing.T, m cfgMap) *ini.File { + t.Helper() + f := ini.Empty() + for sectionName, kvs := range m { + section, err := f.NewSection(sectionName) + require.NoError(t, err) + for k, v := range kvs { + _, err := section.NewKey(k, v) + require.NoError(t, err) + } + } + return f +} + +type ( + testGrafanaTracer struct { + trace.Tracer + } + featureTogglesNop struct{} + nopBus struct{} +) + +func (testGrafanaTracer) Inject(context.Context, http.Header, trace.Span) {} +func newNopTestGrafanaTracer() tracing.Tracer { + return testGrafanaTracer{traceNoop.NewTracerProvider().Tracer("test")} +} + +func (featureTogglesNop) IsEnabled(context.Context, string) bool { + return false +} +func (featureTogglesNop) IsEnabledGlobally(string) bool { + return false +} +func (featureTogglesNop) GetEnabled(context.Context) map[string]bool { + return map[string]bool{} +} + +func (nopBus) Publish(context.Context, bus.Msg) error { return nil } +func (nopBus) AddEventListener(bus.HandlerFunc) {} diff --git a/pkg/storage/unified/sql/db/dbimpl/regression_incident_2144_test.go b/pkg/storage/unified/sql/db/dbimpl/regression_incident_2144_test.go new file mode 100644 index 00000000000..1d0b6ba9b02 --- /dev/null +++ b/pkg/storage/unified/sql/db/dbimpl/regression_incident_2144_test.go @@ -0,0 +1,204 @@ +package dbimpl + +import ( + "context" + "database/sql" + "database/sql/driver" + "sync" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/util/testutil" +) + +// defined in the standard library in database/sql/ctxutil.go +const noIsolationLevelSupportErrStr = "sql: driver does not support non-" + + "default isolation level" + +var _ driver.Driver = driverWithoutIsolationLevel{} +var _ driver.Driver = driverWithIsolationLevel{} + +const ( + driverWithoutIsolationLevelName = "test driver without isolation levels" + driverWithIsolationLevelName = "test driver with isolation levels" +) + +var registerTestDriversOnce sync.Once + +func registerTestSQLDrivers() { + registerTestDriversOnce.Do(func() { + sql.Register(driverWithoutIsolationLevelName, driverWithoutIsolationLevel{}) + sql.Register(driverWithIsolationLevelName, driverWithIsolationLevel{}) + }) +} + +type ( + // without isolation level + + driverWithoutIsolationLevel struct{} + connWithoutIsolationLevel struct{} + + // with isolation level + + driverWithIsolationLevel struct{} + connWithIsolationLevel struct { + connWithoutIsolationLevel + } + + // common + + testStmt struct{} + testTx struct{} + testResults struct{} + testRows struct{} +) + +// driver.Driver + +func (driverWithoutIsolationLevel) Open(name string) (driver.Conn, error) { + return connWithoutIsolationLevel{}, nil +} + +func (driverWithIsolationLevel) Open(name string) (driver.Conn, error) { + return connWithIsolationLevel{}, nil +} + +// driver.Conn + +func (connWithoutIsolationLevel) Prepare(query string) (driver.Stmt, error) { + return testStmt{}, nil +} +func (connWithoutIsolationLevel) Close() error { + return nil +} +func (connWithoutIsolationLevel) Begin() (driver.Tx, error) { + return testTx{}, nil +} + +func (connWithIsolationLevel) BeginTx(context.Context, driver.TxOptions) (driver.Tx, error) { + return testTx{}, nil +} + +// driver.Stmt + +func (testStmt) Close() error { return nil } +func (testStmt) NumInput() int { return 0 } +func (testStmt) Exec(args []driver.Value) (driver.Result, error) { return testResults{}, nil } +func (testStmt) Query(args []driver.Value) (driver.Rows, error) { return testRows{}, nil } + +// driver.Tx + +func (testTx) Commit() error { return nil } +func (testTx) Rollback() error { return nil } + +// driver.Results + +func (testResults) LastInsertId() (int64, error) { return 1, nil } +func (testResults) RowsAffected() (int64, error) { return 1, nil } + +// driver.Rows + +func (testRows) Columns() []string { return nil } +func (testRows) Close() error { return nil } +func (testRows) Next(dest []driver.Value) error { return nil } + +func TestReproIncident2144IndependentOfGrafanaDB(t *testing.T) { + t.Parallel() + registerTestSQLDrivers() + txOpts := &sql.TxOptions{ + Isolation: sql.LevelSerializable, + } + + t.Run("driver without isolation level should fail", func(t *testing.T) { + t.Parallel() + ctx := testutil.NewDefaultTestContext(t) + + db, err := sql.Open(driverWithoutIsolationLevelName, "") + require.NoError(t, err) + require.NotNil(t, db) + + _, err = db.BeginTx(ctx, txOpts) + require.Error(t, err) + require.Equal(t, noIsolationLevelSupportErrStr, err.Error()) + }) + + t.Run("driver with isolation level should work", func(t *testing.T) { + t.Parallel() + ctx := testutil.NewDefaultTestContext(t) + + db, err := sql.Open(driverWithIsolationLevelName, "") + require.NoError(t, err) + require.NotNil(t, db) + + _, err = db.BeginTx(ctx, txOpts) + require.NoError(t, err) + }) +} + +func TestReproIncident2144UsingGrafanaDB(t *testing.T) { + t.Parallel() + txOpts := &sql.TxOptions{ + Isolation: sql.LevelSerializable, + } + + t.Run("core Grafana db without instrumentation preserves driver ability to use isolation levels", + func(t *testing.T) { + t.Parallel() + + t.Run("base behaviour is preserved", func(t *testing.T) { + t.Parallel() + ctx := testutil.NewDefaultTestContext(t) + cfgMap := cfgMap{} + setupDBForGrafana(t, ctx, cfgMap) + grafanaDB := newTestInfraDB(t, cfgMap) + db := grafanaDB.GetEngine().DB().DB + _, err := db.BeginTx(ctx, txOpts) + require.NoError(t, err) + }) + + t.Run("Resource API does not fail and correctly uses Grafana DB as fallback", + func(t *testing.T) { + t.Parallel() + ctx := testutil.NewDefaultTestContext(t) + cfgMap := cfgMap{} + cfg := newCfgFromIniMap(t, cfgMap) + setupDBForGrafana(t, ctx, cfgMap) + grafanaDB := newTestInfraDB(t, cfgMap) + resourceDB, err := ProvideResourceDB(grafanaDB, cfg, testGrafanaTracer{}) + require.NotNil(t, resourceDB) + require.NoError(t, err) + }) + }) + + t.Run("core Grafana db instrumentation removes driver ability to use isolation levels", + func(t *testing.T) { + t.Parallel() + ctx := testutil.NewDefaultTestContext(t) + cfgMap := cfgMap{ + "database": cfgSectionMap{ + grafanaDBInstrumentQueriesKey: "true", + }, + } + setupDBForGrafana(t, ctx, cfgMap) + grafanaDB := newTestInfraDB(t, cfgMap) + + t.Run("base failure caused by instrumentation", func(t *testing.T) { + t.Parallel() + ctx := testutil.NewDefaultTestContext(t) + db := grafanaDB.GetEngine().DB().DB + _, err := db.BeginTx(ctx, txOpts) + require.Error(t, err) + require.Equal(t, noIsolationLevelSupportErrStr, err.Error()) + }) + + t.Run("Resource API provides a reasonable error for this case", func(t *testing.T) { + t.Parallel() + cfg := newCfgFromIniMap(t, cfgMap) + resourceDB, err := ProvideResourceDB(grafanaDB, cfg, testGrafanaTracer{}) + require.Nil(t, resourceDB) + require.Error(t, err) + require.ErrorIs(t, err, errGrafanaDBInstrumentedNotSupported) + }) + }) +} diff --git a/pkg/storage/unified/sql/db/dbimpl/util.go b/pkg/storage/unified/sql/db/dbimpl/util.go index 4065121fe56..da142be7d04 100644 --- a/pkg/storage/unified/sql/db/dbimpl/util.go +++ b/pkg/storage/unified/sql/db/dbimpl/util.go @@ -14,17 +14,35 @@ import ( var errInvalidUTF8Sequence = errors.New("invalid UTF-8 sequence") +type confGetter interface { + Err() error + Bool(key string) bool + String(key string) string +} + +func newConfGetter(ds *setting.DynamicSection, keyPrefix string) confGetter { + return §ionGetter{ + ds: ds, + keyPrefix: keyPrefix, + } +} + type sectionGetter struct { - *setting.DynamicSection - err error + ds *setting.DynamicSection + keyPrefix string + err error } func (g *sectionGetter) Err() error { return g.err } +func (g *sectionGetter) Bool(key string) bool { + return g.ds.Key(g.keyPrefix + key).MustBool(false) +} + func (g *sectionGetter) String(key string) string { - v := g.DynamicSection.Key(key).MustString("") + v := g.ds.Key(g.keyPrefix + key).MustString("") if !utf8.ValidString(v) { g.err = fmt.Errorf("value for key %q: %w", key, errInvalidUTF8Sequence) diff --git a/pkg/storage/unified/sql/db/dbimpl/util_test.go b/pkg/storage/unified/sql/db/dbimpl/util_test.go index 8364d0f653f..9fff4209e3e 100644 --- a/pkg/storage/unified/sql/db/dbimpl/util_test.go +++ b/pkg/storage/unified/sql/db/dbimpl/util_test.go @@ -17,35 +17,75 @@ func setSectionKeyValues(section *setting.DynamicSection, m map[string]string) { } } -func newTestSectionGetter(m map[string]string) *sectionGetter { +func newTestConfGetter(m map[string]string, keyPrefix string) confGetter { section := setting.NewCfg().SectionWithEnvOverrides("entity_api") setSectionKeyValues(section, m) - return §ionGetter{ - DynamicSection: section, - } + return newConfGetter(section, keyPrefix) } func TestSectionGetter(t *testing.T) { t.Parallel() var ( - key = "the key" - val = string(invalidUTF8ByteSequence) + key = "the key" + keyBoolTrue = "I'm true" + keyBoolFalse = "not me!" + prefix = "this is some prefix" + val = string(invalidUTF8ByteSequence) ) - g := newTestSectionGetter(map[string]string{ - key: val, + t.Run("with prefix", func(t *testing.T) { + t.Parallel() + + g := newTestConfGetter(map[string]string{ + prefix + key: val, + prefix + keyBoolTrue: "YES", + prefix + keyBoolFalse: "0", + }, prefix) + + require.False(t, g.Bool("whatever bool")) + require.NoError(t, g.Err()) + + require.False(t, g.Bool(keyBoolFalse)) + require.NoError(t, g.Err()) + + require.True(t, g.Bool(keyBoolTrue)) + require.NoError(t, g.Err()) + + require.Empty(t, g.String("whatever string")) + require.NoError(t, g.Err()) + + require.Empty(t, g.String(key)) + require.Error(t, g.Err()) + require.ErrorIs(t, g.Err(), errInvalidUTF8Sequence) }) - v := g.String("whatever") - require.Empty(t, v) - require.NoError(t, g.Err()) + t.Run("without prefix", func(t *testing.T) { + t.Parallel() - v = g.String(key) - require.Empty(t, v) - require.Error(t, g.Err()) - require.ErrorIs(t, g.Err(), errInvalidUTF8Sequence) + g := newTestConfGetter(map[string]string{ + key: val, + keyBoolTrue: "true", + keyBoolFalse: "f", + }, "") + + require.False(t, g.Bool("whatever bool")) + require.NoError(t, g.Err()) + + require.False(t, g.Bool(keyBoolFalse)) + require.NoError(t, g.Err()) + + require.True(t, g.Bool(keyBoolTrue)) + require.NoError(t, g.Err()) + + require.Empty(t, g.String("whatever string")) + require.NoError(t, g.Err()) + + require.Empty(t, g.String(key)) + require.Error(t, g.Err()) + require.ErrorIs(t, g.Err(), errInvalidUTF8Sequence) + }) } func TestMakeDSN(t *testing.T) { diff --git a/pkg/storage/unified/sql/server.go b/pkg/storage/unified/sql/server.go index 58770875bc3..7fcef520fc1 100644 --- a/pkg/storage/unified/sql/server.go +++ b/pkg/storage/unified/sql/server.go @@ -15,7 +15,7 @@ func NewResourceServer(db infraDB.DB, cfg *setting.Cfg, features featuremgmt.Fea Tracer: tracer, } - eDB, err := dbimpl.ProvideResourceDB(db, cfg, features, tracer) + eDB, err := dbimpl.ProvideResourceDB(db, cfg, tracer) if err != nil { return nil, err } diff --git a/pkg/storage/unified/sql/test/integration_test.go b/pkg/storage/unified/sql/test/integration_test.go index dcc655867d6..6b54d131f95 100644 --- a/pkg/storage/unified/sql/test/integration_test.go +++ b/pkg/storage/unified/sql/test/integration_test.go @@ -31,9 +31,8 @@ func newServer(t *testing.T) (sql.Backend, resource.ResourceServer) { dbstore := infraDB.InitTestDB(t) cfg := setting.NewCfg() - features := featuremgmt.WithFeatures() - eDB, err := dbimpl.ProvideResourceDB(dbstore, cfg, features, nil) + eDB, err := dbimpl.ProvideResourceDB(dbstore, cfg, nil) require.NoError(t, err) require.NotNil(t, eDB)