diff --git a/scripts/images/rds-mariadb/rds-init b/scripts/images/rds-mariadb/rds-init index 57d3298be..02a05c040 100644 --- a/scripts/images/rds-mariadb/rds-init +++ b/scripts/images/rds-mariadb/rds-init @@ -252,14 +252,29 @@ require_data_volume() { log "warning: ${DATA_MOUNT} is not a mount point; data will not survive a VM replace (RDS_ALLOW_LOCAL_DATADIR=1)" } -# True when the datadir holds anything at all. A failed enumeration is neither -# "empty" nor "has content": it is a refusal, because both callers below decide -# whether something may be created or deleted from the answer. -datadir_has_content() { - [ -d "${DATADIR}" ] || return 1 - _contents=$(ls -A -- "${DATADIR}" 2>/dev/null) || - fail "could not inspect datadir ${DATADIR}; refusing to decide whether it is ours" - [ -n "${_contents}" ] +# True when the data volume holds anything that is not ours. The mount is what +# is inspected rather than ${DATADIR}: another engine lays its data out under its +# own name, so a PostgreSQL volume has content here while ${DATADIR} does not +# exist at all. +data_volume_has_content() { + [ -d "${DATA_MOUNT}" ] || return 1 + + # A mount that cannot be read is neither empty nor not: it is a refusal, + # because the caller decides from the answer whether to initialise. + ls -A -- "${DATA_MOUNT}" >/dev/null 2>&1 || + fail "could not inspect ${DATA_MOUNT}; refusing to decide whether the data volume is ours" + + # Globbed rather than split out of `ls`, so an entry carrying whitespace + # counts once. lost+found is the filesystem's, and the metadata and include + # directories are ours — none of the three says whose data the volume holds. + for _entry in "${DATA_MOUNT}"/* "${DATA_MOUNT}"/.[!.]* "${DATA_MOUNT}"/..?*; do + [ -e "${_entry}" ] || [ -L "${_entry}" ] || continue + case "${_entry##*/}" in + lost+found | .spinifex-rds | "${CONF_DIR##*/}") continue ;; + esac + return 0 + done + return 1 } # Written before the datadir is inspected, so a volume this image goes on to @@ -304,11 +319,11 @@ require_engine_stamp() { fi # No MariaDB volume in the field predates the stamp, so unlike PostgreSQL - # there is nothing to backfill onto: an unstamped datadir with content in it + # there is nothing to backfill onto: an unstamped volume with content on it # is by definition not ours, and stamping it would turn the check into a # rubber stamp on exactly the volume it exists to refuse. - if datadir_has_content; then - fail "${DATADIR} holds data but the volume carries no ${ENGINE} engine stamp; refusing to touch it. Recover the volume out of band." + if data_volume_has_content; then + fail "${DATA_MOUNT} holds data but the volume carries no ${ENGINE} engine stamp; refusing to touch it. Recover the volume out of band." fi log "data volume carries no engine stamp — recording ${ENGINE}" @@ -639,13 +654,24 @@ FLUSH PRIVILEGES; SQL } -# The master user is administrative but deliberately holds none of the -# privileges that reach outside the database: FILE is SELECT INTO OUTFILE and -# LOAD_FILE against any path as the mysql OS user, INSERT on mysql.* is native -# code execution through CREATE FUNCTION ... SONAME, SUPER and its 11.8 split-out -# successors reach SET GLOBAL and plugin loading, and SHUTDOWN stops the server -# underneath the control plane's lifecycle. Master credentials must be a database -# privilege, not a shell in the DB VM. +# The master user is administrative but is not granted the privileges that reach +# outside the database: FILE is SELECT INTO OUTFILE and LOAD_FILE against any +# path as the mysql OS user, SUPER and its 11.8 split-out successors reach SET +# GLOBAL and plugin loading, and SHUTDOWN stops the server underneath the control +# plane's lifecycle. Master credentials must be a database privilege, not a shell +# in the DB VM. +# +# What the grant list cannot do is withhold the mysql schema. MariaDB privileges +# are additive and it has no DENY, so the DML below reaches mysql.global_priv, +# and REVOKE ... ON mysql.* is refused outright (ER_NONEXISTING_GRANT) because no +# database-level row exists to revoke from. A master that writes that table and +# runs FLUSH PRIVILEGES therefore holds ALL PRIVILEGES, including the three above. +# +# What still holds after that is the boundary this preset actually depends on: +# secure_file_priv and plugin_dir are read-only variables, so 90-rds-init.cnf +# pins them past any SET GLOBAL and FILE reaches only an empty root-owned +# directory. The residual exposure is SHUTDOWN and unrecorded SET GLOBAL, not a +# shell in the DB VM. apply_master_user() { log "creating the master user ${RDS_MASTER_USERNAME}" # Enumerated rather than GRANT ALL followed by revocations: a MariaDB release diff --git a/scripts/images/rds-mariadb/rds-init_test.sh b/scripts/images/rds-mariadb/rds-init_test.sh index 89d13ae1f..edc189da9 100755 --- a/scripts/images/rds-mariadb/rds-init_test.sh +++ b/scripts/images/rds-mariadb/rds-init_test.sh @@ -387,15 +387,35 @@ if run_ok "initialize"; then || fail "initialize: the database was created under the compiled default" # Withheld deliberately: FILE is arbitrary host file access as the mysql user, - # SUPER reaches SET GLOBAL and plugin loading, and SHUTDOWN stops the server - # underneath the control plane's lifecycle. - for privilege in SUPER FILE SHUTDOWN 'ALL PRIVILEGES'; do + # SUPER and its 11.8 split-out successors reach SET GLOBAL and plugin loading, + # and SHUTDOWN stops the server underneath the control plane's lifecycle. + for privilege in SUPER FILE SHUTDOWN 'ALL PRIVILEGES' 'SET USER' \ + 'BINLOG ADMIN' 'BINLOG REPLAY' 'CONNECTION ADMIN' 'FEDERATED ADMIN' \ + 'READ_ONLY ADMIN' 'REPLICATION MASTER ADMIN' 'REPLICATION SLAVE ADMIN' \ + 'SLAVE MONITOR'; do grep -q "${privilege}" "${CLIENT_CALLS}" \ && fail "initialize: the master grant includes ${privilege}" \ || pass "initialize: master holds no ${privilege}" done - grep -q 'GRANT SELECT, INSERT, UPDATE, DELETE' "${CLIENT_CALLS}" \ - && pass "initialize: the grant is enumerated" || fail "initialize: no enumerated grant" + + # Asserted whole rather than by prefix. A privilege appended to the list is a + # privilege the master silently gains, and the enumeration exists precisely so + # that widening it has to be deliberate. + _grant=$(sed -n '/^GRANT /,/WITH GRANT OPTION;/p' "${CLIENT_CALLS}" | tr '\n' ' ' | tr -s ' ') + _expected_grant="GRANT SELECT, INSERT, UPDATE, DELETE, CREATE, DROP, RELOAD, PROCESS, \ +REFERENCES, INDEX, ALTER, SHOW DATABASES, CREATE TEMPORARY TABLES, \ +LOCK TABLES, EXECUTE, REPLICATION SLAVE, REPLICATION CLIENT, \ +CREATE VIEW, SHOW VIEW, CREATE ROUTINE, ALTER ROUTINE, \ +CREATE USER, EVENT, TRIGGER ON *.* TO 'mulgamaster'@'%' WITH GRANT OPTION; " + [ "${_grant}" = "${_expected_grant}" ] \ + && pass "initialize: the grant is exactly the enumerated list" \ + || fail "initialize: the master grant has changed: ${_grant}" + + # MariaDB privileges are additive and it has no DENY, so scope is the only + # control left: a second ON clause would widen the master beyond this list. + [ "$(grep -c '^ ON ' "${CLIENT_CALLS}")" = "1" ] \ + && pass "initialize: the grant is scoped ON *.* and nothing else" \ + || fail "initialize: the master carries more than one grant scope" grep -q "WITH GRANT OPTION" "${CLIENT_CALLS}" \ && pass "initialize: master can delegate its own privileges" \ || fail "initialize: master cannot grant" @@ -699,7 +719,7 @@ unset LS_FAIL [ -s "${INSTALLDB_CALLS}" ] \ && fail "datadir-probe-fail: initialised after the probe failed" \ || pass "datadir-probe-fail: nothing created" -grep -q 'could not inspect datadir' "${WORK}/out" \ +grep -q 'refusing to decide whether the data volume is ours' "${WORK}/out" \ && pass "datadir-probe-fail: refusal explains why" || fail "datadir-probe-fail: no refusal message" # --- Case 6c: the bootstrap server never starts --- @@ -1012,6 +1032,47 @@ if run_ok "stamp-absent-empty"; then grep -q 'no mariadb engine stamp' "${WORK}/out" \ && pass "stamp-absent-nonempty: refusal names the missing stamp" \ || fail "stamp-absent-nonempty: no refusal message" + + # --- Case 12f: an unstamped PostgreSQL volume is refused --- + # The one unstamped volume that actually exists in the field, and the reason + # emptiness is decided on the mount: PostgreSQL keeps its cluster under + # ${DATA_MOUNT}/18/data, so ${DATADIR} is absent and the volume reads as new. + rm -rf "${DATADIR}" "${STAMP}" + mkdir -p "${DATA_MOUNT}/18/data" + echo 'customer cluster data' > "${DATA_MOUNT}/18/data/PG_VERSION" + : > "${INSTALLDB_CALLS}" + run_fails "stamp-absent-pgvolume" + [ -s "${INSTALLDB_CALLS}" ] \ + && fail "stamp-absent-pgvolume: initialised over a PostgreSQL volume" \ + || pass "stamp-absent-pgvolume: nothing created" + [ -f "${DATA_MOUNT}/18/data/PG_VERSION" ] \ + && pass "stamp-absent-pgvolume: the PostgreSQL cluster is preserved" \ + || fail "stamp-absent-pgvolume: the PostgreSQL cluster was touched" + # A stamp here is the worst outcome of all: the PostgreSQL image refuses a + # 'mariadb' stamp too, so the volume becomes unreachable from both engines. + [ -e "${STAMP}" ] \ + && fail "stamp-absent-pgvolume: another engine's volume was stamped ours" \ + || pass "stamp-absent-pgvolume: no stamp written" + grep -q 'no mariadb engine stamp' "${WORK}/out" \ + && pass "stamp-absent-pgvolume: refusal names the missing stamp" \ + || fail "stamp-absent-pgvolume: no refusal message" + rm -rf "${DATA_MOUNT}/18" + + # --- Case 12g: lost+found alone does not make a fresh volume look used --- + # ext4 creates it at mkfs, so counting it as content would refuse every + # genuinely new data volume. + rm -rf "${DATADIR}" "${STAMP}" + mkdir -p "${DATA_MOUNT}/lost+found" + : > "${INSTALLDB_CALLS}" + write_handoff initialize 's3cr3t' appdb + if run_ok "stamp-absent-lostfound"; then + [ -s "${INSTALLDB_CALLS}" ] \ + && pass "stamp-absent-lostfound: a fresh volume still initialises" \ + || fail "stamp-absent-lostfound: nothing was initialised" + [ "$(cat "${STAMP}")" = "mariadb" ] \ + && pass "stamp-absent-lostfound: the fresh volume was stamped" \ + || fail "stamp-absent-lostfound: no stamp written" + fi fi # --- Case 12e: a stamp that cannot be written is fatal, before initialisation --- diff --git a/spinifex/handlers/rds/engine.go b/spinifex/handlers/rds/engine.go index dd1073a20..689a545c9 100644 --- a/spinifex/handlers/rds/engine.go +++ b/spinifex/handlers/rds/engine.go @@ -40,6 +40,9 @@ type Engine struct { // What a snapshot taken without a quiesce actually recovers on restore, which // is the engine's own guarantee and not a shared one. crashRecoveryNote string + // The same guarantee stated for the next start rather than for a restore, + // which is what an engine that would not shut down cleanly gets. + uncleanStopNote string } var engines = map[string]Engine{ @@ -60,11 +63,15 @@ func indexEnginesByFamily() map[string]Engine { if engine.validateDBName == nil { panic("rds: engine " + engine.Name + " registers no DBName rule") } - // An engine without one would tell a customer nothing about what a - // crash-consistent snapshot of it recovers, on the one event that says so. + // An engine without these would tell a customer nothing about what a + // crash-consistent snapshot or an unclean stop of it recovers, on the two + // events that say so. if engine.crashRecoveryNote == "" { panic("rds: engine " + engine.Name + " registers no crash-recovery note") } + if engine.uncleanStopNote == "" { + panic("rds: engine " + engine.Name + " registers no unclean-stop note") + } family := engine.ParameterGroupFamily() if _, exists := out[family]; exists { panic("rds: parameter group family " + family + " is claimed by two engines") diff --git a/spinifex/handlers/rds/engine_mariadb.go b/spinifex/handlers/rds/engine_mariadb.go index 62397c13e..051b52f41 100644 --- a/spinifex/handlers/rds/engine_mariadb.go +++ b/spinifex/handlers/rds/engine_mariadb.go @@ -36,6 +36,8 @@ var engineMariaDB = Engine{ // mid-write can leave one of their tables inconsistent with no way back. crashRecoveryNote: "InnoDB tables will recover from the redo log when it is restored; " + "non-transactional tables such as Aria and MyISAM may be left inconsistent.", + uncleanStopNote: "InnoDB tables will recover from the redo log on the next start; " + + "non-transactional tables such as Aria and MyISAM may be left inconsistent.", } // The curated MariaDB 11.8 table, the same shape and approximate size as the diff --git a/spinifex/handlers/rds/engine_postgres.go b/spinifex/handlers/rds/engine_postgres.go index bd7c240f7..63ebc882a 100644 --- a/spinifex/handlers/rds/engine_postgres.go +++ b/spinifex/handlers/rds/engine_postgres.go @@ -31,6 +31,7 @@ var enginePostgres = Engine{ validateCombinations: validatePostgresParameterCombinations, // Every table is WAL-logged, so a torn datadir recovers in full. crashRecoveryNote: "It will recover from its write-ahead log when it is restored.", + uncleanStopNote: "It will recover from its write-ahead log on the next start.", } // The curated PostgreSQL 18 table: a genuinely validated subset rather than the diff --git a/spinifex/handlers/rds/enginefamily_test.go b/spinifex/handlers/rds/enginefamily_test.go index 3984dcfcc..5aeae56eb 100644 --- a/spinifex/handlers/rds/enginefamily_test.go +++ b/spinifex/handlers/rds/enginefamily_test.go @@ -42,6 +42,7 @@ var engineUnderTest = Engine{ ), validateCombinations: func([]Parameter) error { return nil }, crashRecoveryNote: "It will recover when it is restored.", + uncleanStopNote: "It will recover on the next start.", } // Registers the second engine for one test. Both indexes are rebuilt, because a diff --git a/spinifex/handlers/rds/lifecycle.go b/spinifex/handlers/rds/lifecycle.go index da3cc5530..cc4a09953 100644 --- a/spinifex/handlers/rds/lifecycle.go +++ b/spinifex/handlers/rds/lifecycle.go @@ -233,19 +233,34 @@ func (s *Service) confirmVMStopped(ctx context.Context, accountID, instanceID st } // Asks the engine to shut down cleanly, and records an event rather than -// failing the call when it cannot. An engine that did not stop cleanly replays -// its WAL on the next start; refusing to stop the VM over it would leave the -// customer unable to stop an instance whose agent is wedged. +// failing the call when it cannot. Refusing to stop the VM over it would leave +// the customer unable to stop an instance whose agent is wedged. func (s *Service) stopEngineOrRecordFallback(ctx context.Context, accountID string, rec *DBInstanceRecord, operation string) { if err := s.stopEngine(ctx, accountID, rec.DBInstanceIdentifier); err != nil { slog.WarnContext(ctx, "rds: graceful engine stop failed; continuing", "dbInstance", rec.DBInstanceIdentifier, "operation", operation, "err", err) s.RecordEvent(ctx, accountID, EventSourceTypeDBInstance, rec.DBInstanceIdentifier, - fmt.Sprintf("The database engine could not be shut down cleanly before %s; it will recover from its write-ahead log on the next start.", operation), + uncleanStopMessage(ctx, rec.Engine, operation), EventCategoryNotification, EventCategoryAvailability) } } +// The half of the warning that holds for every engine. What the next start then +// recovers does not: PostgreSQL replays every table from its write-ahead log, +// MariaDB only its InnoDB ones. +func uncleanStopMessage(ctx context.Context, engineName, operation string) string { + warning := fmt.Sprintf("The database engine could not be shut down cleanly before %s.", operation) + engine, err := LookupEngine(engineName) + if err != nil { + // The VM is going down either way, so the customer still gets the half of + // the warning that does not depend on knowing the engine. + slog.ErrorContext(ctx, "rds: the DB instance names an engine this build does not offer", + "engine", engineName, "err", err) + return warning + } + return warning + " " + engine.uncleanStopNote +} + // Moves the instance into a transitional state under CAS and returns the record // as written. from lists the statuses the operation is legal from, so a stop of // an already-stopping instance is rejected as a state error rather than racing diff --git a/spinifex/handlers/rds/lifecycle_test.go b/spinifex/handlers/rds/lifecycle_test.go index 2b2f987c5..29518c193 100644 --- a/spinifex/handlers/rds/lifecycle_test.go +++ b/spinifex/handlers/rds/lifecycle_test.go @@ -679,3 +679,23 @@ func TestReconciler_MarksFailedWhenARestartOverrunsItsBound(t *testing.T) { assert.Equal(t, StatusFailed, rec.Status) assert.Contains(t, rec.FailureReason, "did not report healthy") } + +// What the next start recovers is the engine's own guarantee. Telling a MariaDB +// customer a write-ahead log will bring back an Aria or MyISAM table would be a +// false assurance about exactly the data most at risk. +func TestUncleanStopMessage_IsEngineAware(t *testing.T) { + postgres := uncleanStopMessage(t.Context(), "postgres", "stopping the instance") + assert.Contains(t, postgres, "could not be shut down cleanly before stopping the instance") + assert.Contains(t, postgres, "write-ahead log") + + mariadb := uncleanStopMessage(t.Context(), "mariadb", "rebooting the instance") + assert.Contains(t, mariadb, "could not be shut down cleanly before rebooting the instance") + assert.Contains(t, mariadb, "InnoDB tables will recover") + assert.Contains(t, mariadb, "may be left inconsistent") + assert.NotContains(t, mariadb, "write-ahead log") + + // The VM is going down either way, so an engine this build cannot resolve + // still gets the half of the warning that does not depend on knowing it. + unknown := uncleanStopMessage(t.Context(), "oracle", "stopping the instance") + assert.Equal(t, "The database engine could not be shut down cleanly before stopping the instance.", unknown) +}