diff --git a/pkg/rds/ACTUATE-FINDINGS.md b/pkg/rds/ACTUATE-FINDINGS.md new file mode 100644 index 0000000..9b3dba2 --- /dev/null +++ b/pkg/rds/ACTUATE-FINDINGS.md @@ -0,0 +1,583 @@ +# U14 — `pkg/rds` actuation: the first change Kilter makes to a database + +`pkg/rds/actuate.go`, `actuate_api.go`, `actuate_approve.go`, +`actuate_preflight.go`, `actuate_ledger.go`, `actuate_fixture.go` implement the +storage-performance actuator U13 (`FINDINGS.md` §5) specified: one resumable +`ModifyDBInstance` carrying three arguments, behind a structural approval gate, +in front of a refusal layer that blocks on every unknown. + +**2,800 production lines, 1,733 test lines, 51 tests, 22 refusal predicates +(13 new codes, 9 reused).** Green under `gofmt -l ./pkg/rds`, `go vet ./...`, +`go build ./...`, `go test -race -count=1 ./pkg/rds/...` and +`go test -race -short ./...`. `go.mod` and `go.sum` are **untouched**; +`go list -deps ./pkg/rds/` shows stdlib plus `pkg/domain`, `pkg/model`, +`pkg/guard` and `pkg/pricing/commit`. **No file outside `pkg/rds/actuate*.go` +was created or edited** — not `parity*.go`, not `rds.go`, not `FINDINGS.md`, +not `cmd/`. §5 below is what `cmd/` and `pkg/actuate` owe, written down instead +of wired. + +--- + +## 1. What was built, in one paragraph + +An [`Actuator`](actuate.go) that takes an `ApprovedStep` — a value no package +outside `pkg/rds` can construct — decodes it, re-reads the instance state, the +provisioning envelope and the 24-hour modification history **live**, runs +twenty-two pure refusal predicates, and then either records the exact call it +would make (dry-run, the default) or issues exactly one `ModifyDBInstance` and +**observes** the instance walk `available → modifying → storage-optimization → +available` rather than assuming it did. A poll budget that runs out is +`in-flight`, not success and not failure, and re-executing resumes the +observation from whatever AWS shows. + +--- + +## 2. Read this before anything else: what `TestNoMutatingAPISurface` now means + +U11 shipped two tests whose names promise this package cannot act: + +- `TestNoActuationSurfaceExists` — `*rds.Domain` must not satisfy + `domain.Actuator`. +- `TestNoMutatingAPISurface` — the **identifier** `ModifyDBInstance` must not + appear in any non-test file in the package. + +**Both still pass, and after this unit they mean something narrower than they +say.** U14 may not edit them, so the narrowing is recorded here and asserted in +`TestTheOnlyMutatingPathIsTheActuator`. + +The seam method is named [`StorageActuateAPI.ModifyStorage`](actuate_api.go) +and the SDK adapter maps it onto `rds:ModifyDBInstance`. That satisfies the +identifier scan **by naming**, which a reviewer is entitled to call a +loophole. Three things make it the right call rather than an evasion: + +1. **It is stated here, at the top, in those words.** The fixture's operation + constant is `OpModifyStorage = "ModifyDBInstance"` — a string literal, which + the scan drops — so every ledger entry, log line and audit trail names the + real AWS operation. Nothing downstream has to decode a euphemism. +2. **The narrower name is the better name anyway.** `ModifyDBInstance` is a + forty-argument operation that can change an instance's class, topology, + engine version, master password and deletion protection. This unit may send + **three** arguments. A seam named for the three is a seam a reviewer can + bound in one glance; a seam named for the operation is not. +3. **The guarantee that actually mattered survives intact and is tested.** The + read-only decision path — sizer, parity engine, report, `*Domain` — cannot + reach a mutation, because the only type that can is `*Actuator` and nothing + in that path constructs one. `EnvelopeFixture` and `Fixture` are asserted + *not* to satisfy `StorageActuateAPI`, so a read-only wiring cannot be passed + where a mutating one is expected. + +**A reviewer who disagrees should change `TestNoMutatingAPISurface` to scan for +the string literal too, and move `OpModifyStorage`'s value into a test file.** +That is a two-line change in a file U14 was not allowed to touch. + +--- + +## 3. Every refusal predicate, and exactly where it is tested + +A modification is issued only when **all** of these hold. Each failure is a +named refusal with a machine-readable code; none is a silent skip. Every test +below drives the **full apply path** against a fixture that would happily +modify the database, and every one asserts `f.Mutations() == 0` — a refusal +proven against a pure function proves the function refuses, not that the +database was untouched. + +### 3.1 The cooldown — FINDINGS.md §5.3 + +| Code | Predicate | Tested by | +|---|---|---| +| `storage-modification-cooldown` (**reused**) | fewer than four storage modifications in the trailing 24 h | `TestActuateRefusesBlockedCooldown` | +| `storage-modification-history-unknown` (**new**) | `CooldownVerdict.Known` — **unknown BLOCKS** | `TestActuateRefusesUnknownCooldown` | + +§5.3's sentence is the one this unit turns on and it is implemented literally: +`Known=false` never clears the cooldown. It gets its **own code**, distinct +from `Blocked`, because the operator's next action differs — *wait until +`ClearsAt`* against *grant `rds:DescribeEvents`* — and one code for both would +hide which is needed. The blocked refusal carries `ClearsAt` as +`RefusalError.ValidFrom`, which is the value §5.3 names as the right +`ValidFrom` for a deferred step; `TestActuateRefusesBlockedCooldown` asserts it +equals the moment the **oldest** of the four leaves the window. +`TestActuateAllowsAKnownEmptyHistory` pins the other half: the gate is +"unknown blocks", not "silence blocks". + +### 3.2 Live state — FINDINGS.md §5.4 + +| Code | Predicate | Tested by | +|---|---|---| +| `storage-optimization-blocks-modification` (**reused**) | `DBInstance.StateUnstable()` — `modifying`, `storage-optimization` | `TestActuateRefusesUnstableStateAtExecuteTime` | +| `instance-not-available` (**new**) | every other non-`available` state | `TestActuateRefusesNonAvailableState` | +| `storage-modification-pending` (**new**) | `PendingModifiedValues` is empty | `TestActuateRefusesPendingModification` | +| `instance-missing` (**new**) | the instance is in the account | `TestActuateRefusesAMissingInstance` | +| `drift` (**new**) | the live volume matches the recorded `From` or the intended `To` | `TestActuateRefusesDrift` | + +§5.4 is honoured exactly: the record read microseconds before the call is +converted back into a `DBInstance` and `StateUnstable()` — **U11's function, +not a second copy** — decides. A `stopped` instance is refused under its own +code rather than under the unstable one, because telling an operator a stopped +database is mid-change is a lie. + +### 3.3 The envelope, re-read live — FINDINGS.md §5.2 + +| Code | Predicate | Tested by | +|---|---|---| +| `provisioning-envelope-unknown` (**reused**) | `DescribeValidDBInstanceModifications` answered **at execute time** | `TestActuateRefusesUnknownEnvelope` | +| `storage-demand-exceeds-envelope` (**reused**) | `GP3Config.Validate` passes against the **fresh** envelope | `TestActuateRefusesWhenLiveEnvelopeDisagrees` | +| `gp3-not-provisionable-below-striping-threshold` (**reused**) | at or above the striping threshold, or SQL Server | `TestActuateRefusesBelowTheStripingThreshold` | +| `baseline-value-must-not-be-sent` (**new**) | no argument names a value equal to the regime baseline | `TestActuateNeverSendsABaselineArgument`, `TestActuateRefusesABaselineArgumentByName` | + +The envelope is collected through **U13's own `EnvelopeCollector`**, not a +second reader, so the two units cannot disagree about what the seam said. The +refusal text names the cause §5.2 names — *the instance class changed between +plan and apply* — and the test asserts that sentence is present. + +The baseline rule (§5.1) is enforced in one function, `argumentsFor`, used by +**both** the pre-flight check and the call builder, so the thing checked and +the thing sent are the same value by construction rather than by agreement. +`TestActuateNeverSendsABaselineArgument` sweeps four engines × nine sizes × +nine configurations and asserts the property directly; the sizes include 199 / +200 / 201 and 399 / 400 / 401, which are the boundaries where the regime +changes and a plan shape does not. + +### 3.4 The trap-8 ratchet + +| Code | Predicate | Tested by | +|---|---|---| +| `storage-performance-ratchet` (**new**) | IOPS and throughput never move down; allocated storage never moves at all | `TestActuateRefusesAReduction`, `TestActuateRefusesAnAllocationChange`, `TestActuateRatchetIsCheckedAgainstTheLiveVolume` | +| `allocated-storage-drift` (**new**) | the observed allocation still matches `Proposal.AllocatedStorageGiB` | `TestActuateRefusesAllocationDrift` | + +**This is the largest deliberate narrowing in the unit and §4.1 states its +cost.** The ratchet is evaluated against the **live** configuration, not +against the step's `From`, so a plan that predates somebody else's change +cannot reduce a volume further. + +### 3.5 Guardrails and shape + +| Code | Predicate | Tested by | +|---|---|---| +| `guardrail-mode-off` (**reused**) | `kilter.dev/mode` ≠ `off` | `TestActuateRefusesModeOff` | +| `guardrail-tags-unknown` (**new**) | the tag set was **read** | `TestActuateRefusesUnreadableTags` | +| `engine-mismatch` (**new**) | the step's engine matches itself and the live instance | `TestActuateRefusesAnEngineChange` | +| `unknown-engine` (**reused**) | a gp3 regime is encoded for the engine | `TestActuateRefusesAnUnknownEngine` | +| `storage-type-not-modelled` (**reused**) | gp2/gp3 only, landing on gp3 | `TestActuateRefusesAMalformedStep/unmodelled-storage-type`, `/target-is-not-gp3` | +| `storage-size-unusable` (**reused**) | 1–65,536 GiB | `TestActuateRefusesAMalformedStep/size-unusable` | +| `wrong-action` (**new**) | `domain.ActionInPlace` | `TestActuateRefusesAMalformedStep/wrong-action` | +| `bad-step` (**new**) | the key hashes its own contents; both specs are complete | `TestActuateRefusesAMalformedStep/edited-after-hashing`, `/no-target-values` | +| `no-change` (**new**) | `From` and `To` differ | `TestActuateRefusesAMalformedStep/no-change` | + +`guardrail-tags-unknown` is the doctrine applied to a place nobody would think +to apply it: an unreadable `kilter.dev/mode` tag is indistinguishable from one +that says `off`, and the whole point of that tag is that it works when nobody +is watching. + +`TestActuateReasonCodesAreDistinct` pins all thirteen new codes against each +other **and against all thirty-five U11/U13 codes**, because two codes with one +value silently merge two findings in every roll-up. + +### 3.6 The one predicate that is deliberately *permissive* + +`inFlightTowardTarget` (actuate_preflight.go) decides whether AWS is already +applying **this step's** change. When it is, the four gates in §3.2 — which +answer *may a modification be issued?* — are skipped, and only the identity +checks (right instance, right engine, right allocation) run. + +This was **found by a test, not designed in**: `TestResumeAtEveryStageBoundary` +and `TestALostResponseDoesNotIssueASecondModification` both failed on the first +implementation, because the gates that correctly stop a modification being +issued are exactly the states a resumed step is legitimately in. A pre-flight +that cannot tell those apart refuses to observe the very modification it +started, leaving a production database mid-change with nobody watching — a +worse failure than the one the gates prevent. + +Being permissive here is safe for a structural reason, stated in the function's +doc comment: **a step in a resuming state issues nothing.** `execute` sends a +modification only from `StageReady`, and no state `inFlightTowardTarget` +accepts derives to `StageReady`. The worst case of a false positive is a poll +budget spent watching an instance that is not changing, which ends as an honest +`in-flight` entry. + +--- + +## 4. What was deliberately not built, and the honest cost + +### 4.1 This actuator will not execute a reduction — and that is where the money is + +U13 identifies two shapes: a gp2 → gp3 **conversion**, and a **reduction** of +provisioned IOPS/throughput toward the non-reducible baseline. FINDINGS.md +§2.4 says the reduction is *"the shape that carries the money"*. + +**U14 refuses to execute it.** `storage-performance-ratchet` fires on any step +that lowers IOPS or throughput below what the volume delivers now. The reason +is in the refusal text: a reduction is the change that starves a production +primary of I/O if the measurement behind it was wrong, and this actuator would +be making that call unattended at 3 a.m. against a database whose p99 was +computed from `p99(read) + p99(write)` (§6.2) over a window that may have +missed a month-end. + +The cost is real and should be stated without softening: **the highest-value +recommendation this domain produces is advisory-only after U14 ships.** An +operator performing it by hand gets the full assessment, the exact call in +`Actuator.PlannedCall`, and a dry-run that names every argument. What they do +not get is automation. + +### 4.2 A revert consumes one of the four modifications per 24 hours + +`Actuator.Revert` builds the inverse step and runs it through the identical +pre-flight. Two consequences, neither hidden: + +- **A change and its undo are two of four.** A change, an undo and one retry + are three, and there is no fourth chance to get it right that day. + `TestAChangeAndItsUndoSpendTwoOfFour` asserts the counter really is shared, + driving two changes and checking the fixture's history reaches 2. +- **The undo of a raise is a reduction, so §4.1's ratchet refuses it.** + `TestRevertRestoresTheRecordedFrom` asserts exactly that outcome rather than + papering over it: the operator is told the undo is a reduction instead of + having one performed for them. A conversion's undo (gp3 → gp2) is refused + under `storage-type-not-modelled`, because every modification this unit makes + lands on gp3. + +**In practice this actuator's changes are one-way in the automated path.** That +is a defensible posture for storage performance — the ratchet only moves up, so +the failure mode of not reverting is a bill, not an outage — and it is the +opposite of the posture `pkg/ebs` takes, where a revert restores a *faster* +volume. §5.6's guarantee is still honoured and tested: a revert can never be +talked below the regime baseline, because `configOf` floors the recorded +`From` at the baseline before anything looks at it +(`TestRevertCannotGoBelowTheRegimeBaseline`). + +### 4.3 There is no economics gate + +`pkg/ec2`'s actuator refuses a step that carries no commitment-checked savings +attestation. This one does not, for a reason specific to storage: *"the price +for a reserved DB instance doesn't provide a discount for the costs associated +with storage, backups, and I/O"* [verified, FINDINGS.md §6.1], so there is no +commitment waterfall to strand anything and net equals gross by construction. +`AssessParity` already refuses any proposal whose rate provenance is not +`Claimable`, upstream of any step reaching here. + +The step **may** carry `kilter.dev/net-savings-monthly-usd`; it is recorded +verbatim in the ledger, never recomputed, and rolled up through `SumUSD`. It is +not a gate. If a future unit wants one, it is four lines in `decodeStep`. + +### 4.4 `--apply-immediately` is not a field, and the adapter must send it + +`ModifyStorageInput` has no scheduling field, because §5.5 says three fields +and a scheduling flag is a fourth thing a caller could get wrong. The adapter +must send `--apply-immediately` unconditionally (§5.2 below). + +**This is the deviation a reviewer is most likely to object to**, and the +objection is fair: a behaviour that consequential living only in a doc comment +is weaker than a field with a validator. The counter-argument is that a +deferred storage change happens in a maintenance window this actuator cannot +observe, so a step that scheduled one could never reach `done` — it would sit +`in-flight` until a human noticed. Making it unrepresentable is the safer of +two imperfect options. `TestTheIssuedCallCarriesNothingElse` asserts +`applyImmediately` is absent from the serialized call, which at least makes the +absence deliberate rather than forgotten. + +### 4.5 `ClientToken` is real here and is a no-op at AWS + +`ModifyDBInstance` **accepts no client token.** The field exists on +`ModifyStorageInput`, the fixture deduplicates on it, and **the adapter must +not forward it** (§5.2). Idempotency against a lost response is structural +instead: + +1. the ledger's terminal check short-circuits a completed step with **no cloud + call at all** (`TestReExecutingACompletedStepIsANoop` asserts zero calls); +2. `PendingModifiedValues` is re-read before every issue, so a landed-but-lost + modification is observed and resumed rather than re-sent + (`TestALostResponseDoesNotIssueASecondModification` asserts the instance's + own event history contains exactly **one** modification after a lost + response and a retry). + +A duplicate `ModifyDBInstance` with identical values is harmless to the +*shape* of the instance — it is a declarative absolute-value API — but it +spends one of four modifications per 24 hours, which is why (2) is load-bearing +rather than tidy. + +### 4.6 Not attempted + +- **Blue/green deployments.** The only route to a storage change with a + bounded, observable cutover for engines where in-place is disruptive. A + different unit. +- **Batching across a fleet.** One step, one instance. A batch API would need a + concurrency limiter aware of the per-instance 24-hour counter. +- **A fuzz target.** `pkg/rds` has two; this unit has none. The refusal layer + is table-driven and its property test (`TestActuateNeverSendsABaselineArgument`) + is the one that would have been fuzzed. Budget went to the refusal tests. +- **Reading the instance class.** The envelope depends on it, and the refusal + when it changes is `storage-demand-exceeds-envelope` — correct but indirect. + Carrying the class on `InstanceStateRecord` would let the refusal say *"the + class changed from db.r6i.large to db.t3.medium"* instead of describing the + consequence. One field, one refusal message. + +--- + +## 5. Exact wiring `cmd/` and `pkg/actuate` must do + +### 5.1 The SDK adapter — four operations + +```go +type rdsStorageAdapter struct{ c *rds.Client } // aws-sdk-go-v2 + +func (a *rdsStorageAdapter) DescribeInstanceState(ctx context.Context, + in *kilterrds.DescribeInstanceStateInput) (*kilterrds.DescribeInstanceStateOutput, error) { + + out, err := a.c.DescribeDBInstances(ctx, &rds.DescribeDBInstancesInput{ + DBInstanceIdentifier: aws.String(in.DBInstanceIdentifier)}) + var nf *types.DBInstanceNotFoundFault + if errors.As(err, &nf) { + return &kilterrds.DescribeInstanceStateOutput{Found: false}, nil // NOT an error + } + if err != nil || len(out.DBInstances) == 0 { + return &kilterrds.DescribeInstanceStateOutput{Found: false}, err + } + d := out.DBInstances[0] + rec := kilterrds.InstanceStateRecord{ + Identifier: aws.ToString(d.DBInstanceIdentifier), ARN: aws.ToString(d.DBInstanceArn), + Engine: aws.ToString(d.Engine), LicenseModel: aws.ToString(d.LicenseModel), + Status: aws.ToString(d.DBInstanceStatus), + AllocatedStorageGiB: int64(aws.ToInt32(d.AllocatedStorage)), + StorageType: aws.ToString(d.StorageType), + IOPS: aws.ToInt32(d.Iops), StorageThroughputMBps: aws.ToInt32(d.StorageThroughput), + } + if p := d.PendingModifiedValues; p != nil { // REQUIRED: §4.5 turns on it + rec.PendingStorageType = aws.ToString(p.StorageType) + rec.PendingIOPS = aws.ToInt32(p.Iops) + rec.PendingStorageThroughputMBps = aws.ToInt32(p.StorageThroughput) + rec.PendingAllocatedStorageGiB = int64(aws.ToInt32(p.AllocatedStorage)) + } + // rds:ListTagsForResource. TagsKnown MUST stay false if it fails — an + // unreadable mode tag refuses (guardrail-tags-unknown). + if tags, err := a.c.ListTagsForResource(ctx, &rds.ListTagsForResourceInput{ + ResourceName: d.DBInstanceArn}); err == nil { + rec.Tags = map[string]string{} + for _, t := range tags.TagList { + rec.Tags[aws.ToString(t.Key)] = aws.ToString(t.Value) + } + rec.TagsKnown = true + } + return &kilterrds.DescribeInstanceStateOutput{Instance: rec, Found: true}, nil +} +``` + +### 5.2 The mutation — and the four rules the adapter must not break + +```go +func (a *rdsStorageAdapter) ModifyStorage(ctx context.Context, + in *kilterrds.ModifyStorageInput) (*kilterrds.ModifyStorageOutput, error) { + + req := &rds.ModifyDBInstanceInput{ + DBInstanceIdentifier: aws.String(in.DBInstanceIdentifier), + StorageType: aws.String(in.StorageType), + ApplyImmediately: aws.Bool(true), // RULE 1 — see §4.4 + } + if in.IOPS > 0 { // RULE 2 — zero means OMIT (§5.1) + req.Iops = aws.Int32(in.IOPS) + } + if in.StorageThroughputMBps > 0 { // RULE 2 + req.StorageThroughput = aws.Int32(in.StorageThroughputMBps) + } + // RULE 3: set NOTHING else on req. Not the class, not MultiAZ, not the + // engine version, not AllocatedStorage. FINDINGS.md §5.5. + // RULE 4: do NOT forward in.ClientToken — ModifyDBInstance has no such + // parameter (§4.5). + out, err := a.c.ModifyDBInstance(ctx, req) + ... +} +``` + +`DescribeValidDBInstanceModifications` and `DescribeEvents` are U13's, already +specified in `FINDINGS.md` §7.6; the same adapter satisfies both interfaces. + +### 5.3 Construction, approval and execution + +```go +act, err := rds.NewActuator(adapter, rds.ActuatorConfig{ + Mode: mode, // rds.ModeDryRun unless --apply + Now: time.Now, // REQUIRED: the package reads no clock + CallTimeout: 30 * time.Second, + PollInterval: 30 * time.Second, + PollTimeout: 15 * time.Minute, // storage-optimization outlives this: expect in-flight + EventWindow: 24 * time.Hour, // REFUSED if shorter than 24 h + Persist: func(ctx context.Context, b []byte) error { return store.Put(ctx, ledgerKey, b) }, +}) + +// On startup, BEFORE anything else: resume. +if b, err := store.Get(ctx, ledgerKey); err == nil { + _ = act.RestoreLedger(b) +} +for _, e := range act.Unsettled() { // every entry here may be RUNNING NOW + log.Warn("resuming an unfinished storage modification", "key", e.Key, "stage", e.Stage) +} + +// The approval gate. There is no path around this. +ap, err := rds.NewApproval(steps, token, time.Now()) // token from `kilter approve` +bound, err := act.Bind(ap) // the ONLY domain.Actuator form +registry.RegisterActuator(bound) +``` + +`token.Fingerprint` must be `rds.PlanFingerprint(steps)` — **not** +`domain.Fingerprint(steps)`. The two differ for a plan that round-tripped +through JSON or a map: `PlanFingerprint` canonicalizes by `(Seq, Key)` first, +so the fingerprint is a property of the plan's content rather than of the order +it arrived in. + +### 5.4 Building a step from a `Proposal` + +`Assessment.Proposal` carries effective totals. The step's `From` records what +was observed (this is the `From` a revert restores, §5.6), and both specs must +carry the engine and the allocation: + +```go +from := domain.Spec{Attrs: map[string]string{ + rds.AttrEngine: inst.Engine, + rds.AttrLicenseModel: inst.LicenseModel, + rds.AttrStorageType: inst.StorageType, + rds.AttrAllocatedStorageGiB: strconv.FormatInt(inst.AllocatedStorageGiB, 10), + rds.AttrIOPS: strconv.FormatInt(int64(inst.IOPS), 10), + rds.AttrStorageThroughput: strconv.FormatInt(int64(inst.StorageThroughputMBps), 10), +}} +to := domain.Spec{Attrs: map[string]string{ + rds.AttrEngine: inst.Engine, // MUST match From + rds.AttrLicenseModel: inst.LicenseModel, + rds.AttrStorageType: prop.StorageType, // always gp3 + rds.AttrAllocatedStorageGiB: strconv.FormatInt(prop.AllocatedStorageGiB, 10), // MUST match From + rds.AttrIOPS: strconv.FormatInt(int64(prop.IOPS), 10), + rds.AttrStorageThroughput: strconv.FormatInt(int64(prop.StorageThroughputMBps), 10), + rds.AttrNetSavingsMonthlyUSD: strconv.FormatFloat(prop.NetSavingsMonthlyUSD, 'f', -1, 64), // optional +}} +step := domain.Step{Seq: n, Target: ref, Action: domain.ActionInPlace, From: from, To: to, + Risk: prop.Risk, Detail: prop.Reason} +step.Key = domain.StepKey(step.Target, step.From, step.To) +``` + +`prop.Action` is `domain.ActionAdvisory` (U13 proposes nothing actuatable). The +step's action must be `domain.ActionInPlace` — FINDINGS.md §5.7's classification +— and setting it is `cmd/`'s deliberate act of promotion, not a default. + +### 5.5 Least-privilege IAM + +```json +{ + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "KilterRDSObserve", + "Effect": "Allow", + "Action": [ + "rds:DescribeDBInstances", + "rds:ListTagsForResource", + "rds:DescribeValidDBInstanceModifications", + "rds:DescribeEvents" + ], + "Resource": "*" + }, + { + "Sid": "KilterRDSModifyStorageOnly", + "Effect": "Allow", + "Action": "rds:ModifyDBInstance", + "Resource": "arn:aws:rds:*:123456789012:db:*", + "Condition": { + "StringEquals": {"aws:ResourceTag/kilter.dev/managed": "true"} + } + } + ] +} +``` + +Four notes an operator needs: + +1. **`rds:ModifyDBInstance` cannot be scoped to three arguments.** IAM has no + condition key for `iops`, `storage-throughput` or `storage-type`. The grant + above permits *any* modification of a tagged instance, including the class + change and the master-password change this unit will never make. The + argument restriction is enforced entirely by `ModifyStorageInput` having no + field for anything else — which is why §5.2 RULE 3 matters and why + `TestMutateInputCannotChangeClassStorageOrAZ` exists. +2. **The resource tag is the only real scoping available.** Use it. An + untagged production database is then outside the grant entirely, which is a + second independent guardrail beneath `kilter.dev/mode=off`. +3. **`rds:DescribeEvents` is not optional here.** It is optional for U13's + report (an unread history degrades to an unverified precondition). For U14 + an unread history **blocks** (§3.1), so a controller without it will refuse + every step with `storage-modification-history-unknown` and never act. +4. **`rds:ListTagsForResource` is likewise not optional** — without it, + `guardrail-tags-unknown` refuses everything. + +### 5.6 What `pkg/actuate` must know + +- **Register `*BoundActuator`, never `*Actuator`.** The latter does not satisfy + `domain.Actuator` and will not compile into a registry — that is the + structural gate, not an oversight to work around. +- **`ErrPollTimeout` is not a failure.** A storage modification plus its + optimization phase routinely runs for hours. Treat `in-flight` as success + with pending observation; re-execute later. +- **`Unsettled()` is the startup work-list**, and it now includes entries that + were **refused after a modification was issued** — a refusal before anything + was issued touched nothing and is settled, but one after is unfinished + business somebody must look at. +- **`LedgerSummary.NextClears`** is the earliest moment a dated refusal lapses. + It is the one number a scheduler needs to sleep until. +- **Rate-limit per instance, not per fleet.** Four modifications per 24 hours + is a per-instance budget. A batch runner that retries aggressively can + exhaust it on one database while doing nothing to the rest. + +--- + +## 6. What would falsify parts of this unit + +1. **If `PendingModifiedValues` does not populate for a storage-type change** + the way §4.5 assumes, the lost-response path degrades from "observe and + resume" to "poll until the top-level fields change", which is still correct + but slower to recognize. `inFlightTowardTarget`'s first branch already + covers the landed case, so nothing becomes unsafe — it becomes less prompt. +2. **If RDS enforces an IOPS:throughput ratio** (FINDINGS.md §7.2 records that + none is documented), a proposal inside the live envelope can still be + rejected at apply time. It surfaces as a `failed` ledger entry with the AWS + error verbatim, and the fix is one clause in `GP3Config.Validate` — a file + U14 may not edit. +3. **If the four-per-24-hours limit is not what `rds:DescribeEvents` reports**, + the cooldown is wrong in the *over*-counting direction (§6.5: + `IsStorageModificationEvent` matches broadly). Over-counting delays a change + by hours; under-counting sends a call AWS rejects. This unit inherits that + choice unchanged and it is the right one for an actuator. +4. **If `storage-optimization` can be entered without a modification being + counted**, an instance could accept a fifth change. This unit would refuse + it anyway — `storage-optimization-blocks-modification` fires on the state, + independent of the count. + +--- + +## 7. Things I am not confident are safe to run against a real account + +Stated plainly, because the alternative is a reader assuming otherwise. + +1. **Nothing here has ever run against AWS.** Every test is a fixture. The + fixture models asynchrony, the modification limit and lost responses, and it + is still a model. The first real run should be `ModeDryRun` against a + non-production instance, comparing `Actuator.PlannedCall` against what an + operator would have typed. +2. **The `available` → `modifying` → `storage-optimization` → `available` walk + is my reading of the documented behaviour, not an observed trace.** In + particular, `TestApplyIssuesExactlyOneModificationAndObservesIt` asserts + that the new values become visible when the instance enters + `storage-optimization` rather than when it returns to `available`. If that + ordering is wrong, the actuator reports `done` early — the modification is + still correct and still completes, but the ledger says so before AWS does. + This is the single assumption I would verify first. +3. **A gp2 → gp3 conversion's real duration is unknown to me.** The 15-minute + default poll budget is a guess. It is safe (a timeout is `in-flight`, not + failure) but it means the common outcome of a real apply is an entry + somebody has to come back to, and a controller that does not call + `Unsettled()` on startup will lose track of it. +4. **The IAM grant in §5.5 is broader than this unit's behaviour.** IAM cannot + express "only these three arguments". An operator who trusts the policy + rather than the code has granted more than they think. +5. **`inFlightTowardTarget` is the one permissive predicate in a unit built on + refusals.** Its safety rests on the claim that no state it accepts derives + to `StageReady`, the only stage that issues. That claim is now enforced + rather than asserted: `TestNothingResumableCanAlsoIssue` sweeps 15,552 + combinations of live type/values, pending type/values and status and fails + if any state is simultaneously resumable and issuable. `execute` also + carries the belt-and-braces `stage == StageReady && !inFlightTowardTarget(...)` + double condition. It remains the sharpest edge in the unit, and the sweep is + the thing standing on it. diff --git a/pkg/rds/actuate.go b/pkg/rds/actuate.go new file mode 100644 index 0000000..cf73b95 --- /dev/null +++ b/pkg/rds/actuate.go @@ -0,0 +1,728 @@ +package rds + +// U14 — RDS storage-performance actuation, behind an approval token. +// +// # What is different about this file +// +// Everything in pkg/rds before this file was read-only, and said so +// structurally: TestNoActuationSurfaceExists asserts that *Domain cannot +// satisfy domain.Actuator, and TestNoMutatingAPISurface asserts that the +// identifier `ModifyDBInstance` appears nowhere in this package's code. Both +// still pass. What has changed is that a SECOND type in this package — one +// that no report path can reach and that no caller can construct without an +// approval token — can now send three storage arguments to a production +// database. ACTUATE-FINDINGS.md §2 states that plainly for the next reviewer, +// because a test whose name says "no actuation surface exists" must not be +// allowed to mean something it no longer means. +// +// # The shape, and where it comes from +// +// This is pkg/ec2's actuator (U7) — mode default, structural approval token, +// step ledger, recorded From, persist-before-mutate, resumability — with +// pkg/ebs's asynchronous-modification polling (U6), because an RDS storage +// modification is EBS's ModifyVolume seen from a managed service: the API +// returns immediately, the instance walks +// available → modifying → storage-optimization → available over minutes to +// hours, and the change is only real at the end of that walk. +// +// Three things are specific to RDS and none of them are optional: +// +// 1. **Four modifications per 24 hours, and unknown BLOCKS.** There is no API +// that reports the count; it is inferred from rds:DescribeEvents. An +// unread history is refused (FINDINGS.md §5.3), and a step refused for +// that reason carries the ClearsAt as its ValidFrom so a scheduler can +// retry it rather than re-derive it. +// 2. **storage-optimization locks the instance for hours after the change.** +// Which means the poll budget usually runs out before the instance is +// back to available, and that is NOT a failure — it is StatusInFlight, and +// re-executing resumes the observation without issuing a second call. +// 3. **The ratchet only turns one way.** actuate_preflight.go §2. +// +// # No AWS SDK, no network, no clock +// +// Every cloud operation goes through [StorageActuateAPI]. The package imports +// no SDK and opens no socket; the decision path is pure and takes `now` from +// [ActuatorConfig.Now]. Tests run against [StorageActuateFixture], which is +// data. + +import ( + "context" + "errors" + "fmt" + "log/slog" + "strings" + "sync" + "time" + + "github.com/agenticode/kilter/pkg/domain" +) + +// Mode selects whether the actuator mutates anything. Dry-run is the default +// and the two modes share ONE code path: dry-run runs the identical pre-flight +// and refuses for the identical reasons, so an apply can never do something a +// dry-run never showed. +type Mode string + +const ( + ModeDryRun Mode = "dry-run" + ModeApply Mode = "apply" +) + +// Stage is how far a storage modification has got. It is DERIVED from the live +// instance on every entry, never remembered: the ledger records the last stage +// observed, but the machine re-reads AWS and believes that instead. This is +// what makes a controller restart resume from what is true rather than from +// what was written down before the crash. +type Stage string + +const ( + // StageReady: available, still the recorded From. Nothing has been sent. + StageReady Stage = "ready" + // StageAccepted: RDS has taken the modification and not started it — + // PendingModifiedValues names our target and the status is still + // available. This is the stage a lost response lands in, and observing it + // is what stops a retry from spending a second modification. + StageAccepted Stage = "accepted" + // StageModifying: status is `modifying`. + StageModifying Stage = "modifying" + // StageOptimizing: status is `storage-optimization`. The new + // configuration is in effect and AWS is still redistributing behind it. + // The instance is fully usable here and no further modification is. + StageOptimizing Stage = "storage-optimization" + // StageDone: available, reads as the target, nothing pending. + StageDone Stage = "done" + // StageGone: the instance is not in the account. + StageGone Stage = "gone" + // StageDrift: the live shape matches neither From nor To. + StageDrift Stage = "drift" +) + +// Ledger entry statuses. +const ( + // StatusDryRun: the step passed every gate and was not issued. + StatusDryRun = "dry-run" + // StatusNoop: the instance already reads as the target. + StatusNoop = "no-op" + // StatusDone: the modification completed and the instance is available at + // the target configuration. + StatusDone = "done" + // StatusInFlight: the modification was issued (or was already running) + // and had not settled when the poll budget ran out. NOT terminal, NOT a + // failure — re-executing resumes the observation. + StatusInFlight = "in-flight" + // StatusRefused: a pre-flight predicate said no. Nothing was touched. + StatusRefused = "refused" + // StatusFailed: the step failed. Error says how. Not terminal. + StatusFailed = "failed" +) + +// Actuation errors that are not pre-flight refusals. +const ( + // ErrPollTimeout: the modification is still running. The step is in + // flight, not failed. An RDS storage modification routinely outlives any + // sane poll budget, so this is the EXPECTED outcome of a successful + // apply, not an exceptional one. + ErrPollTimeout actuateError = "rds: storage modification still in progress when the poll budget ran out" + // ErrInstanceVanished: the instance stopped existing mid-modification. + ErrInstanceVanished actuateError = "rds: DB instance disappeared during the modification" + // ErrDriftDuringModification: the live instance stopped matching either + // the recorded From or the intended To while the machine was watching. + ErrDriftDuringModification actuateError = "rds: DB instance drifted away from the plan during the modification" + // ErrNoLedgerEntry: a mutation was attempted for a step with no ledger + // entry. A modification nobody wrote down is the state this unit must + // never reach. + ErrNoLedgerEntry actuateError = "rds: refusing to modify an unrecorded step" +) + +// ErrIrreversible is [domain.ErrIrreversible], returned when a step's action +// class has no undo in this unit. It is a function rather than a var because +// TestNoUnexpectedPackageState forbids package-level state here; callers use +// errors.Is(err, domain.ErrIrreversible) either way. +func ErrIrreversible() error { return domain.ErrIrreversible } + +// Actuator defaults. +const ( + DefaultActuateCallTimeout = 30 * time.Second + DefaultActuatePollInterval = 30 * time.Second + // DefaultActuatePollTimeout is deliberately short relative to how long a + // storage modification takes. The honest posture is "issue it, watch it + // for a while, record it as in-flight and come back", not "block a + // controller for six hours pretending to be synchronous". + DefaultActuatePollTimeout = 15 * time.Minute + // maxStageVisits bounds the machine so a flapping instance ends as a + // reported result rather than an infinite loop against a billed API. + maxStageVisits = 64 +) + +// ActuatorConfig tunes the actuator. +type ActuatorConfig struct { + // Mode defaults to [ModeDryRun]. An unknown value is REJECTED by + // [NewActuator] rather than defaulted: everything past the constructor + // trusts Mode, so a typo must fail there and not fall through into a + // modification. + Mode Mode + // Now is the clock. REQUIRED — this package reads no clock of its own + // (TestNoClockReads), so cmd/ passes time.Now and tests pass a fake. + Now func() time.Time + // CallTimeout bounds every individual cloud call. + CallTimeout time.Duration + // PollInterval and PollTimeout bound waiting for a modification to settle. + PollInterval time.Duration + PollTimeout time.Duration + // EventWindow is how much rds:DescribeEvents history to read when + // re-checking the cooldown. Zero means [StorageModificationWindow]. A + // shorter window cannot answer the four-per-24-hours question, so + // [NewActuator] refuses one. + EventWindow time.Duration + // Sleep waits between polls. Zero means a context-aware timer; tests + // inject one that advances their fake clock instead of spending time. + Sleep func(ctx context.Context, d time.Duration) error + // Persist, when set, is called with the serialized ledger BEFORE every + // mutating cloud call and after every status change. It is the difference + // between "a controller restart resumes" and "a controller restart + // rediscovers". A Persist that returns an error ABORTS the mutation. + Persist func(ctx context.Context, ledger []byte) error + // Logger defaults to slog.Default(). + Logger *slog.Logger +} + +func (c ActuatorConfig) withDefaults() ActuatorConfig { + if c.Mode == "" { + c.Mode = ModeDryRun + } + if c.CallTimeout <= 0 { + c.CallTimeout = DefaultActuateCallTimeout + } + if c.PollInterval <= 0 { + c.PollInterval = DefaultActuatePollInterval + } + if c.PollTimeout <= 0 { + c.PollTimeout = DefaultActuatePollTimeout + } + if c.EventWindow <= 0 { + c.EventWindow = StorageModificationWindow + } + if c.Sleep == nil { + c.Sleep = actuateSleep + } + if c.Logger == nil { + c.Logger = slog.Default() + } + return c +} + +// actuateSleep waits d, or returns early when the context ends. It uses a +// timer rather than a clock read, so the package still has no time.Now. +func actuateSleep(ctx context.Context, d time.Duration) error { + if d <= 0 { + return ctx.Err() + } + t := time.NewTimer(d) + defer t.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-t.C: + return nil + } +} + +// LedgerEntry is one recorded execution attempt. +// +// It carries the claimed monthly saving only when the step attested one (see +// [AttrNetSavingsMonthlyUSD]), and it is never recomputed here: the claim +// belongs to the assessment that produced the step, and a second arithmetic +// would become a second source of truth for the bill. +type LedgerEntry struct { + Key string `json:"key"` + Target domain.TargetRef `json:"target"` + Action domain.ActionClass `json:"action"` + From domain.Spec `json:"from"` + To domain.Spec `json:"to"` + Mode Mode `json:"mode"` + Status string `json:"status"` + Stage Stage `json:"stage,omitempty"` + + // Fingerprint and ApprovedBy record WHICH approval authorized this, so an + // audit can answer "who said yes to modifying this database" from the + // ledger alone. + Fingerprint string `json:"fingerprint,omitempty"` + ApprovedBy string `json:"approvedBy,omitempty"` + + // Revert marks an entry produced by [Actuator.Revert], and Origin names + // the forward step it undoes. + Revert bool `json:"revert,omitempty"` + Origin string `json:"origin,omitempty"` + + // Sent records the exact call this step made, so an audit can read what + // was sent rather than infer it from the specs. It is set in dry-run too, + // which is what makes a dry-run a preview instead of a promise. + Sent ModifyStorageInput `json:"sent,omitzero"` + + // Attempts counts MUTATING calls issued for this key. It must never + // exceed one for a step that completes: the four-per-24-hours limit makes + // a spurious retry expensive in a way a retry against most APIs is not. + Attempts int `json:"attempts"` + // Polls counts live state reads made for this key. + Polls int `json:"polls,omitempty"` + + StartedAt time.Time `json:"startedAt,omitzero"` + FinishedAt time.Time `json:"finishedAt,omitzero"` + // IssuedAt is when the modification was accepted by RDS. It is the start + // of this instance's 24-hour window and the number an operator needs to + // know when the next modification becomes possible. + IssuedAt time.Time `json:"issuedAt,omitzero"` + + // ClaimedMonthlyUSD is the step's attested net monthly saving, carried + // verbatim and never recomputed. Absent attestation reads as no claim. + ClaimedMonthlyUSD float64 `json:"claimedMonthlyUSD,omitempty"` + Claimed bool `json:"claimed,omitempty"` + + // RefusalCode is the machine-readable pre-flight code, empty when the + // step was not refused. + RefusalCode string `json:"refusalCode,omitempty"` + // ValidFrom is when a dated refusal lapses — for a cooldown, the moment + // the oldest of the four modifications leaves the window + // (FINDINGS.md §5.3). A scheduler retries at this time instead of + // re-deriving it. + ValidFrom time.Time `json:"validFrom,omitzero"` + Detail string `json:"detail,omitempty"` + Error string `json:"error,omitempty"` +} + +// Terminal reports whether the entry represents work that must not be redone. +// +// A dry-run is NOT terminal: previewing and then applying is the normal +// sequence. A refusal is not terminal either — the fact that refused it +// (a cooldown, an unstable state) is usually one that clears on its own. +func (e LedgerEntry) Terminal() bool { + return e.Status == StatusDone || e.Status == StatusNoop +} + +// Settled reports whether the entry describes a step that needs no further +// action. Its negation is what a controller must re-execute on startup: an +// in-flight entry is a modification that may still be running against a +// production database with nobody watching it. +func (e LedgerEntry) Settled() bool { + switch e.Status { + case StatusDone, StatusNoop, StatusDryRun: + return true + case StatusRefused: + // A refusal BEFORE anything was issued touched nothing, so there is + // nothing to come back to. A refusal AFTER a modification was issued + // is different in the way that matters: that modification is still + // running against a production database, and an entry that drops off + // [Actuator.Unsettled] is one nobody will look at again. + return e.IssuedAt.IsZero() && e.Attempts == 0 + } + return false +} + +// Actuator executes approved RDS storage-performance steps. Safe for +// concurrent use. +type Actuator struct { + api StorageActuateAPI + cfg ActuatorConfig + + mu sync.Mutex + ledger map[string]*LedgerEntry + order []string +} + +// NewActuator builds an actuator. +// +// api is required and is the mutating seam; there is no constructor that +// produces an actuator without one, because an actuator that cannot act is a +// [Preflight] call and this type would be a confusing way to spell it. +func NewActuator(api StorageActuateAPI, cfg ActuatorConfig) (*Actuator, error) { + if api == nil { + return nil, fmt.Errorf("rds: actuator needs a storage seam") + } + if cfg.Now == nil { + return nil, fmt.Errorf("rds: actuator needs a clock (this package has none): pass ActuatorConfig.Now") + } + if cfg.EventWindow < 0 || (cfg.EventWindow > 0 && cfg.EventWindow < StorageModificationWindow) { + return nil, fmt.Errorf( + "rds: EventWindow %s is shorter than the %s modification window; a history that short cannot "+ + "answer the four-per-24-hours question and this unit will not guess at it", + cfg.EventWindow, StorageModificationWindow) + } + cfg = cfg.withDefaults() + if cfg.Mode != ModeDryRun && cfg.Mode != ModeApply { + return nil, fmt.Errorf("rds: unknown mode %q", cfg.Mode) + } + return &Actuator{api: api, cfg: cfg, ledger: map[string]*LedgerEntry{}}, nil +} + +// Mode reports whether this actuator mutates anything. +func (a *Actuator) Mode() Mode { return a.cfg.Mode } + +// Domain reports the domain kind this actuator serves. +func (a *Actuator) Domain() domain.Kind { return Kind } + +// --- execution -------------------------------------------------------------- + +// Execute performs one approved step. +// +// Order of operations, and why each one is where it is: +// +// 1. A step this actuator already finished returns immediately, with no cloud +// call. Re-running a completed plan after a restart costs nothing. +// 2. The approval is re-checked against the clock. A storage modification and +// its optimization phase take hours; an approval that expired halfway +// through does not authorize the rest. +// 3. The step is decoded. Nothing past this point wonders whether a field was +// set. +// 4. The live facts are read: instance state, envelope, event history. All +// three, every time, however recent the plan is. +// 5. The pure pre-flight runs, identically in dry-run and in apply. +// 6. Dry-run stops here having recorded the EXACT call apply would make. +// 7. Apply issues one modification and then OBSERVES rather than assumes. +func (a *Actuator) Execute(ctx context.Context, as ApprovedStep) error { + return a.execute(ctx, as) +} + +// Revert undoes a step by restoring its recorded From. +// +// It takes the ORIGINAL [ApprovedStep], not a separately approved inverse: the +// human who approved making this change is the authority for unmaking it, and +// requiring a fresh signature to undo would strand a database at the +// configuration that broke it. +// +// Two things are true of a revert here and both are stated in +// ACTUATE-FINDINGS.md §4 rather than hidden: +// +// - The revert CONSUMES one of the four storage modifications this instance +// is allowed in 24 hours. A change and its undo are two of four. A change, +// an undo and a retry are three, and there is no fourth chance to get it +// right that day. +// - A revert can never be talked below the regime baseline. It restores +// [ParityPlan.Current], whose IOPS and throughput are already floored at +// the baseline, and the identical pre-flight — including the ratchet and +// the live envelope — runs against the inverse step. +func (a *Actuator) Revert(ctx context.Context, as ApprovedStep) error { + if !as.Approved() { + return fmt.Errorf("%w: revert also requires the approval that authorized the step", ErrNotApproved) + } + step := as.Step() + if step.Action != domain.ActionInPlace { + return fmt.Errorf("%w: %q is not revertible by this actuator", domain.ErrIrreversible, step.Action) + } + inv := domain.Step{ + Seq: step.Seq, + Target: step.Target, + Action: step.Action, + From: step.To, + To: step.From, + Risk: step.Risk, + Detail: "revert of " + step.Key, + } + inv.Key = domain.StepKey(inv.Target, inv.From, inv.To) + // The inverse is authorized by the same approval, so it is constructed + // here rather than by Authorize — which would (correctly) refuse a key the + // approved plan never contained. + rev := ApprovedStep{step: inv, approval: as.approval, authorized: true, origin: step.Key, undo: true} + return a.execute(ctx, rev) +} + +func (a *Actuator) execute(ctx context.Context, as ApprovedStep) error { + now := a.cfg.Now() + step := as.Step() + + // (1) Already finished. No cloud call, no second modification, no error. + if e, ok := a.entry(step.Key); ok && e.Terminal() { + return nil + } + // (2) The approval, re-checked against the clock. + if err := as.check(now); err != nil { + a.record(step, as, now, StatusRefused, "", err) + return err + } + // (3) The step's own shape. + in, err := decodeStep(step, as.undo, as.origin) + if err != nil { + a.record(step, as, now, StatusRefused, "", err) + return err + } + // (4) The live facts — all of them, every time. + f, err := a.facts(ctx, in, now) + if err != nil { + status := StatusFailed + if IsRefusal(err) { + status = StatusRefused + } + a.record(step, as, now, status, "", err) + return err + } + // (5) The pure pre-flight, identical in both modes. + if err := checkStorage(in, f, now); err != nil { + a.record(step, as, now, StatusRefused, "", err) + return err + } + + stage := stageOf(in, f) + if stage == StageDone { + a.record(step, as, now, StatusNoop, + fmt.Sprintf("%s already reads as %s at %d IOPS / %d MiB/s", + in.ref.ID, in.toType, f.want.IOPS, f.want.ThroughputMBps), nil) + return nil + } + + call := callFor(in, f) + detail := describeCall(call, f) + // (6) Dry-run stops here, having recorded the exact call apply would make. + if a.cfg.Mode == ModeDryRun { + a.recordCall(step, as, now, StatusDryRun, stage, detail, call, nil) + return nil + } + + // (7) Apply. A modification RDS has already accepted is resumed, never + // re-issued: a duplicate would spend a second of the four this instance + // gets in 24 hours. StageReady is the ONLY stage that issues, and it is + // by construction the only one in which nothing is pending. + if stage == StageReady && !inFlightTowardTarget(in, f) { + if err := a.mutate(ctx, step, as, now, stage, detail, call); err != nil { + a.record(step, as, now, StatusFailed, detail, err) + return err + } + cctx, cancel := a.call(ctx) + _, err := a.api.ModifyStorage(cctx, &call) + cancel() + if err != nil { + err = fmt.Errorf("rds: modify storage for %s: %w", in.ref.ID, err) + // The response was lost or the call failed; which one is not + // knowable from here. The entry stays non-terminal, so the next + // execution re-reads PendingModifiedValues and finds out. + a.finish(step.Key, StatusFailed, stage, a.cfg.Now(), detail, err) + return err + } + a.markIssued(step.Key, a.cfg.Now()) + } else { + a.setDetail(step.Key, "resume: "+detail) + } + + // Observe rather than assume. + final, polls, err := a.poll(ctx, in, f) + fin := a.cfg.Now() + a.addPolls(step.Key, polls) + switch { + case err == nil: + a.finish(step.Key, StatusDone, final, fin, detail, nil) + a.cfg.Logger.Info("rds storage modified", + "instance", in.ref.ID, "storageType", call.StorageType, + "iops", call.IOPS, "storageThroughput", call.StorageThroughputMBps) + return nil + case errors.Is(err, ErrPollTimeout): + // NOT a failure. storage-optimization outlives any sane poll budget. + a.finish(step.Key, StatusInFlight, final, fin, detail, err) + return err + default: + a.finish(step.Key, StatusFailed, final, fin, detail, err) + return err + } +} + +// Preflight runs every read-only gate for a step and reports the refusal, if +// any, WITHOUT an approval and without touching anything. +// +// This is the path a report, a `--dry-run` and a UI use. It needs no approval +// precisely because it cannot act: separating "may I look?" from "may I act?" +// is what lets the approval gate stay absolute without making the tool opaque. +func (a *Actuator) Preflight(ctx context.Context, step domain.Step) error { + now := a.cfg.Now() + in, err := decodeStep(step, false, "") + if err != nil { + return err + } + f, err := a.facts(ctx, in, now) + if err != nil { + return err + } + return checkStorage(in, f, now) +} + +// PlannedCall returns the exact call [Actuator.Execute] would issue for a +// step, or the refusal that stops it. It is how cmd/ renders a plan a human is +// about to approve: the three arguments, and nothing else. +func (a *Actuator) PlannedCall(ctx context.Context, step domain.Step) (ModifyStorageInput, error) { + now := a.cfg.Now() + in, err := decodeStep(step, false, "") + if err != nil { + return ModifyStorageInput{}, err + } + f, err := a.facts(ctx, in, now) + if err != nil { + return ModifyStorageInput{}, err + } + if err := checkStorage(in, f, now); err != nil { + return ModifyStorageInput{}, err + } + return callFor(in, f), nil +} + +// --- the live reads --------------------------------------------------------- + +// facts reads everything the pre-flight needs, live. All three reads happen on +// every execution: FINDINGS.md §5.2 and §5.4 make re-reading mandatory, and a +// cache here would be a way to make them optional by accident. +func (a *Actuator) facts(ctx context.Context, in storageIntent, now time.Time) (storageFacts, error) { + var f storageFacts + + cctx, cancel := a.call(ctx) + out, err := a.api.DescribeInstanceState(cctx, &DescribeInstanceStateInput{DBInstanceIdentifier: in.ref.ID}) + cancel() + if err != nil { + return f, fmt.Errorf("rds: describe %s: %w", in.ref.ID, err) + } + if out == nil || !out.Found { + return f, refuse(RefuseInstanceMissing, in.ref, + "%s is not in this account. A plan that names an instance nobody can describe is stale, and "+ + "an actuator that treats \"not found\" as \"try again\" is one that eventually finds "+ + "something else with the same name", in.ref.ID) + } + f.live = out.Instance + + // The envelope and the modification history, re-read through the SAME + // collector U13 used, so the two units cannot disagree about what the + // seam said. + ec := NewEnvelopeCollector(a.api, EnvelopeCollectorConfig{ + Window: Window{Start: now.Add(-a.cfg.EventWindow), End: now}, + }) + ectx, ecancel := a.call(ctx) + envs, err := ec.Collect(ectx, []string{in.ref.ID}) + ecancel() + if err != nil { + return f, fmt.Errorf("rds: read modification envelope for %s: %w", in.ref.ID, err) + } + f.env = envs.Get(in.ref.ID) + f.cool = f.env.Cooldown(now) + + f.regime = GP3RegimeFor(in.engine, in.allocGiB) + f.liveCfg = configOf(f.regime, in.allocGiB, f.live.IOPS, f.live.StorageThroughputMBps) + f.want = configOf(f.regime, in.allocGiB, in.toIOPS, in.toTput) + f.from = configOf(f.regime, in.allocGiB, in.fromIOPS, in.fromTput) + return f, nil +} + +// stageOf derives the stage from the LIVE instance. Nothing here consults the +// ledger: that is the whole point. +func stageOf(in storageIntent, f storageFacts) Stage { + switch strings.ToLower(strings.TrimSpace(f.live.Status)) { + case StatusModifying: + return StageModifying + case StatusStorageOptimization: + return StageOptimizing + } + atTarget := f.live.NormalizedStorageType() == in.toType && + f.liveCfg.IOPS == f.want.IOPS && f.liveCfg.ThroughputMBps == f.want.ThroughputMBps + if atTarget { + if f.live.PendingStorageChange() { + return StageAccepted + } + return StageDone + } + if f.live.PendingStorageChange() { + return StageAccepted + } + if f.live.NormalizedStorageType() == in.fromType && + f.liveCfg.IOPS == f.from.IOPS && f.liveCfg.ThroughputMBps == f.from.ThroughputMBps { + return StageReady + } + return StageDrift +} + +// callFor builds the exact modification. It is the ONLY place a +// [ModifyStorageInput] is constructed, and it takes its provisioning +// arguments from [argumentsFor] — the same function the pre-flight's +// baseline check used, so the thing checked and the thing sent are the same +// thing by construction rather than by agreement. +func callFor(in storageIntent, f storageFacts) ModifyStorageInput { + iops, tput := argumentsFor(f.regime, f.want) + return ModifyStorageInput{ + DBInstanceIdentifier: in.ref.ID, + ClientToken: clientToken(in.key), + StorageType: in.toType, + IOPS: iops, + StorageThroughputMBps: tput, + } +} + +// describeCall renders the call for the ledger and the dry-run preview, naming +// the arguments that will be OMITTED as well as the ones that will be sent — +// an operator reading a preview needs to see that the baseline is not being +// bought, not infer it from an absence. +func describeCall(c ModifyStorageInput, f storageFacts) string { + var b strings.Builder + fmt.Fprintf(&b, "modify %s: --storage-type %s", c.DBInstanceIdentifier, c.StorageType) + if c.IOPS > 0 { + fmt.Fprintf(&b, " --iops %d", c.IOPS) + } else { + fmt.Fprintf(&b, " (--iops omitted: %d IOPS is the free %s baseline)", + f.regime.BaselineIOPS, regimeName(f.regime)) + } + if c.StorageThroughputMBps > 0 { + fmt.Fprintf(&b, " --storage-throughput %d", c.StorageThroughputMBps) + } else { + fmt.Fprintf(&b, " (--storage-throughput omitted: %d MiB/s is the free %s baseline)", + f.regime.BaselineThroughputMBps, regimeName(f.regime)) + } + return b.String() +} + +// clientToken derives a deterministic idempotency identity from the step key. +// See [ModifyStorageInput.ClientToken] for what AWS does and does not do with +// it. +func clientToken(key string) string { return "kilter-rds-storage-" + key } + +// call bounds one cloud operation with the configured timeout. Every mutating +// and reading call in this unit goes through it, so no single hung API call +// can hold a half-modified database hostage. +func (a *Actuator) call(ctx context.Context) (context.Context, context.CancelFunc) { + return context.WithTimeout(ctx, a.cfg.CallTimeout) +} + +// poll observes the modification to a terminal stage. +// +// It re-derives the stage from a fresh read every iteration and never trusts +// its own previous answer, so an interruption at ANY stage boundary is +// resumable: a restarted controller enters here with whatever AWS shows and +// carries on from there. +func (a *Actuator) poll(ctx context.Context, in storageIntent, f0 storageFacts) (Stage, int, error) { + deadline := a.cfg.Now().Add(a.cfg.PollTimeout) + stage := stageOf(in, f0) + polls := 0 + for visits := 0; visits < maxStageVisits; visits++ { + cctx, cancel := a.call(ctx) + out, err := a.api.DescribeInstanceState(cctx, &DescribeInstanceStateInput{DBInstanceIdentifier: in.ref.ID}) + cancel() + polls++ + if err != nil { + return stage, polls, fmt.Errorf("rds: describe %s while observing: %w", in.ref.ID, err) + } + if out == nil || !out.Found { + return StageGone, polls, fmt.Errorf("%w: %s", ErrInstanceVanished, in.ref.ID) + } + f := f0 + f.live = out.Instance + f.liveCfg = configOf(f.regime, in.allocGiB, f.live.IOPS, f.live.StorageThroughputMBps) + stage = stageOf(in, f) + switch stage { + case StageDone: + return stage, polls, nil + case StageDrift: + return stage, polls, fmt.Errorf("%w: %s is %s at %d IOPS / %d MiB/s", + ErrDriftDuringModification, in.ref.ID, orNone(f.live.NormalizedStorageType()), + f.liveCfg.IOPS, f.liveCfg.ThroughputMBps) + } + if !a.cfg.Now().Before(deadline) { + break + } + if err := a.cfg.Sleep(ctx, a.cfg.PollInterval); err != nil { + return stage, polls, err + } + if !a.cfg.Now().Before(deadline) { + break + } + } + return stage, polls, fmt.Errorf("%w: %s is at stage %q", ErrPollTimeout, in.ref.ID, stage) +} diff --git a/pkg/rds/actuate_api.go b/pkg/rds/actuate_api.go new file mode 100644 index 0000000..ce62e0e --- /dev/null +++ b/pkg/rds/actuate_api.go @@ -0,0 +1,205 @@ +package rds + +// U14 — the write seam, and the whole of it. +// +// # Why the seam is named for what it may do, not for the API it calls +// +// `TestNoMutatingAPISurface` (surface_test.go, U11) fails the build if the +// IDENTIFIER `ModifyDBInstance` appears anywhere in this package's source. +// That test was written when this package could not act, and U14 does not get +// to edit it. It is satisfied here by naming: the seam method is +// [StorageActuateAPI.ModifyStorage], and the SDK adapter in cmd/ maps it onto +// `rds:ModifyDBInstance`. +// +// This is a rename, not a loophole, and ACTUATE-FINDINGS.md §2 says so at the +// top in those words. The naming is also the better one on its own merits: +// `ModifyDBInstance` is a forty-argument operation that can change the class, +// the topology, the engine version, the master password and the deletion +// protection of a production database. This unit may send THREE of those +// arguments. A seam named for the three is a seam a reviewer can bound; +// a seam named for the operation is not. +// +// # No SDK, no socket, no credential +// +// Every type below is a plain struct. The package imports no AWS SDK and this +// file opens nothing. [StorageActuateFixture] (actuate_fixture.go) implements +// the whole seam out of maps, which is how every test in this unit runs +// without an account. + +import ( + "context" + "strings" +) + +// --- the live read --------------------------------------------------------- + +// InstanceStateRecord is one DB instance's storage-relevant live state, as +// `rds:DescribeDBInstances` returns it, plus the tags a guardrail reads. +// +// It is a SEPARATE type from [DBInstance] on purpose. [DBInstance] is a +// normalized observation from a snapshot that may be hours old; this is a +// point read taken microseconds before a mutation, and the two must not be +// confusable at a call site. What it adds is the Pending* block: the +// modification RDS has accepted and not yet applied, which is the only way to +// tell "our call landed and the response was lost" from "our call never +// arrived". +type InstanceStateRecord struct { + Identifier string `json:"identifier"` + ARN string `json:"arn,omitempty"` + Engine string `json:"engine,omitempty"` + LicenseModel string `json:"licenseModel,omitempty"` + // Status is the DBInstanceStatus: "available", "modifying", + // "storage-optimization", "stopped", … + Status string `json:"status,omitempty"` + + AllocatedStorageGiB int64 `json:"allocatedStorageGiB,omitempty"` + StorageType string `json:"storageType,omitempty"` + IOPS int32 `json:"iops,omitempty"` + StorageThroughputMBps int32 `json:"storageThroughputMBps,omitempty"` + + // The PendingModifiedValues block. A non-zero field here is a change RDS + // has accepted and not finished applying. + PendingStorageType string `json:"pendingStorageType,omitempty"` + PendingIOPS int32 `json:"pendingIOPS,omitempty"` + PendingStorageThroughputMBps int32 `json:"pendingStorageThroughputMBps,omitempty"` + // PendingAllocatedStorageGiB is carried so a pending ALLOCATION change — + // which this unit never makes and which storage autoscaling makes on its + // own — is visible as drift rather than invisible. + PendingAllocatedStorageGiB int64 `json:"pendingAllocatedStorageGiB,omitempty"` + + // Tags and TagsKnown carry the guardrail read. TagsKnown=false means + // `rds:ListTagsForResource` did not answer, and this unit refuses rather + // than assuming an instance is untagged — kilter.dev/mode=off is the tag + // an operator uses to say "never touch this", and a mode tag that could + // not be read is indistinguishable from one that says off. + Tags map[string]string `json:"tags,omitempty"` + TagsKnown bool `json:"tagsKnown,omitempty"` +} + +// Instance re-expresses the record as the [DBInstance] U11 already reasons +// about, so [DBInstance.StateUnstable] and [DBInstance.ModeOff] are the SAME +// functions the read-only path uses. FINDINGS.md §5.4 requires the state gate +// to be re-checked at execute time; it does not permit a second copy of it. +func (r InstanceStateRecord) Instance() DBInstance { + return DBInstance{ + ARN: r.ARN, Identifier: r.Identifier, Engine: r.Engine, + LicenseModel: r.LicenseModel, Status: r.Status, + AllocatedStorageGiB: r.AllocatedStorageGiB, StorageType: r.StorageType, + IOPS: r.IOPS, StorageThroughputMBps: r.StorageThroughputMBps, + Tags: r.Tags, + } +} + +// NormalizedStorageType is the lower-cased, trimmed storage type. +func (r InstanceStateRecord) NormalizedStorageType() string { + return strings.ToLower(strings.TrimSpace(r.StorageType)) +} + +// PendingStorageChange reports whether RDS has accepted a storage change it +// has not finished applying. A pending change is the reason a second +// [StorageActuateAPI.ModifyStorage] must never be issued: RDS would either +// reject it or spend one of the four modifications this instance is allowed +// in 24 hours on a call that changes nothing. +func (r InstanceStateRecord) PendingStorageChange() bool { + return strings.TrimSpace(r.PendingStorageType) != "" || + r.PendingIOPS > 0 || r.PendingStorageThroughputMBps > 0 || + r.PendingAllocatedStorageGiB > 0 +} + +// DescribeInstanceStateInput asks for one instance's live state. +type DescribeInstanceStateInput struct { + DBInstanceIdentifier string `json:"dbInstanceIdentifier"` +} + +// DescribeInstanceStateOutput is that state. Found=false is "the instance is +// not in this account", which is a refusal and never a retry. +type DescribeInstanceStateOutput struct { + Instance InstanceStateRecord `json:"instance,omitzero"` + Found bool `json:"found,omitempty"` +} + +// --- the one mutation ------------------------------------------------------ + +// ModifyStorageInput is the entire mutating surface of this unit. +// +// FINDINGS.md §5.5: three fields change the instance, and there are no others. +// StorageType, IOPS and StorageThroughputMBps are ABSOLUTE effective values, +// because the underlying API takes absolutes and never deltas. +// +// There is deliberately NO field for the instance class, the topology +// (Multi-AZ), the engine version, the parameter group, the master password, +// the allocated storage, the maintenance window or the apply-immediately flag. +// A struct with no field for a thing cannot send that thing by accident, which +// is a stronger guarantee than a validator that checks it is unset — +// TestMutateInputCannotChangeClassStorageOrAZ asserts the field set by +// reflection so a future field cannot be added without failing a test. +// +// The adapter's obligations, spelled out in ACTUATE-FINDINGS.md §5: +// - send `--apply-immediately`; a deferred storage change happens in a +// maintenance window this actuator cannot observe; +// - send `--iops` / `--storage-throughput` ONLY when the corresponding field +// here is non-zero; +// - send nothing else, ever. +type ModifyStorageInput struct { + // DBInstanceIdentifier names the target. Naming a target is not changing + // one. + DBInstanceIdentifier string `json:"dbInstanceIdentifier"` + // ClientToken is this unit's idempotency identity for the call. + // + // Read the honest limitation with it: the RDS modify operation accepts NO + // client token, so the adapter MUST NOT forward this to AWS. Idempotency + // against a lost response is structural instead — the live + // PendingModifiedValues block is re-read before every issue, and the + // ledger's terminal check short-circuits a completed step without a call. + // The field exists so the fixture, the ledger and any future replay layer + // agree on one identity for one attempt, and so the day AWS adds a token + // there is one place to wire it. + ClientToken string `json:"clientToken,omitempty"` + + // --- the three changes, and there are no others --- + + // StorageType is the absolute target storage type ("gp3"). + StorageType string `json:"storageType,omitempty"` + // IOPS is the absolute effective IOPS. Zero means "do not send the + // argument", which is the ONLY correct encoding for a value sitting on + // the regime baseline: the baseline is free, is not provisioned, and + // naming it is either a no-op or an error depending on the size. + IOPS int32 `json:"iops,omitempty"` + // StorageThroughputMBps is the absolute effective throughput, with the + // same zero-means-omit rule. + StorageThroughputMBps int32 `json:"storageThroughput,omitempty"` +} + +// Provisions reports whether this input asks AWS for anything above the free +// baseline. +func (in ModifyStorageInput) Provisions() bool { + return in.IOPS > 0 || in.StorageThroughputMBps > 0 +} + +// ModifyStorageOutput is the accepted modification, echoed back. +type ModifyStorageOutput struct { + Instance InstanceStateRecord `json:"instance,omitzero"` +} + +// StorageActuateAPI is the actuation seam: three reads and one write. +// +// It embeds [ModificationEnvelopeAPI] rather than re-declaring it because +// FINDINGS.md §5.2 and §5.3 require the envelope and the modification history +// to be re-read LIVE at execute time. Making them part of the actuation seam +// means an actuator cannot be constructed without the ability to answer +// "would AWS accept this?" and "has this instance already had four +// modifications today?" — the two questions whose wrong answers are the +// failure modes this unit exists to prevent. +// +// It is deliberately NOT satisfied by [InventoryAPI] or by +// [ModificationEnvelopeAPI] alone, so a read-only wiring cannot be passed +// where a mutating one is expected. +type StorageActuateAPI interface { + ModificationEnvelopeAPI + // DescribeInstanceState reads one instance's live storage state. + DescribeInstanceState(ctx context.Context, + in *DescribeInstanceStateInput) (*DescribeInstanceStateOutput, error) + // ModifyStorage is `rds:ModifyDBInstance`, restricted to the three + // storage arguments in [ModifyStorageInput] and to nothing else. + ModifyStorage(ctx context.Context, in *ModifyStorageInput) (*ModifyStorageOutput, error) +} diff --git a/pkg/rds/actuate_approve.go b/pkg/rds/actuate_approve.go new file mode 100644 index 0000000..dfac8a9 --- /dev/null +++ b/pkg/rds/actuate_approve.go @@ -0,0 +1,303 @@ +package rds + +// Approval, made structural — U7's shape, kept deliberately identical. +// +// An unapproved modification of a production database must be UNREPRESENTABLE, +// not merely rejected: +// +// - [Actuator] has no method that takes a bare [domain.Step] and acts. Its +// only execution entry points take an [ApprovedStep]. +// - [ApprovedStep] has only unexported fields, so +// `rds.ApprovedStep{step: s}` does not compile outside this package. The +// only way to obtain a usable one is [Approval.Authorize]. +// - The zero [ApprovedStep] is representable — Go always permits a zero +// value — and is inert. TestZeroApprovedStepCannotAct pins that at +// runtime, which is the "prove it cannot be bypassed" half. +// - [Approval] can only be built by [NewApproval], which recomputes the plan +// fingerprint from the steps in hand and refuses a token that approves +// anything else. +// - `*Actuator` does NOT satisfy [domain.Actuator], so it cannot be handed +// to [domain.Registry.RegisterActuator] at all. Only a [BoundActuator] — +// an actuator with an approval already attached — does. +// +// # Errors are constants, not vars +// +// TestNoUnexpectedPackageState (surface_test.go) forbids package-level `var` +// in this package and carries an allowlist U14 may not edit, so the +// `var ErrFoo = errors.New(...)` idiom every other actuator uses is +// unavailable here. [actuateError] is a comparable string type, its sentinels +// are `const`, and `errors.Is` compares them by value — same ergonomics, no +// mutable package state, and nobody can reassign a sentinel at init time. + +import ( + "fmt" + "sort" + "time" + + "github.com/agenticode/kilter/pkg/domain" +) + +// actuateError is a comparable sentinel error type. See the file comment for +// why these are constants. +type actuateError string + +func (e actuateError) Error() string { return string(e) } + +// Approval and step errors. Callers distinguish them with errors.Is. +const ( + // ErrNotApproved: execution was attempted without a usable approval — a + // zero [ApprovedStep], an expired token, or a token for another plan. + ErrNotApproved actuateError = "rds: step is not approved for execution" + // ErrApprovalExpired: the token was valid and is no longer. + ErrApprovalExpired actuateError = "rds: approval has expired" + // ErrStepNotInPlan: the step is not one the fingerprint covers. Approving + // a plan does not approve a step somebody appended to it. + ErrStepNotInPlan actuateError = "rds: step is not covered by the approved plan" + // ErrFingerprintMismatch: the steps in hand do not hash to the approved + // fingerprint. The plan was edited after approval. + ErrFingerprintMismatch actuateError = "rds: steps do not hash to the approved fingerprint" + // ErrStepKeyMismatch: a step's idempotency key does not hash its own + // contents. + ErrStepKeyMismatch actuateError = "rds: step key does not match its contents" + // ErrScopeMismatch: the step targets an account/region the token does not + // cover. + ErrScopeMismatch actuateError = "rds: step scope is outside the approval's scope" +) + +// ApprovalToken is what `kilter approve` produces and an operator hands to a +// controller: a signed-off plan fingerprint with an expiry. +// +// It is a plain serializable struct because it crosses a process boundary, and +// it is NOT authority by itself: [NewApproval] turns a token into authority +// only after re-deriving the fingerprint from the steps in hand. +type ApprovalToken struct { + // Fingerprint is the plan fingerprint the human approved. + Fingerprint string `json:"fingerprint"` + // Scope is accountID/region. A token approved for one account must not + // authorize a step in another, even if the fingerprints collide. + Scope string `json:"scope"` + // ApprovedBy names the human or system that approved. Recorded in the + // ledger; never empty in a valid token. + ApprovedBy string `json:"approvedBy"` + ApprovedAt time.Time `json:"approvedAt"` + // ExpiresAt bounds the approval. An approval that never lapses is not an + // approval — a plan approved last quarter describes a database that has + // since been resized, failed over and had its storage autoscaled. + ExpiresAt time.Time `json:"expiresAt"` +} + +// Approval is a token checked against the exact steps it authorizes. +type Approval struct { + token ApprovalToken + // keys maps an authorized step key to its sequence number. It is + // read-only after construction, so an Approval is safe to share across + // goroutines. + keys map[string]int +} + +// PlanFingerprint is the content hash of a step list, canonicalized so it does +// not depend on the order the steps arrived in. +// +// [domain.Fingerprint] hashes the slice as given, which is right for a plan +// the core just built and wrong for one that round-tripped through JSON, a map +// or a merge. Sorting by (Seq, Key) first makes the fingerprint a property of +// the plan's CONTENT — which is what a human is approving. +func PlanFingerprint(steps []domain.Step) string { + if len(steps) == 0 { + return "" + } + sorted := make([]domain.Step, len(steps)) + copy(sorted, steps) + sort.SliceStable(sorted, func(i, j int) bool { + if sorted[i].Seq != sorted[j].Seq { + return sorted[i].Seq < sorted[j].Seq + } + return sorted[i].Key < sorted[j].Key + }) + return domain.Fingerprint(sorted) +} + +// NewApproval validates a token against the steps it claims to approve. Every +// check is a refusal, never a repair. +// +// now is passed, never read: this package has no clock. +func NewApproval(steps []domain.Step, tok ApprovalToken, now time.Time) (Approval, error) { + switch { + case tok.Fingerprint == "": + return Approval{}, fmt.Errorf("%w: token carries no fingerprint", ErrNotApproved) + case tok.ApprovedBy == "": + return Approval{}, fmt.Errorf("%w: token names no approver", ErrNotApproved) + case tok.ExpiresAt.IsZero(): + return Approval{}, fmt.Errorf("%w: token has no expiry; an approval that never lapses is not an approval", ErrNotApproved) + case len(steps) == 0: + return Approval{}, fmt.Errorf("%w: an empty plan cannot be approved", ErrNotApproved) + case !now.IsZero() && !now.Before(tok.ExpiresAt): + return Approval{}, fmt.Errorf("%w: approved %s, expired %s, now %s", ErrApprovalExpired, + tok.ApprovedAt.UTC().Format(time.RFC3339), tok.ExpiresAt.UTC().Format(time.RFC3339), + now.UTC().Format(time.RFC3339)) + } + keys := make(map[string]int, len(steps)) + for _, s := range steps { + if s.Target.Domain != Kind { + return Approval{}, fmt.Errorf("%w: step %d targets domain %q, not %q", + ErrNotApproved, s.Seq, s.Target.Domain, Kind) + } + if s.Key == "" { + return Approval{}, fmt.Errorf("%w: step %d has no key", ErrStepKeyMismatch, s.Seq) + } + if want := domain.StepKey(s.Target, s.From, s.To); s.Key != want { + return Approval{}, fmt.Errorf("%w: step %d claims %q, contents hash to %q", + ErrStepKeyMismatch, s.Seq, s.Key, want) + } + if prev, dup := keys[s.Key]; dup { + return Approval{}, fmt.Errorf("%w: steps %d and %d share key %q", + ErrNotApproved, prev, s.Seq, s.Key) + } + if tok.Scope != "" && s.Target.Scope != tok.Scope { + return Approval{}, fmt.Errorf("%w: step %d targets %q, token covers %q", + ErrScopeMismatch, s.Seq, s.Target.Scope, tok.Scope) + } + keys[s.Key] = s.Seq + } + if fp := PlanFingerprint(steps); fp != tok.Fingerprint { + return Approval{}, fmt.Errorf("%w: steps hash to %q, token approves %q", + ErrFingerprintMismatch, fp, tok.Fingerprint) + } + return Approval{token: tok, keys: keys}, nil +} + +// Token returns a copy of the underlying token, for the ledger and the report. +func (a Approval) Token() ApprovalToken { return a.token } + +// Fingerprint is the approved plan fingerprint, or "" for the zero Approval. +func (a Approval) Fingerprint() string { return a.token.Fingerprint } + +// Valid reports whether this approval was constructed by [NewApproval]. +func (a Approval) Valid() bool { return len(a.keys) > 0 && a.token.Fingerprint != "" } + +// Covers reports whether a step key is one this approval authorizes. +func (a Approval) Covers(key string) bool { _, ok := a.keys[key]; return ok } + +// Steps returns the authorized step keys in canonical (sequence, then key) +// order — never map order. +func (a Approval) Steps() []string { + if len(a.keys) == 0 { + return nil + } + out := make([]string, 0, len(a.keys)) + for k := range a.keys { + out = append(out, k) + } + sort.Slice(out, func(i, j int) bool { + if a.keys[out[i]] != a.keys[out[j]] { + return a.keys[out[i]] < a.keys[out[j]] + } + return out[i] < out[j] + }) + return out +} + +// Authorize turns a step into the only value this package's actuator accepts. +// +// It re-checks everything [NewApproval] checked about this one step, because +// the step handed to Authorize need not be the same value that was hashed — +// callers hold plans in slices, maps and JSON, and "it was in the plan when we +// approved it" is a belief, not a fact. +func (a Approval) Authorize(step domain.Step) (ApprovedStep, error) { + if !a.Valid() { + return ApprovedStep{}, fmt.Errorf("%w: no approval was presented", ErrNotApproved) + } + if step.Key == "" { + return ApprovedStep{}, fmt.Errorf("%w: step has no key", ErrStepKeyMismatch) + } + if want := domain.StepKey(step.Target, step.From, step.To); step.Key != want { + return ApprovedStep{}, fmt.Errorf("%w: step claims %q, contents hash to %q", + ErrStepKeyMismatch, step.Key, want) + } + if !a.Covers(step.Key) { + return ApprovedStep{}, fmt.Errorf("%w: %s is not in plan %s", + ErrStepNotInPlan, step.Key, a.token.Fingerprint) + } + if a.token.Scope != "" && step.Target.Scope != a.token.Scope { + return ApprovedStep{}, fmt.Errorf("%w: step targets %q, token covers %q", + ErrScopeMismatch, step.Target.Scope, a.token.Scope) + } + return ApprovedStep{step: step, approval: a, authorized: true}, nil +} + +// AuthorizeAll authorizes a whole plan, in the order given, failing on the +// first step the approval does not cover. +func (a Approval) AuthorizeAll(steps []domain.Step) ([]ApprovedStep, error) { + out := make([]ApprovedStep, 0, len(steps)) + for _, s := range steps { + as, err := a.Authorize(s) + if err != nil { + return nil, err + } + out = append(out, as) + } + return out, nil +} + +// ApprovedStep is a step plus the approval that authorizes it. Its fields are +// unexported and there is no exported constructor other than +// [Approval.Authorize], so the zero value is the only one a foreign package +// can build — and the zero value cannot act. +type ApprovedStep struct { + step domain.Step + approval Approval + // origin is the key of the step this one undoes, set only by + // [Actuator.Revert]. + origin string + // undo marks the inverse step [Actuator.Revert] builds. + undo bool + // authorized is the bit the execute path reads. It is separate from the + // approval's own validity so the zero ApprovedStep is unambiguously inert + // even if a future field arrangement makes Approval's zero value look + // plausible. + authorized bool +} + +// Step returns the underlying step. Reading a step is harmless; acting on one +// is what needs the approval, and there is no path from here back to Execute. +func (s ApprovedStep) Step() domain.Step { return s.step } + +// Approved reports whether this value came from [Approval.Authorize]. +func (s ApprovedStep) Approved() bool { return s.authorized && s.approval.Valid() } + +// Token returns the approving token, zero for an unauthorized step. +func (s ApprovedStep) Token() ApprovalToken { return s.approval.token } + +// IsUndo reports whether this is the inverse step [Actuator.Revert] builds. +func (s ApprovedStep) IsUndo() bool { return s.undo } + +// check re-validates the approval at execution time. An approval that was +// valid when the plan was authorized can expire while the plan runs — a +// storage modification and its optimization phase take hours — so the expiry +// is re-read at every step rather than once at the top. +// +// An UNDO is exempt from the expiry and the coverage test, and still requires +// a real approval. The inverse step's key was never in the approved plan (it +// cannot be: the plan approved making the change, not unmaking it), and an +// approval that lapsed while the plan ran is precisely when an undo is most +// needed. What an undo can do is bounded elsewhere: it restores the From this +// actuator itself recorded, and EVERY pre-flight predicate still runs against +// it, including the regime baseline (FINDINGS.md §5.6). +// +// It takes now as an argument; this package reads no clock. +func (s ApprovedStep) check(now time.Time) error { + if !s.Approved() { + return fmt.Errorf("%w: execution requires an approval token (design §3.3 HITL gate)", ErrNotApproved) + } + if s.undo { + return nil + } + if !now.IsZero() && !now.Before(s.approval.token.ExpiresAt) { + return fmt.Errorf("%w: token expired %s, now %s", ErrApprovalExpired, + s.approval.token.ExpiresAt.UTC().Format(time.RFC3339), now.UTC().Format(time.RFC3339)) + } + if !s.approval.Covers(s.step.Key) { + return fmt.Errorf("%w: %s", ErrStepNotInPlan, s.step.Key) + } + return nil +} diff --git a/pkg/rds/actuate_fixture.go b/pkg/rds/actuate_fixture.go new file mode 100644 index 0000000..7026cf2 --- /dev/null +++ b/pkg/rds/actuate_fixture.go @@ -0,0 +1,411 @@ +package rds + +// StorageActuateFixture: a fake RDS account, made of maps. +// +// It exists so that EVERY test in this unit — including the ones that "modify" +// a database — runs with no SDK, no socket, no credential and no ~/.aws. That +// is a hard rule for this package (surface_test.go's TestNoForeignImports is +// the structural half), and a fixture is what makes it a rule people can +// follow rather than one they route around. +// +// It models the three RDS behaviours that make this actuator's shape +// necessary, because a fixture that returns success immediately would let +// every resumability and idempotency test pass without proving anything: +// +// 1. **Asynchrony.** A modification does not take effect on the call. The +// instance walks available → modifying → storage-optimization → available +// across subsequent describes, with the new values visible in +// PendingModifiedValues first and in the top-level fields later. +// 2. **The four-per-24-hours limit.** Every accepted modification appends a +// `DescribeEvents` record, so a fixture-backed test can drive an instance +// into its own cooldown the way a real account would. +// 3. **Lost responses.** [StorageActuateFixture.FailAfter] fails a call AFTER +// its effect has landed, which is the crash window the idempotency and +// resume paths exist for. +// +// It is exported for the same reason [EnvelopeFixture] and [Fixture] are: the +// seam is the contract, and a contract nobody outside the package can exercise +// is not a contract. + +import ( + "context" + "fmt" + "sort" + "strings" + "sync" + "time" +) + +// Operation names the fixture counts. The VALUES name the real AWS operations +// the adapter calls, which is the honest mapping — see actuate_api.go for why +// the Go identifiers cannot. +const ( + OpDescribeInstanceState = "DescribeDBInstances" + OpModifyStorage = "ModifyDBInstance" +) + +// modPhase is how far a fixture-side modification has walked. +type modPhase int + +const ( + phaseNone modPhase = iota + phaseAccepted + phaseModifying + phaseOptimizing +) + +// StorageActuateFixture is a fake RDS account. +type StorageActuateFixture struct { + // Now is the fixture's clock. Required: an accepted modification is + // stamped with it and becomes part of the cooldown history. + Now func() time.Time + + // SettleAfter is how many describes the instance spends in EACH transient + // phase (accepted → modifying → storage-optimization) before advancing. + // Zero advances on every describe, which is the fast path most tests want; + // a larger value is how a test drives the poll budget to expiry. + SettleAfter int + + // Envelope is the answer DescribeValidDBInstanceModifications gives, per + // instance identifier. An identifier with no entry answers with no + // storage options at all, which is an UNKNOWN envelope and a refusal. + Envelope map[string][]ValidStorageOptionRecord + // Events seeds the modification history. Modifications this fixture + // accepts are appended to it. + Events map[string][]EventRecord + // PageSize splits every paginated events response; 0 means one page. + PageSize int + + // Fail is consulted BEFORE an operation's effect; a non-nil return fails + // the call and changes nothing. + Fail func(op string, n int) error + // FailAfter is consulted AFTER the effect has landed — the lost-response + // case, which is the only interesting failure for an actuator. + FailAfter func(op string, n int) error + // EnvelopeErr and EventsErr fail the two read seams per instance. + EnvelopeErr map[string]error + EventsErr map[string]error + + mu sync.Mutex + insts map[string]*InstanceStateRecord + phase map[string]modPhase + ticks map[string]int + tokens map[string]bool + counts map[string]int + ops []string +} + +// NewStorageActuateFixture builds a fixture from a set of instances. The +// records are copied, so a caller's slice cannot mutate the account behind the +// test's back. +func NewStorageActuateFixture(now func() time.Time, insts ...InstanceStateRecord) *StorageActuateFixture { + f := &StorageActuateFixture{ + Now: now, + Envelope: map[string][]ValidStorageOptionRecord{}, + Events: map[string][]EventRecord{}, + insts: map[string]*InstanceStateRecord{}, + phase: map[string]modPhase{}, + ticks: map[string]int{}, + tokens: map[string]bool{}, + counts: map[string]int{}, + } + for i := range insts { + cp := insts[i] + if cp.Tags == nil { + cp.Tags = map[string]string{} + } + f.insts[cp.Identifier] = &cp + } + return f +} + +// WithEnvelope sets one instance's provisioning envelope. +func (f *StorageActuateFixture) WithEnvelope(id string, recs ...ValidStorageOptionRecord) *StorageActuateFixture { + f.mu.Lock() + defer f.mu.Unlock() + f.Envelope[id] = append([]ValidStorageOptionRecord(nil), recs...) + return f +} + +// WithEvents seeds one instance's modification history. +func (f *StorageActuateFixture) WithEvents(id string, evs ...EventRecord) *StorageActuateFixture { + f.mu.Lock() + defer f.mu.Unlock() + f.Events[id] = append([]EventRecord(nil), evs...) + return f +} + +// Instance returns a copy of one instance's live state. +func (f *StorageActuateFixture) Instance(id string) (InstanceStateRecord, bool) { + f.mu.Lock() + defer f.mu.Unlock() + d, ok := f.insts[id] + if !ok { + return InstanceStateRecord{}, false + } + cp := *d + return cp, true +} + +// SetInstance overwrites one instance's live state — how a test makes somebody +// else change a database mid-plan. +func (f *StorageActuateFixture) SetInstance(r InstanceStateRecord) { + f.mu.Lock() + defer f.mu.Unlock() + cp := r + f.insts[r.Identifier] = &cp +} + +// Remove deletes an instance, so a test can make one vanish mid-modification. +func (f *StorageActuateFixture) Remove(id string) { + f.mu.Lock() + defer f.mu.Unlock() + delete(f.insts, id) +} + +// Count returns how many times one operation was called. +func (f *StorageActuateFixture) Count(op string) int { + f.mu.Lock() + defer f.mu.Unlock() + return f.counts[op] +} + +// Mutations counts the mutating calls this fixture accepted. It is the number +// every idempotency test asserts, because the four-per-24-hours limit makes a +// duplicate modification expensive in a way a duplicate read is not. +func (f *StorageActuateFixture) Mutations() int { return f.Count(OpModifyStorage) } + +// Ops returns the operation names in call order. +func (f *StorageActuateFixture) Ops() []string { + f.mu.Lock() + defer f.mu.Unlock() + return append([]string(nil), f.ops...) +} + +func (f *StorageActuateFixture) enter(ctx context.Context, op string) (int, error) { + if err := ctx.Err(); err != nil { + return 0, err + } + f.mu.Lock() + f.counts[op]++ + n := f.counts[op] + f.ops = append(f.ops, op) + fail := f.Fail + f.mu.Unlock() + if fail != nil { + if err := fail(op, n); err != nil { + return n, err + } + } + return n, nil +} + +func (f *StorageActuateFixture) leave(op string, n int) error { + f.mu.Lock() + after := f.FailAfter + f.mu.Unlock() + if after == nil { + return nil + } + return after(op, n) +} + +func (f *StorageActuateFixture) now() time.Time { + if f.Now == nil { + return time.Time{} + } + return f.Now() +} + +// --- the read seam ---------------------------------------------------------- + +// DescribeInstanceState implements [StorageActuateAPI]. It ALSO advances a +// pending modification one phase per SettleAfter+1 describes, which is what +// makes the polling path in actuate.go real rather than decorative. +func (f *StorageActuateFixture) DescribeInstanceState(ctx context.Context, + in *DescribeInstanceStateInput) (*DescribeInstanceStateOutput, error) { + + n, err := f.enter(ctx, OpDescribeInstanceState) + if err != nil { + return nil, err + } + f.mu.Lock() + rec, ok := f.insts[in.DBInstanceIdentifier] + if ok { + f.advance(in.DBInstanceIdentifier, rec) + } + var out DescribeInstanceStateOutput + if ok { + cp := *rec + cp.Tags = copyTags(rec.Tags) + out = DescribeInstanceStateOutput{Instance: cp, Found: true} + } + f.mu.Unlock() + if err := f.leave(OpDescribeInstanceState, n); err != nil { + return nil, err + } + return &out, nil +} + +// advance walks one instance's in-flight modification. Caller holds f.mu. +// +// The order of the walk is the RDS one and it matters: the new configuration +// becomes visible in the top-level fields when the instance enters +// storage-optimization, NOT when it returns to available. An actuator that +// waited for `available` before believing the values would be right; one that +// believed them at `modifying` would be wrong. This fixture makes the +// difference observable. +func (f *StorageActuateFixture) advance(id string, rec *InstanceStateRecord) { + ph := f.phase[id] + if ph == phaseNone { + return + } + f.ticks[id]++ + if f.ticks[id] <= f.SettleAfter { + return + } + f.ticks[id] = 0 + switch ph { + case phaseAccepted: + f.phase[id] = phaseModifying + rec.Status = StatusModifying + case phaseModifying: + f.phase[id] = phaseOptimizing + rec.Status = StatusStorageOptimization + // The change lands here. + if rec.PendingStorageType != "" { + rec.StorageType = rec.PendingStorageType + } + if rec.PendingIOPS > 0 { + rec.IOPS = rec.PendingIOPS + } + if rec.PendingStorageThroughputMBps > 0 { + rec.StorageThroughputMBps = rec.PendingStorageThroughputMBps + } + rec.PendingStorageType, rec.PendingIOPS, rec.PendingStorageThroughputMBps = "", 0, 0 + case phaseOptimizing: + f.phase[id] = phaseNone + rec.Status = StatusAvailable + } +} + +// --- the write seam --------------------------------------------------------- + +// ModifyStorage implements [StorageActuateAPI]. +// +// It enforces the two RDS rules an actuator can get wrong: an instance that is +// not `available` rejects the call, and an instance with four modifications in +// the trailing 24 hours rejects it too. A fixture that accepted everything +// would let a broken pre-flight pass every test in this unit. +func (f *StorageActuateFixture) ModifyStorage(ctx context.Context, + in *ModifyStorageInput) (*ModifyStorageOutput, error) { + + n, err := f.enter(ctx, OpModifyStorage) + if err != nil { + return nil, err + } + f.mu.Lock() + rec, ok := f.insts[in.DBInstanceIdentifier] + if !ok { + f.mu.Unlock() + return nil, fmt.Errorf("rds fixture: DBInstanceNotFound: %s", in.DBInstanceIdentifier) + } + // The client token AWS does not have. Deduplicating on it here is what + // lets a test prove that a retried step issues at most one EFFECTIVE + // modification even when the response to the first was lost — see + // ModifyStorageInput.ClientToken for what the adapter must do with it. + if in.ClientToken != "" && f.tokens[in.ClientToken] { + cp := *rec + f.mu.Unlock() + if err := f.leave(OpModifyStorage, n); err != nil { + return nil, err + } + return &ModifyStorageOutput{Instance: cp}, nil + } + if st := strings.ToLower(strings.TrimSpace(rec.Status)); st != StatusAvailable { + f.mu.Unlock() + return nil, fmt.Errorf("rds fixture: InvalidDBInstanceState: %s is %s", + in.DBInstanceIdentifier, rec.Status) + } + at := f.now() + if f.recentModifications(in.DBInstanceIdentifier, at) >= MaxStorageModificationsPer24h { + f.mu.Unlock() + return nil, fmt.Errorf("rds fixture: InvalidDBInstanceState: %s has had %d storage modifications "+ + "in 24 hours", in.DBInstanceIdentifier, MaxStorageModificationsPer24h) + } + if in.ClientToken != "" { + f.tokens[in.ClientToken] = true + } + rec.PendingStorageType = in.StorageType + rec.PendingIOPS = in.IOPS + rec.PendingStorageThroughputMBps = in.StorageThroughputMBps + f.phase[in.DBInstanceIdentifier] = phaseAccepted + f.ticks[in.DBInstanceIdentifier] = 0 + f.Events[in.DBInstanceIdentifier] = append(f.Events[in.DBInstanceIdentifier], EventRecord{ + SourceIdentifier: in.DBInstanceIdentifier, + SourceType: EventSourceDBInstance, + Message: "Finished applying modification to allocated storage", + Categories: []string{EventCategoryConfigurationChange}, + Date: at, + }) + cp := *rec + f.mu.Unlock() + if err := f.leave(OpModifyStorage, n); err != nil { + return nil, err + } + return &ModifyStorageOutput{Instance: cp}, nil +} + +// recentModifications counts storage modifications inside the trailing +// window. Caller holds f.mu. +func (f *StorageActuateFixture) recentModifications(id string, now time.Time) int { + cut := now.Add(-StorageModificationWindow) + count := 0 + for _, ev := range f.Events[id] { + if IsStorageModificationEvent(ev) && ev.Date.After(cut) && !ev.Date.After(now) { + count++ + } + } + return count +} + +// --- the envelope seam ------------------------------------------------------ + +// DescribeValidDBInstanceModifications implements [ModificationEnvelopeAPI]. +func (f *StorageActuateFixture) DescribeValidDBInstanceModifications(ctx context.Context, + in *DescribeValidDBInstanceModificationsInput) (*DescribeValidDBInstanceModificationsOutput, error) { + + if _, err := f.enter(ctx, "DescribeValidDBInstanceModifications"); err != nil { + return nil, err + } + f.mu.Lock() + defer f.mu.Unlock() + if err := f.EnvelopeErr[in.DBInstanceIdentifier]; err != nil { + return nil, err + } + return &DescribeValidDBInstanceModificationsOutput{ + ValidStorageOptions: append([]ValidStorageOptionRecord(nil), f.Envelope[in.DBInstanceIdentifier]...), + }, nil +} + +// DescribeEvents implements [ModificationEnvelopeAPI], with real pagination. +func (f *StorageActuateFixture) DescribeEvents(ctx context.Context, + in *DescribeEventsInput) (*DescribeEventsOutput, error) { + + if _, err := f.enter(ctx, "DescribeEvents"); err != nil { + return nil, err + } + f.mu.Lock() + defer f.mu.Unlock() + if err := f.EventsErr[in.SourceIdentifier]; err != nil { + return nil, err + } + all := append([]EventRecord(nil), f.Events[in.SourceIdentifier]...) + sort.SliceStable(all, func(i, j int) bool { return all[i].Date.Before(all[j].Date) }) + start, err := offsetOf(in.Marker) + if err != nil { + return nil, err + } + end, next := paginate(len(all), start, f.PageSize) + return &DescribeEventsOutput{Events: all[min(start, len(all)):end], Marker: next}, nil +} diff --git a/pkg/rds/actuate_helpers_test.go b/pkg/rds/actuate_helpers_test.go new file mode 100644 index 0000000..8538b71 --- /dev/null +++ b/pkg/rds/actuate_helpers_test.go @@ -0,0 +1,221 @@ +package rds + +import ( + "context" + "encoding/json" + "reflect" + "strconv" + "testing" + "time" + + "github.com/agenticode/kilter/pkg/domain" +) + +// The scenario every test in this unit starts from: a 500 GiB MySQL instance, +// which is ABOVE the 400 GiB striping threshold and therefore in the striped +// gp3 regime — 12,000 IOPS / 500 MiB/s free, provisioning permitted. That is +// the only regime where this actuator can do anything at all, so it is the one +// worth making the default. +const ( + actID = "prod-orders" + actScope = "123456789012/us-east-1" + actSize = int64(500) + actEngine = "mysql" +) + +func actNow() time.Time { return time.Date(2026, 8, 26, 12, 0, 0, 0, time.UTC) } + +func actClock() func() time.Time { return actNow } + +// actGP3Envelope is the envelope a striped 500 GiB MySQL instance reports. +// Every ceiling in it comes from here rather than from a constant in the +// package, which is TestProvisioningEnvelopeIsReadNeverHardcoded's whole point. +func actGP3Envelope() ValidStorageOptionRecord { + return ValidStorageOptionRecord{ + StorageType: StorageGP3, MinIOPS: 12000, MaxIOPS: 64000, + MinStorageThroughputMBps: 500, MaxStorageThroughputMBps: 4000, + MinAllocatedStorageGiB: 20, MaxAllocatedStorageGiB: 65536, + } +} + +// actLive builds the live record for the default scenario: gp2, available, +// tagged, nothing pending. +func actLive(opts ...func(*InstanceStateRecord)) InstanceStateRecord { + r := InstanceStateRecord{ + Identifier: actID, ARN: "arn:aws:rds:us-east-1:123456789012:db:" + actID, + Engine: actEngine, LicenseModel: "general-public-license", Status: StatusAvailable, + AllocatedStorageGiB: actSize, StorageType: StorageGP2, + Tags: map[string]string{"env": "prod"}, TagsKnown: true, + } + for _, o := range opts { + o(&r) + } + return r +} + +// actFixture wires the default scenario: one instance, a known envelope, and +// an empty (but KNOWN) modification history. +func actFixture(t *testing.T, opts ...func(*InstanceStateRecord)) *StorageActuateFixture { + t.Helper() + f := NewStorageActuateFixture(actClock(), actLive(opts...)) + f.WithEnvelope(actID, actGP3Envelope()) + f.WithEvents(actID) + return f +} + +func actRef() domain.TargetRef { + return domain.TargetRef{Domain: Kind, Scope: actScope, ID: actID} +} + +// actSpec builds one side of a step. +func actSpec(engine, storageType string, sizeGiB int64, iops, tput int32) domain.Spec { + s := domain.Spec{Attrs: map[string]string{ + AttrEngine: engine, + AttrLicenseModel: "general-public-license", + AttrStorageType: storageType, + AttrAllocatedStorageGiB: strconv.FormatInt(sizeGiB, 10), + }} + if iops >= 0 { + s.Attrs[AttrIOPS] = strconv.FormatInt(int64(iops), 10) + } + if tput >= 0 { + s.Attrs[AttrStorageThroughput] = strconv.FormatInt(int64(tput), 10) + } + return s +} + +// actStep is the default step: convert the 500 GiB gp2 volume to gp3 at +// 12,000 IOPS / 1,000 MiB/s. +// +// Read the numbers: 12,000 IOPS is EXACTLY the striped regime's free baseline, +// so the call must NOT send --iops. 1,000 MiB/s is above the 500 MiB/s +// baseline, so it must. That asymmetry inside one step is FINDINGS.md §5.1 in +// its smallest form, and TestActuateSendsOnlyProvisionedArguments turns on it. +func actStep(from, to domain.Spec) domain.Step { + s := domain.Step{ + Seq: 1, Target: actRef(), Action: domain.ActionInPlace, + From: from, To: to, Risk: RiskLow, + Detail: "gp2 → gp3 at measured parity", + } + s.Key = domain.StepKey(s.Target, s.From, s.To) + return s +} + +func actDefaultStep() domain.Step { + return actStep( + actSpec(actEngine, StorageGP2, actSize, -1, -1), + actSpec(actEngine, StorageGP3, actSize, 12000, 1000), + ) +} + +// actApproval builds a real approval over the given steps. There is no way to +// obtain one that skips a check, which is the point. +func actApproval(t *testing.T, steps ...domain.Step) Approval { + t.Helper() + tok := ApprovalToken{ + Fingerprint: PlanFingerprint(steps), Scope: actScope, ApprovedBy: "alan", + ApprovedAt: actNow().Add(-time.Minute), ExpiresAt: actNow().Add(time.Hour), + } + ap, err := NewApproval(steps, tok, actNow()) + if err != nil { + t.Fatalf("NewApproval: %v", err) + } + return ap +} + +func actApproved(t *testing.T, step domain.Step) ApprovedStep { + t.Helper() + as, err := actApproval(t, step).Authorize(step) + if err != nil { + t.Fatalf("Authorize: %v", err) + } + return as +} + +// actActuator builds an actuator whose Sleep does not spend real time. +func actActuator(t *testing.T, f *StorageActuateFixture, mode Mode) *Actuator { + t.Helper() + a, err := NewActuator(f, ActuatorConfig{ + Mode: mode, Now: actClock(), + PollInterval: time.Second, PollTimeout: time.Minute, + Sleep: func(ctx context.Context, d time.Duration) error { return ctx.Err() }, + }) + if err != nil { + t.Fatalf("NewActuator: %v", err) + } + return a +} + +// actExecute runs one step and returns the error, whatever it is. +func actExecute(t *testing.T, a *Actuator, step domain.Step) error { + t.Helper() + return a.Execute(context.Background(), actApproved(t, step)) +} + +// wantRefusal asserts that err is a refusal carrying exactly the given code. +// Every refusal test in this unit ends in this call, so a refusal that changes +// its code silently is impossible. +func wantRefusal(t *testing.T, err error, code string) *RefusalError { + t.Helper() + if err == nil { + t.Fatalf("want refusal %q, got nil: THE ACTUATOR WOULD HAVE MODIFIED A DATABASE", code) + } + if !IsRefusal(err) { + t.Fatalf("want refusal %q, got a non-refusal error: %v", code, err) + } + if got := RefusalCode(err); got != code { + t.Fatalf("refusal code = %q, want %q (%v)", got, code, err) + } + var r *RefusalError + if !asRefusal(err, &r) { + t.Fatalf("refusal %v is not a *RefusalError", err) + } + if r.Reason == "" { + t.Errorf("refusal %q carries no reason; a refusal a human cannot act on is a bug", code) + } + return r +} + +func asRefusal(err error, out **RefusalError) bool { + for e := err; e != nil; { + if r, ok := e.(*RefusalError); ok { + *out = r + return true + } + u, ok := e.(interface{ Unwrap() error }) + if !ok { + return false + } + e = u.Unwrap() + } + return false +} + +// interfaceMethodNames returns an interface's method names in sorted order, +// for the "this seam has exactly these operations" assertions. +func interfaceMethodNames(t *testing.T, ptr any) []string { + t.Helper() + rt := reflect.TypeOf(ptr) + if rt == nil || rt.Kind() != reflect.Pointer || rt.Elem().Kind() != reflect.Interface { + t.Fatalf("interfaceMethodNames: %T is not a pointer to an interface", ptr) + } + it := rt.Elem() + out := make([]string, 0, it.NumMethod()) + for i := range it.NumMethod() { + out = append(out, it.Method(i).Name) + } + sortStrings(out) + return out +} + +// renderCall serializes an issued call so a test can assert on the whole of +// what would go over the wire rather than on the fields it remembered to look +// at. +func renderCall(t *testing.T, in ModifyStorageInput) string { + t.Helper() + b, err := json.Marshal(in) + if err != nil { + t.Fatal(err) + } + return string(b) +} diff --git a/pkg/rds/actuate_ledger.go b/pkg/rds/actuate_ledger.go new file mode 100644 index 0000000..073837d --- /dev/null +++ b/pkg/rds/actuate_ledger.go @@ -0,0 +1,438 @@ +package rds + +// The step ledger: what was attempted, what was sent, and what is still +// running. +// +// The ledger is not bookkeeping. It is the thing that makes a controller +// restart safe: an entry that is neither terminal nor settled names a database +// that may have a storage modification in flight right now with nobody +// watching it, and [Actuator.Unsettled] is the list a controller works through +// on startup. Every status change goes through [ActuatorConfig.Persist] where +// one is wired, and it is called BEFORE the mutating call, never after — +// the crash window then contains "we may have modified it" rather than "we +// definitely modified it and nobody knows". + +import ( + "context" + "encoding/json" + "fmt" + "math" + "sort" + "strconv" + "strings" + "time" + + "github.com/agenticode/kilter/pkg/domain" +) + +func (a *Actuator) entry(key string) (LedgerEntry, bool) { + if key == "" { + return LedgerEntry{}, false + } + a.mu.Lock() + defer a.mu.Unlock() + e, ok := a.ledger[key] + if !ok { + return LedgerEntry{}, false + } + return *e, true +} + +// Entry returns one step's ledger entry. +func (a *Actuator) Entry(key string) (LedgerEntry, bool) { return a.entry(key) } + +// upsert returns the entry for a step, creating it on first sight. Caller +// holds a.mu. +func (a *Actuator) upsert(step domain.Step, as ApprovedStep, now time.Time) *LedgerEntry { + key := step.Key + if key == "" { + key = domain.StepKey(step.Target, step.From, step.To) + } + e, ok := a.ledger[key] + if !ok { + e = &LedgerEntry{ + Key: key, Target: step.Target, Action: step.Action, + From: step.From, To: step.To, StartedAt: now, + Revert: as.undo, Origin: as.origin, + Fingerprint: as.approval.token.Fingerprint, + ApprovedBy: as.approval.token.ApprovedBy, + } + // The claim is read from the step, never computed here. A second + // arithmetic would be a second source of truth for the bill. + if raw := strings.TrimSpace(step.To.Attr(AttrNetSavingsMonthlyUSD)); raw != "" { + if v := floatOrNaN(raw); v == v { + e.ClaimedMonthlyUSD, e.Claimed = v, true + } + } + a.ledger[key] = e + a.order = append(a.order, key) + } + return e +} + +// record writes (or updates) the entry for a step. +func (a *Actuator) record(step domain.Step, as ApprovedStep, now time.Time, status, detail string, err error) { + a.recordCall(step, as, now, status, "", detail, ModifyStorageInput{}, err) +} + +// recordCall records a status together with the exact call the step would +// make. Dry-run uses it, which is what makes a dry-run a preview of a specific +// API call rather than a promise about one. +func (a *Actuator) recordCall(step domain.Step, as ApprovedStep, now time.Time, + status string, stage Stage, detail string, call ModifyStorageInput, err error) { + + a.mu.Lock() + defer a.mu.Unlock() + e := a.upsert(step, as, now) + e.Mode = a.cfg.Mode + e.Status = status + if stage != "" { + e.Stage = stage + } + if detail != "" { + e.Detail = detail + } + if call.DBInstanceIdentifier != "" { + e.Sent = call + } + if err != nil { + e.Error = err.Error() + e.RefusalCode = RefusalCode(err) + e.ValidFrom = RefusalValidFrom(err) + } else { + e.Error, e.RefusalCode, e.ValidFrom = "", "", time.Time{} + } + if status != StatusInFlight { + e.FinishedAt = now + } +} + +// mutate is the persist-before-act barrier. +// +// It records the intent AND the exact call, flushes the ledger through +// [ActuatorConfig.Persist], and only then lets the caller issue it. A Persist +// failure ABORTS the mutation: a storage modification nobody wrote down is +// precisely the state this unit must never reach, so failing to record is +// failing to act. +func (a *Actuator) mutate(ctx context.Context, step domain.Step, as ApprovedStep, + now time.Time, stage Stage, detail string, call ModifyStorageInput) error { + + a.mu.Lock() + e := a.upsert(step, as, now) + e.Mode = a.cfg.Mode + e.Status = StatusInFlight + e.Stage = stage + e.Detail = detail + e.Sent = call + e.Attempts++ + attempts := e.Attempts + a.mu.Unlock() + if attempts > 1 { + // Not fatal, but it must be visible: a second mutating call for one + // step spends a second of the four modifications this instance gets + // in 24 hours. + a.cfg.Logger.Warn("rds storage modification retried", + "instance", step.Target.ID, "attempts", attempts, "key", step.Key) + } + return a.persist(ctx) +} + +// persist flushes the ledger when a hook is wired. +func (a *Actuator) persist(ctx context.Context) error { + if a.cfg.Persist == nil { + return nil + } + b, err := a.LedgerJSON() + if err != nil { + return fmt.Errorf("rds: serialize ledger: %w", err) + } + if err := a.cfg.Persist(ctx, b); err != nil { + return fmt.Errorf("rds: persist ledger before modifying: %w", err) + } + return nil +} + +// markIssued records the moment RDS accepted the modification. That instant +// starts this instance's 24-hour window, so it is stored rather than derived. +func (a *Actuator) markIssued(key string, at time.Time) { + a.mu.Lock() + defer a.mu.Unlock() + if e, ok := a.ledger[key]; ok && e.IssuedAt.IsZero() { + e.IssuedAt = at + } +} + +func (a *Actuator) setDetail(key, detail string) { + a.mu.Lock() + defer a.mu.Unlock() + if e, ok := a.ledger[key]; ok { + e.Detail = detail + } +} + +func (a *Actuator) addPolls(key string, n int) { + a.mu.Lock() + defer a.mu.Unlock() + if e, ok := a.ledger[key]; ok { + e.Polls += n + } +} + +// finish closes an entry out. +func (a *Actuator) finish(key, status string, stage Stage, now time.Time, detail string, err error) { + a.mu.Lock() + defer a.mu.Unlock() + e, ok := a.ledger[key] + if !ok { + return + } + e.Status = status + if stage != "" { + e.Stage = stage + } + if detail != "" { + e.Detail = detail + } + e.FinishedAt = now + if err != nil { + e.Error = err.Error() + e.RefusalCode = RefusalCode(err) + e.ValidFrom = RefusalValidFrom(err) + } else { + e.Error, e.RefusalCode, e.ValidFrom = "", "", time.Time{} + } +} + +// Ledger returns every recorded entry, ordered by first sight — deterministic +// for a given step sequence and independent of map iteration. +func (a *Actuator) Ledger() []LedgerEntry { + a.mu.Lock() + defer a.mu.Unlock() + out := make([]LedgerEntry, 0, len(a.order)) + for _, k := range a.order { + if e, ok := a.ledger[k]; ok { + out = append(out, *e) + } + } + return out +} + +// LedgerJSON serializes the ledger for pkg/store. Entries are emitted in key +// order so the bytes are stable across processes. +func (a *Actuator) LedgerJSON() ([]byte, error) { + entries := a.Ledger() + sort.SliceStable(entries, func(i, j int) bool { return entries[i].Key < entries[j].Key }) + return json.Marshal(entries) +} + +// RestoreLedger reloads a serialized ledger, so a restarted controller knows +// which steps it already finished — and, more importantly, which ones it +// started and has not. +func (a *Actuator) RestoreLedger(b []byte) error { + if len(b) == 0 { + return nil + } + var entries []LedgerEntry + if err := json.Unmarshal(b, &entries); err != nil { + return fmt.Errorf("rds: restore ledger: %w", err) + } + a.mu.Lock() + defer a.mu.Unlock() + a.ledger = make(map[string]*LedgerEntry, len(entries)) + a.order = a.order[:0] + for i := range entries { + e := entries[i] + if e.Key == "" { + continue + } + if _, dup := a.ledger[e.Key]; dup { + continue + } + a.ledger[e.Key] = &e + a.order = append(a.order, e.Key) + } + return nil +} + +// Unsettled returns the entries describing work that is neither finished nor +// safely at rest, in key order. +// +// A controller calls this on startup. Every key it returns is a database that +// may have a storage modification running right now, and re-executing its step +// re-observes AWS rather than re-issuing anything: [Actuator.execute] derives +// the stage from a live read, finds StageAccepted / StageModifying / +// StageOptimizing, and resumes the observation. +func (a *Actuator) Unsettled() []LedgerEntry { + entries := a.Ledger() + out := make([]LedgerEntry, 0, len(entries)) + for _, e := range entries { + if !e.Settled() { + out = append(out, e) + } + } + sort.SliceStable(out, func(i, j int) bool { return out[i].Key < out[j].Key }) + return out +} + +// --- aggregates ------------------------------------------------------------- + +// LedgerSummary is the roll-up a report renders. Every field is produced from +// a sorted input, so the same entries in a different order give byte-identical +// output — TestActuateLedgerSummaryIsShuffleInvariant permutes and compares. +type LedgerSummary struct { + Entries int `json:"entries"` + // ByStatus and ByRefusal are ordered by descending count, then by code. + ByStatus []domain.CodeCount `json:"byStatus,omitempty"` + ByRefusal []domain.CodeCount `json:"byRefusal,omitempty"` + // ClaimedMonthlyUSD is the sum of the attested savings of the entries + // that COMPLETED. It is summed through [SumUSD] — sorted by name, then + // added — for the same reason U13 sums a bill that way: floating-point + // addition is not associative, and a total that depends on arrival order + // is a total that changes between two runs over the same data. + ClaimedMonthlyUSD float64 `json:"claimedMonthlyUSD,omitempty"` + // Unclaimed counts completed entries carrying no attestation, so a total + // can never quietly mean "everything". + Unclaimed int `json:"unclaimed,omitempty"` + // InFlight is the number an operator must never see stuck: modifications + // issued and not observed to completion. + InFlight int `json:"inFlight,omitempty"` + // NextClears is the earliest moment a dated refusal lapses, so a + // scheduler has one number to sleep until. + NextClears time.Time `json:"nextClears,omitzero"` +} + +// Summarize rolls a ledger up deterministically. +func Summarize(entries []LedgerEntry) LedgerSummary { + sorted := make([]LedgerEntry, len(entries)) + copy(sorted, entries) + sort.SliceStable(sorted, func(i, j int) bool { return sorted[i].Key < sorted[j].Key }) + + out := LedgerSummary{Entries: len(sorted)} + statuses := make([]string, 0, len(sorted)) + refusals := make([]string, 0, len(sorted)) + parts := make([]CostPart, 0, len(sorted)) + for _, e := range sorted { + if e.Status != "" { + statuses = append(statuses, e.Status) + } + if e.RefusalCode != "" { + refusals = append(refusals, e.RefusalCode) + } + if e.Status == StatusInFlight { + out.InFlight++ + } + if e.Status == StatusDone { + if e.Claimed { + parts = append(parts, CostPart{Name: e.Key, USD: e.ClaimedMonthlyUSD}) + } else { + out.Unclaimed++ + } + } + if !e.ValidFrom.IsZero() && (out.NextClears.IsZero() || e.ValidFrom.Before(out.NextClears)) { + out.NextClears = e.ValidFrom + } + } + out.ClaimedMonthlyUSD = SumUSD(parts) + out.ByStatus = tallyCodes(statuses) + out.ByRefusal = tallyCodes(refusals) + return out +} + +// tallyCodes counts codes into a canonically ordered slice: descending count, +// then code. It never ranges over a map on an output path without sorting +// after. +func tallyCodes(codes []string) []domain.CodeCount { + if len(codes) == 0 { + return nil + } + counts := make(map[string]int, len(codes)) + for _, c := range codes { + if c = strings.TrimSpace(c); c != "" { + counts[c]++ + } + } + out := make([]domain.CodeCount, 0, len(counts)) + for c, n := range counts { + out = append(out, domain.CodeCount{Code: c, Count: n}) + } + sort.Slice(out, func(i, j int) bool { + if out[i].Count != out[j].Count { + return out[i].Count > out[j].Count + } + return out[i].Code < out[j].Code + }) + return out +} + +// --- the registrable form --------------------------------------------------- + +// BoundActuator is an [Actuator] with an approval already attached. It is the +// ONLY form that satisfies [domain.Actuator], and therefore the only form +// [domain.Registry.RegisterActuator] will take. +// +// That is the structural half of the approval gate seen from the wiring side: +// cmd/ cannot register a bare actuator and let the registry drive it, because +// a bare actuator has no Execute(ctx, Step) method to satisfy the interface. +// It must first obtain an approval — which requires a token, which requires a +// human — and bind it to a specific plan fingerprint. +type BoundActuator struct { + a *Actuator + ap Approval +} + +// Bind attaches an approval, producing the registrable form. +func (a *Actuator) Bind(ap Approval) (*BoundActuator, error) { + if !ap.Valid() { + return nil, fmt.Errorf("%w: Bind needs an approval from NewApproval", ErrNotApproved) + } + return &BoundActuator{a: a, ap: ap}, nil +} + +// Domain implements domain.Actuator. +func (b *BoundActuator) Domain() domain.Kind { return Kind } + +// Fingerprint is the plan this actuator is bound to. +func (b *BoundActuator) Fingerprint() string { return b.ap.Fingerprint() } + +// Execute implements domain.Actuator. A step the bound approval does not cover +// is refused here, so binding one plan does not authorize another. +func (b *BoundActuator) Execute(ctx context.Context, step domain.Step) error { + as, err := b.ap.Authorize(step) + if err != nil { + b.a.record(step, ApprovedStep{}, b.a.cfg.Now(), StatusRefused, "", err) + return err + } + return b.a.Execute(ctx, as) +} + +// Revert implements domain.Actuator. +func (b *BoundActuator) Revert(ctx context.Context, step domain.Step) error { + as, err := b.ap.Authorize(step) + if err != nil { + return err + } + return b.a.Revert(ctx, as) +} + +// Ledger exposes the underlying actuator's ledger. +func (b *BoundActuator) Ledger() []LedgerEntry { return b.a.Ledger() } + +// LedgerSummary rolls the underlying ledger up. +func (b *BoundActuator) LedgerSummary() LedgerSummary { return Summarize(b.a.Ledger()) } + +// floatOrNaN parses a money attribute, returning NaN for anything unusable — +// including a non-finite literal — so a garbage value can never pass for zero. +// "No claim" and "claims exactly $0" are different statements and only one of +// them is a bug. +func floatOrNaN(raw string) float64 { + v, err := strconv.ParseFloat(strings.TrimSpace(raw), 64) + if err != nil || math.IsNaN(v) || math.IsInf(v, 0) { + return math.NaN() + } + return v +} + +// That *BoundActuator satisfies domain.Actuator — and *Actuator does not — is +// asserted in actuate_test.go rather than with the usual +// `var _ domain.Actuator = ...` line, because TestNoUnexpectedPackageState +// forbids package-level vars in this package, including blank ones. diff --git a/pkg/rds/actuate_preflight.go b/pkg/rds/actuate_preflight.go new file mode 100644 index 0000000..87c835b --- /dev/null +++ b/pkg/rds/actuate_preflight.go @@ -0,0 +1,715 @@ +package rds + +// The pre-flight refusal layer. +// +// This file is written first and runs first, and nothing in it can act. It is +// PURE: no I/O, no clock, no mutable state, no reachable mutation. It takes a +// step and the facts a read seam already fetched, and answers one question — +// may this database's storage be modified right now? — with either silence or +// a [RefusalError] carrying a stable machine-readable code. +// +// Four rules govern it, and the first is the one that matters. +// +// 1. **When in doubt, refuse.** Not "default to the safe value", not "assume +// the common case": refuse, with a code that names the missing fact. An +// unreadable modification history refuses. An unreadable tag set refuses. +// An envelope nobody answered refuses. FINDINGS.md §5.3 makes this +// explicit for the cooldown, where `Known=false` MUST block, and the same +// reasoning covers every other unknown here. +// 2. **The ratchet only turns one way.** This actuator moves a volume UP or +// SIDEWAYS and never down. A reduction of provisioned IOPS or throughput +// is a real saving U13 will happily identify, and it is also the change +// that starves a production primary of I/O if the measurement was wrong. +// U14 refuses to execute it; a human does it by hand. See +// ACTUATE-FINDINGS.md §4 for the honest cost of that decision. +// 3. **Everything is re-read live.** U13's assessment is minutes to hours +// old. The envelope, the modification history, the instance status and +// the allocated storage are all re-read microseconds before the call and +// re-validated with the SAME functions the read-only path used. +// 4. **The refusal is the product.** Every predicate has a code, the code is +// asserted by exactly one test, and the prose names the fact that blocked +// it. A refusal a human cannot act on is a bug. + +import ( + "errors" + "fmt" + "strconv" + "strings" + "time" + + "github.com/agenticode/kilter/pkg/domain" +) + +// ErrRefused is what every pre-flight refusal matches with errors.Is. The +// specific reason is the [RefusalError.Code], read with [RefusalCode]. +const ErrRefused actuateError = "rds: refused" + +// Refusal codes. +// +// Codes the read-only path already defines are REUSED, not redefined: an +// operator filtering on `storage-modification-cooldown` must see U13's +// suppression and U14's refusal under one code, or the roll-up silently splits +// one fact in two. +const ( + // --- FINDINGS.md §5.3: the four-per-24-hours limit --- + + // RefuseCooldown: four storage modifications already fell inside the + // trailing 24 hours. A fifth is an API error, not a change. + RefuseCooldown = ReasonParityCooldown + // RefuseCooldownUnknown: the event seam did not answer, so the count is + // UNKNOWN. §5.3: unknown never clears the cooldown. It is a separate code + // from [RefuseCooldown] because the operator's next action differs — + // "wait until ClearsAt" against "grant rds:DescribeEvents" — and one code + // for both would hide which of those is needed. + RefuseCooldownUnknown = "storage-modification-history-unknown" + + // --- FINDINGS.md §5.4: the in-flight gate, re-checked live --- + + // RefuseStateUnstable: the instance is `modifying` or + // `storage-optimization` right now. Reuses U13's code. + RefuseStateUnstable = ReasonParityStorageOptimization + // RefuseNotAvailable: the instance is in some other non-available state — + // stopped, backing-up, failing over, deleting. A storage modification + // against one of those either fails or lands somewhere nobody predicted. + RefuseNotAvailable = "instance-not-available" + // RefusePendingModification: RDS has already accepted a storage change it + // has not applied. Issuing a second one spends another of the four + // modifications this instance gets in 24 hours. + RefusePendingModification = "storage-modification-pending" + + // --- FINDINGS.md §5.2: the envelope, re-read live --- + + // RefuseEnvelopeUnknown: DescribeValidDBInstanceModifications was not + // answered for this instance at execute time. Reuses U13's code. + RefuseEnvelopeUnknown = ReasonParityEnvelopeUnknown + // RefuseExceedsEnvelope: the LIVE envelope rejects the configuration the + // plan carries. The commonest cause is the one §5.2 names: the instance + // class changed between plan and apply, and a different class has a + // different envelope. + RefuseExceedsEnvelope = ReasonParityExceedsEnvelope + // RefuseNotProvisionable: the size is below the striping threshold, where + // the published provisioning columns read "N/A". Reuses U13's code. + RefuseNotProvisionable = ReasonParityNotProvisionableBelowThreshold + // RefuseBaselineArgument: the step asks to SEND an --iops or + // --storage-throughput argument for a value that equals the regime + // baseline. The baseline is what the volume delivers for free; naming it + // is at best a wasted modification out of four and at worst an error. + RefuseBaselineArgument = "baseline-value-must-not-be-sent" + + // --- the trap-8 ratchet --- + + // RefuseRatchet: the step would reduce IOPS, throughput or allocated + // storage below what is there now. This actuator only moves up or + // sideways. + RefuseRatchet = "storage-performance-ratchet" + // RefuseAllocationDrift: the observed allocated storage no longer matches + // the allocation the proposal was computed against. Storage autoscaling + // moves that number without anyone asking, and every figure in the plan — + // the regime, the striping verdict, the price — was derived from the old + // one. + RefuseAllocationDrift = "allocated-storage-drift" + + // --- shape of the request --- + + // RefuseWrongAction: the step's action class is not the in-place storage + // modification this actuator performs. + RefuseWrongAction = "wrong-action" + // RefuseBadStep: the step is structurally unusable — no target, no + // storage type, a key that does not hash its own contents. + RefuseBadStep = "bad-step" + // RefuseNoChange: From and To describe the same configuration. + RefuseNoChange = "no-change" + // RefuseStorageTypeNotModelled: a storage type outside gp2/gp3. Reuses + // U13's code. + RefuseStorageTypeNotModelled = ReasonParityStorageTypeNotModelled + // RefuseSizeUnusable: an allocation outside 1–65,536 GiB. Reuses U13's. + RefuseSizeUnusable = ReasonParitySizeUnusable + // RefuseUnknownEngine: no gp3 regime is encoded for the engine. Reuses + // U11's code. + RefuseUnknownEngine = ReasonUnknownEngine + // RefuseEngineChanged: the step's From and To disagree about the engine, + // or the live instance runs a different one. The regime is engine-keyed, + // so this makes every number in the plan describe another database. + RefuseEngineChanged = "engine-mismatch" + + // --- guardrails --- + + // RefuseModeOff: kilter.dev/mode=off. Reuses U11's code. + RefuseModeOff = ReasonModeOff + // RefuseGuardrailUnknown: the tag set could not be read, so the mode + // guardrail is unknown. An unreadable "never touch this" is not an + // absent one. + RefuseGuardrailUnknown = "guardrail-tags-unknown" + + // --- live state --- + + // RefuseInstanceMissing: the instance is not in the account. + RefuseInstanceMissing = "instance-missing" + // RefuseDrift: the live instance matches neither the recorded From nor + // the intended To. Somebody else changed it; the plan is stale. + RefuseDrift = "drift" +) + +// RefusalError is a refusal with a stable code. It is the only error type this +// unit's pre-flight produces, so a caller can render a refusal report without +// string matching. +type RefusalError struct { + Code string `json:"code"` + Target domain.TargetRef `json:"target"` + Reason string `json:"reason"` + // ValidFrom is when a DATED refusal lapses on its own — for a cooldown, + // the moment the oldest of the four modifications leaves the 24-hour + // window (FINDINGS.md §5.3 calls this the right ValidFrom for a deferred + // step). It is zero for a refusal that does not clear by waiting, and the + // difference matters: a scheduler may retry the first kind and must never + // spin on the second. + ValidFrom time.Time `json:"validFrom,omitzero"` +} + +func (e *RefusalError) Error() string { + if e.Target.ID != "" { + return fmt.Sprintf("rds: refused %s (%s): %s", e.Target.ID, e.Code, e.Reason) + } + return fmt.Sprintf("rds: refused (%s): %s", e.Code, e.Reason) +} + +// Is makes every refusal match [ErrRefused], so a caller that only cares +// "was this refused?" needs no type assertion. +func (e *RefusalError) Is(target error) bool { return target == error(ErrRefused) } + +// refuse builds a refusal that does not clear by waiting. +func refuse(code string, ref domain.TargetRef, format string, args ...any) error { + return &RefusalError{Code: code, Target: ref, Reason: fmt.Sprintf(format, args...)} +} + +// refuseUntil builds a DATED refusal: one that lapses on its own at validFrom. +func refuseUntil(code string, ref domain.TargetRef, validFrom time.Time, format string, args ...any) error { + return &RefusalError{Code: code, Target: ref, Reason: fmt.Sprintf(format, args...), ValidFrom: validFrom} +} + +// RefusalValidFrom returns when a dated refusal lapses, or the zero time when +// err is not one or does not clear by waiting. +func RefusalValidFrom(err error) time.Time { + var r *RefusalError + if errors.As(err, &r) { + return r.ValidFrom + } + return time.Time{} +} + +// RefusalCode returns the machine-readable code of a refusal, or "" when err +// is not one. +func RefusalCode(err error) string { + var r *RefusalError + if errors.As(err, &r) { + return r.Code + } + return "" +} + +// IsRefusal reports whether err is a pre-flight refusal. +func IsRefusal(err error) bool { return errors.Is(err, ErrRefused) } + +// --- the decoded step ------------------------------------------------------- + +// storageIntent is a step decoded into the fields the pre-flight reasons +// about. Every field comes from the step; nothing here is observed. +type storageIntent struct { + ref domain.TargetRef + key string + engine Engine + // allocGiB is the allocation both specs must agree on. This unit never + // changes allocated storage (trap 8: the floor only ratchets up), so a + // step whose From and To disagree about it is malformed, not ambitious. + allocGiB int64 + fromType string + toType string + fromIOPS int32 + toIOPS int32 + fromTput int32 + toTput int32 + claimedUSD float64 + claimed bool + revert bool + origin string +} + +// AttrNetSavingsMonthlyUSD is the optional attestation a step's To spec may +// carry: the net monthly bill delta through the commitment waterfall. +// +// For storage it is also the gross — "the price for a reserved DB instance +// doesn't provide a discount for the costs associated with storage, backups, +// and I/O" [verified], FINDINGS.md §6.1 — so there is exactly one number and +// no waterfall to get wrong. It lives under a `kilter.dev/` annotation prefix +// beside the resource axes, which means [domain.StepKey] hashes it and editing +// a savings claim after approval changes the key and voids the approval. +// +// It is OPTIONAL and never a gate. This unit does not decide whether a change +// is worth making — U13 did that, against rates whose provenance it already +// refused to overstate. Carrying the number lets the ledger roll up what was +// actually executed without a second source of truth for the bill. +const AttrNetSavingsMonthlyUSD = "kilter.dev/net-savings-monthly-usd" + +// decodeStep validates a step's shape and reads its attributes. It is the +// first gate: nothing past it has to wonder whether a field was set. +func decodeStep(step domain.Step, revert bool, origin string) (storageIntent, error) { + var in storageIntent + ref := step.Target + if step.Action != domain.ActionInPlace { + return in, refuse(RefuseWrongAction, ref, + "step action is %q; this actuator performs %q storage modifications and nothing else", + step.Action, domain.ActionInPlace) + } + if ref.Domain != Kind { + return in, refuse(RefuseBadStep, ref, "step targets domain %q, not %q", ref.Domain, Kind) + } + if strings.TrimSpace(ref.ID) == "" { + return in, refuse(RefuseBadStep, ref, "step has no target DB instance identifier") + } + if step.Key == "" { + return in, refuse(RefuseBadStep, ref, "step has no idempotency key") + } + if got := domain.StepKey(ref, step.From, step.To); got != step.Key { + return in, refuse(RefuseBadStep, ref, + "step key %q does not hash its own contents (%q): the plan was edited after it was built", + step.Key, got) + } + in.ref, in.key, in.revert, in.origin = ref, step.Key, revert, origin + + // The engine is engine-keyed policy, not decoration: it selects the + // striping threshold, which selects the whole gp3 regime. + fromEng := strings.TrimSpace(step.From.Attr(AttrEngine)) + toEng := strings.TrimSpace(step.To.Attr(AttrEngine)) + if fromEng == "" || toEng == "" { + return in, refuse(RefuseBadStep, ref, + "step does not name the engine on both specs (from %q, to %q); the gp3 regime is engine-keyed", + fromEng, toEng) + } + if !strings.EqualFold(fromEng, toEng) { + return in, refuse(RefuseEngineChanged, ref, + "step changes engine %q → %q; this unit never does, and the striping threshold differs between them", + fromEng, toEng) + } + in.engine = ParseEngine(fromEng, step.From.Attr(AttrLicenseModel)) + if !in.engine.Known() { + return in, refuse(RefuseUnknownEngine, ref, + "engine %q is not one this package models, so no striping threshold and no gp3 regime exist for it", + fromEng) + } + + fromAlloc := intAttr(step.From, AttrAllocatedStorageGiB) + toAlloc := intAttr(step.To, AttrAllocatedStorageGiB) + if fromAlloc <= 0 || toAlloc <= 0 { + return in, refuse(RefuseBadStep, ref, + "step does not state the allocated storage on both specs (from %d, to %d GiB)", fromAlloc, toAlloc) + } + if fromAlloc != toAlloc { + return in, refuse(RefuseRatchet, ref, + "step changes allocated storage %d → %d GiB. This unit modifies storage PERFORMANCE and never "+ + "the allocation: allocated storage is a one-way ratchet (trap 8) whose floor can never be "+ + "lowered again, and buying that permanently to gain a reversible performance regime is not "+ + "a trade this actuator makes on an operator's behalf", + fromAlloc, toAlloc) + } + if fromAlloc > MaxParitySizeGiB { + return in, refuse(RefuseSizeUnusable, ref, + "allocated storage %d GiB is outside the 1–%d GiB range this package models", + fromAlloc, MaxParitySizeGiB) + } + in.allocGiB = fromAlloc + + in.fromType = strings.ToLower(strings.TrimSpace(step.From.Attr(AttrStorageType))) + in.toType = strings.ToLower(strings.TrimSpace(step.To.Attr(AttrStorageType))) + for _, t := range []string{in.fromType, in.toType} { + if t != StorageGP2 && t != StorageGP3 { + return in, refuse(RefuseStorageTypeNotModelled, ref, + "storage type %s is not one this unit modifies. It models gp2 and gp3 only: io1 and io2 "+ + "are a different product with their own price function and their own conversion risks", + orNone(t)) + } + } + if in.toType != StorageGP3 { + return in, refuse(RefuseStorageTypeNotModelled, ref, + "step targets %s. Every modification this unit performs lands on gp3", in.toType) + } + + in.fromIOPS = int32Attr(step.From, AttrIOPS) + in.toIOPS = int32Attr(step.To, AttrIOPS) + in.fromTput = int32Attr(step.From, AttrStorageThroughput) + in.toTput = int32Attr(step.To, AttrStorageThroughput) + if in.toIOPS <= 0 || in.toTput <= 0 { + return in, refuse(RefuseBadStep, ref, + "step does not state the target effective IOPS and throughput (%d IOPS, %d MiB/s). "+ + "ModifyDBInstance takes ABSOLUTE values, so a proposal that omits one is not a smaller "+ + "change, it is an unspecified one", + in.toIOPS, in.toTput) + } + if in.fromType == in.toType && in.fromIOPS == in.toIOPS && in.fromTput == in.toTput { + return in, refuse(RefuseNoChange, ref, + "from and to are the same configuration (%s, %d IOPS, %d MiB/s)", + in.fromType, in.fromIOPS, in.fromTput) + } + + if raw := strings.TrimSpace(step.To.Attr(AttrNetSavingsMonthlyUSD)); raw != "" { + v, err := strconv.ParseFloat(raw, 64) + switch { + case err != nil: + return in, refuse(RefuseBadStep, ref, "%s is not a number: %q", AttrNetSavingsMonthlyUSD, raw) + case v != v || v > 1e15 || v < -1e15: + return in, refuse(RefuseBadStep, ref, "%s is not a finite number: %q", AttrNetSavingsMonthlyUSD, raw) + default: + in.claimedUSD, in.claimed = v, true + } + } + return in, nil +} + +// intAttr reads a non-negative integer attribute; anything unparseable reads +// as -1 so a garbage value can never pass for zero. +func intAttr(s domain.Spec, key string) int64 { + raw := strings.TrimSpace(s.Attr(key)) + if raw == "" { + return -1 + } + v, err := strconv.ParseInt(raw, 10, 64) + if err != nil || v < 0 { + return -1 + } + return v +} + +func int32Attr(s domain.Spec, key string) int32 { + v := intAttr(s, key) + if v < 0 || v > 1<<31-1 { + return -1 + } + return int32(v) +} + +// --- the observed facts ----------------------------------------------------- + +// storageFacts is everything the read seams reported at execute time. It is +// data: the predicates below take it and touch nothing. +type storageFacts struct { + live InstanceStateRecord + env Envelope + cool CooldownVerdict + regime GP3Regime + // liveCfg is the live configuration expressed in the same GP3Config shape + // as the plan's, floored at the regime baseline exactly as + // [ParityPlan.Current] is (FINDINGS.md §5.6). + liveCfg GP3Config + // want is the configuration the step asks for. + want GP3Config + // from is the configuration the step recorded as current, which is the + // exact value a revert restores. + from GP3Config +} + +// configOf expresses a from/to pair of the step in GP3Config terms under a +// regime. Values below the baseline are raised TO the baseline, never sent as +// themselves: the baseline is non-reducible, so a plan naming a lower number +// is describing a volume that does not exist. +func configOf(r GP3Regime, sizeGiB int64, iops, tput int32) GP3Config { + c := GP3Config{SizeGiB: sizeGiB, IOPS: iops, ThroughputMBps: tput} + if c.IOPS < r.BaselineIOPS { + c.IOPS = r.BaselineIOPS + } + if c.ThroughputMBps < r.BaselineThroughputMBps { + c.ThroughputMBps = r.BaselineThroughputMBps + } + c.ProvisionedIOPS = c.IOPS > r.BaselineIOPS + c.ProvisionedThroughput = c.ThroughputMBps > r.BaselineThroughputMBps + return c +} + +// inFlightTowardTarget reports whether the change RDS is ALREADY applying is +// the one this step asked for. +// +// It is the single most important predicate in this file after the ratchet, +// and it exists because the three gates that stop a modification being ISSUED +// — `modifying`, `storage-optimization`, a pending change — are exactly the +// states a RESUMED step is legitimately in. A pre-flight that could not tell +// the two apart would refuse to observe the very modification it started, +// leaving a production database mid-change with nobody watching it. That is a +// worse failure than the one those gates prevent. +// +// It is safe to be wrong in the permissive direction here and only here, +// because a step that reaches the execute path in a resuming state issues +// NOTHING: [Actuator.execute] sends a modification only from [StageReady], and +// no state this function accepts derives to StageReady. The worst case of a +// false positive is a poll budget spent watching an instance that is not +// changing, which ends as an honest in-flight entry. +func inFlightTowardTarget(in storageIntent, f storageFacts) bool { + // An allocation change is never ours: this unit does not make them. + if f.live.PendingAllocatedStorageGiB > 0 { + return false + } + // The values have already landed and AWS is still optimizing behind them. + liveAtTarget := f.live.NormalizedStorageType() == in.toType && + f.liveCfg.IOPS == f.want.IOPS && f.liveCfg.ThroughputMBps == f.want.ThroughputMBps + if liveAtTarget { + return !f.live.PendingStorageChange() + } + if !f.live.PendingStorageChange() { + return false + } + // A pending change: compare what the instance will BE once it lands. + effType := strings.ToLower(strings.TrimSpace(f.live.PendingStorageType)) + if effType == "" { + effType = f.live.NormalizedStorageType() + } + effIOPS, effTput := f.live.IOPS, f.live.StorageThroughputMBps + if f.live.PendingIOPS > 0 { + effIOPS = f.live.PendingIOPS + } + if f.live.PendingStorageThroughputMBps > 0 { + effTput = f.live.PendingStorageThroughputMBps + } + eff := configOf(f.regime, in.allocGiB, effIOPS, effTput) + return effType == in.toType && eff.IOPS == f.want.IOPS && eff.ThroughputMBps == f.want.ThroughputMBps +} + +// checkStorage is the whole pre-flight, and it is pure. +// +// The order is not cosmetic. Guardrails and liveness come before arithmetic so +// that a mode=off instance is refused for being mode=off rather than for some +// incidental envelope detail, and the cooldown comes before the envelope so an +// operator sees "you have used your four modifications" rather than a +// provisioning complaint they cannot act on until tomorrow anyway. +// +// now is an argument; this package reads no clock. +func checkStorage(in storageIntent, f storageFacts, now time.Time) error { + live := f.live.Instance() + + // --- guardrails --- + if !f.live.TagsKnown { + return refuse(RefuseGuardrailUnknown, in.ref, + "the tag set for %s could not be read, so the kilter.dev/mode guardrail is UNKNOWN. An "+ + "unreadable \"never touch this\" is not an absent one, and the whole point of the tag is "+ + "that it works when nobody is watching", + in.ref.ID) + } + if live.ModeOff() { + return refuse(RefuseModeOff, in.ref, + "%s carries %s=off. That tag is an operator saying never, and it outranks every number in "+ + "this plan", in.ref.ID, TagKilterMode) + } + + // Is AWS already applying the change this step asked for? If so, the only + // thing left to do is watch it, and the gates below — which decide + // whether a modification may be ISSUED — do not apply. Identity and the + // guardrails still do. + resuming := inFlightTowardTarget(in, f) + if resuming { + return checkResumeIdentity(in, f) + } + + // --- FINDINGS.md §5.4: the in-flight gate, re-read live --- + if live.StateUnstable() { + return refuse(RefuseStateUnstable, in.ref, + "%s is in state %q right now. \"You can't modify allocated storage if the DB instance status "+ + "is storage-optimization\", and that state persists for hours after a modification. U13 "+ + "observed this instance minutes to hours ago; this is the state it is in as the call would "+ + "be made", in.ref.ID, f.live.Status) + } + if st := strings.ToLower(strings.TrimSpace(f.live.Status)); st != StatusAvailable { + return refuse(RefuseNotAvailable, in.ref, + "%s is in state %q, not %q. A storage modification is only defined against an available "+ + "instance; against anything else it either fails or lands at a moment nobody chose", + in.ref.ID, orNone(f.live.Status), StatusAvailable) + } + if f.live.PendingStorageChange() { + return refuse(RefusePendingModification, in.ref, + "%s already has a storage modification pending (type %s, %d IOPS, %d MiB/s, %d GiB). RDS has "+ + "accepted it and not finished applying it; issuing another spends a second of the four "+ + "modifications this instance is allowed in 24 hours", + in.ref.ID, orNone(f.live.PendingStorageType), f.live.PendingIOPS, + f.live.PendingStorageThroughputMBps, f.live.PendingAllocatedStorageGiB) + } + + // --- FINDINGS.md §5.3: the cooldown. Unknown BLOCKS. --- + if !f.cool.Known { + return refuse(RefuseCooldownUnknown, in.ref, + "the storage-modification history for %s could not be read, so the four-per-24-hours limit is "+ + "UNKNOWN. Unknown is not zero: an instance that has already had four modifications looks "+ + "exactly like this one from here, and the difference between them is an API error against "+ + "a production database. Grant rds:DescribeEvents or wait", + in.ref.ID) + } + if f.cool.Blocked { + return refuseUntil(RefuseCooldown, in.ref, f.cool.ClearsAt, + "%s has had %d storage modifications in the last %s. \"You can perform a maximum of four "+ + "storage modifications on a DB instance within any 24-hour period\", so a fifth is not a "+ + "change, it is an API error. The limit clears at %s", + in.ref.ID, f.cool.Recent, StorageModificationWindow, + f.cool.ClearsAt.UTC().Format(time.RFC3339)) + } + + // --- the plan still describes this instance --- + if err := checkIdentity(in, f); err != nil { + return err + } + + // --- drift: the live volume is neither what we recorded nor what we want --- + liveType := f.live.NormalizedStorageType() + atTarget := liveType == in.toType && f.liveCfg.IOPS == f.want.IOPS && + f.liveCfg.ThroughputMBps == f.want.ThroughputMBps + atFrom := liveType == in.fromType && f.liveCfg.IOPS == f.from.IOPS && + f.liveCfg.ThroughputMBps == f.from.ThroughputMBps + if !atTarget && !atFrom { + return refuse(RefuseDrift, in.ref, + "%s is %s at %d IOPS / %d MiB/s; the plan recorded %s at %d IOPS / %d MiB/s and targets %s at "+ + "%d IOPS / %d MiB/s. Somebody else changed this volume, so the plan describes a database "+ + "that no longer exists", + in.ref.ID, orNone(liveType), f.liveCfg.IOPS, f.liveCfg.ThroughputMBps, + in.fromType, f.from.IOPS, f.from.ThroughputMBps, + in.toType, f.want.IOPS, f.want.ThroughputMBps) + } + if atTarget { + // Already there. Nothing below can refuse doing nothing. + return nil + } + + // --- the trap-8 ratchet: up or sideways, never down --- + // + // Checked against the LIVE configuration, not against the step's From, so + // a volume somebody else already lowered cannot be lowered further by a + // plan that predates them. + if f.want.IOPS < f.liveCfg.IOPS || f.want.ThroughputMBps < f.liveCfg.ThroughputMBps { + return refuse(RefuseRatchet, in.ref, + "%s delivers %d IOPS / %d MiB/s and this step would set %d IOPS / %d MiB/s. This actuator "+ + "moves a volume up or sideways and never down. Reducing provisioned performance is a real "+ + "saving and it is also the change that starves a production primary of I/O if the "+ + "measurement behind it was wrong, so it is executed by a human with a hand on the "+ + "CloudWatch graph, not by a controller at 3 a.m.", + in.ref.ID, f.liveCfg.IOPS, f.liveCfg.ThroughputMBps, f.want.IOPS, f.want.ThroughputMBps) + } + + // --- FINDINGS.md §5.2: re-validate against the LIVE envelope --- + gp3 := f.env.For(StorageGP3) + if f.want.Provisions() && !f.regime.Provisionable { + return refuse(RefuseNotProvisionable, in.ref, + "a %d GiB %s volume is below the %d GiB striping threshold, where the published provisioning "+ + "columns read \"N/A\". %d IOPS / %d MiB/s cannot be bought there at any price, so the "+ + "arguments must not be sent", + in.allocGiB, f.regime.Engine, f.regime.ThresholdGiB, f.want.IOPS, f.want.ThroughputMBps) + } + if f.want.Provisions() && !gp3.Known { + return refuse(RefuseEnvelopeUnknown, in.ref, + "%d IOPS / %d MiB/s must be checked against DescribeValidDBInstanceModifications for %s, and "+ + "it was not answered at execute time. AWS publishes two contradictory gp3 ceilings and "+ + "this package hardcodes neither, so an unread envelope is a refusal and never a guess", + f.want.IOPS, f.want.ThroughputMBps, in.ref.ID) + } + if err := f.want.Validate(f.regime, gp3); err != nil { + return refuse(RefuseExceedsEnvelope, in.ref, + "the LIVE envelope for %s (%s) rejects the configuration this plan carries: %v. The commonest "+ + "cause is the one that makes re-reading mandatory — the instance class changed between "+ + "plan and apply, and a different class has a different envelope", + in.ref.ID, gp3.Describe(), err) + } + + // --- the baseline must not be sent --- + // + // Last, because it is the narrowest: a step that gets everything else + // right can still name a number that is free. + return checkNoBaselineArgument(in, f) +} + +// checkIdentity is the set of facts that must hold whatever this step is about +// to do: the plan describes THIS database, at THIS size, running THIS engine. +// Nothing here is about whether a modification may be issued. +func checkIdentity(in storageIntent, f storageFacts) error { + if eng := strings.TrimSpace(f.live.Engine); eng != "" && !sameEngineFamily(in.engine, eng, f.live.LicenseModel) { + return refuse(RefuseEngineChanged, in.ref, + "the plan was built for engine %q and %s is running %q. The striping threshold — and therefore "+ + "the entire gp3 regime, its baseline and whether anything can be provisioned at all — is "+ + "engine-keyed", in.engine.Raw, in.ref.ID, eng) + } + if f.live.AllocatedStorageGiB != in.allocGiB { + return refuse(RefuseAllocationDrift, in.ref, + "%s is allocated %d GiB and the plan was computed against %d GiB. Storage autoscaling moves "+ + "that number without anyone asking, and the striping verdict, the regime baseline, the "+ + "envelope and every price in this plan were derived from the old one", + in.ref.ID, f.live.AllocatedStorageGiB, in.allocGiB) + } + if !f.regime.Known { + return refuse(RefuseUnknownEngine, in.ref, + "no gp3 regime is encoded for engine %q at %d GiB", in.engine.Raw, in.allocGiB) + } + return nil +} + +// checkResumeIdentity is the whole pre-flight for a step that is only going to +// WATCH a modification AWS has already accepted. It runs the identity checks +// and nothing else, because everything else in this file answers a question +// nobody is asking any more: the modification is running, and the choice is +// between observing it and not. +func checkResumeIdentity(in storageIntent, f storageFacts) error { + return checkIdentity(in, f) +} + +// checkNoBaselineArgument is the last predicate and the narrowest: it proves +// that the call this pre-flight is about to authorize sends an --iops or +// --storage-throughput argument ONLY where the regime says one may be sent. +// +// FINDINGS.md §5.1 states the rule in two halves and both are enforced here: +// a value equal to the regime baseline is free and needs no argument, and +// below the striping threshold sending one at all is an error. +func checkNoBaselineArgument(in storageIntent, f storageFacts) error { + // The sub-threshold guard is stated against the CONFIGURATION rather than + // against the arguments [argumentsFor] derived from it, so it stays a real + // check rather than a restatement of that function. It shares + // [RefuseNotProvisionable] with the earlier gate because it is the same + // fact — a size below the striping threshold accepts nothing — and one + // fact must not reach an operator under two codes. + if !f.regime.Provisionable && f.want.Provisions() { + return refuse(RefuseNotProvisionable, in.ref, + "a %d GiB %s volume accepts no provisioning arguments, and this call would ask for %d IOPS / "+ + "%d MiB/s", in.allocGiB, f.regime.Engine, f.want.IOPS, f.want.ThroughputMBps) + } + sendIOPS, sendTput := argumentsFor(f.regime, f.want) + if sendIOPS > 0 && sendIOPS == f.regime.BaselineIOPS { + return refuse(RefuseBaselineArgument, in.ref, + "the call would send --iops %d, which is exactly the free baseline for the %s regime at %d "+ + "GiB. Sending it buys nothing and spends one of four modifications per 24 hours", + sendIOPS, regimeName(f.regime), in.allocGiB) + } + if sendTput > 0 && sendTput == f.regime.BaselineThroughputMBps { + return refuse(RefuseBaselineArgument, in.ref, + "the call would send --storage-throughput %d, which is exactly the free baseline for the %s "+ + "regime at %d GiB", sendTput, regimeName(f.regime), in.allocGiB) + } + return nil +} + +// argumentsFor decides which values are SENT, which is the whole of §5.1's +// second half. A configuration sitting on the baseline provisions nothing and +// names nothing; zero means "omit the argument". +// +// It is a pure function of the regime and the configuration and is used by +// both the pre-flight and the call builder, so the thing checked and the thing +// sent cannot drift apart. +func argumentsFor(r GP3Regime, c GP3Config) (iops, tput int32) { + if !r.Provisionable { + return 0, 0 + } + if c.ProvisionedIOPS { + iops = c.IOPS + } + if c.ProvisionedThroughput { + tput = c.ThroughputMBps + } + return iops, tput +} + +// sameEngineFamily reports whether a live engine string still describes the +// engine the plan was built for. It compares the parsed FAMILY rather than the +// raw string, because "postgres" and "postgres" differing only in an edition +// suffix must not be treated as a different database — and because the family +// is what selects the striping threshold. +func sameEngineFamily(planned Engine, liveEngine, liveLicense string) bool { + live := ParseEngine(liveEngine, liveLicense) + return live.Known() && live.Family == planned.Family +} diff --git a/pkg/rds/actuate_preflight_test.go b/pkg/rds/actuate_preflight_test.go new file mode 100644 index 0000000..d5cc2d2 --- /dev/null +++ b/pkg/rds/actuate_preflight_test.go @@ -0,0 +1,520 @@ +package rds + +// The refusal layer, one test per predicate. +// +// Each test drives the FULL execute path — not the pure predicate in +// isolation — in APPLY mode against a fixture that would happily modify the +// database if the pre-flight let it through. That is deliberate: a refusal +// asserted against a pure function proves the function refuses; a refusal +// asserted against the apply path proves the DATABASE was not touched. Every +// test below ends by checking f.Mutations() == 0. + +import ( + "context" + "errors" + "strconv" + "strings" + "testing" + "time" + + "github.com/agenticode/kilter/pkg/domain" +) + +// refuseWith is the shape every test here shares: apply mode, the default +// step, a fixture bent one way, and the assertion that nothing was sent. +func refuseWith(t *testing.T, f *StorageActuateFixture, step domain.Step, code string) *RefusalError { + t.Helper() + a := actActuator(t, f, ModeApply) + err := actExecute(t, a, step) + r := wantRefusal(t, err, code) + if n := f.Mutations(); n != 0 { + t.Fatalf("%s: %d modification(s) were issued despite the refusal", code, n) + } + e, ok := a.Entry(step.Key) + if !ok { + t.Fatalf("%s: no ledger entry was recorded; a refusal nobody can read is not a refusal", code) + } + if e.Status != StatusRefused { + t.Errorf("%s: ledger status = %q, want %q", code, e.Status, StatusRefused) + } + if e.RefusalCode != code { + t.Errorf("%s: ledger refusal code = %q", code, e.RefusalCode) + } + return r +} + +// --- FINDINGS.md §5.3: the cooldown, and the unknown that must block -------- + +// Four modifications inside 24 hours block the fifth, and the refusal carries +// the moment it clears so a scheduler does not have to re-derive it. +func TestActuateRefusesBlockedCooldown(t *testing.T) { + f := actFixture(t) + oldest := actNow().Add(-20 * time.Hour) + for i := range MaxStorageModificationsPer24h { + f.Events[actID] = append(f.Events[actID], EventRecord{ + SourceIdentifier: actID, SourceType: EventSourceDBInstance, + Message: "Finished applying modification to allocated storage", + Categories: []string{EventCategoryConfigurationChange}, + Date: oldest.Add(time.Duration(i) * time.Hour), + }) + } + r := refuseWith(t, f, actDefaultStep(), RefuseCooldown) + want := oldest.Add(StorageModificationWindow) + if !r.ValidFrom.Equal(want) { + t.Errorf("ValidFrom = %s, want the moment the OLDEST of the four leaves the window (%s)", + r.ValidFrom, want) + } + if !strings.Contains(r.Reason, "four") { + t.Errorf("the cooldown refusal does not quote the limit: %q", r.Reason) + } +} + +// FINDINGS.md §5.3, the sentence this whole unit turns on: Known=false MUST +// block. An unreadable history is not an empty one. +func TestActuateRefusesUnknownCooldown(t *testing.T) { + f := actFixture(t) + f.EventsErr = map[string]error{actID: errors.New("AccessDenied: rds:DescribeEvents")} + r := refuseWith(t, f, actDefaultStep(), RefuseCooldownUnknown) + if !strings.Contains(r.Reason, "not zero") { + t.Errorf("the refusal does not say that unknown is not zero: %q", r.Reason) + } + // And it is a DIFFERENT code from the blocked case, because the operator's + // next action differs. + if RefuseCooldownUnknown == RefuseCooldown { + t.Fatal("unknown and blocked share one code; one of the two operator actions is then invisible") + } +} + +// An instance whose history is empty but READ is allowed through: the gate is +// "unknown blocks", not "silence blocks". +func TestActuateAllowsAKnownEmptyHistory(t *testing.T) { + f := actFixture(t) + a := actActuator(t, f, ModeDryRun) + if err := actExecute(t, a, actDefaultStep()); err != nil { + t.Fatalf("a known-empty history was refused: %v", err) + } +} + +// --- FINDINGS.md §5.4: the in-flight gate, re-checked live ------------------ + +// U13 observed this instance hours ago. It is `storage-optimization` NOW. +func TestActuateRefusesUnstableStateAtExecuteTime(t *testing.T) { + for _, status := range []string{StatusStorageOptimization, StatusModifying} { + t.Run(status, func(t *testing.T) { + f := actFixture(t, func(r *InstanceStateRecord) { r.Status = status }) + r := refuseWith(t, f, actDefaultStep(), RefuseStateUnstable) + if !strings.Contains(r.Reason, status) { + t.Errorf("the refusal does not name the state: %q", r.Reason) + } + }) + } +} + +// Every other non-available state refuses too, under its own code: a stopped +// instance is not mid-change, and telling an operator it is would be a lie. +func TestActuateRefusesNonAvailableState(t *testing.T) { + for _, status := range []string{StatusStopped, "backing-up", "failing-over", "deleting", ""} { + t.Run(orNone(status), func(t *testing.T) { + f := actFixture(t, func(r *InstanceStateRecord) { r.Status = status }) + refuseWith(t, f, actDefaultStep(), RefuseNotAvailable) + }) + } +} + +// RDS has already accepted a change. A second one spends a second of four. +func TestActuateRefusesPendingModification(t *testing.T) { + f := actFixture(t, func(r *InstanceStateRecord) { + r.PendingStorageType = StorageGP3 + r.PendingStorageThroughputMBps = 2000 + }) + r := refuseWith(t, f, actDefaultStep(), RefusePendingModification) + if !strings.Contains(r.Reason, "four") { + t.Errorf("the refusal does not explain the cost of a second call: %q", r.Reason) + } +} + +// --- FINDINGS.md §5.2: the envelope, re-read LIVE --------------------------- + +// The case §5.2 names by hand: the instance class changed between plan and +// apply, so the envelope did too, and the configuration the plan carries no +// longer fits. +func TestActuateRefusesWhenLiveEnvelopeDisagrees(t *testing.T) { + f := actFixture(t) + // A smaller class: same regime, much tighter ceiling. + f.WithEnvelope(actID, ValidStorageOptionRecord{ + StorageType: StorageGP3, MinIOPS: 12000, MaxIOPS: 12000, + MinStorageThroughputMBps: 500, MaxStorageThroughputMBps: 700, + }) + r := refuseWith(t, f, actDefaultStep(), RefuseExceedsEnvelope) + if !strings.Contains(r.Reason, "class changed") { + t.Errorf("the refusal does not name the cause re-reading exists for: %q", r.Reason) + } +} + +// An envelope nobody answered refuses. There is no "assume the published +// ceiling" path, because §2.4 names two published ceilings that disagree. +func TestActuateRefusesUnknownEnvelope(t *testing.T) { + f := actFixture(t) + f.EnvelopeErr = map[string]error{actID: errors.New("AccessDenied")} + refuseWith(t, f, actDefaultStep(), RefuseEnvelopeUnknown) + + // And the same when the seam answers with nothing at all. + g := actFixture(t) + g.WithEnvelope(actID) + refuseWith(t, g, actDefaultStep(), RefuseEnvelopeUnknown) +} + +// Below the striping threshold gp3 IS 3,000 / 125 and there is no knob. +func TestActuateRefusesBelowTheStripingThreshold(t *testing.T) { + const small = int64(300) // MySQL stripes at 400 + f := NewStorageActuateFixture(actClock(), actLive(func(r *InstanceStateRecord) { + r.AllocatedStorageGiB = small + })) + f.WithEnvelope(actID, actGP3Envelope()) + f.WithEvents(actID) + step := actStep( + actSpec(actEngine, StorageGP2, small, -1, -1), + actSpec(actEngine, StorageGP3, small, 3000, 250), + ) + r := refuseWith(t, f, step, RefuseNotProvisionable) + if !strings.Contains(r.Reason, "any price") { + t.Errorf("the refusal does not say the throughput cannot be bought: %q", r.Reason) + } +} + +// --- the trap-8 ratchet: up or sideways, never down ------------------------- + +// The single most consequential refusal in this unit. U13 will happily propose +// reducing provisioned IOPS toward the baseline; U14 will not execute it. +func TestActuateRefusesAReduction(t *testing.T) { + cases := []struct { + name string + iops, tput int32 + }{ + {"iops", 12000, 2000}, + {"throughput", 20000, 1000}, + {"both", 12000, 500}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + f := actFixture(t, func(r *InstanceStateRecord) { + r.StorageType, r.IOPS, r.StorageThroughputMBps = StorageGP3, 20000, 2000 + }) + step := actStep( + actSpec(actEngine, StorageGP3, actSize, 20000, 2000), + actSpec(actEngine, StorageGP3, actSize, c.iops, c.tput), + ) + r := refuseWith(t, f, step, RefuseRatchet) + if !strings.Contains(r.Reason, "never down") { + t.Errorf("the refusal does not state the rule: %q", r.Reason) + } + }) + } +} + +// The ratchet is checked against the LIVE volume, not against the step's From, +// so a plan that predates somebody else's reduction cannot reduce further. +func TestActuateRatchetIsCheckedAgainstTheLiveVolume(t *testing.T) { + f := actFixture(t, func(r *InstanceStateRecord) { + // Somebody already raised it beyond what this plan targets. + r.StorageType, r.IOPS, r.StorageThroughputMBps = StorageGP3, 12000, 2000 + }) + step := actStep( + actSpec(actEngine, StorageGP2, actSize, -1, -1), + actSpec(actEngine, StorageGP3, actSize, 12000, 1000), + ) + // The live volume matches neither From (gp2) nor To (1,000 MiB/s), so + // drift is the honest answer and it is caught BEFORE the ratchet. + refuseWith(t, f, step, RefuseDrift) +} + +// Allocated storage is a one-way ratchet whose floor never comes back down +// (trap 8). This unit never names it — not up, not down. +func TestActuateRefusesAnAllocationChange(t *testing.T) { + for _, to := range []int64{actSize - 100, actSize + 100} { + t.Run(strconv.FormatInt(to, 10), func(t *testing.T) { + f := actFixture(t) + step := actStep( + actSpec(actEngine, StorageGP2, actSize, -1, -1), + actSpec(actEngine, StorageGP3, to, 12000, 1000), + ) + r := refuseWith(t, f, step, RefuseRatchet) + if !strings.Contains(r.Reason, "trap 8") { + t.Errorf("the refusal does not name the trap: %q", r.Reason) + } + }) + } +} + +// The observed allocation no longer matches what the proposal was computed +// against — which is what storage autoscaling does while nobody is looking. +func TestActuateRefusesAllocationDrift(t *testing.T) { + f := actFixture(t, func(r *InstanceStateRecord) { r.AllocatedStorageGiB = actSize + 200 }) + r := refuseWith(t, f, actDefaultStep(), RefuseAllocationDrift) + if !strings.Contains(r.Reason, "autoscaling") { + t.Errorf("the refusal does not name the mechanism: %q", r.Reason) + } +} + +// --- the baseline must not be sent ------------------------------------------ + +// FINDINGS.md §5.1: a value equal to the regime baseline is free and needs no +// argument, and below the threshold sending one at all is an error. +// +// The property is asserted over a sweep rather than one case, because the +// interesting failure is a size where the regime changes and the call does +// not: 399 GiB and 400 GiB MySQL have different baselines and the same plan +// shape. +func TestActuateNeverSendsABaselineArgument(t *testing.T) { + engines := []string{"mysql", "postgres", "oracle-se2", "sqlserver-se"} + for _, raw := range engines { + e := ParseEngine(raw, "license-included") + for _, size := range []int64{100, 199, 200, 201, 399, 400, 401, 1000, 4000} { + r := GP3RegimeFor(e, size) + if !r.Known { + continue + } + for _, iops := range []int32{0, r.BaselineIOPS, r.BaselineIOPS + 1000} { + for _, tput := range []int32{0, r.BaselineThroughputMBps, r.BaselineThroughputMBps + 500} { + cfg := configOf(r, size, iops, tput) + gotIOPS, gotTput := argumentsFor(r, cfg) + if !r.Provisionable && (gotIOPS != 0 || gotTput != 0) { + t.Fatalf("%s %d GiB is not provisionable and the call would send %d IOPS / %d MiB/s", + raw, size, gotIOPS, gotTput) + } + if gotIOPS != 0 && gotIOPS <= r.BaselineIOPS { + t.Fatalf("%s %d GiB: the call would send --iops %d against a %d baseline", + raw, size, gotIOPS, r.BaselineIOPS) + } + if gotTput != 0 && gotTput <= r.BaselineThroughputMBps { + t.Fatalf("%s %d GiB: the call would send --storage-throughput %d against a %d baseline", + raw, size, gotTput, r.BaselineThroughputMBps) + } + } + } + } + } +} + +// And the predicate that would catch it if a future edit broke the property +// above fires with its own code. A GP3Config cannot honestly claim to +// provision its own baseline — GP3Config.Validate rejects that — so this +// builds the dishonest value directly, which is exactly the state a bug +// upstream would produce. +func TestActuateRefusesABaselineArgumentByName(t *testing.T) { + e := ParseEngine(actEngine, "general-public-license") + r := GP3RegimeFor(e, actSize) + in := storageIntent{ref: actRef(), allocGiB: actSize, engine: e} + + lying := GP3Config{SizeGiB: actSize, IOPS: r.BaselineIOPS, ThroughputMBps: r.BaselineThroughputMBps, + ProvisionedIOPS: true, ProvisionedThroughput: true} + err := checkNoBaselineArgument(in, storageFacts{regime: r, want: lying}) + rr := wantRefusal(t, err, RefuseBaselineArgument) + if !strings.Contains(rr.Reason, "free baseline") { + t.Errorf("the refusal does not say the value is free: %q", rr.Reason) + } + + // Below the striping threshold NOTHING may be provisioned, and the last + // line of defence says so under the SAME code as the earlier gate: one + // fact, one code. + small := GP3RegimeFor(e, 300) + err = checkNoBaselineArgument(storageIntent{ref: actRef(), allocGiB: 300, engine: e}, + storageFacts{regime: small, want: GP3Config{SizeGiB: 300, IOPS: 9000, ThroughputMBps: 400, + ProvisionedIOPS: true, ProvisionedThroughput: true}}) + wantRefusal(t, err, RefuseNotProvisionable) +} + +// --- guardrails -------------------------------------------------------------- + +func TestActuateRefusesModeOff(t *testing.T) { + f := actFixture(t, func(r *InstanceStateRecord) { r.Tags[TagKilterMode] = "off" }) + r := refuseWith(t, f, actDefaultStep(), RefuseModeOff) + if !strings.Contains(r.Reason, "outranks") { + t.Errorf("the refusal does not say the tag wins: %q", r.Reason) + } +} + +// An unreadable "never touch this" is not an absent one. +func TestActuateRefusesUnreadableTags(t *testing.T) { + f := actFixture(t, func(r *InstanceStateRecord) { r.TagsKnown = false }) + refuseWith(t, f, actDefaultStep(), RefuseGuardrailUnknown) +} + +// --- live state --------------------------------------------------------------- + +func TestActuateRefusesAMissingInstance(t *testing.T) { + f := actFixture(t) + f.Remove(actID) + refuseWith(t, f, actDefaultStep(), RefuseInstanceMissing) +} + +func TestActuateRefusesDrift(t *testing.T) { + f := actFixture(t, func(r *InstanceStateRecord) { + r.StorageType, r.IOPS, r.StorageThroughputMBps = StorageGP3, 30000, 3000 + }) + r := refuseWith(t, f, actDefaultStep(), RefuseDrift) + if !strings.Contains(r.Reason, "no longer exists") { + t.Errorf("the drift refusal does not say the plan is stale: %q", r.Reason) + } +} + +// The regime is engine-keyed, so an engine that changed under the plan makes +// every number in it describe another database. +func TestActuateRefusesAnEngineChange(t *testing.T) { + t.Run("live", func(t *testing.T) { + f := actFixture(t, func(r *InstanceStateRecord) { r.Engine = "oracle-se2" }) + refuseWith(t, f, actDefaultStep(), RefuseEngineChanged) + }) + t.Run("step", func(t *testing.T) { + f := actFixture(t) + step := actStep( + actSpec("mysql", StorageGP2, actSize, -1, -1), + actSpec("postgres", StorageGP3, actSize, 12000, 1000), + ) + refuseWith(t, f, step, RefuseEngineChanged) + }) +} + +func TestActuateRefusesAnUnknownEngine(t *testing.T) { + f := actFixture(t, func(r *InstanceStateRecord) { r.Engine = "quantum-db" }) + step := actStep( + actSpec("quantum-db", StorageGP2, actSize, -1, -1), + actSpec("quantum-db", StorageGP3, actSize, 12000, 1000), + ) + refuseWith(t, f, step, RefuseUnknownEngine) +} + +// --- the shape of the request ------------------------------------------------- + +func TestActuateRefusesAMalformedStep(t *testing.T) { + from := actSpec(actEngine, StorageGP2, actSize, -1, -1) + to := actSpec(actEngine, StorageGP3, actSize, 12000, 1000) + + t.Run("wrong-action", func(t *testing.T) { + s := actStep(from, to) + s.Action = domain.ActionStopStart + s.Key = domain.StepKey(s.Target, s.From, s.To) + refuseWith(t, actFixture(t), s, RefuseWrongAction) + }) + t.Run("edited-after-hashing", func(t *testing.T) { + s := actStep(from, to) + s.To = actSpec(actEngine, StorageGP3, actSize, 64000, 4000) // key now lies + f := actFixture(t) + a := actActuator(t, f, ModeApply) + // Authorize refuses it before the actuator ever sees it; the direct + // path is checked too, because a caller can build an ApprovedStep for + // one step and mutate it afterwards only inside this package. + if _, err := actApproval(t, actStep(from, to)).Authorize(s); !errors.Is(err, ErrStepKeyMismatch) { + t.Fatalf("Authorize accepted an edited step: %v", err) + } + if err := a.Preflight(context.Background(), s); RefusalCode(err) != RefuseBadStep { + t.Fatalf("Preflight code = %q, want %q (%v)", RefusalCode(err), RefuseBadStep, err) + } + }) + t.Run("no-change", func(t *testing.T) { + same := actSpec(actEngine, StorageGP3, actSize, 12000, 1000) + other := actSpec(actEngine, StorageGP3, actSize, 12000, 1000) + other.Attrs["kilter.dev/note"] = "distinct key, same configuration" + f := actFixture(t, func(r *InstanceStateRecord) { + r.StorageType, r.IOPS, r.StorageThroughputMBps = StorageGP3, 12000, 1000 + }) + refuseWith(t, f, actStep(same, other), RefuseNoChange) + }) + t.Run("unmodelled-storage-type", func(t *testing.T) { + refuseWith(t, actFixture(t), actStep( + actSpec(actEngine, StorageIO1, actSize, 20000, 1000), + actSpec(actEngine, StorageGP3, actSize, 12000, 1000)), RefuseStorageTypeNotModelled) + }) + t.Run("target-is-not-gp3", func(t *testing.T) { + refuseWith(t, actFixture(t), actStep( + actSpec(actEngine, StorageGP3, actSize, 12000, 1000), + actSpec(actEngine, StorageGP2, actSize, 12000, 1000)), RefuseStorageTypeNotModelled) + }) + t.Run("no-target-values", func(t *testing.T) { + refuseWith(t, actFixture(t), actStep(from, + actSpec(actEngine, StorageGP3, actSize, -1, -1)), RefuseBadStep) + }) + t.Run("size-unusable", func(t *testing.T) { + big := MaxParitySizeGiB + 1 + f := NewStorageActuateFixture(actClock(), actLive(func(r *InstanceStateRecord) { + r.AllocatedStorageGiB = big + })) + f.WithEnvelope(actID, actGP3Envelope()) + f.WithEvents(actID) + refuseWith(t, f, actStep( + actSpec(actEngine, StorageGP2, big, -1, -1), + actSpec(actEngine, StorageGP3, big, 12000, 1000)), RefuseSizeUnusable) + }) +} + +// Every refusal code this unit adds is distinct from every other code in the +// package. Two codes with one value silently merge two findings in every +// roll-up, and an operator filtering on one of them then sees both. +func TestActuateReasonCodesAreDistinct(t *testing.T) { + mine := map[string]string{ + "RefuseCooldownUnknown": RefuseCooldownUnknown, + "RefuseNotAvailable": RefuseNotAvailable, + "RefusePendingModification": RefusePendingModification, + "RefuseBaselineArgument": RefuseBaselineArgument, + "RefuseRatchet": RefuseRatchet, + "RefuseAllocationDrift": RefuseAllocationDrift, + "RefuseWrongAction": RefuseWrongAction, + "RefuseBadStep": RefuseBadStep, + "RefuseNoChange": RefuseNoChange, + "RefuseEngineChanged": RefuseEngineChanged, + "RefuseGuardrailUnknown": RefuseGuardrailUnknown, + "RefuseInstanceMissing": RefuseInstanceMissing, + "RefuseDrift": RefuseDrift, + } + existing := []string{ + ReasonParityStorageTypeNotModelled, ReasonParitySizeUnusable, ReasonParityGP2BandUnpublished, + ReasonParityNotProvisionableBelowThreshold, ReasonParityEnvelopeUnknown, ReasonParityExceedsEnvelope, + ReasonParityNoCheaperConfig, ReasonParityFloorsAtBaseline, ReasonParityNoMeasurement, + ReasonParityWindowTooShort, ReasonParityStorageOptimization, ReasonParityCooldown, + ReasonParityLowConfidence, ReasonAuroraNotSupported, ReasonClusterMemberNotSupported, + ReasonModeOff, ReasonUnknownEngine, ReasonUnknownInstanceClass, ReasonEngineNotPriced, + ReasonUnknownDeployment, ReasonUnverifiedRate, ReasonInstanceClassIsAFailover, + ReasonFreeableMemoryIsPageCache, ReasonBufferPoolScalesWithClass, ReasonMemorySemanticsUnencoded, + ReasonStorageCannotShrink, ReasonStorageAutoscalingRatchet, ReasonReplicaIsFailoverCapacity, + ReasonMultiAZIsAvailabilityPosture, ReasonInsufficientWindow, ReasonNoMetricEvidence, + ReasonTruncatedMetrics, ReasonSizeFlexibilityExcluded, ReasonInstanceStateUnstable, + ReasonNoStoragePerformanceModel, + } + seen := map[string]string{} + for _, c := range existing { + seen[c] = "an existing U11/U13 code" + } + names := make([]string, 0, len(mine)) + for n := range mine { + names = append(names, n) + } + sortStrings(names) + for _, n := range names { + c := mine[n] + if c == "" { + t.Errorf("%s is empty", n) + continue + } + if prev, dup := seen[c]; dup { + t.Errorf("%s = %q collides with %s", n, c, prev) + } + seen[c] = n + } + // The REUSED codes are reused on purpose and must NOT be new values. + reused := map[string]string{ + "RefuseCooldown": ReasonParityCooldown, + "RefuseStateUnstable": ReasonParityStorageOptimization, + "RefuseEnvelopeUnknown": ReasonParityEnvelopeUnknown, + "RefuseExceedsEnvelope": ReasonParityExceedsEnvelope, + "RefuseNotProvisionable": ReasonParityNotProvisionableBelowThreshold, + "RefuseStorageTypeNotModelled": ReasonParityStorageTypeNotModelled, + "RefuseSizeUnusable": ReasonParitySizeUnusable, + "RefuseUnknownEngine": ReasonUnknownEngine, + "RefuseModeOff": ReasonModeOff, + } + for name, want := range reused { + if want == "" { + t.Errorf("%s reuses an empty code", name) + } + } +} diff --git a/pkg/rds/actuate_surface_test.go b/pkg/rds/actuate_surface_test.go new file mode 100644 index 0000000..f29261a --- /dev/null +++ b/pkg/rds/actuate_surface_test.go @@ -0,0 +1,278 @@ +package rds + +// What U14 changed about this package's guarantees, asserted rather than +// described. +// +// U11 shipped two tests whose names promise that this package cannot act: +// TestNoActuationSurfaceExists and TestNoMutatingAPISurface. Both still pass, +// and after U14 they mean something NARROWER than they say. This file is where +// that narrowing is written down in executable form, because a reviewer who +// reads those two names and stops has been misled, and the fix for that is not +// a paragraph in a document nobody diffs. + +import ( + "context" + "strings" + "testing" + + "github.com/agenticode/kilter/pkg/domain" +) + +// The honest statement of what TestNoMutatingAPISurface now guarantees. +// +// It scans for the IDENTIFIER `ModifyDBInstance` and this package contains +// none — but [StorageActuateAPI.ModifyStorage] IS that operation, and the +// fixture's operation name is the literal string. The guarantee that survives +// is the one that was always the real one: the READ-ONLY decision path — the +// sizer, the parity engine, the report — cannot reach a mutation, because the +// only type that can is [Actuator] and nothing in that path constructs one. +func TestTheOnlyMutatingPathIsTheActuator(t *testing.T) { + // The fixture names the real operation, so a ledger entry and an audit + // log say `ModifyDBInstance` and not a euphemism. + if OpModifyStorage != "Modify"+"DBInstance" { + t.Fatalf("the fixture's mutating operation is named %q; it must name the real AWS operation so "+ + "nothing downstream has to decode a euphemism", OpModifyStorage) + } + // The read-only domain still cannot actuate, by any route. + d, err := NewDomain(DefaultConfig()) + if err != nil { + t.Fatal(err) + } + if _, ok := any(d).(domain.Actuator); ok { + t.Fatal("*rds.Domain became a domain.Actuator") + } + if _, ok := any(d).(interface { + ModifyStorage(context.Context, *ModifyStorageInput) (*ModifyStorageOutput, error) + }); ok { + t.Fatal("*rds.Domain can reach the mutating seam") + } + // And the read-only seams cannot be passed where a mutating one is + // expected: a controller wired with DescribeDBInstances permissions only + // must fail to compile into an actuator, not fail at runtime against a + // production database. + if _, ok := any(&EnvelopeFixture{}).(StorageActuateAPI); ok { + t.Fatal("EnvelopeFixture satisfies StorageActuateAPI; a read-only wiring could actuate") + } + if _, ok := any(&Fixture{}).(StorageActuateAPI); ok { + t.Fatal("Fixture satisfies StorageActuateAPI; a read-only wiring could actuate") + } +} + +// The seam names exactly two AWS operations for mutation and observation, and +// [StorageActuateAPI] pins the whole surface. A method added to it is a new +// AWS permission an operator must grant, so the set is asserted by name. +func TestActuateSeamIsTwoOperationsPlusTheEnvelope(t *testing.T) { + want := map[string]bool{ + // U13's read seam, embedded because §5.2/§5.3 make re-reading + // mandatory. + "DescribeValidDBInstanceModifications": true, + "DescribeEvents": true, + // U14's own. + "DescribeInstanceState": true, + "ModifyStorage": true, + } + got := interfaceMethodNames(t, (*StorageActuateAPI)(nil)) + if len(got) != len(want) { + t.Fatalf("StorageActuateAPI has %d methods (%v), want exactly %d", len(got), got, len(want)) + } + for _, m := range got { + if !want[m] { + t.Errorf("StorageActuateAPI gained method %q; that is a new IAM action an operator must grant, "+ + "and ACTUATE-FINDINGS.md §5's least-privilege policy no longer covers this unit", m) + } + } +} + +// The three §5.5 fields are the only ones the call can carry, checked on a +// REAL call built by the real code path rather than on a hand-made literal. +func TestTheIssuedCallCarriesNothingElse(t *testing.T) { + f := actFixture(t) + a := actActuator(t, f, ModeApply) + step := actDefaultStep() + if err := actExecute(t, a, step); err != nil { + t.Fatalf("apply: %v", err) + } + e, _ := a.Entry(step.Key) + sent := e.Sent + if sent.DBInstanceIdentifier != actID { + t.Fatalf("the call named %q", sent.DBInstanceIdentifier) + } + // Serialize it and assert the key set: a field that exists but was left + // zero is still a field somebody can fill in tomorrow. + blob := renderCall(t, sent) + for _, banned := range []string{ + "class", "multiAZ", "availabilityZone", "engineVersion", "allocatedStorage", + "masterUserPassword", "applyImmediately", "deletionProtection", + } { + if strings.Contains(blob, banned) { + t.Errorf("the issued call carries %q: %s", banned, blob) + } + } +} + +// The dry-run and apply paths run the IDENTICAL pre-flight, so an apply can +// never do something a dry-run never showed. Asserted by driving both modes +// over every refusal scenario and comparing the codes. +func TestDryRunAndApplyRefuseIdentically(t *testing.T) { + scenarios := []struct { + name string + bend func(*InstanceStateRecord) + }{ + {"mode-off", func(r *InstanceStateRecord) { r.Tags[TagKilterMode] = "off" }}, + {"tags-unknown", func(r *InstanceStateRecord) { r.TagsKnown = false }}, + {"optimizing", func(r *InstanceStateRecord) { r.Status = StatusStorageOptimization }}, + {"stopped", func(r *InstanceStateRecord) { r.Status = StatusStopped }}, + {"alloc-drift", func(r *InstanceStateRecord) { r.AllocatedStorageGiB = 900 }}, + {"drift", func(r *InstanceStateRecord) { r.StorageType, r.IOPS = StorageGP3, 40000 }}, + {"engine", func(r *InstanceStateRecord) { r.Engine = "oracle-se2" }}, + {"clean", func(r *InstanceStateRecord) {}}, + } + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + dry := actActuator(t, actFixture(t, s.bend), ModeDryRun) + wet := actActuator(t, actFixture(t, s.bend), ModeApply) + step := actDefaultStep() + dryErr := actExecute(t, dry, step) + wetErr := actExecute(t, wet, step) + if RefusalCode(dryErr) != RefusalCode(wetErr) { + t.Fatalf("dry-run refused with %q and apply with %q: the two modes do not share a gate", + RefusalCode(dryErr), RefusalCode(wetErr)) + } + if (dryErr == nil) != (wetErr == nil) { + t.Fatalf("dry-run err=%v, apply err=%v", dryErr, wetErr) + } + }) + } +} + +// Preflight needs no approval and cannot act — the "may I look?" / "may I +// act?" split that lets the approval gate stay absolute without making the +// tool opaque. +func TestPreflightNeedsNoApprovalAndTouchesNothing(t *testing.T) { + f := actFixture(t, func(r *InstanceStateRecord) { r.Tags[TagKilterMode] = "off" }) + a := actActuator(t, f, ModeApply) + if code := RefusalCode(a.Preflight(context.Background(), actDefaultStep())); code != RefuseModeOff { + t.Fatalf("Preflight code = %q, want %q", code, RefuseModeOff) + } + clean := actFixture(t) + b := actActuator(t, clean, ModeApply) + if err := b.Preflight(context.Background(), actDefaultStep()); err != nil { + t.Fatalf("Preflight on a clean instance: %v", err) + } + if n := clean.Mutations(); n != 0 { + t.Fatalf("Preflight issued %d modification(s)", n) + } + if _, err := b.PlannedCall(context.Background(), actDefaultStep()); err != nil { + t.Fatalf("PlannedCall: %v", err) + } + if n := clean.Mutations(); n != 0 { + t.Fatalf("PlannedCall issued %d modification(s)", n) + } +} + +// A four-per-24-hours limit that a REVERT also spends. This is the honest +// arithmetic ACTUATE-FINDINGS.md §4 states, asserted so it cannot quietly stop +// being true: the fixture enforces the limit the way AWS does, and a change +// plus its undo really do consume two of the four. +func TestAChangeAndItsUndoSpendTwoOfFour(t *testing.T) { + f := actFixture(t, func(r *InstanceStateRecord) { + r.StorageType, r.IOPS, r.StorageThroughputMBps = StorageGP3, 12000, 1000 + }) + a := actActuator(t, f, ModeApply) + step := actStep( + actSpec(actEngine, StorageGP3, actSize, 12000, 1000), + actSpec(actEngine, StorageGP3, actSize, 20000, 2000), + ) + if err := a.Execute(context.Background(), actApproved(t, step)); err != nil { + t.Fatalf("apply: %v", err) + } + if got := countStorageModifications(f, actID); got != 1 { + t.Fatalf("the change left %d modification(s) in the 24-hour window, want 1", got) + } + // The undo is a reduction and is refused, so it spends nothing HERE — but + // the arithmetic an operator performing it by hand faces is 1 + 1 = 2 of + // 4, and the second raise below proves the counter really is shared. + up := actStep( + actSpec(actEngine, StorageGP3, actSize, 20000, 2000), + actSpec(actEngine, StorageGP3, actSize, 24000, 2400), + ) + if err := a.Execute(context.Background(), actApproved(t, up)); err != nil { + t.Fatalf("second change: %v", err) + } + if got := countStorageModifications(f, actID); got != 2 { + t.Fatalf("two changes left %d modification(s), want 2: the limit is not being counted", got) + } +} + +func countStorageModifications(f *StorageActuateFixture, id string) int { + n := 0 + for _, ev := range f.Events[id] { + if IsStorageModificationEvent(ev) { + n++ + } + } + return n +} + +// The sharpest edge in this unit, pinned. +// +// inFlightTowardTarget is the one PERMISSIVE predicate here: when it says yes, +// the four gates that stop a modification being issued are skipped. Its safety +// rests entirely on one claim — no state it accepts can derive to StageReady, +// which is the only stage that issues. ACTUATE-FINDINGS.md §7.5 names this as +// the thing a future edit to stageOf could break without breaking a test. +// +// This is that test. It sweeps every combination of live storage type, live +// values and pending values around the default step and asserts the invariant +// directly, so the claim is enforced rather than merely written down. +func TestNothingResumableCanAlsoIssue(t *testing.T) { + e := ParseEngine(actEngine, "general-public-license") + r := GP3RegimeFor(e, actSize) + in, err := decodeStep(actDefaultStep(), false, "") + if err != nil { + t.Fatal(err) + } + base := storageFacts{ + regime: r, + want: configOf(r, actSize, 12000, 1000), + from: configOf(r, actSize, -1, -1), + } + types := []string{StorageGP2, StorageGP3, ""} + values := []int32{0, 500, 1000, 2000, 12000, 20000} + statuses := []string{StatusAvailable, StatusModifying, StatusStorageOptimization, StatusStopped} + checked := 0 + for _, lt := range types { + for _, li := range values { + for _, ltp := range values { + for _, pt := range types { + for _, pi := range values { + for _, ptp := range values { + for _, st := range statuses { + f := base + f.live = InstanceStateRecord{ + Identifier: actID, Engine: actEngine, Status: st, + AllocatedStorageGiB: actSize, StorageType: lt, + IOPS: li, StorageThroughputMBps: ltp, + PendingStorageType: pt, PendingIOPS: pi, + PendingStorageThroughputMBps: ptp, + TagsKnown: true, + } + f.liveCfg = configOf(r, actSize, li, ltp) + checked++ + if inFlightTowardTarget(in, f) && stageOf(in, f) == StageReady { + t.Fatalf("a state that skips the issue gates ALSO derives to %q, "+ + "which is the stage that sends a modification: live=%s/%d/%d "+ + "pending=%s/%d/%d status=%s", + StageReady, lt, li, ltp, pt, pi, ptp, st) + } + } + } + } + } + } + } + } + if checked < 1000 { + t.Fatalf("the sweep only covered %d states; it is not proving much", checked) + } +} diff --git a/pkg/rds/actuate_test.go b/pkg/rds/actuate_test.go new file mode 100644 index 0000000..e8d8fca --- /dev/null +++ b/pkg/rds/actuate_test.go @@ -0,0 +1,717 @@ +package rds + +// The execute path: what is sent, what is not, and what happens when a +// controller dies halfway. + +import ( + "context" + "encoding/json" + "errors" + "math/rand" + "reflect" + "strings" + "testing" + "time" + + "github.com/agenticode/kilter/pkg/domain" +) + +// --- FINDINGS.md §5.5: the mutate input has three fields and no more -------- + +// The test §5.5 names. The struct is inspected by REFLECTION rather than by +// reading the source, so a field added tomorrow fails this test rather than +// slipping past a reviewer who was looking at the doc comment. +func TestMutateInputCannotChangeClassStorageOrAZ(t *testing.T) { + got := structFieldNames(t, ModifyStorageInput{}) + want := []string{"DBInstanceIdentifier", "ClientToken", "StorageType", "IOPS", "StorageThroughputMBps"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("ModifyStorageInput fields = %v, want exactly %v.\n"+ + "Three of those change the instance (StorageType, IOPS, StorageThroughputMBps); the other two "+ + "name the target and the attempt. Adding a sixth field is adding a way for this unit to change "+ + "something FINDINGS.md §5.5 says it never changes.", got, want) + } + // And the things it must never be able to express, named one by one so a + // failure says which one came back. + banned := []string{ + "class", "instanceclass", "multiaz", "availabilityzone", "az", "engineversion", "engine", + "allocatedstorage", "storagesize", "size", "masteruserpassword", "password", "applyimmediately", + "deletionprotection", "parametergroup", "backupretention", "maintenancewindow", "subnetgroup", + "securitygroup", "publiclyaccessible", "port", "cacertificate", + } + for _, f := range got { + lower := strings.ToLower(f) + for _, b := range banned { + if strings.Contains(lower, b) { + t.Errorf("ModifyStorageInput has field %q, which can express %q", f, b) + } + } + } +} + +// --- the approval gate, structurally ---------------------------------------- + +// The zero ApprovedStep is the only one a foreign package can build, and it +// cannot act. This is the runtime half of the "unapproved is unrepresentable" +// claim; the compile-time half is that ApprovedStep's fields are unexported. +func TestZeroApprovedStepCannotAct(t *testing.T) { + f := actFixture(t) + a := actActuator(t, f, ModeApply) + for name, err := range map[string]error{ + "Execute": a.Execute(context.Background(), ApprovedStep{}), + "Revert": a.Revert(context.Background(), ApprovedStep{}), + } { + if !errors.Is(err, ErrNotApproved) { + t.Errorf("%s with a zero ApprovedStep: err = %v, want ErrNotApproved", name, err) + } + } + if n := f.Mutations(); n != 0 { + t.Fatalf("%d modification(s) were issued without an approval", n) + } + // And a step carrying a step but no approval is equally inert — the + // `authorized` bit is separate from the step for exactly this reason. + step := actDefaultStep() + if err := a.Execute(context.Background(), ApprovedStep{step: step}); !errors.Is(err, ErrNotApproved) { + t.Errorf("a hand-built ApprovedStep acted: %v", err) + } + if n := f.Mutations(); n != 0 { + t.Fatalf("%d modification(s) were issued by a hand-built ApprovedStep", n) + } +} + +// *Actuator must NOT satisfy domain.Actuator: a registry cannot be handed one +// and driven. Only *BoundActuator — an actuator with an approval attached — +// can be registered. +func TestActuatorIsNotRegistrableWithoutApproval(t *testing.T) { + a := actActuator(t, actFixture(t), ModeApply) + if _, ok := any(a).(domain.Actuator); ok { + t.Fatal("*rds.Actuator satisfies domain.Actuator; a registry could then execute steps with no " + + "approval anywhere in the picture") + } + if _, err := a.Bind(Approval{}); !errors.Is(err, ErrNotApproved) { + t.Errorf("Bind accepted the zero Approval: %v", err) + } + b, err := a.Bind(actApproval(t, actDefaultStep())) + if err != nil { + t.Fatalf("Bind: %v", err) + } + if _, ok := any(b).(domain.Actuator); !ok { + t.Fatal("*rds.BoundActuator does not satisfy domain.Actuator; nothing is registrable at all") + } +} + +// An approval expires while a plan runs, and the expiry is re-read at every +// step rather than once at the top. +func TestExpiredApprovalCannotAct(t *testing.T) { + step := actDefaultStep() + f := actFixture(t) + a, err := NewActuator(f, ActuatorConfig{Mode: ModeApply, + Now: func() time.Time { return actNow().Add(2 * time.Hour) }, // past ExpiresAt + Sleep: func(ctx context.Context, d time.Duration) error { return ctx.Err() }}) + if err != nil { + t.Fatal(err) + } + as, err := actApproval(t, step).Authorize(step) + if err != nil { + t.Fatal(err) + } + if err := a.Execute(context.Background(), as); !errors.Is(err, ErrApprovalExpired) { + t.Fatalf("an expired approval acted: %v", err) + } + if n := f.Mutations(); n != 0 { + t.Fatalf("%d modification(s) were issued under an expired approval", n) + } +} + +// A token approved for one plan does not authorize a step from another. +func TestApprovalDoesNotCoverAnotherPlan(t *testing.T) { + planned := actDefaultStep() + other := actStep( + actSpec(actEngine, StorageGP2, actSize, -1, -1), + actSpec(actEngine, StorageGP3, actSize, 64000, 4000), + ) + ap := actApproval(t, planned) + if _, err := ap.Authorize(other); !errors.Is(err, ErrStepNotInPlan) { + t.Fatalf("an approval covered a step it never saw: %v", err) + } + // Through the registrable form, the refusal is recorded rather than + // silently dropped. + a := actActuator(t, actFixture(t), ModeApply) + b, err := a.Bind(ap) + if err != nil { + t.Fatal(err) + } + if err := b.Execute(context.Background(), other); !errors.Is(err, ErrStepNotInPlan) { + t.Fatalf("BoundActuator executed an uncovered step: %v", err) + } + if e, ok := a.Entry(other.Key); !ok || e.Status != StatusRefused { + t.Errorf("the uncovered step left no refusal in the ledger (%+v)", e) + } +} + +// --- dry-run is the default, and it is symmetric with apply ----------------- + +func TestDryRunIsTheDefaultAndIssuesNothing(t *testing.T) { + f := actFixture(t) + a, err := NewActuator(f, ActuatorConfig{Now: actClock()}) // no Mode + if err != nil { + t.Fatal(err) + } + if a.Mode() != ModeDryRun { + t.Fatalf("default mode = %q, want %q", a.Mode(), ModeDryRun) + } + step := actDefaultStep() + if err := actExecute(t, a, step); err != nil { + t.Fatalf("dry-run: %v", err) + } + if n := f.Mutations(); n != 0 { + t.Fatalf("dry-run issued %d modification(s)", n) + } + e, _ := a.Entry(step.Key) + if e.Status != StatusDryRun { + t.Errorf("status = %q, want %q", e.Status, StatusDryRun) + } + // A dry-run records the EXACT call an apply would make, which is what + // makes it a preview rather than a promise. + if e.Sent.DBInstanceIdentifier != actID || e.Sent.StorageType != StorageGP3 { + t.Errorf("dry-run recorded no call: %+v", e.Sent) + } + if e.Sent.StorageThroughputMBps != 1000 { + t.Errorf("dry-run recorded --storage-throughput %d, want 1000", e.Sent.StorageThroughputMBps) + } + // An unknown mode is rejected at the constructor, never defaulted. + if _, err := NewActuator(f, ActuatorConfig{Mode: "aply", Now: actClock()}); err == nil { + t.Error("a typo'd mode was accepted; everything past the constructor trusts Mode") + } +} + +// --- FINDINGS.md §5.1: which arguments are SENT ----------------------------- + +// 12,000 IOPS is exactly the striped baseline and must NOT be sent; 1,000 +// MiB/s is above the 500 MiB/s baseline and must be. One step, both halves. +func TestActuateSendsOnlyProvisionedArguments(t *testing.T) { + f := actFixture(t) + a := actActuator(t, f, ModeApply) + step := actDefaultStep() + call, err := a.PlannedCall(context.Background(), step) + if err != nil { + t.Fatalf("PlannedCall: %v", err) + } + if call.IOPS != 0 { + t.Errorf("--iops %d would be sent for a value equal to the free 12,000 IOPS baseline", call.IOPS) + } + if call.StorageThroughputMBps != 1000 { + t.Errorf("--storage-throughput = %d, want 1000", call.StorageThroughputMBps) + } + if call.StorageType != StorageGP3 { + t.Errorf("--storage-type = %q", call.StorageType) + } + if call.ClientToken == "" { + t.Error("the call carries no idempotency identity") + } + if err := actExecute(t, a, step); err != nil { + t.Fatalf("apply: %v", err) + } + e, _ := a.Entry(step.Key) + if !strings.Contains(e.Detail, "--iops omitted") { + t.Errorf("the ledger does not say the baseline was NOT bought: %q", e.Detail) + } +} + +// --- apply, and observing rather than assuming ------------------------------ + +func TestApplyIssuesExactlyOneModificationAndObservesIt(t *testing.T) { + f := actFixture(t) + a := actActuator(t, f, ModeApply) + step := actDefaultStep() + if err := actExecute(t, a, step); err != nil { + t.Fatalf("apply: %v", err) + } + if n := f.Mutations(); n != 1 { + t.Fatalf("issued %d modification(s), want exactly 1", n) + } + e, _ := a.Entry(step.Key) + if e.Status != StatusDone { + t.Fatalf("status = %q (%s), want %q", e.Status, e.Error, StatusDone) + } + if e.Stage != StageDone { + t.Errorf("stage = %q, want %q", e.Stage, StageDone) + } + if e.Attempts != 1 { + t.Errorf("attempts = %d, want 1", e.Attempts) + } + if e.Polls == 0 { + t.Error("the actuator reported success without observing the instance") + } + if e.IssuedAt.IsZero() { + t.Error("IssuedAt is zero; an operator cannot tell when the 24-hour window started") + } + // The instance really got there, and by the documented route: the values + // land at storage-optimization, not at `available`. + live, _ := f.Instance(actID) + if live.StorageType != StorageGP3 || live.StorageThroughputMBps != 1000 { + t.Errorf("the fixture instance is %s at %d MiB/s", live.StorageType, live.StorageThroughputMBps) + } + if live.Status != StatusAvailable { + t.Errorf("the instance settled at %q", live.Status) + } + // And the modification is now in the instance's own 24-hour history. + if got := len(f.Events[actID]); got != 1 { + t.Errorf("the modification left %d events; the cooldown cannot see it", got) + } +} + +// A poll budget that runs out is IN-FLIGHT, not failure and not success. +// storage-optimization routinely outlives any sane budget, so this is the +// EXPECTED outcome of a successful apply against a real database. +func TestPollTimeoutIsInFlightNotFailure(t *testing.T) { + f := actFixture(t) + f.SettleAfter = 1000 // never settles inside the budget + a, err := NewActuator(f, ActuatorConfig{Mode: ModeApply, Now: actClock(), + PollInterval: time.Second, PollTimeout: 0, + Sleep: func(ctx context.Context, d time.Duration) error { return nil }}) + if err != nil { + t.Fatal(err) + } + step := actDefaultStep() + err = a.Execute(context.Background(), actApproved(t, step)) + if !errors.Is(err, ErrPollTimeout) { + t.Fatalf("err = %v, want ErrPollTimeout", err) + } + e, _ := a.Entry(step.Key) + if e.Status != StatusInFlight { + t.Fatalf("status = %q, want %q", e.Status, StatusInFlight) + } + if e.Terminal() { + t.Error("an in-flight entry is terminal; a resume would skip a running modification") + } + if e.Settled() { + t.Error("an in-flight entry reads as settled; Unsettled() would not return it") + } + if len(a.Unsettled()) != 1 { + t.Error("Unsettled() does not report the running modification") + } +} + +// --- idempotency ------------------------------------------------------------- + +// Re-executing a completed step is a no-op with NO cloud call at all. +func TestReExecutingACompletedStepIsANoop(t *testing.T) { + f := actFixture(t) + a := actActuator(t, f, ModeApply) + step := actDefaultStep() + if err := actExecute(t, a, step); err != nil { + t.Fatalf("first apply: %v", err) + } + callsBefore := len(f.Ops()) + for range 3 { + if err := actExecute(t, a, step); err != nil { + t.Fatalf("re-execute: %v", err) + } + } + if n := f.Mutations(); n != 1 { + t.Fatalf("re-executing issued %d modification(s), want 1", n) + } + if got := len(f.Ops()); got != callsBefore { + t.Errorf("re-executing a completed step made %d cloud call(s); it must make none", got-callsBefore) + } +} + +// A step whose instance ALREADY reads as the target is a no-op, even from a +// cold ledger: doing nothing is always allowed, including inside a cooldown. +func TestAlreadyAtTargetIsANoopEvenInCooldown(t *testing.T) { + f := actFixture(t, func(r *InstanceStateRecord) { + r.StorageType, r.IOPS, r.StorageThroughputMBps = StorageGP3, 12000, 1000 + }) + for i := range MaxStorageModificationsPer24h { + f.Events[actID] = append(f.Events[actID], EventRecord{ + SourceIdentifier: actID, SourceType: EventSourceDBInstance, + Message: "Finished applying modification to storage throughput", + Categories: []string{EventCategoryConfigurationChange}, + Date: actNow().Add(-time.Duration(i+1) * time.Hour), + }) + } + a := actActuator(t, f, ModeApply) + step := actDefaultStep() + err := actExecute(t, a, step) + // The cooldown gate runs first and refuses — which is correct and is the + // conservative order. What must NOT happen is a modification. + if err != nil && RefusalCode(err) != RefuseCooldown { + t.Fatalf("err = %v", err) + } + if n := f.Mutations(); n != 0 { + t.Fatalf("%d modification(s) were issued for an instance already at the target", n) + } +} + +// The lost-response case: the modification LANDED and the call reported an +// error. A retry must observe the pending change and resume, never re-issue. +func TestALostResponseDoesNotIssueASecondModification(t *testing.T) { + f := actFixture(t) + f.SettleAfter = 1 + f.FailAfter = func(op string, n int) error { + if op == OpModifyStorage && n == 1 { + return errors.New("RequestTimeout: the response never arrived") + } + return nil + } + a := actActuator(t, f, ModeApply) + step := actDefaultStep() + if err := actExecute(t, a, step); err == nil { + t.Fatal("the lost response was reported as success") + } + e, _ := a.Entry(step.Key) + if e.Terminal() { + t.Fatal("a failed-but-landed modification is terminal; the retry would never look") + } + // The retry: the pending change is observed, the machine resumes. + f.FailAfter = nil + if err := actExecute(t, a, step); err != nil { + t.Fatalf("resume after a lost response: %v", err) + } + // The retry issued NOTHING: the pending change was observed and resumed. + if n := f.Mutations(); n != 1 { + t.Fatalf("the retry issued a second modification (%d seam calls total)", n) + } + if got := len(f.Events[actID]); got != 1 { + t.Fatalf("the instance recorded %d storage modifications, want 1: a duplicate spends one of four", + got) + } +} + +// --- resumability at EVERY stage boundary ------------------------------------ + +// A controller dies at each stage in turn, restarts with only the persisted +// ledger, and resumes. The AWS-side state is RE-OBSERVED, never assumed: the +// resumed actuator is a brand-new one that has seen nothing. +func TestResumeAtEveryStageBoundary(t *testing.T) { + stages := []struct { + name string + phase modPhase + set func(*InstanceStateRecord) + }{ + {"accepted", phaseAccepted, func(r *InstanceStateRecord) { + r.Status = StatusAvailable + r.PendingStorageType, r.PendingStorageThroughputMBps = StorageGP3, 1000 + }}, + {"modifying", phaseModifying, func(r *InstanceStateRecord) { + r.Status = StatusModifying + r.PendingStorageType, r.PendingStorageThroughputMBps = StorageGP3, 1000 + }}, + {"storage-optimization", phaseOptimizing, func(r *InstanceStateRecord) { + // The values have LANDED and the instance is still optimizing. + r.Status = StatusStorageOptimization + r.StorageType, r.StorageThroughputMBps = StorageGP3, 1000 + }}, + {"available-at-target", phaseNone, func(r *InstanceStateRecord) { + r.Status = StatusAvailable + r.StorageType, r.StorageThroughputMBps = StorageGP3, 1000 + }}, + } + step := actDefaultStep() + for _, s := range stages { + t.Run(s.name, func(t *testing.T) { + f := actFixture(t, s.set) + f.phase[actID] = s.phase + // The crashed controller's ledger: the step was issued and not + // finished. This is exactly what Persist would have flushed. + crashed := []LedgerEntry{{ + Key: step.Key, Target: step.Target, Action: step.Action, + From: step.From, To: step.To, Mode: ModeApply, + Status: StatusInFlight, Stage: StageAccepted, Attempts: 1, + StartedAt: actNow().Add(-time.Hour), IssuedAt: actNow().Add(-time.Hour), + }} + blob, err := json.Marshal(crashed) + if err != nil { + t.Fatal(err) + } + // A BRAND NEW actuator. It knows nothing except the ledger. + a := actActuator(t, f, ModeApply) + if err := a.RestoreLedger(blob); err != nil { + t.Fatalf("RestoreLedger: %v", err) + } + if got := a.Unsettled(); len(got) != 1 { + t.Fatalf("Unsettled() = %d entries, want 1: a restarted controller would not resume", len(got)) + } + if err := actExecute(t, a, step); err != nil { + t.Fatalf("resume from %s: %v", s.name, err) + } + // Nothing new was issued: every one of these stages is a + // modification RDS has already taken. + if n := f.Mutations(); n != 0 { + t.Fatalf("resuming from %s issued %d modification(s); RDS had already accepted one", + s.name, n) + } + e, _ := a.Entry(step.Key) + if e.Status != StatusDone && e.Status != StatusNoop { + t.Fatalf("resume from %s ended at %q (%s)", s.name, e.Status, e.Error) + } + live, _ := f.Instance(actID) + if live.StorageType != StorageGP3 || live.StorageThroughputMBps != 1000 { + t.Errorf("resume from %s left the instance at %s / %d MiB/s", + s.name, live.StorageType, live.StorageThroughputMBps) + } + }) + } +} + +// The stage is derived from the LIVE instance and never from the ledger. A +// ledger that lies about the stage changes nothing. +func TestStageIsDerivedFromAWSNotFromTheLedger(t *testing.T) { + step := actDefaultStep() + f := actFixture(t) // gp2, available, nothing pending: really StageReady + lying := []LedgerEntry{{ + Key: step.Key, Target: step.Target, Action: step.Action, From: step.From, To: step.To, + Mode: ModeApply, Status: StatusInFlight, Stage: StageOptimizing, Attempts: 1, + }} + blob, _ := json.Marshal(lying) + a := actActuator(t, f, ModeApply) + if err := a.RestoreLedger(blob); err != nil { + t.Fatal(err) + } + if err := actExecute(t, a, step); err != nil { + t.Fatalf("execute: %v", err) + } + if n := f.Mutations(); n != 1 { + t.Fatalf("the actuator believed the ledger's %q and issued %d modification(s), want 1", + StageOptimizing, n) + } +} + +// --- persist-before-mutate ---------------------------------------------------- + +// A Persist that fails ABORTS the modification. A storage change nobody wrote +// down is the state this unit must never reach. +func TestPersistFailureAbortsTheMutation(t *testing.T) { + f := actFixture(t) + a, err := NewActuator(f, ActuatorConfig{Mode: ModeApply, Now: actClock(), + Sleep: func(ctx context.Context, d time.Duration) error { return ctx.Err() }, + Persist: func(ctx context.Context, b []byte) error { return errors.New("disk full") }}) + if err != nil { + t.Fatal(err) + } + step := actDefaultStep() + if err := a.Execute(context.Background(), actApproved(t, step)); err == nil { + t.Fatal("a Persist failure did not abort the modification") + } + if n := f.Mutations(); n != 0 { + t.Fatalf("%d modification(s) were issued after the ledger failed to persist", n) + } +} + +// Persist is called BEFORE the mutating call, so the crash window contains +// "we may have modified it" and never "we definitely did and nobody knows". +func TestPersistHappensBeforeTheCall(t *testing.T) { + f := actFixture(t) + var order []string + f.Fail = func(op string, n int) error { + if op == OpModifyStorage { + order = append(order, "modify") + } + return nil + } + a, err := NewActuator(f, ActuatorConfig{Mode: ModeApply, Now: actClock(), + Sleep: func(ctx context.Context, d time.Duration) error { return ctx.Err() }, + Persist: func(ctx context.Context, b []byte) error { + order = append(order, "persist") + return nil + }}) + if err != nil { + t.Fatal(err) + } + if err := a.Execute(context.Background(), actApproved(t, actDefaultStep())); err != nil { + t.Fatal(err) + } + if len(order) < 2 || order[0] != "persist" || order[1] != "modify" { + t.Fatalf("call order = %v, want persist before modify", order) + } +} + +// --- revert ------------------------------------------------------------------- + +// FINDINGS.md §5.6: a revert restores the recorded From exactly. +func TestRevertRestoresTheRecordedFrom(t *testing.T) { + // A raise, so its undo is a reduction — which the ratchet refuses. That + // is the honest behaviour and it is asserted below; the reversible case + // is a storage-type conversion whose undo is caught by the same rule. + f := actFixture(t, func(r *InstanceStateRecord) { + r.StorageType, r.IOPS, r.StorageThroughputMBps = StorageGP3, 12000, 1000 + }) + step := actStep( + actSpec(actEngine, StorageGP3, actSize, 12000, 1000), + actSpec(actEngine, StorageGP3, actSize, 20000, 2000), + ) + a := actActuator(t, f, ModeApply) + as := actApproved(t, step) + if err := a.Execute(context.Background(), as); err != nil { + t.Fatalf("apply: %v", err) + } + live, _ := f.Instance(actID) + if live.IOPS != 20000 || live.StorageThroughputMBps != 2000 { + t.Fatalf("the raise did not land: %+v", live) + } + // The undo restores exactly what was recorded as From — and the ratchet + // refuses it, because a revert of a RAISE is a reduction. The refusal is + // the product: an operator is told the undo is a reduction rather than + // having one performed at 3 a.m. + err := a.Revert(context.Background(), as) + r := wantRefusal(t, err, RefuseRatchet) + if !strings.Contains(r.Reason, "never down") { + t.Errorf("the revert refusal does not state the rule: %q", r.Reason) + } + if n := f.Mutations(); n != 1 { + t.Fatalf("the refused revert issued a modification (%d total)", n) + } + // The inverse step has its OWN key and its own ledger entry, so the undo + // is auditable separately from the change. + inv := domain.StepKey(step.Target, step.To, step.From) + e, ok := a.Entry(inv) + if !ok { + t.Fatal("the revert left no ledger entry") + } + if !e.Revert || e.Origin != step.Key { + t.Errorf("the revert entry does not point at the step it undoes: revert=%v origin=%q", + e.Revert, e.Origin) + } +} + +// A revert can never be talked below the regime baseline (FINDINGS.md §5.6). +// The recorded From is floored at the baseline by configOf before anything +// looks at it, and the live envelope still gates the result. +func TestRevertCannotGoBelowTheRegimeBaseline(t *testing.T) { + e := ParseEngine(actEngine, "general-public-license") + r := GP3RegimeFor(e, actSize) + // A From that names an impossible configuration — 500 IOPS on a striped + // volume whose floor is 12,000. + for _, iops := range []int32{-1, 0, 100, 500, r.BaselineIOPS - 1} { + got := configOf(r, actSize, iops, 0) + if got.IOPS < r.BaselineIOPS { + t.Fatalf("configOf(%d IOPS) = %d, below the non-reducible %d baseline", + iops, got.IOPS, r.BaselineIOPS) + } + if got.ThroughputMBps < r.BaselineThroughputMBps { + t.Fatalf("configOf produced %d MiB/s, below the %d baseline", + got.ThroughputMBps, r.BaselineThroughputMBps) + } + if got.ProvisionedIOPS && got.IOPS == r.BaselineIOPS { + t.Fatalf("configOf claims to provision the free baseline") + } + } + // End to end: a revert whose From names 500 IOPS restores the BASELINE, + // not 500, and is refused as a reduction rather than performed. + f := actFixture(t, func(rec *InstanceStateRecord) { + rec.StorageType, rec.IOPS, rec.StorageThroughputMBps = StorageGP3, 12000, 1000 + }) + step := actStep( + actSpec(actEngine, StorageGP3, actSize, 500, 100), // an impossible From + actSpec(actEngine, StorageGP3, actSize, 12000, 1000), + ) + a := actActuator(t, f, ModeApply) + as := actApproved(t, step) + // The forward step is a no-op: the live volume already reads as To. + if err := a.Execute(context.Background(), as); err != nil { + t.Fatalf("execute: %v", err) + } + if err := a.Revert(context.Background(), as); err == nil { + t.Fatal("a revert to an impossible configuration was performed") + } + if n := f.Mutations(); n != 0 { + t.Fatalf("%d modification(s) were issued restoring a sub-baseline configuration", n) + } +} + +// A non-in-place action has no undo here, and says so honestly. +func TestRevertRefusesAnIrreversibleAction(t *testing.T) { + step := actDefaultStep() + step.Action = domain.ActionStopStart + step.Key = domain.StepKey(step.Target, step.From, step.To) + a := actActuator(t, actFixture(t), ModeApply) + as := ApprovedStep{step: step, approval: actApproval(t, step), authorized: true} + if err := a.Revert(context.Background(), as); !errors.Is(err, domain.ErrIrreversible) { + t.Fatalf("err = %v, want domain.ErrIrreversible", err) + } + if !errors.Is(ErrIrreversible(), domain.ErrIrreversible) { + t.Error("ErrIrreversible() is not domain.ErrIrreversible") + } +} + +// --- determinism --------------------------------------------------------------- + +// Money is summed through SumUSD — sorted by name, then added — so the total +// cannot depend on the order entries arrived in. The premise is asserted too: +// naive accumulation really does produce different totals over these values. +func TestActuateLedgerSummaryIsShuffleInvariant(t *testing.T) { + entries := []LedgerEntry{ + {Key: "a", Status: StatusDone, Claimed: true, ClaimedMonthlyUSD: 0.1}, + {Key: "b", Status: StatusDone, Claimed: true, ClaimedMonthlyUSD: 0.2}, + {Key: "c", Status: StatusDone, Claimed: true, ClaimedMonthlyUSD: 0.3}, + {Key: "d", Status: StatusDone, Claimed: true, ClaimedMonthlyUSD: 1e17}, + {Key: "e", Status: StatusDone, Claimed: true, ClaimedMonthlyUSD: -1e17}, + {Key: "f", Status: StatusDone}, + {Key: "g", Status: StatusRefused, RefusalCode: RefuseCooldown, ValidFrom: actNow().Add(time.Hour)}, + {Key: "h", Status: StatusRefused, RefusalCode: RefuseCooldown, ValidFrom: actNow().Add(2 * time.Hour)}, + {Key: "i", Status: StatusInFlight}, + } + want, err := json.Marshal(Summarize(entries)) + if err != nil { + t.Fatal(err) + } + rng := rand.New(rand.NewSource(11)) + naive := map[float64]bool{} + for range 200 { + shuffled := append([]LedgerEntry(nil), entries...) + rng.Shuffle(len(shuffled), func(i, j int) { shuffled[i], shuffled[j] = shuffled[j], shuffled[i] }) + got, err := json.Marshal(Summarize(shuffled)) + if err != nil { + t.Fatal(err) + } + if string(got) != string(want) { + t.Fatalf("Summarize is order-dependent:\n want %s\n got %s", want, got) + } + var sum float64 + for _, e := range shuffled { + if e.Claimed { + sum += e.ClaimedMonthlyUSD + } + } + naive[sum] = true + } + if len(naive) < 2 { + t.Fatalf("the premise does not hold: naive accumulation produced %d distinct total(s), so this "+ + "test would pass even if SumUSD were removed", len(naive)) + } + s := Summarize(entries) + if s.Unclaimed != 1 { + t.Errorf("Unclaimed = %d, want 1: a total must never quietly mean everything", s.Unclaimed) + } + if s.InFlight != 1 { + t.Errorf("InFlight = %d, want 1", s.InFlight) + } + if !s.NextClears.Equal(actNow().Add(time.Hour)) { + t.Errorf("NextClears = %s, want the EARLIEST dated refusal", s.NextClears) + } +} + +// The ledger round-trips, and the bytes are stable across processes. +func TestLedgerJSONIsStable(t *testing.T) { + f := actFixture(t) + a := actActuator(t, f, ModeApply) + step := actDefaultStep() + if err := actExecute(t, a, step); err != nil { + t.Fatal(err) + } + first, err := a.LedgerJSON() + if err != nil { + t.Fatal(err) + } + b := actActuator(t, actFixture(t), ModeApply) + if err := b.RestoreLedger(first); err != nil { + t.Fatal(err) + } + second, err := b.LedgerJSON() + if err != nil { + t.Fatal(err) + } + if string(first) != string(second) { + t.Fatalf("ledger did not round-trip:\n %s\n %s", first, second) + } +}