diff --git a/sa/database.go b/sa/database.go index 0d06fac5f90..5d54d3aebfd 100644 --- a/sa/database.go +++ b/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 { // . 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 { diff --git a/sa/database_test.go b/sa/database_test.go index 1585c6d89cf..95cfe07a977 100644 --- a/sa/database_test.go +++ b/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'"}) } diff --git a/test.sh b/test.sh index 81a9dccfda9..2f55d25e97f 100755 --- a/test.sh +++ b/test.sh @@ -12,6 +12,7 @@ fi # Defaults # export RACE="false" +export DB_ADDR="boulder-proxysql:6033" STAGE="starting" STATUS="FAILURE" RUN=() diff --git a/test/db.go b/test/db.go index 13a4e5270f0..a80cb0889a1 100644 --- a/test/db.go +++ b/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) } diff --git a/test/secrets/backfiller_dburl b/test/secrets/backfiller_dburl deleted file mode 100644 index b62d870a545..00000000000 --- a/test/secrets/backfiller_dburl +++ /dev/null @@ -1 +0,0 @@ -sa@tcp(boulder-proxysql:6033)/boulder_sa_integration diff --git a/test/secrets/expiration_mailer_dburl b/test/secrets/expiration_mailer_dburl deleted file mode 100644 index 615415cd8bb..00000000000 --- a/test/secrets/expiration_mailer_dburl +++ /dev/null @@ -1 +0,0 @@ -mailer@tcp(boulder-proxysql:6033)/boulder_sa_integration diff --git a/test/secrets/mailer_dburl b/test/secrets/mailer_dburl deleted file mode 100644 index 615415cd8bb..00000000000 --- a/test/secrets/mailer_dburl +++ /dev/null @@ -1 +0,0 @@ -mailer@tcp(boulder-proxysql:6033)/boulder_sa_integration diff --git a/test/secrets/ocsp_responder_dburl b/test/secrets/ocsp_responder_dburl deleted file mode 100644 index 4a789bad0b1..00000000000 --- a/test/secrets/ocsp_responder_dburl +++ /dev/null @@ -1 +0,0 @@ -ocsp_resp@tcp(boulder-proxysql:6033)/boulder_sa_integration?readTimeout=800ms&writeTimeout=800ms&timeout=100ms diff --git a/test/secrets/ocsp_responder_redis_password b/test/secrets/ocsp_responder_redis_password deleted file mode 100644 index a132ec74b6a..00000000000 --- a/test/secrets/ocsp_responder_redis_password +++ /dev/null @@ -1 +0,0 @@ -0e5a4c8b5faaf3194c8ad83c3dd9a0dd8a75982b diff --git a/test/secrets/rocsp_tool_password b/test/secrets/rocsp_tool_password deleted file mode 100644 index f659bd3fc2e..00000000000 --- a/test/secrets/rocsp_tool_password +++ /dev/null @@ -1 +0,0 @@ -e4e9ce7845cb6adbbc44fb1d9deb05e6b4dc1386 diff --git a/test/vars/vars.go b/test/vars/vars.go index deb2b56df95..ab6ad78574c 100644 --- a/test/vars/vars.go +++ b/test/vars/vars.go @@ -1,25 +1,41 @@ package vars -import "fmt" - -const ( - dbURL = "%s@tcp(boulder-proxysql:6033)/%s" +import ( + "fmt" + "net" + "os" ) +var dbAddr = func() string { + addr := os.Getenv("DB_ADDR") + if addr == "" { + panic("environment variable DB_ADDR must be set") + } + _, _, err := net.SplitHostPort(addr) + if err != nil { + panic(fmt.Sprintf("environment variable DB_ADDR (%s) is not a valid address with host and port: %s", addr, err)) + } + return addr +}() + +func dsn(user, database string) string { + return fmt.Sprintf("%s@tcp(%s)/%s", user, dbAddr, database) +} + var ( // DBConnSA is the sa database connection - DBConnSA = fmt.Sprintf(dbURL, "sa", "boulder_sa_test") + DBConnSA = dsn("sa", "boulder_sa_test") // DBConnSAMailer is the sa mailer database connection - DBConnSAMailer = fmt.Sprintf(dbURL, "mailer", "boulder_sa_test") + DBConnSAMailer = dsn("mailer", "boulder_sa_test") // DBConnSAFullPerms is the sa database connection with full perms - DBConnSAFullPerms = fmt.Sprintf(dbURL, "test_setup", "boulder_sa_test") + DBConnSAFullPerms = dsn("test_setup", "boulder_sa_test") // DBConnSAIntegrationFullPerms is the sa database connection for the // integration test DB, with full perms - DBConnSAIntegrationFullPerms = fmt.Sprintf(dbURL, "test_setup", "boulder_sa_integration") + DBConnSAIntegrationFullPerms = dsn("test_setup", "boulder_sa_integration") // DBInfoSchemaRoot is the root user and the information_schema connection. - DBInfoSchemaRoot = fmt.Sprintf(dbURL, "root", "information_schema") + DBInfoSchemaRoot = dsn("root", "information_schema") // DBConnIncidents is the incidents database connection. - DBConnIncidents = fmt.Sprintf(dbURL, "incidents_sa", "incidents_sa_test") + DBConnIncidents = dsn("incidents_sa", "incidents_sa_test") // DBConnIncidentsFullPerms is the incidents database connection with full perms. - DBConnIncidentsFullPerms = fmt.Sprintf(dbURL, "test_setup", "incidents_sa_test") + DBConnIncidentsFullPerms = dsn("test_setup", "incidents_sa_test") )