diff --git a/docs/invariants.md b/docs/invariants.md index d79d34e14..2ea28c598 100644 --- a/docs/invariants.md +++ b/docs/invariants.md @@ -317,9 +317,11 @@ Every convergence of SchemaBot's own storage — at startup, or on an operator's unless destroying storage state was explicitly permitted, and decides before it writes. Additivity is the whole rule: a statement that loses data and one that removes a schema object without losing any are both refusals, because the exposure is the fleet reading that storage, not the rows alone. -Nothing about which surface asked changes that: an operator's command runs the -bootstrap rather than a second implementation of it, converges the schema of the binary running it, -and cannot narrow the permission a deployment already granted. On MySQL such a statement is refused +Nothing about which surface asked changes that: an operator's command runs the bootstrap rather than +a second implementation of it, and cannot narrow the permission a deployment already granted. Which +schema a convergence runs is the operator's to name, so that the storage a release needs can be in +place before the first pod of it starts; naming one supplies the files the bootstrap computes from +and nothing else, and every gate above applies to it unchanged. On MySQL such a statement is refused unless destructive storage changes are explicitly allowed, and the verdict is the one the plan already carries, read rather than re-derived, so the boot refuses exactly what the operator-facing plan flagged. The verdict is per statement and the differ emits one combined statement per table, so @@ -335,7 +337,12 @@ the unsafe verdict the engine's plan reports (`pkg/engine/spirit/spirit.go`) and partition that reduces a flagged statement to its additions (`pkg/ddl/additive.go`), which the operator-facing storage schema surface reads rather than reimplements (`pkg/api/storage_schema.go`); the instance's own storage is the only target a remote caller can address, and the deployment's -permission is only ever widened, in the adapter that answers for it (`pkg/serve/storage_schema.go`). +permission is only ever widened, in the adapter that answers for it (`pkg/serve/storage_schema.go`); +and a named schema is refused apart from the files it names on every transport that carries one +(`pkg/api/storage_schema_handlers.go`, `pkg/serve/storage_schema.go`), is not converged at all +until the target has confirmed it would use it (`pkg/api/storage_schema_handlers.go`), and is +converged by the CLI only behind a confirmation it refuses to skip +(`pkg/cmd/commands/storage_schema.go`). ### AV-10: Anything the PR can do, the CLI can do diff --git a/docs/release.md b/docs/release.md index 99313840c..0addcee19 100644 --- a/docs/release.md +++ b/docs/release.md @@ -284,9 +284,11 @@ schemabot storage plan --deployment west -e production --release v1.4.0 `storage plan` requires the release to be named — `--release`, or `--schema-dir` pointed at a checkout — so there is no way to get an answer about a release you -did not choose. `storage apply` is the command that runs the schema embedded in -the binary running it, and it takes no selector at all. Operators pre-creating an -index ahead of the roll should also read [Deploying a release that changes the +did not choose. `storage apply` takes the same selectors and converges what they +name, which is how an operator readies the storage before the release that needs +it rolls; naming one is confirmed at a terminal and refused with +`--auto-approve`. Operators pre-creating an index ahead of the roll should also +read [Deploying a release that changes the storage schema](./storage-schema.md#deploying-a-release-that-changes-the-storage-schema) — on MySQL an index drop is refused like any other destructive statement, but diff --git a/docs/storage-schema.md b/docs/storage-schema.md index d6989546d..26d61b7b7 100644 --- a/docs/storage-schema.md +++ b/docs/storage-schema.md @@ -11,6 +11,7 @@ - [Name the release, and the storage database](#name-the-release-and-the-storage-database) - [Converge it](#converge-it) - [While it runs](#while-it-runs) +- [Converging a release before it rolls](#converging-a-release-before-it-rolls) - [Deploying a release that changes the storage schema](#deploying-a-release-that-changes-the-storage-schema) - [When a pod will not start](#when-a-pod-will-not-start) - [Pre-creating indexes on a long-lived database](#pre-creating-indexes-on-a-long-lived-database) @@ -59,8 +60,8 @@ the startup path the CLI path └────────────────┬─────────────────┘ ▼ the storage database, converged to the - schema of the binary that ran it, and to - nothing else + schema the run was given: the running + binary's own, or a release an operator named ``` Four consequences worth holding onto: @@ -68,9 +69,13 @@ Four consequences worth holding onto: - **`storage apply` is what a boot does**, not an equivalent of it: the same differ, the same refusal, the same lock. Converging ahead of a roll cannot disagree with what the roll then does, and concurrent runs serialize. -- **A binary converges its own schema, never another's.** Neither the command - line nor the RPC behind it can hand it another release's files, which stops an - older binary from treating a newer release's tables as state to prune. +- **Which schema it converges is yours to name.** With no selector it runs the + files the answering binary carries, which is what that binary's own next boot + would run. With `--release` or `--schema-dir` it runs that release's files + instead, so the storage a release needs can be in place before its first pod + starts. Naming a release supplies the files and nothing else: every refusal + that guards a boot guards this too, so an older binary handed a newer + release's files still cannot prune the newer tables. - **Convergence is bounded, and the budget follows the path, not the code.** A boot gets five minutes, covering the lock wait and the DDL, because a pod converging is a pod not yet serving, and one holding the lock keeps the others @@ -235,23 +240,28 @@ that only reads the exit status waits for a convergence that is never coming. ## Name the release, and the storage database The live side of a plan is always a read of the database. The desired side is -one release's schema *files*. One selector is required, and naming both is -refused rather than resolved by precedence: +one release's schema *files*, named the same way on both commands: `storage +plan` requires a selector, and `storage apply` converges the answering binary's +own schema when given none. Naming both selectors is refused rather than +resolved by precedence: - **`--release `** fetches that tag's `pkg/schema//` files over the repository's contents API, for the dialect the live storage runs. `--release-repo` points at a fork or mirror (`block/schemabot` by default), `GITHUB_API_URL` at a different API host, and `GITHUB_TOKEN` or `GH_TOKEN` - authorizes the fetch. + authorizes the fetch. Where `GITHUB_API_URL` names a plaintext host, a plan + warns and a convergence is refused: anything on that path can rewrite the + files before they arrive, and a convergence runs whatever arrives. A host on + this machine is exempt, having no network path. - **`--schema-dir `** reads `*.sql` from a checkout or an extracted image layer, needs no network, and is how you ask about a commit that was never tagged. Point it at the dialect directory (`pkg/schema/mysql`), not its parent. -There is no default because the same storage is converged against the release +A plan has no default because the same storage is converged against the release that is running and short of the release about to roll, and an operator who -assumed the wrong one either rolls into a failing bootstrap or converges storage -they did not mean to touch. `storage apply` takes neither selector, because a -convergence runs the schema of the binary running it. +assumed the wrong one either misreads the report or converges storage they did +not mean to touch. What the answering binary carries is not a selector either: +through the API, which release that is depends on which pod took the call. Which storage database either command reads is stated rather than discovered: @@ -278,9 +288,12 @@ access](auth.md#what-read-and-write-access-include)). `storage apply` runs the bootstrap described above, so two operators running it at once serialize the way two booting pods do. It prints the plan, asks for a -literal `yes`, and reports what ran, naming the schema embedded in the binary -that ran it; `--auto-approve` (`-y`) skips the prompt for scripted maintenance. -A convergence that leaves statements behind prints them as a plan underneath. +literal `yes`, and reports what ran, naming the schema it converged: the +answering binary's own, or the release the command named. `--auto-approve` +(`-y`) skips the prompt for scripted maintenance, and is refused when a release +is named, for the reason in [Converging a release before it +rolls](#converging-a-release-before-it-rolls). A convergence that leaves +statements behind prints them as a plan underneath. What it does not share with a boot is the budget. A boot gives up after five minutes, because a converging pod is not yet serving; this runs under an hour, @@ -347,7 +360,10 @@ not, so a maintenance script keyed on the status gets them right: Both non-zero refusals mean the same thing: nothing converged, and a person has to decide something before anything does. A script keyed on the status can -therefore treat them alike and re-read the plan for which one it hit. +therefore treat them alike and re-read the plan for which one it hit. Under +`--json` a refusal comes back in the shape a convergence does, so nothing has to +be re-read to learn which statements were refused: nothing ran, so the planned +and the remaining halves of the report are the same. The one place a refused destructive statement is *not* a failure is a startup bootstrap, which skips it and converges the safe remainder (AV-9). That @@ -473,6 +489,88 @@ The line only ever appears when a convergence is detected. Its absence is not a statement that the database is idle: the same read answers "nothing running" and "could not tell", so a plan never claims the second as the first. +## Converging a release before it rolls + +Name the release and `storage apply` converges *its* files, from whatever CLI +you have to hand: + +```bash +schemabot storage apply --release v1.4.0 --dsn "$STORAGE_DSN" +``` + +This is the command's reason for existing: the storage a release needs has to be +there before the first pod of that release starts, and the pod that would +otherwise converge it is the one that cannot start until it is. + +It is confirmed at a terminal, and `--auto-approve` is refused with it. Naming a +release substitutes files the answering binary never carried, so until that +release is deployed the fleet boots against storage holding more than it +declares, and the confirmation's notice says what that costs for this +deployment. On MySQL every one of those boots refuses the removals and logs +them, so the pre-applied state survives and the fleet warns about it until the +deploy; a release from before indexes were protected is the exception ([What is +never automatic](#what-is-never-automatic)). The case to watch is a MySQL +deployment that has set +[`allow_destructive_schema_changes`](configuration.md#allow_destructive_schema_changes): +its boots converge destructively, so what you pre-apply is dropped by the next +pod to start, and this belongs in the deploy rather than ahead of it. On +PostgreSQL the boots never compute a removal at all, whatever that deployment +has configured, so the state survives unremarked. Converge close to the deploy, +re-run `storage plan` just before it, and leave unattended runs to a job with no +selector, which converges the answering binary's own schema. + +What the notice reports is the deployment's own standing policy, never the flags +on your command. Passing `--allow-unsafe` widens what your convergence may run +and moves nothing about the pods around you: their boots read the deployment's +config and have never heard of your request, so state they would have refused to +drop they still refuse to drop. + +Sometimes there is no policy to report, and the notice says so rather than +picking the reassuring half of it. A `--dsn` target has no deployment config +behind it, and a target answering from a release older than the field reports +nothing for it either. The deployment behind either may be of either kind, so +confirm which before treating pre-applied state as safe until the deploy — check +that deployment's +[`allow_destructive_schema_changes`](configuration.md#allow_destructive_schema_changes), +or converge as part of the deploy instead. PostgreSQL needs no such check: its +bootstrap is additive-only on every release, so the notice answers from the +dialect. + +A target too old to accept a named schema is refused rather than answered, and +the two hops the request crosses refuse it differently. + +The server is what compares. `--release` and `--schema-dir` reach a data plane +over gRPC, where a release that predates the fields drops them and works from +its own embedded schema — silently, since that is what proto does with a field +it does not know. The report says which schema it used, so the server checks +that against the one you asked for. A convergence is checked *before* it runs, +with a diff, because DDL cannot be taken back; it is checked again on the answer, +because a deployment is many pods and a roll makes them different releases. +Either refusal is a 502 naming the schema the target actually used. + +The CLI-to-server hop refuses earlier and says something else. That body is +strictly decoded, so a SchemaBot server predating these fields rejects the +request outright with a 400 about an unknown field, rather than ignoring them. +The hop fails closed either way; the error just does not mention schemas. + +Upgrade that target, or address a release that carries the schema you are +converging. + +The convergence gets no more permission than a boot: a statement that would drop +a storage table or column is refused as it is at startup, and a PostgreSQL +column shape the convergence will not run still stops the whole set. + +What that does **not** cover is a `--schema-dir` missing files, and the two +dialects fail it in opposite directions. On MySQL an omitted table reads as +surplus, so the plan is full of drops and the convergence refuses them: the cost +is a report that overstates what is surplus, and a table is only lost if someone +then permits it. On PostgreSQL the convergence is additive-only and walks only +the files it was given, so an omitted table is not reported at all — a set +missing most of a release's tables converges cleanly and reports converged, for +storage that is not ready. Point `--schema-dir` at a release's whole +`pkg/schema/` directory rather than at a hand-assembled subset; +`--release` cannot get this wrong, because it fetches the directory whole. + ## Deploying a release that changes the storage schema Every startup converges the storage schema on its own, so the routine case needs @@ -491,20 +589,23 @@ starting. 2. **Decide whether the boot should do it.** Additive DDL inside the five-minute budget is fine when the tables are small, and not when they are not: on MySQL an index added to an existing storage table copies the table, and every pod in - the roll pays it. Converge once, ahead of the roll, from a binary of the new - release, or that release's image as a one-shot job. + the roll pays it. Converge once, ahead of the roll, naming the release you are + about to roll: the files come from the tag, so any binary you have to hand + converges it. ```bash - schemabot storage apply --dsn "$STORAGE_DSN" # from the new release's binary + schemabot storage apply --release v1.4.0 --dsn "$STORAGE_DSN" ``` - This takes the work out of the roll *and* out of the boot's budget: `apply` - is the same bootstrap running under an hour rather than five minutes, so a - build a boot could not have finished is exactly what this step is for. - Expect the bootstrap lock to be held throughout — pods booting in that - window will not come up, which is why this belongs ahead of a roll rather - than during one. A build too slow even for the hour has to be created by - hand instead; see [Pre-creating indexes on a long-lived + It prompts, with the notice from [Converging a release before it + rolls](#converging-a-release-before-it-rolls), and `--auto-approve` is + refused here. This takes the work out of the roll *and* out of the boot's + budget: `apply` is the same bootstrap running under an hour rather than five + minutes, so a build a boot could not have finished is exactly what this step + is for. Expect the bootstrap lock to be held throughout — pods booting in + that window will not come up, which is why this belongs ahead of a roll + rather than during one. A build too slow even for the hour has to be created + by hand instead; see [Pre-creating indexes on a long-lived database](#pre-creating-indexes-on-a-long-lived-database). 3. **On MySQL, if you converged ahead of the roll, re-check right before it.** A @@ -541,10 +642,10 @@ schemabot storage plan --release v1.4.0 --dsn "$STORAGE_DSN" The statements it prints are the ones the pod is failing on. With no network to the repository, `--schema-dir` reads the same files from a checkout of that -release; with neither, `storage apply --dsn "$STORAGE_DSN"` prints what the -binary in your hand would run and waits for a `yes`. It clears the statements -under the same advisory lock the pods are contending for, and the pod starts on -its next backoff. Two failures look similar in the logs and are not: +release, on either command: swapping `plan` for `apply` prints them and waits +for a `yes`. It clears the statements under the same advisory lock the pods are +contending for, and the pod starts on its next backoff. Two failures look +similar in the logs and are not: - **The convergence is refusing something.** The plan names it: a destructive statement, or a PostgreSQL column shape needing manual remediation. Nothing diff --git a/pkg/api/ensure_schema.go b/pkg/api/ensure_schema.go index fb800ce03..98622cfa7 100644 --- a/pkg/api/ensure_schema.go +++ b/pkg/api/ensure_schema.go @@ -63,14 +63,73 @@ const MinConvergenceTimeout = time.Second // EnsureSchemaOption customizes EnsureSchema behavior. type EnsureSchemaOption func(*ensureSchemaOptions) +// DeploymentDestructivePolicy is a deployment's standing decision about +// destructive storage schema statements — the one every boot of it converges +// under, which no per-request opt-in moves. +type DeploymentDestructivePolicy int + +const ( + // DestructivePolicyUnknown is a caller that cannot read the deployment's + // decision, which is a convergence addressed by DSN alone: there is a + // deployment behind that database and no config here that names its + // policy. It permits nothing, so it never widens what runs, and it is + // reported as unknown rather than as a refusal, because the two differ + // exactly where an operator is deciding whether to pre-apply. + DestructivePolicyUnknown DeploymentDestructivePolicy = iota + // DestructivePolicyForbids is a deployment whose boots refuse to remove + // storage state they do not declare. It is the default a config carries. + DestructivePolicyForbids + // DestructivePolicyPermits is a deployment that configured + // allow_destructive_schema_changes, whose boots run the removals. + DestructivePolicyPermits +) + +// String names the policy for a log line, where the underlying integer would +// leave an operator counting enum members. +func (p DeploymentDestructivePolicy) String() string { + switch p { + case DestructivePolicyPermits: + return "permits" + case DestructivePolicyForbids: + return "forbids" + case DestructivePolicyUnknown: + return "unknown" + } + return "unknown" +} + +// ConfiguredDestructivePolicy is the standing policy a deployment's config +// states, for a caller that has read one. A caller holding only a DSN has not, +// and passes DestructivePolicyUnknown instead of the false this would give it. +func ConfiguredDestructivePolicy(allowDestructive bool) DeploymentDestructivePolicy { + if allowDestructive { + return DestructivePolicyPermits + } + return DestructivePolicyForbids +} + type ensureSchemaOptions struct { + // allowDestructive is the effective policy for this convergence: the + // deployment's standing one widened by a caller's opt-in. It decides what + // this run does. allowDestructive bool - dialect schema.Dialect + // deploymentDestructive is the standing policy alone, which is what every + // boot of this deployment converges under. It decides nothing here and is + // reported, not enforced: it is how a report can say what happens to state + // this convergence leaves behind, once this command is over and the only + // thing still converging is a pod starting. + deploymentDestructive DeploymentDestructivePolicy + dialect schema.Dialect // convergenceTimeout bounds one whole convergence: the lock wait, the // diff under it, and the DDL. It defaults to EnsureSchemaTimeout, the // budget a boot needs, so a caller that never considered the question // converges the way a pod does. convergenceTimeout time.Duration + // schemaSource is the schema this convergence brings the storage database + // to. Nil is the binary's own embedded files, which is what a boot + // converges to, so the startup path never sets it and every existing call + // site keeps its behavior by not setting it either. + schemaSource *StorageSchemaSource // postgresStatementTimeout bounds a single ordinary query on the // PostgreSQL bootstrap's connection. Zero disables the budget explicitly; // negative means "not set", leaving the platform's ambient value in place. @@ -84,23 +143,64 @@ type ensureSchemaOptions struct { progress func(StorageConvergenceProgress) } -// WithAllowDestructiveSchemaChanges controls whether EnsureSchema may execute +// WithDestructiveSchemaChangePolicy controls whether EnsureSchema may execute // destructive DDL against the storage database — any statement the plan's own // linters report an error against, which for the storage schema means one that // loses data (DROP TABLE, or an ALTER TABLE containing DROP COLUMN) and one -// that removes an index. It defaults to false: those statements are refused -// while the rest of the diff still applies. A mixed ALTER carrying an additive -// clause beside a drop runs the addition and withholds the drop, so a pod never -// starts missing a column its own binary needs. +// that removes an index. Both arguments default to false: those statements are +// refused while the rest of the diff still applies. A mixed ALTER carrying an +// additive clause beside a drop runs the addition and withholds the drop, so a +// pod never starts missing a column its own binary needs. // // This is the only way to have the bootstrap execute one of those statements, // so removing a table, column, or index from the embedded schema on purpose -// means setting this flag or running the DDL by hand. That is the -// intended trade: a surplus index left in place costs write throughput, while -// one dropped out from under the fleet's live queries costs availability. -// Wire this from StorageConfig.AllowDestructiveSchemaChanges. -func WithAllowDestructiveSchemaChanges(allow bool) EnsureSchemaOption { - return func(o *ensureSchemaOptions) { o.allowDestructive = allow } +// means permitting it here or running the DDL by hand. That is the intended +// trade: a surplus index left in place costs write throughput, while one +// dropped out from under the fleet's live queries costs availability. +// +// The two arguments are different facts and the call site has to supply both, +// which is the reason this takes two rather than the single flag a convergence +// needs. deployment is the standing policy — wire it through +// ConfiguredDestructivePolicy from StorageConfig.AllowDestructiveSchemaChanges, +// or pass DestructivePolicyUnknown where there is no config to read — and +// request is one caller's explicit opt-in, which widens that policy for this +// run and never narrows it. What this convergence runs under is the standing +// permission or the request. deployment alone is what the next pod to boot +// runs under, which is a question a report has to be able to answer and cannot +// once the two have been merged into one flag. A boot supplies its own config +// and false: nobody is asking it for anything. +// +// They are also different types, so the compiler rejects the transposition. +// Two bools here would swap silently and leave every gate intact — a gate +// reads whether either permits, which does not depend on which is which — +// while the report went on to describe the caller's own flag as the fleet's +// policy, which is the one mistake this pair exists to make impossible. +func WithDestructiveSchemaChangePolicy(deployment DeploymentDestructivePolicy, request bool) EnsureSchemaOption { + return func(o *ensureSchemaOptions) { + o.deploymentDestructive = deployment + o.allowDestructive = deployment == DestructivePolicyPermits || request + } +} + +// WithStorageSchema converges the storage database to a schema other than the +// binary's own embedded files — the schema of a release an operator is about to +// roll, so the storage is ready before the first pod of it starts. +// +// Unset, the convergence runs the embedded files, which is what a boot does and +// what every startup call site wants. Set, the convergence runs the supplied +// files instead, under the same differ, the same destructive-change refusal and +// the same advisory lock: the schema moves, the policy does not (AV-9). Nothing +// here checks that the files are a *complete* schema, because a file set cannot +// say what is missing from it — an incomplete one reports the storage's own +// tables as surplus, and what keeps that from destroying them is the same +// refusal that guards a boot. +// +// The option carries no version and never resolves one. A caller supplying +// files says in words where they came from (see StorageSchemaSource), because +// only the caller knows, and a convergence attributed to a release whose files +// it did not run is the failure this whole surface exists to prevent. +func WithStorageSchema(source *StorageSchemaSource) EnsureSchemaOption { + return func(o *ensureSchemaOptions) { o.schemaSource = source } } // WithDialect selects the database family of the storage database so @@ -275,7 +375,7 @@ func ensureSchema(parent context.Context, dsn string, logger *slog.Logger, opts // Destructive statements in the diff — those the plan's linters report an error // against, which for the storage schema means losing data (DROP TABLE, or an // ALTER TABLE containing DROP COLUMN) or removing an index — are refused unless -// WithAllowDestructiveSchemaChanges(true) is set. A statement carrying an +// WithDestructiveSchemaChangePolicy permits them. A statement carrying an // addition beside a drop runs the addition, so a pod never starts missing a // column its own binary needs. The statements and clauses that were not refused // apply, and startup proceeds — a deliberate exception to fail-closed, because failing @@ -304,11 +404,14 @@ func ensureMySQLSchema(parent context.Context, dsn string, logger *slog.Logger, ) } - schemaFiles, err := readEmbeddedSchemaFiles() + // Nil is the embedded files, so a boot reads its own schema through the + // same call an operator converging a named release reads theirs. + schemaFiles, err := o.schemaSource.mysqlSchemaFiles() if err != nil { return err } - logger.Info("loaded embedded storage schema files", + logger.Info("loaded storage schema files", + "schema_source", o.schemaSource.Describe(), "namespace_count", len(schemaFiles), "file_count", countSchemaFiles(schemaFiles), "files", schemaFileNames(schemaFiles), diff --git a/pkg/api/ensure_schema_integration_test.go b/pkg/api/ensure_schema_integration_test.go index 17a8a416f..b3a62fc64 100644 --- a/pkg/api/ensure_schema_integration_test.go +++ b/pkg/api/ensure_schema_integration_test.go @@ -274,12 +274,12 @@ func TestEnsureSchema_RemovesObsoleteVitessTasks(t *testing.T) { require.True(t, testutil.TableExists(t, db, sdb.Name, "vitess_tasks")) // EnsureSchema reconciles the obsolete table away without error... - require.NoError(t, EnsureSchema(dsn, logger, WithAllowDestructiveSchemaChanges(true)), + require.NoError(t, EnsureSchema(dsn, logger, WithDestructiveSchemaChangePolicy(DestructivePolicyPermits, false)), "EnsureSchema with an obsolete vitess_tasks table failed") assert.False(t, testutil.TableExists(t, db, sdb.Name, "vitess_tasks"), "obsolete vitess_tasks should be removed") // ...and the next run is a clean no-op. - require.NoError(t, EnsureSchema(dsn, logger, WithAllowDestructiveSchemaChanges(true)), + require.NoError(t, EnsureSchema(dsn, logger, WithDestructiveSchemaChangePolicy(DestructivePolicyPermits, false)), "second EnsureSchema not idempotent") } @@ -653,13 +653,13 @@ func TestEnsureSchema_AllowDestructiveExecutesIndexDrop(t *testing.T) { seedSurplusIndex(t, db) require.Equal(t, surplusIndexColumns, testutil.IndexColumns(t, db, sdb.Name, "tasks", surplusIndexName)) - require.NoError(t, EnsureSchema(dsn, logger, WithAllowDestructiveSchemaChanges(true)), + require.NoError(t, EnsureSchema(dsn, logger, WithDestructiveSchemaChangePolicy(DestructivePolicyPermits, false)), "EnsureSchema with destructive changes allowed failed") assert.Empty(t, testutil.IndexColumns(t, db, sdb.Name, "tasks", surplusIndexName), "surplus index should be dropped when destructive changes are allowed") - require.NoError(t, EnsureSchema(dsn, logger, WithAllowDestructiveSchemaChanges(true)), + require.NoError(t, EnsureSchema(dsn, logger, WithDestructiveSchemaChangePolicy(DestructivePolicyPermits, false)), "second EnsureSchema not idempotent") } @@ -675,7 +675,7 @@ func TestEnsureSchema_AllowDestructiveExecutesDrops(t *testing.T) { require.NoError(t, EnsureSchema(dsn, logger)) surplusColumn, surplusTable := seedSurplusStorageState(t, db) - require.NoError(t, EnsureSchema(dsn, logger, WithAllowDestructiveSchemaChanges(true)), + require.NoError(t, EnsureSchema(dsn, logger, WithDestructiveSchemaChangePolicy(DestructivePolicyPermits, false)), "EnsureSchema with destructive changes allowed failed") assert.False(t, testutil.ColumnExists(t, db, sdb.Name, "tasks", surplusColumn), @@ -683,7 +683,7 @@ func TestEnsureSchema_AllowDestructiveExecutesDrops(t *testing.T) { assert.False(t, testutil.TableExists(t, db, sdb.Name, surplusTable), "surplus table should be dropped when destructive changes are allowed") - require.NoError(t, EnsureSchema(dsn, logger, WithAllowDestructiveSchemaChanges(true)), + require.NoError(t, EnsureSchema(dsn, logger, WithDestructiveSchemaChangePolicy(DestructivePolicyPermits, false)), "second EnsureSchema not idempotent") } diff --git a/pkg/api/ensure_schema_postgres.go b/pkg/api/ensure_schema_postgres.go index 26245eb62..6aa181199 100644 --- a/pkg/api/ensure_schema_postgres.go +++ b/pkg/api/ensure_schema_postgres.go @@ -45,7 +45,9 @@ func ensurePostgresSchema(parent context.Context, dsn string, logger *slog.Logge ctx, cancel := context.WithTimeout(parent, o.convergenceTimeout) defer cancel() - tables, files, err := readEmbeddedPostgresSchemaFiles() + // Nil is the embedded files, so a boot reads its own schema through the + // same call an operator converging a named release reads theirs. + tables, files, err := o.schemaSource.postgresSchemaFiles() if err != nil { return err } @@ -74,7 +76,8 @@ func ensurePostgresSchema(parent context.Context, dsn string, logger *slog.Logge "dialect", schema.DialectPostgres, "database", database, "schema", schemaName, - "embedded_tables", len(tables), + "schema_source", o.schemaSource.Describe(), + "declared_tables", len(tables), ) } diff --git a/pkg/api/storage_schema.go b/pkg/api/storage_schema.go index 4dc4aa6af..5a2e8cf29 100644 --- a/pkg/api/storage_schema.go +++ b/pkg/api/storage_schema.go @@ -57,7 +57,7 @@ const StorageSchemaPlanTimeout = 30 * time.Second // closed for a dialect without one rather than running another family's // catalog queries. Pass the same options EnsureSchema is wired with so the // report describes what a boot would decide; -// WithAllowDestructiveSchemaChanges only labels the report here, since a diff +// WithDestructiveSchemaChangePolicy only labels the report here, since a diff // executes nothing either way. func PlanStorageSchema(ctx context.Context, dsn string, desired *StorageSchemaSource, logger *slog.Logger, opts ...EnsureSchemaOption) (*StorageSchemaReport, error) { // The budget is imposed here rather than left to the caller: a control @@ -124,10 +124,20 @@ func storageApplyOptions(opts []EnsureSchemaOption) []EnsureSchemaOption { // command that converged storage differently from a boot would be a second // implementation of the one path that must not have two. // -// It takes no schema source, and the absence is the safety property rather than -// an omission: a convergence runs the schema embedded in the binary running it, -// so "apply is what a boot does" holds by construction (AV-9). A caller that -// wants a later release's schema on a database runs that release's binary. +// desired is the schema to converge to; nil is the embedded schema of this +// binary, which is what a boot converges to. An operator rolling a later +// release supplies that release's files, so the storage is ready before the +// first pod of it starts — which is the whole reason this is reachable as a +// command and not only as a boot. +// +// Supplying files moves the schema and nothing else. It does not widen what +// runs: the destructive refusal and the manual-remediation gate decide the same +// way they decide for a boot, so a file set that would have to drop a storage +// table is refused here exactly as it would be at startup (AV-9). What it does +// move is the *reason* those gates matter, because a file set the caller +// assembled can be incomplete in a way a binary's own embedded set cannot, and +// an incomplete one reports the storage's own tables as surplus. That report is +// a set of destructive statements, which is the disposition that is refused. // // Two reports bracket the run, because "what happened" and "what is left" are // different questions and an operator mid-incident needs both: @@ -165,9 +175,15 @@ func storageApplyOptions(opts []EnsureSchemaOption) []EnsureSchemaOption { // terminal holding the DSN — so every entry point should get the operator's // ceiling without having to remember to ask for it. A caller passing // WithConvergenceTimeout still wins, since caller options are applied last. -func ApplyStorageSchema(ctx context.Context, dsn string, logger *slog.Logger, opts ...EnsureSchemaOption) (planned, remaining *StorageSchemaReport, err error) { +func ApplyStorageSchema(ctx context.Context, dsn string, desired *StorageSchemaSource, logger *slog.Logger, opts ...EnsureSchemaOption) (planned, remaining *StorageSchemaReport, err error) { opts = storageApplyOptions(opts) - planned, err = PlanStorageSchema(ctx, dsn, nil, logger, opts...) + // The three reads either side of the convergence and the convergence + // itself all resolve the desired schema from this one value, so the plan + // an operator approved, the files that run, and the report of what is left + // cannot describe three different schemas. + converge := append(append([]EnsureSchemaOption(nil), opts...), WithStorageSchema(desired)) + + planned, err = PlanStorageSchema(ctx, dsn, desired, logger, opts...) if err != nil { return nil, nil, fmt.Errorf("diff storage schema before converging it: %w", err) } @@ -196,16 +212,17 @@ func ApplyStorageSchema(ctx context.Context, dsn string, logger *slog.Logger, op logger.Info("converging storage schema on operator request", "dialect", planned.Dialect, "database", planned.Database, + "schema_source", planned.SchemaSource, "outstanding_count", len(planned.Outstanding), "destructive_count", len(planned.Destructive), "destructive_allowed", planned.DestructiveAllowed, "convergence_timeout", newEnsureSchemaOptions(opts...).convergenceTimeout, ) - if err := ensureSchema(ctx, dsn, logger, opts...); err != nil { - return planned, nil, fmt.Errorf("converge storage schema on database %q (%s): %w", planned.Database, planned.Dialect, err) + if err := ensureSchema(ctx, dsn, logger, converge...); err != nil { + return planned, nil, fmt.Errorf("converge storage schema on database %q (%s) to %s: %w", planned.Database, planned.Dialect, planned.SchemaSource, err) } - remaining, err = PlanStorageSchema(ctx, dsn, nil, logger, opts...) + remaining, err = PlanStorageSchema(ctx, dsn, desired, logger, opts...) if err != nil { // The convergence succeeded; only the confirming read failed. Report // that distinctly — an operator must not read a failed verification as @@ -235,6 +252,10 @@ func planMySQLStorageSchema(ctx context.Context, dsn string, desired *StorageSch Dialect: schema.DialectMySQL, SchemaSource: desired.Describe(), DestructiveAllowed: o.allowDestructive, + // A boot runs the standing policy and nothing this caller sent, so the + // two fields part company on exactly the deployment where an operator + // opted in to something their fleet has not. + BootRemovalPolicy: mysqlBootRemovalPolicy(o.deploymentDestructive), } // The database identity is what makes the report readable as being about @@ -313,6 +334,26 @@ func storageSchemaOperation(t ddl.StatementType) (string, error) { } } +// mysqlBootRemovalPolicy is what a MySQL boot of this deployment does to +// storage state its own schema does not declare. +// +// The MySQL bootstrap computes the removals and then decides, so the answer is +// the deployment's standing policy — and a caller who could not read that +// policy gets an unknown rather than the reassuring half of it. Every case is +// named, so a policy added later falls to the unknown rather than inheriting +// whichever answer a default happened to be. +func mysqlBootRemovalPolicy(deployment DeploymentDestructivePolicy) apitypes.BootRemovalPolicy { + switch deployment { + case DestructivePolicyPermits: + return apitypes.BootRemovalRemoves + case DestructivePolicyForbids: + return apitypes.BootRemovalPreserves + case DestructivePolicyUnknown: + return apitypes.BootRemovalUnknown + } + return apitypes.BootRemovalUnknown +} + // planPostgresStorageSchema diffs the desired PostgreSQL schema files against // the live storage database with the additive convergence's own drift scan, so // the report is exactly what ensurePostgresSchema would decide. The convergence @@ -320,7 +361,17 @@ func storageSchemaOperation(t ddl.StatementType) (string, error) { // set; what it does have is the manual-remediation set, whose entries abort a // whole convergence pass rather than being skipped. func planPostgresStorageSchema(ctx context.Context, dsn string, desired *StorageSchemaSource, o ensureSchemaOptions) (*StorageSchemaReport, error) { - report := &StorageSchemaReport{Dialect: schema.DialectPostgres, SchemaSource: desired.Describe()} + // DestructiveAllowed stays false and the boot policy is preservation, and + // neither reads the deployment's standing policy. An additive convergence + // computes no removal, so there is nothing for a policy to permit: a + // deployment that configured the allowance still boots pods that leave + // surplus storage state alone. This is also why the answer here is known + // even when the policy is not — the dialect settles it on its own. + report := &StorageSchemaReport{ + Dialect: schema.DialectPostgres, + SchemaSource: desired.Describe(), + BootRemovalPolicy: apitypes.BootRemovalPreserves, + } tables, files, err := desired.postgresSchemaFiles() if err != nil { diff --git a/pkg/api/storage_schema_handlers.go b/pkg/api/storage_schema_handlers.go index 1e0878707..e2c65569a 100644 --- a/pkg/api/storage_schema_handlers.go +++ b/pkg/api/storage_schema_handlers.go @@ -20,6 +20,7 @@ package api import ( + "context" "encoding/json" "errors" "fmt" @@ -152,7 +153,7 @@ func (s *Service) handleStorageSchemaPlan(w http.ResponseWriter, r *http.Request if !s.authorizeStorageSchemaOperation(w, r, storageSchemaPlanOperation) { return } - if err := validateStorageSchemaPlanRequest(req); err != nil { + if err := validateStorageSchemaSource(req.SchemaFiles, req.SchemaSource); err != nil { s.logger.Warn("rejecting storage schema plan because its desired schema is incomplete", "deployment", req.Deployment, "environment", req.Environment, "error", err) s.writeError(w, http.StatusBadRequest, err.Error()) @@ -189,6 +190,9 @@ func (s *Service) handleStorageSchemaPlan(w http.ResponseWriter, r *http.Request s.writeError(w, http.StatusInternalServerError, "storage schema plan returned no report") return } + if s.refuseUnhonoredSchema(w, target, storageSchemaPlanOperation, req.SchemaSource, report) { + return + } s.writeJSON(w, http.StatusOK, apitypes.StorageSchemaPlanResponse{Report: report}) } @@ -196,6 +200,12 @@ func (s *Service) handleStorageSchemaPlan(w http.ResponseWriter, r *http.Request // POST /api/storage/schema/apply. It runs the target instance's startup // bootstrap, under the advisory lock that bootstrap already takes, so two // operators running it at once serialize the same way two booting pods do. +// +// The schema it converges to is the target's own embedded files, or the ones +// the request carries — a release an operator is rolling, which the target +// cannot converge from files it does not have. Either way the bootstrap decides +// what runs, so a supplied schema changes which statements are computed and +// nothing about which of them are permitted (AV-9). func (s *Service) handleStorageSchemaApply(w http.ResponseWriter, r *http.Request) { req, err := decodeStorageSchemaApplyRequest(r) if err != nil { @@ -205,6 +215,12 @@ func (s *Service) handleStorageSchemaApply(w http.ResponseWriter, r *http.Reques if !s.authorizeStorageSchemaOperation(w, r, storageSchemaApplyOperation) { return } + if err := validateStorageSchemaSource(req.SchemaFiles, req.SchemaSource); err != nil { + s.logger.Warn("rejecting storage schema apply because the schema to converge to is incomplete", + "deployment", req.Deployment, "environment", req.Environment, "error", err) + s.writeError(w, http.StatusBadRequest, err.Error()) + return + } // A request naming no budget runs under the boot's. SchemaBot's own client // always names one, so this is a caller that could not — and the boot // budget is the one such a caller can wait out, where the operator default @@ -223,27 +239,42 @@ func (s *Service) handleStorageSchemaApply(w http.ResponseWriter, r *http.Reques return } + // A named schema costs a diff first, so the deadline has to cover both. + writeBudget := storageSchemaApplyWriteBudget(budget) + if namesSchema(req.SchemaSource) { + writeBudget += StorageSchemaPlanTimeout + } + ctx, cancel := s.extendOperatorWriteDeadline(w, r, writeBudget) + defer cancel() + + if s.refuseUnverifiedNamedSchema(ctx, w, target, req) { + return + } + operator := resolveCaller(r.Context(), req.Caller) s.logger.Info("converging storage schema on operator request", "deployment", target.deployment, "environment", target.environment, "allow_destructive", req.AllowDestructive, "convergence_timeout", budget, + "schema_source", req.SchemaSource, + "schema_file_count", len(req.SchemaFiles), "caller", operator) - ctx, cancel := s.extendOperatorWriteDeadline(w, r, storageSchemaApplyWriteBudget(budget)) - defer cancel() - resp, err := target.service.StorageSchemaApply(ctx, &ternv1.StorageSchemaApplyRequest{ AllowDestructive: req.AllowDestructive, Caller: operator, TimeoutSeconds: int64(budget / time.Second), + SchemaFiles: req.SchemaFiles, + SchemaSource: req.SchemaSource, }) if err != nil { s.logger.Error("storage schema apply failed", "deployment", target.deployment, "environment", target.environment, "allow_destructive", req.AllowDestructive, + "schema_source", req.SchemaSource, + "schema_file_count", len(req.SchemaFiles), "caller", operator, "error", err) s.writeStorageSchemaFailure(w, err, "storage schema apply failed") @@ -263,6 +294,9 @@ func (s *Service) handleStorageSchemaApply(w http.ResponseWriter, r *http.Reques s.writeError(w, http.StatusInternalServerError, "storage schema apply returned an incomplete result; check the target's logs for whether it converged") return } + if s.refuseUnhonoredConvergence(w, target, req.SchemaSource, planned, remaining) { + return + } s.writeJSON(w, http.StatusOK, apitypes.StorageSchemaApplyResponse{Planned: planned, Remaining: remaining}) } @@ -356,6 +390,161 @@ func (s *Service) writeStorageSchemaFailure(w http.ResponseWriter, err error, su } } +// namesSchema reports whether a request asked about a schema of its own rather +// than about the answering target's embedded one. +func namesSchema(source string) bool { return strings.TrimSpace(source) != "" } + +// refuseUnverifiedNamedSchema proves, before a single statement runs, that the +// target honors the schema this convergence names. +// +// A diff is the only way to ask: there is no capability handshake, and the +// fields carrying a named schema are ones an instance that predates them drops +// silently. Since a diff answers the same question a convergence does and runs +// nothing, the answer to "would this target work from the schema I sent" can be +// had for a catalog read instead of for executed DDL. +// +// Asking after the convergence instead is not equivalent, because a target that +// ignored the schema did not ignore the rest of the request. It still honors +// allow_destructive, so it diffs its own older embedded schema against a +// database holding a newer release's state, finds that state surplus, and drops +// it — removals no boot of that deployment would have run, since a boot never +// sees an operator's opt-in. The convergence cannot be taken back once it has +// run, so the question is asked while the answer can still prevent it (AV-9). +func (s *Service) refuseUnverifiedNamedSchema(ctx context.Context, w http.ResponseWriter, target *storageSchemaTarget, req apitypes.StorageSchemaApplyRequest) bool { + if !namesSchema(req.SchemaSource) { + return false + } + // Bounded on its own so a slow diff cannot spend the convergence's budget: + // what this buys is worth a catalog read, not the run it is protecting. + ctx, cancel := context.WithTimeout(ctx, StorageSchemaPlanTimeout) + defer cancel() + + resp, err := target.service.StorageSchemaPlan(ctx, &ternv1.StorageSchemaPlanRequest{ + AllowDestructive: req.AllowDestructive, + SchemaFiles: req.SchemaFiles, + SchemaSource: req.SchemaSource, + }) + if err != nil { + s.logger.Error("refusing a storage convergence whose target could not be asked which schema it would use", + "operation", storageSchemaApplyOperation, + "deployment", target.deployment, + "environment", target.environment, + "asked_schema_source", req.SchemaSource, + "schema_file_count", len(req.SchemaFiles), + "error", err) + s.writeStorageSchemaFailure(w, err, "could not confirm the target converges the named schema, so nothing was converged") + return true + } + report := storageSchemaReportResponse(target, resp.GetReport()) + if report == nil { + s.logger.Error("refusing a storage convergence whose target answered no report when asked which schema it would use", + "operation", storageSchemaApplyOperation, + "deployment", target.deployment, + "environment", target.environment, + "asked_schema_source", req.SchemaSource) + s.writeError(w, http.StatusBadGateway, + "the target returned no report when asked which schema it would converge, so nothing was converged; see the answering deployment's logs") + return true + } + return s.refuseUnhonoredSchema(w, target, storageSchemaApplyOperation, req.SchemaSource, report) +} + +// refuseUnhonoredConvergence is refuseUnhonoredSchema for an answer that +// arrives once the DDL has run. +// +// The convergence is preceded by a diff that proves the target honors the named +// schema, so reaching here means the pod that converged is not the pod that +// answered the diff. A deployment is many pods and a roll makes them different +// releases, so the check is made again on the answer that matters rather than +// trusted from the one before it. +// +// It is a separate refusal because the remedy is: statements have executed +// against a schema nobody asked for, and saying only "upgrade that target" +// would leave an operator to discover that from the database. What ran is +// named here, and the reports are logged whole, since the response carries an +// error rather than them. +func (s *Service) refuseUnhonoredConvergence(w http.ResponseWriter, target *storageSchemaTarget, asked string, planned, remaining *apitypes.StorageSchemaReport) bool { + asked = strings.TrimSpace(asked) + if asked == "" || planned == nil || planned.SchemaSource == asked { + return false + } + s.logger.Error("a storage convergence ran against a schema the caller did not ask for", + "operation", storageSchemaApplyOperation, + "deployment", target.deployment, + "environment", target.environment, + "database", planned.Database, + "dialect", planned.Dialect, + "asked_schema_source", asked, + "converged_schema_source", planned.SchemaSource, + "converged_version", planned.Version, + "statements_run", storageSchemaStatementsRun(planned), + "planned", planned, + "remaining", remaining) + s.writeError(w, http.StatusBadGateway, fmt.Sprintf( + "the target converged %q, not the schema that was asked for: it is running a release that does not accept a named schema and worked from its own embedded files instead, and %d statement(s) have already run against %s. Reconcile that database against the release the target is running before retrying, then upgrade the target or address a release that carries this schema", + planned.SchemaSource, storageSchemaStatementsRun(planned), storageSchemaDatabaseLabel(planned))) + return true +} + +// storageSchemaStatementsRun is how many statements a convergence executed, +// which is the outstanding set plus the destructive one wherever the target +// permitted it. A refused destructive statement did not run, so counting it +// would overstate what an operator has to reconcile. +func storageSchemaStatementsRun(planned *apitypes.StorageSchemaReport) int { + run := len(planned.Outstanding) + if planned.DestructiveAllowed { + run += len(planned.Destructive) + } + return run +} + +// storageSchemaDatabaseLabel names the database a report is about, for an error +// an operator has to act on. The host is included when the target reported one, +// since a database name alone does not say which instance to go to. +func storageSchemaDatabaseLabel(report *apitypes.StorageSchemaReport) string { + if report.Host == "" { + return report.Database + } + return report.Database + " on " + report.Host +} + +// refuseUnhonoredSchema stops an answer whose report does not name the schema +// the caller asked about, where nothing has run yet. +// +// A named schema travels as fields on the request, and an instance that +// predates them ignores what it does not recognize and works from its own +// embedded files instead — the defined behavior of the wire format, and +// indistinguishable from success in the response. The report's own schema +// source is the proof, because the answering side sets it from the schema it +// actually diffed: honored, it echoes what was sent; ignored, it names the +// answering binary's own files. +// +// Refusing is the only safe reading. A report attributed to the release an +// operator asked about but computed from another is the input to their decision +// about whether to pre-apply, and it says the named release's storage is ready +// when nothing has compared the two. The gap surfaces when that release's pods +// boot and find their storage short — the failure this command exists to +// prevent, now with an operator who has been told it cannot happen. +func (s *Service) refuseUnhonoredSchema(w http.ResponseWriter, target *storageSchemaTarget, operation, asked string, report *apitypes.StorageSchemaReport) bool { + asked = strings.TrimSpace(asked) + if asked == "" || report == nil || report.SchemaSource == asked { + return false + } + s.logger.Error("refusing a storage schema answer computed from a schema the caller did not ask for", + "operation", operation, + "deployment", target.deployment, + "environment", target.environment, + "database", report.Database, + "dialect", report.Dialect, + "asked_schema_source", asked, + "answered_schema_source", report.SchemaSource, + "answered_version", report.Version) + s.writeError(w, http.StatusBadGateway, fmt.Sprintf( + "the target answered about %q, not the schema that was asked for; it is running a release that does not accept a named schema and worked from its own embedded files instead. Upgrade that target, or address a release that carries this schema", + report.SchemaSource)) + return true +} + // storageSchemaReportResponse converts one wire report to its HTTP form, // stamping the target it describes. Stamping here rather than at the source is // deliberate: only the control plane knows which route it asked, and an @@ -464,19 +653,24 @@ func decodeOptionalStorageSchemaBody[T any](r *http.Request) (T, error) { return req, nil } -// validateStorageSchemaPlanRequest refuses a desired schema that is only half -// supplied. The two fields travel together or not at all: files without a -// source would produce a report that cannot say what it was diffed against, -// and a source without files would label this server's own embedded schema -// with somebody else's name — which is the one way a report of this kind can -// be actively misleading rather than merely wrong. -func validateStorageSchemaPlanRequest(req apitypes.StorageSchemaPlanRequest) error { - source := strings.TrimSpace(req.SchemaSource) +// validateStorageSchemaSource refuses a schema that is only half supplied. The +// two fields travel together or not at all: files without a source would +// produce a report that cannot say which schema it describes, and a source +// without files would label this server's own embedded schema with somebody +// else's name — which is the one way an answer of this kind can be actively +// misleading rather than merely wrong. +// +// Both routes validate through this, because on the convergence route the +// misattribution is worse than a wrong report. A convergence that ran the +// embedded schema under a release's name would tell an operator their storage +// is ready for a release it was never compared against. +func validateStorageSchemaSource(files map[string]string, schemaSource string) error { + source := strings.TrimSpace(schemaSource) switch { - case len(req.SchemaFiles) > 0 && source == "": - return fmt.Errorf("schema_files was sent without schema_source: a report has to say which schema it was diffed against, so name the source (a release, a directory) alongside the files") - case len(req.SchemaFiles) == 0 && source != "": - return fmt.Errorf("schema_source %q was sent without schema_files: with no files the diff would run against this server's own embedded schema and report it under that name; send the files, or drop schema_source to ask about the embedded schema", source) + case len(files) > 0 && source == "": + return fmt.Errorf("schema_files was sent without schema_source: an answer has to say which schema it used, so name the source (a release, a directory) alongside the files") + case len(files) == 0 && source != "": + return fmt.Errorf("schema_source %q was sent without schema_files: with no files this server's own embedded schema would be used and reported under that name; send the files, or drop schema_source to use the embedded schema", source) default: return nil } diff --git a/pkg/api/storage_schema_handlers_test.go b/pkg/api/storage_schema_handlers_test.go index 2e39ee3bf..571f89331 100644 --- a/pkg/api/storage_schema_handlers_test.go +++ b/pkg/api/storage_schema_handlers_test.go @@ -626,21 +626,212 @@ func TestStorageSchemaRoutes_OutliveTheServerWideWriteTimeout(t *testing.T) { } } -// A half-supplied desired schema is refused. Files with no source produce a -// report that cannot say what it was compared against; a source with no files -// would label this server's own embedded schema with another release's name. -func TestValidateStorageSchemaPlanRequest(t *testing.T) { - require.NoError(t, validateStorageSchemaPlanRequest(apitypes.StorageSchemaPlanRequest{})) - require.NoError(t, validateStorageSchemaPlanRequest(apitypes.StorageSchemaPlanRequest{ - SchemaFiles: validStorageSchemaFiles(), - SchemaSource: "the schema files of release v1.4.0", - })) - - err := validateStorageSchemaPlanRequest(apitypes.StorageSchemaPlanRequest{SchemaFiles: validStorageSchemaFiles()}) +// A half-supplied schema is refused on both routes. Files with no source +// produce an answer that cannot say which schema it used; a source with no +// files would label this server's own embedded schema with another release's +// name, which on the convergence route means telling an operator their storage +// is ready for a release it was never compared against. +func TestValidateStorageSchemaSource(t *testing.T) { + require.NoError(t, validateStorageSchemaSource(nil, "")) + require.NoError(t, validateStorageSchemaSource(validStorageSchemaFiles(), "the schema files of release v1.4.0")) + + err := validateStorageSchemaSource(validStorageSchemaFiles(), "") require.Error(t, err) assert.Contains(t, err.Error(), "without schema_source") - err = validateStorageSchemaPlanRequest(apitypes.StorageSchemaPlanRequest{SchemaSource: "the schema files of release v1.4.0"}) + err = validateStorageSchemaSource(nil, "the schema files of release v1.4.0") require.Error(t, err) assert.Contains(t, err.Error(), "without schema_files") } + +// Both storage schema routes refuse a half-supplied schema over HTTP, before +// anything is resolved or converged. On the convergence route that refusal is +// the one that matters: a source with no files would run the server's own +// embedded schema and report it as the named release's, telling an operator +// their storage is ready for a release it was never compared against. +func TestStorageSchemaRoutes_RefuseHalfSuppliedSchema(t *testing.T) { + local := &fakeStorageSchemaService{ + planResp: &ternv1.StorageSchemaPlanResponse{Report: storageSchemaReportMessage("schemabot_storage")}, + applyResp: &ternv1.StorageSchemaApplyResponse{ + Planned: storageSchemaReportMessage("schemabot_storage"), + Remaining: storageSchemaReportMessage("schemabot_storage"), + }, + } + const body = `{"schema_source":"the schema files of release v1.5.0"}` + + t.Run("plan", func(t *testing.T) { + svc := newStorageSchemaService(t, &ServerConfig{}) + svc.SetStorageSchemaService(local) + rec := storageSchemaPlanRequestFor(t, svc, body) + assert.Equal(t, http.StatusBadRequest, rec.Code, "body: %s", rec.Body.String()) + assert.Contains(t, rec.Body.String(), "without schema_files") + }) + t.Run("apply", func(t *testing.T) { + svc := newStorageSchemaService(t, &ServerConfig{}) + svc.SetStorageSchemaService(local) + rec := storageSchemaApplyRequest(t, svc, body) + assert.Equal(t, http.StatusBadRequest, rec.Code, "body: %s", rec.Body.String()) + assert.Contains(t, rec.Body.String(), "without schema_files") + assert.Nil(t, local.applyReq, "nothing converges on a refused request") + }) +} + +// A target that ignored the named schema is refused, on both routes, rather +// than answered as though it had honored it. +// +// The fields carrying a named schema are new, and the wire format's defined +// behavior is that an instance predating them drops what it does not recognize +// and answers from its own embedded files. Nothing in that response says so, +// which leaves the report's own schema source as the only evidence: honored, it +// echoes what was asked; ignored, it names the answering binary's schema. +// +// A diff read this way is attributed to a release it never compared against. A +// convergence is worse, so a convergence is refused before it runs: the diff +// that proves the target honors the schema costs a catalog read, and executed +// DDL cannot be taken back. A target that ignored the schema did not ignore +// allow_destructive along with it, so left to run it drops state its own older +// schema does not declare. +func TestStorageSchemaRoutes_RefuseAnAnswerAboutAnotherSchema(t *testing.T) { + const asked = "the schema files of release v1.5.0 in block/schemabot" + + // What a release too old to accept a named schema answers: its own + // embedded schema, reported as a success. + ignored := func(outstanding ...string) *ternv1.StorageSchemaReport { + report := storageSchemaReportMessage("schemabot_storage", outstanding...) + report.SchemaSource = "the schema embedded in v1.4.0" + report.Host = "db-1.example" + return report + } + honored := func(outstanding ...string) *ternv1.StorageSchemaReport { + report := storageSchemaReportMessage("schemabot_storage", outstanding...) + report.SchemaSource = asked + return report + } + + files, err := json.Marshal(validStorageSchemaFiles()) + require.NoError(t, err) + body := fmt.Sprintf(`{"schema_source":%q,"schema_files":%s}`, asked, files) + + t.Run("plan", func(t *testing.T) { + svc := newStorageSchemaService(t, &ServerConfig{}) + svc.SetStorageSchemaService(&fakeStorageSchemaService{ + planResp: &ternv1.StorageSchemaPlanResponse{Report: ignored()}, + }) + + rec := storageSchemaPlanRequestFor(t, svc, body) + assert.Equal(t, http.StatusBadGateway, rec.Code, "body: %s", rec.Body.String()) + assert.Contains(t, rec.Body.String(), "the schema embedded in v1.4.0", + "the answer names the schema it really used, which is what tells an operator what happened") + assert.Contains(t, rec.Body.String(), "Upgrade that target") + }) + + // The convergence is stopped by the diff in front of it, so the refusal + // costs the operator a catalog read rather than a reconciliation. + t.Run("apply refuses before converging", func(t *testing.T) { + svc := newStorageSchemaService(t, &ServerConfig{}) + local := &fakeStorageSchemaService{ + planResp: &ternv1.StorageSchemaPlanResponse{Report: ignored()}, + applyResp: &ternv1.StorageSchemaApplyResponse{ + Planned: ignored("applies"), + Remaining: ignored(), + }, + } + svc.SetStorageSchemaService(local) + + rec := storageSchemaApplyRequest(t, svc, body) + assert.Equal(t, http.StatusBadGateway, rec.Code, "body: %s", rec.Body.String()) + assert.Contains(t, rec.Body.String(), "the schema embedded in v1.4.0") + assert.Nil(t, local.applyReq, "the convergence must not have been attempted") + assert.NotContains(t, rec.Body.String(), "have already run", + "nothing ran, so the refusal must not send an operator reconciling") + }) + + // A deployment is many pods, and a roll makes them different releases, so + // the pod that answered the diff need not be the pod that converges. Here + // the DDL has run against the wrong schema, and the refusal has to say so + // and name what to reconcile rather than only naming the upgrade. + t.Run("apply refuses after a later pod ran it", func(t *testing.T) { + svc := newStorageSchemaService(t, &ServerConfig{}) + ran := ignored("applies", "checks") + ran.DestructiveAllowed = true + ran.Destructive = []*ternv1.StorageSchemaStatement{{ + Table: "newer_release_state", + Operation: storageSchemaOpDropTable, + Ddl: "DROP TABLE `newer_release_state`", + Reason: "DROP TABLE destroys data", + }} + local := &fakeStorageSchemaService{ + planResp: &ternv1.StorageSchemaPlanResponse{Report: honored()}, + applyResp: &ternv1.StorageSchemaApplyResponse{ + Planned: ran, + Remaining: ignored(), + }, + } + svc.SetStorageSchemaService(local) + + rec := storageSchemaApplyRequest(t, svc, body) + assert.Equal(t, http.StatusBadGateway, rec.Code, "body: %s", rec.Body.String()) + require.NotNil(t, local.applyReq, "the diff passed, so the convergence was attempted") + assert.Contains(t, rec.Body.String(), "the schema embedded in v1.4.0") + assert.Contains(t, rec.Body.String(), "3 statement(s) have already run", + "two outstanding and one permitted destructive statement executed") + assert.Contains(t, rec.Body.String(), "schemabot_storage on db-1.example", + "the database to reconcile has to be named, not left to the operator to work out") + assert.Contains(t, rec.Body.String(), "Reconcile that database") + }) + + // A target that echoes the schema it was asked about honored it, and is + // answered normally. Without this the guards would refuse every named + // convergence rather than the ones that went wrong. + t.Run("honored", func(t *testing.T) { + svc := newStorageSchemaService(t, &ServerConfig{}) + local := &fakeStorageSchemaService{ + planResp: &ternv1.StorageSchemaPlanResponse{Report: honored()}, + applyResp: &ternv1.StorageSchemaApplyResponse{Planned: honored(), Remaining: honored()}, + } + svc.SetStorageSchemaService(local) + + plan := storageSchemaPlanRequestFor(t, svc, body) + require.Equal(t, http.StatusOK, plan.Code, "body: %s", plan.Body.String()) + assert.Equal(t, asked, decodePlanResponse(t, plan).Report.SchemaSource) + + apply := storageSchemaApplyRequest(t, svc, body) + require.Equal(t, http.StatusOK, apply.Code, "body: %s", apply.Body.String()) + assert.Equal(t, asked, local.applyReq.GetSchemaSource(), "the schema still reaches the target") + }) + + // A target that cannot answer the diff has not said it honors the schema, + // which is not the same as saying it does. Converging anyway would run the + // DDL the diff exists to gate. + t.Run("apply refuses when the target cannot be asked", func(t *testing.T) { + svc := newStorageSchemaService(t, &ServerConfig{}) + local := &fakeStorageSchemaService{ + planErr: status.Error(codes.Unavailable, "connection refused"), + applyResp: &ternv1.StorageSchemaApplyResponse{ + Planned: honored(), + Remaining: honored(), + }, + } + svc.SetStorageSchemaService(local) + + rec := storageSchemaApplyRequest(t, svc, body) + assert.Equal(t, http.StatusServiceUnavailable, rec.Code, "body: %s", rec.Body.String()) + assert.Contains(t, rec.Body.String(), "nothing was converged") + assert.Nil(t, local.applyReq, "an unanswered diff converges nothing") + }) + + // A request that named no schema asks about the target's own, so whatever + // the target says it used is the right answer by construction — and the + // convergence needs no diff in front of it. + t.Run("no schema named", func(t *testing.T) { + svc := newStorageSchemaService(t, &ServerConfig{}) + local := &fakeStorageSchemaService{ + applyResp: &ternv1.StorageSchemaApplyResponse{Planned: ignored(), Remaining: ignored()}, + } + svc.SetStorageSchemaService(local) + + rec := storageSchemaApplyRequest(t, svc, "") + require.Equal(t, http.StatusOK, rec.Code, "body: %s", rec.Body.String()) + assert.Nil(t, local.planReq, "an unnamed convergence costs no extra diff") + }) +} diff --git a/pkg/api/storage_schema_integration_test.go b/pkg/api/storage_schema_integration_test.go index 2d05ffa0a..74560ba6f 100644 --- a/pkg/api/storage_schema_integration_test.go +++ b/pkg/api/storage_schema_integration_test.go @@ -20,6 +20,7 @@ import ( "github.com/stretchr/testify/require" seedutil "github.com/block/schemabot/e2e/testutil" + "github.com/block/schemabot/pkg/apitypes" "github.com/block/schemabot/pkg/engine" "github.com/block/schemabot/pkg/namedlock" "github.com/block/schemabot/pkg/postgresconn" @@ -95,7 +96,7 @@ func TestDiffStorageSchemaMySQL_EmptyDatabaseNeedsEveryTable(t *testing.T) { func TestApplyStorageSchemaMySQL_ConvergesEmptyDatabase(t *testing.T) { sdb, db := openEnsureSchemaDatabase(t) - planned, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, storageSchemaTestLogger()) + planned, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger()) require.NoError(t, err) assert.NotEmpty(t, planned.Outstanding, "an empty database has statements to run") @@ -146,7 +147,7 @@ func TestDiffStorageSchemaMySQL_ReportsMissingColumn(t *testing.T) { // Applying the reported statement is what converges it, and nothing else // is left behind. - _, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, storageSchemaTestLogger()) + _, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger()) require.NoError(t, err) assert.True(t, remaining.Converged(), "the reported statement should be the whole difference") assert.True(t, testutil.ColumnExists(t, db, sdb.Name, "applies", "caller")) @@ -179,7 +180,7 @@ func TestDiffStorageSchemaMySQL_RefusesSurplusTable(t *testing.T) { assert.Contains(t, statement.DDL, "DROP TABLE") assert.NotEmpty(t, statement.Reason, "a refusal has to say why it was refused") - planned, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, storageSchemaTestLogger()) + planned, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger()) require.NoError(t, err, "a convergence that refuses destructive statements still succeeds") assert.Len(t, planned.Destructive, 1) assert.Len(t, remaining.Destructive, 1, "the refused statement is still outstanding afterwards") @@ -188,7 +189,7 @@ func TestDiffStorageSchemaMySQL_RefusesSurplusTable(t *testing.T) { // The same report with destructive changes allowed says the statement would // run, which is what the operator opting in is asking to be told. - allowed, err := PlanStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger(), WithAllowDestructiveSchemaChanges(true)) + allowed, err := PlanStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger(), WithDestructiveSchemaChangePolicy(DestructivePolicyPermits, false)) require.NoError(t, err) assert.True(t, allowed.DestructiveAllowed) require.Len(t, allowed.Destructive, 1) @@ -230,7 +231,7 @@ func TestDiffStorageSchemaMySQL_RefusesSurplusIndex(t *testing.T) { // And a convergence leaves it intact, which is what a boot of this binary // does to an index a later release owns. - _, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, storageSchemaTestLogger()) + _, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger()) require.NoError(t, err, "a convergence that refuses destructive statements still succeeds") assert.Len(t, remaining.Destructive, 1, "the refused statement is still outstanding afterwards") assert.Equal(t, []string{"caller"}, testutil.IndexColumns(t, db, sdb.Name, "applies", "idx_applies_caller"), @@ -238,13 +239,66 @@ func TestDiffStorageSchemaMySQL_RefusesSurplusIndex(t *testing.T) { // An operator who removed the index from the embedded schema on purpose // opts in, and the report then says the statement would run. - allowed, err := PlanStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger(), WithAllowDestructiveSchemaChanges(true)) + allowed, err := PlanStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger(), WithDestructiveSchemaChangePolicy(DestructivePolicyPermits, false)) require.NoError(t, err) assert.True(t, allowed.DestructiveAllowed) require.Len(t, allowed.Destructive, 1) assert.Equal(t, "applies", allowed.Destructive[0].Table) } +// A report separates what this call may run from what the next pod to boot +// will run, because only one of them is moved by the caller asking. +// +// An operator converging a later release's schema ahead of the deploy is +// asking whether the state they apply survives until the deploy, and the +// answer belongs to the boots in between. Those read the deployment's config +// and have never heard of this request, so a per-request opt-in has to leave +// the boot's answer exactly where it was. Reported as one field, an operator +// passing --allow-unsafe would be told their own flag had changed what the +// fleet does. +func TestDiffStorageSchemaMySQL_BootPolicyIsNotMovedByTheRequestOptIn(t *testing.T) { + sdb, db := openEnsureSchemaDatabase(t) + require.NoError(t, EnsureSchema(sdb.DSN, storageSchemaTestLogger())) + + _, err := db.ExecContext(t.Context(), "CREATE INDEX `idx_applies_caller` ON `applies` (`caller`)") + require.NoError(t, err, "pre-create an index the embedded schema does not declare") + + // A deployment that configured nothing, on the run the destructive gate's + // own rerun hint asks for. + optedIn, err := PlanStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger(), + WithDestructiveSchemaChangePolicy(DestructivePolicyForbids, true)) + require.NoError(t, err) + require.Len(t, optedIn.Destructive, 1, "the surplus index is the drift under test") + assert.True(t, optedIn.DestructiveAllowed, "this convergence may drop it") + assert.Equal(t, apitypes.BootRemovalPreserves, optedIn.BootRemovalPolicy, + "and every boot of the deployed release still refuses to, which is what keeps pre-applied state alive") + + // The same drift on a deployment that has permitted destructive storage + // changes: here the boots really do drop it, and only the config says so. + configured, err := PlanStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger(), + WithDestructiveSchemaChangePolicy(DestructivePolicyPermits, false)) + require.NoError(t, err) + assert.True(t, configured.DestructiveAllowed) + assert.Equal(t, apitypes.BootRemovalRemoves, configured.BootRemovalPolicy) + + // And a deployment that permitted nothing, where neither this run nor a + // boot removes anything. + neither, err := PlanStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger(), + WithDestructiveSchemaChangePolicy(DestructivePolicyForbids, false)) + require.NoError(t, err) + assert.False(t, neither.DestructiveAllowed) + assert.Equal(t, apitypes.BootRemovalPreserves, neither.BootRemovalPolicy) + + // A database addressed directly, with no deployment config to read. The + // run itself is still refused, but the boot's answer is nobody's to give: + // reported as preservation, an operator pre-applying against a fleet that + // drops surplus state would be told it survives. + unread, err := PlanStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger()) + require.NoError(t, err) + assert.False(t, unread.DestructiveAllowed, "an unread policy grants this run nothing") + assert.Equal(t, apitypes.BootRemovalUnknown, unread.BootRemovalPolicy) +} + // One table can drift in both directions at once: it misses a column the // running binary declares and holds an index a later release owns. The differ // emits that as a single ALTER, and the report has to say what the boot will @@ -280,7 +334,7 @@ func TestDiffStorageSchemaMySQL_ReportsBothHalvesOfAMixedStatement(t *testing.T) // The convergence does what the report said: the column lands, the index // survives, and the refusal is still outstanding afterwards. - _, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, storageSchemaTestLogger()) + _, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger()) require.NoError(t, err) assert.True(t, testutil.ColumnExists(t, db, sdb.Name, "tasks", missingColumn), "the addition the report listed as outstanding must have run") @@ -407,7 +461,7 @@ func TestDiffStorageSchemaPostgres_ConvergesEmptyDatabase(t *testing.T) { assert.Equal(t, postgresOpCreateTable, applies.Operation) assert.Contains(t, strings.ToUpper(applies.DDL), "CREATE TABLE") - planned, remaining, err := ApplyStorageSchema(t.Context(), dsn, logger, postgres) + planned, remaining, err := ApplyStorageSchema(t.Context(), dsn, nil, logger, postgres) require.NoError(t, err) assert.Len(t, planned.Outstanding, len(tables)) assert.True(t, remaining.Converged(), "outstanding after convergence: %v", statementTables(remaining.Outstanding)) @@ -448,7 +502,7 @@ func TestDiffStorageSchemaPostgres_ReportsMissingColumn(t *testing.T) { assert.Contains(t, strings.ToUpper(index.DDL), "CREATE INDEX") assert.Contains(t, index.DDL, "deployment") - _, remaining, err := ApplyStorageSchema(t.Context(), dsn, logger, postgres) + _, remaining, err := ApplyStorageSchema(t.Context(), dsn, nil, logger, postgres) require.NoError(t, err) assert.True(t, remaining.Converged(), "outstanding after convergence: %v", remaining.Outstanding) assert.True(t, testutil.PostgresColumnExists(t, db, "public", "applies", "deployment")) @@ -465,7 +519,7 @@ func TestDiffStorageSchemaPostgres_ReportsMissingColumn(t *testing.T) { // catalog does not have it, which is why the answer stays correct on a database // a failed deploy left half converged. func TestDiffStorageSchemaMySQL_DiffsAgainstASuppliedSchema(t *testing.T) { - sdb, _ := openEnsureSchemaDatabase(t) + sdb, db := openEnsureSchemaDatabase(t) require.NoError(t, EnsureSchema(sdb.DSN, storageSchemaTestLogger())) converged, err := PlanStorageSchema(t.Context(), sdb.DSN, EmbeddedStorageSchema("v1.2.3"), storageSchemaTestLogger()) @@ -499,12 +553,131 @@ func TestDiffStorageSchemaMySQL_DiffsAgainstASuppliedSchema(t *testing.T) { assert.Equal(t, "the schema files of release v1.4.0", report.SchemaSource) assert.Equal(t, "v1.2.3", report.Version) - // A convergence has no way to run the supplied schema, so the column stays - // off the database until the release that declares it boots. - _, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, storageSchemaTestLogger()) + // The convergence runs the supplied schema, which is the whole point of the + // command: the column the next release declares is on the database before + // any pod of that release starts. + planned, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, desired, storageSchemaTestLogger()) + require.NoError(t, err) + require.Len(t, planned.Outstanding, 1, "the supplied schema's one statement is what ran: %v", statementTables(planned.Outstanding)) + assert.True(t, remaining.Converged(), "outstanding against the supplied schema after converging it: %v", statementTables(remaining.Outstanding)) + assert.Equal(t, "the schema files of release v1.4.0", remaining.SchemaSource, + "a convergence is attributed to the schema whose files it ran") + assert.True(t, testutil.ColumnExists(t, db, sdb.Name, "applies", "release_note"), + "the supplied schema's column has to be on the database, or the convergence ran the embedded files instead") + + // And it survives the running release. Against the schema this binary + // carries the column is now surplus, which is a DROP COLUMN — refused + // unless destroying storage state was permitted — so every pod of the + // deployed release leaves it in place until that release rolls (AV-9). + running, err := PlanStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger()) + require.NoError(t, err) + assert.Empty(t, running.Outstanding, "nothing about the running schema runs automatically here: %v", statementTables(running.Outstanding)) + surplus := statementFor(t, running.Destructive, "applies") + assert.Contains(t, surplus.DDL, "DROP COLUMN") + assert.Contains(t, surplus.DDL, "`release_note`") + + require.NoError(t, EnsureSchema(sdb.DSN, storageSchemaTestLogger()), "a boot of the running release must not fail on surplus state") + assert.True(t, testutil.ColumnExists(t, db, sdb.Name, "applies", "release_note"), + "a pre-applied column survives a boot of the release that does not declare it") +} + +// An operator converges a later release's schema ahead of its roll, and that +// schema declares an index the deployed release does not. This is the case the +// confirmation's notice makes a prediction about, so the prediction is checked +// here against what a boot of the deployed release actually does: it refuses +// the drop and leaves the index in place, exactly as it does for a table or a +// column, and it says so in the log every time a pod starts. An operator told +// the opposite would schedule the deploy window around an index that was never +// at risk, and one not told about the logging would read a fleet warning on +// every boot as an incident. +func TestApplyStorageSchemaMySQL_PreAppliedIndexSurvivesTheRunningRelease(t *testing.T) { + const preAppliedIndex = "idx_release_caller_created" + preAppliedColumns := []string{"caller", "created_at"} + + sdb, db := openEnsureSchemaDatabase(t) + require.NoError(t, EnsureSchema(sdb.DSN, storageSchemaTestLogger())) + + // The next release's schema: the running one, plus an index on one table. + files, err := storageSchemaFilesForTest() + require.NoError(t, err) + const lastKey = "KEY `idx_completed_at_state` (`completed_at`,`state`)" + require.Contains(t, files["applies.sql"], lastKey, "the fixture this test appends to has moved") + files["applies.sql"] = strings.Replace(files["applies.sql"], lastKey, + lastKey+",\n KEY `"+preAppliedIndex+"` (`caller`,`created_at`)", 1) + desired, err := StorageSchemaFromFiles("the schema files of release v1.4.0", files) + require.NoError(t, err) + + _, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, desired, storageSchemaTestLogger()) + require.NoError(t, err) + require.True(t, remaining.Converged(), "outstanding against the supplied schema after converging it: %v", statementTables(remaining.Outstanding)) + require.Equal(t, preAppliedColumns, testutil.IndexColumns(t, db, sdb.Name, "applies", preAppliedIndex), + "the named schema's index has to be on the database, or the convergence ran the embedded files instead") + + // Against the running release the index is now surplus, and the drop that + // would remove it is refused rather than outstanding. + running, err := PlanStorageSchema(t.Context(), sdb.DSN, nil, storageSchemaTestLogger()) + require.NoError(t, err) + assert.Empty(t, running.Outstanding, "nothing about the running schema runs automatically here: %v", statementTables(running.Outstanding)) + surplus := statementFor(t, running.Destructive, "applies") + assert.Contains(t, surplus.DDL, "DROP INDEX") + assert.Contains(t, surplus.DDL, preAppliedIndex) + + // So every pod of the deployed release leaves it in place, and logs the + // refusal naming it, until the release that declares it rolls (AV-9). + var logBuf syncBuffer + bootLogger := slog.New(slog.NewTextHandler(&logBuf, &slog.HandlerOptions{Level: slog.LevelDebug})) + require.NoError(t, EnsureSchema(sdb.DSN, bootLogger), "a boot of the running release must not fail on a surplus index") + assert.Equal(t, preAppliedColumns, testutil.IndexColumns(t, db, sdb.Name, "applies", preAppliedIndex), + "a pre-applied index survives a boot of the release that does not declare it, like a pre-applied column") + + logs := logBuf.String() + assert.Contains(t, logs, "refusing destructive storage-schema change") + assert.Contains(t, logs, preAppliedIndex, + "the refusal has to name the index, or an operator cannot tell which object the fleet is warning about") +} + +// The PostgreSQL bootstrap converges the schema it is given too, which the +// MySQL test cannot show for it: the two dialects read their files and compute +// their drift in separate code, so a source that reached one and not the other +// would converge the running release's schema while reporting the named one. +// +// The supplied set is deliberately one table. An empty storage database +// converged against it has exactly that table afterwards, and would have every +// storage table if the embedded files had been used instead. +func TestApplyStorageSchemaPostgres_ConvergesASuppliedSchema(t *testing.T) { + dsn, db := startPostgresStorage(t) + logger := storageSchemaTestLogger() + postgres := WithDialect(schema.DialectPostgres) + + desired, err := StorageSchemaFromFiles("the schema files of release v1.4.0", map[string]string{ + "locks.sql": `CREATE TABLE IF NOT EXISTS "locks" ( + "name" TEXT PRIMARY KEY, + "owner" TEXT NOT NULL +)`, + }) require.NoError(t, err) - assert.True(t, remaining.Converged(), "an apply converges the running binary's schema, not the supplied one") - assert.Equal(t, "the schema embedded in this binary", remaining.SchemaSource) + + planned, remaining, err := ApplyStorageSchema(t.Context(), dsn, desired, logger, postgres) + require.NoError(t, err) + require.Len(t, planned.Outstanding, 1, "the supplied schema declares one table: %v", statementTables(planned.Outstanding)) + assert.Equal(t, "locks", planned.Outstanding[0].Table) + assert.True(t, remaining.Converged(), "outstanding after converging the supplied schema: %v", statementTables(remaining.Outstanding)) + assert.Equal(t, "the schema files of release v1.4.0", remaining.SchemaSource) + + assert.True(t, postgresTableExists(t, db, "locks"), "the supplied schema's table must be created") + assert.False(t, postgresTableExists(t, db, "applies"), + "a table only the embedded schema declares must not appear, or the convergence ran the embedded files") +} + +// postgresTableExists reports whether one table is in the storage database's +// current schema. +func postgresTableExists(t *testing.T, db *sql.DB, table string) bool { + t.Helper() + var exists bool + require.NoError(t, db.QueryRowContext(t.Context(), + `SELECT EXISTS (SELECT 1 FROM information_schema.tables + WHERE table_schema = current_schema() AND table_name = $1)`, table).Scan(&exists)) + return exists } // storageSchemaFilesForTest is the embedded MySQL schema as a file-name map, the @@ -557,7 +730,7 @@ func TestApplyStorageSchemaMySQL_StopsWhenItsCallerStops(t *testing.T) { ctx, stop := context.WithCancel(t.Context()) done := make(chan error, 1) go func() { - _, _, err := ApplyStorageSchema(ctx, sdb.DSN, logger) + _, _, err := ApplyStorageSchema(ctx, sdb.DSN, nil, logger) done <- err }() @@ -612,7 +785,7 @@ func TestApplyStorageSchemaMySQL_StopsTheSchemaChangeItStarted(t *testing.T) { ctx, stop := context.WithCancel(t.Context()) done := make(chan error, 1) go func() { - _, _, err := ApplyStorageSchema(ctx, sdb.DSN, logger) + _, _, err := ApplyStorageSchema(ctx, sdb.DSN, nil, logger) done <- err }() @@ -669,7 +842,7 @@ func TestApplyStorageSchemaPostgres_StopsWhenItsCallerStops(t *testing.T) { ctx, stop := context.WithCancel(t.Context()) done := make(chan error, 1) go func() { - _, _, err := ApplyStorageSchema(ctx, dsn, logger, postgres) + _, _, err := ApplyStorageSchema(ctx, dsn, nil, logger, postgres) done <- err }() @@ -756,7 +929,7 @@ func TestApplyStorageSchemaMySQL_ReportsProgressToAWatchingCaller(t *testing.T) logger := storageSchemaTestLogger() var observed []StorageConvergenceProgress - _, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, logger, + _, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, nil, logger, WithConvergenceProgress(func(p StorageConvergenceProgress) { observed = append(observed, p) })) require.NoError(t, err) require.True(t, remaining.Converged()) @@ -796,7 +969,7 @@ func TestApplyStorageSchemaMySQL_ReportsTheEnginesOwnPerTableVocabulary(t *testi require.NoError(t, err) var observed []StorageConvergenceProgress - _, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, logger, + _, remaining, err := ApplyStorageSchema(t.Context(), sdb.DSN, nil, logger, WithConvergenceProgress(func(p StorageConvergenceProgress) { observed = append(observed, p) })) require.NoError(t, err) require.True(t, remaining.Converged()) @@ -841,7 +1014,7 @@ func TestApplyStorageSchemaPostgres_ReportsProgressToAWatchingCaller(t *testing. postgres := WithDialect(schema.DialectPostgres) var observed []StorageConvergenceProgress - _, remaining, err := ApplyStorageSchema(t.Context(), dsn, logger, postgres, + _, remaining, err := ApplyStorageSchema(t.Context(), dsn, nil, logger, postgres, WithConvergenceProgress(func(p StorageConvergenceProgress) { observed = append(observed, p) })) require.NoError(t, err) require.True(t, remaining.Converged()) diff --git a/pkg/api/storage_schema_proto.go b/pkg/api/storage_schema_proto.go index d3bf5d999..00aba3a83 100644 --- a/pkg/api/storage_schema_proto.go +++ b/pkg/api/storage_schema_proto.go @@ -1,6 +1,7 @@ package api import ( + "github.com/block/schemabot/pkg/apitypes" ternv1 "github.com/block/schemabot/pkg/proto/ternv1" "github.com/block/schemabot/pkg/schema" ) @@ -29,6 +30,7 @@ func StorageSchemaReportProto(r *StorageSchemaReport) *ternv1.StorageSchemaRepor Manual: storageSchemaStatementsProto(r.Manual), ConvergenceInFlight: r.ConvergenceInFlight, + BootRemovalPolicy: bootRemovalPolicyProto(r.BootRemovalPolicy), } } @@ -53,9 +55,42 @@ func StorageSchemaReportFromProto(p *ternv1.StorageSchemaReport) *StorageSchemaR Manual: storageSchemaStatementsFromProto(p.GetManual()), ConvergenceInFlight: p.GetConvergenceInFlight(), + BootRemovalPolicy: bootRemovalPolicyFromProto(p.GetBootRemovalPolicy()), } } +// bootRemovalPolicyProto and bootRemovalPolicyFromProto carry what a boot does +// to surplus storage state across the wire. +// +// Both map an unrecognized value to unknown rather than to preservation. A data +// plane older than the field leaves it at the zero value, and a newer one may +// send a policy this binary has no name for; in both cases the honest answer is +// that this side could not be told, and the reassuring answer is the one that +// gets an operator to pre-apply storage the next pod will drop. +func bootRemovalPolicyProto(policy apitypes.BootRemovalPolicy) ternv1.BootRemovalPolicy { + switch policy { + case apitypes.BootRemovalPreserves: + return ternv1.BootRemovalPolicy_BOOT_REMOVAL_POLICY_PRESERVES + case apitypes.BootRemovalRemoves: + return ternv1.BootRemovalPolicy_BOOT_REMOVAL_POLICY_REMOVES + case apitypes.BootRemovalUnknown: + return ternv1.BootRemovalPolicy_BOOT_REMOVAL_POLICY_UNSPECIFIED + } + return ternv1.BootRemovalPolicy_BOOT_REMOVAL_POLICY_UNSPECIFIED +} + +func bootRemovalPolicyFromProto(policy ternv1.BootRemovalPolicy) apitypes.BootRemovalPolicy { + switch policy { + case ternv1.BootRemovalPolicy_BOOT_REMOVAL_POLICY_PRESERVES: + return apitypes.BootRemovalPreserves + case ternv1.BootRemovalPolicy_BOOT_REMOVAL_POLICY_REMOVES: + return apitypes.BootRemovalRemoves + case ternv1.BootRemovalPolicy_BOOT_REMOVAL_POLICY_UNSPECIFIED: + return apitypes.BootRemovalUnknown + } + return apitypes.BootRemovalUnknown +} + func storageSchemaStatementsProto(statements []StorageSchemaStatement) []*ternv1.StorageSchemaStatement { if len(statements) == 0 { return nil diff --git a/pkg/api/storage_schema_proto_test.go b/pkg/api/storage_schema_proto_test.go index 3ef124128..e7f9bffce 100644 --- a/pkg/api/storage_schema_proto_test.go +++ b/pkg/api/storage_schema_proto_test.go @@ -6,6 +6,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/block/schemabot/pkg/apitypes" + ternv1 "github.com/block/schemabot/pkg/proto/ternv1" "github.com/block/schemabot/pkg/schema" ) @@ -38,6 +40,7 @@ func TestStorageSchemaReport_ProtoRoundTrip(t *testing.T) { Reason: "column is NOT NULL without a DEFAULT", }}, ConvergenceInFlight: true, + BootRemovalPolicy: apitypes.BootRemovalRemoves, } wire := StorageSchemaReportProto(report) @@ -67,12 +70,48 @@ func TestStorageSchemaReport_ProtoRoundTrip(t *testing.T) { assert.Equal(t, "column is NOT NULL without a DEFAULT", wire.GetManual()[0].GetReason()) assert.True(t, wire.GetConvergenceInFlight(), "a control plane that dropped this would render a mid-convergence database as an idle one") + assert.Equal(t, ternv1.BootRemovalPolicy_BOOT_REMOVAL_POLICY_REMOVES, wire.GetBootRemovalPolicy(), + "a control plane that dropped this would tell an operator their pre-applied state survives a fleet that drops it") round := StorageSchemaReportFromProto(wire) require.NotNil(t, round) assert.Equal(t, report, round) } +// A data plane that does not set the boot removal policy is reporting that it +// could not say, and the control plane must not read that as preservation. +// +// An instance older than the field leaves it at the wire's zero value while +// still answering everything else, including destructive_allowed. Resolving +// that silence into "your fleet keeps what you pre-apply" is how an operator on +// a deployment that drops it is told to go ahead. +func TestStorageSchemaReport_AnUnsetBootPolicyStaysUnknown(t *testing.T) { + older := &ternv1.StorageSchemaReport{ + Dialect: "mysql", + Database: "schemabot", + SchemaSource: "release v0.1.68", + DestructiveAllowed: true, + } + + report := StorageSchemaReportFromProto(older) + require.NotNil(t, report) + assert.Equal(t, apitypes.BootRemovalUnknown, report.BootRemovalPolicy) + assert.NotEqual(t, apitypes.BootRemovalPreserves, report.BootRemovalPolicy, + "an absent answer is not the reassuring one") + assert.True(t, report.DestructiveAllowed, "the fields an older release does set still arrive") + assert.Equal(t, apitypes.BootRemovalUnknown, report.APIType().BootRemovalPolicy, + "and the unknown survives the hop to the shape the CLI reads") +} + +// A policy this binary has no name for is unknown, not preservation. The +// answering side is newer and has a case this one cannot act on, which is the +// same position as being told nothing. +func TestStorageSchemaReport_AnUnrecognizedBootPolicyStaysUnknown(t *testing.T) { + assert.Equal(t, apitypes.BootRemovalUnknown, bootRemovalPolicyFromProto(ternv1.BootRemovalPolicy(99))) + assert.Equal(t, ternv1.BootRemovalPolicy_BOOT_REMOVAL_POLICY_UNSPECIFIED, + bootRemovalPolicyProto(apitypes.BootRemovalPolicy("a policy from a later release"))) +} + // A nil wire report converts to a nil report rather than to an empty one. An // RPC that answered with no report at all is a different condition from one // that reported convergence, and inventing convergence here would turn a diff --git a/pkg/api/storage_schema_report.go b/pkg/api/storage_schema_report.go index 940de7e5b..33d031eb8 100644 --- a/pkg/api/storage_schema_report.go +++ b/pkg/api/storage_schema_report.go @@ -72,6 +72,23 @@ type StorageSchemaReport struct { // actually run. It reflects the effective policy for the request — the // storage config's allowance, or an explicit per-request opt-in. DestructiveAllowed bool + // BootRemovalPolicy is what the next pod to start does to storage state its + // own schema does not declare. + // + // This is the deployment's standing policy and its dialect, never the + // request's opt-in: a caller permitting destructive statements moves + // DestructiveAllowed and leaves this alone, because a boot reads config + // and has never heard of the request. A dialect whose bootstrap is + // additive-only preserves the state whatever that deployment configured, + // since there a boot computes no removal to permit. + // + // It answers what becomes of state this convergence leaves behind, which + // DestructiveAllowed cannot: an operator converging a later release's + // schema ahead of the deploy is asking whether it survives until the + // deploy, and the answer belongs to the boots in between rather than to + // the command they ran. Unknown is one of the answers — see + // apitypes.BootRemovalUnknown — and it never collapses into "preserved". + BootRemovalPolicy apitypes.BootRemovalPolicy // Manual lists changes that cannot run automatically, each naming the // situation and the remediation. Any entry aborts convergence before a // single statement executes, so an apply is refused while one is present. @@ -156,6 +173,7 @@ func (r *StorageSchemaReport) APIType() *apitypes.StorageSchemaReport { Manual: storageSchemaStatementsAPIType(r.Manual), ConvergenceInFlight: r.ConvergenceInFlight, + BootRemovalPolicy: r.BootRemovalPolicy, } } diff --git a/pkg/api/storage_schema_report_test.go b/pkg/api/storage_schema_report_test.go index 9ea346e64..a2249fcfd 100644 --- a/pkg/api/storage_schema_report_test.go +++ b/pkg/api/storage_schema_report_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/block/schemabot/pkg/apitypes" "github.com/block/schemabot/pkg/schema" ) @@ -54,6 +55,7 @@ func TestStorageSchemaReport_APIType(t *testing.T) { DDL: "DROP TABLE `newer_release_state`", Reason: "DROP TABLE destroys data", }}, + BootRemovalPolicy: apitypes.BootRemovalPreserves, } converted := report.APIType() @@ -69,6 +71,8 @@ func TestStorageSchemaReport_APIType(t *testing.T) { assert.Equal(t, storageSchemaOpDropTable, converted.Destructive[0].Operation) assert.Equal(t, "DROP TABLE `newer_release_state`", converted.Destructive[0].DDL) assert.Equal(t, "DROP TABLE destroys data", converted.Destructive[0].Reason) + assert.Equal(t, apitypes.BootRemovalPreserves, converted.BootRemovalPolicy, + "what a boot does to surplus state is what the refusal above means for an operator, so it has to survive the conversion") } // A convergence permitted to run destructive statements runs them, so they diff --git a/pkg/api/storage_schema_source.go b/pkg/api/storage_schema_source.go index 677c9783d..a9a6cf901 100644 --- a/pkg/api/storage_schema_source.go +++ b/pkg/api/storage_schema_source.go @@ -26,10 +26,13 @@ import ( // report that did not name its desired side would leave an operator holding // the wrong one with no way to tell. // -// A supplied schema is a diff-only input. ApplyStorageSchema takes no source -// and has no way to accept one: a convergence runs the schema of the binary -// running it, or "apply is what a boot does" — the property that makes this -// usable as a pre-deploy step — stops being true (AV-9). +// A convergence takes the same input, for the same reason: an operator rolling +// a later release converges the storage to that release before its first pod +// starts, which is the whole point of having this as a command. Supplying the +// files moves which schema runs and nothing else — the destructive refusal and +// the manual-remediation gate decide exactly as they decide for a boot, so no +// supplied set can cost the storage a table that nobody permitted losing +// (AV-9). // storageSchemaNamespace is the namespace the storage schema files declare. // EnsureSchema passes the same value to the engine; naming it once keeps the @@ -79,12 +82,29 @@ func EmbeddedStorageSchema(version string) *StorageSchemaSource { // // What it does not do is establish that a readable set is *complete*, because // nothing here can: a set is a map, and a map has no way to say what is -// missing from it. Two things keep that from being a safety hole rather than a -// caveat. The two selectors that reach this both produce a complete set by -// construction — a directory listing and a release listing, each an error if it -// fetches partially — and no convergence can consume a supplied set at all -// (AV-9), so the worst an incomplete one produces is a report that overstates -// what is surplus, never a database that has lost a table. +// missing from it. A convergence can consume a supplied set, so what an +// incomplete one costs is a real question rather than a hypothetical, and the +// two dialects answer it differently enough to be worth stating separately. +// +// What both rest on is that the selectors reaching here produce a complete set +// by construction — a directory listing and a release listing, each an error +// if it fetches partially. Assembling a partial set means going around them, +// through the API. +// +// On MySQL an omitted table is reported as surplus, which is a statement that +// would drop it. That is destructive, so it is refused unless destroying +// storage state was explicitly permitted (AV-9), and a convergence to a schema +// the running binary does not carry is confirmed by an operator who is shown +// those statements first. An incomplete set therefore costs a report that +// overstates what is surplus, and it takes a separate, explicit permission +// before it costs a table. +// +// On PostgreSQL the convergence is additive-only and walks only the tables the +// set supplies, so an omitted table is not reported at all. Nothing is dropped, +// and the cost is the other way round: a set missing most of a release's files +// can plan as converged, so a caller assembling its own files is answering for +// the completeness of what it sent. The selectors are what make that answer +// true for every operator-facing path. func StorageSchemaFromFiles(description string, files map[string]string) (*StorageSchemaSource, error) { description = strings.TrimSpace(description) if description == "" { diff --git a/pkg/api/storage_schema_test.go b/pkg/api/storage_schema_test.go index fdf69c90f..a86e80529 100644 --- a/pkg/api/storage_schema_test.go +++ b/pkg/api/storage_schema_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/block/schemabot/pkg/apitypes" "github.com/block/schemabot/pkg/ddl" "github.com/block/schemabot/pkg/schema" ) @@ -44,12 +45,74 @@ func TestNewEnsureSchemaOptions_Defaults(t *testing.T) { assert.Equal(t, DefaultPostgresStatementTimeout, defaults.postgresStatementTimeout) assert.False(t, defaults.allowDestructive, "destructive storage changes are refused unless asked for") + assert.Equal(t, DestructivePolicyUnknown, defaults.deploymentDestructive, + "and a caller that named no deployment policy is not assumed to have read one") + configured := newEnsureSchemaOptions( WithDialect(schema.DialectPostgres), - WithAllowDestructiveSchemaChanges(true), + WithDestructiveSchemaChangePolicy(DestructivePolicyPermits, false), WithPostgresStatementTimeout(0), ) assert.Equal(t, schema.DialectPostgres, configured.dialect) assert.True(t, configured.allowDestructive) assert.Zero(t, configured.postgresStatementTimeout, "zero disables the statement budget explicitly") } + +// A caller's opt-in widens the deployment's standing policy for the run it +// asked for, and says nothing about the deployment. The two are tracked apart +// because a report has to be able to state what the next pod to boot does, +// which no per-request flag moves: a convergence run with the flag against a +// deployment that has not configured it leaves surplus state that the +// deployment's own boots still refuse to drop. +func TestWithDestructiveSchemaChangePolicy_RequestWidensOnlyThisRun(t *testing.T) { + forbidding := newEnsureSchemaOptions(WithDestructiveSchemaChangePolicy(DestructivePolicyForbids, false)) + assert.False(t, forbidding.allowDestructive) + assert.Equal(t, DestructivePolicyForbids, forbidding.deploymentDestructive) + + requestOnly := newEnsureSchemaOptions(WithDestructiveSchemaChangePolicy(DestructivePolicyForbids, true)) + assert.True(t, requestOnly.allowDestructive, "the opt-in widens what this run may execute") + assert.Equal(t, DestructivePolicyForbids, requestOnly.deploymentDestructive, + "and leaves the deployment's own policy exactly where it was") + + deploymentOnly := newEnsureSchemaOptions(WithDestructiveSchemaChangePolicy(DestructivePolicyPermits, false)) + assert.True(t, deploymentOnly.allowDestructive, + "a standing policy is never narrowed by a request that did not mention it") + assert.Equal(t, DestructivePolicyPermits, deploymentOnly.deploymentDestructive) + + both := newEnsureSchemaOptions(WithDestructiveSchemaChangePolicy(DestructivePolicyPermits, true)) + assert.True(t, both.allowDestructive) + assert.Equal(t, DestructivePolicyPermits, both.deploymentDestructive) +} + +// An unread deployment policy permits nothing, so it never widens what runs, +// and it stays distinguishable from a policy that was read and forbids. +// +// The execution half matters because a convergence addressed by DSN must not +// acquire permissions from the fact that nobody could say what it had. The +// reporting half matters because the two answers differ exactly where an +// operator is deciding whether to pre-apply: one deployment keeps what they +// converge, and the other may drop it. +func TestWithDestructiveSchemaChangePolicy_UnknownPermitsNothingAndStaysUnknown(t *testing.T) { + unknown := newEnsureSchemaOptions(WithDestructiveSchemaChangePolicy(DestructivePolicyUnknown, false)) + assert.False(t, unknown.allowDestructive, "an unread policy grants nothing") + assert.Equal(t, DestructivePolicyUnknown, unknown.deploymentDestructive) + assert.NotEqual(t, DestructivePolicyForbids, unknown.deploymentDestructive, + "not knowing is not the same answer as knowing it forbids") + + widened := newEnsureSchemaOptions(WithDestructiveSchemaChangePolicy(DestructivePolicyUnknown, true)) + assert.True(t, widened.allowDestructive, "--allow-unsafe is still the way to widen it") + assert.Equal(t, DestructivePolicyUnknown, widened.deploymentDestructive, + "and widening this run still says nothing about the deployment") + + assert.Equal(t, DestructivePolicyPermits, ConfiguredDestructivePolicy(true)) + assert.Equal(t, DestructivePolicyForbids, ConfiguredDestructivePolicy(false), + "a config that was read and says no is a real answer, unlike the absence of one") +} + +// The report says what a boot does, which on MySQL is the deployment's policy +// and never a default standing in for one that could not be read. +func TestMySQLBootRemovalPolicy(t *testing.T) { + assert.Equal(t, apitypes.BootRemovalRemoves, mysqlBootRemovalPolicy(DestructivePolicyPermits)) + assert.Equal(t, apitypes.BootRemovalPreserves, mysqlBootRemovalPolicy(DestructivePolicyForbids)) + assert.Equal(t, apitypes.BootRemovalUnknown, mysqlBootRemovalPolicy(DestructivePolicyUnknown)) +} diff --git a/pkg/apitypes/storage_schema.go b/pkg/apitypes/storage_schema.go index 765610b79..a8fe6ad6c 100644 --- a/pkg/apitypes/storage_schema.go +++ b/pkg/apitypes/storage_schema.go @@ -12,6 +12,26 @@ type StorageSchemaStatement struct { Reason string `json:"reason,omitempty"` } +// BootRemovalPolicy is what a boot of the answering deployment does to storage +// state its own schema does not declare. +type BootRemovalPolicy string + +const ( + // BootRemovalUnknown is the answering side declining to say, and it is a + // real answer rather than a default. A release that predates the field + // leaves it empty, and so does a convergence addressed by DSN alone, which + // has no deployment config to read. Treating it as BootRemovalPreserves is + // how an operator gets told to pre-apply storage the next pod will drop, so + // callers state the uncertainty instead of resolving it. + BootRemovalUnknown BootRemovalPolicy = "" + // BootRemovalPreserves is a boot leaving surplus storage state in place: it + // either refuses the removals, or never computes one. + BootRemovalPreserves BootRemovalPolicy = "preserves" + // BootRemovalRemoves is a boot dropping the tables, columns and indexes its + // own schema does not declare. + BootRemovalRemoves BootRemovalPolicy = "removes" +) + // StorageSchemaReport is what one SchemaBot instance's storage database needs // in order to match that instance's embedded schema. // @@ -44,11 +64,23 @@ type StorageSchemaReport struct { Version string `json:"version,omitempty"` // Converged reports that the storage schema needs nothing at all. A report // carrying only refused destructive statements is not converged. - Converged bool `json:"converged"` - Outstanding []StorageSchemaStatement `json:"outstanding,omitempty"` - Destructive []StorageSchemaStatement `json:"destructive,omitempty"` + Converged bool `json:"converged"` + Outstanding []StorageSchemaStatement `json:"outstanding,omitempty"` + Destructive []StorageSchemaStatement `json:"destructive,omitempty"` + // DestructiveAllowed reports whether the destructive statements run on this + // call: the deployment's standing policy, widened by an opt-in this caller + // sent. BootRemovalPolicy is the one to read for what some other process + // does. DestructiveAllowed bool `json:"destructive_allowed"` Manual []StorageSchemaStatement `json:"manual,omitempty"` + // BootRemovalPolicy is what the next pod to start does to storage state its + // own schema does not declare, which is what says whether state converged + // ahead of a deploy survives until that deploy. + // + // It is the deployment's policy and its dialect, never this request's + // opt-in. Empty is BootRemovalUnknown and is a real answer: see the + // constants. + BootRemovalPolicy BootRemovalPolicy `json:"boot_removal_policy,omitempty"` // ConvergenceInFlight reports that some instance held the storage bootstrap // lock when the diff was taken — a pod booting, or another operator's // apply. It is what separates "this DDL is outstanding" from "this DDL is diff --git a/pkg/apitypes/storage_schema_requests.go b/pkg/apitypes/storage_schema_requests.go index 6a6c71718..8e20cb0e1 100644 --- a/pkg/apitypes/storage_schema_requests.go +++ b/pkg/apitypes/storage_schema_requests.go @@ -40,6 +40,11 @@ type StorageSchemaPlanResponse struct { // StorageSchemaApplyRequest is the HTTP request for // POST /api/storage/schema/apply. +// +// Every field is optional. The defaults converge the addressed server's own +// storage to its own embedded schema, which is what its next boot would +// converge; SchemaFiles is how an operator converges it to a release that is +// about to roll instead. type StorageSchemaApplyRequest struct { // Deployment names the data plane whose storage to converge. Empty // converges the storage of the server the request is made to. @@ -68,6 +73,13 @@ type StorageSchemaApplyRequest struct { // fails its own lock wait. A value above the target's maximum is refused // rather than clamped. TimeoutSeconds int64 `json:"timeout_seconds,omitempty"` + // SchemaFiles is the schema to converge to, as file name → file contents. + // Empty converges the answering binary's own embedded schema. + SchemaFiles map[string]string `json:"schema_files,omitempty"` + // SchemaSource says where SchemaFiles came from, in words, for the reports + // to attribute the convergence to. Required with SchemaFiles and never + // inferred. + SchemaSource string `json:"schema_source,omitempty"` } // StorageSchemaApplyResponse is the HTTP response for diff --git a/pkg/cmd/commands/common.go b/pkg/cmd/commands/common.go index a4a76d39b..9a6bd0ce2 100644 --- a/pkg/cmd/commands/common.go +++ b/pkg/cmd/commands/common.go @@ -139,7 +139,45 @@ func resolveEndpoint(endpoint, profile string) (string, error) { // confirmAction prompts the user for "yes" confirmation. Returns true if confirmed. func confirmAction(prompt, cancelMsg string) (bool, error) { - fmt.Print(prompt) + return confirmActionOn(os.Stdout, prompt, cancelMsg) +} + +// writeToTerminal runs fn with the human-facing renderers writing to stderr +// instead of stdout, when the command was asked for machine-readable output. +// With divert false it just runs fn, so a caller can wrap unconditionally. +// +// A command under --json owes stdout to the program reading it, and still owes +// a person at the terminal everything they are being asked to approve. Those +// are two audiences, not a choice between them: the plan goes to one and the +// response to the other. The renderers print through fmt.Print, which resolves +// os.Stdout per call, so pointing it at stderr for the duration is what moves +// them; nothing writes the response until after it is restored. +func writeToTerminal(divert bool, fn func() error) error { + if !divert { + return fn() + } + restore := os.Stdout + os.Stdout = os.Stderr + defer func() { os.Stdout = restore }() + return fn() +} + +// confirmActionOn is confirmAction with the prompt written somewhere other than +// stdout. A command asked for machine-readable output owes stdout to the +// program reading it, and still has to ask a person before it converges, so the +// conversation goes to stderr and the answer stays parseable. +func confirmActionOn(out io.Writer, prompt, cancelMsg string) (bool, error) { + // A prompt nobody can be shown is not a prompt, so a write that fails is + // an error rather than an unanswered question read from stdin anyway. + if _, err := fmt.Fprint(out, prompt); err != nil { + return false, fmt.Errorf("write confirmation prompt: %w", err) + } + cancelled := func() error { + if _, err := fmt.Fprintln(out, cancelMsg); err != nil { + return fmt.Errorf("write cancellation notice: %w", err) + } + return nil + } sigCh := make(chan os.Signal, 1) signal.Notify(sigCh, os.Interrupt) @@ -158,8 +196,7 @@ func confirmAction(prompt, cancelMsg string) (bool, error) { select { case <-sigCh: - fmt.Println(cancelMsg) - return false, nil + return false, cancelled() case r := <-resultCh: // EOF with data is valid (e.g., echo -n yes | schemabot apply) if r.err != nil && !errors.Is(r.err, io.EOF) { @@ -167,12 +204,10 @@ func confirmAction(prompt, cancelMsg string) (bool, error) { } response := strings.TrimSpace(strings.ToLower(r.response)) if errors.Is(r.err, io.EOF) && response == "" { - fmt.Println(cancelMsg) - return false, nil + return false, cancelled() } if response != "yes" { - fmt.Println(cancelMsg) - return false, nil + return false, cancelled() } return true, nil } diff --git a/pkg/cmd/commands/storage_schema.go b/pkg/cmd/commands/storage_schema.go index 0b1b41fdb..03d3f9709 100644 --- a/pkg/cmd/commands/storage_schema.go +++ b/pkg/cmd/commands/storage_schema.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "io" "log/slog" "os" "strings" @@ -51,6 +52,30 @@ type storageSchemaTargetFlags struct { DSN string `help:"Connect to the storage database directly with this DSN instead of going through the API; for when the server is down"` Config string `help:"Server config file to resolve the storage DSN from, connecting directly instead of going through the API"` Dialect string `help:"Storage database family of --dsn (mysql or postgres) when its form does not say"` + // resolvedTarget is the storage database these flags name, once it has been + // resolved. It sits on the flags rather than on a command because a command + // that hands the target off hands these flags off with it, so the copy + // carries the resolution and cannot resolve a second one. + resolvedTarget *storageTarget `kong:"-"` +} + +// target is the storage database these flags address, resolved at most once. +// +// One resolution per run is the point. A config using storage.dsn_from resolves +// its DSN from secret references, so every resolution is a call to someone +// else's system, with its own audit trail and its own chance to answer +// differently than it did a moment ago. A convergence that resolved again could +// run against a database its own preview never looked at. +func (f *storageSchemaTargetFlags) target() (*storageTarget, error) { + if f.resolvedTarget != nil { + return f.resolvedTarget, nil + } + target, err := resolveStorageTarget(f.DSN, f.Config, f.Dialect) + if err != nil { + return nil, err + } + f.resolvedTarget = target + return target, nil } // direct reports whether the operator asked for a direct connection. Passing @@ -128,6 +153,11 @@ type StoragePlanCmd struct { // standing storage policy can allow destructive changes with no flag on the // line at all. AllowUnsafe bool `kong:"-"` + // resolved is a schema the caller has already read from the selectors, + // used instead of reading them again. The apply's preview sets it so the + // plan an operator approves is computed from the same bytes the + // convergence then runs. + resolved *api.StorageSchemaSource `kong:"-"` } func (cmd *StoragePlanCmd) Run(ctx context.Context, g *Globals) error { @@ -147,7 +177,7 @@ func (cmd *StoragePlanCmd) Run(ctx context.Context, g *Globals) error { if err := encoder.Encode(apitypes.StorageSchemaPlanResponse{Report: report}); err != nil { return fmt.Errorf("encode storage schema report: %w", err) } - } else if err := outputStorageSchemaPlan(report, false, "", nil); err != nil { + } else if err := outputStorageSchemaPlan(report, false, "", storageSchemaPlanHints(report)); err != nil { return err } if report.Converged { @@ -168,13 +198,13 @@ func (cmd *StoragePlanCmd) read(ctx context.Context, g *Globals) (*apitypes.Stor // here, for when the server is down — including when it is down because its own // schema bootstrap is failing. func (cmd *StoragePlanCmd) readDirect(ctx context.Context, g *Globals) (*apitypes.StorageSchemaReport, error) { - target, err := resolveStorageTarget(cmd.DSN, cmd.Config, cmd.Dialect) + target, err := cmd.target() if err != nil { return nil, err } // The dialect is already resolved on this path, so a release fetch costs no // extra round trip. - desired, err := cmd.resolve(ctx, func() (schema.Dialect, error) { return target.dialect, nil }) + desired, err := cmd.desiredSchema(ctx, func() (schema.Dialect, error) { return target.dialect, nil }) if err != nil { return nil, err } @@ -205,7 +235,7 @@ func (cmd *StoragePlanCmd) readThroughAPI(ctx context.Context, g *Globals) (*api Environment: cmd.Environment, AllowDestructive: cmd.AllowUnsafe, } - desired, err := cmd.resolve(ctx, func() (schema.Dialect, error) { + desired, err := cmd.desiredSchema(ctx, func() (schema.Dialect, error) { return cmd.dialectThroughAPI(ctx, endpoint) }) if err != nil { @@ -226,6 +256,15 @@ func (cmd *StoragePlanCmd) readThroughAPI(ctx context.Context, g *Globals) (*api return response.Report, nil } +// desiredSchema is the schema this plan compares against: one the caller +// already read, or the selectors read now. +func (cmd *StoragePlanCmd) desiredSchema(ctx context.Context, dialect func() (schema.Dialect, error)) (*api.StorageSchemaSource, error) { + if cmd.resolved != nil { + return cmd.resolved, nil + } + return cmd.resolve(ctx, dialect, false) +} + // dialectThroughAPI asks the target which storage family it runs, so a release // fetch reads the right one of its schema directories. // @@ -255,12 +294,20 @@ func (cmd *StoragePlanCmd) dialectThroughAPI(ctx context.Context, endpoint strin // same refusal of destructive statements, and the same advisory lock — so two // operators running this at once serialize exactly the way two booting pods // do, and a pre-deploy convergence step is this command with nothing added. -// It converges to the schema of the binary that runs it, and there is no flag -// to point it at another release's — see storageSchemaSourceRefusal. +// +// Which schema it converges to is the operator's to name. Named nothing, it +// converges the answering binary's own embedded files, which is what that +// binary's next boot converges. Named a release, it converges that release's +// files — the reason the command exists, because the storage a release needs +// has to be there before the first pod of that release starts, and the pod +// that would converge it is the one that cannot start until it is. Naming a +// release is confirmed at a terminal and never runs unattended; see +// blockUnattendedNamedSchema and storageSchemaConfirmation. type StorageApplyCmd struct { storageSchemaTargetFlags `embed:""` + storageSchemaSourceFlags `embed:""` AllowUnsafe bool `help:"Permit the destructive statements the convergence would otherwise refuse; it widens the target's standing storage policy and never narrows it" name:"allow-unsafe"` - AutoApprove bool `short:"y" help:"Skip confirmation prompt" name:"auto-approve"` + AutoApprove bool `short:"y" help:"Skip confirmation prompt; refused with --release or --schema-dir, which are always confirmed at a terminal" name:"auto-approve"` JSON bool `help:"Output as JSON"` // Timeout is how a convergence outlives the budget a boot runs under. A // booting pod gives up after minutes because a pod converging is a pod not @@ -268,23 +315,19 @@ type StorageApplyCmd struct { // to. Left unset, the operator default is named on its behalf, so the // target always runs the budget this command is waiting for. Timeout time.Duration `help:"Bound the whole convergence — the lock wait, the diff under it, and the DDL. Defaults to the operator budget, which is already far above a booting pod's. Raising it holds the storage bootstrap lock for that long, and pods booting in the window will not come up" name:"timeout"` - // The diff's file selectors are accepted here only to be refused with the - // reason and the alternative. An operator who has just run the diff against - // a release reaches for the same flags on the apply, and Kong's bare - // "unknown flag" would leave them guessing at whether the convergence - // silently used a different schema. --release-repo is accepted for the same - // reason: it is the flag most likely to be left on the line after the one - // it modifies has been dropped. - SchemaDir string `hidden:"" name:"schema-dir"` - Release string `hidden:""` - Repo string `hidden:"" name:"release-repo"` } func (cmd *StorageApplyCmd) Run(ctx context.Context, g *Globals) error { if err := cmd.validate(); err != nil { return err } - if err := storageSchemaSourceRefusal(cmd.SchemaDir, cmd.Release, cmd.Repo); err != nil { + if err := cmd.validateSourceCombination(); err != nil { + return err + } + // At the door, before anything is read: this refuses a command form, not + // something a plan could have found, so there is nothing to learn from the + // database first. + if err := blockUnattendedNamedSchema(cmd.namedSource(), cmd.AutoApprove); err != nil { return err } // Checked before the preview rather than on the way to the convergence: a @@ -301,21 +344,42 @@ func (cmd *StorageApplyCmd) Run(ctx context.Context, g *Globals) error { // statements, and the read is free of side effects. Unattended, it is what // the destructive gate below decides from. // - // No selector on the preview asks the target about its own embedded schema, - // which is the schema this convergence is about to run. + // It is built before anything is read so that its target flags are the only + // ones anything here uses: the schema a named release resolves for, the + // plan, and the convergence all take their storage database from this one + // value, which resolves it once and remembers it (see + // storageSchemaTargetFlags.target). preview := &StoragePlanCmd{ storageSchemaTargetFlags: cmd.storageSchemaTargetFlags, + storageSchemaSourceFlags: cmd.storageSchemaSourceFlags, AllowUnsafe: cmd.AllowUnsafe, } + flags := &preview.storageSchemaTargetFlags + + // The schema is resolved once and used for the preview, the confirmation + // and the convergence. Resolving it again for the convergence would open a + // window where a moved tag or an edited directory made the operator + // approve one file set and converge another. + desired, err := cmd.resolveDesired(ctx, g, flags) + if err != nil { + return err + } + preview.resolved = desired + report, err := preview.read(ctx, g) if err != nil { return err } // With --auto-approve the convergence's own planned report says what ran, // so the plan is printed after the fact instead — unless the gate stops the - // run, which prints it itself. + // run, which prints it itself. Under --json it goes to the terminal rather + // than into the stream being parsed: the statements are what the person at + // the prompt is being asked to approve, so the one thing this must never do + // is skip them. if !cmd.AutoApprove { - if err := outputStorageSchemaPlan(report, true, cmd.rerunWithAllowUnsafe(), nil); err != nil { + if err := writeToTerminal(cmd.JSON, func() error { + return outputStorageSchemaPlan(report, true, cmd.rerunWithAllowUnsafe(), nil) + }); err != nil { return err } } @@ -324,35 +388,48 @@ func (cmd *StorageApplyCmd) Run(ctx context.Context, g *Globals) error { // operator and a pre-deploy job are told the same thing. Manual first: it // gates the whole drift set, which makes a destructive statement behind it // unreachable rather than merely refused. - if err := blockManualStorageApply(report, cmd.AutoApprove, cmd.rerunWithAllowUnsafe()); err != nil { - return err + // + // A --json run is read by a program, so the gates print no plan on it and + // the refusal is reported as the response shape instead. + gatePrintsPlan := cmd.AutoApprove && !cmd.JSON + if err := blockManualStorageApply(report, gatePrintsPlan, cmd.rerunWithAllowUnsafe()); err != nil { + return cmd.reportRefusal(report, err) } - if err := blockDestructiveStorageApply(report, cmd.AutoApprove, cmd.rerunWithAllowUnsafe()); err != nil { - return err + if err := blockDestructiveStorageApply(report, gatePrintsPlan, cmd.rerunWithAllowUnsafe()); err != nil { + return cmd.reportRefusal(report, err) } if !cmd.AutoApprove { - confirmed, err := confirmAction( - storageSchemaConfirmation(report), + prompts := io.Writer(os.Stdout) + if cmd.JSON { + prompts = os.Stderr + } + confirmed, err := confirmActionOn( + prompts, + storageSchemaConfirmation(report, cmd.namedSource() != ""), "\nApply cancelled.", ) if err != nil { return err } if !confirmed { + // A decline is an outcome a program has to be able to read, and it + // leaves the database exactly as the plan found it — the same shape + // a refusal answers with, for the same reason. + if cmd.JSON { + return encodeStorageSchemaConvergence(report, report) + } return nil } } - planned, remaining, err := cmd.converge(ctx, g) + planned, remaining, err := cmd.converge(ctx, g, flags, desired) if err != nil { return err } if cmd.JSON { - encoder := json.NewEncoder(os.Stdout) - encoder.SetIndent("", " ") - if err := encoder.Encode(apitypes.StorageSchemaApplyResponse{Planned: planned, Remaining: remaining}); err != nil { - return fmt.Errorf("encode storage schema convergence: %w", err) + if err := encodeStorageSchemaConvergence(planned, remaining); err != nil { + return err } } else if err := outputStorageSchemaConvergence(planned, remaining, cmd.AutoApprove, cmd.rerunWithAllowUnsafe()); err != nil { return err @@ -360,6 +437,60 @@ func (cmd *StorageApplyCmd) Run(ctx context.Context, g *Globals) error { return storageSchemaConvergenceOutcome(remaining) } +// reportRefusal hands a refused convergence back in the shape the caller asked +// for, and returns the refusal either way. +// +// A refusal under --json is the case a program most needs to read: nothing ran, +// so every statement the plan found is still outstanding, which is why the +// planned and remaining halves are the same report. Without this the run +// printed a human plan on a surface a program parses, or on the destructive +// gate nothing at all, and a caller could not tell a refusal from a crash. +func (cmd *StorageApplyCmd) reportRefusal(report *apitypes.StorageSchemaReport, refusal error) error { + if !cmd.JSON { + return refusal + } + // An encode failure is the answer to why there is no report on stdout, so it + // is the error to return: the refusal it would have carried is in the + // statements the plan already holds, and the command fails on either. + if err := encodeStorageSchemaConvergence(report, report); err != nil { + return err + } + return refusal +} + +func encodeStorageSchemaConvergence(planned, remaining *apitypes.StorageSchemaReport) error { + encoder := json.NewEncoder(os.Stdout) + encoder.SetIndent("", " ") + if err := encoder.Encode(apitypes.StorageSchemaApplyResponse{Planned: planned, Remaining: remaining}); err != nil { + return fmt.Errorf("encode storage schema convergence: %w", err) + } + return nil +} + +// blockUnattendedNamedSchema refuses to converge a named release's schema +// without a person watching. +// +// Naming a schema is the one thing about this command that a boot cannot check +// for itself. Every other decision here is the bootstrap's, made the same way +// on the same files whoever asked; this one substitutes files the answering +// binary never carried, so the fleet's own boots will not agree with what was +// just converged until the release that carries them is deployed. That +// disagreement is safe and expected, and it is also the kind of thing an +// operator must be looking at when it starts — so the confirmation is the +// consent, and --auto-approve is not a way to give it (AV-9). +// +// Refusing the combination is stricter than refusing at the prompt: a +// pre-deploy job cannot converge a release's schema at all. That is the right +// default for a surface whose whole value is that the fleet and the storage +// agree, and an unattended convergence of the answering binary's own schema — +// the pre-deploy step this command was built for — is untouched by it. +func blockUnattendedNamedSchema(selector string, autoApprove bool) error { + if selector == "" || !autoApprove { + return nil + } + return fmt.Errorf("%s cannot be combined with --auto-approve: converging storage to a schema this binary does not carry is confirmed at a terminal, because until that release is deployed the running one will not converge the same schema. Run it without --auto-approve and answer the prompt, or drop %s to converge what this binary's own next boot would", selector, selector) +} + // rerunWithAllowUnsafe is the command that permits what this one refused, for // an operator to copy off a refusal. // @@ -368,6 +499,13 @@ func (cmd *StorageApplyCmd) Run(ctx context.Context, g *Globals) error { // convergence of the storage of the server the CLI happens to point at, which // during a rollback is a different database than the one being looked at. // +// It carries the schema selector forward for a stronger version of the same +// reason. The refused statements were computed against the schema the operator +// named, so a command that dropped the selector would permit destructive +// changes while converging a different schema than the one the refusal was +// about — and on a cross-release convergence that is the schema of a different +// release. +// // A DSN is named rather than repeated: it carries the storage database's // credentials, and this is printed to a terminal and scrolled back through. func (cmd *StorageApplyCmd) rerunWithAllowUnsafe() string { @@ -389,29 +527,18 @@ func (cmd *StorageApplyCmd) rerunWithAllowUnsafe() string { if cmd.Timeout > 0 { parts = append(parts, "--timeout", cmd.Timeout.String()) } + switch { + case strings.TrimSpace(cmd.Release) != "": + parts = append(parts, "--release", cmd.Release) + if strings.TrimSpace(cmd.Repo) != "" { + parts = append(parts, "--release-repo", cmd.Repo) + } + case strings.TrimSpace(cmd.SchemaDir) != "": + parts = append(parts, "--schema-dir", cmd.SchemaDir) + } return strings.Join(append(parts, "--allow-unsafe"), " ") } -// blockDestructiveStorageApply stops a convergence that would have to destroy -// storage state nothing has permitted, before it runs anything. -// -// This is the gate `apply` puts in front of a destructive schema change, in the -// same place and behind the same flag: the plan is on screen, the statements -// are named, and --allow-unsafe is the way through. It is in front of the -// confirmation rather than after it because --auto-approve skips a prompt, and -// consenting to a convergence is not consenting to destroy state. -// -// A deployment that already allows destructive storage changes has permitted -// them, so there is nothing here to ask: the report says so and the statements -// run. The gate narrows no standing policy (AV-9), and where it does stop a run -// it runs strictly less than the convergence would have — the bootstrap refuses -// the same statements on its own and converges the safe remainder. What changes -// is when the operator finds out: before a DROP against SchemaBot's own storage -// is decided, rather than in a report of what was already done. -// -// withPlan prints the plan for an unattended run, which has not printed one -// yet. Naming refused statements without showing them would send the operator -// back to `storage plan` to find out what was refused. // blockManualStorageApply stops a convergence that cannot run at all. // // A manual entry gates the whole drift set: the bootstrap refuses every @@ -436,6 +563,26 @@ func blockManualStorageApply(report *apitypes.StorageSchemaReport, withPlan bool storageSchemaDatabaseLabel(report), len(report.Manual)) } +// blockDestructiveStorageApply stops a convergence that would have to destroy +// storage state nothing has permitted, before it runs anything. +// +// This is the gate `apply` puts in front of a destructive schema change, in the +// same place and behind the same flag: the plan is on screen, the statements +// are named, and --allow-unsafe is the way through. It is in front of the +// confirmation rather than after it because --auto-approve skips a prompt, and +// consenting to a convergence is not consenting to destroy state. +// +// A deployment that already allows destructive storage changes has permitted +// them, so there is nothing here to ask: the report says so and the statements +// run. The gate narrows no standing policy (AV-9), and where it does stop a run +// it runs strictly less than the convergence would have — the bootstrap refuses +// the same statements on its own and converges the safe remainder. What changes +// is when the operator finds out: before a DROP against SchemaBot's own storage +// is decided, rather than in a report of what was already done. +// +// withPlan prints the plan for an unattended run, which has not printed one +// yet. Naming refused statements without showing them would send the operator +// back to `storage plan` to find out what was refused. func blockDestructiveStorageApply(report *apitypes.StorageSchemaReport, withPlan bool, rerun string) error { if report.DestructiveAllowed || len(report.Destructive) == 0 { return nil @@ -461,12 +608,112 @@ func blockDestructiveStorageApply(report *apitypes.StorageSchemaReport, withPlan // here would leave them on the database and make an interactive apply do less // than the same command with --auto-approve — and the one an operator reaches // for mid-incident is the interactive one. -func storageSchemaConfirmation(report *apitypes.StorageSchemaReport) string { +// +// namedSchema says the operator named a schema rather than taking the +// answering binary's own, which changes what they are consenting to and gets +// the notice that says how. +func storageSchemaConfirmation(report *apitypes.StorageSchemaReport, namedSchema bool) string { label := storageSchemaDatabaseLabel(report) + notice := "" + if namedSchema { + notice = crossReleaseStorageNotice(report) + } if report.Converged { - return fmt.Sprintf("\nThe catalog of %s already matches. Run the bootstrap anyway, to clear any engine state a catalog diff cannot see? Only 'yes' will be accepted: ", label) + return fmt.Sprintf("\n%sThe catalog of %s already matches. Run the bootstrap anyway, to clear any engine state a catalog diff cannot see? Only 'yes' will be accepted: ", notice, label) + } + return fmt.Sprintf("\n%sDo you want to apply these changes to %s? Only 'yes' will be accepted: ", notice, label) +} + +// crossReleaseStorageNotice states the consequences of converging a schema the +// deployed release does not carry, which an operator has no other way to find +// out. +// +// A boot of the deployed release diffs this schema against its own, so +// everything converged here that the deployed release does not declare is a +// removal its bootstrap has to decide about. What it decides is the whole +// content of this notice, and it is not one answer: it depends on whether the +// deployment permits destructive storage changes, and on the dialect. The plan +// carries both, and shows what the storage needs rather than what will undo +// it, so neither is visible to an operator reading it. +func crossReleaseStorageNotice(report *apitypes.StorageSchemaReport) string { + running := "the release answering this command" + if version := strings.TrimSpace(report.Version); version != "" { + running = version + } + return fmt.Sprintf("Converging %s to %s.\n\n%s\n\n", storageSchemaDatabaseLabel(report), report.SchemaSource, + crossReleaseConsequence(report, running)) +} + +// crossReleaseConsequence is what the deployed release does to the state this +// convergence leaves behind, for the deployment and dialect the report +// describes. +// +// It reads BootRemovalPolicy and never DestructiveAllowed. The subject is what +// happens after this command exits, when the only thing still converging is a +// pod starting, and a pod converges the deployment's standing policy. The +// effective policy is this command's alone: an operator who passed +// --allow-unsafe widened what their own convergence runs and moved nothing +// about what the fleet's boots do, so reading it here would report a +// deployment's behavior from a flag the deployment never saw. +// +// An unknown policy gets its own answer rather than the reassuring one. It +// arrives from a release too old to report the field and from a database +// addressed by DSN alone, and in both cases the deployment behind it may be +// either kind. Survival is the claim an operator acts on by pre-applying, so it +// is the claim that has to be earned. +// +// PostgreSQL is the one case a dialect answers on its own, so it is read before +// the policy: an additive-only bootstrap preserves surplus state on every +// release, including the ones too old to say so. +func crossReleaseConsequence(report *apitypes.StorageSchemaReport, running string) string { + // PostgreSQL's bootstrap is additive-only: it walks what its own schema + // declares and never computes a removal, so surplus state is not refused so + // much as never considered. There is no refusal to log, and no release with + // a gap to warn about. + if schema.Dialect(report.Dialect) == schema.DialectPostgres { + return fmt.Sprintf(` This is not the schema %s converges on boot. Until that release is deployed, + every pod that boots %s converges only what its own schema adds, so the + tables, columns and indexes applied here survive untouched — and the boot + says nothing about them, because it never considers removing them.`, running, running) + } + + switch report.BootRemovalPolicy { + // A deployment whose boots remove surplus state drops what this leaves + // rather than refusing it. The convergence is still worth running as part + // of a deploy; what it is not is something to do in advance, which is the + // reason an operator reaches for it. + case apitypes.BootRemovalRemoves: + return fmt.Sprintf(` This is not the schema %s converges on boot, and this deployment permits + destructive storage changes — so it will not leave what is applied here in + place. The next pod to boot %s drops the tables, columns and indexes its own + schema does not declare, which is every one of them until that release is + deployed. Converge as part of the deploy rather than ahead of it.`, running, running) + + case apitypes.BootRemovalPreserves: + return fmt.Sprintf(` This is not the schema %s converges on boot. Until that release is deployed, + every pod that boots %s refuses to drop what it does not declare, so the + tables, columns and indexes applied here survive — and every one of those + boots logs a refused destructive change for them. A release from before + indexes were protected is the exception: its boots converge a surplus index + away. Re-run `+"`storage plan`"+` just before the deploy to confirm what you + applied is still there.`, running, running) + + // Unknown, and every policy a later release adds that this binary has no + // text for. The CLI is versioned apart from the server it dials, so the + // values arriving here grow without it, and the default has to be the + // answer that claims nothing — survival is what an operator acts on by + // pre-applying, so it is never what a value this binary cannot read + // resolves to. + default: + return fmt.Sprintf(` This is not the schema %s converges on boot, and what its pods do with the + difference could not be established: this target was reached without a + deployment config to read, or answered from a release that does not report + it. A deployment permitting destructive storage changes drops the tables, + columns and indexes applied here on the next boot; one that does not keeps + them. Confirm which before relying on this: converge as part of the deploy + instead, or check %s's storage policy and re-run `+"`storage plan`"+` afterwards to + see what survived.`, running, running) } - return fmt.Sprintf("\nDo you want to apply these changes to %s? Only 'yes' will be accepted: ", label) } // storageSchemaConvergenceOutcome is whether a convergence counts as having @@ -529,10 +776,52 @@ func (cmd *StorageApplyCmd) timeoutSeconds() int64 { return int64(cmd.Timeout / time.Second) } -// converge runs the convergence over whichever path the flags selected. -func (cmd *StorageApplyCmd) converge(ctx context.Context, g *Globals) (planned, remaining *apitypes.StorageSchemaReport, err error) { - if cmd.direct() { - target, err := resolveStorageTarget(cmd.DSN, cmd.Config, cmd.Dialect) +// resolveDesired reads the schema this convergence will run, once, before +// anything is planned. +// +// Naming nothing resolves to nil, which is the answering binary's own embedded +// schema — and resolves without asking anything, because there are no files to +// fetch and no dialect to discover. +// +// A named release needs the target's storage dialect, since a release keeps one +// schema directory per family. Where that costs a round trip it is the same +// round trip the plan would have made; hoisting it here buys the guarantee that +// the file set is read once (see StoragePlanCmd.resolved). +func (cmd *StorageApplyCmd) resolveDesired(ctx context.Context, g *Globals, flags *storageSchemaTargetFlags) (*api.StorageSchemaSource, error) { + if cmd.namedSource() == "" { + return nil, nil + } + // One call, so that "these files are about to run" is stated once and + // cannot be true on one path and false on the other. + return cmd.resolve(ctx, dialectOfTarget(ctx, g, flags), true) +} + +// dialectOfTarget asks whichever side owns the storage which family it runs. +// It is a function rather than a value because only --release needs the answer, +// and on the API path the answer costs a round trip. +func dialectOfTarget(ctx context.Context, g *Globals, flags *storageSchemaTargetFlags) func() (schema.Dialect, error) { + return func() (schema.Dialect, error) { + if flags.direct() { + target, err := flags.target() + if err != nil { + return "", err + } + return target.dialect, nil + } + endpoint, err := g.Resolve() + if err != nil { + return "", err + } + probe := &StoragePlanCmd{storageSchemaTargetFlags: *flags} + return probe.dialectThroughAPI(ctx, endpoint) + } +} + +// converge runs the convergence over whichever path the flags selected, against +// the schema already resolved for the plan the operator approved. +func (cmd *StorageApplyCmd) converge(ctx context.Context, g *Globals, flags *storageSchemaTargetFlags, desired *api.StorageSchemaSource) (planned, remaining *apitypes.StorageSchemaReport, err error) { + if flags.direct() { + target, err := flags.target() if err != nil { return nil, nil, err } @@ -540,8 +829,9 @@ func (cmd *StorageApplyCmd) converge(ctx context.Context, g *Globals) (planned, logger.Info("converging storage schema directly", "source", target.source, "dialect", target.dialect, - "allow_destructive", target.allowDestructive || cmd.AllowUnsafe, - "config_allows_destructive", target.allowDestructive) + "schema_source", desired.Describe(), + "allow_destructive", target.destructive == api.DestructivePolicyPermits || cmd.AllowUnsafe, + "deployment_destructive_policy", target.destructive) budget, err := cmd.convergenceBudget() if err != nil { return nil, nil, err @@ -552,7 +842,7 @@ func (cmd *StorageApplyCmd) converge(ctx context.Context, g *Globals) (planned, // document a caller parses, and a progress line in the middle of it is // not a progress line, it is a parse error. progress := newStorageProgressPrinter(os.Stderr, ui.SupportsColors(os.Stderr)) - plannedReport, remainingReport, err := api.ApplyStorageSchema(ctx, target.dsn, logger, + plannedReport, remainingReport, err := api.ApplyStorageSchema(ctx, target.dsn, desired, logger, append(target.ensureSchemaOptions(cmd.AllowUnsafe), api.WithConvergenceTimeout(budget), api.WithConvergenceProgress(progress.observe))...) @@ -567,20 +857,28 @@ func (cmd *StorageApplyCmd) converge(ctx context.Context, g *Globals) (planned, if err != nil { return nil, nil, err } - response, err := cmdclient.StorageSchemaApply(ctx, endpoint, apitypes.StorageSchemaApplyRequest{ - Deployment: cmd.Deployment, - Environment: cmd.Environment, + request := apitypes.StorageSchemaApplyRequest{ + Deployment: flags.Deployment, + Environment: flags.Environment, AllowDestructive: cmd.AllowUnsafe, TimeoutSeconds: cmd.timeoutSeconds(), - }) + } + // A named schema travels with the request, because the answering binary + // does not carry another release's files. Sent as the same pair the plan + // sends, so the convergence is attributed to the release whose files ran. + if desired != nil { + request.SchemaFiles = desired.Files + request.SchemaSource = desired.Description + } + response, err := cmdclient.StorageSchemaApply(ctx, endpoint, request) if err != nil { - return nil, nil, fmt.Errorf("converge storage schema%s: %w", storageSchemaTargetSuffix(cmd.Deployment, cmd.Environment), err) + return nil, nil, fmt.Errorf("converge storage schema%s: %w", storageSchemaTargetSuffix(flags.Deployment, flags.Environment), err) } if response.Planned == nil || response.Remaining == nil { // Both halves are required to say what happened; without the pair there // is no way to tell a convergence that finished from one that left // statements behind. - return nil, nil, fmt.Errorf("storage schema convergence%s returned an incomplete result; check the target's logs for whether it converged", storageSchemaTargetSuffix(cmd.Deployment, cmd.Environment)) + return nil, nil, fmt.Errorf("storage schema convergence%s returned an incomplete result; check the target's logs for whether it converged", storageSchemaTargetSuffix(flags.Deployment, flags.Environment)) } return response.Planned, response.Remaining, nil } @@ -588,11 +886,13 @@ func (cmd *StorageApplyCmd) converge(ctx context.Context, g *Globals) (planned, // attributeStorageSchemaConvergence says which release a convergence ran, on // both halves of it. // -// A convergence names no schema source of its own — that is the invariant, not -// an omission (AV-9) — so the attribution is what turns "the embedded schema" -// into this binary's release. Both halves take it, and by the same call the -// preview took: a run whose plan header named a release and whose result header -// named a placeholder would read as two runs against two schemas. +// A convergence of the answering binary's own schema names no release of its +// own, so the attribution is what turns "the embedded schema" into this +// binary's version; a convergence of files the operator named keeps their +// attribution, which is what AttributeTo already does. Both halves take it, and +// by the same call the preview took: a run whose plan header named a release +// and whose result header named a placeholder would read as two runs against +// two schemas. func attributeStorageSchemaConvergence(version string, planned, remaining *api.StorageSchemaReport) { planned.AttributeTo(version) remaining.AttributeTo(version) diff --git a/pkg/cmd/commands/storage_schema_render.go b/pkg/cmd/commands/storage_schema_render.go index b5f8cfcaa..e75542c07 100644 --- a/pkg/cmd/commands/storage_schema_render.go +++ b/pkg/cmd/commands/storage_schema_render.go @@ -447,3 +447,15 @@ func storageSchemaDatabaseLabel(report *apitypes.StorageSchemaReport) string { } return label } + +// storageSchemaPlanHints names the next step for the plan that was just +// printed. +// +// The next step is the same command with `apply` in place of `plan`, which is +// what an operator will reach for and is the answer: the apply converges the +// schema this plan named, not the one the answering binary happens to carry. +// The hint does not reproduce the operator's flags, because the plan they just +// ran already has them on the line above. +func storageSchemaPlanHints(report *apitypes.StorageSchemaReport) []string { + return []string{fmt.Sprintf("These are what %s needs in order to match %s. To converge them, re-run this as `storage apply` with the same flags.", storageSchemaHeaderDatabase(report), report.SchemaSource)} +} diff --git a/pkg/cmd/commands/storage_schema_render_test.go b/pkg/cmd/commands/storage_schema_render_test.go index 589c5d2e5..1cabb0d9a 100644 --- a/pkg/cmd/commands/storage_schema_render_test.go +++ b/pkg/cmd/commands/storage_schema_render_test.go @@ -28,7 +28,7 @@ func TestOutputStorageSchemaPlan_RendersAsAPlan(t *testing.T) { } out := captureStdout(func() { - require.NoError(t, outputStorageSchemaPlan(report, false, "", nil)) + require.NoError(t, outputStorageSchemaPlan(report, false, "", storageSchemaPlanHints(report))) }) assert.Contains(t, out, "MySQL Schema Change Plan") @@ -37,6 +37,8 @@ func TestOutputStorageSchemaPlan_RendersAsAPlan(t *testing.T) { assert.Contains(t, out, "+ checks") assert.Contains(t, out, "~ applies") assert.Contains(t, out, "📋 Plan: 1 table to create, 1 table to alter") + assert.Contains(t, out, "re-run this as `storage apply` with the same flags", + "the plan names the next step, which is the same command with the same schema selector") assert.NotContains(t, out, "(mysql)", "the title already names the family; repeating it in the database line is noise") } @@ -58,6 +60,7 @@ func TestOutputStorageSchemaPlan_Converged(t *testing.T) { assert.Contains(t, out, "PostgreSQL Schema Change Plan") assert.Contains(t, out, "✓ No schema changes detected.") assert.NotContains(t, out, "resolve the manual entries above") + assert.NotContains(t, out, "storage apply", "a converged database has no next step to name") assert.NotContains(t, out, "📋 Plan:") } @@ -388,6 +391,23 @@ func TestStorageSchemaEngineLabel(t *testing.T) { "a report that names no dialect still gets a title rather than a blank one") } +// A plan always compares the database against a release the operator named, so +// its next step is the same command with `apply` in place of `plan`, since the +// apply converges the schema the plan named rather than whichever one the +// answering binary happens to carry. +func TestStorageSchemaPlanHints(t *testing.T) { + hints := storageSchemaPlanHints(&apitypes.StorageSchemaReport{ + Database: "schemabot", + Host: "db-1.example", + Dialect: "mysql", + SchemaSource: "the schema files of release v1.4.0", + }) + require.Len(t, hints, 1) + assert.Contains(t, hints[0], "schemabot on db-1.example") + assert.Contains(t, hints[0], "the schema files of release v1.4.0") + assert.Contains(t, hints[0], "re-run this as `storage apply` with the same flags") +} + // A report is labelled with the database, the server it is on, its family, and // the deployment it came from, so an error naming it is unambiguous. The header // box drops the family and the environment, which it states on their own lines. diff --git a/pkg/cmd/commands/storage_schema_source.go b/pkg/cmd/commands/storage_schema_source.go index 9151002e3..9a3006dde 100644 --- a/pkg/cmd/commands/storage_schema_source.go +++ b/pkg/cmd/commands/storage_schema_source.go @@ -36,33 +36,53 @@ import ( // // Both name a schema the operator can point at and read for themselves. The // schema compiled into whichever binary answered the request is deliberately -// not offered: which release that is depends on which pod took the call, so an -// operator mid-roll would be asking about a release they had not chosen and -// could not predict, and would learn which one only from the report they were -// about to act on. +// not offered as a *selector*: which release that is depends on which pod took +// the call, so an operator mid-roll would be asking about a release they had +// not chosen and could not predict, and would learn which one only from the +// report they were about to act on. It is what an apply converges when the +// operator names nothing, which is a different thing — there the answering +// binary's own schema is the one they asked for. // -// Neither selector is available on `storage apply`, and that is the safety -// property rather than an omission: a convergence runs the schema of the binary -// running it, so "apply is what a boot does" holds by construction (AV-9). To -// converge a release's schema, run that release's binary. +// Both selectors work on `storage apply` too, and that is the point of having +// the command: an operator decides which release's storage schema to converge, +// and converges it before the release that needs it rolls. What a named schema +// cannot do is get more permission than a boot has. Every gate that guards a +// boot guards this — destructive statements refused, manual remediation gating +// the whole set — plus two that exist only here: the convergence is confirmed +// at a terminal, and it is never reachable unattended (AV-9). -// storageSchemaSourceFlags names the desired side of the diff. Exactly one -// selector is required: each names a complete schema, so the parser refuses -// both of them and validateSource refuses neither. +// storageSchemaSourceFlags names the schema to compare against, or to converge +// to. Each selector names a complete schema, so the parser refuses both at +// once; whether naming none is allowed is the command's own question, which is +// why requiring one is a separate check from validating the combination. type storageSchemaSourceFlags struct { - Release string `help:"Diff against the schema files of this published tag, fetched from the SchemaBot repository (e.g. v1.4.0)" xor:"desired-schema"` - SchemaDir string `help:"Diff against the .sql files in this directory instead — a checkout of the release you are about to deploy (e.g. ./pkg/schema/mysql)" name:"schema-dir" type:"path" xor:"desired-schema"` + Release string `help:"Use the schema files of this published tag, fetched from the SchemaBot repository (e.g. v1.4.0)" xor:"desired-schema"` + SchemaDir string `help:"Use the .sql files in this directory instead — a checkout of the release you are about to deploy (e.g. ./pkg/schema/mysql)" name:"schema-dir" type:"path" xor:"desired-schema"` // Repo carries no Kong default, so an unset flag stays empty and - // validateSource can tell "omitted" from "typed the default value". The - // default is applied in resolve instead, where nothing has to distinguish - // the two any more. + // validateSourceCombination can tell "omitted" from "typed the default + // value". The default is applied in resolve instead, where nothing has to + // distinguish the two any more. Repo string `help:"Repository to fetch --release schema files from (default block/schemabot)" name:"release-repo"` } -// validateSource requires a desired schema and refuses --release-repo without -// the flag it modifies. Both selectors at once is the parser's own refusal, and -// restated here for callers that build the command in Go. -func (f *storageSchemaSourceFlags) validateSource() error { +// namedSource is the selector the operator used, or empty when they named +// none. Empty means the answering binary's own embedded schema — the schema its +// next boot converges. +func (f *storageSchemaSourceFlags) namedSource() string { + if strings.TrimSpace(f.Release) != "" { + return "--release" + } + if strings.TrimSpace(f.SchemaDir) != "" { + return "--schema-dir" + } + return "" +} + +// validateSourceCombination refuses combinations that are wrong on either +// command: both selectors at once, and --release-repo without the flag it +// modifies. The parser refuses the first on its own; it is restated here for +// callers that build the command in Go. +func (f *storageSchemaSourceFlags) validateSourceCombination() error { named := make([]string, 0, 2) if strings.TrimSpace(f.Release) != "" { named = append(named, "--release") @@ -70,41 +90,27 @@ func (f *storageSchemaSourceFlags) validateSource() error { if strings.TrimSpace(f.SchemaDir) != "" { named = append(named, "--schema-dir") } - switch { - case len(named) == 0: - return fmt.Errorf("missing flags: --release=STRING or --schema-dir=STRING") - case len(named) > 1: + if len(named) > 1 { return fmt.Errorf("%s can't be used together", strings.Join(named, " and ")) } - repo := strings.TrimSpace(f.Repo) - if strings.TrimSpace(f.Release) == "" && repo != "" { + if strings.TrimSpace(f.Release) == "" && strings.TrimSpace(f.Repo) != "" { return fmt.Errorf("--release-repo only applies with --release: it says which repository to fetch a published tag's schema files from") } return nil } -// storageSchemaSourceRefusal refuses the diff's release selectors on a -// convergence, and names the two ways to converge a release's schema instead. -// -// The refusal is the invariant, stated where an operator meets it. A -// convergence runs the schema embedded in the binary running it, which is what -// makes an operator apply identical to the next boot's — so it can be used to -// converge storage ahead of a deploy without the fleet's own boots then -// disagreeing with it (AV-9). A convergence to files named on the command line -// would be a second implementation of the one path that must not have two. -func storageSchemaSourceRefusal(schemaDir, release, repo string) error { - selector := "" - switch { - case strings.TrimSpace(release) != "": - selector = "--release" - case strings.TrimSpace(schemaDir) != "": - selector = "--schema-dir" - case strings.TrimSpace(repo) != "": - selector = "--release-repo" - default: - return nil +// validateSource additionally requires a schema to be named, which the diff +// does: its whole question is whether the storage is ready for a release, and +// a diff that defaulted to the answering binary's own would answer about a +// release the operator did not choose. +func (f *storageSchemaSourceFlags) validateSource() error { + if err := f.validateSourceCombination(); err != nil { + return err + } + if f.namedSource() == "" { + return fmt.Errorf("missing flags: --release=STRING or --schema-dir=STRING") } - return fmt.Errorf("%s cannot be used with a convergence: an apply runs the schema embedded in the binary running it, so that it converges exactly what that binary's next boot would. To converge a release's schema, run that release's binary — its container image is that release — or let the release's own first boot converge it. To see what it would do, use the same flag on `storage plan`", selector) + return nil } // resolve reads the desired schema the flags named. @@ -120,7 +126,7 @@ func storageSchemaSourceRefusal(schemaDir, release, repo string) error { // repository, while a directory on disk is read as given. On the API path // resolving the dialect costs a round trip, so the release path pays for it and // the directory path does not. -func (f *storageSchemaSourceFlags) resolve(ctx context.Context, dialect func() (schema.Dialect, error)) (*api.StorageSchemaSource, error) { +func (f *storageSchemaSourceFlags) resolve(ctx context.Context, dialect func() (schema.Dialect, error), converging bool) (*api.StorageSchemaSource, error) { if dir := strings.TrimSpace(f.SchemaDir); dir != "" { return storageSchemaFromDirectory(dir) } @@ -136,7 +142,7 @@ func (f *storageSchemaSourceFlags) resolve(ctx context.Context, dialect func() ( if repo == "" { repo = defaultStorageSchemaRepo } - return storageSchemaFromRelease(ctx, repo, release, d) + return storageSchemaFromRelease(ctx, repo, release, d, converging) } // storageSchemaFromDirectory reads a release's schema files from a checkout. @@ -237,7 +243,7 @@ const ( // captured, at the same commit. A tag that does not exist, or a repository the // caller cannot read, is an error naming the tag — never an empty schema, which // would diff as "every storage table is surplus". -func storageSchemaFromRelease(ctx context.Context, repo, tag string, dialect schema.Dialect) (*api.StorageSchemaSource, error) { +func storageSchemaFromRelease(ctx context.Context, repo, tag string, dialect schema.Dialect, converging bool) (*api.StorageSchemaSource, error) { directory, err := storageSchemaReleaseDirectory(dialect) if err != nil { return nil, err @@ -249,9 +255,11 @@ func storageSchemaFromRelease(ctx context.Context, repo, tag string, dialect sch ctx, cancel := context.WithTimeout(ctx, storageSchemaFetchTimeout) defer cancel() - warnIfReleaseHostIsPlaintext() + if err := guardReleaseHostPlaintext(converging); err != nil { + return nil, err + } - client := &http.Client{CheckRedirect: refuseInsecureRedirect} + client := &http.Client{CheckRedirect: refuseInsecureRedirect(converging)} listing, err := listReleaseSchemaFiles(ctx, client, repo, tag, directory) if err != nil { return nil, err @@ -284,25 +292,39 @@ func storageSchemaFromRelease(ctx context.Context, repo, tag string, dialect sch // is what actually travels: net/http copies it onto the redirect request — and // drops it when the destination is a different host — before consulting this // policy, so its presence here is exactly the question of whether this hop -// carries the token. A fetch with no token is left alone; an unauthenticated -// public release has nothing to protect on the wire. -func refuseInsecureRedirect(request *http.Request, via []*http.Request) error { - if len(via) >= maxStorageSchemaRedirects { - return fmt.Errorf("stopped after %d redirects fetching release schema files", maxStorageSchemaRedirects) - } - if request.Header.Get("Authorization") == "" { - // Nothing to leak on this hop, so it is followed. The schema still - // arrives over whatever channel the redirect chose, though, and the - // warning on the configured base URL cannot speak for a host the - // operator never named — so the downgrade says so here instead of - // passing silently. - warnPlaintextSchemaSource(request.URL) +// carries the token. +// +// A hop with no token has nothing to protect on the wire, but it still decides +// which channel the files arrive over, so it is held to the rule the +// configured base URL was held to rather than left alone: a convergence +// refuses a plaintext destination and a diff is warned about one. Guarding the +// base URL alone would leave the policy in the hands of the remote end, which +// can redirect an https base wherever it likes. +func refuseInsecureRedirect(converging bool) func(*http.Request, []*http.Request) error { + return func(request *http.Request, via []*http.Request) error { + if len(via) >= maxStorageSchemaRedirects { + return fmt.Errorf("stopped after %d redirects fetching release schema files", maxStorageSchemaRedirects) + } + if request.Header.Get("Authorization") == "" { + // Nothing to leak on this hop, but the files still arrive over + // whatever channel the redirect chose, and the check on the + // configured base URL cannot speak for a host the operator never + // named. So the channel is held to the same rule here that the + // base URL was held to: a convergence refuses it, a diff is warned + // that its report is unverified. + if err := cmdclient.GuardInsecureToken(request.URL); err != nil { + if converging { + return fmt.Errorf("refusing to converge a release redirected to %s://%s: %w; anything on the network path can change the schema files before they arrive, and a convergence runs whatever arrives against SchemaBot's own storage", request.URL.Scheme, request.URL.Host, err) + } + warnPlaintextSchemaSource(request.URL) + } + return nil + } + if err := cmdclient.GuardInsecureToken(request.URL); err != nil { + return fmt.Errorf("refusing a redirect to %s://%s: %w; the redirect stays on the same host, so the token would follow it in plaintext", request.URL.Scheme, request.URL.Host, err) + } return nil } - if err := cmdclient.GuardInsecureToken(request.URL); err != nil { - return fmt.Errorf("refusing a redirect to %s://%s: %w; the redirect stays on the same host, so the token would follow it in plaintext", request.URL.Scheme, request.URL.Host, err) - } - return nil } // maxStorageSchemaRedirects matches the ceiling net/http applies when a client @@ -322,10 +344,10 @@ func storageSchemaReleaseDirectory(dialect schema.Dialect) (string, error) { } } -// warnIfReleaseHostIsPlaintext says so when the desired side of the diff is -// about to be read over a channel anyone on the path can rewrite. +// guardReleaseHostPlaintext handles a release read over a channel anyone on the +// path can rewrite, and what it does depends on what the files are for. // -// It warns rather than refuses. With no token there is nothing to leak, and a +// A diff gets a warning. With no token there is nothing to leak, and a // plaintext mirror is a legitimate thing for an operator to point // $GITHUB_API_URL at; refusing would take away a working configuration to // protect against a tampered report. What the warning buys is that the report @@ -333,29 +355,42 @@ func storageSchemaReleaseDirectory(dialect schema.Dialect) (string, error) { // describe work the release does not need, and nothing downstream can tell, // because the files parsed and diffed exactly as a real schema would. // -// A convergence is a different matter and needs no warning here: apply refuses -// --release and --schema-dir by construction (AV-9), so a binary only ever -// converges the schema it embeds. Nothing fetched over this path can be -// applied to a database. +// A convergence is refused. The same rewrite that costs a plan its authority +// costs a convergence the database: whatever DDL arrives is what runs against +// SchemaBot's own storage, having parsed exactly as a real schema would, so no +// gate downstream has anything to catch. The refusal costs an operator an https +// mirror or a --schema-dir, and the loopback carve-out below means a mirror on +// this machine still works. // // It asks GuardInsecureToken what counts as insecure rather than testing the // scheme itself, so "plaintext host" has one definition in the CLI — including // its loopback carve-out, which is right here for the same reason: a mirror on // the loopback interface has no network path for anything to sit on. -func warnIfReleaseHostIsPlaintext() { +func guardReleaseHostPlaintext(converging bool) error { if storageSchemaReleaseToken() != "" { - // A token makes this a refusal instead, at the request that would - // carry it — see getReleaseContents. - return + // A token makes this a refusal on either path, at the request that + // would carry it, see getReleaseContents. + return nil } parsed, err := url.Parse(storageSchemaReleaseAPIBase()) if err != nil { // An unparseable base is the fetch's problem to report, with the // reason; a warning about a URL nothing could read would only add // noise to the error that follows. - return + return nil + } + if cmdclient.GuardInsecureToken(parsed) == nil { + return nil + } + // A diff read over plaintext is reported as unverified, because a rewritten + // report costs an operator a wrong belief. A convergence runs whatever + // arrives against SchemaBot's own storage, so the same channel costs the + // database, and it is refused instead. + if converging { + return fmt.Errorf("refusing to converge a release read from %s over plaintext: anything on the network path can change the schema files before they arrive, and a convergence runs whatever arrives against SchemaBot's own storage. Point $GITHUB_API_URL at an https host, or use --schema-dir to converge a checkout, which needs no network at all", parsed.Scheme+"://"+parsed.Host) } warnPlaintextSchemaSource(parsed) + return nil } // warnPlaintextSchemaSource says once, for one URL, that the desired side of diff --git a/pkg/cmd/commands/storage_schema_source_test.go b/pkg/cmd/commands/storage_schema_source_test.go index c054d06c5..9e2deb76c 100644 --- a/pkg/cmd/commands/storage_schema_source_test.go +++ b/pkg/cmd/commands/storage_schema_source_test.go @@ -41,7 +41,8 @@ func TestStorageSchemaSourceFlags_ValidateSource(t *testing.T) { err = (&storageSchemaSourceFlags{Repo: "example/mirror"}).validateSource() require.Error(t, err) - assert.Contains(t, err.Error(), "missing flags") + assert.Contains(t, err.Error(), "--release-repo only applies with --release", + "the flag left on the line is named rather than reported as a missing selector") err = (&storageSchemaSourceFlags{SchemaDir: "./schema/mysql", Repo: "example/mirror"}).validateSource() require.Error(t, err) @@ -67,12 +68,14 @@ func TestRefuseInsecureRedirect(t *testing.T) { return request } - err := refuseInsecureRedirect(withToken("http://api.example/repos"), nil) + planning := refuseInsecureRedirect(false) + + err := planning(withToken("http://api.example/repos"), nil) require.Error(t, err) assert.Contains(t, err.Error(), "refusing a redirect to http://api.example") assert.Contains(t, err.Error(), "the token would follow it in plaintext") - require.NoError(t, refuseInsecureRedirect(withToken("https://api.example/repos"), nil)) + require.NoError(t, planning(withToken("https://api.example/repos"), nil)) // Without a token there is nothing to protect on the wire, so a plaintext // mirror is left alone — it is how a public release is fetched with no @@ -82,7 +85,7 @@ func TestRefuseInsecureRedirect(t *testing.T) { anonymous, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://api.example/repos", nil) require.NoError(t, err) stderr := captureStderr(t, func() { - require.NoError(t, refuseInsecureRedirect(anonymous, nil)) + require.NoError(t, planning(anonymous, nil)) }) assert.Contains(t, stderr, "read from http://api.example over plaintext") @@ -91,27 +94,44 @@ func TestRefuseInsecureRedirect(t *testing.T) { secure, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://api.example/repos", nil) require.NoError(t, err) assert.Empty(t, captureStderr(t, func() { - require.NoError(t, refuseInsecureRedirect(secure, nil)) + require.NoError(t, planning(secure, nil)) })) var chain []*http.Request for range maxStorageSchemaRedirects { chain = append(chain, anonymous) } - err = refuseInsecureRedirect(anonymous, chain) + err = planning(anonymous, chain) require.Error(t, err) assert.Contains(t, err.Error(), "stopped after 10 redirects") + + // A convergence holds every hop to the rule its configured base URL was + // held to. Checking only the URL the operator typed would leave the policy + // bypassable by the remote end: an https base that redirects to plaintext + // supplies the DDL that runs against SchemaBot's own storage, over a + // channel anything on the path can rewrite. + converging := refuseInsecureRedirect(true) + err = converging(anonymous, nil) + require.Error(t, err, "a plaintext hop must fail a convergence closed, not warn it") + assert.Contains(t, err.Error(), "refusing to converge a release redirected to http://api.example") + + // The carve-out is the same one the base URL gets: a mirror on the + // loopback interface has no network path for anything to sit on. + loopback, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://127.0.0.1:8080/repos", nil) + require.NoError(t, err) + require.NoError(t, converging(loopback, nil), "a loopback hop has no network path to protect") + require.NoError(t, converging(secure, nil)) } // A command that names no release asks the target about its own embedded // schema: resolving reads nothing, fetches nothing, and needs no dialect. An -// apply's preview is the caller that does this — the schema a convergence runs -// is the answering binary's own, so it is asked rather than handed a copy. +// convergence that names no release is the caller that does this — the schema a convergence runs +// is then the answering binary's own, so it is asked rather than handed a copy. func TestStorageSchemaSourceFlags_ResolveTargetsOwnSchema(t *testing.T) { desired, err := (&storageSchemaSourceFlags{}).resolve(t.Context(), func() (schema.Dialect, error) { t.Fatal("the dialect must not be resolved for the answering binary's own schema") return "", nil - }) + }, false) require.NoError(t, err) assert.Nil(t, desired, "a nil source is the answering binary's own embedded schema") } @@ -127,7 +147,7 @@ func TestStorageSchemaFromDirectory(t *testing.T) { []byte("CREATE TABLE `checks` (`id` BIGINT UNSIGNED AUTO_INCREMENT PRIMARY KEY)"), 0o600)) require.NoError(t, os.WriteFile(filepath.Join(dir, "README.md"), []byte("not schema"), 0o600)) - desired, err := (&storageSchemaSourceFlags{SchemaDir: dir}).resolve(t.Context(), mysqlDialect) + desired, err := (&storageSchemaSourceFlags{SchemaDir: dir}).resolve(t.Context(), mysqlDialect, false) require.NoError(t, err) require.NotNil(t, desired) assert.Equal(t, fmt.Sprintf("the schema files in %s", dir), desired.Description) @@ -140,7 +160,7 @@ func TestStorageSchemaFromDirectory(t *testing.T) { // would propose dropping every existing storage table, which reads as a // storage database that needs destroying rather than as a mistyped path. func TestStorageSchemaFromDirectory_RefusesEmptyDirectory(t *testing.T) { - _, err := (&storageSchemaSourceFlags{SchemaDir: t.TempDir()}).resolve(t.Context(), mysqlDialect) + _, err := (&storageSchemaSourceFlags{SchemaDir: t.TempDir()}).resolve(t.Context(), mysqlDialect, false) require.Error(t, err) assert.Contains(t, err.Error(), "no .sql files in --schema-dir") assert.Contains(t, err.Error(), "one .sql file per storage table") @@ -154,7 +174,7 @@ func TestStorageSchemaFromDirectory_NamesDialectSubdirectories(t *testing.T) { require.NoError(t, os.Mkdir(filepath.Join(dir, string(schema.DialectMySQL)), 0o750)) require.NoError(t, os.Mkdir(filepath.Join(dir, string(schema.DialectPostgres)), 0o750)) - _, err := (&storageSchemaSourceFlags{SchemaDir: dir}).resolve(t.Context(), mysqlDialect) + _, err := (&storageSchemaSourceFlags{SchemaDir: dir}).resolve(t.Context(), mysqlDialect, false) require.Error(t, err) assert.Contains(t, err.Error(), filepath.Join(dir, "mysql")) assert.Contains(t, err.Error(), filepath.Join(dir, "postgres")) @@ -164,7 +184,7 @@ func TestStorageSchemaFromDirectory_NamesDialectSubdirectories(t *testing.T) { // schema. func TestStorageSchemaFromDirectory_RefusesMissingDirectory(t *testing.T) { missing := filepath.Join(t.TempDir(), "not-a-checkout") - _, err := (&storageSchemaSourceFlags{SchemaDir: missing}).resolve(t.Context(), mysqlDialect) + _, err := (&storageSchemaSourceFlags{SchemaDir: missing}).resolve(t.Context(), mysqlDialect, false) require.Error(t, err) assert.Contains(t, err.Error(), missing) } @@ -212,7 +232,7 @@ func TestStorageSchemaFromRelease(t *testing.T) { }) flags := &storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/schemabot"} - desired, err := flags.resolve(t.Context(), mysqlDialect) + desired, err := flags.resolve(t.Context(), mysqlDialect, false) require.NoError(t, err) require.NotNil(t, desired) assert.Equal(t, "the schema files of release v1.4.0 in example/schemabot", desired.Description) @@ -232,12 +252,12 @@ func TestStorageSchemaFromRelease_FetchesTheStorageDialectsFiles(t *testing.T) { }) flags := &storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/schemabot"} - desired, err := flags.resolve(t.Context(), func() (schema.Dialect, error) { return schema.DialectPostgres, nil }) + desired, err := flags.resolve(t.Context(), func() (schema.Dialect, error) { return schema.DialectPostgres, nil }, false) require.NoError(t, err) require.NotNil(t, desired) assert.Contains(t, desired.Files["applies.sql"], `CREATE TABLE "applies"`) - _, err = flags.resolve(t.Context(), mysqlDialect) + _, err = flags.resolve(t.Context(), mysqlDialect, false) require.Error(t, err, "the MySQL directory is not published by this fixture") assert.Contains(t, err.Error(), "pkg/schema/mysql") } @@ -248,7 +268,7 @@ func TestStorageSchemaFromRelease_FetchesTheStorageDialectsFiles(t *testing.T) { func TestStorageSchemaFromRelease_RefusesUnknownTag(t *testing.T) { releaseSchemaServer(t, "pkg/schema/mysql", map[string]string{}) - _, err := (&storageSchemaSourceFlags{Release: "v9.9.9", Repo: "example/schemabot"}).resolve(t.Context(), mysqlDialect) + _, err := (&storageSchemaSourceFlags{Release: "v9.9.9", Repo: "example/schemabot"}).resolve(t.Context(), mysqlDialect, false) require.Error(t, err) assert.Contains(t, err.Error(), "v9.9.9") assert.Contains(t, err.Error(), "no .sql files") @@ -264,7 +284,7 @@ func TestStorageSchemaFromRelease_RefusalNamesTheRemedy(t *testing.T) { t.Cleanup(server.Close) t.Setenv("GITHUB_API_URL", server.URL) - _, err := (&storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/private"}).resolve(t.Context(), mysqlDialect) + _, err := (&storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/private"}).resolve(t.Context(), mysqlDialect, false) require.Error(t, err) assert.Contains(t, err.Error(), "GITHUB_TOKEN") assert.Contains(t, err.Error(), "--schema-dir") @@ -281,7 +301,7 @@ func TestStorageSchemaFromRelease_NotFoundNamesTheTokenToo(t *testing.T) { t.Cleanup(server.Close) t.Setenv("GITHUB_API_URL", server.URL) - _, err := (&storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/private"}).resolve(t.Context(), mysqlDialect) + _, err := (&storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/private"}).resolve(t.Context(), mysqlDialect, false) require.Error(t, err) assert.Contains(t, err.Error(), "check the tag spelling") assert.Contains(t, err.Error(), "GITHUB_TOKEN") @@ -312,7 +332,7 @@ func TestStorageSchemaFromRelease_RateLimitIsNotAPermissionFailure(t *testing.T) t.Cleanup(server.Close) t.Setenv("GITHUB_API_URL", server.URL) - _, err := (&storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/schemabot"}).resolve(t.Context(), mysqlDialect) + _, err := (&storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/schemabot"}).resolve(t.Context(), mysqlDialect, false) require.Error(t, err) assert.Contains(t, err.Error(), "rate-limited") assert.Contains(t, err.Error(), "--schema-dir") @@ -332,7 +352,7 @@ func TestStorageSchemaFromRelease_PermissionFailureWithBudgetLeft(t *testing.T) t.Cleanup(server.Close) t.Setenv("GITHUB_API_URL", server.URL) - _, err := (&storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/private"}).resolve(t.Context(), mysqlDialect) + _, err := (&storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/private"}).resolve(t.Context(), mysqlDialect, false) require.Error(t, err) assert.Contains(t, err.Error(), "not allowed to read") assert.NotContains(t, err.Error(), "rate-limited") @@ -351,7 +371,7 @@ func TestStorageSchemaFromRelease_SendsTheToken(t *testing.T) { t.Setenv("GITHUB_API_URL", server.URL) t.Setenv("GITHUB_TOKEN", "fetch-token") - _, err := (&storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/private"}).resolve(t.Context(), mysqlDialect) + _, err := (&storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/private"}).resolve(t.Context(), mysqlDialect, false) require.Error(t, err, "an empty listing is still refused") assert.Equal(t, "Bearer fetch-token", authorization) } @@ -365,7 +385,7 @@ func TestStorageSchemaRelease_RefusesTokenOverPlaintext(t *testing.T) { t.Setenv("GITHUB_API_URL", "http://ghe.example") t.Setenv("GITHUB_TOKEN", "fetch-token") - _, err := (&storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/private"}).resolve(t.Context(), mysqlDialect) + _, err := (&storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/private"}).resolve(t.Context(), mysqlDialect, false) require.Error(t, err) assert.ErrorIs(t, err, cmdclient.ErrInsecureTokenTransport) assert.Contains(t, err.Error(), "GITHUB_API_URL is http://ghe.example") @@ -389,20 +409,66 @@ func TestStorageSchemaRelease_WarnsOnPlaintextWithoutAToken(t *testing.T) { t.Setenv("GITHUB_TOKEN", "") t.Setenv("GH_TOKEN", "") - stderr := captureStderr(t, warnIfReleaseHostIsPlaintext) + stderr := captureStderr(t, func() { + _, err := (&storageSchemaSourceFlags{Release: "v1.4.0"}).resolve(t.Context(), mysqlDialect, false) + require.Error(t, err, "the host does not answer; the warning does not stand in for that") + }) assert.Contains(t, stderr, "read from http://ghe.example over plaintext") assert.Contains(t, stderr, "treat this report as unverified") } // A token turns the same host into a refusal at the request that would carry -// it, so the warning does not also fire — an operator told "treat this as +// it, so the warning does not also fire: an operator told "treat this as // unverified" about a fetch that never happened is being told about the wrong // thing. func TestStorageSchemaRelease_QuietOnPlaintextWithAToken(t *testing.T) { t.Setenv("GITHUB_API_URL", "http://ghe.example") t.Setenv("GITHUB_TOKEN", "fetch-token") - assert.Empty(t, captureStderr(t, warnIfReleaseHostIsPlaintext)) + stderr := captureStderr(t, func() { + _, err := (&storageSchemaSourceFlags{Release: "v1.4.0"}).resolve(t.Context(), mysqlDialect, false) + require.Error(t, err, "a token over plaintext is refused at the request, so nothing is fetched") + }) + assert.Empty(t, stderr) +} + +// A convergence over that same plaintext path is refused instead of warned. +// What a rewrite costs a diff is a misleading report; what it costs a +// convergence is the storage database, since whatever DDL arrives is what runs +// against it — and it arrives having parsed exactly as a real schema would, so +// nothing further along has anything to catch. The refusal lands before the +// fetch, and names both ways out. +func TestStorageSchemaRelease_RefusesToConvergePlaintextRelease(t *testing.T) { + t.Setenv("GITHUB_API_URL", "http://ghe.example") + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "") + + stderr := captureStderr(t, func() { + _, err := (&storageSchemaSourceFlags{Release: "v1.4.0"}).resolve(t.Context(), mysqlDialect, true) + require.Error(t, err) + assert.Contains(t, err.Error(), "refusing to converge a release read from http://ghe.example over plaintext") + assert.Contains(t, err.Error(), "--schema-dir") + assert.NotContains(t, err.Error(), "no such host", "the refusal lands before the fetch, not after it fails") + }) + assert.Empty(t, stderr, "a convergence is refused, so there is nothing to warn about") +} + +// The convergence refusal follows the same definition of an insecure host as +// everything else here, so a mirror on this machine converges: an operator +// fetching a release from loopback has no network path for anything to sit on. +func TestStorageSchemaRelease_ConvergesFromLoopbackMirror(t *testing.T) { + _, refs := releaseSchemaServer(t, "pkg/schema/mysql", map[string]string{ + "applies.sql": "CREATE TABLE `applies` (`id` BIGINT UNSIGNED AUTO_INCREMENT PRIMARY KEY)", + }) + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "") + + flags := &storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/schemabot"} + source, err := flags.resolve(t.Context(), mysqlDialect, true) + require.NoError(t, err) + require.NotNil(t, source) + assert.Contains(t, source.Files, "applies.sql") + assert.NotEmpty(t, *refs, "the fetch really ran") } // The warning follows the token refusal's definition of an insecure host, so a @@ -417,7 +483,7 @@ func TestStorageSchemaRelease_QuietOnLoopback(t *testing.T) { stderr := captureStderr(t, func() { flags := &storageSchemaSourceFlags{Release: "v1.4.0", Repo: "example/schemabot"} - source, err := flags.resolve(t.Context(), mysqlDialect) + source, err := flags.resolve(t.Context(), mysqlDialect, false) require.NoError(t, err) require.NotNil(t, source) }) @@ -439,27 +505,31 @@ func TestEscapePathSegments(t *testing.T) { assert.Equal(t, "", escapePathSegments("")) } -// A convergence runs the schema embedded in the binary running it, so the -// diff's release selectors are refused on `storage apply` — with the two ways -// to converge a release named, since that is what the operator is reaching -// for. -func TestStorageSchemaSourceRefusal(t *testing.T) { - require.NoError(t, storageSchemaSourceRefusal("", "", "")) - - release := storageSchemaSourceRefusal("", "v1.4.0", "") - require.Error(t, release) - assert.Contains(t, release.Error(), "--release cannot be used with a convergence") - assert.Contains(t, release.Error(), "run that release's binary") - assert.Contains(t, release.Error(), "storage plan") - - dir := storageSchemaSourceRefusal("./schema/mysql", "", "") - require.Error(t, dir) - assert.Contains(t, dir.Error(), "--schema-dir cannot be used with a convergence") - - // --release-repo is the flag most likely to be left on the line after the - // one it modifies has been dropped, so it earns the same refusal rather - // than Kong's bare "unknown flag". - repo := storageSchemaSourceRefusal("", "", "block/schemabot") - require.Error(t, repo) - assert.Contains(t, repo.Error(), "--release-repo cannot be used with a convergence") +// Naming no schema is allowed where the command has a meaning for it — a +// convergence of the answering binary's own — while the combinations that are +// wrong on either command are still refused. The two checks are separate so +// the apply can take the first without taking the requirement the diff has. +func TestStorageSchemaSourceFlags_ValidateSourceCombination(t *testing.T) { + require.NoError(t, (&storageSchemaSourceFlags{}).validateSourceCombination(), + "naming nothing is a convergence of the answering binary's own schema, not an error") + require.NoError(t, (&storageSchemaSourceFlags{Release: "v1.4.0"}).validateSourceCombination()) + require.NoError(t, (&storageSchemaSourceFlags{SchemaDir: "./schema/mysql"}).validateSourceCombination()) + + both := (&storageSchemaSourceFlags{Release: "v1.4.0", SchemaDir: "./schema/mysql"}).validateSourceCombination() + require.Error(t, both) + assert.Contains(t, both.Error(), "--release and --schema-dir can't be used together") + + orphanRepo := (&storageSchemaSourceFlags{Repo: "example/mirror"}).validateSourceCombination() + require.Error(t, orphanRepo) + assert.Contains(t, orphanRepo.Error(), "--release-repo only applies with --release") +} + +// namedSource reports the selector the operator used, which is what decides +// whether the convergence is of a schema the answering binary vouched for. +func TestStorageSchemaSourceFlags_NamedSource(t *testing.T) { + assert.Empty(t, (&storageSchemaSourceFlags{}).namedSource()) + assert.Empty(t, (&storageSchemaSourceFlags{Release: " "}).namedSource(), + "whitespace names no release") + assert.Equal(t, "--release", (&storageSchemaSourceFlags{Release: "v1.4.0"}).namedSource()) + assert.Equal(t, "--schema-dir", (&storageSchemaSourceFlags{SchemaDir: "./schema/mysql"}).namedSource()) } diff --git a/pkg/cmd/commands/storage_schema_test.go b/pkg/cmd/commands/storage_schema_test.go index 9157d5717..764454c63 100644 --- a/pkg/cmd/commands/storage_schema_test.go +++ b/pkg/cmd/commands/storage_schema_test.go @@ -2,10 +2,12 @@ package commands import ( "encoding/json" + "fmt" "net/http" "net/http/httptest" "os" "path/filepath" + "strings" "testing" "time" @@ -609,6 +611,24 @@ func TestStorageApplyCmd_RerunCommandAddressesTheSameTarget(t *testing.T) { }, rerun: "storage apply --deployment shard-a -e production --timeout 10m0s --allow-unsafe", }, + { + // The refused statements were computed against the named release, + // so a command that dropped the selector would permit destructive + // changes while converging a different release's schema. + name: "a named release travels with the permission", + cmd: StorageApplyCmd{storageSchemaSourceFlags: storageSchemaSourceFlags{ + Release: "v1.5.0", + Repo: "example/mirror", + }}, + rerun: "storage apply --release v1.5.0 --release-repo example/mirror --allow-unsafe", + }, + { + name: "a named checkout travels with the permission", + cmd: StorageApplyCmd{storageSchemaSourceFlags: storageSchemaSourceFlags{ + SchemaDir: "./pkg/schema/mysql", + }}, + rerun: "storage apply --schema-dir ./pkg/schema/mysql --allow-unsafe", + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -619,6 +639,410 @@ func TestStorageApplyCmd_RerunCommandAddressesTheSameTarget(t *testing.T) { } } +// Converging a release the answering binary does not carry is confirmed at a +// terminal, so the selectors are refused together with --auto-approve. The +// refusal is of the command form, before anything is read: an unattended run +// must not reach a database at all to find out it will not be allowed to +// converge (AV-9). +func TestBlockUnattendedNamedSchema(t *testing.T) { + require.NoError(t, blockUnattendedNamedSchema("", true), + "an unattended convergence of the answering binary's own schema is the pre-deploy step this command exists for") + require.NoError(t, blockUnattendedNamedSchema("--release", false), + "a named release is fine with a person watching; the prompt is the consent") + + for _, selector := range []string{"--release", "--schema-dir"} { + err := blockUnattendedNamedSchema(selector, true) + require.Error(t, err) + assert.Contains(t, err.Error(), selector+" cannot be combined with --auto-approve") + assert.Contains(t, err.Error(), "confirmed at a terminal") + } +} + +// The refusal reaches no database and no server. An operator's pre-deploy job +// that names a release learns that from the flags alone, and a storage +// database that is unreachable or mid-incident is not touched to tell them. +func TestStorageApplyCmd_UnattendedNamedReleaseRunsNothing(t *testing.T) { + endpoint, routes := storageSchemaTestServer(t, &apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", Host: "db-1.example", + }, nil, nil) + + cmd := StorageApplyCmd{ + storageSchemaSourceFlags: storageSchemaSourceFlags{SchemaDir: storageSchemaCheckoutDir(t)}, + AutoApprove: true, + } + err := cmd.Run(t.Context(), &Globals{Endpoint: endpoint, Version: "v1.4.0"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot be combined with --auto-approve") + assert.Empty(t, *routes, "the flags are refused before anything is read") +} + +// A named release's files travel with the convergence, because the answering +// binary does not carry them — and they travel with the attribution, so the +// reports name the release whose files ran rather than the binary that ran +// them. +func TestStorageApplyCmd_SendsTheNamedSchemaToConverge(t *testing.T) { + dir := storageSchemaCheckoutDir(t) + report := &apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", Host: "db-1.example", + Version: "v1.4.0", + SchemaSource: "the schema files in " + dir, + Outstanding: []apitypes.StorageSchemaStatement{ + {Table: "applies", Operation: "create_table", DDL: "CREATE TABLE `applies` (`id` BIGINT UNSIGNED AUTO_INCREMENT PRIMARY KEY)"}, + }, + } + converged := &apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", Host: "db-1.example", + Version: "v1.4.0", SchemaSource: "the schema files in " + dir, + } + + var applied apitypes.StorageSchemaApplyRequest + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/api/storage/schema/plan": + assert.NoError(t, json.NewEncoder(w).Encode(apitypes.StorageSchemaPlanResponse{Report: report})) + case "/api/storage/schema/apply": + assert.NoError(t, json.NewDecoder(r.Body).Decode(&applied)) + assert.NoError(t, json.NewEncoder(w).Encode(apitypes.StorageSchemaApplyResponse{ + Planned: report, Remaining: converged, + })) + default: + http.Error(w, "unexpected request", http.StatusBadRequest) + } + })) + t.Cleanup(server.Close) + + answerPrompt(t, "yes") + cmd := StorageApplyCmd{storageSchemaSourceFlags: storageSchemaSourceFlags{SchemaDir: dir}} + out := captureStdout(func() { + require.NoError(t, cmd.Run(t.Context(), &Globals{Endpoint: server.URL, Version: "v1.4.0"})) + }) + + assert.Equal(t, "the schema files in "+dir, applied.SchemaSource, + "the convergence is attributed to the release whose files ran") + require.Contains(t, applied.SchemaFiles, "applies.sql", + "the answering binary does not carry the named release's files, so they travel with the request") + assert.Contains(t, applied.SchemaFiles["applies.sql"], "CREATE TABLE `applies`") + assert.Contains(t, out, "This is not the schema v1.4.0 converges on boot", + "the operator is told the deployed release will converge the difference back") +} + +// A named schema has to be confirmed at a terminal, so --json cannot be paired +// with --auto-approve to get a clean stream. The two therefore have to coexist +// on one run, and the person and the program are separate audiences rather +// than a choice: the statements being approved and the prompt go to the +// terminal, and stdout carries the response and nothing else. Neither half may +// be dropped to serve the other — an operator asked to type yes to DDL that +// was printed nowhere is the worse failure of the two. +func TestStorageApplyCmd_NamedSchemaKeepsJSONParseable(t *testing.T) { + dir := storageSchemaCheckoutDir(t) + report := &apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", Host: "db-1.example", + Version: "v1.4.0", + SchemaSource: "the schema files in " + dir, + Outstanding: []apitypes.StorageSchemaStatement{ + {Table: "applies", Operation: "create_table", DDL: "CREATE TABLE `applies` (`id` BIGINT UNSIGNED AUTO_INCREMENT PRIMARY KEY)"}, + }, + } + converged := &apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", Host: "db-1.example", + Version: "v1.4.0", SchemaSource: "the schema files in " + dir, + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/api/storage/schema/plan": + assert.NoError(t, json.NewEncoder(w).Encode(apitypes.StorageSchemaPlanResponse{Report: report})) + case "/api/storage/schema/apply": + assert.NoError(t, json.NewEncoder(w).Encode(apitypes.StorageSchemaApplyResponse{ + Planned: report, Remaining: converged, + })) + default: + http.Error(w, "unexpected request", http.StatusBadRequest) + } + })) + t.Cleanup(server.Close) + + answerPrompt(t, "yes") + cmd := StorageApplyCmd{ + storageSchemaSourceFlags: storageSchemaSourceFlags{SchemaDir: dir}, + JSON: true, + } + var out string + stderr := captureStderr(t, func() { + out = captureStdout(func() { + require.NoError(t, cmd.Run(t.Context(), &Globals{Endpoint: server.URL, Version: "v1.4.0"})) + }) + }) + + var decoded apitypes.StorageSchemaApplyResponse + require.NoError(t, json.Unmarshal([]byte(out), &decoded), + "stdout has to parse as the convergence response on its own: %q", out) + require.NotNil(t, decoded.Planned) + assert.Equal(t, "the schema files in "+dir, decoded.Planned.SchemaSource) + + assert.Contains(t, stripANSI(stderr), "CREATE TABLE `applies`", + "the person answering the prompt has to be shown the statements they are approving") + assert.Contains(t, stderr, "This is not the schema v1.4.0 converges on boot", + "the operator is still told what converging another release's schema costs") + assert.Contains(t, stderr, "Only 'yes' will be accepted") +} + +// Declining is an outcome, not an absence of one. Under --json a wrapper reads +// stdout to find out what happened, and an empty stream is indistinguishable +// from a crash that exited 0 — so a decline answers in the same shape a +// refusal does, with both halves of the report equal because nothing ran. +func TestStorageApplyCmd_DeclinedJSONConvergenceAnswersInJSON(t *testing.T) { + dir := storageSchemaCheckoutDir(t) + report := &apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", Host: "db-1.example", + Version: "v1.4.0", + SchemaSource: "the schema files in " + dir, + Outstanding: []apitypes.StorageSchemaStatement{ + {Table: "applies", Operation: "create_table", DDL: "CREATE TABLE `applies` (`id` BIGINT UNSIGNED AUTO_INCREMENT PRIMARY KEY)"}, + }, + } + + var routes []string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + routes = append(routes, r.URL.Path) + w.Header().Set("Content-Type", "application/json") + assert.NoError(t, json.NewEncoder(w).Encode(apitypes.StorageSchemaPlanResponse{Report: report})) + })) + t.Cleanup(server.Close) + + answerPrompt(t, "no") + cmd := StorageApplyCmd{ + storageSchemaSourceFlags: storageSchemaSourceFlags{SchemaDir: dir}, + JSON: true, + } + var out string + captureStderr(t, func() { + out = captureStdout(func() { + require.NoError(t, cmd.Run(t.Context(), &Globals{Endpoint: server.URL, Version: "v1.4.0"})) + }) + }) + + assert.NotContains(t, routes, "/api/storage/schema/apply", "nothing converges after a decline") + + var decoded apitypes.StorageSchemaApplyResponse + require.NoError(t, json.Unmarshal([]byte(out), &decoded), + "a decline has to be readable on stdout, not an empty stream: %q", out) + require.NotNil(t, decoded.Planned) + require.NotNil(t, decoded.Remaining) + assert.Len(t, decoded.Remaining.Outstanding, 1, + "nothing ran, so everything the plan found is still outstanding") +} + +// A convergence resolves its release as a convergence, not as a diff. The two +// read the same files over the same path, but only one of them runs them: a +// release read over plaintext is a report to distrust for a plan and a hazard +// to the storage database for an apply, so the apply is refused and nothing +// reaches the target. +func TestStorageApplyCmd_RefusesAReleaseReadOverPlaintext(t *testing.T) { + t.Setenv("GITHUB_API_URL", "http://ghe.example") + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "") + + var routes []string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + routes = append(routes, r.URL.Path) + w.Header().Set("Content-Type", "application/json") + assert.NoError(t, json.NewEncoder(w).Encode(apitypes.StorageSchemaPlanResponse{Report: &apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", Host: "db-1.example", Version: "v1.4.0", + }})) + })) + t.Cleanup(server.Close) + + cmd := StorageApplyCmd{storageSchemaSourceFlags: storageSchemaSourceFlags{Release: "v1.5.0"}} + err := cmd.Run(t.Context(), &Globals{Endpoint: server.URL, Version: "v1.4.0"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "refusing to converge a release read from http://ghe.example over plaintext") + assert.NotContains(t, routes, "/api/storage/schema/apply", "nothing converges on a refused release") +} + +// The notice under a cross-release convergence states the consequences an +// operator cannot see in the plan. The deployed release refuses to drop what +// it does not declare, so what was applied here survives its pods and each of +// their boots logs a refused destructive change for it — a fleet warning about +// work that was done on purpose, which needs saying before it is read as an +// incident. A release from before indexes were protected is the exception. +// What the notice actually predicts about a surplus index is pinned against a +// real convergence and boot by +// TestApplyStorageSchemaMySQL_PreAppliedIndexSurvivesTheRunningRelease. +func TestStorageSchemaConfirmation_CrossReleaseNotice(t *testing.T) { + report := &apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", Host: "db-1.example", + Version: "v1.4.0", + SchemaSource: "the schema files of release v1.5.0 in block/schemabot", + BootRemovalPolicy: apitypes.BootRemovalPreserves, + } + + named := storageSchemaConfirmation(report, true) + assert.Contains(t, named, "Converging schemabot on db-1.example (mysql) to the schema files of release v1.5.0 in block/schemabot") + assert.Contains(t, named, "This is not the schema v1.4.0 converges on boot") + assert.Contains(t, named, "refuses to drop what it does not declare") + assert.Contains(t, named, "logs a refused destructive change") + assert.Contains(t, named, "indexes were protected is the exception") + assert.Contains(t, named, "storage plan") + assert.Contains(t, named, "Only 'yes' will be accepted") + + own := storageSchemaConfirmation(report, false) + assert.NotContains(t, own, "This is not the schema", + "a convergence of the answering binary's own schema has no cross-release consequence to state") + assert.Contains(t, own, "Only 'yes' will be accepted") + + // A report with no version still states the consequences; the release it + // names is the one that answered, which is all the operator needs to know + // they are ahead of it. + unversioned := storageSchemaConfirmation(&apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", SchemaSource: "the schema files in ./pkg/schema/mysql", + }, true) + assert.Contains(t, unversioned, "the release answering this command") + assert.NotContains(t, unversioned, "schema converges", "an empty version must not leave a gap in the sentence") +} + +// What the deployed release does with the surplus this convergence leaves is +// not one answer, and a notice that gives one is wrong for every deployment it +// does not describe. A deployment permitting destructive storage changes drops +// the pre-applied state instead of keeping it, which inverts the advice to +// converge ahead of the deploy. PostgreSQL keeps it for a different reason — +// its bootstrap never computes a removal — so it logs no refusal to grep for +// and has no release with an index gap to warn about. +func TestStorageSchemaConfirmation_CrossReleaseNoticePerDeployment(t *testing.T) { + report := func(dialect string, boot apitypes.BootRemovalPolicy) *apitypes.StorageSchemaReport { + return &apitypes.StorageSchemaReport{ + Dialect: dialect, Database: "schemabot", Host: "db-1.example", + Version: "v1.4.0", + SchemaSource: "the schema files of release v1.5.0 in block/schemabot", + BootRemovalPolicy: boot, + } + } + + permitted := storageSchemaConfirmation(report("mysql", apitypes.BootRemovalRemoves), true) + assert.Contains(t, permitted, "this deployment permits") + assert.Contains(t, permitted, "drops the tables, columns and indexes its own", + "a deployment that converges destructively undoes what was applied here") + assert.Contains(t, permitted, "Converge as part of the deploy rather than ahead of it.") + assert.NotContains(t, permitted, "refuses to drop what it does not declare", + "the refusal the notice normally relies on is exactly what this deployment has turned off") + + preserved := storageSchemaConfirmation(report("mysql", apitypes.BootRemovalPreserves), true) + assert.Contains(t, preserved, "refuses to drop what it does not declare") + assert.Contains(t, preserved, "logs a refused destructive change") + + postgres := storageSchemaConfirmation(report("postgres", apitypes.BootRemovalPreserves), true) + assert.Contains(t, postgres, "converges only what its own schema adds") + assert.NotContains(t, postgres, "logs a refused destructive change", + "an additive-only bootstrap computes no removal, so there is no refusal to log or to grep for") + assert.NotContains(t, postgres, "indexes were protected", + "the index gap is a MySQL release history, and PostgreSQL has no such window") +} + +// A boot policy nobody could report is answered as such, and never as the +// reassuring half of it. +// +// It arrives two ways: a target addressed by DSN, which has no deployment +// config to read, and a target answering from a release older than the field, +// which leaves it at the wire's zero value while answering everything else. In +// both, the deployment behind it may be either kind, and the one an operator +// acts on — that pre-applied state survives to the deploy — is the one that has +// to be earned. PostgreSQL is the exception that needs no policy: an +// additive-only bootstrap preserves surplus state on every release, including +// the ones too old to say so. +func TestStorageSchemaConfirmation_CrossReleaseNoticeWithoutABootPolicy(t *testing.T) { + unread := &apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", Host: "db-1.example", + Version: "v1.4.0", + SchemaSource: "the schema files of release v1.5.0 in block/schemabot", + } + require.Equal(t, apitypes.BootRemovalUnknown, unread.BootRemovalPolicy) + + notice := storageSchemaConfirmation(unread, true) + assert.Contains(t, notice, "could not be established") + assert.Contains(t, notice, "Confirm which before relying on this") + assert.NotContains(t, notice, "refuses to drop what it does not declare", + "an unread policy must not be rendered as the deployment that keeps what is applied here") + assert.NotContains(t, notice, "this deployment permits", + "nor as the one that drops it") + + // The same silence on PostgreSQL, where the dialect settles it without a + // policy to read. + postgres := storageSchemaConfirmation(&apitypes.StorageSchemaReport{ + Dialect: "postgres", Database: "schemabot", Host: "db-1.example", + Version: "v1.4.0", + SchemaSource: "the schema files of release v1.5.0 in block/schemabot", + }, true) + assert.Contains(t, postgres, "converges only what its own schema adds") + assert.NotContains(t, postgres, "could not be established", + "an additive-only bootstrap preserves surplus state whether or not the release says so") +} + +// A policy this binary has no text for reads as unknown, not as survival. +// +// The CLI is versioned apart from the control plane it dials, and the policy +// crosses the HTTP hop as an open string that the decoder passes through +// verbatim — so a value only ever arrives here from a *newer* server, and the +// set of them grows for as long as the field does. The producer is exhaustive +// for the same reason; the consumer is the half an operator reads, so it is the +// half where falling through to "your pre-applied state survives" would be +// acted on. +func TestStorageSchemaConfirmation_CrossReleaseNoticeWithAPolicyFromALaterRelease(t *testing.T) { + later := storageSchemaConfirmation(&apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", Host: "db-1.example", + Version: "v1.4.0", + SchemaSource: "the schema files of release v1.5.0 in block/schemabot", + BootRemovalPolicy: apitypes.BootRemovalPolicy("removes_after_grace"), + }, true) + + assert.Contains(t, later, "could not be established") + assert.NotContains(t, later, "refuses to drop what it does not declare", + "a policy this binary cannot read must not resolve to the one that keeps what is applied here") + assert.NotContains(t, later, "this deployment permits", + "nor to the one that drops it") + + preserves := storageSchemaConfirmation(&apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", Host: "db-1.example", + Version: "v1.4.0", + SchemaSource: "the schema files of release v1.5.0 in block/schemabot", + BootRemovalPolicy: apitypes.BootRemovalPreserves, + }, true) + assert.Contains(t, preserves, "refuses to drop what it does not declare", + "and the policy that does mean survival still says so, so the default is not swallowing it") +} + +// The notice describes what the fleet's own boots do, so it reads the +// deployment's standing policy and never this command's flag. +// +// An operator whose deployment has not permitted destructive storage changes +// meets the destructive gate, is handed `--allow-unsafe` by the rerun hint, and +// runs it. That widens their convergence and moves nothing about the pods +// around them: those boots still refuse to drop what they do not declare, and +// the state applied here still survives until the deploy. A notice reading the +// effective policy would tell them the opposite on exactly the run the hint +// told them to make, and they would abandon a safe pre-deploy convergence over +// it. +func TestStorageSchemaConfirmation_CrossReleaseNoticeIgnoresTheRequestOptIn(t *testing.T) { + optedIn := &apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", Host: "db-1.example", + Version: "v1.4.0", + SchemaSource: "the schema files of release v1.5.0 in block/schemabot", + // --allow-unsafe on a deployment that configured nothing: this run may + // drop, and every boot of v1.4.0 still refuses to. + DestructiveAllowed: true, + BootRemovalPolicy: apitypes.BootRemovalPreserves, + } + + notice := storageSchemaConfirmation(optedIn, true) + assert.Contains(t, notice, "refuses to drop what it does not declare", + "the deployed release's own behavior is what the notice is about") + assert.NotContains(t, notice, "this deployment permits", + "one command's opt-in is not the deployment's standing policy") + assert.NotContains(t, notice, "Converge as part of the deploy rather than ahead of it.", + "the advice to wait for the deploy belongs to a deployment whose boots would drop this") +} + // Destructive statements the target has permitted are not gated: the report // says they will run, and blocking them would narrow a standing storage policy // the CLI has no business narrowing (AV-9). @@ -676,3 +1100,171 @@ func TestStorageSchemaTargetSuffix(t *testing.T) { assert.Empty(t, storageSchemaTargetSuffix("", "")) assert.Equal(t, " for deployment west in production", storageSchemaTargetSuffix("west", "production")) } + +// The storage target is resolved once per run. Every resolution of a config +// using storage.dsn_from is a fresh read of secret references, so a second one +// can answer differently than the first — and the run that resolved twice would +// be the one that converged a database its own preview never looked at. +func TestStorageSchemaTargetFlags_ResolveTheTargetOnce(t *testing.T) { + flags := storageSchemaTargetFlags{DSN: "postgres://schemabot@db-1.example:5432/schemabot"} + first, err := flags.target() + require.NoError(t, err) + assert.Equal(t, "--dsn flag", first.source) + require.NotNil(t, flags.resolvedTarget) + + // The selectors are emptied, so a second resolution has nothing to resolve + // from: an answer at all is the memo answering. + flags.DSN = "" + second, err := flags.target() + require.NoError(t, err) + assert.Same(t, first, second, "the target is resolved once; a second read is the same database, not another lookup") +} + +// The resolution travels with the flags that named it. An apply hands its +// target flags to its own preview, and it is that hand-off — a struct copy — +// that has to carry the resolved database rather than the selectors that would +// resolve one again. +func TestStorageSchemaTargetFlags_CopyCarriesTheResolution(t *testing.T) { + flags := storageSchemaTargetFlags{DSN: "schemabot@tcp(db-1.example:3306)/schemabot"} + resolved, err := flags.target() + require.NoError(t, err) + + preview := StoragePlanCmd{storageSchemaTargetFlags: flags} + preview.DSN = "" + carried, err := preview.target() + require.NoError(t, err) + assert.Same(t, resolved, carried, "the preview addresses the database the apply resolved") +} + +// A refused --json convergence is reported as JSON. It is the case a program +// most needs to read, and nothing ran, so every statement the plan found is +// still outstanding and both halves of the response are the same report. A +// human plan printed instead would be unparseable text where a caller expects +// an object, and the destructive gate alone would print nothing at all. +func TestStorageApplyCmd_RefusalIsReportedAsJSON(t *testing.T) { + tests := []struct { + name string + plan *apitypes.StorageSchemaReport + human string + }{ + { + name: "manual remediation", + human: "Needs manual remediation", + plan: &apitypes.StorageSchemaReport{ + Dialect: "postgres", + Database: "schemabot", + Host: "db-1.example", + SchemaSource: "the schema embedded in v1.4.0", + Manual: []apitypes.StorageSchemaStatement{{ + Table: "checks", + Operation: "add_column", + DDL: `ALTER TABLE "checks" ADD COLUMN "head_sha" varchar(64) NOT NULL`, + Reason: "definition is NOT NULL without a DEFAULT", + }}, + }, + }, + { + name: "destructive statement", + human: "Apply blocked", + plan: &apitypes.StorageSchemaReport{ + Dialect: "mysql", + Database: "schemabot", + Host: "db-1.example", + SchemaSource: "the schema embedded in v1.4.0", + Destructive: []apitypes.StorageSchemaStatement{ + {Table: "stale_state", Operation: "drop_table", DDL: "DROP TABLE `stale_state`", Reason: "DROP TABLE destroys data"}, + }, + }, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + endpoint, routes := storageSchemaTestServer(t, tc.plan, nil, nil) + + var err error + out := captureStdout(func() { + cmd := StorageApplyCmd{AutoApprove: true, JSON: true} + err = cmd.Run(t.Context(), &Globals{Endpoint: endpoint}) + }) + + require.Error(t, err) + assert.Equal(t, []string{"POST /api/storage/schema/plan"}, *routes, "a refusal converges nothing") + assert.NotContains(t, out, tc.human, "a program reads this surface, so the refusal is the response shape") + + var response apitypes.StorageSchemaApplyResponse + require.NoError(t, json.Unmarshal([]byte(out), &response), "the whole of stdout is the response") + require.NotNil(t, response.Planned) + require.NotNil(t, response.Remaining) + assert.Equal(t, tc.plan.Manual, response.Remaining.Manual) + assert.Equal(t, tc.plan.Destructive, response.Remaining.Destructive) + assert.Equal(t, response.Planned, response.Remaining, "nothing ran, so what was planned is what is still outstanding") + }) + } +} + +// The schema an operator approves is the schema that converges, read once. +// +// A convergence resolves its selectors once and threads the result through the +// preview, the confirmation and the apply. Reading them a second time opens a +// window the operator cannot see: a tag that moves, or a checkout someone +// edits or rebuilds while the prompt is waiting, and the file set that runs is +// not the one the plan showed. The window is small and the consequence is not — +// it is DDL nobody approved, against the storage database. +// +// A release is the case where the two reads are separated by a network, so the +// count is what proves it. Asserting on the DDL instead would not: the two +// reads sit next to each other in one call, so there is no seam for a test to +// change the files through, and a second read of unchanged files is invisible +// in what runs while still being a second read. +func TestStorageApplyCmd_ResolvesTheNamedReleaseOnce(t *testing.T) { + var fetches int + releases := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fetches++ + const directory = "pkg/schema/mysql" + path := strings.TrimPrefix(r.URL.Path, "/repos/example/schemabot/contents/") + if path == directory { + _, err := fmt.Fprintf(w, `[{"name":"applies.sql","path":%q,"type":"file"}]`, directory+"/applies.sql") + assert.NoError(t, err) + return + } + _, err := w.Write([]byte("CREATE TABLE `applies` (`id` BIGINT UNSIGNED AUTO_INCREMENT PRIMARY KEY)")) + assert.NoError(t, err) + })) + t.Cleanup(releases.Close) + t.Setenv("GITHUB_API_URL", releases.URL) + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "") + + const source = "the schema files of release v1.4.0 in example/schemabot" + report := &apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", Host: "db-1.example", + SchemaSource: source, + Outstanding: []apitypes.StorageSchemaStatement{ + {Table: "applies", Operation: "create_table", DDL: "CREATE TABLE `applies` (`id` BIGINT UNSIGNED AUTO_INCREMENT PRIMARY KEY)"}, + }, + } + converged := &apitypes.StorageSchemaReport{ + Dialect: "mysql", Database: "schemabot", Host: "db-1.example", + SchemaSource: source, Converged: true, + } + + var applied apitypes.StorageSchemaApplyRequest + endpoint, _ := storageSchemaTestServerRecording(t, report, report, converged, &applied) + + answerPrompt(t, "yes") + var err error + captureStdout(func() { + cmd := StorageApplyCmd{} + cmd.Release = "v1.4.0" + cmd.Repo = "example/schemabot" + err = cmd.Run(t.Context(), &Globals{Endpoint: endpoint}) + }) + require.NoError(t, err) + + // One listing and one file read. A second resolution would double both. + assert.Equal(t, 2, fetches, + "the release is fetched once and reused; fetching it again for the convergence is the moved-tag window") + assert.Equal(t, source, applied.SchemaSource) + assert.Contains(t, applied.SchemaFiles, "applies.sql", + "and the schema that was read is the schema that converges") +} diff --git a/pkg/cmd/commands/storage_target.go b/pkg/cmd/commands/storage_target.go index 172ace1a0..bf64ddd08 100644 --- a/pkg/cmd/commands/storage_target.go +++ b/pkg/cmd/commands/storage_target.go @@ -37,14 +37,21 @@ type storageTarget struct { dsn string dialect schema.Dialect source string - // allowDestructive is the deployment's standing storage policy + // destructive is the deployment's standing storage policy // (storage.allow_destructive_schema_changes), when the DSN came from a // server config. A boot of that config converges under it, so an operator - // convergence against the same database must too — otherwise "apply is what - // a boot does" (AV-9) stops being true on exactly the deployments that - // opted in. A DSN passed on the command line carries no config and so no - // policy, leaving --allow-unsafe as the only way to widen it. - allowDestructive bool + // convergence against the same database must too — an operator may name + // which schema runs, but not the policy it runs under (AV-9), and otherwise + // the two would disagree on exactly the deployments that opted in. + // + // A DSN passed on the command line carries no config, and that is unknown + // rather than forbidding. The two behave alike for what this run may + // execute — neither permits anything, so --allow-unsafe stays the only way + // to widen it — and differently for what the report may claim, because a + // database reached by DSN still belongs to a deployment whose policy nobody + // here has read. Reporting that absence as a refusal is how an operator is + // told to pre-apply storage their fleet will drop. + destructive api.DeploymentDestructivePolicy // postgresStatementTimeout is the config's statement budget, for the same // reason: a convergence run here has to read and write under the budget the // deployment's own bootstrap uses. Nil where no config was loaded, which @@ -52,20 +59,16 @@ type storageTarget struct { postgresStatementTimeout *time.Duration } -// allowsDestructive is whether destructive storage statements run against this -// target. The operator's per-command flag only ever widens the target's -// standing policy: a request may permit destructive statements on a deployment -// that does not, and must never refuse ones the deployment's own boot would -// run. -func (t *storageTarget) allowsDestructive(requestAllowDestructive bool) bool { - return t.allowDestructive || requestAllowDestructive -} - // ensureSchemaOptions is the policy this target converges or diffs under. +// +// The target's standing policy and the operator's per-command flag are handed +// over as the two separate facts they are. Widening the one by the other is +// api's to do, so that every caller widens it the same way and a report can +// still name what a boot of this deployment does on its own. func (t *storageTarget) ensureSchemaOptions(requestAllowDestructive bool) []api.EnsureSchemaOption { opts := []api.EnsureSchemaOption{ api.WithDialect(t.dialect), - api.WithAllowDestructiveSchemaChanges(t.allowsDestructive(requestAllowDestructive)), + api.WithDestructiveSchemaChangePolicy(t.destructive, requestAllowDestructive), } if t.postgresStatementTimeout != nil { opts = append(opts, api.WithPostgresStatementTimeout(*t.postgresStatementTimeout)) @@ -182,7 +185,7 @@ func (c *storageConfig) target() (*storageTarget, error) { dsn: dsn, dialect: c.dialect, source: source, - allowDestructive: c.cfg.Storage.AllowDestructiveSchemaChanges, + destructive: api.ConfiguredDestructivePolicy(c.cfg.Storage.AllowDestructiveSchemaChanges), postgresStatementTimeout: &statementTimeout, }, nil } diff --git a/pkg/cmd/commands/storage_target_resolution_test.go b/pkg/cmd/commands/storage_target_resolution_test.go new file mode 100644 index 000000000..4a415df10 --- /dev/null +++ b/pkg/cmd/commands/storage_target_resolution_test.go @@ -0,0 +1,60 @@ +package commands + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Resolving the storage target is a read of secret references, so the commands +// do it in exactly one place and pass the answer around (see +// storageSchemaTargetFlags.target). A second call site would be invisible in +// behavior — both resolutions usually agree — and would only show itself on the +// run where the value changed in between, which is the run that converges a +// database its own preview never looked at. +// +// Each resolution is also a call to someone else's secret store, recorded in +// its audit trail as one more read of SchemaBot's storage credential. +func TestStorageTargetIsResolvedInOnePlace(t *testing.T) { + const ( + resolver = "resolveStorageTarget" + memo = "target" + ) + + entries, err := os.ReadDir(".") + require.NoError(t, err) + + callers := map[string][]string{} + fset := token.NewFileSet() + for _, entry := range entries { + name := entry.Name() + if entry.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + file, err := parser.ParseFile(fset, filepath.Join(".", name), nil, 0) + require.NoError(t, err, "parse %s", name) + + var enclosing string + ast.Inspect(file, func(node ast.Node) bool { + switch n := node.(type) { + case *ast.FuncDecl: + enclosing = n.Name.Name + case *ast.CallExpr: + if ident, ok := n.Fun.(*ast.Ident); ok && ident.Name == resolver { + callers[enclosing] = append(callers[enclosing], name) + } + } + return true + }) + } + + assert.Equal(t, map[string][]string{memo: {"storage_schema.go"}}, callers, + "the storage target is resolved by %s alone; a caller that resolves its own would read the storage credential a second time and could get a different database than the one already previewed", memo) +} diff --git a/pkg/cmd/commands/storage_target_test.go b/pkg/cmd/commands/storage_target_test.go index 66e5ed536..ff18925f7 100644 --- a/pkg/cmd/commands/storage_target_test.go +++ b/pkg/cmd/commands/storage_target_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/block/schemabot/pkg/api" "github.com/block/schemabot/pkg/schema" ) @@ -233,32 +234,32 @@ postgres: `) target, err := resolveStorageTarget("", path, "") require.NoError(t, err) - assert.True(t, target.allowDestructive, "the config's standing policy travels with the target") + assert.Equal(t, api.DestructivePolicyPermits, target.destructive, + "the config's standing policy travels with the target") require.NotNil(t, target.postgresStatementTimeout) assert.Equal(t, 45*time.Second, *target.postgresStatementTimeout) - // A DSN passed on the command line has no config behind it, so it carries - // no policy and the request flag is the only way to widen one. + // The same config with the policy left out is a policy that was read and + // says no, which is a different answer from one nobody could read. + silent := writeStorageTestConfig(t, ` +storage: + dialect: postgres + dsn: postgres://schemabot@storage-host:5432/schemabot +`) + configured, err := resolveStorageTarget("", silent, "") + require.NoError(t, err) + assert.Equal(t, api.DestructivePolicyForbids, configured.destructive) + + // A DSN passed on the command line has no config behind it, so nobody can + // say what the deployment's boots do. The run itself is still refused, and + // the request flag is the only way to widen it. direct, err := resolveStorageTarget("postgres://schemabot@db.example:5432/schemabot", "", "") require.NoError(t, err) - assert.False(t, direct.allowDestructive) + assert.Equal(t, api.DestructivePolicyUnknown, direct.destructive, + "a target with no config behind it has no deployment policy to report") assert.Nil(t, direct.postgresStatementTimeout) } -// The request flag only ever widens the target's standing policy. A convergence -// that narrowed it would refuse statements the deployment's own next boot runs. -func TestStorageTargetAllowsDestructive_RequestOnlyWidens(t *testing.T) { - permissive := &storageTarget{dialect: schema.DialectMySQL, allowDestructive: true} - assert.True(t, permissive.allowsDestructive(false), - "a config that allows destructive changes is not narrowed by a request without the flag") - assert.True(t, permissive.allowsDestructive(true)) - - strict := &storageTarget{dialect: schema.DialectMySQL} - assert.False(t, strict.allowsDestructive(false)) - assert.True(t, strict.allowsDestructive(true), - "the request flag widens a target with no standing policy") -} - // A target converges or diffs under the policy it resolved: the family whose // differ runs, whether destructive statements are permitted, and the statement // budget its config asked for. A target with no config behind it passes no @@ -269,16 +270,14 @@ func TestStorageTargetEnsureSchemaOptions_CarryTheResolvedPolicy(t *testing.T) { budget := 45 * time.Second configured := &storageTarget{ dialect: schema.DialectPostgres, - allowDestructive: true, + destructive: api.DestructivePolicyPermits, postgresStatementTimeout: &budget, } assert.Equal(t, schema.DialectPostgres, configured.dialect) - assert.True(t, configured.allowsDestructive(false), "the config's own policy converges without a flag") assert.Len(t, configured.ensureSchemaOptions(false), 3, "dialect, destructive policy, and the config's statement budget") direct := &storageTarget{dialect: schema.DialectMySQL} - assert.False(t, direct.allowsDestructive(false)) assert.Nil(t, direct.postgresStatementTimeout, "a DSN on the command line carries no config budget") assert.Len(t, direct.ensureSchemaOptions(false), 2, "dialect and destructive policy only, so the package default budget stands") diff --git a/pkg/proto/tern.proto b/pkg/proto/tern.proto index b5d027d69..3e9c953ce 100644 --- a/pkg/proto/tern.proto +++ b/pkg/proto/tern.proto @@ -990,8 +990,8 @@ message StartResponse { // caller deploying a later release asks what this storage needs in order to // match that release, which the serving binary cannot answer from files it does // not have. Sending the files is the only way to ask the question, and it is -// safe to ask because a diff executes nothing. The apply RPC carries no such -// field, so a convergence can only ever run the serving binary's own schema. +// safe to ask because a diff executes nothing. The apply RPC takes the same +// field, so the schema an operator previewed here is the schema they converge. message StorageSchemaPlanRequest { // AllowDestructive reports the destructive statements as allowed rather than // refused, matching what an apply carrying the same flag would run. It does @@ -1043,7 +1043,10 @@ message StorageSchemaReport { // Statements classified as destroying data. Refused unless destructive // changes are allowed. repeated StorageSchemaStatement destructive = 5; - // Whether the destructive statements would actually run. + // Whether the destructive statements would actually run on this call. It is + // the effective policy: the deployment's standing one, widened by an opt-in + // this caller sent. Use boot_removal_policy to reason about what some other + // process does, which a caller's opt-in never moves. bool destructive_allowed = 6; // Changes that cannot run automatically, each naming the situation and its // remediation. Any entry aborts the whole convergence before a single @@ -1065,6 +1068,35 @@ message StorageSchemaReport { // a shadow table until it cuts over. Only true is a finding; false is the // absence of evidence, not a claim that the database is idle. bool convergence_in_flight = 10; + // What the next pod to start does to storage state its own schema does not + // declare, which is what says whether state converged ahead of a deploy + // survives until that deploy. + // + // This is a property of the deployment and its dialect, never of the call + // that asked. A caller opting in to destructive statements moves + // destructive_allowed and leaves this alone, because a boot reads the + // deployment's config and has never heard of the request. + BootRemovalPolicy boot_removal_policy = 11; +} + +// BootRemovalPolicy is what a boot of the answering deployment does to storage +// state its own schema does not declare. +// +// UNSPECIFIED is a real answer and the reason this is not a bool. A release +// that predates this field leaves it unset, and so does a caller holding a DSN +// rather than a deployment's config, so an unset value means the answering side +// could not say — never that the state is safe. Reading it as "preserved" is +// how an operator is told to pre-apply storage the next pod will drop. +enum BootRemovalPolicy { + // The answering side did not say. Older releases do not set this field, and + // a convergence addressed by DSN alone has no deployment config to read. + BOOT_REMOVAL_POLICY_UNSPECIFIED = 0; + // A boot leaves surplus storage state in place: it either refuses the + // removals, or never computes one. + BOOT_REMOVAL_POLICY_PRESERVES = 1; + // A boot drops the tables, columns and indexes its own schema does not + // declare. + BOOT_REMOVAL_POLICY_REMOVES = 2; } // StorageSchemaPlanResponse carries the outstanding storage DDL. @@ -1073,7 +1105,16 @@ message StorageSchemaPlanResponse { } // StorageSchemaApplyRequest converges the serving instance's own storage -// schema. +// schema. Like the diff, it carries no target: the instance converges the +// storage it uses, and nothing a caller sends can point it at a different +// database. +// +// What a caller may send is the schema to converge *to*. An operator rolling a +// later release converges the storage to that release before its first pod +// starts, and the serving binary cannot do that from files it does not have. +// Sending the files moves which schema runs and nothing else: the destructive +// refusal and the manual-remediation gate decide exactly as they decide for a +// boot. message StorageSchemaApplyRequest { // AllowDestructive permits the destructive statements this call would // otherwise refuse. It is an addition to the serving instance's storage @@ -1100,6 +1141,16 @@ message StorageSchemaApplyRequest { // A value above the serving instance's maximum is refused rather than // clamped, so a caller is never told a budget it did not get. int64 timeout_seconds = 3; + // SchemaFiles is the schema to converge to, as file name → file contents + // (for example "applies.sql" → "CREATE TABLE ..."). Empty converges the + // serving binary's own embedded files, which is what its next boot would + // converge. + map schema_files = 4; + // SchemaSource says where schema_files came from, in words, for the reports + // to carry back and for the converging instance's logs to name. It is + // required with schema_files and never inferred: a convergence attributed to + // a release whose files it did not run is worse than one that names nothing. + string schema_source = 5; } // StorageSchemaApplyResponse brackets the convergence with the report from diff --git a/pkg/proto/ternv1/tern.pb.go b/pkg/proto/ternv1/tern.pb.go index c91ec8c26..5f7187f24 100644 --- a/pkg/proto/ternv1/tern.pb.go +++ b/pkg/proto/ternv1/tern.pb.go @@ -319,6 +319,69 @@ func (PullCatalogDetail) EnumDescriptor() ([]byte, []int) { return file_tern_proto_rawDescGZIP(), []int{3} } +// BootRemovalPolicy is what a boot of the answering deployment does to storage +// state its own schema does not declare. +// +// UNSPECIFIED is a real answer and the reason this is not a bool. A release +// that predates this field leaves it unset, and so does a caller holding a DSN +// rather than a deployment's config, so an unset value means the answering side +// could not say — never that the state is safe. Reading it as "preserved" is +// how an operator is told to pre-apply storage the next pod will drop. +type BootRemovalPolicy int32 + +const ( + // The answering side did not say. Older releases do not set this field, and + // a convergence addressed by DSN alone has no deployment config to read. + BootRemovalPolicy_BOOT_REMOVAL_POLICY_UNSPECIFIED BootRemovalPolicy = 0 + // A boot leaves surplus storage state in place: it either refuses the + // removals, or never computes one. + BootRemovalPolicy_BOOT_REMOVAL_POLICY_PRESERVES BootRemovalPolicy = 1 + // A boot drops the tables, columns and indexes its own schema does not + // declare. + BootRemovalPolicy_BOOT_REMOVAL_POLICY_REMOVES BootRemovalPolicy = 2 +) + +// Enum value maps for BootRemovalPolicy. +var ( + BootRemovalPolicy_name = map[int32]string{ + 0: "BOOT_REMOVAL_POLICY_UNSPECIFIED", + 1: "BOOT_REMOVAL_POLICY_PRESERVES", + 2: "BOOT_REMOVAL_POLICY_REMOVES", + } + BootRemovalPolicy_value = map[string]int32{ + "BOOT_REMOVAL_POLICY_UNSPECIFIED": 0, + "BOOT_REMOVAL_POLICY_PRESERVES": 1, + "BOOT_REMOVAL_POLICY_REMOVES": 2, + } +) + +func (x BootRemovalPolicy) Enum() *BootRemovalPolicy { + p := new(BootRemovalPolicy) + *p = x + return p +} + +func (x BootRemovalPolicy) String() string { + return protoimpl.X.EnumStringOf(x.Descriptor(), protoreflect.EnumNumber(x)) +} + +func (BootRemovalPolicy) Descriptor() protoreflect.EnumDescriptor { + return file_tern_proto_enumTypes[4].Descriptor() +} + +func (BootRemovalPolicy) Type() protoreflect.EnumType { + return &file_tern_proto_enumTypes[4] +} + +func (x BootRemovalPolicy) Number() protoreflect.EnumNumber { + return protoreflect.EnumNumber(x) +} + +// Deprecated: Use BootRemovalPolicy.Descriptor instead. +func (BootRemovalPolicy) EnumDescriptor() ([]byte, []int) { + return file_tern_proto_rawDescGZIP(), []int{4} +} + // SchemaFiles contains the schema files for a namespace (e.g. schema name for MySQL, keyspace for Vitess). type SchemaFiles struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -3962,8 +4025,8 @@ func (x *StartResponse) GetSkippedCount() int64 { // caller deploying a later release asks what this storage needs in order to // match that release, which the serving binary cannot answer from files it does // not have. Sending the files is the only way to ask the question, and it is -// safe to ask because a diff executes nothing. The apply RPC carries no such -// field, so a convergence can only ever run the serving binary's own schema. +// safe to ask because a diff executes nothing. The apply RPC takes the same +// field, so the schema an operator previewed here is the schema they converge. type StorageSchemaPlanRequest struct { state protoimpl.MessageState `protogen:"open.v1"` // AllowDestructive reports the destructive statements as allowed rather than @@ -4131,7 +4194,10 @@ type StorageSchemaReport struct { // Statements classified as destroying data. Refused unless destructive // changes are allowed. Destructive []*StorageSchemaStatement `protobuf:"bytes,5,rep,name=destructive,proto3" json:"destructive,omitempty"` - // Whether the destructive statements would actually run. + // Whether the destructive statements would actually run on this call. It is + // the effective policy: the deployment's standing one, widened by an opt-in + // this caller sent. Use boot_removal_policy to reason about what some other + // process does, which a caller's opt-in never moves. DestructiveAllowed bool `protobuf:"varint,6,opt,name=destructive_allowed,json=destructiveAllowed,proto3" json:"destructive_allowed,omitempty"` // Changes that cannot run automatically, each naming the situation and its // remediation. Any entry aborts the whole convergence before a single @@ -4153,8 +4219,17 @@ type StorageSchemaReport struct { // a shadow table until it cuts over. Only true is a finding; false is the // absence of evidence, not a claim that the database is idle. ConvergenceInFlight bool `protobuf:"varint,10,opt,name=convergence_in_flight,json=convergenceInFlight,proto3" json:"convergence_in_flight,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + // What the next pod to start does to storage state its own schema does not + // declare, which is what says whether state converged ahead of a deploy + // survives until that deploy. + // + // This is a property of the deployment and its dialect, never of the call + // that asked. A caller opting in to destructive statements moves + // destructive_allowed and leaves this alone, because a boot reads the + // deployment's config and has never heard of the request. + BootRemovalPolicy BootRemovalPolicy `protobuf:"varint,11,opt,name=boot_removal_policy,json=bootRemovalPolicy,proto3,enum=tern.v1.BootRemovalPolicy" json:"boot_removal_policy,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *StorageSchemaReport) Reset() { @@ -4257,6 +4332,13 @@ func (x *StorageSchemaReport) GetConvergenceInFlight() bool { return false } +func (x *StorageSchemaReport) GetBootRemovalPolicy() BootRemovalPolicy { + if x != nil { + return x.BootRemovalPolicy + } + return BootRemovalPolicy_BOOT_REMOVAL_POLICY_UNSPECIFIED +} + // StorageSchemaPlanResponse carries the outstanding storage DDL. type StorageSchemaPlanResponse struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -4303,7 +4385,16 @@ func (x *StorageSchemaPlanResponse) GetReport() *StorageSchemaReport { } // StorageSchemaApplyRequest converges the serving instance's own storage -// schema. +// schema. Like the diff, it carries no target: the instance converges the +// storage it uses, and nothing a caller sends can point it at a different +// database. +// +// What a caller may send is the schema to converge *to*. An operator rolling a +// later release converges the storage to that release before its first pod +// starts, and the serving binary cannot do that from files it does not have. +// Sending the files moves which schema runs and nothing else: the destructive +// refusal and the manual-remediation gate decide exactly as they decide for a +// boot. type StorageSchemaApplyRequest struct { state protoimpl.MessageState `protogen:"open.v1"` // AllowDestructive permits the destructive statements this call would @@ -4331,8 +4422,18 @@ type StorageSchemaApplyRequest struct { // A value above the serving instance's maximum is refused rather than // clamped, so a caller is never told a budget it did not get. TimeoutSeconds int64 `protobuf:"varint,3,opt,name=timeout_seconds,json=timeoutSeconds,proto3" json:"timeout_seconds,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + // SchemaFiles is the schema to converge to, as file name → file contents + // (for example "applies.sql" → "CREATE TABLE ..."). Empty converges the + // serving binary's own embedded files, which is what its next boot would + // converge. + SchemaFiles map[string]string `protobuf:"bytes,4,rep,name=schema_files,json=schemaFiles,proto3" json:"schema_files,omitempty" protobuf_key:"bytes,1,opt,name=key" protobuf_val:"bytes,2,opt,name=value"` + // SchemaSource says where schema_files came from, in words, for the reports + // to carry back and for the converging instance's logs to name. It is + // required with schema_files and never inferred: a convergence attributed to + // a release whose files it did not run is worse than one that names nothing. + SchemaSource string `protobuf:"bytes,5,opt,name=schema_source,json=schemaSource,proto3" json:"schema_source,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *StorageSchemaApplyRequest) Reset() { @@ -4386,6 +4487,20 @@ func (x *StorageSchemaApplyRequest) GetTimeoutSeconds() int64 { return 0 } +func (x *StorageSchemaApplyRequest) GetSchemaFiles() map[string]string { + if x != nil { + return x.SchemaFiles + } + return nil +} + +func (x *StorageSchemaApplyRequest) GetSchemaSource() string { + if x != nil { + return x.SchemaSource + } + return "" +} + // StorageSchemaApplyResponse brackets the convergence with the report from // before it and the report from after it. type StorageSchemaApplyResponse struct { @@ -4802,7 +4917,7 @@ const file_tern_proto_rawDesc = "" + "\x05table\x18\x01 \x01(\tR\x05table\x12\x1c\n" + "\toperation\x18\x02 \x01(\tR\toperation\x12\x10\n" + "\x03ddl\x18\x03 \x01(\tR\x03ddl\x12\x16\n" + - "\x06reason\x18\x04 \x01(\tR\x06reason\"\xc2\x03\n" + + "\x06reason\x18\x04 \x01(\tR\x06reason\"\x8e\x04\n" + "\x13StorageSchemaReport\x12\x18\n" + "\adialect\x18\x01 \x01(\tR\adialect\x12\x1a\n" + "\bdatabase\x18\x02 \x01(\tR\bdatabase\x12\x18\n" + @@ -4814,13 +4929,19 @@ const file_tern_proto_rawDesc = "" + "\x04host\x18\b \x01(\tR\x04host\x12#\n" + "\rschema_source\x18\t \x01(\tR\fschemaSource\x122\n" + "\x15convergence_in_flight\x18\n" + - " \x01(\bR\x13convergenceInFlight\"Q\n" + + " \x01(\bR\x13convergenceInFlight\x12J\n" + + "\x13boot_removal_policy\x18\v \x01(\x0e2\x1a.tern.v1.BootRemovalPolicyR\x11bootRemovalPolicy\"Q\n" + "\x19StorageSchemaPlanResponse\x124\n" + - "\x06report\x18\x01 \x01(\v2\x1c.tern.v1.StorageSchemaReportR\x06report\"\x89\x01\n" + + "\x06report\x18\x01 \x01(\v2\x1c.tern.v1.StorageSchemaReportR\x06report\"\xc6\x02\n" + "\x19StorageSchemaApplyRequest\x12+\n" + "\x11allow_destructive\x18\x01 \x01(\bR\x10allowDestructive\x12\x16\n" + "\x06caller\x18\x02 \x01(\tR\x06caller\x12'\n" + - "\x0ftimeout_seconds\x18\x03 \x01(\x03R\x0etimeoutSeconds\"\x90\x01\n" + + "\x0ftimeout_seconds\x18\x03 \x01(\x03R\x0etimeoutSeconds\x12V\n" + + "\fschema_files\x18\x04 \x03(\v23.tern.v1.StorageSchemaApplyRequest.SchemaFilesEntryR\vschemaFiles\x12#\n" + + "\rschema_source\x18\x05 \x01(\tR\fschemaSource\x1a>\n" + + "\x10SchemaFilesEntry\x12\x10\n" + + "\x03key\x18\x01 \x01(\tR\x03key\x12\x14\n" + + "\x05value\x18\x02 \x01(\tR\x05value:\x028\x01\"\x90\x01\n" + "\x1aStorageSchemaApplyResponse\x126\n" + "\aplanned\x18\x01 \x01(\v2\x1c.tern.v1.StorageSchemaReportR\aplanned\x12:\n" + "\tremaining\x18\x02 \x01(\v2\x1c.tern.v1.StorageSchemaReportR\tremaining*[\n" + @@ -4868,7 +4989,11 @@ const file_tern_proto_rawDesc = "" + "\x17CHANGE_TYPE_CREATE_VIEW\x10\t*T\n" + "\x11PullCatalogDetail\x12\x1d\n" + "\x19PULL_CATALOG_DETAIL_BASIC\x10\x00\x12 \n" + - "\x1cPULL_CATALOG_DETAIL_DETAILED\x10\x012\xc5\n" + + "\x1cPULL_CATALOG_DETAIL_DETAILED\x10\x01*|\n" + + "\x11BootRemovalPolicy\x12#\n" + + "\x1fBOOT_REMOVAL_POLICY_UNSPECIFIED\x10\x00\x12!\n" + + "\x1dBOOT_REMOVAL_POLICY_PRESERVES\x10\x01\x12\x1f\n" + + "\x1bBOOT_REMOVAL_POLICY_REMOVES\x10\x022\xc5\n" + "\n" + "\x04Tern\x12a\n" + "\n" + @@ -4904,162 +5029,166 @@ func file_tern_proto_rawDescGZIP() []byte { return file_tern_proto_rawDescData } -var file_tern_proto_enumTypes = make([]protoimpl.EnumInfo, 4) -var file_tern_proto_msgTypes = make([]protoimpl.MessageInfo, 62) +var file_tern_proto_enumTypes = make([]protoimpl.EnumInfo, 5) +var file_tern_proto_msgTypes = make([]protoimpl.MessageInfo, 63) var file_tern_proto_goTypes = []any{ (Engine)(0), // 0: tern.v1.Engine (State)(0), // 1: tern.v1.State (ChangeType)(0), // 2: tern.v1.ChangeType (PullCatalogDetail)(0), // 3: tern.v1.PullCatalogDetail - (*SchemaFiles)(nil), // 4: tern.v1.SchemaFiles - (*PullSchemaRequest)(nil), // 5: tern.v1.PullSchemaRequest - (*PulledNamespace)(nil), // 6: tern.v1.PulledNamespace - (*NamespaceCatalog)(nil), // 7: tern.v1.NamespaceCatalog - (*TableCatalog)(nil), // 8: tern.v1.TableCatalog - (*ColumnCatalog)(nil), // 9: tern.v1.ColumnCatalog - (*IndexCatalog)(nil), // 10: tern.v1.IndexCatalog - (*ForeignKeyCatalog)(nil), // 11: tern.v1.ForeignKeyCatalog - (*PullSchemaResponse)(nil), // 12: tern.v1.PullSchemaResponse - (*PlanRequest)(nil), // 13: tern.v1.PlanRequest - (*TableChange)(nil), // 14: tern.v1.TableChange - (*SchemaChange)(nil), // 15: tern.v1.SchemaChange - (*LintViolation)(nil), // 16: tern.v1.LintViolation - (*ExistingCopy)(nil), // 17: tern.v1.ExistingCopy - (*ExemptTables)(nil), // 18: tern.v1.ExemptTables - (*ShardPlan)(nil), // 19: tern.v1.ShardPlan - (*PlanResponse)(nil), // 20: tern.v1.PlanResponse - (*PlanDiffResponse)(nil), // 21: tern.v1.PlanDiffResponse - (*ApplyRequest)(nil), // 22: tern.v1.ApplyRequest - (*ApplyConflict)(nil), // 23: tern.v1.ApplyConflict - (*ApplyResponse)(nil), // 24: tern.v1.ApplyResponse - (*ProgressRequest)(nil), // 25: tern.v1.ProgressRequest - (*LogsRequest)(nil), // 26: tern.v1.LogsRequest - (*ApplyLog)(nil), // 27: tern.v1.ApplyLog - (*LogsResponse)(nil), // 28: tern.v1.LogsResponse - (*ShardProgress)(nil), // 29: tern.v1.ShardProgress - (*TableProgress)(nil), // 30: tern.v1.TableProgress - (*SettledControlRequest)(nil), // 31: tern.v1.SettledControlRequest - (*ProgressResponse)(nil), // 32: tern.v1.ProgressResponse - (*CutoverRequest)(nil), // 33: tern.v1.CutoverRequest - (*CutoverResponse)(nil), // 34: tern.v1.CutoverResponse - (*RevertRequest)(nil), // 35: tern.v1.RevertRequest - (*RevertResponse)(nil), // 36: tern.v1.RevertResponse - (*SkipRevertRequest)(nil), // 37: tern.v1.SkipRevertRequest - (*SkipRevertResponse)(nil), // 38: tern.v1.SkipRevertResponse - (*HealthRequest)(nil), // 39: tern.v1.HealthRequest - (*HealthResponse)(nil), // 40: tern.v1.HealthResponse - (*StopRequest)(nil), // 41: tern.v1.StopRequest - (*StopResponse)(nil), // 42: tern.v1.StopResponse - (*CancelRequest)(nil), // 43: tern.v1.CancelRequest - (*CancelResponse)(nil), // 44: tern.v1.CancelResponse - (*StartRequest)(nil), // 45: tern.v1.StartRequest - (*StartResponse)(nil), // 46: tern.v1.StartResponse - (*StorageSchemaPlanRequest)(nil), // 47: tern.v1.StorageSchemaPlanRequest - (*StorageSchemaStatement)(nil), // 48: tern.v1.StorageSchemaStatement - (*StorageSchemaReport)(nil), // 49: tern.v1.StorageSchemaReport - (*StorageSchemaPlanResponse)(nil), // 50: tern.v1.StorageSchemaPlanResponse - (*StorageSchemaApplyRequest)(nil), // 51: tern.v1.StorageSchemaApplyRequest - (*StorageSchemaApplyResponse)(nil), // 52: tern.v1.StorageSchemaApplyResponse - nil, // 53: tern.v1.SchemaFiles.FilesEntry - nil, // 54: tern.v1.PulledNamespace.TablesEntry - nil, // 55: tern.v1.PulledNamespace.ArtifactsEntry - nil, // 56: tern.v1.PulledNamespace.TableCatalogEntry - nil, // 57: tern.v1.PullSchemaResponse.NamespacesEntry - nil, // 58: tern.v1.PlanRequest.SchemaFilesEntry - nil, // 59: tern.v1.TableChange.MetadataEntry - nil, // 60: tern.v1.SchemaChange.MetadataEntry - nil, // 61: tern.v1.SchemaChange.OriginalFilesEntry - nil, // 62: tern.v1.ApplyRequest.OptionsEntry - nil, // 63: tern.v1.ApplyRequest.SchemaFilesEntry - nil, // 64: tern.v1.ProgressResponse.MetadataEntry - nil, // 65: tern.v1.StorageSchemaPlanRequest.SchemaFilesEntry + (BootRemovalPolicy)(0), // 4: tern.v1.BootRemovalPolicy + (*SchemaFiles)(nil), // 5: tern.v1.SchemaFiles + (*PullSchemaRequest)(nil), // 6: tern.v1.PullSchemaRequest + (*PulledNamespace)(nil), // 7: tern.v1.PulledNamespace + (*NamespaceCatalog)(nil), // 8: tern.v1.NamespaceCatalog + (*TableCatalog)(nil), // 9: tern.v1.TableCatalog + (*ColumnCatalog)(nil), // 10: tern.v1.ColumnCatalog + (*IndexCatalog)(nil), // 11: tern.v1.IndexCatalog + (*ForeignKeyCatalog)(nil), // 12: tern.v1.ForeignKeyCatalog + (*PullSchemaResponse)(nil), // 13: tern.v1.PullSchemaResponse + (*PlanRequest)(nil), // 14: tern.v1.PlanRequest + (*TableChange)(nil), // 15: tern.v1.TableChange + (*SchemaChange)(nil), // 16: tern.v1.SchemaChange + (*LintViolation)(nil), // 17: tern.v1.LintViolation + (*ExistingCopy)(nil), // 18: tern.v1.ExistingCopy + (*ExemptTables)(nil), // 19: tern.v1.ExemptTables + (*ShardPlan)(nil), // 20: tern.v1.ShardPlan + (*PlanResponse)(nil), // 21: tern.v1.PlanResponse + (*PlanDiffResponse)(nil), // 22: tern.v1.PlanDiffResponse + (*ApplyRequest)(nil), // 23: tern.v1.ApplyRequest + (*ApplyConflict)(nil), // 24: tern.v1.ApplyConflict + (*ApplyResponse)(nil), // 25: tern.v1.ApplyResponse + (*ProgressRequest)(nil), // 26: tern.v1.ProgressRequest + (*LogsRequest)(nil), // 27: tern.v1.LogsRequest + (*ApplyLog)(nil), // 28: tern.v1.ApplyLog + (*LogsResponse)(nil), // 29: tern.v1.LogsResponse + (*ShardProgress)(nil), // 30: tern.v1.ShardProgress + (*TableProgress)(nil), // 31: tern.v1.TableProgress + (*SettledControlRequest)(nil), // 32: tern.v1.SettledControlRequest + (*ProgressResponse)(nil), // 33: tern.v1.ProgressResponse + (*CutoverRequest)(nil), // 34: tern.v1.CutoverRequest + (*CutoverResponse)(nil), // 35: tern.v1.CutoverResponse + (*RevertRequest)(nil), // 36: tern.v1.RevertRequest + (*RevertResponse)(nil), // 37: tern.v1.RevertResponse + (*SkipRevertRequest)(nil), // 38: tern.v1.SkipRevertRequest + (*SkipRevertResponse)(nil), // 39: tern.v1.SkipRevertResponse + (*HealthRequest)(nil), // 40: tern.v1.HealthRequest + (*HealthResponse)(nil), // 41: tern.v1.HealthResponse + (*StopRequest)(nil), // 42: tern.v1.StopRequest + (*StopResponse)(nil), // 43: tern.v1.StopResponse + (*CancelRequest)(nil), // 44: tern.v1.CancelRequest + (*CancelResponse)(nil), // 45: tern.v1.CancelResponse + (*StartRequest)(nil), // 46: tern.v1.StartRequest + (*StartResponse)(nil), // 47: tern.v1.StartResponse + (*StorageSchemaPlanRequest)(nil), // 48: tern.v1.StorageSchemaPlanRequest + (*StorageSchemaStatement)(nil), // 49: tern.v1.StorageSchemaStatement + (*StorageSchemaReport)(nil), // 50: tern.v1.StorageSchemaReport + (*StorageSchemaPlanResponse)(nil), // 51: tern.v1.StorageSchemaPlanResponse + (*StorageSchemaApplyRequest)(nil), // 52: tern.v1.StorageSchemaApplyRequest + (*StorageSchemaApplyResponse)(nil), // 53: tern.v1.StorageSchemaApplyResponse + nil, // 54: tern.v1.SchemaFiles.FilesEntry + nil, // 55: tern.v1.PulledNamespace.TablesEntry + nil, // 56: tern.v1.PulledNamespace.ArtifactsEntry + nil, // 57: tern.v1.PulledNamespace.TableCatalogEntry + nil, // 58: tern.v1.PullSchemaResponse.NamespacesEntry + nil, // 59: tern.v1.PlanRequest.SchemaFilesEntry + nil, // 60: tern.v1.TableChange.MetadataEntry + nil, // 61: tern.v1.SchemaChange.MetadataEntry + nil, // 62: tern.v1.SchemaChange.OriginalFilesEntry + nil, // 63: tern.v1.ApplyRequest.OptionsEntry + nil, // 64: tern.v1.ApplyRequest.SchemaFilesEntry + nil, // 65: tern.v1.ProgressResponse.MetadataEntry + nil, // 66: tern.v1.StorageSchemaPlanRequest.SchemaFilesEntry + nil, // 67: tern.v1.StorageSchemaApplyRequest.SchemaFilesEntry } var file_tern_proto_depIdxs = []int32{ - 53, // 0: tern.v1.SchemaFiles.files:type_name -> tern.v1.SchemaFiles.FilesEntry + 54, // 0: tern.v1.SchemaFiles.files:type_name -> tern.v1.SchemaFiles.FilesEntry 3, // 1: tern.v1.PullSchemaRequest.catalog_detail:type_name -> tern.v1.PullCatalogDetail - 54, // 2: tern.v1.PulledNamespace.tables:type_name -> tern.v1.PulledNamespace.TablesEntry - 55, // 3: tern.v1.PulledNamespace.artifacts:type_name -> tern.v1.PulledNamespace.ArtifactsEntry - 7, // 4: tern.v1.PulledNamespace.namespace_catalog:type_name -> tern.v1.NamespaceCatalog - 56, // 5: tern.v1.PulledNamespace.table_catalog:type_name -> tern.v1.PulledNamespace.TableCatalogEntry - 9, // 6: tern.v1.TableCatalog.columns:type_name -> tern.v1.ColumnCatalog - 10, // 7: tern.v1.TableCatalog.indexes:type_name -> tern.v1.IndexCatalog - 11, // 8: tern.v1.TableCatalog.foreign_keys:type_name -> tern.v1.ForeignKeyCatalog - 57, // 9: tern.v1.PullSchemaResponse.namespaces:type_name -> tern.v1.PullSchemaResponse.NamespacesEntry - 58, // 10: tern.v1.PlanRequest.schema_files:type_name -> tern.v1.PlanRequest.SchemaFilesEntry + 55, // 2: tern.v1.PulledNamespace.tables:type_name -> tern.v1.PulledNamespace.TablesEntry + 56, // 3: tern.v1.PulledNamespace.artifacts:type_name -> tern.v1.PulledNamespace.ArtifactsEntry + 8, // 4: tern.v1.PulledNamespace.namespace_catalog:type_name -> tern.v1.NamespaceCatalog + 57, // 5: tern.v1.PulledNamespace.table_catalog:type_name -> tern.v1.PulledNamespace.TableCatalogEntry + 10, // 6: tern.v1.TableCatalog.columns:type_name -> tern.v1.ColumnCatalog + 11, // 7: tern.v1.TableCatalog.indexes:type_name -> tern.v1.IndexCatalog + 12, // 8: tern.v1.TableCatalog.foreign_keys:type_name -> tern.v1.ForeignKeyCatalog + 58, // 9: tern.v1.PullSchemaResponse.namespaces:type_name -> tern.v1.PullSchemaResponse.NamespacesEntry + 59, // 10: tern.v1.PlanRequest.schema_files:type_name -> tern.v1.PlanRequest.SchemaFilesEntry 2, // 11: tern.v1.TableChange.change_type:type_name -> tern.v1.ChangeType - 59, // 12: tern.v1.TableChange.metadata:type_name -> tern.v1.TableChange.MetadataEntry - 14, // 13: tern.v1.SchemaChange.table_changes:type_name -> tern.v1.TableChange - 60, // 14: tern.v1.SchemaChange.metadata:type_name -> tern.v1.SchemaChange.MetadataEntry - 61, // 15: tern.v1.SchemaChange.original_files:type_name -> tern.v1.SchemaChange.OriginalFilesEntry - 14, // 16: tern.v1.ShardPlan.changes:type_name -> tern.v1.TableChange + 60, // 12: tern.v1.TableChange.metadata:type_name -> tern.v1.TableChange.MetadataEntry + 15, // 13: tern.v1.SchemaChange.table_changes:type_name -> tern.v1.TableChange + 61, // 14: tern.v1.SchemaChange.metadata:type_name -> tern.v1.SchemaChange.MetadataEntry + 62, // 15: tern.v1.SchemaChange.original_files:type_name -> tern.v1.SchemaChange.OriginalFilesEntry + 15, // 16: tern.v1.ShardPlan.changes:type_name -> tern.v1.TableChange 0, // 17: tern.v1.PlanResponse.engine:type_name -> tern.v1.Engine - 15, // 18: tern.v1.PlanResponse.changes:type_name -> tern.v1.SchemaChange - 16, // 19: tern.v1.PlanResponse.lint_violations:type_name -> tern.v1.LintViolation - 19, // 20: tern.v1.PlanResponse.shards:type_name -> tern.v1.ShardPlan - 17, // 21: tern.v1.PlanResponse.existing_copies:type_name -> tern.v1.ExistingCopy - 18, // 22: tern.v1.PlanResponse.exempt_tables:type_name -> tern.v1.ExemptTables + 16, // 18: tern.v1.PlanResponse.changes:type_name -> tern.v1.SchemaChange + 17, // 19: tern.v1.PlanResponse.lint_violations:type_name -> tern.v1.LintViolation + 20, // 20: tern.v1.PlanResponse.shards:type_name -> tern.v1.ShardPlan + 18, // 21: tern.v1.PlanResponse.existing_copies:type_name -> tern.v1.ExistingCopy + 19, // 22: tern.v1.PlanResponse.exempt_tables:type_name -> tern.v1.ExemptTables 0, // 23: tern.v1.PlanDiffResponse.engine:type_name -> tern.v1.Engine - 15, // 24: tern.v1.PlanDiffResponse.changes:type_name -> tern.v1.SchemaChange - 16, // 25: tern.v1.PlanDiffResponse.lint_violations:type_name -> tern.v1.LintViolation - 19, // 26: tern.v1.PlanDiffResponse.shards:type_name -> tern.v1.ShardPlan - 62, // 27: tern.v1.ApplyRequest.options:type_name -> tern.v1.ApplyRequest.OptionsEntry - 63, // 28: tern.v1.ApplyRequest.schema_files:type_name -> tern.v1.ApplyRequest.SchemaFilesEntry - 14, // 29: tern.v1.ApplyRequest.ddl_changes:type_name -> tern.v1.TableChange - 23, // 30: tern.v1.ApplyResponse.conflict:type_name -> tern.v1.ApplyConflict - 27, // 31: tern.v1.LogsResponse.logs:type_name -> tern.v1.ApplyLog - 29, // 32: tern.v1.TableProgress.shards:type_name -> tern.v1.ShardProgress + 16, // 24: tern.v1.PlanDiffResponse.changes:type_name -> tern.v1.SchemaChange + 17, // 25: tern.v1.PlanDiffResponse.lint_violations:type_name -> tern.v1.LintViolation + 20, // 26: tern.v1.PlanDiffResponse.shards:type_name -> tern.v1.ShardPlan + 63, // 27: tern.v1.ApplyRequest.options:type_name -> tern.v1.ApplyRequest.OptionsEntry + 64, // 28: tern.v1.ApplyRequest.schema_files:type_name -> tern.v1.ApplyRequest.SchemaFilesEntry + 15, // 29: tern.v1.ApplyRequest.ddl_changes:type_name -> tern.v1.TableChange + 24, // 30: tern.v1.ApplyResponse.conflict:type_name -> tern.v1.ApplyConflict + 28, // 31: tern.v1.LogsResponse.logs:type_name -> tern.v1.ApplyLog + 30, // 32: tern.v1.TableProgress.shards:type_name -> tern.v1.ShardProgress 2, // 33: tern.v1.TableProgress.change_type:type_name -> tern.v1.ChangeType 1, // 34: tern.v1.ProgressResponse.state:type_name -> tern.v1.State 0, // 35: tern.v1.ProgressResponse.engine:type_name -> tern.v1.Engine - 30, // 36: tern.v1.ProgressResponse.tables:type_name -> tern.v1.TableProgress - 64, // 37: tern.v1.ProgressResponse.metadata:type_name -> tern.v1.ProgressResponse.MetadataEntry - 31, // 38: tern.v1.ProgressResponse.settled_control_requests:type_name -> tern.v1.SettledControlRequest - 65, // 39: tern.v1.StorageSchemaPlanRequest.schema_files:type_name -> tern.v1.StorageSchemaPlanRequest.SchemaFilesEntry - 48, // 40: tern.v1.StorageSchemaReport.outstanding:type_name -> tern.v1.StorageSchemaStatement - 48, // 41: tern.v1.StorageSchemaReport.destructive:type_name -> tern.v1.StorageSchemaStatement - 48, // 42: tern.v1.StorageSchemaReport.manual:type_name -> tern.v1.StorageSchemaStatement - 49, // 43: tern.v1.StorageSchemaPlanResponse.report:type_name -> tern.v1.StorageSchemaReport - 49, // 44: tern.v1.StorageSchemaApplyResponse.planned:type_name -> tern.v1.StorageSchemaReport - 49, // 45: tern.v1.StorageSchemaApplyResponse.remaining:type_name -> tern.v1.StorageSchemaReport - 8, // 46: tern.v1.PulledNamespace.TableCatalogEntry.value:type_name -> tern.v1.TableCatalog - 6, // 47: tern.v1.PullSchemaResponse.NamespacesEntry.value:type_name -> tern.v1.PulledNamespace - 4, // 48: tern.v1.PlanRequest.SchemaFilesEntry.value:type_name -> tern.v1.SchemaFiles - 4, // 49: tern.v1.ApplyRequest.SchemaFilesEntry.value:type_name -> tern.v1.SchemaFiles - 5, // 50: tern.v1.Tern.PullSchema:input_type -> tern.v1.PullSchemaRequest - 13, // 51: tern.v1.Tern.Plan:input_type -> tern.v1.PlanRequest - 13, // 52: tern.v1.Tern.PlanDiff:input_type -> tern.v1.PlanRequest - 22, // 53: tern.v1.Tern.Apply:input_type -> tern.v1.ApplyRequest - 25, // 54: tern.v1.Tern.Progress:input_type -> tern.v1.ProgressRequest - 26, // 55: tern.v1.Tern.Logs:input_type -> tern.v1.LogsRequest - 33, // 56: tern.v1.Tern.Cutover:input_type -> tern.v1.CutoverRequest - 35, // 57: tern.v1.Tern.Revert:input_type -> tern.v1.RevertRequest - 37, // 58: tern.v1.Tern.SkipRevert:input_type -> tern.v1.SkipRevertRequest - 39, // 59: tern.v1.Tern.Health:input_type -> tern.v1.HealthRequest - 41, // 60: tern.v1.Tern.Stop:input_type -> tern.v1.StopRequest - 43, // 61: tern.v1.Tern.Cancel:input_type -> tern.v1.CancelRequest - 45, // 62: tern.v1.Tern.Start:input_type -> tern.v1.StartRequest - 47, // 63: tern.v1.Tern.StorageSchemaPlan:input_type -> tern.v1.StorageSchemaPlanRequest - 51, // 64: tern.v1.Tern.StorageSchemaApply:input_type -> tern.v1.StorageSchemaApplyRequest - 12, // 65: tern.v1.Tern.PullSchema:output_type -> tern.v1.PullSchemaResponse - 20, // 66: tern.v1.Tern.Plan:output_type -> tern.v1.PlanResponse - 21, // 67: tern.v1.Tern.PlanDiff:output_type -> tern.v1.PlanDiffResponse - 24, // 68: tern.v1.Tern.Apply:output_type -> tern.v1.ApplyResponse - 32, // 69: tern.v1.Tern.Progress:output_type -> tern.v1.ProgressResponse - 28, // 70: tern.v1.Tern.Logs:output_type -> tern.v1.LogsResponse - 34, // 71: tern.v1.Tern.Cutover:output_type -> tern.v1.CutoverResponse - 36, // 72: tern.v1.Tern.Revert:output_type -> tern.v1.RevertResponse - 38, // 73: tern.v1.Tern.SkipRevert:output_type -> tern.v1.SkipRevertResponse - 40, // 74: tern.v1.Tern.Health:output_type -> tern.v1.HealthResponse - 42, // 75: tern.v1.Tern.Stop:output_type -> tern.v1.StopResponse - 44, // 76: tern.v1.Tern.Cancel:output_type -> tern.v1.CancelResponse - 46, // 77: tern.v1.Tern.Start:output_type -> tern.v1.StartResponse - 50, // 78: tern.v1.Tern.StorageSchemaPlan:output_type -> tern.v1.StorageSchemaPlanResponse - 52, // 79: tern.v1.Tern.StorageSchemaApply:output_type -> tern.v1.StorageSchemaApplyResponse - 65, // [65:80] is the sub-list for method output_type - 50, // [50:65] is the sub-list for method input_type - 50, // [50:50] is the sub-list for extension type_name - 50, // [50:50] is the sub-list for extension extendee - 0, // [0:50] is the sub-list for field type_name + 31, // 36: tern.v1.ProgressResponse.tables:type_name -> tern.v1.TableProgress + 65, // 37: tern.v1.ProgressResponse.metadata:type_name -> tern.v1.ProgressResponse.MetadataEntry + 32, // 38: tern.v1.ProgressResponse.settled_control_requests:type_name -> tern.v1.SettledControlRequest + 66, // 39: tern.v1.StorageSchemaPlanRequest.schema_files:type_name -> tern.v1.StorageSchemaPlanRequest.SchemaFilesEntry + 49, // 40: tern.v1.StorageSchemaReport.outstanding:type_name -> tern.v1.StorageSchemaStatement + 49, // 41: tern.v1.StorageSchemaReport.destructive:type_name -> tern.v1.StorageSchemaStatement + 49, // 42: tern.v1.StorageSchemaReport.manual:type_name -> tern.v1.StorageSchemaStatement + 4, // 43: tern.v1.StorageSchemaReport.boot_removal_policy:type_name -> tern.v1.BootRemovalPolicy + 50, // 44: tern.v1.StorageSchemaPlanResponse.report:type_name -> tern.v1.StorageSchemaReport + 67, // 45: tern.v1.StorageSchemaApplyRequest.schema_files:type_name -> tern.v1.StorageSchemaApplyRequest.SchemaFilesEntry + 50, // 46: tern.v1.StorageSchemaApplyResponse.planned:type_name -> tern.v1.StorageSchemaReport + 50, // 47: tern.v1.StorageSchemaApplyResponse.remaining:type_name -> tern.v1.StorageSchemaReport + 9, // 48: tern.v1.PulledNamespace.TableCatalogEntry.value:type_name -> tern.v1.TableCatalog + 7, // 49: tern.v1.PullSchemaResponse.NamespacesEntry.value:type_name -> tern.v1.PulledNamespace + 5, // 50: tern.v1.PlanRequest.SchemaFilesEntry.value:type_name -> tern.v1.SchemaFiles + 5, // 51: tern.v1.ApplyRequest.SchemaFilesEntry.value:type_name -> tern.v1.SchemaFiles + 6, // 52: tern.v1.Tern.PullSchema:input_type -> tern.v1.PullSchemaRequest + 14, // 53: tern.v1.Tern.Plan:input_type -> tern.v1.PlanRequest + 14, // 54: tern.v1.Tern.PlanDiff:input_type -> tern.v1.PlanRequest + 23, // 55: tern.v1.Tern.Apply:input_type -> tern.v1.ApplyRequest + 26, // 56: tern.v1.Tern.Progress:input_type -> tern.v1.ProgressRequest + 27, // 57: tern.v1.Tern.Logs:input_type -> tern.v1.LogsRequest + 34, // 58: tern.v1.Tern.Cutover:input_type -> tern.v1.CutoverRequest + 36, // 59: tern.v1.Tern.Revert:input_type -> tern.v1.RevertRequest + 38, // 60: tern.v1.Tern.SkipRevert:input_type -> tern.v1.SkipRevertRequest + 40, // 61: tern.v1.Tern.Health:input_type -> tern.v1.HealthRequest + 42, // 62: tern.v1.Tern.Stop:input_type -> tern.v1.StopRequest + 44, // 63: tern.v1.Tern.Cancel:input_type -> tern.v1.CancelRequest + 46, // 64: tern.v1.Tern.Start:input_type -> tern.v1.StartRequest + 48, // 65: tern.v1.Tern.StorageSchemaPlan:input_type -> tern.v1.StorageSchemaPlanRequest + 52, // 66: tern.v1.Tern.StorageSchemaApply:input_type -> tern.v1.StorageSchemaApplyRequest + 13, // 67: tern.v1.Tern.PullSchema:output_type -> tern.v1.PullSchemaResponse + 21, // 68: tern.v1.Tern.Plan:output_type -> tern.v1.PlanResponse + 22, // 69: tern.v1.Tern.PlanDiff:output_type -> tern.v1.PlanDiffResponse + 25, // 70: tern.v1.Tern.Apply:output_type -> tern.v1.ApplyResponse + 33, // 71: tern.v1.Tern.Progress:output_type -> tern.v1.ProgressResponse + 29, // 72: tern.v1.Tern.Logs:output_type -> tern.v1.LogsResponse + 35, // 73: tern.v1.Tern.Cutover:output_type -> tern.v1.CutoverResponse + 37, // 74: tern.v1.Tern.Revert:output_type -> tern.v1.RevertResponse + 39, // 75: tern.v1.Tern.SkipRevert:output_type -> tern.v1.SkipRevertResponse + 41, // 76: tern.v1.Tern.Health:output_type -> tern.v1.HealthResponse + 43, // 77: tern.v1.Tern.Stop:output_type -> tern.v1.StopResponse + 45, // 78: tern.v1.Tern.Cancel:output_type -> tern.v1.CancelResponse + 47, // 79: tern.v1.Tern.Start:output_type -> tern.v1.StartResponse + 51, // 80: tern.v1.Tern.StorageSchemaPlan:output_type -> tern.v1.StorageSchemaPlanResponse + 53, // 81: tern.v1.Tern.StorageSchemaApply:output_type -> tern.v1.StorageSchemaApplyResponse + 67, // [67:82] is the sub-list for method output_type + 52, // [52:67] is the sub-list for method input_type + 52, // [52:52] is the sub-list for extension type_name + 52, // [52:52] is the sub-list for extension extendee + 0, // [0:52] is the sub-list for field type_name } func init() { file_tern_proto_init() } @@ -5074,8 +5203,8 @@ func file_tern_proto_init() { File: protoimpl.DescBuilder{ GoPackagePath: reflect.TypeOf(x{}).PkgPath(), RawDescriptor: unsafe.Slice(unsafe.StringData(file_tern_proto_rawDesc), len(file_tern_proto_rawDesc)), - NumEnums: 4, - NumMessages: 62, + NumEnums: 5, + NumMessages: 63, NumExtensions: 0, NumServices: 1, }, diff --git a/pkg/serve/serve.go b/pkg/serve/serve.go index 03e51aa75..7ddf3e5a4 100644 --- a/pkg/serve/serve.go +++ b/pkg/serve/serve.go @@ -646,7 +646,7 @@ func connectStorage(ctx context.Context, cfg *api.ServerConfig, dialect schema.D return nil, "", fmt.Errorf("resolve storage DSN: %w", err) } if err := api.EnsureSchema(dsn, logger, - api.WithAllowDestructiveSchemaChanges(cfg.Storage.AllowDestructiveSchemaChanges), + api.WithDestructiveSchemaChangePolicy(api.ConfiguredDestructivePolicy(cfg.Storage.AllowDestructiveSchemaChanges), false), api.WithPostgresStatementTimeout(cfg.Postgres.StatementTimeoutOrDefault()), api.WithDialect(dialect)); err != nil { return nil, "", fmt.Errorf("ensure storage schema: %w", err) diff --git a/pkg/serve/serve_storage_integration_test.go b/pkg/serve/serve_storage_integration_test.go index fff93213e..e6c488c5c 100644 --- a/pkg/serve/serve_storage_integration_test.go +++ b/pkg/serve/serve_storage_integration_test.go @@ -156,8 +156,7 @@ func TestBuildCarriesLocalHostingToTheStorageSchemaAdapter(t *testing.T) { t.Cleanup(func() { assert.NoError(t, local.Close()) }) localAdapter, ok := local.storageSchema.(*storageSchemaAdapter) require.True(t, ok, "the server registers its own adapter") - _, err = localAdapter.destructivePolicy(true) - require.ErrorIs(t, err, tern.ErrInvalidStorageSchemaRequest, + require.ErrorIs(t, localAdapter.checkDestructiveOptIn(true), tern.ErrInvalidStorageSchemaRequest, "a locally hosted server has no route to a destructive storage bootstrap") deployed, err := Build(t.Context(), newConfig(), WithLogger(logger)) @@ -165,7 +164,6 @@ func TestBuildCarriesLocalHostingToTheStorageSchemaAdapter(t *testing.T) { t.Cleanup(func() { assert.NoError(t, deployed.Close()) }) deployedAdapter, ok := deployed.storageSchema.(*storageSchemaAdapter) require.True(t, ok, "the server registers its own adapter") - allow, err := deployedAdapter.destructivePolicy(true) - require.NoError(t, err) - assert.True(t, allow, "the same request is honored on a normally hosted server") + assert.NoError(t, deployedAdapter.checkDestructiveOptIn(true), + "the same request is honored on a normally hosted server") } diff --git a/pkg/serve/serve_version_test.go b/pkg/serve/serve_version_test.go index 7b128b708..5c264ec7a 100644 --- a/pkg/serve/serve_version_test.go +++ b/pkg/serve/serve_version_test.go @@ -13,7 +13,6 @@ import ( "github.com/stretchr/testify/require" "github.com/block/schemabot/pkg/api" - ternv1 "github.com/block/schemabot/pkg/proto/ternv1" "github.com/block/schemabot/pkg/schema" ) @@ -219,7 +218,7 @@ func TestStorageSchemaAdapterNeverAttributesToTheSentinel(t *testing.T) { require.NoError(t, err) assert.Empty(t, adapter.version, "the sentinel is a log value, not a version a report can attribute files to") - desired, err := adapter.desiredSchema(&ternv1.StorageSchemaPlanRequest{}) + desired, err := adapter.desiredSchema(nil, "", "diff") require.NoError(t, err) assert.Equal(t, "the schema embedded in this binary", desired.Description) } @@ -238,7 +237,7 @@ func TestStorageSchemaAdapterAttributesToANamedBuild(t *testing.T) { adapter, err := srv.newStorageSchemaService() require.NoError(t, err) - desired, err := adapter.desiredSchema(&ternv1.StorageSchemaPlanRequest{}) + desired, err := adapter.desiredSchema(nil, "", "diff") require.NoError(t, err) assert.Equal(t, "the schema embedded in v1.2.3", desired.Description) } diff --git a/pkg/serve/storage_schema.go b/pkg/serve/storage_schema.go index 52bf75971..4dcb14dd9 100644 --- a/pkg/serve/storage_schema.go +++ b/pkg/serve/storage_schema.go @@ -6,6 +6,7 @@ import ( "log/slog" "maps" "slices" + "strings" "time" "github.com/block/schemabot/pkg/api" @@ -26,13 +27,15 @@ import ( // the request, so no caller — not even the control plane — can point this at a // different database. // -// Two things a caller may influence, and neither reaches the database a -// convergence writes to. Whether destructive statements run only ever widens -// what the local config already allows, and on a locally hosted server not even -// that (see destructivePolicy). A -// desired schema sent with a diff replaces the files the comparison reads, and -// is accepted only there: the convergence RPC carries no schema, so an apply -// always runs this binary's own (see desiredSchema). +// Two things a caller may influence, and neither moves the database. Whether +// destructive statements run only ever widens what the local config already +// allows, and on a locally hosted server not even that (see +// checkDestructiveOptIn). A desired schema replaces the files both RPCs read, +// on a diff and on a convergence alike (see desiredSchema), because an operator +// rolling a later release has to be able to converge this storage to it before +// its first pod starts. What a supplied schema cannot do is widen what the +// bootstrap permits: the destructive refusal and the manual-remediation gate +// decide on supplied files the same way they decide on embedded ones (AV-9). type storageSchemaAdapter struct { // resolveDSN re-resolves the storage DSN per call rather than capturing a // string, so a credential rotated since startup is picked up the same way @@ -50,8 +53,9 @@ type storageSchemaAdapter struct { version string // configAllowsDestructive is the deployment's standing storage policy // (storage.allow_destructive_schema_changes). A boot converges under it, so - // an operator convergence must too — otherwise "apply is what a boot does" - // would stop being true on exactly the deployments that opted in. + // an operator convergence must too — an operator may name which schema + // runs, never the policy it runs under, and otherwise the two would + // disagree on exactly the deployments that opted in. configAllowsDestructive bool // localHosted marks a server the local runtime hosts, where no route to a // destructive storage bootstrap exists at all (AZ-6). @@ -136,7 +140,7 @@ func (a *storageSchemaAdapter) StorageSchemaPlan(ctx context.Context, req *ternv if err != nil { return nil, err } - desired, err := a.desiredSchema(req) + desired, err := a.desiredSchema(req.GetSchemaFiles(), req.GetSchemaSource(), "diff") if err != nil { return nil, err } @@ -151,20 +155,35 @@ func (a *storageSchemaAdapter) StorageSchemaPlan(ctx context.Context, req *ternv return &ternv1.StorageSchemaPlanResponse{Report: api.StorageSchemaReportProto(report)}, nil } -// desiredSchema resolves the schema the diff compares the live database -// against: the files the caller sent, or this binary's own when it sent none. +// desiredSchema resolves the schema a request works from: the files the caller +// sent, or this binary's own when it sent none. // -// A caller-supplied schema is accepted here and nowhere else. A diff executes -// nothing, so answering "what would this database need in order to match that -// release" is a read however the release's files arrived; the convergence RPC -// has no such field to send, so the schema a convergence runs is always this -// binary's (AV-9). -func (a *storageSchemaAdapter) desiredSchema(req *ternv1.StorageSchemaPlanRequest) (*api.StorageSchemaSource, error) { - files := req.GetSchemaFiles() +// Both RPCs resolve it here, so the schema a caller previews is the schema they +// converge. Sending files is how an operator asks about — and readies — a +// release this binary does not carry, which is the question a deploy actually +// has. What the supplied files never do is decide what may run: the convergence +// applies the bootstrap's own destructive refusal and manual-remediation gate +// to them, unchanged (AV-9). +// +// operation names what the files are for, so the log line says whether a +// supplied schema was read for a diff or run against the database. +// +// A name with no files to go with it is refused here rather than at a +// transport, because the tern gRPC server reaches this adapter without passing +// the HTTP API's validation, and this is the one half of that pair the +// defaulting below would swallow: it would resolve to the embedded schema and +// converge it, running one release's schema for a caller that named another's. +// Files with no name are refused by StorageSchemaFromFiles, which cannot +// attribute a report without one. +func (a *storageSchemaAdapter) desiredSchema(files map[string]string, schemaSource, operation string) (*api.StorageSchemaSource, error) { if len(files) == 0 { + if strings.TrimSpace(schemaSource) != "" { + return nil, fmt.Errorf("%w: schema_source %q was sent without schema_files: with no files this server's own embedded schema would run and be reported under that name; send the files, or drop schema_source to use the embedded schema", + tern.ErrInvalidStorageSchemaRequest, schemaSource) + } return api.EmbeddedStorageSchema(a.version), nil } - desired, err := api.StorageSchemaFromFiles(req.GetSchemaSource(), files) + desired, err := api.StorageSchemaFromFiles(schemaSource, files) if err != nil { // The caller wrote these files, so the reason goes back to the caller // rather than into a log it cannot read (see ErrInvalidStorageSchemaRequest). @@ -173,7 +192,8 @@ func (a *storageSchemaAdapter) desiredSchema(req *ternv1.StorageSchemaPlanReques if err := a.parseSupplied(desired); err != nil { return nil, err } - a.logger.Info("diffing storage schema against a supplied schema", + a.logger.Info("storage schema request carries a supplied schema", + "operation", operation, "dialect", a.dialect, "schema_source", desired.Description, "schema_file_count", len(files), @@ -191,21 +211,26 @@ func (a *storageSchemaAdapter) StorageSchemaApply(ctx context.Context, req *tern return nil, err } opts = append(opts, api.WithConvergenceTimeout(budget)) + desired, err := a.desiredSchema(req.GetSchemaFiles(), req.GetSchemaSource(), "converge") + if err != nil { + return nil, err + } // ApplyStorageSchema cancels its convergence when its context is cancelled // — that is what lets an operator at a terminal stop a run they are // watching. This context is a request's, and cancelling it means the // connection dropped, not that anyone decided anything. Stripping the // cancellation is what keeps a lost TCP connection from taking a table // copy with it; the budget above is still what bounds the run. - planned, remaining, err := api.ApplyStorageSchema(context.WithoutCancel(ctx), dsn, a.logger, opts...) + planned, remaining, err := api.ApplyStorageSchema(context.WithoutCancel(ctx), dsn, desired, a.logger, opts...) if err != nil { - return nil, fmt.Errorf("converge storage schema (dialect %s): %w", a.dialect, err) + return nil, fmt.Errorf("converge storage schema (dialect %s) to %s: %w", a.dialect, desired.Describe(), err) } planned.AttributeTo(a.version) remaining.AttributeTo(a.version) a.logger.InfoContext(ctx, "storage schema convergence answered", "dialect", a.dialect, "database", remaining.Database, + "schema_source", remaining.SchemaSource, "caller", req.GetCaller(), "planned_count", len(planned.Outstanding), "remaining_count", len(remaining.Outstanding), @@ -264,13 +289,12 @@ func (a *storageSchemaAdapter) target(requestAllowsDestructive bool) (string, [] if err := a.checkBootTarget(dsn); err != nil { return "", nil, err } - allowDestructive, err := a.destructivePolicy(requestAllowsDestructive) - if err != nil { + if err := a.checkDestructiveOptIn(requestAllowsDestructive); err != nil { return "", nil, err } return dsn, []api.EnsureSchemaOption{ api.WithDialect(a.dialect), - api.WithAllowDestructiveSchemaChanges(allowDestructive), + api.WithDestructiveSchemaChangePolicy(api.ConfiguredDestructivePolicy(a.configAllowsDestructive), requestAllowsDestructive), api.WithPostgresStatementTimeout(a.postgresStatementTimeout), }, nil } @@ -282,9 +306,9 @@ func (a *storageSchemaAdapter) target(requestAllowsDestructive bool) (string, [] // a restart, and a rotation is the only movement that may be honored. A DSN // that now names a different server or a different database describes some // other SchemaBot's storage: reading it would attribute another instance's -// drift to this one, and converging it would run this binary's embedded schema -// against a database it never booted on. Both are refused, which is the -// binding the adapter documents and the one AV-9 rests on. +// drift to this one, and converging it would run DDL against a database this +// instance never booted on. Both are refused, which is the binding the adapter +// documents and the one AV-9 rests on. func (a *storageSchemaAdapter) checkBootTarget(dsn string) error { resolved, err := storageTargetFor(a.dialect, dsn) if err != nil { @@ -302,17 +326,21 @@ func (a *storageSchemaAdapter) checkBootTarget(dsn string) error { "the storage schema surface only answers for the database this instance is running on, so restart it to adopt the new storage", resolved, a.bootTarget) } -// destructivePolicy resolves whether this call may run destructive statements: -// the deployment's policy widened by an explicit per-request opt-in, never -// narrowed by its absence — and never widened at all on a locally hosted -// server, which refuses the request instead. +// checkDestructiveOptIn admits or refuses a per-request opt-in to destructive +// statements. It resolves nothing: the two policies travel separately into the +// bootstrap options, which is what lets a report say what this call runs and +// what a boot runs without the second being inferred from the first. +// +// What this decides is whether the opt-in may be honored at all — it is never +// honored on a locally hosted server, which refuses the request rather than +// widening anything. // // The two directions are not symmetric and the asymmetry is the point. A // deployment that configured allow_destructive_schema_changes has already made // the decision for every boot; a convergence that ignored it would run less -// than the next boot runs, so "apply is what a boot does" — the property that -// makes this usable as a pre-deploy convergence step — would quietly stop -// holding there. In the other direction, a request opting in is exactly the +// than the next boot runs — an apply refusing on a deployment where a boot +// proceeds, which is the deployment an operator converging ahead of a roll most +// needs it not to. In the other direction, a request opting in is exactly the // explicit operator consent AV-9 asks for before surplus storage state is // destroyed, arriving through a command an admin had to issue rather than // through a config file nobody re-read. @@ -324,14 +352,14 @@ func (a *storageSchemaAdapter) checkBootTarget(dsn string) error { // rather than running the safe remainder under a flag it ignored, so an // operator learns their opt-in did not apply instead of reading a report that // looks like it did (AZ-6). -func (a *storageSchemaAdapter) destructivePolicy(requestAllowsDestructive bool) (bool, error) { +func (a *storageSchemaAdapter) checkDestructiveOptIn(requestAllowsDestructive bool) error { if requestAllowsDestructive && a.localHosted { a.logger.Warn("refusing a storage schema request that opts in to destructive statements: this server is locally hosted", "dialect", a.dialect, "boot_target", a.bootTarget.String(), ) - return false, fmt.Errorf("%w: this server is locally hosted, which never runs destructive storage schema statements; "+ + return fmt.Errorf("%w: this server is locally hosted, which never runs destructive storage schema statements; "+ "re-run without the destructive opt-in to converge everything else, or address a deployed server to run them", tern.ErrInvalidStorageSchemaRequest) } - return a.configAllowsDestructive || requestAllowsDestructive, nil + return nil } diff --git a/pkg/serve/storage_schema_test.go b/pkg/serve/storage_schema_test.go index f78c58933..59c07fb8e 100644 --- a/pkg/serve/storage_schema_test.go +++ b/pkg/serve/storage_schema_test.go @@ -20,41 +20,6 @@ import ( "github.com/block/schemabot/pkg/tern" ) -// A request may widen the deployment's destructive-statement policy and can -// never narrow it. -// -// The asymmetry is the point. A deployment that configured -// allow_destructive_schema_changes has already decided for every boot, so a -// convergence that ignored it would run less than the next boot runs — and -// "apply is what a boot does", the property that makes this usable as a -// pre-deploy step, would quietly stop holding there. In the other direction, a -// request opting in is the explicit operator consent required before surplus -// storage state is destroyed. -func TestStorageSchemaAdapter_DestructivePolicy(t *testing.T) { - tests := []struct { - name string - configAllows bool - requestAllows bool - want bool - }{ - {"neither", false, false, false}, - {"request opts in", false, true, true}, - {"config already opted in", true, false, true}, - {"both", true, true, true}, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - adapter := &storageSchemaAdapter{ - configAllowsDestructive: tc.configAllows, - logger: slog.New(slog.DiscardHandler), - } - allow, err := adapter.destructivePolicy(tc.requestAllows) - require.NoError(t, err) - assert.Equal(t, tc.want, allow) - }) - } -} - // A request naming no budget runs under the boot's, and one naming a budget // runs under exactly that (AV-11). The control plane always names the budget // it resolved, so a zero here is a caller reaching the data plane directly @@ -103,15 +68,14 @@ func TestStorageSchemaAdapter_ApplyRefusesAnOutOfRangeBudget(t *testing.T) { // runtime cannot say who issued the command, and a local host can be pointed at // a real deployment's storage, so consent arriving this way is not the consent // the widening is granted for. -func TestStorageSchemaAdapter_DestructivePolicyRefusesTheOptInWhenLocallyHosted(t *testing.T) { +func TestStorageSchemaAdapter_RefusesTheDestructiveOptInWhenLocallyHosted(t *testing.T) { adapter := &storageSchemaAdapter{ localHosted: true, logger: slog.New(slog.DiscardHandler), } - allow, err := adapter.destructivePolicy(true) + err := adapter.checkDestructiveOptIn(true) require.Error(t, err) - assert.False(t, allow) assert.ErrorIs(t, err, tern.ErrInvalidStorageSchemaRequest, "the request is what is wrong, not this server") assert.Contains(t, err.Error(), "locally hosted") assert.Contains(t, err.Error(), "re-run without the destructive opt-in", @@ -120,15 +84,13 @@ func TestStorageSchemaAdapter_DestructivePolicyRefusesTheOptInWhenLocallyHosted( // Local hosting refuses the destructive opt-in without refusing the request // that never asked for it: an ordinary local convergence still runs. -func TestStorageSchemaAdapter_DestructivePolicyAllowsAnOrdinaryLocalConvergence(t *testing.T) { +func TestStorageSchemaAdapter_AllowsAnOrdinaryLocalConvergence(t *testing.T) { adapter := &storageSchemaAdapter{ localHosted: true, logger: slog.New(slog.DiscardHandler), } - allow, err := adapter.destructivePolicy(false) - require.NoError(t, err) - assert.False(t, allow) + require.NoError(t, adapter.checkDestructiveOptIn(false)) } // The server carries its local hosting into the adapter it builds, so the @@ -146,8 +108,7 @@ func TestNewStorageSchemaServiceCarriesLocalHosting(t *testing.T) { adapter, err := srv.newStorageSchemaService() require.NoError(t, err) - _, err = adapter.destructivePolicy(true) - require.ErrorIs(t, err, tern.ErrInvalidStorageSchemaRequest) + require.ErrorIs(t, adapter.checkDestructiveOptIn(true), tern.ErrInvalidStorageSchemaRequest) } // The refusal is reached through the path every RPC takes, not only by calling @@ -375,7 +336,7 @@ func TestStorageSchemaAdapter_RefusesWithoutStorageDSN(t *testing.T) { func TestStorageSchemaAdapter_DesiredSchemaDefaultsToThisBinary(t *testing.T) { adapter := &storageSchemaAdapter{version: "v1.2.3", logger: slog.New(slog.DiscardHandler)} - desired, err := adapter.desiredSchema(&ternv1.StorageSchemaPlanRequest{}) + desired, err := adapter.desiredSchema(nil, "", "diff") require.NoError(t, err) assert.Equal(t, "the schema embedded in v1.2.3", desired.Description) assert.Empty(t, desired.Files, "the answering binary's own files are read here, not sent to it") @@ -388,15 +349,28 @@ func TestStorageSchemaAdapter_DesiredSchemaDefaultsToThisBinary(t *testing.T) { func TestStorageSchemaAdapter_DesiredSchemaAcceptsASuppliedSchema(t *testing.T) { adapter := &storageSchemaAdapter{version: "v1.2.3", dialect: schema.DialectMySQL, logger: slog.New(slog.DiscardHandler)} - desired, err := adapter.desiredSchema(&ternv1.StorageSchemaPlanRequest{ - SchemaSource: "the schema files of release v1.4.0", - SchemaFiles: map[string]string{"applies.sql": "CREATE TABLE `applies` (`id` BIGINT UNSIGNED PRIMARY KEY)"}, - }) + desired, err := adapter.desiredSchema( + map[string]string{"applies.sql": "CREATE TABLE `applies` (`id` BIGINT UNSIGNED PRIMARY KEY)"}, + "the schema files of release v1.4.0", "diff") require.NoError(t, err) assert.Equal(t, "the schema files of release v1.4.0", desired.Description) assert.Len(t, desired.Files, 1) } +// A schema named with no files to go with it is an invalid request, and it is +// refused in this adapter because the tern gRPC server reaches it without +// passing the HTTP API's validation. Defaulting to the embedded schema here +// would converge it successfully, so a caller that asked for one release's +// schema would have another's run against its storage and be told it worked. +func TestStorageSchemaAdapter_DesiredSchemaRefusesANameWithNoFiles(t *testing.T) { + adapter := &storageSchemaAdapter{version: "v1.2.3", dialect: schema.DialectMySQL, logger: slog.New(slog.DiscardHandler)} + + _, err := adapter.desiredSchema(nil, "the schema files of release v1.4.0", "converge") + require.ErrorIs(t, err, tern.ErrInvalidStorageSchemaRequest, + "a source with no files must be an invalid request, not a silent convergence of the embedded schema") + assert.Contains(t, err.Error(), "without schema_files") +} + // An unusable supplied schema is refused before anything reads a database. A // file set that cannot be read as one .sql file per table would otherwise diff // as a storage database full of surplus tables. @@ -407,9 +381,9 @@ func TestStorageSchemaAdapter_DesiredSchemaAcceptsASuppliedSchema(t *testing.T) func TestStorageSchemaAdapter_DesiredSchemaRefusesAnUnusableSchema(t *testing.T) { adapter := &storageSchemaAdapter{version: "v1.2.3", logger: slog.New(slog.DiscardHandler)} - _, err := adapter.desiredSchema(&ternv1.StorageSchemaPlanRequest{ - SchemaFiles: map[string]string{"applies.sql": "CREATE TABLE `applies` (`id` BIGINT UNSIGNED PRIMARY KEY)"}, - }) + _, err := adapter.desiredSchema( + map[string]string{"applies.sql": "CREATE TABLE `applies` (`id` BIGINT UNSIGNED PRIMARY KEY)"}, + "", "diff") require.Error(t, err, "files with no source leave the report unable to attribute its answer") assert.Contains(t, err.Error(), "needs a description") assert.ErrorIs(t, err, tern.ErrInvalidStorageSchemaRequest) @@ -442,10 +416,7 @@ func TestStorageSchemaAdapter_DesiredSchemaRefusesUnparseableContent(t *testing. t.Run(tc.name, func(t *testing.T) { adapter := &storageSchemaAdapter{version: "v1.2.3", dialect: tc.dialect, logger: slog.New(slog.DiscardHandler)} - _, err := adapter.desiredSchema(&ternv1.StorageSchemaPlanRequest{ - SchemaSource: "the schema files in ./schema", - SchemaFiles: tc.files, - }) + _, err := adapter.desiredSchema(tc.files, "the schema files in ./schema", "diff") require.Error(t, err) assert.ErrorIs(t, err, tern.ErrInvalidStorageSchemaRequest) assert.Contains(t, err.Error(), `schema file "applies.sql"`, "the refusal has to name the file the caller must fix") @@ -460,10 +431,11 @@ func TestStorageSchemaAdapter_DesiredSchemaRefusesUnparseableContent(t *testing. func TestStorageSchemaAdapter_DesiredSchemaBlamesAnUnparseableDialectOnTheServer(t *testing.T) { adapter := &storageSchemaAdapter{version: "v1.2.3", dialect: schema.Dialect("cockroach"), logger: slog.New(slog.DiscardHandler)} - _, err := adapter.desiredSchema(&ternv1.StorageSchemaPlanRequest{ - SchemaSource: "the schema files in ./schema", - SchemaFiles: map[string]string{"applies.sql": "CREATE TABLE applies (id BIGINT)"}, - }) + _, err := adapter.desiredSchema( + map[string]string{"applies.sql": "CREATE TABLE applies (id BIGINT)"}, + "the schema files in ./schema", + "diff", + ) require.Error(t, err) assert.NotErrorIs(t, err, tern.ErrInvalidStorageSchemaRequest) assert.Contains(t, err.Error(), "no statement parser registered")