From 041c343a86dedf8e0bcf0cde6867babcc14cef88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20=C5=A0tibran=C3=BD?= Date: Thu, 3 Jul 2025 11:57:40 +0200 Subject: [PATCH] Unified storage: Respect GF_DATABASE_URL override (#105331) * Database for unified storage resources now reuses DB code that respects URL override. Access instrument_queries via section getter. --- .../unified/sql/db/dbimpl/db_engine.go | 27 ++----- .../unified/sql/db/dbimpl/db_engine_test.go | 73 +++++++++++++++++-- pkg/storage/unified/sql/db/dbimpl/dbimpl.go | 28 ++++--- 3 files changed, 91 insertions(+), 37 deletions(-) diff --git a/pkg/storage/unified/sql/db/dbimpl/db_engine.go b/pkg/storage/unified/sql/db/dbimpl/db_engine.go index ccc994ec715..9712de24f02 100644 --- a/pkg/storage/unified/sql/db/dbimpl/db_engine.go +++ b/pkg/storage/unified/sql/db/dbimpl/db_engine.go @@ -12,7 +12,6 @@ import ( "github.com/grafana/grafana/pkg/util/xorm" "github.com/grafana/grafana/pkg/services/sqlstore" - "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/storage/unified/sql/db" ) @@ -20,33 +19,21 @@ import ( // driver. const tlsConfigName = "db_engine_tls" -func getEngine(cfg *setting.Cfg) (*xorm.Engine, error) { - dbSection := cfg.SectionWithEnvOverrides("database") - dbType := dbSection.Key("type").String() - if dbType == "" { - return nil, fmt.Errorf("no database type specified") - } - - switch dbType { +func getEngine(config *sqlstore.DatabaseConfig) (*xorm.Engine, error) { + switch config.Type { case dbTypeMySQL, dbTypePostgres, dbTypeSQLite: - config, err := sqlstore.NewDatabaseConfig(cfg, nil) - if err != nil { - return nil, nil - } - - engine, err := xorm.NewEngine(dbType, config.ConnectionString) + engine, err := xorm.NewEngine(config.Type, config.ConnectionString) if err != nil { return nil, fmt.Errorf("open database: %w", err) } - engine.SetMaxOpenConns(dbSection.Key("max_open_conn").MustInt(0)) - engine.SetMaxIdleConns(dbSection.Key("max_idle_conn").MustInt(4)) - maxLifetime := time.Duration(dbSection.Key("conn_max_lifetime").MustInt(14400)) * time.Second - engine.SetConnMaxLifetime(maxLifetime) + engine.SetMaxOpenConns(config.MaxOpenConn) + engine.SetMaxIdleConns(config.MaxIdleConn) + engine.SetConnMaxLifetime(time.Duration(config.ConnMaxLifetime) * time.Second) return engine, nil default: - return nil, fmt.Errorf("unsupported database type: %s", dbType) + return nil, fmt.Errorf("unsupported database type: %s", config.Type) } } diff --git a/pkg/storage/unified/sql/db/dbimpl/db_engine_test.go b/pkg/storage/unified/sql/db/dbimpl/db_engine_test.go index d85260b161a..fbaa147070a 100644 --- a/pkg/storage/unified/sql/db/dbimpl/db_engine_test.go +++ b/pkg/storage/unified/sql/db/dbimpl/db_engine_test.go @@ -9,11 +9,14 @@ import ( "math/big" "os" "path/filepath" + "strings" "testing" "time" - "github.com/grafana/grafana/pkg/setting" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/setting" ) func newValidMySQLGetter(withKeyPrefix bool) confGetter { @@ -30,7 +33,7 @@ func newValidMySQLGetter(withKeyPrefix bool) confGetter { }, prefix) } -func TestGetEngine(t *testing.T) { +func TestNewResourceDbProvider(t *testing.T) { t.Parallel() t.Run("MySQL engine", func(t *testing.T) { @@ -43,9 +46,10 @@ func TestGetEngine(t *testing.T) { dbSection.Key("user").SetValue("user") dbSection.Key("password").SetValue("password") - engine, err := getEngine(cfg) + engine, err := newResourceDBProvider(nil, cfg, nil) require.NoError(t, err) require.NotNil(t, engine) + require.Equal(t, dbTypeMySQL, engine.engine.Dialect().DriverName()) }) t.Run("Postgres engine", func(t *testing.T) { @@ -58,9 +62,10 @@ func TestGetEngine(t *testing.T) { dbSection.Key("user").SetValue("user") dbSection.Key("password").SetValue("password") - engine, err := getEngine(cfg) + engine, err := newResourceDBProvider(nil, cfg, nil) require.NoError(t, err) require.NotNil(t, engine) + require.Equal(t, dbTypePostgres, engine.engine.Dialect().DriverName()) }) t.Run("SQLite engine", func(t *testing.T) { @@ -70,9 +75,20 @@ func TestGetEngine(t *testing.T) { dbSection.Key("type").SetValue(dbTypeSQLite) dbSection.Key("path").SetValue(":memory:") - engine, err := getEngine(cfg) + engine, err := newResourceDBProvider(nil, cfg, nil) require.NoError(t, err) require.NotNil(t, engine) + require.Equal(t, dbTypeSQLite, engine.engine.Dialect().DriverName()) + }) + + t.Run("No database type", func(t *testing.T) { + t.Parallel() + cfg := setting.NewCfg() + + engine, err := newResourceDBProvider(nil, cfg, nil) + require.Error(t, err) + require.Nil(t, engine) + require.Contains(t, err.Error(), "unknown") }) t.Run("Unknown database type", func(t *testing.T) { @@ -81,13 +97,56 @@ func TestGetEngine(t *testing.T) { dbSection := cfg.SectionWithEnvOverrides("database") dbSection.Key("type").SetValue("unknown") - engine, err := getEngine(cfg) + engine, err := newResourceDBProvider(nil, cfg, nil) require.Error(t, err) require.Nil(t, engine) - require.Contains(t, err.Error(), "unsupported database type") + require.Contains(t, err.Error(), "unknown") }) } +func TestDatabaseConfigOverridenByEnvVariable(t *testing.T) { + prevEnv := os.Environ() + t.Cleanup(func() { + // Revert env variables to state before this test. + os.Clearenv() + for _, e := range prevEnv { + sp := strings.SplitN(e, "=", 2) + if len(sp) == 2 { + assert.NoError(t, os.Setenv(sp[0], sp[1])) + } + } + }) + + tmpDir := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, "conf"), 0750)) + // We need to include database.url in defaults, otherwise it won't be overridden by environment variable! + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "conf/defaults.ini"), []byte("[log.console]\nlevel =\n[database]\nurl = \n"), 0644)) + + dbConfig := ` +[database] +type = postgres +host = localhost +name = grafana +user = user +password = password +` + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "conf/custom.ini"), []byte(dbConfig), 0644)) + + // Override database URL + require.NoError(t, os.Setenv("GF_DATABASE_URL", "mysql://gf:pwd@overthere:3306/grafana")) + + cfg := setting.NewCfg() + require.NoError(t, cfg.Load(setting.CommandLineArgs{HomePath: tmpDir})) + + engine, err := newResourceDBProvider(nil, cfg, nil) + require.NoError(t, err) + require.NotNil(t, engine) + // Verify that GF_DATABASE_URL value is used. + require.Equal(t, dbTypeMySQL, engine.engine.Dialect().DriverName()) + require.Contains(t, engine.engine.DataSourceName(), "overthere:3306") +} + func TestGetEngineMySQLFromConfig(t *testing.T) { t.Parallel() diff --git a/pkg/storage/unified/sql/db/dbimpl/dbimpl.go b/pkg/storage/unified/sql/db/dbimpl/dbimpl.go index 4db1ca56a85..0109d994afd 100644 --- a/pkg/storage/unified/sql/db/dbimpl/dbimpl.go +++ b/pkg/storage/unified/sql/db/dbimpl/dbimpl.go @@ -11,13 +11,15 @@ import ( "go.opentelemetry.io/otel/trace" "go.opentelemetry.io/otel/trace/noop" + "github.com/grafana/grafana/pkg/services/sqlstore" + "github.com/grafana/grafana/pkg/util/xorm" + infraDB "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" "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" "github.com/grafana/grafana/pkg/storage/unified/sql/db/otel" - "github.com/grafana/grafana/pkg/util/xorm" ) const ( @@ -31,8 +33,8 @@ 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" + + " in the [resource_api] section. But we detected that the key" + + " `instrument_queries` is enabled in [database], and that" + " setup is currently unsupported. Please, consider disabling that flag") func ProvideResourceDB(grafanaDB infraDB.DB, cfg *setting.Cfg, tracer trace.Tracer) (db.DBProvider, error) { @@ -66,7 +68,11 @@ func newResourceDBProvider(grafanaDB infraDB.DB, cfg *setting.Cfg, tracer trace. // 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"), "") + fallbackConfig, fallbackErr := sqlstore.NewDatabaseConfig(cfg, nil) + if fallbackErr != nil { + // Ignore error here and keep going. + fallbackConfig = nil + } logger := log.New("entity-db") p = &resourceDBProvider{ @@ -78,7 +84,6 @@ func newResourceDBProvider(grafanaDB infraDB.DB, cfg *setting.Cfg, tracer trace. } dbType := getter.String("type") - grafanaDBType := fallbackGetter.String("type") switch { // Deprecated: First try with the config in the "resource_api" section, which is specific to Unified Storage case dbType == dbTypePostgres: @@ -97,20 +102,23 @@ func newResourceDBProvider(grafanaDB infraDB.DB, cfg *setting.Cfg, tracer trace. 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 != "": - logger.Info("Using database section", "db_type", grafanaDBType) + case fallbackConfig != nil && fallbackConfig.Type != "": + logger.Info("Using database section", "db_type", fallbackConfig.Type) p.registerMetrics = true - p.engine, err = getEngine(cfg) + p.engine, err = getEngine(fallbackConfig) return p, err case grafanaDB != nil: // try to use the grafana db connection (should only happen in tests) - if fallbackGetter.Bool(grafanaDBInstrumentQueriesKey) { + if newConfGetter(cfg.SectionWithEnvOverrides("database"), "").Bool(grafanaDBInstrumentQueriesKey) { return nil, errGrafanaDBInstrumentedNotSupported } p.engine = grafanaDB.GetEngine() return p, nil default: - return p, fmt.Errorf("no database type specified") + if fallbackErr != nil { + return nil, fallbackErr + } + return nil, fmt.Errorf("no database type specified") } }