From 4b552478195624e2e6ac6a710ffb846b1e4bfa2d Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Thu, 8 Oct 2026 17:33:11 +1000 Subject: [PATCH] refactor: extract RDS engine catalogue into domains/rds/engine Move engine identity and validation, the per-engine parameter catalogues, effective-parameter resolution, db.* instance classes and engine/orderable selection into an engine package. Behaviour, error codes and ordering are unchanged. The package imports no EC2 package: instance-class memory comes from an immutable Sizing value built from an engine-declared InstanceTypes interface, backed by the fixed instance-type table in handlers/rds. Effective parameters leave as an engine-owned Setting, converted to Parameter in handlers/rds. AWS output building, paging, apply-method mapping, storage limits and the gateway runnable check stay in the adapter path. The RDS agent declares its own EngineCatalog interface and types and receives an engine-backed implementation from cmd/rds-agent, so agent code does not import the catalogue. Cross-engine tests use the two real engines instead of a registered fake. --- cmd/rds-agent/main.go | 47 ++ docs/service-interfaces.yaml | 1 + spinifex/agents/rds/agent/agent.go | 21 +- spinifex/agents/rds/agent/bootstrap.go | 13 +- spinifex/agents/rds/agent/bootstrap_test.go | 35 +- spinifex/agents/rds/agent/config.go | 5 + spinifex/agents/rds/agent/config_test.go | 4 +- .../agents/rds/agent/defaultparams_test.go | 10 +- spinifex/agents/rds/agent/engine_mariadb.go | 45 +- .../agents/rds/agent/engine_mariadb_test.go | 54 ++- spinifex/agents/rds/agent/engine_postgres.go | 19 +- spinifex/agents/rds/agent/engine_test.go | 4 +- spinifex/agents/rds/agent/enginecatalog.go | 41 ++ .../rds/agent/enginecatalog_fake_test.go | 68 +++ spinifex/agents/rds/agent/parameters.go | 7 +- spinifex/daemon/daemon.go | 3 +- .../rds => domains/rds/engine}/engine.go | 87 ++-- spinifex/domains/rds/engine/engine_test.go | 288 ++++++++++++ .../rds/engine/mariadb.go} | 6 +- spinifex/domains/rds/engine/mariadb_test.go | 437 ++++++++++++++++++ .../rds/engine/parameters.go} | 60 +-- .../rds/engine/parameters_test.go} | 229 +++++++-- .../rds/engine/postgres.go} | 6 +- spinifex/domains/rds/engine/selection.go | 144 ++++++ spinifex/domains/rds/engine/selection_test.go | 193 ++++++++ spinifex/domains/rds/engine/sizing.go | 107 +++++ .../rds => domains/rds/engine}/sizing_test.go | 22 +- .../domains/rds/engine/testsizing_test.go | 17 + spinifex/gateway/rds/enginecatalog.go | 13 +- spinifex/gateway/rds/enginecatalog_test.go | 18 +- spinifex/handlers/rds/catalog.go | 141 +----- spinifex/handlers/rds/catalog_test.go | 205 +------- .../engine_catalogue_characterization_test.go | 253 +--------- spinifex/handlers/rds/engine_mariadb_test.go | 434 +---------------- spinifex/handlers/rds/engine_test.go | 135 ------ spinifex/handlers/rds/enginefamily_test.go | 194 ++------ .../handlers/rds/enginetest_helpers_test.go | 48 ++ spinifex/handlers/rds/lifecycle.go | 5 +- spinifex/handlers/rds/modify.go | 7 +- spinifex/handlers/rds/parametergroup.go | 88 ++-- spinifex/handlers/rds/parametergroup_test.go | 20 +- spinifex/handlers/rds/pending.go | 5 +- spinifex/handlers/rds/pending_test.go | 12 +- spinifex/handlers/rds/replace.go | 2 +- spinifex/handlers/rds/restore.go | 9 +- spinifex/handlers/rds/service.go | 6 +- spinifex/handlers/rds/sizing.go | 64 +-- spinifex/handlers/rds/snapshot.go | 5 +- spinifex/handlers/rds/tlsenforcement_test.go | 5 +- spinifex/handlers/rds/validate.go | 51 +- spinifex/handlers/rds/validate_test.go | 34 ++ 51 files changed, 2110 insertions(+), 1617 deletions(-) create mode 100644 spinifex/agents/rds/agent/enginecatalog.go create mode 100644 spinifex/agents/rds/agent/enginecatalog_fake_test.go rename spinifex/{handlers/rds => domains/rds/engine}/engine.go (82%) create mode 100644 spinifex/domains/rds/engine/engine_test.go rename spinifex/{handlers/rds/engine_mariadb.go => domains/rds/engine/mariadb.go} (99%) create mode 100644 spinifex/domains/rds/engine/mariadb_test.go rename spinifex/{handlers/rds/paramcatalog.go => domains/rds/engine/parameters.go} (86%) rename spinifex/{handlers/rds/paramcatalog_test.go => domains/rds/engine/parameters_test.go} (55%) rename spinifex/{handlers/rds/engine_postgres.go => domains/rds/engine/postgres.go} (99%) create mode 100644 spinifex/domains/rds/engine/selection.go create mode 100644 spinifex/domains/rds/engine/selection_test.go create mode 100644 spinifex/domains/rds/engine/sizing.go rename spinifex/{handlers/rds => domains/rds/engine}/sizing_test.go (77%) create mode 100644 spinifex/domains/rds/engine/testsizing_test.go delete mode 100644 spinifex/handlers/rds/engine_test.go create mode 100644 spinifex/handlers/rds/enginetest_helpers_test.go create mode 100644 spinifex/handlers/rds/validate_test.go diff --git a/cmd/rds-agent/main.go b/cmd/rds-agent/main.go index 095b40e0d..2a4e588d6 100644 --- a/cmd/rds-agent/main.go +++ b/cmd/rds-agent/main.go @@ -15,15 +15,62 @@ import ( _ "github.com/mulgadc/bluebottle/pkg/fipsboot" rdsagent "github.com/mulgadc/spinifex/spinifex/agents/rds/agent" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" ) // version is the agent build version, reported at registration. // Overridable via -ldflags "-X main.version=...". var version = "dev" +// engineCatalogAdapter composes the agent's narrow EngineCatalog over the +// control plane's own engine package. This binary is the only place in the +// agent's dependency graph that imports it. +type engineCatalogAdapter struct { + engine rdsengine.Engine +} + +var _ rdsagent.EngineCatalog = engineCatalogAdapter{} + +func (a engineCatalogAdapter) OptionFileName(name string) string { + return a.engine.OptionFileName(name) +} + +func (a engineCatalogAdapter) LookupParameter(name string) (rdsagent.CatalogParameter, bool) { + spec, ok := a.engine.LookupParameter(name) + if !ok { + return rdsagent.CatalogParameter{}, false + } + return rdsagent.CatalogParameter{DataType: spec.DataType, Static: spec.ApplyType == rdsengine.ApplyTypeStatic}, true +} + +func (a engineCatalogAdapter) CatalogParameterNames() []string { + return a.engine.CatalogParameterNames() +} + +func (a engineCatalogAdapter) TLSEnforcementParameter() string { + return a.engine.TLSEnforcementParameter() +} + +func (a engineCatalogAdapter) ValidateMasterUsername(username string) error { + return a.engine.ValidateMasterUsername(username) +} + +func (a engineCatalogAdapter) ValidateUsernameNotReserved(username string) error { + return a.engine.ValidateUsernameNotReserved(username) +} + +func lookupEngineCatalog(name string) (rdsagent.EngineCatalog, error) { + e, err := rdsengine.LookupEngine(name) + if err != nil { + return nil, err + } + return engineCatalogAdapter{engine: e}, nil +} + func main() { cfg := rdsagent.LoadConfig(rdsagent.DefaultEnvFile) cfg.AgentVersion = version + cfg.EngineCatalog = lookupEngineCatalog ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer stop() diff --git a/docs/service-interfaces.yaml b/docs/service-interfaces.yaml index 1a1d65f1e..d0b0d7828 100644 --- a/docs/service-interfaces.yaml +++ b/docs/service-interfaces.yaml @@ -77,6 +77,7 @@ services: - spinifex/handlers/ecs - spinifex/handlers/rds - spinifex/domains/rds/subnetgroup + - spinifex/domains/rds/engine - spinifex/domains/ecs/taskdefinition - spinifex/handlers/eks - spinifex/domains/eks/access diff --git a/spinifex/agents/rds/agent/agent.go b/spinifex/agents/rds/agent/agent.go index 299a236e9..18418ea97 100644 --- a/spinifex/agents/rds/agent/agent.go +++ b/spinifex/agents/rds/agent/agent.go @@ -215,14 +215,27 @@ func newAgent(cfg Config, cp controlPlane, probe *engineProbe) (*Agent, error) { AgentVersion: cfg.AgentVersion, EngineVersion: cfg.EngineVersion, } - a := &Agent{ - cfg: cfg, id: id, cp: cp, probe: probe, - handoffWriter: writeHandoff, dataMountWaiter: waitForDataMount, - } eng, err := newEngine(cfg, execCommandRunner, execSessionRunner, probe) if err != nil { return nil, err } + // Resolved once here, after the baked engine is known good, so the handoff + // renders parameters under the same engine's startup spellings the + // implementation above was just built from. + if cfg.EngineCatalog == nil { + return nil, fmt.Errorf("this build wires no engine catalog lookup") + } + catalog, err := cfg.EngineCatalog(cfg.BakedEngine) + if err != nil { + return nil, fmt.Errorf("resolve the engine catalog for the boot handoff: %w", err) + } + a := &Agent{ + cfg: cfg, id: id, cp: cp, probe: probe, + handoffWriter: func(dir string, bootCfg *handlers_rds.GetDBBootstrapConfigOutput) error { + return writeHandoff(dir, bootCfg, catalog.OptionFileName) + }, + dataMountWaiter: waitForDataMount, + } a.engine = eng a.hb = newHeartbeater(cp, probe, a.engine, handlers_rds.HeartbeatInterval) a.cmd = newCommander(cp, newCommandRegistry(a.engine, newGuestStorage(cfg, execCommandRunner)), cfg.PollWait) diff --git a/spinifex/agents/rds/agent/bootstrap.go b/spinifex/agents/rds/agent/bootstrap.go index f06c7a9ff..a17a19dba 100644 --- a/spinifex/agents/rds/agent/bootstrap.go +++ b/spinifex/agents/rds/agent/bootstrap.go @@ -92,7 +92,7 @@ const ( // bootstrap.env is written last and renamed into place, so its appearance — // which rds-init waits on — means the whole handoff is complete. -func writeHandoff(dir string, cfg *handlers_rds.GetDBBootstrapConfigOutput) error { +func writeHandoff(dir string, cfg *handlers_rds.GetDBBootstrapConfigOutput, optionFileName func(name string) string) error { if cfg.MasterUsername == "" { return fmt.Errorf("bootstrap config carries no master username") } @@ -114,7 +114,7 @@ func writeHandoff(dir string, cfg *handlers_rds.GetDBBootstrapConfigOutput) erro return fmt.Errorf("secure handoff dir %s: %w", dir, err) } - params, err := renderParameters(cfg.Engine, cfg.Parameters) + params, err := renderParameters(optionFileName, cfg.Parameters) if err != nil { return err } @@ -193,10 +193,9 @@ func shellQuote(s string) string { // postgresql.conf syntax, which MariaDB's option files also parse. Values are // quoted so a space or unit suffix survives. Names are the engine's startup // spellings, not the customer's: the wrong one leaves the engine unable to boot. -func renderParameters(engineName string, params []handlers_rds.Parameter) (string, error) { - engine, err := handlers_rds.LookupEngine(engineName) - if err != nil { - return "", fmt.Errorf("render the resolved parameters: %w", err) +func renderParameters(optionFileName func(name string) string, params []handlers_rds.Parameter) (string, error) { + if optionFileName == nil { + return "", fmt.Errorf("render the resolved parameters: no engine catalog resolved") } var b strings.Builder b.WriteString("# Resolved parameter group, written by rds-agent.\n") @@ -204,7 +203,7 @@ func renderParameters(engineName string, params []handlers_rds.Parameter) (strin if p.Name == "" { continue } - fmt.Fprintf(&b, "%s = '%s'\n", engine.OptionFileName(p.Name), strings.ReplaceAll(p.Value, "'", "''")) + fmt.Fprintf(&b, "%s = '%s'\n", optionFileName(p.Name), strings.ReplaceAll(p.Value, "'", "''")) } return b.String(), nil } diff --git a/spinifex/agents/rds/agent/bootstrap_test.go b/spinifex/agents/rds/agent/bootstrap_test.go index 162aa9380..556c18e50 100644 --- a/spinifex/agents/rds/agent/bootstrap_test.go +++ b/spinifex/agents/rds/agent/bootstrap_test.go @@ -10,6 +10,17 @@ import ( handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" ) +// testPostgresOptionFileName resolves the real catalog's startup spellings, +// so a handoff test renders exactly what rds-init would. +func testPostgresOptionFileName(t *testing.T) func(string) string { + t.Helper() + catalog, err := testEngineCatalogLookup(enginePostgres) + if err != nil { + t.Fatalf("testEngineCatalogLookup(%s): %v", enginePostgres, err) + } + return catalog.OptionFileName +} + func TestWriteHandoff_WritesEveryFileRootOnly(t *testing.T) { dir := filepath.Join(t.TempDir(), "handoff") password := "s3cret" @@ -33,7 +44,7 @@ func TestWriteHandoff_WritesEveryFileRootOnly(t *testing.T) { }, ServingCertificate: "CERT", ServingPrivateKey: "KEY", - }) + }, testPostgresOptionFileName(t)) if err != nil { t.Fatalf("writeHandoff: %v", err) } @@ -90,7 +101,7 @@ func TestWriteHandoff_AttachOmitsPassword(t *testing.T) { ListenAddress: "10.0.1.20", ClientCIDR: "10.0.0.0/16", Port: 5432, - }); err != nil { + }, testPostgresOptionFileName(t)); err != nil { t.Fatalf("writeHandoff: %v", err) } @@ -124,7 +135,7 @@ func TestWriteHandoff_QuotesShellMetacharacters(t *testing.T) { ClientCIDR: "10.0.0.0/16", MasterUserPassword: &password, Port: 5432, - }); err != nil { + }, testPostgresOptionFileName(t)); err != nil { t.Fatalf("writeHandoff: %v", err) } @@ -147,7 +158,7 @@ func TestWriteHandoff_SkipsHalfATLSPair(t *testing.T) { ClientCIDR: "10.0.0.0/16", Port: 5432, ServingCertificate: "CERT", - }); err != nil { + }, testPostgresOptionFileName(t)); err != nil { t.Fatalf("writeHandoff: %v", err) } @@ -164,7 +175,7 @@ func TestWriteHandoff_RejectsConfigWithNoMasterUser(t *testing.T) { Mode: handlers_rds.BootstrapModeAttach, Engine: "postgres", Port: 5432, - }) + }, testPostgresOptionFileName(t)) if err == nil { t.Fatal("writeHandoff accepted a config with no master username, want an error") } @@ -188,7 +199,7 @@ func TestWriteHandoff_TightensExistingDirectory(t *testing.T) { ListenAddress: "10.0.1.20", ClientCIDR: "10.0.0.0/16", Port: 5432, - }); err != nil { + }, testPostgresOptionFileName(t)); err != nil { t.Fatalf("writeHandoff: %v", err) } if mode := statMode(t, dir); mode != handoffDirMode { @@ -244,7 +255,11 @@ func TestRenderParameters_UsesTheEngineStartupSpelling(t *testing.T) { {Name: "max_connections", Value: "85"}, } - rendered, err := renderParameters("mariadb", params) + catalog, err := testEngineCatalogLookup(engineMariaDB) + if err != nil { + t.Fatalf("testEngineCatalogLookup(%s): %v", engineMariaDB, err) + } + rendered, err := renderParameters(catalog.OptionFileName, params) if err != nil { t.Fatalf("renderParameters: %v", err) } @@ -260,7 +275,7 @@ func TestRenderParameters_UsesTheEngineStartupSpelling(t *testing.T) { } func TestRenderParameters_PostgresKeepsEveryName(t *testing.T) { - rendered, err := renderParameters("postgres", []handlers_rds.Parameter{ + rendered, err := renderParameters(testPostgresOptionFileName(t), []handlers_rds.Parameter{ {Name: "shared_buffers", Value: "262144"}, }) if err != nil { @@ -274,7 +289,7 @@ func TestRenderParameters_PostgresKeepsEveryName(t *testing.T) { // Failing closed beats writing a file the engine may refuse: the bad include // would already be on the data volume by the time the server rejected it. func TestRenderParameters_RefusesAnUnknownEngine(t *testing.T) { - if _, err := renderParameters("", nil); err == nil { - t.Fatal("renderParameters accepted a config naming no engine, want an error") + if _, err := renderParameters(nil, nil); err == nil { + t.Fatal("renderParameters accepted no engine catalog, want an error") } } diff --git a/spinifex/agents/rds/agent/config.go b/spinifex/agents/rds/agent/config.go index e4bcd4574..6db391f5d 100644 --- a/spinifex/agents/rds/agent/config.go +++ b/spinifex/agents/rds/agent/config.go @@ -79,6 +79,11 @@ type Config struct { // AgentVersion is the binary's build version, reported at registration; // the binary sets it, not the env file. AgentVersion string + + // EngineCatalog resolves the control plane's metadata for the engine + // this image bakes. Set by the binary's entry point, which is the only + // place in this agent that imports the catalog that defines it. + EngineCatalog EngineCatalogLookup } // LoadConfig reads the cloud-init env file, then lets real env vars override. diff --git a/spinifex/agents/rds/agent/config_test.go b/spinifex/agents/rds/agent/config_test.go index 1f1361e99..62bec708a 100644 --- a/spinifex/agents/rds/agent/config_test.go +++ b/spinifex/agents/rds/agent/config_test.go @@ -17,7 +17,9 @@ func testLoadConfig(t *testing.T, engine string) Config { t.Fatalf("write the engine stamp: %v", err) } t.Setenv("RDS_ENGINE_FILE", stamp) - return LoadConfig(filepath.Join(t.TempDir(), "absent.env")) + cfg := LoadConfig(filepath.Join(t.TempDir(), "absent.env")) + cfg.EngineCatalog = testEngineCatalogLookup + return cfg } func TestLoadConfig_Defaults(t *testing.T) { diff --git a/spinifex/agents/rds/agent/defaultparams_test.go b/spinifex/agents/rds/agent/defaultparams_test.go index fb69fbe16..fb74a4006 100644 --- a/spinifex/agents/rds/agent/defaultparams_test.go +++ b/spinifex/agents/rds/agent/defaultparams_test.go @@ -10,6 +10,7 @@ import ( "strings" "testing" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" ) @@ -25,16 +26,17 @@ const mariadbDefaultParametersFixture = "../../../../scripts/images/rds-mariadb/ // mariadbd: a catalog that grew a name the server refuses at startup would // otherwise reach a customer as an instance that never boots. func TestMariaDBDefaultParametersFixture_MatchesTheCatalog(t *testing.T) { - engine, err := handlers_rds.LookupEngine("mariadb") + engine, err := rdsengine.LookupEngine("mariadb") if err != nil { t.Fatalf("LookupEngine: %v", err) } - class := handlers_rds.SmallestInstanceClass() - params, err := engine.ResolveEffectiveParameters(class, nil) + sizing := handlers_rds.InstanceSizing() + class := sizing.SmallestInstanceClass() + settings, err := engine.ResolveEffectiveParameters(sizing, class, nil) if err != nil { t.Fatalf("ResolveEffectiveParameters(%s): %v", class, err) } - body, err := renderParameters("mariadb", params) + body, err := renderParameters(engine.OptionFileName, settingsToParameters(settings)) if err != nil { t.Fatalf("renderParameters: %v", err) } diff --git a/spinifex/agents/rds/agent/engine_mariadb.go b/spinifex/agents/rds/agent/engine_mariadb.go index 5017d4191..ab6cf7e7b 100644 --- a/spinifex/agents/rds/agent/engine_mariadb.go +++ b/spinifex/agents/rds/agent/engine_mariadb.go @@ -37,9 +37,13 @@ type controlPlaneRules struct { // connection. Held rather than spelled out here, so the guest derives // enforcement from the same key the control plane resolves it under. tlsEnforcementParameter string + // The engine's startup spelling for a catalog name, the forward + // direction of catalogName above. Used to render a resolved set rather + // than to classify one read back. + optionFileName func(name string) string } -func controlPlaneRulesFrom(meta handlers_rds.Engine) controlPlaneRules { +func controlPlaneRulesFrom(meta EngineCatalog) controlPlaneRules { // Built once per engine: everything read back out of a generated file has to // return to the catalog's namespace before it is classified or reported, or a // startup spelling would read as an unknown name. @@ -56,7 +60,7 @@ func controlPlaneRulesFrom(meta handlers_rds.Engine) controlPlaneRules { spec, ok := meta.LookupParameter(name) // A name the catalog does not carry cannot be shown to have been adopted // without a restart, and is never issued as a live SET GLOBAL either. - return !ok || spec.ApplyType == handlers_rds.ApplyTypeStatic + return !ok || spec.Static }, catalogName: func(optionFileName string) string { if name, ok := catalogNames[optionFileName]; ok { @@ -72,6 +76,7 @@ func controlPlaneRulesFrom(meta handlers_rds.Engine) controlPlaneRules { return spec.DataType }, tlsEnforcementParameter: meta.TLSEnforcementParameter(), + optionFileName: meta.OptionFileName, } } @@ -127,7 +132,10 @@ const ( // of this engine, so a build whose control plane does not offer MariaDB refuses // to run this implementation rather than inventing a definition for it. func newMariaDBEngineFromCatalog(cfg Config, run commandRunner, startSess sessionRunner, probe *engineProbe) (engine, error) { - meta, err := handlers_rds.LookupEngine(engineMariaDB) + if cfg.EngineCatalog == nil { + return nil, fmt.Errorf("this build wires no engine catalog lookup") + } + meta, err := cfg.EngineCatalog(engineMariaDB) if err != nil { return nil, fmt.Errorf("this image bakes %s, which this build's control plane does not offer: %w", engineMariaDB, err) } @@ -148,13 +156,13 @@ func newMariaDBEngine(cfg Config, rules controlPlaneRules, run commandRunner, st params: parameterStore{ // The mount point rather than the datadir one level inside it: the // include directory has to outlive the sweep a failed bootstrap runs. - dir: filepath.Join(cfg.DataMount, "conf.d"), - installed: mariadbParametersFile, - lastGood: mariadbLastGoodFile, - serving: mariadbServingFile, - header: mariadbParametersHeader, - osUser: cfg.EngineUser, - engine: engineMariaDB, + dir: filepath.Join(cfg.DataMount, "conf.d"), + installed: mariadbParametersFile, + lastGood: mariadbLastGoodFile, + serving: mariadbServingFile, + header: mariadbParametersHeader, + osUser: cfg.EngineUser, + optionFileName: rules.optionFileName, }, repairTimeout: parameterRepairTimeout, repairPoll: parameterRepairPoll, @@ -328,24 +336,33 @@ var mariadbBooleanValues = map[string]string{ "on": "ON", "off": "OFF", "true": "ON", "false": "OFF", "1": "ON", "0": "OFF", } +// Agent-owned data-type spellings, equal in value to the control plane's own +// so a CatalogParameter crossing the boundary needs no translation, but held +// here so this file depends on no catalog type for them. +const ( + paramTypeInteger = "integer" + paramTypeReal = "real" + paramTypeBoolean = "boolean" +) + // One SET GLOBAL right-hand side. MariaDB refuses a quoted literal for a numeric // or boolean with ER_WRONG_TYPE_FOR_VAR, so the catalog's data type picks the // form; re-rendering from the parsed value is what keeps it safe to interpolate. func mariadbSetValue(dataType, value string) (string, error) { switch dataType { - case handlers_rds.ParamTypeInteger: + case paramTypeInteger: n, err := strconv.ParseInt(strings.TrimSpace(value), 10, 64) if err != nil { return "", fmt.Errorf("value %q is not an integer", value) } return strconv.FormatInt(n, 10), nil - case handlers_rds.ParamTypeReal: + case paramTypeReal: f, err := strconv.ParseFloat(strings.TrimSpace(value), 64) if err != nil { return "", fmt.Errorf("value %q is not a real number", value) } return strconv.FormatFloat(f, 'g', -1, 64), nil - case handlers_rds.ParamTypeBoolean: + case paramTypeBoolean: if literal, ok := mariadbBooleanValues[strings.ToLower(strings.TrimSpace(value))]; ok { return literal, nil } @@ -536,7 +553,7 @@ func (e *mariadbEngine) setTLSEnforcement(ctx context.Context, enforce bool) err } // Rendered the way the batch renders every other boolean, so the statement the // server sees does not depend on which path issued it. - literal, err := mariadbSetValue(handlers_rds.ParamTypeBoolean, value) + literal, err := mariadbSetValue(paramTypeBoolean, value) if err != nil { return fmt.Errorf("render %s: %w", name, err) } diff --git a/spinifex/agents/rds/agent/engine_mariadb_test.go b/spinifex/agents/rds/agent/engine_mariadb_test.go index ab0abacc9..204fb6639 100644 --- a/spinifex/agents/rds/agent/engine_mariadb_test.go +++ b/spinifex/agents/rds/agent/engine_mariadb_test.go @@ -17,6 +17,7 @@ import ( "testing" "time" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" ) @@ -26,25 +27,25 @@ import ( // prevent. func testMariaDBRules() controlPlaneRules { applyTypes := map[string]string{ - "innodb_buffer_pool_size": handlers_rds.ApplyTypeStatic, - "innodb_log_file_size": handlers_rds.ApplyTypeStatic, - "max_connections": handlers_rds.ApplyTypeDynamic, - "long_query_time": handlers_rds.ApplyTypeDynamic, - "time_zone": handlers_rds.ApplyTypeDynamic, + "innodb_buffer_pool_size": rdsengine.ApplyTypeStatic, + "innodb_log_file_size": rdsengine.ApplyTypeStatic, + "max_connections": rdsengine.ApplyTypeDynamic, + "long_query_time": rdsengine.ApplyTypeDynamic, + "time_zone": rdsengine.ApplyTypeDynamic, - "innodb_adaptive_hash_index": handlers_rds.ApplyTypeDynamic, - "log_output": handlers_rds.ApplyTypeDynamic, - "require_secure_transport": handlers_rds.ApplyTypeDynamic, + "innodb_adaptive_hash_index": rdsengine.ApplyTypeDynamic, + "log_output": rdsengine.ApplyTypeDynamic, + "require_secure_transport": rdsengine.ApplyTypeDynamic, } dataTypes := map[string]string{ - "innodb_buffer_pool_size": handlers_rds.ParamTypeInteger, - "innodb_log_file_size": handlers_rds.ParamTypeInteger, - "max_connections": handlers_rds.ParamTypeInteger, - "long_query_time": handlers_rds.ParamTypeReal, - "time_zone": handlers_rds.ParamTypeString, - "innodb_adaptive_hash_index": handlers_rds.ParamTypeBoolean, - "log_output": handlers_rds.ParamTypeEnum, - "require_secure_transport": handlers_rds.ParamTypeBoolean, + "innodb_buffer_pool_size": rdsengine.ParamTypeInteger, + "innodb_log_file_size": rdsengine.ParamTypeInteger, + "max_connections": rdsengine.ParamTypeInteger, + "long_query_time": rdsengine.ParamTypeReal, + "time_zone": rdsengine.ParamTypeString, + "innodb_adaptive_hash_index": rdsengine.ParamTypeBoolean, + "log_output": rdsengine.ParamTypeEnum, + "require_secure_transport": rdsengine.ParamTypeBoolean, } reserved := []string{"root", "mysql", "mariadb.sys", "rdsadmin", "public"} @@ -63,7 +64,7 @@ func testMariaDBRules() controlPlaneRules { }, isStatic: func(name string) bool { applyType, ok := applyTypes[name] - return !ok || applyType == handlers_rds.ApplyTypeStatic + return !ok || applyType == rdsengine.ApplyTypeStatic }, catalogName: func(optionFileName string) string { if optionFileName == "default_time_zone" { @@ -73,6 +74,12 @@ func testMariaDBRules() controlPlaneRules { }, dataType: func(name string) string { return dataTypes[name] }, tlsEnforcementParameter: "require_secure_transport", + optionFileName: func(name string) string { + if name == "time_zone" { + return "default_time_zone" + } + return name + }, } } @@ -137,8 +144,8 @@ func TestControlPlaneRulesFrom_TakesTheClassificationFromTheCatalog(t *testing.T if !ok { t.Fatalf("the catalog no longer carries %s", name) } - if (spec.ApplyType == handlers_rds.ApplyTypeStatic) != want { - t.Fatalf("%s is %s in the catalog, so this test is asserting the wrong way round", name, spec.ApplyType) + if spec.Static != want { + t.Fatalf("%s is static=%v in the catalog, so this test is asserting the wrong way round", name, spec.Static) } if got := rules.isStatic(name); got != want { t.Errorf("isStatic(%s) = %v, want %v", name, got, want) @@ -1122,14 +1129,15 @@ func TestMariaDBEngine_PendingRestartReportsCatalogNames(t *testing.T) { // surface as a failed apply on a live instance rather than as a test failure on // the change that introduced it. func TestMariaDBEngine_EveryResolvedDynamicValueRenders(t *testing.T) { - meta, err := handlers_rds.LookupEngine("mariadb") + meta, err := rdsengine.LookupEngine("mariadb") if err != nil { t.Fatalf("LookupEngine: %v", err) } - rules := controlPlaneRulesFrom(meta) + rules := controlPlaneRulesFrom(catalogFromEngine{engine: meta}) + sizing := handlers_rds.InstanceSizing() - for _, class := range handlers_rds.SupportedInstanceClasses() { - params, err := meta.ResolveEffectiveParameters(class, nil) + for _, class := range rdsengine.SupportedInstanceClasses() { + params, err := meta.ResolveEffectiveParameters(sizing, class, nil) if err != nil { t.Fatalf("ResolveEffectiveParameters(%s): %v", class, err) } diff --git a/spinifex/agents/rds/agent/engine_postgres.go b/spinifex/agents/rds/agent/engine_postgres.go index b295e9112..5095e920c 100644 --- a/spinifex/agents/rds/agent/engine_postgres.go +++ b/spinifex/agents/rds/agent/engine_postgres.go @@ -23,7 +23,7 @@ type postgresEngine struct { // The control plane's own metadata for this engine, resolved once at // startup: the live password apply runs as the cluster superuser, so it // re-checks the role name against the same reserved set the API validates. - meta handlers_rds.Engine + meta EngineCatalog run commandRunner startSess sessionRunner psql string @@ -108,14 +108,17 @@ hostnossl all all ::/0 reject // The layout's factory resolves the control plane metadata during startup. func newPostgresEngineFromCatalog(cfg Config, run commandRunner, startSess sessionRunner, probe *engineProbe) (engine, error) { - meta, err := handlers_rds.LookupEngine(enginePostgres) + if cfg.EngineCatalog == nil { + return nil, fmt.Errorf("this build wires no engine catalog lookup") + } + meta, err := cfg.EngineCatalog(enginePostgres) if err != nil { return nil, fmt.Errorf("this image bakes %s, which this build's control plane does not offer: %w", enginePostgres, err) } return newPostgresEngine(cfg, meta, run, startSess, probe), nil } -func newPostgresEngine(cfg Config, meta handlers_rds.Engine, run commandRunner, startSess sessionRunner, probe *engineProbe) *postgresEngine { +func newPostgresEngine(cfg Config, meta EngineCatalog, run commandRunner, startSess sessionRunner, probe *engineProbe) *postgresEngine { return &postgresEngine{ meta: meta, run: run, @@ -129,11 +132,11 @@ func newPostgresEngine(cfg Config, meta handlers_rds.Engine, run commandRunner, parameterManager: parameterManager{ probe: probe, params: parameterStore{ - dir: filepath.Join(cfg.EngineDataDir, "conf.d"), - installed: postgresParametersFile, - lastGood: postgresLastGoodFile, - osUser: cfg.EngineUser, - engine: enginePostgres, + dir: filepath.Join(cfg.EngineDataDir, "conf.d"), + installed: postgresParametersFile, + lastGood: postgresLastGoodFile, + osUser: cfg.EngineUser, + optionFileName: meta.OptionFileName, }, repairTimeout: parameterRepairTimeout, repairPoll: parameterRepairPoll, diff --git a/spinifex/agents/rds/agent/engine_test.go b/spinifex/agents/rds/agent/engine_test.go index c964348c7..ee2eb3ea5 100644 --- a/spinifex/agents/rds/agent/engine_test.go +++ b/spinifex/agents/rds/agent/engine_test.go @@ -140,9 +140,9 @@ func (r *recordingRunner) run(_ context.Context, c command) (string, error) { return r.out, r.err } -func testPostgresEngineMeta(t *testing.T) handlers_rds.Engine { +func testPostgresEngineMeta(t *testing.T) EngineCatalog { t.Helper() - meta, err := handlers_rds.LookupEngine(enginePostgres) + meta, err := testEngineCatalogLookup(enginePostgres) if err != nil { t.Fatalf("LookupEngine(%s): %v", enginePostgres, err) } diff --git a/spinifex/agents/rds/agent/enginecatalog.go b/spinifex/agents/rds/agent/enginecatalog.go new file mode 100644 index 000000000..37db5447d --- /dev/null +++ b/spinifex/agents/rds/agent/enginecatalog.go @@ -0,0 +1,41 @@ +package agent + +// CatalogParameter is the agent-owned shape of the control plane's knowledge +// about one parameter: enough to render, classify and read it back without +// this package depending on the catalog that defines it. +type CatalogParameter struct { + // The catalog's data type for the setting, spelled the same as the + // control plane's own ("integer", "real", "boolean", ...) but held here + // as a plain string so no shared constant crosses the boundary. + DataType string + // Whether only a restart adopts the setting. Already customer-facing and + // authoritative, since the API refuses ApplyMethod=immediate on one. + Static bool +} + +// EngineCatalog is exactly the control plane's per-engine metadata this +// agent consumes, kept narrow so this package does not import the catalog +// that defines it. The production implementation is composed at the +// binary's entry point and reaches here through Config.EngineCatalog. +type EngineCatalog interface { + // The engine's startup spelling for a catalog parameter name. + OptionFileName(name string) string + // The catalog's own metadata for a parameter name, or false for one the + // catalog does not carry. + LookupParameter(name string) (CatalogParameter, bool) + // Every name the catalog defines, for a read-back that has to translate + // a generated file's spellings back to it. + CatalogParameterNames() []string + // The setting name that requires TLS of a client connection. + TLSEnforcementParameter() string + // The whole master-username rule: length, characters and reserved names. + ValidateMasterUsername(username string) error + // Only the reserved-name half, for a rotation that must not hand the + // customer an account the engine itself relies on. + ValidateUsernameNotReserved(username string) error +} + +// EngineCatalogLookup resolves the control plane's metadata for a named +// engine. Nil in a build that wires none, which newEngine refuses rather +// than guessing at a definition. +type EngineCatalogLookup func(name string) (EngineCatalog, error) diff --git a/spinifex/agents/rds/agent/enginecatalog_fake_test.go b/spinifex/agents/rds/agent/enginecatalog_fake_test.go new file mode 100644 index 000000000..78f5c39df --- /dev/null +++ b/spinifex/agents/rds/agent/enginecatalog_fake_test.go @@ -0,0 +1,68 @@ +package agent + +//test:in-package — covers the adapter tests use to reach the production +// catalog without this package importing it outside _test.go files. + +import ( + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" + handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" +) + +// catalogFromEngine adapts a real engine-package Engine to this package's +// own narrow EngineCatalog, so tests exercise the production catalog's +// behaviour without the agent's production code importing the package that +// defines it. +type catalogFromEngine struct { + engine rdsengine.Engine +} + +var _ EngineCatalog = catalogFromEngine{} + +func (c catalogFromEngine) OptionFileName(name string) string { + return c.engine.OptionFileName(name) +} + +func (c catalogFromEngine) LookupParameter(name string) (CatalogParameter, bool) { + spec, ok := c.engine.LookupParameter(name) + if !ok { + return CatalogParameter{}, false + } + return CatalogParameter{DataType: spec.DataType, Static: spec.ApplyType == rdsengine.ApplyTypeStatic}, true +} + +func (c catalogFromEngine) CatalogParameterNames() []string { + return c.engine.CatalogParameterNames() +} + +func (c catalogFromEngine) TLSEnforcementParameter() string { + return c.engine.TLSEnforcementParameter() +} + +func (c catalogFromEngine) ValidateMasterUsername(username string) error { + return c.engine.ValidateMasterUsername(username) +} + +func (c catalogFromEngine) ValidateUsernameNotReserved(username string) error { + return c.engine.ValidateUsernameNotReserved(username) +} + +// testEngineCatalogLookup is the EngineCatalogLookup tests wire into Config, +// backed by the real catalog so fixtures and golden files stay meaningful. +func testEngineCatalogLookup(name string) (EngineCatalog, error) { + e, err := rdsengine.LookupEngine(name) + if err != nil { + return nil, err + } + return catalogFromEngine{engine: e}, nil +} + +// settingsToParameters converts the engine package's own result type to the +// wire type the agent's command handlers carry, at the boundary test code +// crosses it. +func settingsToParameters(settings []rdsengine.Setting) []handlers_rds.Parameter { + out := make([]handlers_rds.Parameter, 0, len(settings)) + for _, s := range settings { + out = append(out, handlers_rds.Parameter{Name: s.Name, Value: s.Value}) + } + return out +} diff --git a/spinifex/agents/rds/agent/parameters.go b/spinifex/agents/rds/agent/parameters.go index b6f8b41ef..a1f987bb9 100644 --- a/spinifex/agents/rds/agent/parameters.go +++ b/spinifex/agents/rds/agent/parameters.go @@ -32,8 +32,9 @@ type parameterStore struct { // The engine parses its own configuration, so a root-owned file would be // unreadable to it. osUser string - // Names the engine whose option-file spellings these files are written in. - engine string + // The engine's own startup spelling for a catalog name, resolved once at + // construction from the control plane's own metadata for this engine. + optionFileName func(name string) string } func (s parameterStore) installedPath() string { return filepath.Join(s.dir, s.installed) } @@ -43,7 +44,7 @@ func (s parameterStore) servingPath() string { return filepath.Join(s.dir, s.s // Byte-for-byte what rds-init writes for the same set, so the copies compared // against it are comparing values rather than formatting. func (s parameterStore) render(params []handlers_rds.Parameter) ([]byte, error) { - body, err := renderParameters(s.engine, params) + body, err := renderParameters(s.optionFileName, params) if err != nil { return nil, err } diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index 0ea1c616e..7ef3f3e5f 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -70,6 +70,7 @@ import ( "github.com/mulgadc/spinifex/spinifex/domains/network/host" "github.com/mulgadc/spinifex/spinifex/domains/ochre" ochrevector "github.com/mulgadc/spinifex/spinifex/domains/ochre/vector" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" "github.com/mulgadc/spinifex/spinifex/foundation/state/clustersize" "github.com/mulgadc/spinifex/spinifex/foundation/state/kvutil" @@ -2085,7 +2086,7 @@ func (d *Daemon) startCluster() error { // whichever node the queue group picks, so a node without it would make the // first boot of a DB instance fail intermittently rather than not at all. d.rdsService, err = initServiceWithRetry("RDS service", func() (*handlers_rds.Service, error) { - if registryErr := handlers_rds.ValidateEngineRegistry(); registryErr != nil { + if registryErr := rdsengine.ValidateEngineRegistry(); registryErr != nil { return nil, registryErr } deps, depsErr := d.buildRDSDeps() diff --git a/spinifex/handlers/rds/engine.go b/spinifex/domains/rds/engine/engine.go similarity index 82% rename from spinifex/handlers/rds/engine.go rename to spinifex/domains/rds/engine/engine.go index 41fa214f6..09eed7b64 100644 --- a/spinifex/handlers/rds/engine.go +++ b/spinifex/domains/rds/engine/engine.go @@ -1,4 +1,8 @@ -package handlers_rds +// Package engine owns the RDS engine catalogue: the registry of supported +// engines, their parameter catalogues, and the size-derived sizing table a +// parameter default is evaluated against. It is the control-plane half of +// the engine seam; the in-guest half lives in rds-init and rds-agent. +package engine import ( "errors" @@ -9,6 +13,14 @@ import ( "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" ) +// Setting is one resolved name/value pair. Separate from any caller's own +// parameter type, so this package reports a result without depending on how +// a caller stores or renders it. +type Setting struct { + Name string + Value string +} + // Engine is the control-plane half of the engine seam: what CreateDBInstance needs to validate a // request and assemble a bootstrap config. The in-guest half — initdb, quiesce, live password apply — // lives in rds-init and rds-agent. @@ -41,7 +53,7 @@ type Engine struct { catalog map[string]ParameterSpec // Cross-parameter checks a resolved set must satisfy, which are the // combinations the engine itself would refuse to start under. - validateCombinations func([]Parameter) error + validateCombinations func([]Setting) error // The engine's own name for the setting that requires TLS of a client // connection, which is AWS's name for it. Named here so the control plane and // the guest agree on the key without either spelling out an engine. @@ -65,6 +77,12 @@ var engines = map[string]Engine{ // and engine are 1:1 by construction, so one registry serves both. var enginesByFamily, engineRegistryValidationErr = indexEnginesByFamily(engines) +// The prefix AWS reserves for the groups the service itself owns. Duplicated +// against handlers_rds's own copy deliberately: each package's constant backs +// a different check (this one a name this package derives, that one a name a +// caller recognises), and neither may reach across the boundary to the other. +const defaultParameterGroupPrefix = "default." + // ValidateEngineRegistry reports invalid built-in engine metadata before RDS starts. func ValidateEngineRegistry() error { if err := validateParameterCatalogs(); err != nil { @@ -134,6 +152,18 @@ func (e Engine) ParameterGroupFamily() string { return e.Name + e.MajorVersion } +// CrashRecoveryNote returns what a snapshot taken without a quiesce actually +// recovers on restore, for the event the control plane records alongside it. +func (e Engine) CrashRecoveryNote() string { + return e.crashRecoveryNote +} + +// UncleanStopNote returns the same guarantee stated for the next start rather +// than for a restore, for the event an unclean stop records. +func (e Engine) UncleanStopNote() string { + return e.uncleanStopNote +} + // LookupEngine resolves an engine name case-insensitively. An unknown engine is rejected with // InvalidParameterValue at validation, before any volume or ENI exists. func LookupEngine(name string) (Engine, error) { @@ -150,10 +180,10 @@ func SupportedEngines() []string { return slices.Sorted(maps.Keys(engines)) } -// The engine a parameter group's family belongs to, for the paths that hold a -// group and no instance. A family naming no engine is a group written by a build -// that offered an engine this one does not. -func engineForFamily(family string) (Engine, error) { +// EngineForFamily returns the engine a parameter group's family belongs to, for the paths that hold a +// group and no instance. A family naming no engine is a group written by a build that offered an +// engine this one does not. +func EngineForFamily(family string) (Engine, error) { engine, ok := enginesByFamily[normaliseFamily(family)] if !ok { return Engine{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, @@ -163,6 +193,17 @@ func engineForFamily(family string) (Engine, error) { return engine, nil } +// EngineForDefaultParameterGroup returns the engine whose implicit default group carries this name, so +// an unrecognised default.* name is a not-found rather than a group that resolves to nothing. +func EngineForDefaultParameterGroup(name string) (Engine, bool) { + for _, engine := range engines { + if engine.DefaultParameterGroupName() == name { + return engine, true + } + } + return Engine{}, false +} + // SupportedParameterGroupFamilies returns every engine's parameter group family name (e.g. postgres18), // sorted. func SupportedParameterGroupFamilies() []string { @@ -250,37 +291,9 @@ func validateIdentifier(field, value string, maxLen int, allowEmpty bool) error return nil } -// ValidateMasterUserPassword enforces the bounds and printable-ASCII range AWS accepts. The password is -// never inspected beyond this and never stored in cleartext past the first bootstrap fetch. -func ValidateMasterUserPassword(password string) error { - switch { - case password == "": - return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "MasterUserPassword is required") - case len(password) < minMasterPasswordLen || len(password) > maxMasterPasswordLen: - return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, - "MasterUserPassword must be between %d and %d characters", minMasterPasswordLen, maxMasterPasswordLen) - } - for _, r := range password { - // A control character would also survive the bootstrap handoff and defeat - // the line-oriented redaction that keeps the password off the guest - // console, so the range is refused here rather than sanitised there. - if r < 0x20 || r > 0x7e { - return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, - "MasterUserPassword may only contain printable ASCII characters") - } - if r == '/' || r == '"' || r == '@' || r == ' ' { - return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, - "MasterUserPassword may not contain '/', '\"', '@' or spaces") - } - } - return nil -} - -const ( - minMasterPasswordLen = 8 - maxMasterPasswordLen = 128 -) - +// isLetter and isDigit are duplicated against handlers_rds's own copy +// deliberately: that one backs identifier checks this package does not own, +// and neither may reach across the boundary to the other. func isLetter(r rune) bool { return (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') } diff --git a/spinifex/domains/rds/engine/engine_test.go b/spinifex/domains/rds/engine/engine_test.go new file mode 100644 index 000000000..f54da6f30 --- /dev/null +++ b/spinifex/domains/rds/engine/engine_test.go @@ -0,0 +1,288 @@ +package engine + +//test:in-package — reaches unexported fields (catalog, maxUsernameLen, +// validateDBName, crashRecoveryNote, uncleanStopNote, tlsEnforcementParameter) +// and the unexported engines/enginesByFamily registries, none of which this +// package exports. + +import ( + "strings" + "testing" + + "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLookupEngine(t *testing.T) { + t.Parallel() + _, err := LookupEngine("PostgreSQL") + require.Error(t, err, "the AWS engine identifier is 'postgres', not the product name") + + engine, err := LookupEngine(" POSTGRES ") + require.NoError(t, err, "the engine name is matched case-insensitively and trimmed") + assert.Equal(t, "postgres", engine.Name) + assert.Equal(t, int64(5432), engine.DefaultPort) + assert.Equal(t, "default.postgres18", engine.DefaultParameterGroupName()) + + _, err = LookupEngine("mysql") + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + assert.Equal(t, []string{"mariadb", "postgres"}, SupportedEngines()) +} + +// A version other than the pinned one would be served by an image that is +// not the one asked for, so it is rejected rather than quietly substituted. +func TestEngineValidateVersion(t *testing.T) { + t.Parallel() + engine, err := LookupEngine("postgres") + require.NoError(t, err) + + for _, version := range []string{"", "18"} { + assert.NoError(t, engine.ValidateVersion(version), "version %q", version) + } + for _, version := range []string{"17", "16.2", "18.1", "18.4", " 18 ", "19", "latest"} { + assert.Error(t, engine.ValidateVersion(version), "version %q", version) + } + + // The pin, not the request, is what the record and the AMI lookup carry. + assert.Equal(t, "18", engine.EngineVersion()) +} + +// The AWS engine identifier is "postgres", not the marketing name a customer +// might reasonably try first. +func TestLookupEngine_RejectsProductNameWithExactCode(t *testing.T) { + t.Parallel() + _, err := LookupEngine("PostgreSQL") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) +} + +func TestEngineValidateVersion_UnsupportedVersionCarriesExactCode(t *testing.T) { + t.Parallel() + engine, err := LookupEngine("postgres") + require.NoError(t, err) + + err = engine.ValidateVersion("19") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) +} + +func TestEngineValidateMasterUsername(t *testing.T) { + engine, err := LookupEngine("postgres") + require.NoError(t, err) + + for _, username := range []string{"appuser", "app_user1", "Orders"} { + assert.NoError(t, engine.ValidateMasterUsername(username), "username %q", username) + } + + cases := map[string]string{ + "empty": "", + "leading digit": "1user", + "hyphen": "app-user", + "too long": strings.Repeat("a", enginePostgres.maxUsernameLen+1), + "reserved": "rdsadmin", + "reserved cased": "RDSAdmin", + // The cluster superuser and the group role the master's administrative + // privileges come through. Both exist by the time the master is created, + // so a collision would surface as a failed bootstrap inside the guest. + "cluster superuser": "postgres", + "group role": "rds_superuser", + // postgres reserves the pg_ prefix for its own internal roles, so a master + // role taking one would collide inside the engine, not at the API. + "reserved prefix": "pg_backup", + } + for name, username := range cases { + t.Run(name, func(t *testing.T) { + err := engine.ValidateMasterUsername(username) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + }) + } +} + +// The name is interpolated into a CREATE DATABASE in the guest, so the rule is +// narrower than the engine's own: it is what makes the interpolation safe. +func TestEngineValidateDBName(t *testing.T) { + engine, err := LookupEngine("postgres") + require.NoError(t, err) + + // Empty is not a rejection: AWS creates no initial database when none is named. + for _, name := range []string{"", "orders", "order_db1", "Orders"} { + assert.NoError(t, engine.ValidateDBName(name), "DBName %q", name) + } + + cases := map[string]string{ + "hyphen": "my-db", + "leading digit": "1db", + "space": "my db", + "dot": "my.db", + // MariaDB maps a database name onto a directory name, so these are + // structurally dangerous there rather than merely awkward. + "slash": "my/db", + "backslash": `my\db`, + "trailing space": "orders ", + "quote": "orders'", + "too long": strings.Repeat("a", 64), + } + for name, dbName := range cases { + t.Run(name, func(t *testing.T) { + err := engine.ValidateDBName(dbName) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + }) + } +} + +// EngineForFamily and normaliseFamily fold case and trim surrounding +// whitespace before the registry lookup, exactly as LookupEngine does for an +// engine name. +func TestEngineForFamily_NormalisesCaseAndWhitespace(t *testing.T) { + t.Parallel() + assert.Equal(t, "postgres18", normaliseFamily(" POSTGRES18 ")) + assert.Equal(t, "mariadb11.8", normaliseFamily("MariaDB11.8")) + + byCase, err := EngineForFamily(" POSTGRES18 ") + require.NoError(t, err) + assert.Equal(t, enginePostgres.Name, byCase.Name) + + byMixedCase, err := EngineForFamily("MariaDB11.8") + require.NoError(t, err) + assert.Equal(t, engineMariaDB.Name, byMixedCase.Name) +} + +func TestEngineForFamily_RejectsUnknownFamily(t *testing.T) { + t.Parallel() + _, err := EngineForFamily("mysql8") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") +} + +func TestValidateEngineRegistry(t *testing.T) { + require.NoError(t, ValidateEngineRegistry()) +} + +// A second engine that exists solely inside the test binary, so the cross-engine +// registry validation is proven against the seam rather than against whichever +// engines the build happens to offer. +const ( + testEngineName = "testdb" + testEngineGroup = "tuned-testdb" +) + +var engineUnderTest = Engine{ + Name: testEngineName, + MajorVersion: "1", + DefaultPort: 3306, + description: "Test Engine", + licenseModel: "test-license", + reservedUsernames: []string{"root"}, + reservedUsernamePrefixes: []string{"testdb_"}, + maxUsernameLen: 80, + validateDBName: dbNameRule(64), + catalog: map[string]ParameterSpec{ + "buffer_size": { + Name: "buffer_size", DataType: ParamTypeInteger, ApplyType: ApplyTypeStatic, + IsModifiable: true, Min: 1, Max: 1024, Default: "64", Unit: "MB", + Description: "Buffer the server allocates at startup, in MB.", + }, + "connection_limit": { + Name: "connection_limit", DataType: ParamTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 1, Max: 1000, Default: "100", + Description: "Maximum concurrent connections to the server.", + }, + }, + validateCombinations: func([]Setting) error { return nil }, + crashRecoveryNote: "It will recover when it is restored.", + uncleanStopNote: "It will recover on the next start.", +} + +func TestIndexEnginesByFamily_RejectsInvalidMetadata(t *testing.T) { + noDBNameRule := engineUnderTest + noDBNameRule.validateDBName = nil + noCrashRecoveryNote := engineUnderTest + noCrashRecoveryNote.crashRecoveryNote = "" + noUncleanStopNote := engineUnderTest + noUncleanStopNote.uncleanStopNote = "" + invalidTLSParameter := engineUnderTest + invalidTLSParameter.tlsEnforcementParameter = "not-a-boolean-parameter" + + tests := []struct { + name string + registry map[string]Engine + wantErr string + }{ + { + name: "missing DB name rule", + registry: map[string]Engine{"test": noDBNameRule}, + wantErr: "registers no DBName rule", + }, + { + name: "missing crash recovery note", + registry: map[string]Engine{"test": noCrashRecoveryNote}, + wantErr: "registers no crash-recovery note", + }, + { + name: "missing unclean stop note", + registry: map[string]Engine{"test": noUncleanStopNote}, + wantErr: "registers no unclean-stop note", + }, + { + name: "invalid TLS enforcement parameter", + registry: map[string]Engine{"test": invalidTLSParameter}, + wantErr: "is not a boolean parameter it exposes", + }, + { + name: "duplicate parameter group family", + registry: map[string]Engine{ + "first": engineUnderTest, + "second": engineUnderTest, + }, + wantErr: "is claimed by two engines", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + indexed, err := indexEnginesByFamily(tt.registry) + require.ErrorContains(t, err, tt.wantErr) + assert.Nil(t, indexed) + }) + } +} + +// ResolveEffectiveParameters checks validateCombinations before anything else, +// so an engine that registers none fails with ServerInternal rather than +// resolving a parameter set it cannot then validate. +func TestResolveEffectiveParameters_NoCombinationCheckIsServerInternal(t *testing.T) { + t.Parallel() + bare := Engine{Name: "bare-engine"} + _, err := bare.ResolveEffectiveParameters(testSizing, testSizing.SmallestInstanceClass(), nil) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, awserrors.ValidErrorCodeFromError(err)) + assert.Contains(t, err.Error(), "registers no parameter combination checks") +} + +// A combination check that reads a name absent from its own catalog is a +// catalog bug, not anything a customer did, so the resolver's internal reader +// reports ServerInternal naming the missing key. +func TestResolveEffectiveParameters_CombinationCheckMissingKeyIsServerInternal(t *testing.T) { + t.Parallel() + broken := Engine{ + Name: "broken-combination-checks", + catalog: map[string]ParameterSpec{ + "foo": {Name: "foo", DataType: ParamTypeInteger, ApplyType: ApplyTypeStatic, + IsModifiable: true, Min: 0, Max: 10, Default: "1"}, + }, + validateCombinations: func(settings []Setting) error { + values := resolvedValues(settings) + _, err := resolvedInteger(values, "bar") + return err + }, + } + _, err := broken.ResolveEffectiveParameters(testSizing, testSizing.SmallestInstanceClass(), nil) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, awserrors.ValidErrorCodeFromError(err)) + assert.Contains(t, err.Error(), "resolved parameter set is missing bar") +} diff --git a/spinifex/handlers/rds/engine_mariadb.go b/spinifex/domains/rds/engine/mariadb.go similarity index 99% rename from spinifex/handlers/rds/engine_mariadb.go rename to spinifex/domains/rds/engine/mariadb.go index 22bed1b04..48931138f 100644 --- a/spinifex/handlers/rds/engine_mariadb.go +++ b/spinifex/domains/rds/engine/mariadb.go @@ -1,4 +1,4 @@ -package handlers_rds +package engine import ( "slices" @@ -467,8 +467,8 @@ func parseTimeZoneOffset(value string) (int, error) { return offsetSeconds / int(time.Minute/time.Second), nil } -func validateMariaDBParameterCombinations(params []Parameter) error { - values := resolvedValues(params) +func validateMariaDBParameterCombinations(settings []Setting) error { + values := resolvedValues(settings) // A collation outside the server's character set is one of the few settings // mysqld refuses to start under rather than adjusting. diff --git a/spinifex/domains/rds/engine/mariadb_test.go b/spinifex/domains/rds/engine/mariadb_test.go new file mode 100644 index 000000000..638656b80 --- /dev/null +++ b/spinifex/domains/rds/engine/mariadb_test.go @@ -0,0 +1,437 @@ +package engine + +//test:in-package — reaches the unexported engineMariaDB registry entry, +// which this package does not export. + +import ( + "strconv" + "strings" + "testing" + + "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Offered only now that the image and the guest implementation exist: until +// they did, a create would have resolved an AMI nothing builds and left a volume +// and an ENI behind for an instance that could never become available. +func TestMariaDB_IsOffered(t *testing.T) { + t.Parallel() + engine, err := LookupEngine(" MariaDB ") + require.NoError(t, err, "the engine name is matched case-insensitively and trimmed") + assert.Equal(t, engineMariaDB.Name, engine.Name) + assert.Contains(t, SupportedEngines(), "mariadb") + + byFamily, err := EngineForFamily(engineMariaDB.ParameterGroupFamily()) + require.NoError(t, err) + assert.Equal(t, engineMariaDB.Name, byFamily.Name) + assert.Contains(t, SupportedParameterGroupFamilies(), "mariadb11.8") + + // mysql is a distinct AWS engine this platform does not offer, and MariaDB is + // deliberately not aliased onto it: the alias would report an engine and a + // version the instance is not running. + _, err = LookupEngine("mysql") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) +} + +func TestMariaDB_Identity(t *testing.T) { + t.Parallel() + assert.Equal(t, "mariadb", engineMariaDB.Name) + assert.Equal(t, int64(3306), engineMariaDB.DefaultPort) + // The version series the API pins to, not an integer major: PostgreSQL's + // version axis is one integer and MariaDB's is two. + assert.Equal(t, "11.8", engineMariaDB.EngineVersion()) + assert.Equal(t, "mariadb11.8", engineMariaDB.ParameterGroupFamily()) + assert.Equal(t, "default.mariadb11.8", engineMariaDB.DefaultParameterGroupName()) + + for _, version := range []string{"", "11.8"} { + assert.NoError(t, engineMariaDB.ValidateVersion(version), "version %q", version) + } + for _, version := range []string{"11", "11.4", "11.8.8", "10.11", "12", "latest"} { + assert.Error(t, engineMariaDB.ValidateVersion(version), "version %q", version) + } +} + +// The engine's own limit rather than AWS's documented 16, so a client written +// against real AWS is a strict subset and nothing that works there breaks here. +func TestMariaDB_ValidateMasterUsername(t *testing.T) { + for _, username := range []string{"appuser", "app_user1", "Orders", strings.Repeat("a", 80)} { + assert.NoError(t, engineMariaDB.ValidateMasterUsername(username), "username %q", username) + } + + cases := map[string]string{ + "empty": "", + "leading digit": "1user", + "hyphen": "app-user", + "dot": "app.user", + "too long": strings.Repeat("a", 81), + "install-db root": "root", + "cased root": "ROOT", + "sys schema owner": "mariadb.sys", + "socket account": "mysql", + "aws management": "rdsadmin", + // The server refuses CREATE USER PUBLIC, which would spend the one-shot + // bootstrap password on a guaranteed failure. + "role name": "PUBLIC", + } + for name, username := range cases { + t.Run(name, func(t *testing.T) { + err := engineMariaDB.ValidateMasterUsername(username) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) + }) + } +} + +// The guest re-checks the name it is handed rather than trusting the control +// plane, and it reaches the reserved half on its own. +func TestMariaDB_ValidateUsernameNotReserved(t *testing.T) { + t.Parallel() + for _, username := range []string{"mysql.session", "MySQL.infoschema", "mariadb.sys", "mariadb.anything"} { + err := engineMariaDB.ValidateUsernameNotReserved(username) + require.Error(t, err, "username %q", username) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) + } + assert.NoError(t, engineMariaDB.ValidateUsernameNotReserved("mysqladmin"), + "the prefix is mysql. with the dot, not the bare word") +} + +// MariaDB maps a database onto a directory name and rds-init interpolates the +// name into CREATE DATABASE from the shell, so this rule is the actual barrier. +func TestMariaDB_ValidateDBName(t *testing.T) { + t.Parallel() + for _, name := range []string{"", "orders", "Orders_2", strings.Repeat("d", 64)} { + assert.NoError(t, engineMariaDB.ValidateDBName(name), "name %q", name) + } + for _, name := range []string{"my-db", "1db", "my db", "my/db", `my\db`, "my.db", "db ", strings.Repeat("d", 65)} { + err := engineMariaDB.ValidateDBName(name) + require.Error(t, err, "name %q", name) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) + } + + // Each engine takes its own identifier limit rather than a shared one. + assert.Error(t, enginePostgres.ValidateDBName(strings.Repeat("d", 64))) + assert.NoError(t, engineMariaDB.ValidateDBName(strings.Repeat("d", 64))) +} + +// The two catalogs are separate tables, not one table with an engine column, so +// neither engine can be handed a setting the other's server would not parse. +func TestMariaDBCatalog_IsSeparateFromPostgres(t *testing.T) { + t.Parallel() + for _, name := range []string{"innodb_buffer_pool_size", "sql_mode", "key_buffer_size"} { + _, ok := enginePostgres.LookupParameter(name) + assert.False(t, ok, "PostgreSQL must not expose %s", name) + } + for _, name := range []string{"work_mem", "shared_buffers", "autovacuum", "wal_level", "datestyle"} { + _, ok := engineMariaDB.LookupParameter(name) + assert.False(t, ok, "MariaDB must not expose %s", name) + } + // max_connections exists in both, and each takes its own engine's bounds. + _, err := engineMariaDB.ValidateParameterValue("max_connections", "20000") + assert.NoError(t, err) + _, err = enginePostgres.ValidateParameterValue("max_connections", "20000") + assert.Error(t, err, "PostgreSQL's own ceiling is 5000") +} + +// Absent rather than present-and-unmodifiable, because they are not the +// customer's to set: the endpoint, the agent's socket access, the serving +// certificate and the snapshot recovery guarantee all depend on them. +func TestMariaDBCatalog_OmitsPlatformOwnedSettings(t *testing.T) { + t.Parallel() + owned := []string{ + "port", "datadir", "socket", "bind_address", + "ssl_ca", "ssl_cert", "ssl_key", + "secure_file_priv", "skip_symbolic_links", "log_bin", + "default_storage_engine", + "innodb_buffer_pool_chunk_size", "innodb_buffer_pool_instances", + } + for _, name := range owned { + _, ok := engineMariaDB.LookupParameter(name) + assert.False(t, ok, "%s is platform-owned and must not be in the catalog", name) + } +} + +// The size-derived defaults have to move with the class, or the formulas are +// decoration and one end of the supported range is mis-tuned. +func TestMariaDBCatalog_SizeDerivedDefaultsScaleWithClassMemory(t *testing.T) { + t.Parallel() + smallest, err := testSizing.ClassMemoryMiB("db.t3.micro") + require.NoError(t, err) + largest, err := testSizing.ClassMemoryMiB("db.m5.xlarge") + require.NoError(t, err) + + for _, name := range []string{"innodb_buffer_pool_size", "max_connections"} { + spec, ok := engineMariaDB.LookupParameter(name) + require.True(t, ok, "%s is expected to carry a computed default", name) + require.NotNil(t, spec.DefaultFor, "%s should be size-derived", name) + require.NotNil(t, spec.MaxFor, "%s should carry a class ceiling", name) + + small, err := strconv.ParseInt(spec.DefaultAt(smallest), 10, 64) + require.NoError(t, err) + large, err := strconv.ParseInt(spec.DefaultAt(largest), 10, 64) + require.NoError(t, err) + assert.Less(t, small, large, "%s does not grow with the class", name) + assert.LessOrEqual(t, small, spec.MaxFor(smallest), "%s defaults above its own class ceiling", name) + } +} + +// Three quarters of class memory is RDS's own default for this engine family, +// and it is the setting whose being wrong stops the server from starting. +func TestMariaDBCatalog_BufferPoolIsThreeQuartersOfClassMemory(t *testing.T) { + t.Parallel() + memoryMiB, err := testSizing.ClassMemoryMiB("db.m5.large") + require.NoError(t, err) + + bytes, err := strconv.ParseInt(innodbBufferPoolSizeFor(memoryMiB), 10, 64) + require.NoError(t, err) + assert.Equal(t, memoryMiB*mibToBytes*3/4, bytes) + assert.Less(t, bytes, innodbBufferPoolSizeCeilingFor(memoryMiB), + "the ceiling has to leave headroom above the default") + assert.Less(t, innodbBufferPoolSizeCeilingFor(memoryMiB), memoryMiB*mibToBytes, + "a pool at class memory is a server that never starts") +} + +// max_connections = {DBInstanceClassMemory/12582880}, which is RDS's own formula. +func TestMariaDBCatalog_MaxConnectionsFollowsTheRDSFormula(t *testing.T) { + t.Parallel() + memoryMiB, err := testSizing.ClassMemoryMiB("db.m5.xlarge") + require.NoError(t, err) + assert.Equal(t, strconv.FormatInt(memoryMiB*mibToBytes/12582880, 10), mariadbMaxConnectionsFor(memoryMiB)) + + // The floor keeps the smallest class above what the platform's own probe, + // parameter apply and password rotation sessions need. + smallest, err := testSizing.ClassMemoryMiB(testSizing.SmallestInstanceClass()) + require.NoError(t, err) + connections, err := strconv.ParseInt(mariadbMaxConnectionsFor(smallest), 10, 64) + require.NoError(t, err) + assert.GreaterOrEqual(t, connections, int64(20)) +} + +func TestMariaDBCatalog_RejectsBadValues(t *testing.T) { + cases := []struct { + name, param, value, want string + }{ + {"UnknownName", "not_a_setting", "1", "is not a parameter this engine exposes"}, + {"PostgresSetting", "work_mem", "16384", "is not a parameter this engine exposes"}, + {"NotAnInteger", "max_connections", "many", "takes an integer"}, + {"BelowRange", "max_connections", "1", "outside its allowed range"}, + {"BufferPoolBelowMinimum", "innodb_buffer_pool_size", "1048576", "outside its allowed range"}, + {"RedoLogBelowMinimum", "innodb_log_file_size", "1048576", "outside its allowed range"}, + {"NotAReal", "long_query_time", "slow", "takes a number"}, + {"RealOutOfRange", "innodb_max_dirty_pages_pct", "100", "outside its allowed range"}, + {"NotInEnum", "log_output", "syslog", "does not accept"}, + {"NotAnIsolationLevel", "transaction_isolation", "snapshot", "does not accept"}, + {"UnknownSQLMode", "sql_mode", "STRICT_TRANS_TABLES,NOT_A_MODE", "does not accept"}, + {"EmptySQLModeToken", "sql_mode", "STRICT_TRANS_TABLES,,ANSI_QUOTES", "does not accept"}, + {"NamedTimeZone", "time_zone", "Australia/Sydney", "does not accept"}, + {"OffsetOutOfRange", "time_zone", "+15:00", "does not accept"}, + {"UnsignedOffset", "time_zone", "10:00", "does not accept"}, + {"OffsetWithoutMinutes", "time_zone", "+10", "does not accept"}, + {"OffsetWithShortHour", "time_zone", "+1:00", "does not accept"}, + {"OffsetWithLongHour", "time_zone", "+0001:00", "does not accept"}, + {"Empty", "sort_buffer_size", "", "empty value"}, + {"Formula", "innodb_buffer_pool_size", "{DBInstanceClassMemory*3/4}", "is a formula"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := engineMariaDB.ValidateParameterValue(tc.param, tc.value) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") + assert.Contains(t, err.Error(), tc.want) + }) + } +} + +func TestMariaDBCatalog_AcceptsEngineSpelledValues(t *testing.T) { + cases := []struct{ param, value string }{ + {"sql_mode", "STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION"}, + {"sql_mode", "ansi_quotes"}, + {"sql_mode", "TRADITIONAL, ONLY_FULL_GROUP_BY"}, + {"time_zone", "SYSTEM"}, + {"time_zone", "system"}, + {"time_zone", "+10:00"}, + {"time_zone", "-05:30"}, + {"time_zone", "+00:00"}, + {"innodb_flush_method", "O_DIRECT"}, + {"transaction_isolation", "READ-COMMITTED"}, + {"log_output", "TABLE"}, + } + for _, tc := range cases { + t.Run(tc.param+"/"+tc.value, func(t *testing.T) { + _, err := engineMariaDB.ValidateParameterValue(tc.param, tc.value) + assert.NoError(t, err) + }) + } +} + +// MariaDB refuses yes and no for a boolean system variable where PostgreSQL +// accepts them, so a value the API takes must be one mysqld will parse. +// mariadbd's own parser refuses yes and no, but no value reaches it unresolved: +// the resolver canonicalises every boolean to 1 or 0 first. So the API takes all +// eight spellings here as it does on PostgreSQL, and the engine still only ever +// sees the two it parses. +func TestMariaDBCatalog_TakesEveryBooleanSpelling(t *testing.T) { + for _, param := range []string{"slow_query_log", "require_secure_transport"} { + t.Run(param, func(t *testing.T) { + for _, value := range []string{"on", "OFF", "true", "False", "yes", "No", "1", "0"} { + _, err := engineMariaDB.ValidateParameterValue(param, value) + assert.NoError(t, err, "%s rejected %q, which the API accepts", param, value) + } + + spec, ok := engineMariaDB.LookupParameter(param) + require.True(t, ok) + assert.Equal(t, "on,off,true,false,yes,no,1,0", spec.AllowedValues(), + "a customer has to be able to read which spellings are accepted") + }) + } +} + +func TestMariaDBCatalog_RejectsFatalCombinations(t *testing.T) { + tests := []struct { + name string + overrides map[string]string + want string + }{ + { + name: "collation outside the server character set", + overrides: map[string]string{"character_set_server": "latin1"}, + want: "does not belong to character_set_server", + }, + { + name: "io capacity maximum below the steady rate", + overrides: map[string]string{"innodb_io_capacity": "4000"}, + want: "innodb_io_capacity_max must be at least innodb_io_capacity", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := engineMariaDB.ResolveEffectiveParameters(testSizing, "db.m5.xlarge", tt.overrides) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) + assert.Contains(t, err.Error(), tt.want) + }) + } + + _, err := engineMariaDB.ResolveEffectiveParameters(testSizing, "db.m5.xlarge", map[string]string{ + "character_set_server": "latin1", "collation_server": "latin1_bin", + }) + assert.NoError(t, err, "a consistent pair is the customer's to choose") +} + +// The buffer pool is the one setting a large-class literal makes a smaller guest +// unbootable with, so its class ceiling has to bind. +func TestMariaDBCatalog_BoundsSizeDerivedOverridesToTheClass(t *testing.T) { + tests := []struct{ name, value string }{ + {name: "innodb_buffer_pool_size", value: "12884901888"}, + {name: "max_connections", value: "2000"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := engineMariaDB.ResolveEffectiveParameters(testSizing, "db.t3.micro", map[string]string{tt.name: tt.value}) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) + assert.Contains(t, err.Error(), "db.t3.micro") + assert.Contains(t, err.Error(), "ceiling") + + _, err = engineMariaDB.ResolveEffectiveParameters(testSizing, "db.m5.xlarge", map[string]string{tt.name: tt.value}) + assert.NoError(t, err, "the same literal should fit the larger class") + }) + } +} + +// The apply type is what decides whether a change is a SET GLOBAL or a restart, +// and the guest's pending-restart comparison reads the static keys out of the +// installed file. Getting one wrong is a permanent pending-reboot or a change +// the customer is told took effect and did not. +func TestMariaDBCatalog_ApplyTypesMatchTheEngine(t *testing.T) { + t.Parallel() + static := []string{ + "innodb_buffer_pool_size", "innodb_log_file_size", "innodb_flush_method", + "innodb_read_io_threads", "innodb_write_io_threads", "innodb_purge_threads", + "innodb_autoinc_lock_mode", "performance_schema", + } + for _, name := range static { + spec, ok := engineMariaDB.LookupParameter(name) + require.True(t, ok, "%s", name) + assert.Equal(t, ApplyTypeStatic, spec.ApplyType, "%s", name) + } + + dynamic := []string{ + "max_connections", "max_allowed_packet", "innodb_flush_log_at_trx_commit", + "innodb_io_capacity", "key_buffer_size", "table_open_cache", "sql_mode", + "slow_query_log", "long_query_time", "time_zone", "transaction_isolation", + } + for _, name := range dynamic { + spec, ok := engineMariaDB.LookupParameter(name) + require.True(t, ok, "%s", name) + assert.Equal(t, ApplyTypeDynamic, spec.ApplyType, "%s", name) + } +} + +func TestMariaDBCatalog_ResolvesEveryParameterAsALiteral(t *testing.T) { + t.Parallel() + resolved, err := engineMariaDB.ResolveEffectiveParameters(testSizing, "db.m5.large", map[string]string{"max_connections": "300"}) + require.NoError(t, err) + require.Len(t, resolved, len(engineMariaDB.CatalogParameterNames())) + + values := map[string]string{} + for i, setting := range resolved { + if i > 0 { + assert.Less(t, resolved[i-1].Name, setting.Name, "the resolved set is not sorted by name") + } + assert.NotContains(t, setting.Value, "{", "%s reached the agent as a formula", setting.Name) + assert.NotContains(t, setting.Value, "DBInstanceClass", "%s reached the agent as a reference", setting.Name) + values[setting.Name] = setting.Value + } + assert.Equal(t, "300", values["max_connections"], "the override did not win") + + memoryMiB, err := testSizing.ClassMemoryMiB("db.m5.large") + require.NoError(t, err) + assert.Equal(t, innodbBufferPoolSizeFor(memoryMiB), values["innodb_buffer_pool_size"], + "a parameter the group does not set should carry its computed default") +} + +// The bug that kept every MariaDB instance from booting: mariadbd has no +// time_zone startup option, so an option file naming it aborts the server before +// it opens the datadir. +func TestMariaDBCatalog_TimeZoneUsesItsStartupSpellingInTheOptionFile(t *testing.T) { + t.Parallel() + assert.Equal(t, "default_time_zone", engineMariaDB.OptionFileName("time_zone")) + + // The customer-facing name is what the API reports and what a live SET GLOBAL + // names, so the catalog must still carry it. + spec, ok := engineMariaDB.LookupParameter("time_zone") + require.True(t, ok) + assert.Equal(t, "time_zone", spec.Name) + assert.True(t, spec.IsModifiable) +} + +func TestMariaDBCatalog_EveryOtherParameterKeepsItsName(t *testing.T) { + t.Parallel() + for _, name := range engineMariaDB.CatalogParameterNames() { + if name == "time_zone" { + continue + } + assert.Equal(t, name, engineMariaDB.OptionFileName(name), + "%s was given an option-file spelling but the fixture asserts none", name) + } +} + +// PostgreSQL sets every one of its settings under the name the customer knows, +// so a mapping there would be a bug rather than a fix. +func TestPostgresCatalog_NoParameterIsRenamedForTheOptionFile(t *testing.T) { + t.Parallel() + for _, name := range enginePostgres.CatalogParameterNames() { + assert.Equal(t, name, enginePostgres.OptionFileName(name)) + } +} + +// A name the catalog does not carry is the customer's own, and the option file +// takes it unchanged rather than silently dropping it. +func TestMariaDBCatalog_OptionFileNamePassesAnUnknownNameThrough(t *testing.T) { + t.Parallel() + assert.Equal(t, "not_a_parameter", engineMariaDB.OptionFileName("not_a_parameter")) +} diff --git a/spinifex/handlers/rds/paramcatalog.go b/spinifex/domains/rds/engine/parameters.go similarity index 86% rename from spinifex/handlers/rds/paramcatalog.go rename to spinifex/domains/rds/engine/parameters.go index 111cd9d9f..3b81bced3 100644 --- a/spinifex/handlers/rds/paramcatalog.go +++ b/spinifex/domains/rds/engine/parameters.go @@ -1,4 +1,4 @@ -package handlers_rds +package engine import ( "errors" @@ -10,7 +10,6 @@ import ( "strings" "unicode" - "github.com/mulgadc/spinifex/spinifex/domains/ec2/instancetypes" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" ) @@ -25,19 +24,6 @@ const ( ApplyTypeDynamic = "dynamic" ) -// AWS's own ApplyMethod values, echoed back on DescribeDBParameters. -const ( - ApplyMethodImmediate = "immediate" - ApplyMethodPendingReboot = "pending-reboot" -) - -// Where a reported value came from. AWS distinguishes these and the Terraform -// provider reads them, so a computed default must not be reported as user. -const ( - ParameterSourceUser = "user" - ParameterSourceEngineDefault = "engine-default" -) - // The parameter data types the catalog offers. Every one is validated on input, // so a value the API accepts is one the engine will parse. const ( @@ -213,23 +199,6 @@ func formatBound(v float64) string { return strconv.FormatFloat(v, 'g', -1, 64) } -// The memory an instance class's guest has, which every size-derived default is -// computed from. An unknown class is a validation failure upstream, so this is -// the last line rather than the check. -func classMemoryMiB(instanceClass string) (int64, error) { - instanceType, err := InstanceTypeForClass(instanceClass) - if err != nil { - return 0, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, - "DBInstanceClass %q is not supported; supported classes are %s", instanceClass, strings.Join(SupportedInstanceClasses(), ", ")) - } - memoryMiB, ok := instancetypes.DefaultMemoryMiB(instanceType) - if !ok || memoryMiB <= 0 { - return 0, awserrors.Errorf(awserrors.ErrorServerInternal, - "no memory footprint is known for instance type %s", instanceType) - } - return memoryMiB, nil -} - // AWS accepts formulas like {DBInstanceClassMemory/32768} and references like // DBInstanceClassMemory from a customer. This platform does not: the catalog // validates literals, and a formula that reached the engine unvalidated would be @@ -243,11 +212,10 @@ func rejectFormulaValue(name, value string) error { "and the size-derived defaults are computed for you", value, name) } -// Checks one customer-supplied value against its catalog entry. A parameter the -// catalog does not hold, one the platform owns, and one outside its own range -// are all rejected here — at the API, rather than by an engine that then refuses -// to start with the bad config already on the data volume. -func (e Engine) validateParameterValue(name, value string) (ParameterSpec, error) { +// ValidateParameterValue checks one customer-supplied value against its +// catalog entry, rejecting an unknown name, a platform-owned one, or a value +// outside its range here rather than at an engine that refuses to start. +func (e Engine) ValidateParameterValue(name, value string) (ParameterSpec, error) { spec, ok := e.LookupParameter(name) if !ok { return ParameterSpec{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, @@ -351,22 +319,22 @@ func rangeError(spec ParameterSpec, value string) error { // // Overrides are re-validated rather than trusted: a catalog whose bounds // tightened must not keep handing the engine a value it would now reject. -func (e Engine) ResolveEffectiveParameters(instanceClass string, overrides map[string]string) ([]Parameter, error) { +func (e Engine) ResolveEffectiveParameters(sizing Sizing, instanceClass string, overrides map[string]string) ([]Setting, error) { if e.validateCombinations == nil { return nil, awserrors.Errorf(awserrors.ErrorServerInternal, "engine %s registers no parameter combination checks", e.Name) } - memoryMiB, err := classMemoryMiB(instanceClass) + memoryMiB, err := sizing.ClassMemoryMiB(instanceClass) if err != nil { return nil, err } names := e.CatalogParameterNames() - resolved := make([]Parameter, 0, len(names)) + resolved := make([]Setting, 0, len(names)) for _, name := range names { spec := e.catalog[name] value := spec.DefaultAt(memoryMiB) if override, ok := overrides[name]; ok { - if _, err := e.validateParameterValue(name, override); err != nil { + if _, err := e.ValidateParameterValue(name, override); err != nil { return nil, err } if err := validateClassParameterValue(instanceClass, memoryMiB, spec, override); err != nil { @@ -381,7 +349,7 @@ func (e Engine) ResolveEffectiveParameters(instanceClass string, overrides map[s } value = canonical } - resolved = append(resolved, Parameter{Name: name, Value: value}) + resolved = append(resolved, Setting{Name: name, Value: value}) } if err := e.validateCombinations(resolved); err != nil { return nil, err @@ -401,10 +369,10 @@ func clampInt64(v, lo, hi int64) int64 { // Reads one setting out of a resolved set for a combination check. The set is // every catalog name by construction, so an absent one is a catalog bug rather // than anything a customer did. -func resolvedValues(params []Parameter) map[string]string { - values := make(map[string]string, len(params)) - for _, param := range params { - values[param.Name] = strings.ToLower(strings.TrimSpace(param.Value)) +func resolvedValues(settings []Setting) map[string]string { + values := make(map[string]string, len(settings)) + for _, setting := range settings { + values[setting.Name] = strings.ToLower(strings.TrimSpace(setting.Value)) } return values } diff --git a/spinifex/handlers/rds/paramcatalog_test.go b/spinifex/domains/rds/engine/parameters_test.go similarity index 55% rename from spinifex/handlers/rds/paramcatalog_test.go rename to spinifex/domains/rds/engine/parameters_test.go index 955c8bd75..198bdfab7 100644 --- a/spinifex/handlers/rds/paramcatalog_test.go +++ b/spinifex/domains/rds/engine/parameters_test.go @@ -1,4 +1,4 @@ -package handlers_rds +package engine import ( "strconv" @@ -82,7 +82,7 @@ func TestParameterCatalog_DefaultsSatisfyTheirOwnConstraintsAtEveryClass(t *test for _, engine := range catalogEngines { for _, class := range SupportedInstanceClasses() { t.Run(engine.Name+"/"+class, func(t *testing.T) { - memoryMiB, err := classMemoryMiB(class) + memoryMiB, err := testSizing.ClassMemoryMiB(class) require.NoError(t, err) for _, name := range engine.CatalogParameterNames() { @@ -100,7 +100,7 @@ func TestParameterCatalog_DefaultsSatisfyTheirOwnConstraintsAtEveryClass(t *test // The defaults also have to satisfy the engine's combination checks, // which no single parameter's own validation can see. - _, err = engine.ResolveEffectiveParameters(class, nil) + _, err = engine.ResolveEffectiveParameters(testSizing, class, nil) assert.NoError(t, err, "the untouched defaults do not resolve at %s", class) }) } @@ -134,9 +134,9 @@ func TestParameterCatalog_EntriesAreWellFormed(t *testing.T) { // formulas are decoration and one end of the supported range is mis-tuned. func TestParameterCatalog_SizeDerivedDefaultsScaleWithClassMemory(t *testing.T) { t.Parallel() - smallest, err := classMemoryMiB("db.t3.micro") + smallest, err := testSizing.ClassMemoryMiB("db.t3.micro") require.NoError(t, err) - largest, err := classMemoryMiB("db.m5.xlarge") + largest, err := testSizing.ClassMemoryMiB("db.m5.xlarge") require.NoError(t, err) require.Less(t, smallest, largest) @@ -157,7 +157,7 @@ func TestParameterCatalog_SizeDerivedDefaultsScaleWithClassMemory(t *testing.T) // whose being wrong at the small end stops the engine from starting. func TestParameterCatalog_SharedBuffersIsAQuarterOfClassMemory(t *testing.T) { t.Parallel() - memoryMiB, err := classMemoryMiB("db.m5.large") + memoryMiB, err := testSizing.ClassMemoryMiB("db.m5.large") require.NoError(t, err) blocks, err := strconv.ParseInt(sharedBuffersFor(memoryMiB), 10, 64) @@ -199,7 +199,7 @@ func TestValidateParameterValue_RejectsBadInput(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - _, err := enginePostgres.validateParameterValue(tc.param, tc.value) + _, err := enginePostgres.ValidateParameterValue(tc.param, tc.value) require.Error(t, err) assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err), "the code has to survive resolution or the client sees a 500") @@ -224,7 +224,7 @@ func TestValidateParameterValue_AcceptsPostgresStringValues(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - _, err := enginePostgres.validateParameterValue(tc.param, tc.value) + _, err := enginePostgres.ValidateParameterValue(tc.param, tc.value) assert.NoError(t, err) }) } @@ -233,7 +233,7 @@ func TestValidateParameterValue_AcceptsPostgresStringValues(t *testing.T) { func TestValidateParameterValue_AcceptsEveryPostgresSpellingOfABoolean(t *testing.T) { t.Parallel() for _, value := range []string{"on", "OFF", "true", "False", "yes", "no", "1", "0"} { - _, err := enginePostgres.validateParameterValue("autovacuum", value) + _, err := enginePostgres.ValidateParameterValue("autovacuum", value) assert.NoError(t, err, "autovacuum rejected %q, which the engine accepts", value) } } @@ -241,11 +241,11 @@ func TestValidateParameterValue_AcceptsEveryPostgresSpellingOfABoolean(t *testin // The value one name carries in a resolved set, exactly as the guest is handed // it: resolvedValues lowercases, which the combination checks want and an // assertion about what is written into an option file does not. -func resolvedParameter(t *testing.T, resolved []Parameter, name string) string { +func resolvedParameter(t *testing.T, resolved []Setting, name string) string { t.Helper() - for _, param := range resolved { - if param.Name == name { - return param.Value + for _, setting := range resolved { + if setting.Name == name { + return setting.Value } } t.Fatalf("the resolved set carries no %s", name) @@ -281,7 +281,7 @@ func TestResolveEffectiveParameters_CanonicalisesEveryBooleanSpelling(t *testing for _, tc := range tests { for spelling, want := range tc.spellings { t.Run(tc.engine.Name+"/"+spelling, func(t *testing.T) { - resolved, err := tc.engine.ResolveEffectiveParameters(SmallestInstanceClass(), + resolved, err := tc.engine.ResolveEffectiveParameters(testSizing, testSizing.SmallestInstanceClass(), map[string]string{tc.name: spelling}) require.NoError(t, err) assert.Equal(t, want, resolvedParameter(t, resolved, tc.name)) @@ -295,19 +295,19 @@ func TestResolveEffectiveParameters_CanonicalisesEveryBooleanSpelling(t *testing func TestResolveEffectiveParameters_CanonicalisesTheDefaultsToo(t *testing.T) { for _, engine := range catalogEngines { t.Run(engine.Name, func(t *testing.T) { - resolved, err := engine.ResolveEffectiveParameters(SmallestInstanceClass(), nil) + resolved, err := engine.ResolveEffectiveParameters(testSizing, testSizing.SmallestInstanceClass(), nil) require.NoError(t, err) booleans := 0 - for _, param := range resolved { - spec, ok := engine.LookupParameter(param.Name) + for _, setting := range resolved { + spec, ok := engine.LookupParameter(setting.Name) require.True(t, ok) if spec.DataType != ParamTypeBoolean { continue } booleans++ - assert.Contains(t, []string{"1", "0"}, param.Value, - "%s reaches the guest as %q", param.Name, param.Value) + assert.Contains(t, []string{"1", "0"}, setting.Value, + "%s reaches the guest as %q", setting.Name, setting.Value) } assert.NotZero(t, booleans, "no boolean in the set, so this asserts nothing") }) @@ -332,7 +332,7 @@ func TestParameterCatalog_TLSFloorIsPinnedAndNotModifiable(t *testing.T) { assert.Equal(t, "TLSv1.3", spec.Default) assert.False(t, spec.IsModifiable) - _, err := tc.engine.validateParameterValue(tc.name, "TLSv1.2") + _, err := tc.engine.ValidateParameterValue(tc.name, "TLSv1.2") require.Error(t, err) assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err), "the code has to survive resolution or the client sees a 500") @@ -340,7 +340,7 @@ func TestParameterCatalog_TLSFloorIsPinnedAndNotModifiable(t *testing.T) { // Pinned in the catalog and inert unless it is also in the set the guest // installs, which is the half a customer's group cannot reach. - resolved, err := tc.engine.ResolveEffectiveParameters(SmallestInstanceClass(), nil) + resolved, err := tc.engine.ResolveEffectiveParameters(testSizing, testSizing.SmallestInstanceClass(), nil) require.NoError(t, err) assert.Equal(t, "TLSv1.3", resolvedParameter(t, resolved, tc.name)) }) @@ -378,7 +378,7 @@ func TestParameterCatalog_EachEngineExposesItsTLSEnforcementParameter(t *testing // Whichever spelling the group carries, the guest has one literal to compare. for spelling, want := range tc.spellings { - resolved, err := tc.engine.ResolveEffectiveParameters(SmallestInstanceClass(), + resolved, err := tc.engine.ResolveEffectiveParameters(testSizing, testSizing.SmallestInstanceClass(), map[string]string{tc.name: spelling}) require.NoError(t, err) assert.Equal(t, want, resolvedParameter(t, resolved, tc.name)) @@ -391,17 +391,17 @@ func TestParameterCatalog_EachEngineExposesItsTLSEnforcementParameter(t *testing // every parameter is present, and one the group does not set carries its default. func TestResolveEffectiveParameters_OverlaysOverridesOnDefaults(t *testing.T) { t.Parallel() - resolved, err := enginePostgres.ResolveEffectiveParameters("db.t3.medium", map[string]string{"work_mem": "16384"}) + resolved, err := enginePostgres.ResolveEffectiveParameters(testSizing, "db.t3.medium", map[string]string{"work_mem": "16384"}) require.NoError(t, err) require.Len(t, resolved, len(enginePostgres.CatalogParameterNames())) values := map[string]string{} - for _, param := range resolved { - values[param.Name] = param.Value + for _, setting := range resolved { + values[setting.Name] = setting.Value } assert.Equal(t, "16384", values["work_mem"], "the override did not win") - memoryMiB, err := classMemoryMiB("db.t3.medium") + memoryMiB, err := testSizing.ClassMemoryMiB("db.t3.medium") require.NoError(t, err) assert.Equal(t, sharedBuffersFor(memoryMiB), values["shared_buffers"], "a parameter the group does not set should carry its computed default") @@ -411,15 +411,15 @@ func TestResolveEffectiveParameters_OverlaysOverridesOnDefaults(t *testing.T) { // byte-identical include, so an apply does not restart an engine for no reason. func TestResolveEffectiveParameters_IsSortedAndCarriesNoFormula(t *testing.T) { t.Parallel() - resolved, err := enginePostgres.ResolveEffectiveParameters("db.m5.xlarge", nil) + resolved, err := enginePostgres.ResolveEffectiveParameters(testSizing, "db.m5.xlarge", nil) require.NoError(t, err) for i := 1; i < len(resolved); i++ { assert.Less(t, resolved[i-1].Name, resolved[i].Name, "the resolved set is not sorted by name") } - for _, param := range resolved { - assert.NotContains(t, param.Value, "{", "%s reached the agent as a formula", param.Name) - assert.NotContains(t, param.Value, "DBInstanceClass", "%s reached the agent as a reference", param.Name) + for _, setting := range resolved { + assert.NotContains(t, setting.Value, "{", "%s reached the agent as a formula", setting.Name) + assert.NotContains(t, setting.Value, "DBInstanceClass", "%s reached the agent as a reference", setting.Name) } } @@ -427,7 +427,7 @@ func TestResolveEffectiveParameters_IsSortedAndCarriesNoFormula(t *testing.T) { // engine, so the resolve re-validates rather than trusting what was written. func TestResolveEffectiveParameters_RevalidatesStoredOverrides(t *testing.T) { t.Parallel() - _, err := enginePostgres.ResolveEffectiveParameters("db.t3.medium", map[string]string{"max_connections": "999999"}) + _, err := enginePostgres.ResolveEffectiveParameters(testSizing, "db.t3.medium", map[string]string{"max_connections": "999999"}) require.Error(t, err) assert.Contains(t, err.Error(), "max_connections") } @@ -444,14 +444,14 @@ func TestResolveEffectiveParameters_BoundsSizeDerivedOverridesToTheClass(t *test for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, err := enginePostgres.ResolveEffectiveParameters("db.t3.micro", map[string]string{tt.name: tt.value}) + _, err := enginePostgres.ResolveEffectiveParameters(testSizing, "db.t3.micro", map[string]string{tt.name: tt.value}) require.Error(t, err) assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) assert.Contains(t, err.Error(), "db.t3.micro") assert.Contains(t, err.Error(), tt.value) assert.Contains(t, err.Error(), "ceiling") - _, err = enginePostgres.ResolveEffectiveParameters("db.m5.xlarge", map[string]string{tt.name: tt.value}) + _, err = enginePostgres.ResolveEffectiveParameters(testSizing, "db.m5.xlarge", map[string]string{tt.name: tt.value}) assert.NoError(t, err, "the same literal should fit the larger class") }) } @@ -484,7 +484,7 @@ func TestResolveEffectiveParameters_RejectsFatalCombinations(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, err := enginePostgres.ResolveEffectiveParameters("db.m5.xlarge", tt.overrides) + _, err := enginePostgres.ResolveEffectiveParameters(testSizing, "db.m5.xlarge", tt.overrides) require.Error(t, err) assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) assert.Contains(t, err.Error(), tt.want) @@ -494,7 +494,7 @@ func TestResolveEffectiveParameters_RejectsFatalCombinations(t *testing.T) { func TestResolveEffectiveParameters_AcceptsMinimalWALWithoutReplication(t *testing.T) { t.Parallel() - _, err := enginePostgres.ResolveEffectiveParameters("db.t3.micro", map[string]string{ + _, err := enginePostgres.ResolveEffectiveParameters(testSizing, "db.t3.micro", map[string]string{ "wal_level": "minimal", "max_wal_senders": "0", "max_replication_slots": "0", }) require.NoError(t, err) @@ -511,7 +511,7 @@ func TestParameterCatalog_Postgres18ApplyTypesAndBuiltCompressionMethods(t *test func TestResolveEffectiveParameters_RejectsAnUnknownClass(t *testing.T) { t.Parallel() - _, err := enginePostgres.ResolveEffectiveParameters("db.x99.mega", nil) + _, err := enginePostgres.ResolveEffectiveParameters(testSizing, "db.x99.mega", nil) require.Error(t, err) assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err), "the code has to survive resolution or the client sees a 500") @@ -533,3 +533,160 @@ func TestParameterSpec_AllowedValuesDescribesTheConstraint(t *testing.T) { target, _ := enginePostgres.LookupParameter("checkpoint_completion_target") assert.True(t, strings.HasPrefix(target.AllowedValues(), "0-1"), "got %q", target.AllowedValues()) } + +// catalogAgentContract is one row of the agent-contract metadata a catalog +// parameter carries: the name the option file is written under, its data type +// and its apply semantics. Hand-transcribed from postgres.go and mariadb.go, +// not derived from the catalog at test time. +type catalogAgentContract struct { + optionFileName string + dataType string + applyType string +} + +var postgresAgentContract = map[string]catalogAgentContract{ + "max_connections": {"max_connections", ParamTypeInteger, ApplyTypeStatic}, + "superuser_reserved_connections": {"superuser_reserved_connections", ParamTypeInteger, ApplyTypeStatic}, + "idle_in_transaction_session_timeout": {"idle_in_transaction_session_timeout", ParamTypeInteger, ApplyTypeDynamic}, + "statement_timeout": {"statement_timeout", ParamTypeInteger, ApplyTypeDynamic}, + "lock_timeout": {"lock_timeout", ParamTypeInteger, ApplyTypeDynamic}, + "tcp_keepalives_idle": {"tcp_keepalives_idle", ParamTypeInteger, ApplyTypeDynamic}, + "shared_buffers": {"shared_buffers", ParamTypeInteger, ApplyTypeStatic}, + "effective_cache_size": {"effective_cache_size", ParamTypeInteger, ApplyTypeDynamic}, + "work_mem": {"work_mem", ParamTypeInteger, ApplyTypeDynamic}, + "maintenance_work_mem": {"maintenance_work_mem", ParamTypeInteger, ApplyTypeDynamic}, + "temp_buffers": {"temp_buffers", ParamTypeInteger, ApplyTypeDynamic}, + "max_prepared_transactions": {"max_prepared_transactions", ParamTypeInteger, ApplyTypeStatic}, + "max_worker_processes": {"max_worker_processes", ParamTypeInteger, ApplyTypeStatic}, + "max_parallel_workers": {"max_parallel_workers", ParamTypeInteger, ApplyTypeDynamic}, + "max_parallel_workers_per_gather": {"max_parallel_workers_per_gather", ParamTypeInteger, ApplyTypeDynamic}, + "wal_level": {"wal_level", ParamTypeEnum, ApplyTypeStatic}, + "synchronous_commit": {"synchronous_commit", ParamTypeEnum, ApplyTypeDynamic}, + "wal_compression": {"wal_compression", ParamTypeEnum, ApplyTypeDynamic}, + "max_wal_size": {"max_wal_size", ParamTypeInteger, ApplyTypeDynamic}, + "min_wal_size": {"min_wal_size", ParamTypeInteger, ApplyTypeDynamic}, + "checkpoint_timeout": {"checkpoint_timeout", ParamTypeInteger, ApplyTypeDynamic}, + "checkpoint_completion_target": {"checkpoint_completion_target", ParamTypeReal, ApplyTypeDynamic}, + "wal_buffers": {"wal_buffers", ParamTypeInteger, ApplyTypeStatic}, + "max_wal_senders": {"max_wal_senders", ParamTypeInteger, ApplyTypeStatic}, + "max_replication_slots": {"max_replication_slots", ParamTypeInteger, ApplyTypeStatic}, + "autovacuum": {"autovacuum", ParamTypeBoolean, ApplyTypeDynamic}, + "autovacuum_max_workers": {"autovacuum_max_workers", ParamTypeInteger, ApplyTypeDynamic}, + "autovacuum_naptime": {"autovacuum_naptime", ParamTypeInteger, ApplyTypeDynamic}, + "autovacuum_vacuum_threshold": {"autovacuum_vacuum_threshold", ParamTypeInteger, ApplyTypeDynamic}, + "autovacuum_vacuum_scale_factor": {"autovacuum_vacuum_scale_factor", ParamTypeReal, ApplyTypeDynamic}, + "autovacuum_analyze_threshold": {"autovacuum_analyze_threshold", ParamTypeInteger, ApplyTypeDynamic}, + "autovacuum_analyze_scale_factor": {"autovacuum_analyze_scale_factor", ParamTypeReal, ApplyTypeDynamic}, + "autovacuum_vacuum_cost_limit": {"autovacuum_vacuum_cost_limit", ParamTypeInteger, ApplyTypeDynamic}, + "random_page_cost": {"random_page_cost", ParamTypeReal, ApplyTypeDynamic}, + "seq_page_cost": {"seq_page_cost", ParamTypeReal, ApplyTypeDynamic}, + "effective_io_concurrency": {"effective_io_concurrency", ParamTypeInteger, ApplyTypeDynamic}, + "default_statistics_target": {"default_statistics_target", ParamTypeInteger, ApplyTypeDynamic}, + "jit": {"jit", ParamTypeBoolean, ApplyTypeDynamic}, + "log_min_duration_statement": {"log_min_duration_statement", ParamTypeInteger, ApplyTypeDynamic}, + "log_statement": {"log_statement", ParamTypeEnum, ApplyTypeDynamic}, + "log_min_messages": {"log_min_messages", ParamTypeEnum, ApplyTypeDynamic}, + "log_connections": {"log_connections", ParamTypeBoolean, ApplyTypeDynamic}, + "log_disconnections": {"log_disconnections", ParamTypeBoolean, ApplyTypeDynamic}, + "log_lock_waits": {"log_lock_waits", ParamTypeBoolean, ApplyTypeDynamic}, + "log_temp_files": {"log_temp_files", ParamTypeInteger, ApplyTypeDynamic}, + "log_autovacuum_min_duration": {"log_autovacuum_min_duration", ParamTypeInteger, ApplyTypeDynamic}, + "deadlock_timeout": {"deadlock_timeout", ParamTypeInteger, ApplyTypeDynamic}, + "timezone": {"timezone", ParamTypeString, ApplyTypeDynamic}, + "datestyle": {"datestyle", ParamTypeString, ApplyTypeDynamic}, + "track_activity_query_size": {"track_activity_query_size", ParamTypeInteger, ApplyTypeStatic}, + "track_io_timing": {"track_io_timing", ParamTypeBoolean, ApplyTypeDynamic}, + "ssl_min_protocol_version": {"ssl_min_protocol_version", ParamTypeString, ApplyTypeDynamic}, + "rds.force_ssl": {"rds.force_ssl", ParamTypeBoolean, ApplyTypeDynamic}, +} + +var mariadbAgentContract = map[string]catalogAgentContract{ + "max_connections": {"max_connections", ParamTypeInteger, ApplyTypeDynamic}, + "max_connect_errors": {"max_connect_errors", ParamTypeInteger, ApplyTypeDynamic}, + "max_allowed_packet": {"max_allowed_packet", ParamTypeInteger, ApplyTypeDynamic}, + "thread_cache_size": {"thread_cache_size", ParamTypeInteger, ApplyTypeDynamic}, + "wait_timeout": {"wait_timeout", ParamTypeInteger, ApplyTypeDynamic}, + "interactive_timeout": {"interactive_timeout", ParamTypeInteger, ApplyTypeDynamic}, + "connect_timeout": {"connect_timeout", ParamTypeInteger, ApplyTypeDynamic}, + "net_read_timeout": {"net_read_timeout", ParamTypeInteger, ApplyTypeDynamic}, + "net_write_timeout": {"net_write_timeout", ParamTypeInteger, ApplyTypeDynamic}, + "lock_wait_timeout": {"lock_wait_timeout", ParamTypeInteger, ApplyTypeDynamic}, + "innodb_lock_wait_timeout": {"innodb_lock_wait_timeout", ParamTypeInteger, ApplyTypeDynamic}, + "max_statement_time": {"max_statement_time", ParamTypeReal, ApplyTypeDynamic}, + "idle_transaction_timeout": {"idle_transaction_timeout", ParamTypeInteger, ApplyTypeDynamic}, + "innodb_buffer_pool_size": {"innodb_buffer_pool_size", ParamTypeInteger, ApplyTypeStatic}, + "innodb_log_file_size": {"innodb_log_file_size", ParamTypeInteger, ApplyTypeStatic}, + "innodb_flush_method": {"innodb_flush_method", ParamTypeEnum, ApplyTypeStatic}, + "innodb_flush_log_at_trx_commit": {"innodb_flush_log_at_trx_commit", ParamTypeInteger, ApplyTypeDynamic}, + "innodb_read_io_threads": {"innodb_read_io_threads", ParamTypeInteger, ApplyTypeStatic}, + "innodb_write_io_threads": {"innodb_write_io_threads", ParamTypeInteger, ApplyTypeStatic}, + "innodb_purge_threads": {"innodb_purge_threads", ParamTypeInteger, ApplyTypeStatic}, + "innodb_autoinc_lock_mode": {"innodb_autoinc_lock_mode", ParamTypeInteger, ApplyTypeStatic}, + "innodb_io_capacity": {"innodb_io_capacity", ParamTypeInteger, ApplyTypeDynamic}, + "innodb_io_capacity_max": {"innodb_io_capacity_max", ParamTypeInteger, ApplyTypeDynamic}, + "innodb_max_dirty_pages_pct": {"innodb_max_dirty_pages_pct", ParamTypeReal, ApplyTypeDynamic}, + "innodb_flush_neighbors": {"innodb_flush_neighbors", ParamTypeInteger, ApplyTypeDynamic}, + "innodb_file_per_table": {"innodb_file_per_table", ParamTypeBoolean, ApplyTypeDynamic}, + "innodb_stats_persistent": {"innodb_stats_persistent", ParamTypeBoolean, ApplyTypeDynamic}, + "innodb_strict_mode": {"innodb_strict_mode", ParamTypeBoolean, ApplyTypeDynamic}, + "innodb_adaptive_hash_index": {"innodb_adaptive_hash_index", ParamTypeBoolean, ApplyTypeDynamic}, + "sort_buffer_size": {"sort_buffer_size", ParamTypeInteger, ApplyTypeDynamic}, + "join_buffer_size": {"join_buffer_size", ParamTypeInteger, ApplyTypeDynamic}, + "read_buffer_size": {"read_buffer_size", ParamTypeInteger, ApplyTypeDynamic}, + "read_rnd_buffer_size": {"read_rnd_buffer_size", ParamTypeInteger, ApplyTypeDynamic}, + "tmp_table_size": {"tmp_table_size", ParamTypeInteger, ApplyTypeDynamic}, + "max_heap_table_size": {"max_heap_table_size", ParamTypeInteger, ApplyTypeDynamic}, + "key_buffer_size": {"key_buffer_size", ParamTypeInteger, ApplyTypeDynamic}, + "table_open_cache": {"table_open_cache", ParamTypeInteger, ApplyTypeDynamic}, + "table_definition_cache": {"table_definition_cache", ParamTypeInteger, ApplyTypeDynamic}, + "max_prepared_stmt_count": {"max_prepared_stmt_count", ParamTypeInteger, ApplyTypeDynamic}, + "group_concat_max_len": {"group_concat_max_len", ParamTypeInteger, ApplyTypeDynamic}, + "performance_schema": {"performance_schema", ParamTypeBoolean, ApplyTypeStatic}, + "slow_query_log": {"slow_query_log", ParamTypeBoolean, ApplyTypeDynamic}, + "long_query_time": {"long_query_time", ParamTypeReal, ApplyTypeDynamic}, + "log_queries_not_using_indexes": {"log_queries_not_using_indexes", ParamTypeBoolean, ApplyTypeDynamic}, + "general_log": {"general_log", ParamTypeBoolean, ApplyTypeDynamic}, + "log_output": {"log_output", ParamTypeEnum, ApplyTypeDynamic}, + "log_warnings": {"log_warnings", ParamTypeInteger, ApplyTypeDynamic}, + "sql_mode": {"sql_mode", ParamTypeString, ApplyTypeDynamic}, + "character_set_server": {"character_set_server", ParamTypeEnum, ApplyTypeDynamic}, + "collation_server": {"collation_server", ParamTypeEnum, ApplyTypeDynamic}, + // The one remapped entry: mariadbd has no time_zone startup option, so the + // option file carries it under default_time_zone instead. + "time_zone": {"default_time_zone", ParamTypeString, ApplyTypeDynamic}, + "transaction_isolation": {"transaction_isolation", ParamTypeEnum, ApplyTypeDynamic}, + "autocommit": {"autocommit", ParamTypeBoolean, ApplyTypeDynamic}, + "event_scheduler": {"event_scheduler", ParamTypeEnum, ApplyTypeDynamic}, + "tls_version": {"tls_version", ParamTypeString, ApplyTypeStatic}, + "require_secure_transport": {"require_secure_transport", ParamTypeBoolean, ApplyTypeDynamic}, +} + +// Every catalog parameter of every engine, checked against a golden table +// transcribed from the catalogue source: its option-file spelling, data type +// and apply semantics. A package split that drops or renames one of these +// has to fail here rather than surface as a guest that refuses to start. +func TestCatalogueAgentContractMetadata_MatchesGoldenTable(t *testing.T) { + tests := []struct { + engine Engine + golden map[string]catalogAgentContract + }{ + {enginePostgres, postgresAgentContract}, + {engineMariaDB, mariadbAgentContract}, + } + for _, tc := range tests { + t.Run(tc.engine.Name, func(t *testing.T) { + names := tc.engine.CatalogParameterNames() + require.Len(t, names, len(tc.golden), "the catalogue gained or lost a parameter since this table was written") + for _, name := range names { + want, ok := tc.golden[name] + require.True(t, ok, "%s is not in the golden table", name) + + spec, ok := tc.engine.LookupParameter(name) + require.True(t, ok) + assert.Equal(t, want.dataType, spec.DataType, "%s DataType", name) + assert.Equal(t, want.applyType, spec.ApplyType, "%s ApplyType", name) + assert.Equal(t, want.optionFileName, tc.engine.OptionFileName(name), "%s OptionFileName", name) + } + }) + } +} diff --git a/spinifex/handlers/rds/engine_postgres.go b/spinifex/domains/rds/engine/postgres.go similarity index 99% rename from spinifex/handlers/rds/engine_postgres.go rename to spinifex/domains/rds/engine/postgres.go index 8e13d3e17..abd43e2a3 100644 --- a/spinifex/handlers/rds/engine_postgres.go +++ b/spinifex/domains/rds/engine/postgres.go @@ -1,4 +1,4 @@ -package handlers_rds +package engine import ( "strconv" @@ -460,8 +460,8 @@ func maintenanceWorkMemCeilingFor(memoryMiB int64) int64 { return clampInt64(memoryMiB*1024/2, 16384, 2147483647) } -func validatePostgresParameterCombinations(params []Parameter) error { - values := resolvedValues(params) +func validatePostgresParameterCombinations(settings []Setting) error { + values := resolvedValues(settings) maxWALSenders, err := resolvedInteger(values, "max_wal_senders") if err != nil { diff --git a/spinifex/domains/rds/engine/selection.go b/spinifex/domains/rds/engine/selection.go new file mode 100644 index 000000000..ab97a63d6 --- /dev/null +++ b/spinifex/domains/rds/engine/selection.go @@ -0,0 +1,144 @@ +package engine + +import ( + "slices" + "strconv" + "strings" +) + +// The status every offered version carries. One version per engine and it is +// the one the AMI ships, so nothing is ever deprecated, pending or beta. +// +// Duplicated against handlers_rds's own copy deliberately: this one backs the +// selection filter here, that one backs the AWS SDK projection there. +const engineVersionStatusAvailable = "available" + +// Every endpoint is a private VPC address, so a vpc=false filter matches +// nothing. Duplicated against handlers_rds's own copy for the same reason as +// engineVersionStatusAvailable above. +const orderableVpc = true + +// ValueSets is a conjunction of accepted-value sets, one per source: the typed +// parameter and each Filters entry that names the same field. A row must satisfy +// every set, so naming two different engines matches nothing rather than both. +type ValueSets [][]string + +// AddParam constrains by a typed parameter. An omitted one narrows nothing, +// which is why an empty value is not recorded as a set that matches nothing. +func (v *ValueSets) AddParam(value string) { + if strings.TrimSpace(value) == "" { + return + } + *v = append(*v, []string{normaliseFilterValue(value)}) +} + +// AddFilter constrains by one Filters entry. Unlike AddParam an empty or +// unmatchable value is kept, because the caller wrote the filter deliberately. +func (v *ValueSets) AddFilter(values []string) { + set := make([]string, 0, len(values)) + for _, value := range values { + set = append(set, normaliseFilterValue(value)) + } + *v = append(*v, set) +} + +// A free function rather than a method, so ValueSets keeps the pointer receivers +// its two mutators need without mixing the two receiver kinds on one type. +func accepts(sets ValueSets, value string) bool { + value = normaliseFilterValue(value) + for _, set := range sets { + if !slices.Contains(set, value) { + return false + } + } + return true +} + +// Every value either side of the comparison is a lowercase identifier already, +// so folding here only makes a shouted filter work rather than widening a match. +func normaliseFilterValue(value string) string { + return strings.ToLower(strings.TrimSpace(value)) +} + +// EngineVersionFilter narrows the engine-version catalog. Every field is +// optional; a zero filter returns every row. +type EngineVersionFilter struct { + Engine ValueSets + EngineVersion ValueSets + ParameterGroupFamily ValueSets + Status ValueSets +} + +func (f EngineVersionFilter) matches(e Engine) bool { + return accepts(f.Engine, e.Name) && + accepts(f.EngineVersion, e.EngineVersion()) && + accepts(f.ParameterGroupFamily, e.ParameterGroupFamily()) && + accepts(f.Status, engineVersionStatusAvailable) +} + +// OrderableFilter narrows the orderable-option catalog. Vpc is a value set like +// the rest, holding "true" or "false", so an unset filter stays distinct from +// one asking for non-VPC options. +type OrderableFilter struct { + Engine ValueSets + EngineVersion ValueSets + DBInstanceClass ValueSets + LicenseModel ValueSets + Vpc ValueSets +} + +func (f OrderableFilter) matchesEngine(e Engine) bool { + return accepts(f.Engine, e.Name) && + accepts(f.EngineVersion, e.EngineVersion()) && + accepts(f.LicenseModel, e.licenseModel) && + accepts(f.Vpc, strconv.FormatBool(orderableVpc)) +} + +// EngineVersions is the engine half of the catalog: one row per engine, since +// v1 pins a single major each and that pin is the only version an AMI serves. +// The result is engine-owned; a caller that needs an AWS SDK projection builds +// it from these rather than this package depending on the SDK. +func EngineVersions(filter EngineVersionFilter) []Engine { + out := make([]Engine, 0, len(engines)) + for _, name := range SupportedEngines() { + e := engines[name] + if !filter.matches(e) { + continue + } + out = append(out, e) + } + return out +} + +// OrderableOption is one orderable engine/class combination that passed every +// filter and whose EC2 instance type runnable accepted, ready for a caller to +// project into its own output shape. +type OrderableOption struct { + Engine Engine + DBInstanceClass string +} + +// OrderableOptions is the cross product of the engines, their pinned version and +// the db.* classes, minus every class whose EC2 instance type runnable rejects. +// runnable is the cluster's own answer to "can a node run this", which is the +// difference between a class that validates and one that can actually launch. +func OrderableOptions(filter OrderableFilter, sizing Sizing, runnable func(instanceType string) bool) []OrderableOption { + out := make([]OrderableOption, 0, len(engines)*len(dbInstanceClasses)) + for _, name := range SupportedEngines() { + e := engines[name] + if !filter.matchesEngine(e) { + continue + } + for _, class := range SupportedInstanceClasses() { + if !accepts(filter.DBInstanceClass, class) { + continue + } + instanceType, err := sizing.InstanceTypeForClass(class) + if err != nil || !runnable(instanceType) { + continue + } + out = append(out, OrderableOption{Engine: e, DBInstanceClass: class}) + } + } + return out +} diff --git a/spinifex/domains/rds/engine/selection_test.go b/spinifex/domains/rds/engine/selection_test.go new file mode 100644 index 000000000..250e075ef --- /dev/null +++ b/spinifex/domains/rds/engine/selection_test.go @@ -0,0 +1,193 @@ +package engine + +//test:in-package — reaches the unexported engines/dbInstanceClasses registries +// and engineVersionStatusAvailable, none of which this package exports. + +import ( + "strings" + "testing" + + "github.com/mulgadc/spinifex/spinifex/domains/ec2/instancetypes" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func runsEverything(string) bool { return true } + +func runsNothing(string) bool { return false } + +// Both are reported verbatim by the two catalogs, and the licence model is +// filterable, so an engine registering neither would answer a --license-model +// filter with an empty row set rather than with its own licence. +func TestEngines_RegisterADescriptionAndALicenceModel(t *testing.T) { + t.Parallel() + for _, name := range SupportedEngines() { + t.Run(name, func(t *testing.T) { + t.Parallel() + engine, err := LookupEngine(name) + require.NoError(t, err) + assert.NotEmpty(t, engine.Description()) + assert.NotEmpty(t, engine.LicenseModel()) + }) + } +} + +// db.X is the EC2 instance type X, which is what lets a client read vCPU and +// memory from ec2:DescribeInstanceTypes for the identically named type. Asserted +// by name rather than by footprint: t3.large and m5.large are both 2 vCPU and +// 8 GiB, so repointing db.t3.large at m5.large would break the documented +// identity and pass a footprint comparison unchanged. +func TestInstanceTypeForClass_IsTheClassNameWithoutItsPrefix(t *testing.T) { + t.Parallel() + for _, class := range SupportedInstanceClasses() { + t.Run(class, func(t *testing.T) { + t.Parallel() + instanceType, err := testSizing.InstanceTypeForClass(class) + require.NoError(t, err) + assert.Equal(t, strings.TrimPrefix(class, "db."), instanceType) + + // Both lookups prove the name is known to instancetypes, which is what + // makes SmallestInstanceClass safe: it silently skips a class whose + // footprint will not resolve. + _, knownVCPUs := instancetypes.DefaultVCPUs(instanceType) + assert.True(t, knownVCPUs, "instancetypes should know the vCPU count of %s", instanceType) + _, knownMemory := instancetypes.DefaultMemoryMiB(instanceType) + assert.True(t, knownMemory, "instancetypes should know the memory of %s", instanceType) + }) + } +} + +func TestEngineVersionFilter_NarrowsByEveryRecognisedField(t *testing.T) { + t.Parallel() + postgres, err := LookupEngine("postgres") + require.NoError(t, err) + + cases := []struct { + name string + filter func() EngineVersionFilter + want []string + }{ + {"unfiltered", func() EngineVersionFilter { return EngineVersionFilter{} }, SupportedEngines()}, + {"engine", func() (f EngineVersionFilter) { + f.Engine.AddParam("postgres") + return f + }, []string{"postgres"}}, + {"engine is case insensitive", func() (f EngineVersionFilter) { + f.Engine.AddFilter([]string{"PostgreSQL", "POSTGRES"}) + return f + }, []string{"postgres"}}, + {"engine version", func() (f EngineVersionFilter) { + f.EngineVersion.AddParam(postgres.MajorVersion) + return f + }, []string{"postgres"}}, + {"parameter group family", func() (f EngineVersionFilter) { + f.ParameterGroupFamily.AddParam(postgres.ParameterGroupFamily()) + return f + }, []string{"postgres"}}, + {"status", func() (f EngineVersionFilter) { + f.Status.AddFilter([]string{engineVersionStatusAvailable}) + return f + }, SupportedEngines()}, + {"deprecated status matches nothing", func() (f EngineVersionFilter) { + f.Status.AddFilter([]string{"deprecated"}) + return f + }, nil}, + {"unknown engine matches nothing", func() (f EngineVersionFilter) { + f.Engine.AddParam("oracle-ee") + return f + }, nil}, + {"unpinned version matches nothing", func() (f EngineVersionFilter) { + f.EngineVersion.AddParam("17") + return f + }, nil}, + // Two sources naming different engines is a conjunction, not a union: a + // row has to satisfy both, and no row is two engines. + {"typed parameter and filter are both applied", func() (f EngineVersionFilter) { + f.Engine.AddParam("postgres") + f.Engine.AddFilter([]string{"mariadb"}) + return f + }, nil}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var got []string + for _, row := range EngineVersions(tc.filter()) { + got = append(got, row.Name) + } + assert.Equal(t, tc.want, got) + }) + } +} + +func TestOrderableFilter_NarrowsByEveryRecognisedField(t *testing.T) { + t.Parallel() + postgres, err := LookupEngine("postgres") + require.NoError(t, err) + classes := len(SupportedInstanceClasses()) + + cases := []struct { + name string + filter func() OrderableFilter + want int + }{ + {"unfiltered", func() OrderableFilter { return OrderableFilter{} }, len(SupportedEngines()) * classes}, + {"engine", func() (f OrderableFilter) { + f.Engine.AddParam("postgres") + return f + }, classes}, + {"engine version", func() (f OrderableFilter) { + f.EngineVersion.AddParam(postgres.MajorVersion) + return f + }, classes}, + {"instance class", func() (f OrderableFilter) { + f.DBInstanceClass.AddParam("db.t3.micro") + return f + }, len(SupportedEngines())}, + {"license model", func() (f OrderableFilter) { + f.LicenseModel.AddParam(postgres.LicenseModel()) + return f + }, classes}, + {"vpc true", func() (f OrderableFilter) { + f.Vpc.AddParam("true") + return f + }, len(SupportedEngines()) * classes}, + // Every endpoint is a private VPC address, so asking for a non-VPC option + // is an empty list rather than the whole catalog. + {"vpc false", func() (f OrderableFilter) { + f.Vpc.AddParam("false") + return f + }, 0}, + {"unknown class matches nothing", func() (f OrderableFilter) { + f.DBInstanceClass.AddParam("db.r5.24xlarge") + return f + }, 0}, + {"unpinned version matches nothing", func() (f OrderableFilter) { + f.EngineVersion.AddParam("17") + return f + }, 0}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Len(t, OrderableOptions(tc.filter(), testSizing, runsEverything), tc.want) + }) + } +} + +// A class the cluster's nodes cannot run is never offered, and a cluster that +// runs none of them is an empty list rather than an error. +func TestOrderableOptions_FilterOnClusterCapability(t *testing.T) { + t.Parallel() + onlyMicro := OrderableOptions(OrderableFilter{}, testSizing, func(instanceType string) bool { + return instanceType == "t3.micro" + }) + require.Len(t, onlyMicro, len(SupportedEngines())) + for _, option := range onlyMicro { + assert.Equal(t, "db.t3.micro", option.DBInstanceClass) + } + + assert.Empty(t, OrderableOptions(OrderableFilter{}, testSizing, runsNothing)) +} diff --git a/spinifex/domains/rds/engine/sizing.go b/spinifex/domains/rds/engine/sizing.go new file mode 100644 index 000000000..632cfa77a --- /dev/null +++ b/spinifex/domains/rds/engine/sizing.go @@ -0,0 +1,107 @@ +package engine + +import ( + "errors" + "maps" + "slices" + "strings" + + "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" +) + +// db.* is a naming facade, not a second sizing table: every entry resolves +// through the platform's instance-type definitions. +var dbInstanceClasses = map[string]string{ + "db.t3.micro": "t3.micro", + "db.t3.small": "t3.small", + "db.t3.medium": "t3.medium", + "db.t3.large": "t3.large", + "db.m5.large": "m5.large", + "db.m5.xlarge": "m5.xlarge", +} + +// InstanceTypes is the narrow EC2 fact this package needs: an instance type's +// memory footprint. The production implementation, backed by domains/ec2/ +// instancetypes, is composed outside this package by whoever builds a Sizing. +type InstanceTypes interface { + MemoryMiB(instanceType string) (memoryMiB int64, ok bool) +} + +// Sizing is the class→memory projection every size-derived parameter default is +// computed from. It is built once, at composition time: no live availability, +// no cluster state, no NATS. +type Sizing struct { + memoryMiB map[string]int64 +} + +// NewSizing precomputes the db.* class to memory projection from types. A class +// whose backing EC2 type types does not know is excluded rather than failing +// construction, so one unsupported class does not take the whole catalog down; +// InstanceTypeForClass reports the same exclusion at validation. types is a +// composition-time dependency; a nil one is a wiring bug that fails on first +// use here, not a condition this constructor reports. +func NewSizing(types InstanceTypes) Sizing { + memoryMiB := make(map[string]int64, len(dbInstanceClasses)) + for class, instanceType := range dbInstanceClasses { + if mem, ok := types.MemoryMiB(instanceType); ok && mem > 0 { + memoryMiB[class] = mem + } + } + return Sizing{memoryMiB: memoryMiB} +} + +// InstanceTypeForClass maps a db.* instance class to the EC2 instance type backing it. An unknown class +// is rejected here at validation rather than surfacing as a launch failure after the volume and ENI exist. +func (s Sizing) InstanceTypeForClass(class string) (string, error) { + instanceType, ok := dbInstanceClasses[class] + if !ok { + return "", errors.New(awserrors.ErrorInvalidParameterValue) + } + if _, known := s.memoryMiB[class]; !known { + return "", errors.New(awserrors.ErrorInvalidParameterValue) + } + return instanceType, nil +} + +// SupportedInstanceClasses returns every db.* instance class this platform offers, sorted alphabetically. +// Unlike InstanceTypeForClass this does not depend on Sizing: the name facade is static regardless of +// which classes a particular instance-types source can resolve. +func SupportedInstanceClasses() []string { + return slices.Sorted(maps.Keys(dbInstanceClasses)) +} + +// SmallestInstanceClass returns the supported class with the least memory. Deliberately computed from +// the footprints rather than taken as the first name SupportedInstanceClasses reports: that list is +// sorted alphabetically, so its head is db.m5.large. +func (s Sizing) SmallestInstanceClass() string { + smallest, least := "", int64(0) + for _, class := range SupportedInstanceClasses() { + memoryMiB, err := s.ClassMemoryMiB(class) + // A class with no known footprint cannot be compared; the catalog's + // self-consistency test pins that every supported class has one. + if err != nil { + continue + } + if smallest == "" || memoryMiB < least { + smallest, least = class, memoryMiB + } + } + return smallest +} + +// ClassMemoryMiB returns the memory an instance class's guest has, which every +// size-derived default is computed from. An unknown class is a validation +// failure upstream, so this is the last line rather than the check. +func (s Sizing) ClassMemoryMiB(instanceClass string) (int64, error) { + instanceType, err := s.InstanceTypeForClass(instanceClass) + if err != nil { + return 0, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBInstanceClass %q is not supported; supported classes are %s", instanceClass, strings.Join(SupportedInstanceClasses(), ", ")) + } + memoryMiB, ok := s.memoryMiB[instanceClass] + if !ok || memoryMiB <= 0 { + return 0, awserrors.Errorf(awserrors.ErrorServerInternal, + "no memory footprint is known for instance type %s", instanceType) + } + return memoryMiB, nil +} diff --git a/spinifex/handlers/rds/sizing_test.go b/spinifex/domains/rds/engine/sizing_test.go similarity index 77% rename from spinifex/handlers/rds/sizing_test.go rename to spinifex/domains/rds/engine/sizing_test.go index 059e43849..8b4ce1947 100644 --- a/spinifex/handlers/rds/sizing_test.go +++ b/spinifex/domains/rds/engine/sizing_test.go @@ -1,4 +1,4 @@ -package handlers_rds +package engine import ( "testing" @@ -20,7 +20,7 @@ func TestInstanceTypeForClass_KnownClasses(t *testing.T) { "db.m5.xlarge": "m5.xlarge", } for class, want := range tests { - got, err := InstanceTypeForClass(class) + got, err := testSizing.InstanceTypeForClass(class) require.NoError(t, err, "class %q", class) assert.Equal(t, want, got, "class %q", class) } @@ -35,7 +35,7 @@ func TestInstanceTypeForClass_UnmappedClassRejected(t *testing.T) { "db.r5.large", "db.m5.24xlarge", "db.t3.nano", "db.serverless", "m5.large", "", "db.", "DB.T3.MICRO", } { - _, err := InstanceTypeForClass(class) + _, err := testSizing.InstanceTypeForClass(class) require.Error(t, err, "class %q should be rejected", class) assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error(), "class %q", class) } @@ -67,13 +67,23 @@ func TestSupportedInstanceClasses_SortedAndComplete(t *testing.T) { // no small instance ever runs. func TestSmallestInstanceClass_IsTheLeastMemory(t *testing.T) { t.Parallel() - assert.Equal(t, "db.t3.micro", SmallestInstanceClass()) + assert.Equal(t, "db.t3.micro", testSizing.SmallestInstanceClass()) - least, err := classMemoryMiB(SmallestInstanceClass()) + least, err := testSizing.ClassMemoryMiB(testSizing.SmallestInstanceClass()) require.NoError(t, err) for _, class := range SupportedInstanceClasses() { - memoryMiB, err := classMemoryMiB(class) + memoryMiB, err := testSizing.ClassMemoryMiB(class) require.NoError(t, err, "every supported class needs a known footprint") assert.LessOrEqual(t, least, memoryMiB, "class %q is smaller than the one reported as smallest", class) } } + +// The literal DescribeDBParameters' size-derived defaults resolve against for +// the smallest class: db.t3.micro's 1 GiB, stated here as the MiB ClassMemoryMiB +// actually returns rather than only the ordering checked above. +func TestSmallestInstanceClass_MemoryIsExactlyOneGiB(t *testing.T) { + t.Parallel() + memoryMiB, err := testSizing.ClassMemoryMiB(testSizing.SmallestInstanceClass()) + require.NoError(t, err) + assert.Equal(t, int64(1024), memoryMiB) +} diff --git a/spinifex/domains/rds/engine/testsizing_test.go b/spinifex/domains/rds/engine/testsizing_test.go new file mode 100644 index 000000000..d81f482d9 --- /dev/null +++ b/spinifex/domains/rds/engine/testsizing_test.go @@ -0,0 +1,17 @@ +package engine + +//test:in-package — every test file in this package that needs a Sizing shares +// this fixture, built from the real EC2 instance-type table rather than a fake: +// the size-derived defaults are asserted against real memory footprints. + +import ( + "github.com/mulgadc/spinifex/spinifex/domains/ec2/instancetypes" +) + +type testInstanceTypes struct{} + +func (testInstanceTypes) MemoryMiB(instanceType string) (int64, bool) { + return instancetypes.DefaultMemoryMiB(instanceType) +} + +var testSizing = NewSizing(testInstanceTypes{}) diff --git a/spinifex/gateway/rds/enginecatalog.go b/spinifex/gateway/rds/enginecatalog.go index d4b72b253..f74887ff6 100644 --- a/spinifex/gateway/rds/enginecatalog.go +++ b/spinifex/gateway/rds/enginecatalog.go @@ -12,6 +12,7 @@ import ( "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/rds" ec2instanceapi "github.com/mulgadc/spinifex/spinifex/domains/ec2/awsapi/instance" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" "github.com/nats-io/nats.go" @@ -29,6 +30,10 @@ const ( filterNameVpc = "vpc" ) +// rdsSizing is this gateway's composition of the engine package's class-to- +// memory projection, built once from the platform's instance-type table. +var rdsSizing = handlers_rds.InstanceSizing() + var ( engineVersionFilterNames = []string{ filterNameEngine, filterNameEngineVersion, filterNameParameterGroupFamily, filterNameStatus, @@ -42,7 +47,7 @@ var ( // rather than a required parameter, and an unknown one is an empty list rather than the rejection // create-db-instance gives it. func DescribeDBEngineVersions(ctx context.Context, input *rds.DescribeDBEngineVersionsInput, _ *nats.Conn, _ Caller) (any, error) { - filter := handlers_rds.EngineVersionFilter{} + filter := rdsengine.EngineVersionFilter{} filter.Engine.AddParam(aws.StringValue(input.Engine)) filter.EngineVersion.AddParam(aws.StringValue(input.EngineVersion)) filter.ParameterGroupFamily.AddParam(aws.StringValue(input.DBParameterGroupFamily)) @@ -85,12 +90,12 @@ func DescribeOrderableDBInstanceOptions(ctx context.Context, input *rds.Describe if strings.TrimSpace(aws.StringValue(input.Engine)) == "" { return nil, awserrors.Errorf(awserrors.ErrorMissingParameter, "Engine is required") } - engine, err := handlers_rds.LookupEngine(aws.StringValue(input.Engine)) + engine, err := rdsengine.LookupEngine(aws.StringValue(input.Engine)) if err != nil { return nil, err } - filter := handlers_rds.OrderableFilter{} + filter := rdsengine.OrderableFilter{} filter.Engine.AddParam(engine.Name) filter.EngineVersion.AddParam(aws.StringValue(input.EngineVersion)) filter.DBInstanceClass.AddParam(aws.StringValue(input.DBInstanceClass)) @@ -126,7 +131,7 @@ func DescribeOrderableDBInstanceOptions(ctx context.Context, input *rds.Describe if err != nil { return nil, err } - options, marker, err := handlers_rds.Page(handlers_rds.OrderableOptions(filter, runnable), + options, marker, err := handlers_rds.Page(handlers_rds.OrderableOptions(filter, rdsSizing, runnable), handlers_rds.OrderableOptionPageKey, input.MaxRecords, input.Marker) if err != nil { return nil, err diff --git a/spinifex/gateway/rds/enginecatalog_test.go b/spinifex/gateway/rds/enginecatalog_test.go index 700675efc..27baf37bb 100644 --- a/spinifex/gateway/rds/enginecatalog_test.go +++ b/spinifex/gateway/rds/enginecatalog_test.go @@ -14,8 +14,8 @@ import ( "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/rds" "github.com/mulgadc/spinifex/internal/testkit" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" - handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" "github.com/nats-io/nats.go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -65,8 +65,8 @@ func classNames(options []*rds.OrderableDBInstanceOption) []string { // find one written any other way. func TestDispatch_DescribeDBEngineVersionsRendersTheNestedList(t *testing.T) { rows := describeEngineVersions(t, map[string]string{}) - require.Len(t, rows, len(handlers_rds.SupportedEngines())) - assert.Equal(t, handlers_rds.SupportedEngines(), engineNames(rows)) + require.Len(t, rows, len(rdsengine.SupportedEngines())) + assert.Equal(t, rdsengine.SupportedEngines(), engineNames(rows)) for _, row := range rows { assert.NotEmpty(t, aws.StringValue(row.EngineVersion)) @@ -77,8 +77,8 @@ func TestDispatch_DescribeDBEngineVersionsRendersTheNestedList(t *testing.T) { func TestDispatch_DescribeOrderableDBInstanceOptionsRendersTheNestedList(t *testing.T) { options := describeOrderable(t, newStubbedNATS(t), map[string]string{"Engine": "postgres"}) - require.Len(t, options, len(handlers_rds.SupportedInstanceClasses())) - assert.Equal(t, handlers_rds.SupportedInstanceClasses(), classNames(options)) + require.Len(t, options, len(rdsengine.SupportedInstanceClasses())) + assert.Equal(t, rdsengine.SupportedInstanceClasses(), classNames(options)) for _, option := range options { assert.Equal(t, "postgres", aws.StringValue(option.Engine)) @@ -102,7 +102,7 @@ func TestDescribeDBEngineVersions_AppliesTypedParametersAndFilters(t *testing.T) "Filters.Filter.1.Name": "engine", "Filters.Filter.1.Values.Value.1": "postgres", "Filters.Filter.1.Values.Value.2": "mariadb", - }, handlers_rds.SupportedEngines()}, + }, rdsengine.SupportedEngines()}, {"engine version parameter", map[string]string{"EngineVersion": "18"}, []string{"postgres"}}, {"parameter group family parameter", map[string]string{"DBParameterGroupFamily": "mariadb11.8"}, []string{"mariadb"}}, @@ -111,7 +111,7 @@ func TestDescribeDBEngineVersions_AppliesTypedParametersAndFilters(t *testing.T) }, []string{"postgres"}}, {"status filter", map[string]string{ "Filters.Filter.1.Name": "status", "Filters.Filter.1.Values.Value.1": "available", - }, handlers_rds.SupportedEngines()}, + }, rdsengine.SupportedEngines()}, // A query for a row that does not exist is empty; only a request to build // something that cannot exist is refused. {"unpinned version", map[string]string{"EngineVersion": "17"}, []string{}}, @@ -127,7 +127,7 @@ func TestDescribeDBEngineVersions_AppliesTypedParametersAndFilters(t *testing.T) func TestDescribeOrderableDBInstanceOptions_AppliesTypedParametersAndFilters(t *testing.T) { nc := newStubbedNATS(t) - all := handlers_rds.SupportedInstanceClasses() + all := rdsengine.SupportedInstanceClasses() cases := []struct { name string @@ -273,7 +273,7 @@ func TestDescribeOrderableDBInstanceOptions_PagesTheCatalog(t *testing.T) { break } } - assert.Equal(t, handlers_rds.SupportedInstanceClasses(), classes) + assert.Equal(t, rdsengine.SupportedInstanceClasses(), classes) } func TestDescribeCatalogs_RejectAMarkerTheyDidNotIssue(t *testing.T) { diff --git a/spinifex/handlers/rds/catalog.go b/spinifex/handlers/rds/catalog.go index bbe6c8d5e..7c2a142d0 100644 --- a/spinifex/handlers/rds/catalog.go +++ b/spinifex/handlers/rds/catalog.go @@ -1,16 +1,16 @@ package handlers_rds import ( - "slices" - "strconv" - "strings" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/rds" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" ) // The status every offered version carries. One version per engine and it is // the one the AMI ships, so nothing is ever deprecated, pending or beta. +// +// Duplicated against the engine package's own copy deliberately: that one +// backs the selection filter there, this one backs the SDK projection here. const engineVersionStatusAvailable = "available" // The only network type offered. Advertising DUAL would promise an address @@ -18,120 +18,29 @@ const engineVersionStatusAvailable = "available" const networkTypeIPv4 = "IPV4" // Every endpoint is a private VPC address, so a vpc=false filter matches -// nothing. Named once so the filter and the reported field cannot drift apart. +// nothing. Duplicated against the engine package's own copy for the same +// reason as engineVersionStatusAvailable above. const orderableVpc = true -// ValueSets is a conjunction of accepted-value sets, one per source: the typed -// parameter and each Filters entry that names the same field. A row must satisfy -// every set, so naming two different engines matches nothing rather than both. -type ValueSets [][]string - -// AddParam constrains by a typed parameter. An omitted one narrows nothing, -// which is why an empty value is not recorded as a set that matches nothing. -func (v *ValueSets) AddParam(value string) { - if strings.TrimSpace(value) == "" { - return - } - *v = append(*v, []string{normaliseFilterValue(value)}) -} - -// AddFilter constrains by one Filters entry. Unlike AddParam an empty or -// unmatchable value is kept, because the caller wrote the filter deliberately. -func (v *ValueSets) AddFilter(values []string) { - set := make([]string, 0, len(values)) - for _, value := range values { - set = append(set, normaliseFilterValue(value)) - } - *v = append(*v, set) -} - -// A free function rather than a method, so ValueSets keeps the pointer receivers -// its two mutators need without mixing the two receiver kinds on one type. -func accepts(sets ValueSets, value string) bool { - value = normaliseFilterValue(value) - for _, set := range sets { - if !slices.Contains(set, value) { - return false - } - } - return true -} - -// Every value either side of the comparison is a lowercase identifier already, -// so folding here only makes a shouted filter work rather than widening a match. -func normaliseFilterValue(value string) string { - return strings.ToLower(strings.TrimSpace(value)) -} - -// EngineVersionFilter narrows the engine-version catalog. Every field is -// optional; a zero filter returns every row. -type EngineVersionFilter struct { - Engine ValueSets - EngineVersion ValueSets - ParameterGroupFamily ValueSets - Status ValueSets -} - -func (f EngineVersionFilter) matches(e Engine) bool { - return accepts(f.Engine, e.Name) && - accepts(f.EngineVersion, e.EngineVersion()) && - accepts(f.ParameterGroupFamily, e.ParameterGroupFamily()) && - accepts(f.Status, engineVersionStatusAvailable) -} - -// OrderableFilter narrows the orderable-option catalog. Vpc is a value set like -// the rest, holding "true" or "false", so an unset filter stays distinct from -// one asking for non-VPC options. -type OrderableFilter struct { - Engine ValueSets - EngineVersion ValueSets - DBInstanceClass ValueSets - LicenseModel ValueSets - Vpc ValueSets -} - -func (f OrderableFilter) matchesEngine(e Engine) bool { - return accepts(f.Engine, e.Name) && - accepts(f.EngineVersion, e.EngineVersion()) && - accepts(f.LicenseModel, e.licenseModel) && - accepts(f.Vpc, strconv.FormatBool(orderableVpc)) -} - -// EngineVersions is the engine half of the catalog: one row per engine, since -// v1 pins a single major each and that pin is the only version an AMI serves. -func EngineVersions(filter EngineVersionFilter) []*rds.DBEngineVersion { +// EngineVersions is the AWS SDK projection of the engine package's own +// selection: it builds the DBEngineVersion rows this handler reports, from the +// engine-owned result the selection itself does not depend on the SDK for. +func EngineVersions(filter rdsengine.EngineVersionFilter) []*rds.DBEngineVersion { + engines := rdsengine.EngineVersions(filter) out := make([]*rds.DBEngineVersion, 0, len(engines)) - for _, name := range SupportedEngines() { - engine := engines[name] - if !filter.matches(engine) { - continue - } - out = append(out, engine.describeVersion()) + for _, e := range engines { + out = append(out, describeVersion(e)) } return out } -// OrderableOptions is the cross product of the engines, their pinned version and -// the db.* classes, minus every class whose EC2 instance type runnable rejects. -// runnable is the cluster's own answer to "can a node run this", which is the -// difference between a class that validates and one that can actually launch. -func OrderableOptions(filter OrderableFilter, runnable func(instanceType string) bool) []*rds.OrderableDBInstanceOption { - out := make([]*rds.OrderableDBInstanceOption, 0, len(engines)*len(dbInstanceClasses)) - for _, name := range SupportedEngines() { - engine := engines[name] - if !filter.matchesEngine(engine) { - continue - } - for _, class := range SupportedInstanceClasses() { - if !accepts(filter.DBInstanceClass, class) { - continue - } - instanceType, err := InstanceTypeForClass(class) - if err != nil || !runnable(instanceType) { - continue - } - out = append(out, engine.orderableOption(class)) - } +// OrderableOptions is the AWS SDK projection of the engine package's own +// selection, for the same reason as EngineVersions above. +func OrderableOptions(filter rdsengine.OrderableFilter, sizing rdsengine.Sizing, runnable func(instanceType string) bool) []*rds.OrderableDBInstanceOption { + options := rdsengine.OrderableOptions(filter, sizing, runnable) + out := make([]*rds.OrderableDBInstanceOption, 0, len(options)) + for _, option := range options { + out = append(out, orderableOption(option.Engine, option.DBInstanceClass)) } return out } @@ -139,14 +48,14 @@ func OrderableOptions(filter OrderableFilter, runnable func(instanceType string) // Everything the platform has a truth for. Fields the SDK struct carries that // nothing here answers — the custom-engine manifest, the CA identifiers, the // installation-file locations — are left nil rather than guessed at. -func (e Engine) describeVersion() *rds.DBEngineVersion { +func describeVersion(e rdsengine.Engine) *rds.DBEngineVersion { return &rds.DBEngineVersion{ Engine: aws.String(e.Name), EngineVersion: aws.String(e.EngineVersion()), MajorEngineVersion: aws.String(e.MajorVersion), DBParameterGroupFamily: aws.String(e.ParameterGroupFamily()), - DBEngineDescription: aws.String(e.description), - DBEngineVersionDescription: aws.String(e.description + " " + e.MajorVersion), + DBEngineDescription: aws.String(e.Description()), + DBEngineVersionDescription: aws.String(e.Description() + " " + e.MajorVersion), Status: aws.String(engineVersionStatusAvailable), // Empty rather than absent: there is no in-place upgrade to target, no log @@ -175,12 +84,12 @@ func (e Engine) describeVersion() *rds.DBEngineVersion { // Every false below restates a line of rejectUnimplemented or of the // accepted-but-inert set, so an option cannot advertise a capability the create // path refuses. -func (e Engine) orderableOption(class string) *rds.OrderableDBInstanceOption { +func orderableOption(e rdsengine.Engine, class string) *rds.OrderableDBInstanceOption { return &rds.OrderableDBInstanceOption{ Engine: aws.String(e.Name), EngineVersion: aws.String(e.EngineVersion()), DBInstanceClass: aws.String(class), - LicenseModel: aws.String(e.licenseModel), + LicenseModel: aws.String(e.LicenseModel()), StorageType: aws.String(storageTypeGP3), MinStorageSize: aws.Int64(minAllocatedStorageGiB), diff --git a/spinifex/handlers/rds/catalog_test.go b/spinifex/handlers/rds/catalog_test.go index 148406ca0..fefcc49c1 100644 --- a/spinifex/handlers/rds/catalog_test.go +++ b/spinifex/handlers/rds/catalog_test.go @@ -5,50 +5,31 @@ package handlers_rds // constants deliberately, so the option cannot claim what create refuses. import ( - "strings" "testing" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/rds" - "github.com/mulgadc/spinifex/spinifex/domains/ec2/instancetypes" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func runsEverything(string) bool { return true } -func runsNothing(string) bool { return false } - -// Both are reported verbatim by the two catalogs, and the licence model is -// filterable, so an engine registering neither would answer a --license-model -// filter with an empty row set rather than with its own licence. -func TestEngines_RegisterADescriptionAndALicenceModel(t *testing.T) { - t.Parallel() - for _, name := range SupportedEngines() { - t.Run(name, func(t *testing.T) { - t.Parallel() - engine, err := LookupEngine(name) - require.NoError(t, err) - assert.NotEmpty(t, engine.Description()) - assert.NotEmpty(t, engine.LicenseModel()) - }) - } -} - -// The catalog is a read of engine.go, so a pin bump has to fail here rather than -// in a client that hardcoded the old one. +// The catalog is a read of the engine package's registry, so a pin bump has to +// fail here rather than in a client that hardcoded the old one. func TestEngineVersions_ReportWhatTheEngineTableSays(t *testing.T) { t.Parallel() byEngine := map[string]*rds.DBEngineVersion{} - for _, row := range EngineVersions(EngineVersionFilter{}) { + for _, row := range EngineVersions(rdsengine.EngineVersionFilter{}) { byEngine[aws.StringValue(row.Engine)] = row } - require.Len(t, byEngine, len(SupportedEngines())) + require.Len(t, byEngine, len(rdsengine.SupportedEngines())) - for _, name := range SupportedEngines() { + for _, name := range rdsengine.SupportedEngines() { t.Run(name, func(t *testing.T) { t.Parallel() - engine, err := LookupEngine(name) + engine, err := rdsengine.LookupEngine(name) require.NoError(t, err) row := byEngine[name] require.NotNil(t, row) @@ -71,7 +52,7 @@ func TestEngineVersions_ReportWhatTheEngineTableSays(t *testing.T) { // set or timezone, so every one of these is empty rather than unpopulated. func TestEngineVersions_ReportNoCapabilityThePlatformLacks(t *testing.T) { t.Parallel() - for _, row := range EngineVersions(EngineVersionFilter{}) { + for _, row := range EngineVersions(rdsengine.EngineVersionFilter{}) { name := aws.StringValue(row.Engine) assert.Empty(t, row.ValidUpgradeTarget, "%s should offer no upgrade target", name) assert.Empty(t, row.ExportableLogTypes, "%s should export no log type", name) @@ -86,17 +67,17 @@ func TestEngineVersions_ReportNoCapabilityThePlatformLacks(t *testing.T) { func TestOrderableOptions_CoverEveryEngineAndClass(t *testing.T) { t.Parallel() - options := OrderableOptions(OrderableFilter{}, runsEverything) - require.Len(t, options, len(SupportedEngines())*len(SupportedInstanceClasses())) + options := OrderableOptions(rdsengine.OrderableFilter{}, InstanceSizing(), runsEverything) + require.Len(t, options, len(rdsengine.SupportedEngines())*len(rdsengine.SupportedInstanceClasses())) for _, option := range options { label := aws.StringValue(option.Engine) + "/" + aws.StringValue(option.DBInstanceClass) - engine, err := LookupEngine(aws.StringValue(option.Engine)) + engine, err := rdsengine.LookupEngine(aws.StringValue(option.Engine)) require.NoError(t, err, label) assert.Equal(t, engine.EngineVersion(), aws.StringValue(option.EngineVersion), label) assert.Equal(t, engine.LicenseModel(), aws.StringValue(option.LicenseModel), label) - assert.Contains(t, SupportedInstanceClasses(), aws.StringValue(option.DBInstanceClass), label) + assert.Contains(t, rdsengine.SupportedInstanceClasses(), aws.StringValue(option.DBInstanceClass), label) assert.Equal(t, storageTypeGP3, aws.StringValue(option.StorageType), label) assert.Equal(t, int64(minAllocatedStorageGiB), aws.Int64Value(option.MinStorageSize), label) @@ -116,7 +97,7 @@ func TestOrderableOptions_CoverEveryEngineAndClass(t *testing.T) { // catalog should not be able to hold quietly. func TestOrderableOptions_AgreeWithTheRejectedCreateParameters(t *testing.T) { t.Parallel() - option := OrderableOptions(OrderableFilter{}, runsEverything)[0] + option := OrderableOptions(rdsengine.OrderableFilter{}, InstanceSizing(), runsEverything)[0] cases := []struct { capability string @@ -149,163 +130,3 @@ func TestOrderableOptions_AgreeWithTheRejectedCreateParameters(t *testing.T) { assert.True(t, aws.BoolValue(option.SupportsStorageEncryption)) assert.Error(t, rejectUnimplemented(&rds.CreateDBInstanceInput{StorageEncrypted: aws.Bool(false)})) } - -// A class the cluster's nodes cannot run is never offered, and a cluster that -// runs none of them is an empty list rather than an error. -func TestOrderableOptions_FilterOnClusterCapability(t *testing.T) { - t.Parallel() - onlyMicro := OrderableOptions(OrderableFilter{}, func(instanceType string) bool { - return instanceType == "t3.micro" - }) - require.Len(t, onlyMicro, len(SupportedEngines())) - for _, option := range onlyMicro { - assert.Equal(t, "db.t3.micro", aws.StringValue(option.DBInstanceClass)) - } - - assert.Empty(t, OrderableOptions(OrderableFilter{}, runsNothing)) -} - -// db.X is the EC2 instance type X, which is what lets a client read vCPU and -// memory from ec2:DescribeInstanceTypes for the identically named type. Asserted -// by name rather than by footprint: t3.large and m5.large are both 2 vCPU and -// 8 GiB, so repointing db.t3.large at m5.large would break the documented -// identity and pass a footprint comparison unchanged. -func TestInstanceTypeForClass_IsTheClassNameWithoutItsPrefix(t *testing.T) { - t.Parallel() - for _, class := range SupportedInstanceClasses() { - t.Run(class, func(t *testing.T) { - t.Parallel() - instanceType, err := InstanceTypeForClass(class) - require.NoError(t, err) - assert.Equal(t, strings.TrimPrefix(class, "db."), instanceType) - - // Both lookups prove the name is known to instancetypes, which is what - // makes SmallestInstanceClass safe: it silently skips a class whose - // footprint will not resolve. - _, knownVCPUs := instancetypes.DefaultVCPUs(instanceType) - assert.True(t, knownVCPUs, "instancetypes should know the vCPU count of %s", instanceType) - _, knownMemory := instancetypes.DefaultMemoryMiB(instanceType) - assert.True(t, knownMemory, "instancetypes should know the memory of %s", instanceType) - }) - } -} - -func TestEngineVersionFilter_NarrowsByEveryRecognisedField(t *testing.T) { - t.Parallel() - postgres, err := LookupEngine("postgres") - require.NoError(t, err) - - cases := []struct { - name string - filter func() EngineVersionFilter - want []string - }{ - {"unfiltered", func() EngineVersionFilter { return EngineVersionFilter{} }, SupportedEngines()}, - {"engine", func() (f EngineVersionFilter) { - f.Engine.AddParam("postgres") - return f - }, []string{"postgres"}}, - {"engine is case insensitive", func() (f EngineVersionFilter) { - f.Engine.AddFilter([]string{"PostgreSQL", "POSTGRES"}) - return f - }, []string{"postgres"}}, - {"engine version", func() (f EngineVersionFilter) { - f.EngineVersion.AddParam(postgres.MajorVersion) - return f - }, []string{"postgres"}}, - {"parameter group family", func() (f EngineVersionFilter) { - f.ParameterGroupFamily.AddParam(postgres.ParameterGroupFamily()) - return f - }, []string{"postgres"}}, - {"status", func() (f EngineVersionFilter) { - f.Status.AddFilter([]string{engineVersionStatusAvailable}) - return f - }, SupportedEngines()}, - {"deprecated status matches nothing", func() (f EngineVersionFilter) { - f.Status.AddFilter([]string{"deprecated"}) - return f - }, nil}, - {"unknown engine matches nothing", func() (f EngineVersionFilter) { - f.Engine.AddParam("oracle-ee") - return f - }, nil}, - {"unpinned version matches nothing", func() (f EngineVersionFilter) { - f.EngineVersion.AddParam("17") - return f - }, nil}, - // Two sources naming different engines is a conjunction, not a union: a - // row has to satisfy both, and no row is two engines. - {"typed parameter and filter are both applied", func() (f EngineVersionFilter) { - f.Engine.AddParam("postgres") - f.Engine.AddFilter([]string{"mariadb"}) - return f - }, nil}, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - var got []string - for _, row := range EngineVersions(tc.filter()) { - got = append(got, aws.StringValue(row.Engine)) - } - assert.Equal(t, tc.want, got) - }) - } -} - -func TestOrderableFilter_NarrowsByEveryRecognisedField(t *testing.T) { - t.Parallel() - postgres, err := LookupEngine("postgres") - require.NoError(t, err) - classes := len(SupportedInstanceClasses()) - - cases := []struct { - name string - filter func() OrderableFilter - want int - }{ - {"unfiltered", func() OrderableFilter { return OrderableFilter{} }, len(SupportedEngines()) * classes}, - {"engine", func() (f OrderableFilter) { - f.Engine.AddParam("postgres") - return f - }, classes}, - {"engine version", func() (f OrderableFilter) { - f.EngineVersion.AddParam(postgres.MajorVersion) - return f - }, classes}, - {"instance class", func() (f OrderableFilter) { - f.DBInstanceClass.AddParam("db.t3.micro") - return f - }, len(SupportedEngines())}, - {"license model", func() (f OrderableFilter) { - f.LicenseModel.AddParam(postgres.LicenseModel()) - return f - }, classes}, - {"vpc true", func() (f OrderableFilter) { - f.Vpc.AddParam("true") - return f - }, len(SupportedEngines()) * classes}, - // Every endpoint is a private VPC address, so asking for a non-VPC option - // is an empty list rather than the whole catalog. - {"vpc false", func() (f OrderableFilter) { - f.Vpc.AddParam("false") - return f - }, 0}, - {"unknown class matches nothing", func() (f OrderableFilter) { - f.DBInstanceClass.AddParam("db.r5.24xlarge") - return f - }, 0}, - {"unpinned version matches nothing", func() (f OrderableFilter) { - f.EngineVersion.AddParam("17") - return f - }, 0}, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - assert.Len(t, OrderableOptions(tc.filter(), runsEverything), tc.want) - }) - } -} diff --git a/spinifex/handlers/rds/engine_catalogue_characterization_test.go b/spinifex/handlers/rds/engine_catalogue_characterization_test.go index 0721f9fbb..f44eb6ba6 100644 --- a/spinifex/handlers/rds/engine_catalogue_characterization_test.go +++ b/spinifex/handlers/rds/engine_catalogue_characterization_test.go @@ -1,8 +1,9 @@ package handlers_rds -//test:in-package — pins present behaviour of the engine catalogue (engine.go, -// engine_postgres.go, engine_mariadb.go, paramcatalog.go, sizing.go, catalog.go) -// ahead of a package split, including unexported seams no exported API reaches. +//test:in-package — pins present behaviour of the engine-catalogue consumers +// that stay in this package after the split: the recovery-message text built +// from the engine's crash/unclean-stop notes, and this package's own +// ValidateMasterUserPassword. import ( "testing" @@ -12,50 +13,6 @@ import ( "github.com/stretchr/testify/require" ) -// engineForFamily and normaliseFamily fold case and trim surrounding -// whitespace before the registry lookup, exactly as LookupEngine does for an -// engine name. -func TestEngineForFamily_NormalisesCaseAndWhitespace(t *testing.T) { - t.Parallel() - assert.Equal(t, "postgres18", normaliseFamily(" POSTGRES18 ")) - assert.Equal(t, "mariadb11.8", normaliseFamily("MariaDB11.8")) - - byCase, err := engineForFamily(" POSTGRES18 ") - require.NoError(t, err) - assert.Equal(t, enginePostgres.Name, byCase.Name) - - byMixedCase, err := engineForFamily("MariaDB11.8") - require.NoError(t, err) - assert.Equal(t, engineMariaDB.Name, byMixedCase.Name) -} - -func TestEngineForFamily_RejectsUnknownFamily(t *testing.T) { - t.Parallel() - _, err := engineForFamily("mysql8") - require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err), - "the code has to survive resolution or the client sees a 500") -} - -// The AWS engine identifier is "postgres", not the marketing name a customer -// might reasonably try first. -func TestLookupEngine_RejectsProductNameWithExactCode(t *testing.T) { - t.Parallel() - _, err := LookupEngine("PostgreSQL") - require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) -} - -func TestEngineValidateVersion_UnsupportedVersionCarriesExactCode(t *testing.T) { - t.Parallel() - engine, err := LookupEngine("postgres") - require.NoError(t, err) - - err = engine.ValidateVersion("19") - require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) -} - // ValidateMasterUserPassword's three rejections all go through awserrors.Errorf, // so each one resolves through ValidErrorCodeFromError to the same code. func TestValidateMasterUserPassword_ExactCodesAndMessages(t *testing.T) { @@ -86,198 +43,6 @@ func repeatByte(b byte, n int) string { return string(out) } -// catalogAgentContract is one row of the agent-contract metadata a catalog -// parameter carries: the name the option file is written under, its data type -// and its apply semantics. Hand-transcribed from engine_postgres.go and -// engine_mariadb.go, not derived from the catalog at test time. -type catalogAgentContract struct { - optionFileName string - dataType string - applyType string -} - -var postgresAgentContract = map[string]catalogAgentContract{ - "max_connections": {"max_connections", ParamTypeInteger, ApplyTypeStatic}, - "superuser_reserved_connections": {"superuser_reserved_connections", ParamTypeInteger, ApplyTypeStatic}, - "idle_in_transaction_session_timeout": {"idle_in_transaction_session_timeout", ParamTypeInteger, ApplyTypeDynamic}, - "statement_timeout": {"statement_timeout", ParamTypeInteger, ApplyTypeDynamic}, - "lock_timeout": {"lock_timeout", ParamTypeInteger, ApplyTypeDynamic}, - "tcp_keepalives_idle": {"tcp_keepalives_idle", ParamTypeInteger, ApplyTypeDynamic}, - "shared_buffers": {"shared_buffers", ParamTypeInteger, ApplyTypeStatic}, - "effective_cache_size": {"effective_cache_size", ParamTypeInteger, ApplyTypeDynamic}, - "work_mem": {"work_mem", ParamTypeInteger, ApplyTypeDynamic}, - "maintenance_work_mem": {"maintenance_work_mem", ParamTypeInteger, ApplyTypeDynamic}, - "temp_buffers": {"temp_buffers", ParamTypeInteger, ApplyTypeDynamic}, - "max_prepared_transactions": {"max_prepared_transactions", ParamTypeInteger, ApplyTypeStatic}, - "max_worker_processes": {"max_worker_processes", ParamTypeInteger, ApplyTypeStatic}, - "max_parallel_workers": {"max_parallel_workers", ParamTypeInteger, ApplyTypeDynamic}, - "max_parallel_workers_per_gather": {"max_parallel_workers_per_gather", ParamTypeInteger, ApplyTypeDynamic}, - "wal_level": {"wal_level", ParamTypeEnum, ApplyTypeStatic}, - "synchronous_commit": {"synchronous_commit", ParamTypeEnum, ApplyTypeDynamic}, - "wal_compression": {"wal_compression", ParamTypeEnum, ApplyTypeDynamic}, - "max_wal_size": {"max_wal_size", ParamTypeInteger, ApplyTypeDynamic}, - "min_wal_size": {"min_wal_size", ParamTypeInteger, ApplyTypeDynamic}, - "checkpoint_timeout": {"checkpoint_timeout", ParamTypeInteger, ApplyTypeDynamic}, - "checkpoint_completion_target": {"checkpoint_completion_target", ParamTypeReal, ApplyTypeDynamic}, - "wal_buffers": {"wal_buffers", ParamTypeInteger, ApplyTypeStatic}, - "max_wal_senders": {"max_wal_senders", ParamTypeInteger, ApplyTypeStatic}, - "max_replication_slots": {"max_replication_slots", ParamTypeInteger, ApplyTypeStatic}, - "autovacuum": {"autovacuum", ParamTypeBoolean, ApplyTypeDynamic}, - "autovacuum_max_workers": {"autovacuum_max_workers", ParamTypeInteger, ApplyTypeDynamic}, - "autovacuum_naptime": {"autovacuum_naptime", ParamTypeInteger, ApplyTypeDynamic}, - "autovacuum_vacuum_threshold": {"autovacuum_vacuum_threshold", ParamTypeInteger, ApplyTypeDynamic}, - "autovacuum_vacuum_scale_factor": {"autovacuum_vacuum_scale_factor", ParamTypeReal, ApplyTypeDynamic}, - "autovacuum_analyze_threshold": {"autovacuum_analyze_threshold", ParamTypeInteger, ApplyTypeDynamic}, - "autovacuum_analyze_scale_factor": {"autovacuum_analyze_scale_factor", ParamTypeReal, ApplyTypeDynamic}, - "autovacuum_vacuum_cost_limit": {"autovacuum_vacuum_cost_limit", ParamTypeInteger, ApplyTypeDynamic}, - "random_page_cost": {"random_page_cost", ParamTypeReal, ApplyTypeDynamic}, - "seq_page_cost": {"seq_page_cost", ParamTypeReal, ApplyTypeDynamic}, - "effective_io_concurrency": {"effective_io_concurrency", ParamTypeInteger, ApplyTypeDynamic}, - "default_statistics_target": {"default_statistics_target", ParamTypeInteger, ApplyTypeDynamic}, - "jit": {"jit", ParamTypeBoolean, ApplyTypeDynamic}, - "log_min_duration_statement": {"log_min_duration_statement", ParamTypeInteger, ApplyTypeDynamic}, - "log_statement": {"log_statement", ParamTypeEnum, ApplyTypeDynamic}, - "log_min_messages": {"log_min_messages", ParamTypeEnum, ApplyTypeDynamic}, - "log_connections": {"log_connections", ParamTypeBoolean, ApplyTypeDynamic}, - "log_disconnections": {"log_disconnections", ParamTypeBoolean, ApplyTypeDynamic}, - "log_lock_waits": {"log_lock_waits", ParamTypeBoolean, ApplyTypeDynamic}, - "log_temp_files": {"log_temp_files", ParamTypeInteger, ApplyTypeDynamic}, - "log_autovacuum_min_duration": {"log_autovacuum_min_duration", ParamTypeInteger, ApplyTypeDynamic}, - "deadlock_timeout": {"deadlock_timeout", ParamTypeInteger, ApplyTypeDynamic}, - "timezone": {"timezone", ParamTypeString, ApplyTypeDynamic}, - "datestyle": {"datestyle", ParamTypeString, ApplyTypeDynamic}, - "track_activity_query_size": {"track_activity_query_size", ParamTypeInteger, ApplyTypeStatic}, - "track_io_timing": {"track_io_timing", ParamTypeBoolean, ApplyTypeDynamic}, - "ssl_min_protocol_version": {"ssl_min_protocol_version", ParamTypeString, ApplyTypeDynamic}, - "rds.force_ssl": {"rds.force_ssl", ParamTypeBoolean, ApplyTypeDynamic}, -} - -var mariadbAgentContract = map[string]catalogAgentContract{ - "max_connections": {"max_connections", ParamTypeInteger, ApplyTypeDynamic}, - "max_connect_errors": {"max_connect_errors", ParamTypeInteger, ApplyTypeDynamic}, - "max_allowed_packet": {"max_allowed_packet", ParamTypeInteger, ApplyTypeDynamic}, - "thread_cache_size": {"thread_cache_size", ParamTypeInteger, ApplyTypeDynamic}, - "wait_timeout": {"wait_timeout", ParamTypeInteger, ApplyTypeDynamic}, - "interactive_timeout": {"interactive_timeout", ParamTypeInteger, ApplyTypeDynamic}, - "connect_timeout": {"connect_timeout", ParamTypeInteger, ApplyTypeDynamic}, - "net_read_timeout": {"net_read_timeout", ParamTypeInteger, ApplyTypeDynamic}, - "net_write_timeout": {"net_write_timeout", ParamTypeInteger, ApplyTypeDynamic}, - "lock_wait_timeout": {"lock_wait_timeout", ParamTypeInteger, ApplyTypeDynamic}, - "innodb_lock_wait_timeout": {"innodb_lock_wait_timeout", ParamTypeInteger, ApplyTypeDynamic}, - "max_statement_time": {"max_statement_time", ParamTypeReal, ApplyTypeDynamic}, - "idle_transaction_timeout": {"idle_transaction_timeout", ParamTypeInteger, ApplyTypeDynamic}, - "innodb_buffer_pool_size": {"innodb_buffer_pool_size", ParamTypeInteger, ApplyTypeStatic}, - "innodb_log_file_size": {"innodb_log_file_size", ParamTypeInteger, ApplyTypeStatic}, - "innodb_flush_method": {"innodb_flush_method", ParamTypeEnum, ApplyTypeStatic}, - "innodb_flush_log_at_trx_commit": {"innodb_flush_log_at_trx_commit", ParamTypeInteger, ApplyTypeDynamic}, - "innodb_read_io_threads": {"innodb_read_io_threads", ParamTypeInteger, ApplyTypeStatic}, - "innodb_write_io_threads": {"innodb_write_io_threads", ParamTypeInteger, ApplyTypeStatic}, - "innodb_purge_threads": {"innodb_purge_threads", ParamTypeInteger, ApplyTypeStatic}, - "innodb_autoinc_lock_mode": {"innodb_autoinc_lock_mode", ParamTypeInteger, ApplyTypeStatic}, - "innodb_io_capacity": {"innodb_io_capacity", ParamTypeInteger, ApplyTypeDynamic}, - "innodb_io_capacity_max": {"innodb_io_capacity_max", ParamTypeInteger, ApplyTypeDynamic}, - "innodb_max_dirty_pages_pct": {"innodb_max_dirty_pages_pct", ParamTypeReal, ApplyTypeDynamic}, - "innodb_flush_neighbors": {"innodb_flush_neighbors", ParamTypeInteger, ApplyTypeDynamic}, - "innodb_file_per_table": {"innodb_file_per_table", ParamTypeBoolean, ApplyTypeDynamic}, - "innodb_stats_persistent": {"innodb_stats_persistent", ParamTypeBoolean, ApplyTypeDynamic}, - "innodb_strict_mode": {"innodb_strict_mode", ParamTypeBoolean, ApplyTypeDynamic}, - "innodb_adaptive_hash_index": {"innodb_adaptive_hash_index", ParamTypeBoolean, ApplyTypeDynamic}, - "sort_buffer_size": {"sort_buffer_size", ParamTypeInteger, ApplyTypeDynamic}, - "join_buffer_size": {"join_buffer_size", ParamTypeInteger, ApplyTypeDynamic}, - "read_buffer_size": {"read_buffer_size", ParamTypeInteger, ApplyTypeDynamic}, - "read_rnd_buffer_size": {"read_rnd_buffer_size", ParamTypeInteger, ApplyTypeDynamic}, - "tmp_table_size": {"tmp_table_size", ParamTypeInteger, ApplyTypeDynamic}, - "max_heap_table_size": {"max_heap_table_size", ParamTypeInteger, ApplyTypeDynamic}, - "key_buffer_size": {"key_buffer_size", ParamTypeInteger, ApplyTypeDynamic}, - "table_open_cache": {"table_open_cache", ParamTypeInteger, ApplyTypeDynamic}, - "table_definition_cache": {"table_definition_cache", ParamTypeInteger, ApplyTypeDynamic}, - "max_prepared_stmt_count": {"max_prepared_stmt_count", ParamTypeInteger, ApplyTypeDynamic}, - "group_concat_max_len": {"group_concat_max_len", ParamTypeInteger, ApplyTypeDynamic}, - "performance_schema": {"performance_schema", ParamTypeBoolean, ApplyTypeStatic}, - "slow_query_log": {"slow_query_log", ParamTypeBoolean, ApplyTypeDynamic}, - "long_query_time": {"long_query_time", ParamTypeReal, ApplyTypeDynamic}, - "log_queries_not_using_indexes": {"log_queries_not_using_indexes", ParamTypeBoolean, ApplyTypeDynamic}, - "general_log": {"general_log", ParamTypeBoolean, ApplyTypeDynamic}, - "log_output": {"log_output", ParamTypeEnum, ApplyTypeDynamic}, - "log_warnings": {"log_warnings", ParamTypeInteger, ApplyTypeDynamic}, - "sql_mode": {"sql_mode", ParamTypeString, ApplyTypeDynamic}, - "character_set_server": {"character_set_server", ParamTypeEnum, ApplyTypeDynamic}, - "collation_server": {"collation_server", ParamTypeEnum, ApplyTypeDynamic}, - // The one remapped entry: mariadbd has no time_zone startup option, so the - // option file carries it under default_time_zone instead. - "time_zone": {"default_time_zone", ParamTypeString, ApplyTypeDynamic}, - "transaction_isolation": {"transaction_isolation", ParamTypeEnum, ApplyTypeDynamic}, - "autocommit": {"autocommit", ParamTypeBoolean, ApplyTypeDynamic}, - "event_scheduler": {"event_scheduler", ParamTypeEnum, ApplyTypeDynamic}, - "tls_version": {"tls_version", ParamTypeString, ApplyTypeStatic}, - "require_secure_transport": {"require_secure_transport", ParamTypeBoolean, ApplyTypeDynamic}, -} - -// Every catalog parameter of every engine, checked against a golden table -// transcribed from the catalogue source: its option-file spelling, data type -// and apply semantics. A package split that drops or renames one of these -// has to fail here rather than surface as a guest that refuses to start. -func TestCatalogueAgentContractMetadata_MatchesGoldenTable(t *testing.T) { - tests := []struct { - engine Engine - golden map[string]catalogAgentContract - }{ - {enginePostgres, postgresAgentContract}, - {engineMariaDB, mariadbAgentContract}, - } - for _, tc := range tests { - t.Run(tc.engine.Name, func(t *testing.T) { - names := tc.engine.CatalogParameterNames() - require.Len(t, names, len(tc.golden), "the catalogue gained or lost a parameter since this table was written") - for _, name := range names { - want, ok := tc.golden[name] - require.True(t, ok, "%s is not in the golden table", name) - - spec, ok := tc.engine.LookupParameter(name) - require.True(t, ok) - assert.Equal(t, want.dataType, spec.DataType, "%s DataType", name) - assert.Equal(t, want.applyType, spec.ApplyType, "%s ApplyType", name) - assert.Equal(t, want.optionFileName, tc.engine.OptionFileName(name), "%s OptionFileName", name) - } - }) - } -} - -// ResolveEffectiveParameters checks validateCombinations before anything else, -// so an engine that registers none fails with ServerInternal rather than -// resolving a parameter set it cannot then validate. -func TestResolveEffectiveParameters_NoCombinationCheckIsServerInternal(t *testing.T) { - t.Parallel() - bare := Engine{Name: "bare-engine"} - _, err := bare.ResolveEffectiveParameters(SmallestInstanceClass(), nil) - require.Error(t, err) - assert.Equal(t, awserrors.ErrorServerInternal, awserrors.ValidErrorCodeFromError(err)) - assert.Contains(t, err.Error(), "registers no parameter combination checks") -} - -// A combination check that reads a name absent from its own catalog is a -// catalog bug, not anything a customer did, so the resolver's internal reader -// reports ServerInternal naming the missing key. -func TestResolveEffectiveParameters_CombinationCheckMissingKeyIsServerInternal(t *testing.T) { - t.Parallel() - broken := Engine{ - Name: "broken-combination-checks", - catalog: map[string]ParameterSpec{ - "foo": {Name: "foo", DataType: ParamTypeInteger, ApplyType: ApplyTypeStatic, - IsModifiable: true, Min: 0, Max: 10, Default: "1"}, - }, - validateCombinations: func(params []Parameter) error { - values := resolvedValues(params) - _, err := resolvedInteger(values, "bar") - return err - }, - } - _, err := broken.ResolveEffectiveParameters(SmallestInstanceClass(), nil) - require.Error(t, err) - assert.Equal(t, awserrors.ErrorServerInternal, awserrors.ValidErrorCodeFromError(err)) - assert.Contains(t, err.Error(), "resolved parameter set is missing bar") -} - // The half of each recovery warning that depends on the engine, pinned // verbatim rather than by substring: a wording change to either note should // fail here even if it keeps every substring the existing Contains assertions @@ -313,13 +78,3 @@ func TestCrashConsistentSnapshotMessage_PinsExactTextPerEngine(t *testing.T) { "non-transactional tables such as Aria and MyISAM may be left inconsistent.", mariadb) } - -// The literal DescribeDBParameters' size-derived defaults resolve against for -// the smallest class: db.t3.micro's 1 GiB, stated here as the MiB classMemoryMiB -// actually returns rather than only the ordering sizing_test.go already checks. -func TestSmallestInstanceClass_MemoryIsExactlyOneGiB(t *testing.T) { - t.Parallel() - memoryMiB, err := classMemoryMiB(SmallestInstanceClass()) - require.NoError(t, err) - assert.Equal(t, int64(1024), memoryMiB) -} diff --git a/spinifex/handlers/rds/engine_mariadb_test.go b/spinifex/handlers/rds/engine_mariadb_test.go index 40661f8a6..aa82eeb90 100644 --- a/spinifex/handlers/rds/engine_mariadb_test.go +++ b/spinifex/handlers/rds/engine_mariadb_test.go @@ -1,42 +1,14 @@ package handlers_rds -//test:in-package — reaches the unexported engineForFamily and the engineMariaDB -// registry entry, neither of which the package exports. - import ( - "strconv" - "strings" "testing" "github.com/aws/aws-sdk-go/aws" - "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// Offered only now that the image and the guest implementation exist: until -// they did, a create would have resolved an AMI nothing builds and left a volume -// and an ENI behind for an instance that could never become available. -func TestMariaDB_IsOffered(t *testing.T) { - t.Parallel() - engine, err := LookupEngine(" MariaDB ") - require.NoError(t, err, "the engine name is matched case-insensitively and trimmed") - assert.Equal(t, engineMariaDB.Name, engine.Name) - assert.Contains(t, SupportedEngines(), "mariadb") - - byFamily, err := engineForFamily(engineMariaDB.ParameterGroupFamily()) - require.NoError(t, err) - assert.Equal(t, engineMariaDB.Name, byFamily.Name) - assert.Contains(t, SupportedParameterGroupFamilies(), "mariadb11.8") - - // mysql is a distinct AWS engine this platform does not offer, and MariaDB is - // deliberately not aliased onto it: the alias would report an engine and a - // version the instance is not running. - _, err = LookupEngine("mysql") - require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) -} - // The whole point of registering the engine: a request naming it has to reach // the MariaDB AMI carrying MariaDB's own port, family and guest configuration, // not PostgreSQL's defaults with a different name on them. @@ -58,7 +30,9 @@ func TestCreateDBInstance_LaunchesMariaDB(t *testing.T) { rec := h.record(t, testDBInstanceID) assert.Equal(t, "mariadb", rec.Engine) assert.Equal(t, int64(3306), rec.Port) - assert.Equal(t, engineMariaDB.DefaultParameterGroupName(), rec.DBParameterGroupName) + mariadb, err := rdsengine.LookupEngine("mariadb") + require.NoError(t, err) + assert.Equal(t, mariadb.DefaultParameterGroupName(), rec.DBParameterGroupName) // The AMI is selected by the engine's own tags, so a MariaDB create can never // land on the PostgreSQL image however the two are published. @@ -75,403 +49,3 @@ func TestCreateDBInstance_LaunchesMariaDB(t *testing.T) { assert.Contains(t, h.launch.launcher.input.UserData, "RDS_ENGINE=mariadb") assert.Contains(t, h.launch.launcher.input.UserData, "RDS_ENGINE_PORT=3306") } - -func TestMariaDB_Identity(t *testing.T) { - t.Parallel() - assert.Equal(t, "mariadb", engineMariaDB.Name) - assert.Equal(t, int64(3306), engineMariaDB.DefaultPort) - // The version series the API pins to, not an integer major: PostgreSQL's - // version axis is one integer and MariaDB's is two. - assert.Equal(t, "11.8", engineMariaDB.EngineVersion()) - assert.Equal(t, "mariadb11.8", engineMariaDB.ParameterGroupFamily()) - assert.Equal(t, "default.mariadb11.8", engineMariaDB.DefaultParameterGroupName()) - - for _, version := range []string{"", "11.8"} { - assert.NoError(t, engineMariaDB.ValidateVersion(version), "version %q", version) - } - for _, version := range []string{"11", "11.4", "11.8.8", "10.11", "12", "latest"} { - assert.Error(t, engineMariaDB.ValidateVersion(version), "version %q", version) - } -} - -// The engine's own limit rather than AWS's documented 16, so a client written -// against real AWS is a strict subset and nothing that works there breaks here. -func TestMariaDB_ValidateMasterUsername(t *testing.T) { - for _, username := range []string{"appuser", "app_user1", "Orders", strings.Repeat("a", 80)} { - assert.NoError(t, engineMariaDB.ValidateMasterUsername(username), "username %q", username) - } - - cases := map[string]string{ - "empty": "", - "leading digit": "1user", - "hyphen": "app-user", - "dot": "app.user", - "too long": strings.Repeat("a", 81), - "install-db root": "root", - "cased root": "ROOT", - "sys schema owner": "mariadb.sys", - "socket account": "mysql", - "aws management": "rdsadmin", - // The server refuses CREATE USER PUBLIC, which would spend the one-shot - // bootstrap password on a guaranteed failure. - "role name": "PUBLIC", - } - for name, username := range cases { - t.Run(name, func(t *testing.T) { - err := engineMariaDB.ValidateMasterUsername(username) - require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) - }) - } -} - -// The guest re-checks the name it is handed rather than trusting the control -// plane, and it reaches the reserved half on its own. -func TestMariaDB_ValidateUsernameNotReserved(t *testing.T) { - t.Parallel() - for _, username := range []string{"mysql.session", "MySQL.infoschema", "mariadb.sys", "mariadb.anything"} { - err := engineMariaDB.ValidateUsernameNotReserved(username) - require.Error(t, err, "username %q", username) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) - } - assert.NoError(t, engineMariaDB.ValidateUsernameNotReserved("mysqladmin"), - "the prefix is mysql. with the dot, not the bare word") -} - -// MariaDB maps a database onto a directory name and rds-init interpolates the -// name into CREATE DATABASE from the shell, so this rule is the actual barrier. -func TestMariaDB_ValidateDBName(t *testing.T) { - t.Parallel() - for _, name := range []string{"", "orders", "Orders_2", strings.Repeat("d", 64)} { - assert.NoError(t, engineMariaDB.ValidateDBName(name), "name %q", name) - } - for _, name := range []string{"my-db", "1db", "my db", "my/db", `my\db`, "my.db", "db ", strings.Repeat("d", 65)} { - err := engineMariaDB.ValidateDBName(name) - require.Error(t, err, "name %q", name) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) - } - - // Each engine takes its own identifier limit rather than a shared one. - assert.Error(t, enginePostgres.ValidateDBName(strings.Repeat("d", 64))) - assert.NoError(t, engineMariaDB.ValidateDBName(strings.Repeat("d", 64))) -} - -// The two catalogs are separate tables, not one table with an engine column, so -// neither engine can be handed a setting the other's server would not parse. -func TestMariaDBCatalog_IsSeparateFromPostgres(t *testing.T) { - t.Parallel() - for _, name := range []string{"innodb_buffer_pool_size", "sql_mode", "key_buffer_size"} { - _, ok := enginePostgres.LookupParameter(name) - assert.False(t, ok, "PostgreSQL must not expose %s", name) - } - for _, name := range []string{"work_mem", "shared_buffers", "autovacuum", "wal_level", "datestyle"} { - _, ok := engineMariaDB.LookupParameter(name) - assert.False(t, ok, "MariaDB must not expose %s", name) - } - // max_connections exists in both, and each takes its own engine's bounds. - _, err := engineMariaDB.validateParameterValue("max_connections", "20000") - assert.NoError(t, err) - _, err = enginePostgres.validateParameterValue("max_connections", "20000") - assert.Error(t, err, "PostgreSQL's own ceiling is 5000") -} - -// Absent rather than present-and-unmodifiable, because they are not the -// customer's to set: the endpoint, the agent's socket access, the serving -// certificate and the snapshot recovery guarantee all depend on them. -func TestMariaDBCatalog_OmitsPlatformOwnedSettings(t *testing.T) { - t.Parallel() - owned := []string{ - "port", "datadir", "socket", "bind_address", - "ssl_ca", "ssl_cert", "ssl_key", - "secure_file_priv", "skip_symbolic_links", "log_bin", - "default_storage_engine", - "innodb_buffer_pool_chunk_size", "innodb_buffer_pool_instances", - } - for _, name := range owned { - _, ok := engineMariaDB.LookupParameter(name) - assert.False(t, ok, "%s is platform-owned and must not be in the catalog", name) - } -} - -// The size-derived defaults have to move with the class, or the formulas are -// decoration and one end of the supported range is mis-tuned. -func TestMariaDBCatalog_SizeDerivedDefaultsScaleWithClassMemory(t *testing.T) { - t.Parallel() - smallest, err := classMemoryMiB("db.t3.micro") - require.NoError(t, err) - largest, err := classMemoryMiB("db.m5.xlarge") - require.NoError(t, err) - - for _, name := range []string{"innodb_buffer_pool_size", "max_connections"} { - spec, ok := engineMariaDB.LookupParameter(name) - require.True(t, ok, "%s is expected to carry a computed default", name) - require.NotNil(t, spec.DefaultFor, "%s should be size-derived", name) - require.NotNil(t, spec.MaxFor, "%s should carry a class ceiling", name) - - small, err := strconv.ParseInt(spec.DefaultAt(smallest), 10, 64) - require.NoError(t, err) - large, err := strconv.ParseInt(spec.DefaultAt(largest), 10, 64) - require.NoError(t, err) - assert.Less(t, small, large, "%s does not grow with the class", name) - assert.LessOrEqual(t, small, spec.MaxFor(smallest), "%s defaults above its own class ceiling", name) - } -} - -// Three quarters of class memory is RDS's own default for this engine family, -// and it is the setting whose being wrong stops the server from starting. -func TestMariaDBCatalog_BufferPoolIsThreeQuartersOfClassMemory(t *testing.T) { - t.Parallel() - memoryMiB, err := classMemoryMiB("db.m5.large") - require.NoError(t, err) - - bytes, err := strconv.ParseInt(innodbBufferPoolSizeFor(memoryMiB), 10, 64) - require.NoError(t, err) - assert.Equal(t, memoryMiB*mibToBytes*3/4, bytes) - assert.Less(t, bytes, innodbBufferPoolSizeCeilingFor(memoryMiB), - "the ceiling has to leave headroom above the default") - assert.Less(t, innodbBufferPoolSizeCeilingFor(memoryMiB), memoryMiB*mibToBytes, - "a pool at class memory is a server that never starts") -} - -// max_connections = {DBInstanceClassMemory/12582880}, which is RDS's own formula. -func TestMariaDBCatalog_MaxConnectionsFollowsTheRDSFormula(t *testing.T) { - t.Parallel() - memoryMiB, err := classMemoryMiB("db.m5.xlarge") - require.NoError(t, err) - assert.Equal(t, strconv.FormatInt(memoryMiB*mibToBytes/12582880, 10), mariadbMaxConnectionsFor(memoryMiB)) - - // The floor keeps the smallest class above what the platform's own probe, - // parameter apply and password rotation sessions need. - smallest, err := classMemoryMiB(SmallestInstanceClass()) - require.NoError(t, err) - connections, err := strconv.ParseInt(mariadbMaxConnectionsFor(smallest), 10, 64) - require.NoError(t, err) - assert.GreaterOrEqual(t, connections, int64(20)) -} - -func TestMariaDBCatalog_RejectsBadValues(t *testing.T) { - cases := []struct { - name, param, value, want string - }{ - {"UnknownName", "not_a_setting", "1", "is not a parameter this engine exposes"}, - {"PostgresSetting", "work_mem", "16384", "is not a parameter this engine exposes"}, - {"NotAnInteger", "max_connections", "many", "takes an integer"}, - {"BelowRange", "max_connections", "1", "outside its allowed range"}, - {"BufferPoolBelowMinimum", "innodb_buffer_pool_size", "1048576", "outside its allowed range"}, - {"RedoLogBelowMinimum", "innodb_log_file_size", "1048576", "outside its allowed range"}, - {"NotAReal", "long_query_time", "slow", "takes a number"}, - {"RealOutOfRange", "innodb_max_dirty_pages_pct", "100", "outside its allowed range"}, - {"NotInEnum", "log_output", "syslog", "does not accept"}, - {"NotAnIsolationLevel", "transaction_isolation", "snapshot", "does not accept"}, - {"UnknownSQLMode", "sql_mode", "STRICT_TRANS_TABLES,NOT_A_MODE", "does not accept"}, - {"EmptySQLModeToken", "sql_mode", "STRICT_TRANS_TABLES,,ANSI_QUOTES", "does not accept"}, - {"NamedTimeZone", "time_zone", "Australia/Sydney", "does not accept"}, - {"OffsetOutOfRange", "time_zone", "+15:00", "does not accept"}, - {"UnsignedOffset", "time_zone", "10:00", "does not accept"}, - {"OffsetWithoutMinutes", "time_zone", "+10", "does not accept"}, - {"OffsetWithShortHour", "time_zone", "+1:00", "does not accept"}, - {"OffsetWithLongHour", "time_zone", "+0001:00", "does not accept"}, - {"Empty", "sort_buffer_size", "", "empty value"}, - {"Formula", "innodb_buffer_pool_size", "{DBInstanceClassMemory*3/4}", "is a formula"}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - _, err := engineMariaDB.validateParameterValue(tc.param, tc.value) - require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err), - "the code has to survive resolution or the client sees a 500") - assert.Contains(t, err.Error(), tc.want) - }) - } -} - -func TestMariaDBCatalog_AcceptsEngineSpelledValues(t *testing.T) { - cases := []struct{ param, value string }{ - {"sql_mode", "STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION"}, - {"sql_mode", "ansi_quotes"}, - {"sql_mode", "TRADITIONAL, ONLY_FULL_GROUP_BY"}, - {"time_zone", "SYSTEM"}, - {"time_zone", "system"}, - {"time_zone", "+10:00"}, - {"time_zone", "-05:30"}, - {"time_zone", "+00:00"}, - {"innodb_flush_method", "O_DIRECT"}, - {"transaction_isolation", "READ-COMMITTED"}, - {"log_output", "TABLE"}, - } - for _, tc := range cases { - t.Run(tc.param+"/"+tc.value, func(t *testing.T) { - _, err := engineMariaDB.validateParameterValue(tc.param, tc.value) - assert.NoError(t, err) - }) - } -} - -// MariaDB refuses yes and no for a boolean system variable where PostgreSQL -// accepts them, so a value the API takes must be one mysqld will parse. -// mariadbd's own parser refuses yes and no, but no value reaches it unresolved: -// the resolver canonicalises every boolean to 1 or 0 first. So the API takes all -// eight spellings here as it does on PostgreSQL, and the engine still only ever -// sees the two it parses. -func TestMariaDBCatalog_TakesEveryBooleanSpelling(t *testing.T) { - for _, param := range []string{"slow_query_log", "require_secure_transport"} { - t.Run(param, func(t *testing.T) { - for _, value := range []string{"on", "OFF", "true", "False", "yes", "No", "1", "0"} { - _, err := engineMariaDB.validateParameterValue(param, value) - assert.NoError(t, err, "%s rejected %q, which the API accepts", param, value) - } - - spec, ok := engineMariaDB.LookupParameter(param) - require.True(t, ok) - assert.Equal(t, "on,off,true,false,yes,no,1,0", spec.AllowedValues(), - "a customer has to be able to read which spellings are accepted") - }) - } -} - -func TestMariaDBCatalog_RejectsFatalCombinations(t *testing.T) { - tests := []struct { - name string - overrides map[string]string - want string - }{ - { - name: "collation outside the server character set", - overrides: map[string]string{"character_set_server": "latin1"}, - want: "does not belong to character_set_server", - }, - { - name: "io capacity maximum below the steady rate", - overrides: map[string]string{"innodb_io_capacity": "4000"}, - want: "innodb_io_capacity_max must be at least innodb_io_capacity", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - _, err := engineMariaDB.ResolveEffectiveParameters("db.m5.xlarge", tt.overrides) - require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) - assert.Contains(t, err.Error(), tt.want) - }) - } - - _, err := engineMariaDB.ResolveEffectiveParameters("db.m5.xlarge", map[string]string{ - "character_set_server": "latin1", "collation_server": "latin1_bin", - }) - assert.NoError(t, err, "a consistent pair is the customer's to choose") -} - -// The buffer pool is the one setting a large-class literal makes a smaller guest -// unbootable with, so its class ceiling has to bind. -func TestMariaDBCatalog_BoundsSizeDerivedOverridesToTheClass(t *testing.T) { - tests := []struct{ name, value string }{ - {name: "innodb_buffer_pool_size", value: "12884901888"}, - {name: "max_connections", value: "2000"}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - _, err := engineMariaDB.ResolveEffectiveParameters("db.t3.micro", map[string]string{tt.name: tt.value}) - require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) - assert.Contains(t, err.Error(), "db.t3.micro") - assert.Contains(t, err.Error(), "ceiling") - - _, err = engineMariaDB.ResolveEffectiveParameters("db.m5.xlarge", map[string]string{tt.name: tt.value}) - assert.NoError(t, err, "the same literal should fit the larger class") - }) - } -} - -// The apply type is what decides whether a change is a SET GLOBAL or a restart, -// and the guest's pending-restart comparison reads the static keys out of the -// installed file. Getting one wrong is a permanent pending-reboot or a change -// the customer is told took effect and did not. -func TestMariaDBCatalog_ApplyTypesMatchTheEngine(t *testing.T) { - t.Parallel() - static := []string{ - "innodb_buffer_pool_size", "innodb_log_file_size", "innodb_flush_method", - "innodb_read_io_threads", "innodb_write_io_threads", "innodb_purge_threads", - "innodb_autoinc_lock_mode", "performance_schema", - } - for _, name := range static { - spec, ok := engineMariaDB.LookupParameter(name) - require.True(t, ok, "%s", name) - assert.Equal(t, ApplyTypeStatic, spec.ApplyType, "%s", name) - } - - dynamic := []string{ - "max_connections", "max_allowed_packet", "innodb_flush_log_at_trx_commit", - "innodb_io_capacity", "key_buffer_size", "table_open_cache", "sql_mode", - "slow_query_log", "long_query_time", "time_zone", "transaction_isolation", - } - for _, name := range dynamic { - spec, ok := engineMariaDB.LookupParameter(name) - require.True(t, ok, "%s", name) - assert.Equal(t, ApplyTypeDynamic, spec.ApplyType, "%s", name) - } -} - -func TestMariaDBCatalog_ResolvesEveryParameterAsALiteral(t *testing.T) { - t.Parallel() - resolved, err := engineMariaDB.ResolveEffectiveParameters("db.m5.large", map[string]string{"max_connections": "300"}) - require.NoError(t, err) - require.Len(t, resolved, len(engineMariaDB.CatalogParameterNames())) - - values := map[string]string{} - for i, param := range resolved { - if i > 0 { - assert.Less(t, resolved[i-1].Name, param.Name, "the resolved set is not sorted by name") - } - assert.NotContains(t, param.Value, "{", "%s reached the agent as a formula", param.Name) - assert.NotContains(t, param.Value, "DBInstanceClass", "%s reached the agent as a reference", param.Name) - values[param.Name] = param.Value - } - assert.Equal(t, "300", values["max_connections"], "the override did not win") - - memoryMiB, err := classMemoryMiB("db.m5.large") - require.NoError(t, err) - assert.Equal(t, innodbBufferPoolSizeFor(memoryMiB), values["innodb_buffer_pool_size"], - "a parameter the group does not set should carry its computed default") -} - -// The bug that kept every MariaDB instance from booting: mariadbd has no -// time_zone startup option, so an option file naming it aborts the server before -// it opens the datadir. -func TestMariaDBCatalog_TimeZoneUsesItsStartupSpellingInTheOptionFile(t *testing.T) { - t.Parallel() - assert.Equal(t, "default_time_zone", engineMariaDB.OptionFileName("time_zone")) - - // The customer-facing name is what the API reports and what a live SET GLOBAL - // names, so the catalog must still carry it. - spec, ok := engineMariaDB.LookupParameter("time_zone") - require.True(t, ok) - assert.Equal(t, "time_zone", spec.Name) - assert.True(t, spec.IsModifiable) -} - -func TestMariaDBCatalog_EveryOtherParameterKeepsItsName(t *testing.T) { - t.Parallel() - for _, name := range engineMariaDB.CatalogParameterNames() { - if name == "time_zone" { - continue - } - assert.Equal(t, name, engineMariaDB.OptionFileName(name), - "%s was given an option-file spelling but the fixture asserts none", name) - } -} - -// PostgreSQL sets every one of its settings under the name the customer knows, -// so a mapping there would be a bug rather than a fix. -func TestPostgresCatalog_NoParameterIsRenamedForTheOptionFile(t *testing.T) { - t.Parallel() - for _, name := range enginePostgres.CatalogParameterNames() { - assert.Equal(t, name, enginePostgres.OptionFileName(name)) - } -} - -// A name the catalog does not carry is the customer's own, and the option file -// takes it unchanged rather than silently dropping it. -func TestMariaDBCatalog_OptionFileNamePassesAnUnknownNameThrough(t *testing.T) { - t.Parallel() - assert.Equal(t, "not_a_parameter", engineMariaDB.OptionFileName("not_a_parameter")) -} diff --git a/spinifex/handlers/rds/engine_test.go b/spinifex/handlers/rds/engine_test.go deleted file mode 100644 index a1562fca3..000000000 --- a/spinifex/handlers/rds/engine_test.go +++ /dev/null @@ -1,135 +0,0 @@ -package handlers_rds - -import ( - "strings" - "testing" - - "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestLookupEngine(t *testing.T) { - t.Parallel() - _, err := LookupEngine("PostgreSQL") - require.Error(t, err, "the AWS engine identifier is 'postgres', not the product name") - - engine, err := LookupEngine(" POSTGRES ") - require.NoError(t, err, "the engine name is matched case-insensitively and trimmed") - assert.Equal(t, "postgres", engine.Name) - assert.Equal(t, int64(5432), engine.DefaultPort) - assert.Equal(t, "default.postgres18", engine.DefaultParameterGroupName()) - - _, err = LookupEngine("mysql") - require.Error(t, err) - assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) - assert.Equal(t, []string{"mariadb", "postgres"}, SupportedEngines()) -} - -// A version other than the pinned one would be served by an image that is -// not the one asked for, so it is rejected rather than quietly substituted. -func TestEngineValidateVersion(t *testing.T) { - t.Parallel() - engine, err := LookupEngine("postgres") - require.NoError(t, err) - - for _, version := range []string{"", "18"} { - assert.NoError(t, engine.ValidateVersion(version), "version %q", version) - } - for _, version := range []string{"17", "16.2", "18.1", "18.4", " 18 ", "19", "latest"} { - assert.Error(t, engine.ValidateVersion(version), "version %q", version) - } - - // The pin, not the request, is what the record and the AMI lookup carry. - assert.Equal(t, "18", engine.EngineVersion()) -} - -func TestEngineValidateMasterUsername(t *testing.T) { - engine, err := LookupEngine("postgres") - require.NoError(t, err) - - for _, username := range []string{"appuser", "app_user1", "Orders"} { - assert.NoError(t, engine.ValidateMasterUsername(username), "username %q", username) - } - - cases := map[string]string{ - "empty": "", - "leading digit": "1user", - "hyphen": "app-user", - "too long": strings.Repeat("a", enginePostgres.maxUsernameLen+1), - "reserved": "rdsadmin", - "reserved cased": "RDSAdmin", - // The cluster superuser and the group role the master's administrative - // privileges come through. Both exist by the time the master is created, - // so a collision would surface as a failed bootstrap inside the guest. - "cluster superuser": "postgres", - "group role": "rds_superuser", - // postgres reserves the pg_ prefix for its own internal roles, so a master - // role taking one would collide inside the engine, not at the API. - "reserved prefix": "pg_backup", - } - for name, username := range cases { - t.Run(name, func(t *testing.T) { - err := engine.ValidateMasterUsername(username) - require.Error(t, err) - assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) - }) - } -} - -// The name is interpolated into a CREATE DATABASE in the guest, so the rule is -// narrower than the engine's own: it is what makes the interpolation safe. -func TestEngineValidateDBName(t *testing.T) { - engine, err := LookupEngine("postgres") - require.NoError(t, err) - - // Empty is not a rejection: AWS creates no initial database when none is named. - for _, name := range []string{"", "orders", "order_db1", "Orders"} { - assert.NoError(t, engine.ValidateDBName(name), "DBName %q", name) - } - - cases := map[string]string{ - "hyphen": "my-db", - "leading digit": "1db", - "space": "my db", - "dot": "my.db", - // MariaDB maps a database name onto a directory name, so these are - // structurally dangerous there rather than merely awkward. - "slash": "my/db", - "backslash": `my\db`, - "trailing space": "orders ", - "quote": "orders'", - "too long": strings.Repeat("a", 64), - } - for name, dbName := range cases { - t.Run(name, func(t *testing.T) { - err := engine.ValidateDBName(dbName) - require.Error(t, err) - assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) - }) - } -} - -func TestValidateMasterUserPassword(t *testing.T) { - t.Parallel() - assert.NoError(t, ValidateMasterUserPassword("Sup3rSecret!")) - assert.NoError(t, ValidateMasterUserPassword(strings.Repeat("x", maxMasterPasswordLen))) - - for _, password := range []string{ - "", - strings.Repeat("x", minMasterPasswordLen-1), - strings.Repeat("x", maxMasterPasswordLen+1), - // The characters AWS excludes, which would otherwise break a connection - // string or the engine's own role syntax. - "pass/word1", `pass"word1`, "pass@word1", "pass word1", - // Outside printable ASCII. A newline survives the bootstrap handoff and - // defeats the guest's line-oriented redaction, putting the password on - // the host serial console; the rest are refused as the same class. - "pass\nword1", "pass\rword1", "pass\tword1", "pass\x00word1", "pass\x7fword1", - "pässword1", "passwordé", "pass\u00a0word1", - } { - err := ValidateMasterUserPassword(password) - require.Error(t, err, "password %q should be rejected", password) - assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) - } -} diff --git a/spinifex/handlers/rds/enginefamily_test.go b/spinifex/handlers/rds/enginefamily_test.go index 600fbe085..69b6f8979 100644 --- a/spinifex/handlers/rds/enginefamily_test.go +++ b/spinifex/handlers/rds/enginefamily_test.go @@ -1,139 +1,16 @@ package handlers_rds -//test:in-package — builds Engine values from unexported fields (catalog, -// maxUsernameLen, validateDBName) and swaps the unexported engines and -// enginesByFamily registries, none of which are reachable from outside. - import ( "testing" "time" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/rds" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// A second engine that exists solely inside the test binary, so the cross-engine -// refusal is proven against the seam rather than against whichever engines the -// build happens to offer. Nothing about its catalog matters beyond being -// distinguishable from the shipped ones. -const ( - testEngineName = "testdb" - testEngineGroup = "tuned-testdb" -) - -var engineUnderTest = Engine{ - Name: testEngineName, - MajorVersion: "1", - DefaultPort: 3306, - description: "Test Engine", - licenseModel: "test-license", - reservedUsernames: []string{"root"}, - reservedUsernamePrefixes: []string{"testdb_"}, - maxUsernameLen: 80, - validateDBName: dbNameRule(64), - catalog: map[string]ParameterSpec{ - "buffer_size": { - Name: "buffer_size", DataType: ParamTypeInteger, ApplyType: ApplyTypeStatic, - IsModifiable: true, Min: 1, Max: 1024, Default: "64", Unit: "MB", - Description: "Buffer the server allocates at startup, in MB.", - }, - "connection_limit": { - Name: "connection_limit", DataType: ParamTypeInteger, ApplyType: ApplyTypeDynamic, - IsModifiable: true, Min: 1, Max: 1000, Default: "100", - Description: "Maximum concurrent connections to the server.", - }, - }, - 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 -// family-knowing caller reads the one keyed by family. -func registerTestEngine(t *testing.T) { - t.Helper() - require.NotContains(t, engines, engineUnderTest.Name) - engines[engineUnderTest.Name] = engineUnderTest - indexed, err := indexEnginesByFamily(engines) - require.NoError(t, err) - enginesByFamily = indexed - t.Cleanup(func() { - delete(engines, engineUnderTest.Name) - indexed, err := indexEnginesByFamily(engines) - require.NoError(t, err) - enginesByFamily = indexed - }) -} - -func TestValidateEngineRegistry(t *testing.T) { - require.NoError(t, ValidateEngineRegistry()) -} - -func TestIndexEnginesByFamily_RejectsInvalidMetadata(t *testing.T) { - noDBNameRule := engineUnderTest - noDBNameRule.validateDBName = nil - noCrashRecoveryNote := engineUnderTest - noCrashRecoveryNote.crashRecoveryNote = "" - noUncleanStopNote := engineUnderTest - noUncleanStopNote.uncleanStopNote = "" - invalidTLSParameter := engineUnderTest - invalidTLSParameter.tlsEnforcementParameter = "not-a-boolean-parameter" - - tests := []struct { - name string - registry map[string]Engine - wantErr string - }{ - { - name: "missing DB name rule", - registry: map[string]Engine{"test": noDBNameRule}, - wantErr: "registers no DBName rule", - }, - { - name: "missing crash recovery note", - registry: map[string]Engine{"test": noCrashRecoveryNote}, - wantErr: "registers no crash-recovery note", - }, - { - name: "missing unclean stop note", - registry: map[string]Engine{"test": noUncleanStopNote}, - wantErr: "registers no unclean-stop note", - }, - { - name: "invalid TLS enforcement parameter", - registry: map[string]Engine{"test": invalidTLSParameter}, - wantErr: "is not a boolean parameter it exposes", - }, - { - name: "duplicate parameter group family", - registry: map[string]Engine{ - "first": engineUnderTest, - "second": engineUnderTest, - }, - wantErr: "is claimed by two engines", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - indexed, err := indexEnginesByFamily(tt.registry) - require.ErrorContains(t, err, tt.wantErr) - assert.Nil(t, indexed) - }) - } -} - -func testEngineGroupInput(name string) *rds.CreateDBParameterGroupInput { - return &rds.CreateDBParameterGroupInput{ - DBParameterGroupName: aws.String(name), - DBParameterGroupFamily: aws.String(engineUnderTest.ParameterGroupFamily()), - Description: aws.String("Tuned for the second engine"), - } -} - func requireFamilyMismatch(t *testing.T, err error) { t.Helper() require.Error(t, err) @@ -143,17 +20,16 @@ func requireFamilyMismatch(t *testing.T, err error) { // A group carries the settings of exactly one engine, so attaching one of // another engine would boot the database on a configuration file it cannot -// parse. +// parse. Proven with the two shipped engines rather than a fake third one. func TestCreateDBInstance_RefusesAGroupOfAnotherEnginesFamily(t *testing.T) { - registerTestEngine(t) h := newCreateHarness(t, testBaseDomain) - _, err := h.svc.CreateDBParameterGroup(t.Context(), testEngineGroupInput(testEngineGroup), testAccountID) + _, err := h.svc.CreateDBParameterGroup(t.Context(), mariadbParameterGroupInput(testMariaDBParameterGroup), testAccountID) require.NoError(t, err) _, err = h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) require.NoError(t, err) input := validCreateInput() - input.DBParameterGroupName = aws.String(testEngineGroup) + input.DBParameterGroupName = aws.String(testMariaDBParameterGroup) _, err = h.svc.CreateDBInstance(t.Context(), input, testAccountID) requireFamilyMismatch(t, err) assert.Contains(t, err.Error(), enginePostgres.ParameterGroupFamily(), "the error names the family the instance needs") @@ -161,23 +37,22 @@ func TestCreateDBInstance_RefusesAGroupOfAnotherEnginesFamily(t *testing.T) { // The other direction: neither engine may borrow the other's group. input = validCreateInput() - input.Engine = aws.String(testEngineName) + input.Engine = aws.String("mariadb") input.DBParameterGroupName = aws.String(testParameterGroup) _, err = h.svc.CreateDBInstance(t.Context(), input, testAccountID) requireFamilyMismatch(t, err) - assert.Contains(t, err.Error(), engineUnderTest.ParameterGroupFamily()) + assert.Contains(t, err.Error(), engineMariaDB.ParameterGroupFamily()) assert.False(t, h.recordExists(t, testDBInstanceID)) } func TestModifyDBInstance_RefusesAGroupOfAnotherEnginesFamily(t *testing.T) { - registerTestEngine(t) h := newModifyHarness(t) - _, err := h.svc.CreateDBParameterGroup(t.Context(), testEngineGroupInput(testEngineGroup), testAccountID) + _, err := h.svc.CreateDBParameterGroup(t.Context(), mariadbParameterGroupInput(testMariaDBParameterGroup), testAccountID) require.NoError(t, err) seedInstance(t, h.svc, modifiableRecord()) input := modifyInput() - input.DBParameterGroupName = aws.String(testEngineGroup) + input.DBParameterGroupName = aws.String(testMariaDBParameterGroup) input.ApplyImmediately = aws.Bool(true) _, err = h.svc.ModifyDBInstance(t.Context(), input, testAccountID) requireFamilyMismatch(t, err) @@ -188,14 +63,13 @@ func TestModifyDBInstance_RefusesAGroupOfAnotherEnginesFamily(t *testing.T) { // The deferred half of a modify re-resolves at apply time, so the check has to // hold there too rather than only at the request that recorded it. func TestApplyPendingModifications_RefusesAGroupOfAnotherEnginesFamily(t *testing.T) { - registerTestEngine(t) h := newModifyHarness(t) h.agent.replyWith("") - _, err := h.svc.CreateDBParameterGroup(t.Context(), testEngineGroupInput(testEngineGroup), testAccountID) + _, err := h.svc.CreateDBParameterGroup(t.Context(), mariadbParameterGroupInput(testMariaDBParameterGroup), testAccountID) require.NoError(t, err) rec := modifyingRecord(&PendingModifiedValues{ - DBParameterGroupName: testEngineGroup, + DBParameterGroupName: testMariaDBParameterGroup, RequestedAt: time.Now().UTC(), }) seedInstance(t, h.svc, rec) @@ -211,14 +85,13 @@ func TestApplyPendingModifications_RefusesAGroupOfAnotherEnginesFamily(t *testin // A restore takes its engine from the snapshot rather than the request, so the // group named alongside it is checked against what the restored data is. func TestRestoreDBInstanceFromDBSnapshot_RefusesAGroupOfAnotherEnginesFamily(t *testing.T) { - registerTestEngine(t) h := newSnapshotHarness(t, false) h.seedSnapshot(t) - _, err := h.svc.CreateDBParameterGroup(t.Context(), testEngineGroupInput(testEngineGroup), testAccountID) + _, err := h.svc.CreateDBParameterGroup(t.Context(), mariadbParameterGroupInput(testMariaDBParameterGroup), testAccountID) require.NoError(t, err) input := restoreInput() - input.DBParameterGroupName = aws.String(testEngineGroup) + input.DBParameterGroupName = aws.String(testMariaDBParameterGroup) _, err = h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), input, testAccountID) requireFamilyMismatch(t, err) assert.False(t, h.instanceExists(t, testRestoredID), "a rejected restore must reserve nothing") @@ -228,46 +101,46 @@ func TestRestoreDBInstanceFromDBSnapshot_RefusesAGroupOfAnotherEnginesFamily(t * // alternative stores one engine's setting into another's group and defers the // failure to whichever instance next attaches it. func TestModifyDBParameterGroup_ValidatesAgainstTheGroupsOwnEngine(t *testing.T) { - registerTestEngine(t) h := newCreateHarness(t, testBaseDomain) - _, err := h.svc.CreateDBParameterGroup(t.Context(), testEngineGroupInput(testEngineGroup), testAccountID) + _, err := h.svc.CreateDBParameterGroup(t.Context(), mariadbParameterGroupInput(testMariaDBParameterGroup), testAccountID) require.NoError(t, err) _, err = h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) require.NoError(t, err) + // work_mem is PostgreSQL-only: a MariaDB group does not carry it. _, err = h.svc.ModifyDBParameterGroup(t.Context(), - modifyParameters(testEngineGroup, parameter("work_mem", "16384", "")), testAccountID) + modifyParameters(testMariaDBParameterGroup, parameter("work_mem", "16384", "")), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) + + // max_connect_errors is MariaDB-only: a PostgreSQL group does not carry it. + _, err = h.svc.ModifyDBParameterGroup(t.Context(), + modifyParameters(testParameterGroup, parameter("max_connect_errors", "50", "")), testAccountID) require.Error(t, err) assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) _, err = h.svc.ModifyDBParameterGroup(t.Context(), - modifyParameters(testParameterGroup, parameter("connection_limit", "50", "")), testAccountID) - require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) - - _, err = h.svc.ModifyDBParameterGroup(t.Context(), - modifyParameters(testEngineGroup, parameter("connection_limit", "50", "")), testAccountID) + modifyParameters(testMariaDBParameterGroup, parameter("max_connect_errors", "50", "")), testAccountID) require.NoError(t, err, "a group takes its own engine's parameters") - assert.Equal(t, "50", aws.StringValue(describedParameters(t, h, testEngineGroup)["connection_limit"].ParameterValue)) + assert.Equal(t, "50", aws.StringValue(describedParameters(t, h, testMariaDBParameterGroup)["max_connect_errors"].ParameterValue)) } func TestDescribeDBParameters_ListsTheGroupsOwnEngineCatalog(t *testing.T) { - registerTestEngine(t) h := newCreateHarness(t, testBaseDomain) - _, err := h.svc.CreateDBParameterGroup(t.Context(), testEngineGroupInput(testEngineGroup), testAccountID) + _, err := h.svc.CreateDBParameterGroup(t.Context(), mariadbParameterGroupInput(testMariaDBParameterGroup), testAccountID) require.NoError(t, err) - params := describedParameters(t, h, testEngineGroup) - require.Len(t, params, len(engineUnderTest.CatalogParameterNames())) + params := describedParameters(t, h, testMariaDBParameterGroup) + require.Len(t, params, len(engineMariaDB.CatalogParameterNames())) assert.NotContains(t, params, "work_mem", "a group must not report another engine's settings") - assert.Equal(t, "100", aws.StringValue(params["connection_limit"].ParameterValue)) - assert.Equal(t, ParameterSourceEngineDefault, aws.StringValue(params["connection_limit"].Source)) + assert.Equal(t, "100", aws.StringValue(params["max_connect_errors"].ParameterValue)) + assert.Equal(t, ParameterSourceEngineDefault, aws.StringValue(params["max_connect_errors"].Source)) } -// The rejection has to name what the client can use instead, which stops being -// one family as soon as a second engine registers. +// The rejection has to name what the client can use instead. Exercised again +// here (alongside TestCreateDBParameterGroup_RejectsAnUnofferedFamily) because +// this assertion additionally pins the exact, sorted family list. func TestCreateDBParameterGroup_RejectionNamesEverySupportedFamily(t *testing.T) { - registerTestEngine(t) h := newCreateHarness(t, testBaseDomain) input := parameterGroupInput(testParameterGroup) @@ -275,12 +148,11 @@ func TestCreateDBParameterGroup_RejectionNamesEverySupportedFamily(t *testing.T) _, err := h.svc.CreateDBParameterGroup(t.Context(), input, testAccountID) require.Error(t, err) assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) - for _, family := range SupportedParameterGroupFamilies() { + for _, family := range rdsengine.SupportedParameterGroupFamilies() { assert.Contains(t, err.Error(), family) } assert.Equal(t, []string{ engineMariaDB.ParameterGroupFamily(), enginePostgres.ParameterGroupFamily(), - engineUnderTest.ParameterGroupFamily(), - }, SupportedParameterGroupFamilies()) + }, rdsengine.SupportedParameterGroupFamilies()) } diff --git a/spinifex/handlers/rds/enginetest_helpers_test.go b/spinifex/handlers/rds/enginetest_helpers_test.go new file mode 100644 index 000000000..d15bac6d3 --- /dev/null +++ b/spinifex/handlers/rds/enginetest_helpers_test.go @@ -0,0 +1,48 @@ +package handlers_rds + +import ( + "testing" + + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" +) + +// Test-only handles onto the two built-in engines. Resolved through LookupEngine +// rather than reaching into the engine package's own registry, which this +// package cannot do from outside it. +var ( + enginePostgres = mustLookupEngine("postgres") + engineMariaDB = mustLookupEngine("mariadb") +) + +func mustLookupEngine(name string) rdsengine.Engine { + engine, err := rdsengine.LookupEngine(name) + if err != nil { + panic(err) + } + return engine +} + +// The value one name carries in a resolved set, exactly as the guest is handed +// it: resolvedParameterValues lowercases, which the combination checks want and +// an assertion about what is written into an option file does not. +func resolvedParameter(t *testing.T, resolved []Parameter, name string) string { + t.Helper() + for _, param := range resolved { + if param.Name == name { + return param.Value + } + } + t.Fatalf("the resolved set carries no %s", name) + return "" +} + +// Converts the engine package's own Setting slice to this package's Parameter, +// for a test that calls ResolveEffectiveParameters directly rather than through +// resolveGroupParameters, which performs the same conversion in production. +func toParameters(settings []rdsengine.Setting) []Parameter { + out := make([]Parameter, len(settings)) + for i, setting := range settings { + out[i] = Parameter{Name: setting.Name, Value: setting.Value} + } + return out +} diff --git a/spinifex/handlers/rds/lifecycle.go b/spinifex/handlers/rds/lifecycle.go index c796912de..0ace41ede 100644 --- a/spinifex/handlers/rds/lifecycle.go +++ b/spinifex/handlers/rds/lifecycle.go @@ -15,6 +15,7 @@ import ( "github.com/aws/aws-sdk-go/service/rds" "github.com/mulgadc/spinifex/contracts/ec2/v1" ec2instance "github.com/mulgadc/spinifex/spinifex/domains/ec2/instance" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" "github.com/mulgadc/spinifex/spinifex/foundation/state/kvstore" "github.com/nats-io/nats.go" @@ -257,7 +258,7 @@ func (s *Service) stopEngineOrRecordFallback(ctx context.Context, accountID stri // 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) + engine, err := rdsengine.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. @@ -265,7 +266,7 @@ func uncleanStopMessage(ctx context.Context, engineName, operation string) strin "engine", engineName, "err", err) return warning } - return warning + " " + engine.uncleanStopNote + return warning + " " + engine.UncleanStopNote() } // Moves the instance into a transitional state under CAS and returns the record diff --git a/spinifex/handlers/rds/modify.go b/spinifex/handlers/rds/modify.go index b60ab99f3..0a26612f1 100644 --- a/spinifex/handlers/rds/modify.go +++ b/spinifex/handlers/rds/modify.go @@ -12,6 +12,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/rds" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" "github.com/mulgadc/spinifex/spinifex/foundation/state/kvstore" ) @@ -214,10 +215,10 @@ func (s *Service) planModify(ctx context.Context, input *rds.ModifyDBInstanceInp } if class := aws.StringValue(input.DBInstanceClass); class != "" && class != rec.DBInstanceClass { - instanceType, err := InstanceTypeForClass(class) + instanceType, err := s.sizing.InstanceTypeForClass(class) if err != nil { return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, - "DBInstanceClass %q is not supported; supported classes are %s", class, strings.Join(SupportedInstanceClasses(), ", ")) + "DBInstanceClass %q is not supported; supported classes are %s", class, strings.Join(rdsengine.SupportedInstanceClasses(), ", ")) } plan.InstanceClass, plan.InstanceType = class, instanceType } @@ -237,7 +238,7 @@ func (s *Service) planModify(ctx context.Context, input *rds.ModifyDBInstanceInp if plan.ParameterGroup != "" { targetGroup = plan.ParameterGroup } - engine, err := LookupEngine(rec.Engine) + engine, err := rdsengine.LookupEngine(rec.Engine) if err != nil { return nil, err } diff --git a/spinifex/handlers/rds/parametergroup.go b/spinifex/handlers/rds/parametergroup.go index 76d50ddd9..c1289866f 100644 --- a/spinifex/handlers/rds/parametergroup.go +++ b/spinifex/handlers/rds/parametergroup.go @@ -12,6 +12,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/rds" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" "github.com/mulgadc/spinifex/spinifex/foundation/state/kvstore" ) @@ -19,10 +20,25 @@ import ( // AWS's cap on one ModifyDBParameterGroup call. const maxParametersPerModify = 20 -// The prefix AWS reserves for the groups the service itself owns. A group named -// with it is reported and resolvable but never modifiable or deletable. +// The prefix AWS reserves for the groups the service itself owns. Duplicated +// against the engine package's own copy deliberately: that one backs a name +// this package derives, this one a name a caller recognises, and neither may +// reach across the boundary to the other. const defaultParameterGroupPrefix = "default." +// AWS's own ApplyMethod values, echoed back on DescribeDBParameters. +const ( + ApplyMethodImmediate = "immediate" + ApplyMethodPendingReboot = "pending-reboot" +) + +// Where a reported value came from. AWS distinguishes these and the Terraform +// provider reads them, so a computed default must not be reported as user. +const ( + ParameterSourceUser = "user" + ParameterSourceEngineDefault = "engine-default" +) + // CreateDBParameterGroup creates a customer-owned parameter group. It starts empty: every value is a // catalog default until the customer overrides one, so a fresh group and the default group resolve to // the same effective set. @@ -111,7 +127,11 @@ func (s *Service) DescribeDBParameterGroups(ctx context.Context, input *rds.Desc } // The default group is synthesised rather than read, so it appears exactly // once whether or not a prior create persisted it. - for _, engine := range engines { + for _, name := range rdsengine.SupportedEngines() { + engine, err := rdsengine.LookupEngine(name) + if err != nil { + return nil, err + } names = append(names, engine.DefaultParameterGroupName()) } slices.Sort(names) @@ -173,7 +193,7 @@ func (s *Service) ModifyDBParameterGroup(ctx context.Context, input *rds.ModifyD if err != nil { return nil, err } - engine, err := engineForFamily(rec.Family) + engine, err := rdsengine.EngineForFamily(rec.Family) if err != nil { return nil, err } @@ -266,7 +286,7 @@ func (s *Service) DescribeDBParameters(ctx context.Context, input *rds.DescribeD } // The catalog listed is the one the group's family names, so a group of one // engine never reports another engine's settings. - engine, err := engineForFamily(rec.Family) + engine, err := rdsengine.EngineForFamily(rec.Family) if err != nil { return nil, err } @@ -274,7 +294,7 @@ func (s *Service) DescribeDBParameters(ctx context.Context, input *rds.DescribeD if err != nil { return nil, err } - memoryMiB, err := classMemoryMiB(SmallestInstanceClass()) + memoryMiB, err := s.sizing.ClassMemoryMiB(s.sizing.SmallestInstanceClass()) if err != nil { return nil, err } @@ -377,13 +397,13 @@ type parameterUpdate struct { // Every entry is checked before any is written. A name repeated in one request // keeps its last value, as AWS does. -func validateParameterUpdates(engine Engine, params []*rds.Parameter) ([]parameterUpdate, error) { +func validateParameterUpdates(engine rdsengine.Engine, params []*rds.Parameter) ([]parameterUpdate, error) { byName := make(map[string]parameterUpdate, len(params)) for _, param := range params { if param == nil { return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "a parameter entry is empty") } - spec, err := engine.validateParameterValue(aws.StringValue(param.ParameterName), aws.StringValue(param.ParameterValue)) + spec, err := engine.ValidateParameterValue(aws.StringValue(param.ParameterName), aws.StringValue(param.ParameterValue)) if err != nil { return nil, err } @@ -408,18 +428,18 @@ func validateParameterUpdates(engine Engine, params []*rds.Parameter) ([]paramet // A static parameter cannot be applied immediately, which AWS rejects rather // than silently downgrading — accepting it would tell the customer the change is // live when the engine has not adopted it. -func resolveApplyMethod(spec ParameterSpec, requested string) (string, error) { +func resolveApplyMethod(spec rdsengine.ParameterSpec, requested string) (string, error) { method := strings.ToLower(strings.TrimSpace(requested)) switch method { case "": - if spec.ApplyType == ApplyTypeStatic { + if spec.ApplyType == rdsengine.ApplyTypeStatic { return ApplyMethodPendingReboot, nil } return ApplyMethodImmediate, nil case ApplyMethodPendingReboot: return ApplyMethodPendingReboot, nil case ApplyMethodImmediate: - if spec.ApplyType == ApplyTypeStatic { + if spec.ApplyType == rdsengine.ApplyTypeStatic { return "", awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "parameter %s is static, so ApplyMethod must be %s", spec.Name, ApplyMethodPendingReboot) } @@ -443,7 +463,7 @@ func getDBParameterGroup(ctx context.Context, kv *kvstore.Bucket, accountID, nam if found { return &rec, rev, nil } - if engine, ok := engineForDefaultParameterGroup(name); ok { + if engine, ok := rdsengine.EngineForDefaultParameterGroup(name); ok { return defaultParameterGroupRecord(engine, accountID), 0, nil } return nil, 0, awserrors.Errorf(awserrors.ErrorDBParameterGroupNotFound, "DB parameter group %s not found", name) @@ -451,7 +471,7 @@ func getDBParameterGroup(ctx context.Context, kv *kvstore.Bucket, accountID, nam // The implicit group, identical for every account. It carries no tags and no // stored values, so it resolves to the catalog defaults alone. -func defaultParameterGroupRecord(engine Engine, accountID string) *DBParameterGroupRecord { +func defaultParameterGroupRecord(engine rdsengine.Engine, accountID string) *DBParameterGroupRecord { return &DBParameterGroupRecord{ Name: engine.DefaultParameterGroupName(), AccountID: accountID, @@ -464,25 +484,14 @@ func isDefaultParameterGroupName(name string) bool { return strings.HasPrefix(strings.ToLower(name), defaultParameterGroupPrefix) } -// The engine whose implicit default group carries this name, so an unrecognised -// default.* name is a not-found rather than a group that resolves to nothing. -func engineForDefaultParameterGroup(name string) (Engine, bool) { - for _, engine := range engines { - if engine.DefaultParameterGroupName() == name { - return engine, true - } - } - return Engine{}, false -} - // The family is required: defaulting it would create a group for an engine the // caller never named, refused only later when another engine tries to attach it. func validateParameterGroupFamily(family string) (string, error) { - if normaliseFamily(family) == "" { + if strings.TrimSpace(family) == "" { return "", awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "The parameter ParameterGroupFamily must be provided and must not be empty.") } - engine, err := engineForFamily(family) + engine, err := rdsengine.EngineForFamily(family) if err != nil { return "", err } @@ -498,7 +507,7 @@ func parameterSource(isOverride bool) string { // A computed default is reported as engine-default with its literal value, never // as the formula that produced it. -func projectParameter(spec ParameterSpec, value, storedApplyMethod string, isOverride bool) *rds.Parameter { +func projectParameter(spec rdsengine.ParameterSpec, value, storedApplyMethod string, isOverride bool) *rds.Parameter { out := &rds.Parameter{ ParameterName: aws.String(spec.Name), ParameterValue: aws.String(value), @@ -515,7 +524,7 @@ func projectParameter(spec ParameterSpec, value, storedApplyMethod string, isOve // defaults and records written before ApplyMethod was persisted. if storedApplyMethod != "" { out.ApplyMethod = aws.String(storedApplyMethod) - } else if spec.ApplyType == ApplyTypeStatic { + } else if spec.ApplyType == rdsengine.ApplyTypeStatic { out.ApplyMethod = aws.String(ApplyMethodPendingReboot) } else { out.ApplyMethod = aws.String(ApplyMethodImmediate) @@ -543,7 +552,7 @@ func (s *Service) projectParameterGroupRecord(rec *DBParameterGroupRecord) *rds. // Every path that binds a group to an instance comes through here — create, // modify, restore, the deferred apply and group propagation — so the // cross-engine refusal is one check rather than five. -func (s *Service) resolveGroupParameters(ctx context.Context, kv *kvstore.Bucket, accountID string, engine Engine, group, instanceClass string) ([]Parameter, error) { +func (s *Service) resolveGroupParameters(ctx context.Context, kv *kvstore.Bucket, accountID string, engine rdsengine.Engine, group, instanceClass string) ([]Parameter, error) { rec, _, err := getDBParameterGroup(ctx, kv, accountID, group) if err != nil { return nil, err @@ -561,10 +570,14 @@ func (s *Service) resolveGroupParameters(ctx context.Context, kv *kvstore.Bucket for name, override := range overrides { values[name] = override.Value } - resolved, err := engine.ResolveEffectiveParameters(instanceClass, values) + settings, err := engine.ResolveEffectiveParameters(s.sizing, instanceClass, values) if err != nil { return nil, err } + resolved := make([]Parameter, len(settings)) + for i, setting := range settings { + resolved[i] = Parameter{Name: setting.Name, Value: setting.Value} + } if err := s.checkTLSEnforceable(engine, group, resolved); err != nil { return nil, err } @@ -575,9 +588,9 @@ func (s *Service) resolveGroupParameters(ctx context.Context, kv *kvstore.Bucket // serve one, so a binding is refused here rather than at the boot that would // otherwise start an instance nothing can reach. A formed deployment always // holds a cluster CA, so this is not expected to fire. -func (s *Service) checkTLSEnforceable(engine Engine, group string, resolved []Parameter) error { +func (s *Service) checkTLSEnforceable(engine rdsengine.Engine, group string, resolved []Parameter) error { name := engine.TLSEnforcementParameter() - if name == "" || resolvedValues(resolved)[name] != "1" { + if name == "" || resolvedParameterValues(resolved)[name] != "1" { return nil } available, err := s.tlsAvailable() @@ -594,3 +607,14 @@ func (s *Service) checkTLSEnforceable(engine Engine, group string, resolved []Pa "configure the cluster CA, or set %s to 0 in %s", name, group, name, group) } + +// Reads a resolved parameter set, keyed by name, for the one check above that +// needs a value out of it. This package's own Parameter, not the engine +// package's Setting: the conversion happens at resolveGroupParameters. +func resolvedParameterValues(params []Parameter) map[string]string { + values := make(map[string]string, len(params)) + for _, param := range params { + values[param.Name] = strings.ToLower(strings.TrimSpace(param.Value)) + } + return values +} diff --git a/spinifex/handlers/rds/parametergroup_test.go b/spinifex/handlers/rds/parametergroup_test.go index 3c3e27098..fff53aa35 100644 --- a/spinifex/handlers/rds/parametergroup_test.go +++ b/spinifex/handlers/rds/parametergroup_test.go @@ -7,6 +7,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/rds" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -128,7 +129,7 @@ func TestCreateDBParameterGroup_RejectsAnUnofferedFamily(t *testing.T) { const prefix = "ParameterGroupFamily mysql8.0 is not a valid parameter group family. Supported families: " require.True(t, strings.HasPrefix(message, prefix), message) named := strings.Split(strings.TrimSuffix(strings.TrimPrefix(message, prefix), "."), ", ") - assert.ElementsMatch(t, SupportedParameterGroupFamilies(), named) + assert.ElementsMatch(t, rdsengine.SupportedParameterGroupFamilies(), named) } // A customer group under the reserved prefix would be indistinguishable from the @@ -521,11 +522,13 @@ func TestDescribeDBParameters_ReportsComputedDefaultsAsLiterals(t *testing.T) { // The class is named rather than derived from the class catalog, which would // make this assert the code agrees with itself. - memoryMiB, err := classMemoryMiB("db.t3.micro") + memoryMiB, err := InstanceSizing().ClassMemoryMiB("db.t3.micro") require.NoError(t, err) - assert.Equal(t, sharedBuffersFor(memoryMiB), aws.StringValue(shared.ParameterValue)) + spec, ok := enginePostgres.LookupParameter("shared_buffers") + require.True(t, ok) + assert.Equal(t, spec.DefaultAt(memoryMiB), aws.StringValue(shared.ParameterValue)) assert.Equal(t, ParameterSourceEngineDefault, aws.StringValue(shared.Source)) - assert.Equal(t, ApplyTypeStatic, aws.StringValue(shared.ApplyType)) + assert.Equal(t, rdsengine.ApplyTypeStatic, aws.StringValue(shared.ApplyType)) assert.Equal(t, ApplyMethodPendingReboot, aws.StringValue(shared.ApplyMethod)) assert.True(t, aws.BoolValue(shared.IsModifiable)) assert.NotEmpty(t, aws.StringValue(shared.AllowedValues)) @@ -548,8 +551,9 @@ func TestDescribeDBParameters_ReportsTheCustomersBooleanSpelling(t *testing.T) { assert.Equal(t, "off", aws.StringValue(params["autovacuum"].ParameterValue)) assert.Equal(t, ParameterSourceUser, aws.StringValue(params["autovacuum"].Source)) - resolved, err := enginePostgres.ResolveEffectiveParameters("db.t3.micro", map[string]string{"autovacuum": "off"}) + settings, err := enginePostgres.ResolveEffectiveParameters(InstanceSizing(), "db.t3.micro", map[string]string{"autovacuum": "off"}) require.NoError(t, err) + resolved := toParameters(settings) assert.Equal(t, "0", resolvedParameter(t, resolved, "autovacuum"), "the guest should be handed the canonical spelling rather than the customer's") } @@ -766,9 +770,11 @@ func TestCreateDBInstance_ResolvesTheNamedGroupIntoTheBootstrapSet(t *testing.T) assert.Equal(t, "16384", values["work_mem"]) // Evaluated against the instance's own class, which is the whole point of a // computed default: a literal tuned elsewhere would be wrong here. - memoryMiB, err := classMemoryMiB("db.t3.medium") + memoryMiB, err := InstanceSizing().ClassMemoryMiB("db.t3.medium") require.NoError(t, err) - assert.Equal(t, sharedBuffersFor(memoryMiB), values["shared_buffers"]) + spec, ok := enginePostgres.LookupParameter("shared_buffers") + require.True(t, ok) + assert.Equal(t, spec.DefaultAt(memoryMiB), values["shared_buffers"]) } // An unnamed group takes the implicit default, which resolves to the catalog diff --git a/spinifex/handlers/rds/pending.go b/spinifex/handlers/rds/pending.go index d7fa92ee4..51f138c8b 100644 --- a/spinifex/handlers/rds/pending.go +++ b/spinifex/handlers/rds/pending.go @@ -7,6 +7,7 @@ import ( "log/slog" "github.com/aws/aws-sdk-go/aws" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/mulgadc/spinifex/spinifex/foundation/state/kvstore" ) @@ -73,7 +74,7 @@ func (s *Service) applyPendingModifications(ctx context.Context, kv *kvstore.Buc } switch { case pending.DBInstanceClass != "": - instanceType, err := InstanceTypeForClass(pending.DBInstanceClass) + instanceType, err := s.sizing.InstanceTypeForClass(pending.DBInstanceClass) if err != nil { return fmt.Errorf("rds: DBInstanceClass %q is not supported", pending.DBInstanceClass) } @@ -147,7 +148,7 @@ func (s *Service) applyPendingModifications(ctx context.Context, kv *kvstore.Buc // the new group's overrides, so a parameter the old group set and the new one // does not reverts to its default rather than lingering. func (s *Service) applyParameterGroup(ctx context.Context, kv *kvstore.Bucket, accountID string, rec *DBInstanceRecord, group, instanceClass string, tolerateUnreachableAgent bool) error { - engine, err := LookupEngine(rec.Engine) + engine, err := rdsengine.LookupEngine(rec.Engine) if err != nil { return err } diff --git a/spinifex/handlers/rds/pending_test.go b/spinifex/handlers/rds/pending_test.go index ab76b49df..01ea721eb 100644 --- a/spinifex/handlers/rds/pending_test.go +++ b/spinifex/handlers/rds/pending_test.go @@ -362,9 +362,11 @@ func TestApplyPendingModifications_ReResolvesTheParametersForTheNewClass(t *test for _, param := range issued[0].Parameters { applied[param.Name] = param.Value } - memoryMiB, err := classMemoryMiB("db.m5.xlarge") + memoryMiB, err := InstanceSizing().ClassMemoryMiB("db.m5.xlarge") require.NoError(t, err) - assert.Equal(t, sharedBuffersFor(memoryMiB), applied["shared_buffers"], + spec, ok := enginePostgres.LookupParameter("shared_buffers") + require.True(t, ok) + assert.Equal(t, spec.DefaultAt(memoryMiB), applied["shared_buffers"], "the set was resolved against the class the instance is becoming") stored := h.record(t) @@ -467,9 +469,11 @@ func TestApplyPendingModifications_ClassChangeSurvivesAnUnreachableAgent(t *test assert.False(t, stored.ParameterApplyFailed) assert.Empty(t, stored.PendingRebootParameters) - memoryMiB, err := classMemoryMiB("db.m5.xlarge") + memoryMiB, err := InstanceSizing().ClassMemoryMiB("db.m5.xlarge") require.NoError(t, err) - assert.Equal(t, sharedBuffersFor(memoryMiB), parameterValue(stored.Bootstrap.ResolvedParameters, "shared_buffers"), + spec, ok := enginePostgres.LookupParameter("shared_buffers") + require.True(t, ok) + assert.Equal(t, spec.DefaultAt(memoryMiB), parameterValue(stored.Bootstrap.ResolvedParameters, "shared_buffers"), "the stored set was resolved against the class the instance is becoming, for the replacement to boot on") } diff --git a/spinifex/handlers/rds/replace.go b/spinifex/handlers/rds/replace.go index 874340f77..e7070cac6 100644 --- a/spinifex/handlers/rds/replace.go +++ b/spinifex/handlers/rds/replace.go @@ -47,7 +47,7 @@ func (s *Service) replaceInstanceVM(ctx context.Context, kv *kvstore.Bucket, acc } instanceType := in.InstanceType if instanceType == "" { - resolved, err := InstanceTypeForClass(rec.DBInstanceClass) + resolved, err := s.sizing.InstanceTypeForClass(rec.DBInstanceClass) if err != nil { return fmt.Errorf("rds: DB instance %s has an unmapped class %q", rec.DBInstanceIdentifier, rec.DBInstanceClass) } diff --git a/spinifex/handlers/rds/restore.go b/spinifex/handlers/rds/restore.go index 11c889061..e7eee62c2 100644 --- a/spinifex/handlers/rds/restore.go +++ b/spinifex/handlers/rds/restore.go @@ -11,6 +11,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/rds" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" "github.com/mulgadc/spinifex/spinifex/foundation/aws/tags" "github.com/mulgadc/spinifex/spinifex/foundation/state/kvstore" @@ -162,7 +163,7 @@ func (s *Service) RestoreDBInstanceFromDBSnapshot(ctx context.Context, input *rd func (s *Service) resolveRestoreRequest(input *rds.RestoreDBInstanceFromDBSnapshotInput, snapshot *DBSnapshotRecord) (*validatedCreate, error) { // The snapshot's engine, never the request's: the datadir is written in one // engine's on-disk format and no other can read it. - engine, err := LookupEngine(snapshot.Engine) + engine, err := rdsengine.LookupEngine(snapshot.Engine) if err != nil { return nil, err } @@ -176,10 +177,10 @@ func (s *Service) resolveRestoreRequest(input *rds.RestoreDBInstanceFromDBSnapsh if instanceClass == "" { instanceClass = snapshot.DBInstanceClass } - instanceType, err := InstanceTypeForClass(instanceClass) + instanceType, err := s.sizing.InstanceTypeForClass(instanceClass) if err != nil { return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, - "DBInstanceClass %q is not supported; supported classes are %s", instanceClass, strings.Join(SupportedInstanceClasses(), ", ")) + "DBInstanceClass %q is not supported; supported classes are %s", instanceClass, strings.Join(rdsengine.SupportedInstanceClasses(), ", ")) } storage, err := resolveRestoreStorage(input, snapshot) @@ -280,7 +281,7 @@ func resolveRestoreStorageType(input *rds.RestoreDBInstanceFromDBSnapshotInput, return storageType, nil } -func resolveRestorePort(input *rds.RestoreDBInstanceFromDBSnapshotInput, snapshot *DBSnapshotRecord, engine Engine) (int64, error) { +func resolveRestorePort(input *rds.RestoreDBInstanceFromDBSnapshotInput, snapshot *DBSnapshotRecord, engine rdsengine.Engine) (int64, error) { if input.Port != nil { port := aws.Int64Value(input.Port) if port < minDBPort || port > maxDBPort { diff --git a/spinifex/handlers/rds/service.go b/spinifex/handlers/rds/service.go index 5fe22c527..2f9dc55f4 100644 --- a/spinifex/handlers/rds/service.go +++ b/spinifex/handlers/rds/service.go @@ -11,6 +11,7 @@ import ( "sync" "time" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/mulgadc/spinifex/spinifex/foundation/state/kvstore" "github.com/nats-io/nats.go" "github.com/nats-io/nats.go/jetstream" @@ -82,6 +83,9 @@ type Service struct { nc *nats.Conn region string deps Deps + // The class-to-memory projection size-derived parameter defaults are + // computed from. Built once at construction; never mutated. + sizing rdsengine.Sizing // Heartbeat state that never reaches KV: beats are counted here and // persisted only on change or on the slower floor. @@ -105,7 +109,7 @@ type agentLiveness struct { // NewService returns a Service with no Deps; region scopes the ARNs the Service mints. func NewService(nc *nats.Conn, region string) *Service { - return &Service{nc: nc, region: region, liveness: make(map[string]*agentLiveness)} + return &Service{nc: nc, region: region, liveness: make(map[string]*agentLiveness), sizing: InstanceSizing()} } func (s *Service) WithDeps(d Deps) *Service { diff --git a/spinifex/handlers/rds/sizing.go b/spinifex/handlers/rds/sizing.go index c73f3041b..2abd854a7 100644 --- a/spinifex/handlers/rds/sizing.go +++ b/spinifex/handlers/rds/sizing.go @@ -1,58 +1,24 @@ package handlers_rds import ( - "errors" - "maps" - "slices" - "github.com/mulgadc/spinifex/spinifex/domains/ec2/instancetypes" - "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" ) -// db.* is a naming facade, not a second sizing table: every entry resolves -// through the platform's instance-type definitions. -var dbInstanceClasses = map[string]string{ - "db.t3.micro": "t3.micro", - "db.t3.small": "t3.small", - "db.t3.medium": "t3.medium", - "db.t3.large": "t3.large", - "db.m5.large": "m5.large", - "db.m5.xlarge": "m5.xlarge", +// ec2InstanceTypes adapts the platform's instance-type table to the engine +// package's narrow InstanceTypes seam, keeping that package free of an EC2 +// import. +type ec2InstanceTypes struct{} + +var _ rdsengine.InstanceTypes = ec2InstanceTypes{} + +func (ec2InstanceTypes) MemoryMiB(instanceType string) (int64, bool) { + return instancetypes.DefaultMemoryMiB(instanceType) } -// InstanceTypeForClass maps a db.* instance class to the EC2 instance type backing it. An unknown class -// is rejected here at validation rather than surfacing as a launch failure after the volume and ENI exist. -func InstanceTypeForClass(class string) (string, error) { - instanceType, ok := dbInstanceClasses[class] - if !ok { - return "", errors.New(awserrors.ErrorInvalidParameterValue) - } - if _, known := instancetypes.DefaultVCPUs(instanceType); !known { - return "", errors.New(awserrors.ErrorInvalidParameterValue) - } - return instanceType, nil -} - -// SupportedInstanceClasses returns every db.* instance class this platform offers, sorted alphabetically. -func SupportedInstanceClasses() []string { - return slices.Sorted(maps.Keys(dbInstanceClasses)) -} - -// SmallestInstanceClass returns the supported class with the least memory. Deliberately computed from -// the footprints rather than taken as the first name SupportedInstanceClasses reports: that list is -// sorted alphabetically, so its head is db.m5.large. -func SmallestInstanceClass() string { - smallest, least := "", int64(0) - for _, class := range SupportedInstanceClasses() { - memoryMiB, err := classMemoryMiB(class) - // A class with no known footprint cannot be compared; the catalog's - // self-consistency test pins that every supported class has one. - if err != nil { - continue - } - if smallest == "" || memoryMiB < least { - smallest, least = class, memoryMiB - } - } - return smallest +// InstanceSizing builds the class-to-memory projection every size-derived +// parameter default is computed from, backed by this platform's own +// instance-type table. +func InstanceSizing() rdsengine.Sizing { + return rdsengine.NewSizing(ec2InstanceTypes{}) } diff --git a/spinifex/handlers/rds/snapshot.go b/spinifex/handlers/rds/snapshot.go index 02eda5ba9..e796982a9 100644 --- a/spinifex/handlers/rds/snapshot.go +++ b/spinifex/handlers/rds/snapshot.go @@ -13,6 +13,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/rds" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" "github.com/mulgadc/spinifex/spinifex/foundation/aws/tags" "github.com/mulgadc/spinifex/spinifex/foundation/state/kvstore" @@ -190,7 +191,7 @@ const crashConsistentSnapshotWarning = "The database engine could not be quiesce "the snapshot is crash consistent." func crashConsistentSnapshotMessage(ctx context.Context, engineName string) string { - engine, err := LookupEngine(engineName) + engine, err := rdsengine.LookupEngine(engineName) if err != nil { // The snapshot has already been taken, so the customer still gets the half // of the warning that does not depend on knowing the engine. @@ -198,7 +199,7 @@ func crashConsistentSnapshotMessage(ctx context.Context, engineName string) stri "engine", engineName, "err", err) return crashConsistentSnapshotWarning } - return crashConsistentSnapshotWarning + " " + engine.crashRecoveryNote + return crashConsistentSnapshotWarning + " " + engine.CrashRecoveryNote() } // Releases the quiesce on a context detached from the caller's, so a snapshot diff --git a/spinifex/handlers/rds/tlsenforcement_test.go b/spinifex/handlers/rds/tlsenforcement_test.go index 99db0249a..3ab6450fa 100644 --- a/spinifex/handlers/rds/tlsenforcement_test.go +++ b/spinifex/handlers/rds/tlsenforcement_test.go @@ -12,6 +12,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/rds" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -71,7 +72,7 @@ func TestTLSAvailable_ReportsWhetherTheDeploymentCanServeTLS(t *testing.T) { // of its own requires TLS, on both engines, with no customer action. func TestCreateDBInstance_EnforcesTLSWithNoParameterGroupOfItsOwn(t *testing.T) { tests := []struct { - engine Engine + engine rdsengine.Engine parameter string }{ {enginePostgres, "rds.force_ssl"}, @@ -115,7 +116,7 @@ func TestCreateDBInstance_RefusesEnforcementWithNoClusterCA(t *testing.T) { // a modifiable parameter rather than a platform constant. func TestCreateDBInstance_AcceptsEnforcementTurnedOffWithNoClusterCA(t *testing.T) { tests := []struct { - engine Engine + engine rdsengine.Engine group string input *rds.CreateDBParameterGroupInput parameter string diff --git a/spinifex/handlers/rds/validate.go b/spinifex/handlers/rds/validate.go index 4381988cf..fc5a9f805 100644 --- a/spinifex/handlers/rds/validate.go +++ b/spinifex/handlers/rds/validate.go @@ -5,6 +5,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/rds" + rdsengine "github.com/mulgadc/spinifex/spinifex/domains/rds/engine" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" ) @@ -32,7 +33,7 @@ const ( // unimplemented parameter already rejected. type validatedCreate struct { Identifier string - Engine Engine + Engine rdsengine.Engine EngineVersion string InstanceClass string InstanceType string @@ -79,7 +80,7 @@ func (s *Service) validateCreateRequest(input *rds.CreateDBInstanceInput) (*vali return nil, err } - engine, err := LookupEngine(aws.StringValue(input.Engine)) + engine, err := rdsengine.LookupEngine(aws.StringValue(input.Engine)) if err != nil { return nil, err } @@ -88,10 +89,10 @@ func (s *Service) validateCreateRequest(input *rds.CreateDBInstanceInput) (*vali } instanceClass := aws.StringValue(input.DBInstanceClass) - instanceType, err := InstanceTypeForClass(instanceClass) + instanceType, err := s.sizing.InstanceTypeForClass(instanceClass) if err != nil { return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, - "DBInstanceClass %q is not supported; supported classes are %s", instanceClass, strings.Join(SupportedInstanceClasses(), ", ")) + "DBInstanceClass %q is not supported; supported classes are %s", instanceClass, strings.Join(rdsengine.SupportedInstanceClasses(), ", ")) } storage := aws.Int64Value(input.AllocatedStorage) @@ -269,3 +270,45 @@ func rejectUnimplemented(input *rds.CreateDBInstanceInput) error { func unimplemented(parameter, why string) error { return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s is not supported: %s", parameter, why) } + +// ValidateMasterUserPassword enforces the bounds and printable-ASCII range AWS accepts. The password is +// never inspected beyond this and never stored in cleartext past the first bootstrap fetch. +func ValidateMasterUserPassword(password string) error { + switch { + case password == "": + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "MasterUserPassword is required") + case len(password) < minMasterPasswordLen || len(password) > maxMasterPasswordLen: + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "MasterUserPassword must be between %d and %d characters", minMasterPasswordLen, maxMasterPasswordLen) + } + for _, r := range password { + // A control character would also survive the bootstrap handoff and defeat + // the line-oriented redaction that keeps the password off the guest + // console, so the range is refused here rather than sanitised there. + if r < 0x20 || r > 0x7e { + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "MasterUserPassword may only contain printable ASCII characters") + } + if r == '/' || r == '"' || r == '@' || r == ' ' { + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "MasterUserPassword may not contain '/', '\"', '@' or spaces") + } + } + return nil +} + +const ( + minMasterPasswordLen = 8 + maxMasterPasswordLen = 128 +) + +// isLetter and isDigit are duplicated against the engine package's own copy +// deliberately: that one backs identifier checks this package does not own, +// and neither may reach across the boundary to the other. +func isLetter(r rune) bool { + return (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') +} + +func isDigit(r rune) bool { + return r >= '0' && r <= '9' +} diff --git a/spinifex/handlers/rds/validate_test.go b/spinifex/handlers/rds/validate_test.go new file mode 100644 index 000000000..86142e4f8 --- /dev/null +++ b/spinifex/handlers/rds/validate_test.go @@ -0,0 +1,34 @@ +package handlers_rds + +import ( + "strings" + "testing" + + "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidateMasterUserPassword(t *testing.T) { + t.Parallel() + assert.NoError(t, ValidateMasterUserPassword("Sup3rSecret!")) + assert.NoError(t, ValidateMasterUserPassword(strings.Repeat("x", maxMasterPasswordLen))) + + for _, password := range []string{ + "", + strings.Repeat("x", minMasterPasswordLen-1), + strings.Repeat("x", maxMasterPasswordLen+1), + // The characters AWS excludes, which would otherwise break a connection + // string or the engine's own role syntax. + "pass/word1", `pass"word1`, "pass@word1", "pass word1", + // Outside printable ASCII. A newline survives the bootstrap handoff and + // defeats the guest's line-oriented redaction, putting the password on + // the host serial console; the rest are refused as the same class. + "pass\nword1", "pass\rword1", "pass\tword1", "pass\x00word1", "pass\x7fword1", + "pässword1", "passwordé", "pass word1", + } { + err := ValidateMasterUserPassword(password) + require.Error(t, err, "password %q should be rejected", password) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + } +}