Merge pull request #20 from Icinga/support-set-session-vars

MySQL/MariaDB set session vars unittests

Author: yhabteab

.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 ./...

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

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

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
+}