sa: Stop injecting max_statement_time and long_query_time into DSNs (letsencrypt#8490)

Author: beautifulentropy

sa/database.go

@@ -201,13 +201,6 @@ func adjustMySQLConfig(conf *mysql.Config) error {
 		}
 	}
 
-	// If a given parameter has the value "0", delete it from conf.Params.
-	omitZero := func(name string) {
-		if conf.Params[name] == "0" {
-			delete(conf.Params, name)
-		}
-	}
-
 	// Ensures that MySQL/MariaDB warnings are treated as errors. This
 	// avoids a number of nasty edge conditions we could wander into.
 	// Common things this discovers includes places where data being sent
@@ -216,25 +209,10 @@ func adjustMySQLConfig(conf *mysql.Config) error {
 	// <https://dev.mysql.com/doc/refman/5.0/en/sql-mode.html#sql-mode-strict>.
 	setDefault("sql_mode", "'STRICT_ALL_TABLES'")
 
-	// If a read timeout is set, we set max_statement_time to 95% of that, and
-	// long_query_time to 80% of that. That way we get logs of queries that are
-	// close to timing out but not yet doing so, and our queries get stopped by
-	// max_statement_time before timing out the read. This generates clearer
-	// errors, and avoids unnecessary reconnects.
-	// To override these values, set them in the DSN, e.g.
-	// `?max_statement_time=2`. A zero value in the DSN means these won't be
-	// sent on new connections.
-	if conf.ReadTimeout != 0 {
-		// In MariaDB, max_statement_time and long_query_time are both seconds,
-		// but can have up to microsecond granularity.
-		// Note: in MySQL (which we don't use), max_statement_time is millis.
-		readTimeout := conf.ReadTimeout.Seconds()
-		setDefault("max_statement_time", fmt.Sprintf("%.6f", readTimeout*0.95))
-		setDefault("long_query_time", fmt.Sprintf("%.6f", readTimeout*0.80))
-	}
-
-	omitZero("max_statement_time")
-	omitZero("long_query_time")
+	// Omit max_statement_time and max_execution_time from the DSN. Query
+	// timeouts are managed exclusively by ProxySQL and/or Vitess.
+	delete(conf.Params, "max_statement_time")
+	delete(conf.Params, "max_execution_time")
 
 	// Finally, perform validation over all variables set by the DSN and via Boulder.
 	for k, v := range conf.Params {

sa/database_test.go

@@ -21,19 +21,19 @@ func TestInvalidDSN(t *testing.T) {
 	_, err := DBMapForTest("invalid")
 	test.AssertError(t, err, "DB connect string missing the slash separating the database name")
 
-	DSN := "policy:password@tcp(boulder-proxysql:6033)/boulder_policy_integration?readTimeout=800ms&writeTimeout=800ms&stringVarThatDoesntExist=%27whoopsidaisies"
+	DSN := "policy:password@tcp(foo-database:1337)/boulder_policy_integration?readTimeout=800ms&writeTimeout=800ms&stringVarThatDoesntExist=%27whoopsidaisies"
 	_, err = DBMapForTest(DSN)
 	test.AssertError(t, err, "Variable does not exist in curated system var list, but didn't return an error and should have")
 
-	DSN = "policy:password@tcp(boulder-proxysql:6033)/boulder_policy_integration?readTimeout=800ms&writeTimeout=800ms&concurrent_insert=2"
+	DSN = "policy:password@tcp(foo-database:1337)/boulder_policy_integration?readTimeout=800ms&writeTimeout=800ms&concurrent_insert=2"
 	_, err = DBMapForTest(DSN)
 	test.AssertError(t, err, "Variable is unable to be set in the SESSION scope, but was declared")
 
-	DSN = "policy:password@tcp(boulder-proxysql:6033)/boulder_policy_integration?readTimeout=800ms&writeTimeout=800ms&optimizer_switch=incorrect-quoted-string"
+	DSN = "policy:password@tcp(foo-database:1337)/boulder_policy_integration?readTimeout=800ms&writeTimeout=800ms&optimizer_switch=incorrect-quoted-string"
 	_, err = DBMapForTest(DSN)
 	test.AssertError(t, err, "Variable declared with incorrect quoting")
 
-	DSN = "policy:password@tcp(boulder-proxysql:6033)/boulder_policy_integration?readTimeout=800ms&writeTimeout=800ms&concurrent_insert=%272%27"
+	DSN = "policy:password@tcp(foo-database:1337)/boulder_policy_integration?readTimeout=800ms&writeTimeout=800ms&concurrent_insert=%272%27"
 	_, err = DBMapForTest(DSN)
 	test.AssertError(t, err, "Integer enum declared, but should not have been quoted")
 }
@@ -75,9 +75,8 @@ func TestDbSettings(t *testing.T) {
 		oldSetConnMaxIdleTime(db, connMaxIdleTime)
 	}
 	dsnFile := path.Join(t.TempDir(), "dbconnect")
-	err := os.WriteFile(dsnFile,
-		[]byte("sa@tcp(boulder-proxysql:6033)/boulder_sa_integration"),
-		os.ModeAppend)
+
+	err := os.WriteFile(dsnFile, []byte(vars.DBConnSA), os.ModeAppend)
 	test.AssertNotError(t, err, "writing dbconnect file")
 
 	config := cmd.DBConfig{
@@ -108,7 +107,7 @@ func TestDbSettings(t *testing.T) {
 // TODO: Change this to test `newDbMapFromMySQLConfig` instead?
 func TestNewDbMap(t *testing.T) {
 	const mysqlConnectURL = "policy:password@tcp(boulder-proxysql:6033)/boulder_policy_integration?readTimeout=800ms&writeTimeout=800ms"
-	const expected = "policy:password@tcp(boulder-proxysql:6033)/boulder_policy_integration?clientFoundRows=true&parseTime=true&readTimeout=800ms&writeTimeout=800ms&long_query_time=0.640000&max_statement_time=0.760000&sql_mode=%27STRICT_ALL_TABLES%27"
+	const expected = "policy:password@tcp(boulder-proxysql:6033)/boulder_policy_integration?clientFoundRows=true&parseTime=true&readTimeout=800ms&writeTimeout=800ms&sql_mode=%27STRICT_ALL_TABLES%27"
 	oldSQLOpen := sqlOpen
 	defer func() {
 		sqlOpen = oldSQLOpen
@@ -146,27 +145,6 @@ func TestStrictness(t *testing.T) {
 	}
 }
 
-func TestTimeouts(t *testing.T) {
-	dbMap, err := DBMapForTest(vars.DBConnSA + "?max_statement_time=1")
-	if err != nil {
-		t.Fatal("Error setting up DB:", err)
-	}
-	// SLEEP is defined to return 1 if it was interrupted, but we want to actually
-	// get an error to simulate what would happen with a slow query. So we wrap
-	// the SLEEP in a subselect.
-	_, err = dbMap.ExecContext(ctx, `SELECT 1 FROM (SELECT SLEEP(5)) as subselect;`)
-	if err == nil {
-		t.Fatal("Expected error when running slow query, got none.")
-	}
-
-	// We expect to get:
-	// Error 1969: Query execution was interrupted (max_statement_time exceeded)
-	// https://mariadb.com/kb/en/mariadb/mariadb-error-codes/
-	if !strings.Contains(err.Error(), "Error 1969") {
-		t.Fatalf("Got wrong type of error: %s", err)
-	}
-}
-
 // TestAutoIncrementSchema tests that all of the tables in the boulder_*
 // databases that have auto_increment columns use BIGINT for the data type. Our
 // data is too big for INT.
@@ -190,40 +168,7 @@ func TestAdjustMySQLConfig(t *testing.T) {
 	conf := &mysql.Config{}
 	err := adjustMySQLConfig(conf)
 	test.AssertNotError(t, err, "unexpected err setting server variables")
-	test.AssertDeepEquals(t, conf.Params, map[string]string{
-		"sql_mode": "'STRICT_ALL_TABLES'",
-	})
-
-	conf = &mysql.Config{ReadTimeout: 100 * time.Second}
-	err = adjustMySQLConfig(conf)
-	test.AssertNotError(t, err, "unexpected err setting server variables")
-	test.AssertDeepEquals(t, conf.Params, map[string]string{
-		"sql_mode":           "'STRICT_ALL_TABLES'",
-		"max_statement_time": "95.000000",
-		"long_query_time":    "80.000000",
-	})
-
-	conf = &mysql.Config{
-		ReadTimeout: 100 * time.Second,
-		Params: map[string]string{
-			"max_statement_time": "0",
-		},
-	}
-	err = adjustMySQLConfig(conf)
-	test.AssertNotError(t, err, "unexpected err setting server variables")
-	test.AssertDeepEquals(t, conf.Params, map[string]string{
-		"sql_mode":        "'STRICT_ALL_TABLES'",
-		"long_query_time": "80.000000",
-	})
-
-	conf = &mysql.Config{
-		Params: map[string]string{
-			"max_statement_time": "0",
-		},
-	}
-	err = adjustMySQLConfig(conf)
-	test.AssertNotError(t, err, "unexpected err setting server variables")
-	test.AssertDeepEquals(t, conf.Params, map[string]string{
-		"sql_mode": "'STRICT_ALL_TABLES'",
-	})
+	test.Assert(t, conf.ParseTime, "ParseTime should be enabled")
+	test.Assert(t, conf.ClientFoundRows, "ClientFoundRows should be enabled")
+	test.AssertDeepEquals(t, conf.Params, map[string]string{"sql_mode": "'STRICT_ALL_TABLES'"})
 }

test.sh

@@ -12,6 +12,7 @@ fi
 # Defaults
 #
 export RACE="false"
+export DB_ADDR="boulder-proxysql:6033"
 STAGE="starting"
 STATUS="FAILURE"
 RUN=()

test/db.go

@@ -6,12 +6,12 @@ import (
 	"fmt"
 	"io"
 	"testing"
-)
 
-var (
-	_ CleanUpDB = &sql.DB{}
+	"github.com/letsencrypt/boulder/test/vars"
 )
 
+var _ CleanUpDB = &sql.DB{}
+
 // CleanUpDB is an interface with only what is needed to delete all
 // rows in all tables in a database plus close the database
 // connection. It is satisfied by *sql.DB.
@@ -27,7 +27,7 @@ type CleanUpDB interface {
 // table as this is used by sql-migrate (https://github.com/rubenv/sql-migrate)
 // to track migrations. If it encounters an error it fails the tests.
 func ResetBoulderTestDatabase(t testing.TB) func() {
-	return resetTestDatabase(t, context.Background(), "boulder")
+	return resetTestDatabase(t, context.Background(), vars.DBConnSAFullPerms)
 }
 
 // ResetIncidentsTestDatabase returns a cleanup function which deletes all rows
@@ -36,11 +36,11 @@ func ResetBoulderTestDatabase(t testing.TB) func() {
 // (https://github.com/rubenv/sql-migrate) to track migrations. If it encounters
 // an error it fails the tests.
 func ResetIncidentsTestDatabase(t testing.TB) func() {
-	return resetTestDatabase(t, context.Background(), "incidents")
+	return resetTestDatabase(t, context.Background(), vars.DBConnIncidentsFullPerms)
 }
 
-func resetTestDatabase(t testing.TB, ctx context.Context, dbPrefix string) func() {
-	db, err := sql.Open("mysql", fmt.Sprintf("test_setup@tcp(boulder-proxysql:6033)/%s_sa_test", dbPrefix))
+func resetTestDatabase(t testing.TB, ctx context.Context, dsn string) func() {
+	db, err := sql.Open("mysql", dsn)
 	if err != nil {
 		t.Fatalf("Couldn't create db: %s", err)
 	}