From f13ea3597567f45d7b082a667d9967b65e7a3ff6 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Mon, 27 May 2024 16:57:48 +0200 Subject: [PATCH 1/2] Rename `setGaleraOpts` and make it testable --- database/db.go | 8 +++++++- database/utils.go | 22 +++++++++++----------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/database/db.go b/database/db.go index d024e42..99661cc 100644 --- a/database/db.go +++ b/database/db.go @@ -141,7 +141,13 @@ func NewDbFromConfig(c *Config, logger *logging.Logger, connectorCallbacks Retry } } - return setGaleraOpts(ctx, conn, int64(c.Options.WsrepSyncWait)) + // Set the "wsrep_sync_wait" variable for each session and ensures that causality checks are performed + // before execution and that each statement is executed on a fully synchronized node. Doing so prevents + // foreign key violation when inserting into dependent tables on different MariaDB/MySQL nodes. When using + // MySQL single nodes, the "SET SESSION" command will fail with "Unknown system variable (1193)" and will + // therefore be silently dropped. + // https://mariadb.com/kb/en/galera-cluster-system-variables/#wsrep_sync_wait + return unsafeSetSessionVariableIfExists(ctx, conn, "wsrep_sync_wait", fmt.Sprint(c.Options.WsrepSyncWait)) } addr = config.Addr diff --git a/database/utils.go b/database/utils.go index 55c9264..067b156 100644 --- a/database/utils.go +++ b/database/utils.go @@ -3,12 +3,12 @@ package database import ( "context" "database/sql/driver" + "fmt" "github.com/go-sql-driver/mysql" "github.com/icinga/icinga-go-library/com" "github.com/icinga/icinga-go-library/strcase" "github.com/icinga/icinga-go-library/types" "github.com/pkg/errors" - "strconv" ) // CantPerformQuery wraps the given error with the specified query that cannot be executed. @@ -44,22 +44,22 @@ func SplitOnDupId[T IDer]() com.BulkChunkSplitPolicy[T] { } } -// setGaleraOpts sets the "wsrep_sync_wait" variable for each session ensures that causality checks are performed -// before execution and that each statement is executed on a fully synchronized node. Doing so prevents foreign key -// violation when inserting into dependent tables on different MariaDB/MySQL nodes. When using MySQL single nodes, -// the "SET SESSION" command will fail with "Unknown system variable (1193)" and will therefore be silently dropped. +// unsafeSetSessionVariableIfExists sets the given MySQL/MariaDB system variable for the specified database session. // -// https://mariadb.com/kb/en/galera-cluster-system-variables/#wsrep_sync_wait -func setGaleraOpts(ctx context.Context, conn driver.Conn, wsrepSyncWait int64) error { - galeraOpts := "SET SESSION wsrep_sync_wait=" + strconv.FormatInt(wsrepSyncWait, 10) +// NOTE: It is unsafe to use this function with untrusted/user supplied inputs and poses an SQL injection, +// because it doesn't use a prepared statement, but executes the SQL command directly with the provided inputs. +// +// When the "SET SESSION" command fails with "Unknown system variable (1193)", the error will be silently +// dropped but returns all other database errors. +func unsafeSetSessionVariableIfExists(ctx context.Context, conn driver.Conn, variable, value string) error { + stmt := fmt.Sprintf("SET SESSION %s=%s", variable, value) - _, err := conn.(driver.ExecerContext).ExecContext(ctx, galeraOpts, nil) - if err != nil { + if _, err := conn.(driver.ExecerContext).ExecContext(ctx, stmt, nil); err != nil { if errors.Is(err, &mysql.MySQLError{Number: 1193}) { // Unknown system variable return nil } - return errors.Wrap(err, "cannot execute "+galeraOpts) + return CantPerformQuery(err, stmt) } return nil From 89a88c0ee964c22806ea3f14be5fbacdabc55676 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Mon, 27 May 2024 18:05:09 +0200 Subject: [PATCH 2/2] Test MySQL/MariaDB set session vars --- .github/workflows/sql.yml | 69 +++++++++++++++++++++++ database/utils_test.go | 112 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 181 insertions(+) create mode 100644 .github/workflows/sql.yml create mode 100644 database/utils_test.go diff --git a/.github/workflows/sql.yml b/.github/workflows/sql.yml new file mode 100644 index 0000000..27bff22 --- /dev/null +++ b/.github/workflows/sql.yml @@ -0,0 +1,69 @@ +name: SQL + +on: + push: + branches: + - main + pull_request: {} + +jobs: + mysql: + name: ${{ matrix.database.name }} + runs-on: ubuntu-latest + + strategy: + fail-fast: false + matrix: + database: + - {name: MySQL 5.7, image: "mysql:5.7"} + - {name: MySQL 8.0, image: "mysql:8.0"} + - {name: MySQL latest, image: "mysql:latest"} + - {name: MariaDB 10.1, image: "mariadb:10.1"} + - {name: MariaDB 10.2, image: "mariadb:10.2"} + - {name: MariaDB 10.3, image: "mariadb:10.3"} + - {name: MariaDB 10.4, image: "mariadb:10.4"} + - {name: MariaDB 10.5, image: "mariadb:10.5"} + - {name: MariaDB 10.6, image: "mariadb:10.6"} + - {name: MariaDB 10.7, image: "mariadb:10.7"} + - {name: MariaDB 10.11, image: "mariadb:10.11"} + - {name: MariaDB 11.0, image: "mariadb:11.0"} + - {name: MariaDB latest, image: "mariadb:latest"} + + env: + ICINGAGOLIBRARY_TESTS_DB_TYPE: mysql + ICINGAGOLIBRARY_TESTS_DB: icinga_unittest + ICINGAGOLIBRARY_TESTS_DB_USER: root + ICINGAGOLIBRARY_TESTS_DB_PASSWORD: password + ICINGAGOLIBRARY_TESTS_DB_HOST: 127.0.0.1 + ICINGAGOLIBRARY_TESTS_DB_PORT: 3306 + + services: + mysql: + image: ${{ matrix.database.image }} + env: + MYSQL_ROOT_PASSWORD: ${{ env.ICINGAGOLIBRARY_TESTS_DB_PASSWORD }} + MYSQL_DATABASE: ${{ env.ICINGAGOLIBRARY_TESTS_DB }} + # Wait for the containers to become ready + options: >- + --health-cmd "${{ (startsWith(matrix.database.image, 'mysql:') || startsWith(matrix.database.image, 'mariadb:10')) && 'mysqladmin ping' || 'healthcheck.sh --connect --innodb_initialized' }}" + --health-interval 10s + --health-timeout 5s + --health-retries 10 + ports: + - 3306:3306 + + steps: + - name: Setup Go + uses: actions/setup-go@v5 + with: + go-version: stable + + - name: Checkout code + uses: actions/checkout@v4 + + - name: Download dependencies + run: go get -v -t -d ./... + + - name: Run tests + timeout-minutes: 10 + run: go test -v -timeout 5m ./... diff --git a/database/utils_test.go b/database/utils_test.go new file mode 100644 index 0000000..ba74631 --- /dev/null +++ b/database/utils_test.go @@ -0,0 +1,112 @@ +package database + +import ( + "context" + "database/sql/driver" + "github.com/creasty/defaults" + "github.com/go-sql-driver/mysql" + "github.com/icinga/icinga-go-library/logging" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap/zaptest" + "os" + "strconv" + "strings" + "testing" + "time" +) + +func TestSetMysqlSessionVars(t *testing.T) { + vars := map[string][]struct { + name string + value string + expect error + }{ + "UnknownVariables": { + // MySQL single nodes do not recognise the "wsrep_sync_wait" system variable, but MariaDB does! + {name: "wsrep_sync_wait", value: "15"}, // MySQL unknown sys var | MariaDB succeeds + {name: "wsrep_sync_wait", value: "7"}, // MySQL unknown sys var | MariaDB succeeds + // Just some random unknown system variables :-) + {name: "Icinga", value: "Icinga"}, // unknown sys var + {name: "IcingaDB", value: "IcingaDB"}, // unknown sys var + }, + "VariablesWithCorrectValue": { // Setting system variables known by MySQL/MariaDB to a valid value + {name: "autocommit", value: "true"}, + {name: "binlog_format", value: "MIXED"}, + {name: "completion_type", value: "1" /** CHAIN */}, + {name: "completion_type", value: "CHAIN"}, + {name: "default_storage_engine", value: "InnoDB"}, + }, + "VariablesWithInvalidValues": { // System variables set to an invalid value + {name: "autocommit", value: "SOMETHING", expect: &mysql.MySQLError{Number: 1231}}, + {name: "binlog_format", value: "IcingaDB", expect: &mysql.MySQLError{Number: 1231}}, // Invalid val! + {name: "completion_type", value: "-10", expect: &mysql.MySQLError{Number: 1231}}, // Min valid val 0 + {name: "default_storage_engine", value: "IcingaDB", expect: &mysql.MySQLError{Number: 1286}}, // Unknown storage Engine! + }, + } + + ctx := context.Background() + db := GetTestDB(ctx, t, "ICINGAGOLIBRARY") + if db.DriverName() != MySQL { + t.Skipf("skipping set session vars test for %q driver", db.DriverName()) + } + + for name, vs := range vars { + t.Run(name, func(t *testing.T) { + for _, v := range vs { + conn, err := db.DB.Conn(ctx) + require.NoError(t, err, "connecting to MySQL/MariaDB database should not fail") + + err = conn.Raw(func(conn any) error { + return unsafeSetSessionVariableIfExists(ctx, conn.(driver.Conn), v.name, v.value) + }) + + assert.ErrorIsf(t, err, v.expect, "setting %q variable to '%v' returns unexpected result", v.name, v.value) + assert.NoError(t, conn.Close(), "closing MySQL/MariaDB connection should not fail") + } + }) + } +} + +// GetTestDB retrieves the database config from env variables, opens a new database and returns it. +// The [envPrefix] argument defines the environment variables prefix to look for e.g. `ICINGAGOLIBRARY`. +// +// The test suite will be skipped if no `envPrefix+"_TESTS_DB_TYPE" environment variable is +// set, otherwise fails fatally when invalid configurations are specified. +func GetTestDB(ctx context.Context, t *testing.T, envPrefix string) *DB { + c := &Config{} + require.NoError(t, defaults.Set(c), "applying config default should not fail") + + if v, ok := os.LookupEnv(envPrefix + "_TESTS_DB_TYPE"); ok { + c.Type = strings.ToLower(v) + } else { + t.Skipf("Environment %q not set, skipping test!", envPrefix+"_TESTS_DB_TYPE") + } + + if v, ok := os.LookupEnv(envPrefix + "_TESTS_DB"); ok { + c.Database = v + } + if v, ok := os.LookupEnv(envPrefix + "_TESTS_DB_USER"); ok { + c.User = v + } + if v, ok := os.LookupEnv(envPrefix + "_TESTS_DB_PASSWORD"); ok { + c.Password = v + } + if v, ok := os.LookupEnv(envPrefix + "_TESTS_DB_HOST"); ok { + c.Host = v + } + if v, ok := os.LookupEnv(envPrefix + "_TESTS_DB_PORT"); ok { + port, err := strconv.Atoi(v) + require.NoError(t, err, "invalid port provided") + + c.Port = port + } + + require.NoError(t, c.Validate(), "database config validation should not fail") + + db, err := NewDbFromConfig(c, logging.NewLogger(zaptest.NewLogger(t).Sugar(), time.Hour), RetryConnectorCallbacks{}) + require.NoError(t, err, "connecting to database should not fail") + require.NoError(t, db.PingContext(ctx), "pinging the database should not fail") + + return db +}