From aa8e15dc8d9e43f012018223648edd99a36b03c2 Mon Sep 17 00:00:00 2001 From: agenticode <16611333+agenticode@users.noreply.github.com> Date: Wed, 26 Aug 2026 17:28:18 +0900 Subject: [PATCH] feat(cmd): collect RDS live and unlock the storage-parity seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --rds-region REGION drives the same pkg/rds collector through pkg/provider's SDK adapters. --rds-fixture is untouched and remains a sibling; TestTheLiveRDSCollectorIsReachableFromTheBinary asserts both sources produce a byte-identical report over the same account. --rds-parity fills rds.Config.Parity from DescribeValidDBInstanceModifications and DescribeEvents, on both sources. Without it four fixture instances refuse with no-storage-performance-model; with it that code is gone and 4 no-io-measurement, 2 provisioning-envelope-unknown and 1 gp2-band-unpublished take its place. The cmd-side holder returns a suppression, never ok=false, which would emit neither proposal nor refusal. Degraded is not broken: one retry gated on IsAccessDenied and a get-metric-data error, so a throttle stays fatal and a denied DescribeDBInstances stays fatal. An unreadable tag is not an absent tag — a denied ListTagsForResource warns naming the ARN and the key, and the guardrail refusal disappears rather than collapsing into 'looked and found none'. buildRuntime returns nil on error, so rdsFailure folds warnings recorded before a hard failure into the error. No actuator becomes reachable: TestNoActuatorBecomesReachableThroughTheLivePath asserts a live parity-enabled domain still yields actuatable=false, 0 steps, RefuseReportOnly. 19 test functions, zero credentials or sockets. go.mod and go.sum unchanged. Co-authored-by: kording <74226694+kording@users.noreply.github.com> --- cmd/RDSLIVE-FINDINGS.md | 455 +++++++++++++++++++++++++ cmd/kilter/domains.go | 108 +++--- cmd/kilter/rds.go | 100 ++++-- cmd/kilter/rdslive.go | 507 ++++++++++++++++++++++++++++ cmd/kilter/rdslive_test.go | 555 +++++++++++++++++++++++++++++++ cmd/kilter/rdsliveparity_test.go | 416 +++++++++++++++++++++++ 6 files changed, 2078 insertions(+), 63 deletions(-) create mode 100644 cmd/RDSLIVE-FINDINGS.md create mode 100644 cmd/kilter/rdslive.go create mode 100644 cmd/kilter/rdslive_test.go create mode 100644 cmd/kilter/rdsliveparity_test.go diff --git a/cmd/RDSLIVE-FINDINGS.md b/cmd/RDSLIVE-FINDINGS.md new file mode 100644 index 0000000..86f5f1d --- /dev/null +++ b/cmd/RDSLIVE-FINDINGS.md @@ -0,0 +1,455 @@ +# S3 — the live RDS collector and the storage-parity seam become reachable + +Two shipped, tested, read-only features could not be run from the binary. + +`cmd/WIRING-FINDINGS.md` **§6.1**: the live RDS collector was blocked on +`go.mod`, and that blocker is gone — `pkg/provider.NewRDSAPI` and +`NewCloudWatchAPI` landed with PR#45 and implement all four `pkg/rds` seams. +**Nothing called them.** `cmd/kilter/rds.go` could only be driven by +`--rds-fixture`. + +`cmd/WIRING-FINDINGS.md` **§6.4**, last bullet: `pkg/rds`'s `StorageParity` +seam was "still nil, as U11 shipped it — `--rds-fixture` cannot enable it and +no flag pretends to". `pkg/rds/parity_assess.go` implements it and is merged. +**Nothing constructed it.** + +Both now work: + +``` +kilter domains report --domain rds --rds-region us-east-1 --scope 000000000000/us-east-1 +kilter domains report --domain rds --rds-fixture account.json --rds-parity +``` + +**Status:** `gofmt -l ./cmd` empty, `go vet ./...`, `go build ./...`, +`go test -race -count=1 ./cmd/...` and `go test -race -short ./...` all green. +**`go.mod` and `go.sum` are unchanged** — every module this unit imports was +already required. No existing test was weakened, edited or deleted; the +committed `testdata/rds-account.json` is byte-identical and +`TestWriteRDSFixture` still passes. + +| | | +|---|---| +| New production code | `cmd/kilter/rdslive.go` (507 lines) + 145 added lines across `rds.go` and `domains.go` | +| New tests | 971 lines, **19 test functions**, zero AWS calls | + +``` +cmd/kilter/rdslive.go the live seams, the one retry, the parity holder, the rate loader +cmd/kilter/rds.go rdsFixtureFile grows the U13 seam; collectRDS splits +cmd/kilter/domains.go three flags, one constructor call, one new loop +cmd/kilter/rdslive_test.go the live path: degradation, denial, tags, notes, determinism +cmd/kilter/rdsliveparity_test.go the parity seam: off, on-without-envelope, on-with-envelope +``` + +--- + +## 1. The exact IAM policy a user now needs + +Two statements, because the required/optional split is the whole point of the +seam split. Grant the first and withhold the second and you get a **complete +report with fewer dimensions and a note on each**; withhold anything in the +first and you get either a loud failure or a report that quietly disobeys an +operator. + +```json +{ + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "KilterRDSRequired", + "Effect": "Allow", + "Action": [ + "rds:DescribeDBInstances", + "rds:DescribeDBClusters", + "rds:ListTagsForResource" + ], + "Resource": "*" + }, + { + "Sid": "KilterRDSOptionalEachBuysOneDimension", + "Effect": "Allow", + "Action": [ + "cloudwatch:GetMetricData", + "rds:DescribeReservedDBInstances", + "rds:DescribeValidDBInstanceModifications", + "rds:DescribeEvents" + ], + "Resource": "*" + } + ] +} +``` + +Seven actions, **all of them GETs**. This policy grants nothing that can change +an account: no `rds:Modify*`, no `rds:Reboot*`, no `rds:Create*`, no +`rds:Delete*`. `pkg/provider`'s `TestNoMutatingSDKSurface` walks both SDK +interfaces by reflection and fails on any mutating verb, and +`TestNoActuatorBecomesReachableThroughTheLivePath` asserts the same thing from +the CLI end: a live, parity-enabled `rds` domain still produces +`actuatable=false`, zero steps and `RefuseReportOnly`. + +### 1.1 Required — what each one is required *for* + +| Action | Why it is required | +|---|---| +| `rds:DescribeDBInstances` | The one **hard** dependency. With no inventory there is nothing to report on, and a report that silently covered fewer databases than the account holds is worse than no report. Denial fails the run, loudly, naming the action. | +| `rds:DescribeDBClusters` | Required-but-degrades. It is the only way to tell an **Aurora** cluster from a **Multi-AZ DB cluster** without inferring it from a member's engine string, and calling a MySQL Multi-AZ cluster "Aurora" would be a false statement in a report whose entire value is that its statements are true. Denial warns and falls back to the more cautious `cluster-member-not-supported`. | +| `rds:ListTagsForResource` | Required, and not as a nicety. The `kilter.dev/mode=off` opt-out lives in a tag. Without this action an operator who tagged a database to be left alone is **not obeyed**. See §2.2 — this is the degradation with the worst failure mode in the set. | + +### 1.2 Optional — what each one buys, and what you get without it + +| Action | Buys | Without it | +|---|---|---| +| `cloudwatch:GetMetricData` | Every measurement-derived finding: idle detection, memory verdicts, unused storage, and the four I/O series `--rds-parity` needs. **The only action here that costs money at scale** — `pkg/rds` issues `len(rds.CollectedMetrics()) == 11` queries per instance, batched 500 per request. Not resource-scopable; `"Resource": "*"` is the only form CloudWatch accepts. | A **complete inventory** in which every instance refuses with `no-metric-evidence`, plus a warning. No idle verdict is ever drawn from the silence. | +| `rds:DescribeReservedDBInstances` | Net-of-commitment savings. | Complete report, `net == gross`. That **under**-claims, and a warning says so. | +| `rds:DescribeValidDBInstanceModifications` | *(only read under `--rds-parity`)* The live provisioning envelope — the IOPS and throughput ranges AWS will actually accept for this instance. | Every provisioning proposal refused by name with `provisioning-envelope-unknown`. **No published ceiling is assumed**: §2.4 names two AWS ceilings that contradict each other and `pkg/rds` hardcodes neither. | +| `rds:DescribeEvents` | *(only read under `--rds-parity`)* The storage-modification history, i.e. the four-per-24-hours limit. | Per-instance warning; `HistoryKnown=false`, so the limit is reported **unverified rather than cleared**. | + +Both `--rds-parity` actions can be narrowed to +`arn:aws:rds:::db:*`, as can `rds:ListTagsForResource` — they +act on a single instance. Whether the `Describe*` **list** actions accept +resource-level ARNs varies by action and has changed over time +[unverified], so `"Resource": "*"` is what is published here. + +--- + +## 2. Every degradation path, and how it is reported + +The table `cmd/` had to read before deciding what to wire is **not symmetric**, +and getting it wrong is exactly how an optional permission becomes a failed +run. Every row below is tested from the CLI. + +| Failure | Result | Where it is said | Test | +|---|---|---|---| +| `DescribeDBInstances` denied/fails | **Hard failure**, non-zero exit | the error, naming the action | `TestAMissingRequiredPermissionFailsLoudly` | +| `DescribeDBClusters` denied | complete report; members excluded under `cluster-member-not-supported` | `snap.Warnings` → `rt.Warnings` | `TestOptionalPermissionsDegradeToAWarningAndNeverToAFailure` | +| `ListTagsForResource` denied | complete report; guardrail **unevaluated** for that instance | warning naming the **ARN** and `kilter.dev/mode` | `TestAnUnreadableTagIsNotAnAbsentTag` | +| `GetMetricData` **denied** | one retry with the seam dropped ⇒ complete report, every instance `no-metric-evidence` | warning naming `cloudwatch:GetMetricData` | `TestAFailingMetricsSeamIsNotTheSameAsAnAbsentOne` | +| `GetMetricData` throttled/timed out | **hard failure** — not a degradation | the error | `TestAThrottleIsNotMistakenForAMissingPermission` | +| `DescribeReservedDBInstances` denied | complete report; `net == gross` | warning saying it "under-claims" | `TestOptionalPermissionsDegradeToAWarningAndNeverToAFailure` | +| `DescribeValidDBInstanceModifications` absent/denied | complete report; `provisioning-envelope-unknown` per instance | `Envelopes.Warnings` → `rt.Warnings` | `TestParityWithoutTheEnvelopeSeamRefusesByName…` | +| `DescribeEvents` denied | complete report; `HistoryKnown=false` | warning: "unverified rather than cleared" | `TestADeniedDescribeEventsLeavesTheLimitUnverifiedRatherThanCleared` | +| SDK facts with no seam field | complete report | `RDSAPI.Notes()` / `CloudWatchAPI.Notes()`, rendered beside `snap.Warnings` | `TestTheAdaptersNotesAreRenderedBesideTheSnapshotWarnings` | +| window > CloudWatch retention | clamped by `pkg/rds` | warning naming the **observed** window | `TestTheLiveWindowIsClampedByPkgRDSAndTheClampIsSaidOutLoud` | + +### 2.1 The one degradation `cmd/` had to implement itself + +`RDS-ADAPTER-FINDINGS.md` §3 puts the asymmetry in bold and it is the entire +content of this unit's error handling: **a nil `MetricsAPI` degrades, a failing +one does not.** `readMetrics` returns `fmt.Errorf("rds: get metric data: %w", +err)` and `Collect` propagates it, so a credential that lacks +`cloudwatch:GetMetricData` and is wired anyway produces `AccessDeniedException` +and **no report at all** where the design promises the documented degraded one. + +So there is exactly one retry, and it is gated on two conditions, both +load-bearing: + +```go +func isMetricsAccessDenied(err error) bool { + return err != nil && provider.IsAccessDenied(err) && + strings.Contains(err.Error(), "get metric data") +} +``` + +- **`provider.IsAccessDenied`** matches only permission denials. A throttle or + a timeout stays an error, because swallowing it would turn a transient fault + into a permanently degraded report claiming the credential lacks a permission + it actually holds — and the operator would then go and grant a permission + that was never missing. +- **the message test** keeps the retry pointed at the metrics seam. Without it, + an `AccessDenied` on the **required** `DescribeDBInstances` would be caught by + the metrics branch and downgraded into "no CloudWatch", producing a + confident, complete-looking report over **zero databases**. + +Deleting either half makes a test fail. Replacing `err != nil && +isMetricsAccessDenied(err)` with `false` makes +`TestAFailingMetricsSeamIsNotTheSameAsAnAbsentOne` fail with the raw +`AccessDeniedException`, which is the behaviour it exists to prevent. + +Every other optional seam is wired **unconditionally**, because `pkg/rds` +already degrades it internally. Adding a second retry layer for +`DescribeReservedDBInstances` would be dead code guarding a call that cannot +fail the collection. + +### 2.2 "I could not look" is not "I looked and there was nothing" + +The single most dangerous class of bug in this codebase, and the reason +`TestAnUnreadableTagIsNotAnAbsentTag` asserts **two** things rather than one. + +`db-legacy` carries `kilter.dev/mode=off`. `provider.RDSAPI.ListTagsForResource` +returns the error rather than an empty `TagList`; `pkg/rds`'s `readTags` +returns `(nil, warning)` and the collector appends a warning naming the ARN and +the tag key. The wiring passes both through untouched. What it must never do — +and does not — is fold an `AccessDenied` into an empty tag map, because +`DBInstance.ModeOff()` reads a missing key as "not opted out". + +The test asserts the control (tag readable ⇒ `guardrail-mode-off` fires, no +warning) **and** the subject (tag denied ⇒ warning naming the ARN, and +`guardrail-mode-off` does **not** fire). The disappearance of the refusal is +what makes the warning load-bearing: assert only the warning and the test still +passes on a wiring that honours the tag anyway; assert only the refusal and the +test cannot tell "unreadable" from "absent". + +**A finding for `pkg/rds`, unfixed here and out of scope:** an unreadable tag +adds a warning but does **not** set `Snapshot.Stale`, and there is no refusal +code for it — so an instance whose guardrail could not be evaluated is +assessed, priced and reported exactly like one whose guardrail was read and +found empty. The warning is the only thing distinguishing them. A +`ReasonTagsUnreadable` exclusion — the same shape as `ReasonModeOff` — would +make "could not look" a *verdict* rather than a footnote, and would be the +conservative direction: excluding an instance can only ever under-claim. + +### 2.3 A failed collection still says what it learned + +`buildRuntime` returns `nil` on error, so anything appended to +`runtime.Warnings` on the way to a failure was discarded. That is how an +operator ends up staring at one `AccessDeniedException` with no idea that the +run had *already* fallen back from a denied `GetMetricData` before dying of +something else. `rdsFailure(err, warnings)` appends them to the error, which is +the only channel that survives an aborted build +(`TestAFailedCollectionSaysWhatItHadAlreadyLearned`). + +### 2.4 One region fails ⇒ the whole run fails + +`--rds-region` is repeatable and one collector runs per region. If any region +fails hard, the run fails. That is deliberate: continuing would produce a +report covering three regions out of four, with the same shape, the same field +names and the same confident tone as a complete one. The alternative — a +partial report plus a warning — is defensible, but a warning is not the right +weight for "a quarter of your fleet is missing from these totals". + +--- + +## 3. The parity seam: how it is enabled, and how its absence is surfaced + +### 3.1 Enabling it + +``` +--rds-parity read the modification envelope and run pkg/rds's parity engine +--rds-parity-rates PATH verified provisioned-IOPS / provisioned-throughput rates +``` + +`--rds-parity` works on **both** sources. On `--rds-region` the envelope seam is +the same `*provider.RDSAPI` as the inventory seam. On `--rds-fixture` it is +`rds.EnvelopeFixture`, driven from three new `omitempty` fields on +`rdsFixtureFile` — `storageOptions`, `events`, `noEnvelopeAPI` — so the seam is +fully exercisable without an AWS account. `TestParityIsReachableOnTheLivePathToo` +asserts the two sources produce a byte-identical report over the same account. + +The event window is **48 h**, hardcoded with the reason: the question the +events answer is "have there been four storage modifications in the last 24 +hours", and a window that cannot contain 24 hours cannot rule one out — +`EnvelopeCollector` warns if you hand it a shorter one, and +`TestTheStorageModificationCooldownIsReadFromRecordedEvents` asserts that +warning is absent. + +### 3.2 The ordering problem, and the holder that solves it + +`rds.Config.Parity` is fixed when the domain is **constructed**; the parity +engine needs the envelopes, which can only be read once the inventory names the +instances — which happens after the domain is registered. So the domain is +registered holding `*rdsParitySeam`, an indirection, and every collected source +folds its envelopes in through `observe()`. Envelopes accumulate across sources +(`rds.NewEnvelopes` sorts and de-duplicates by identifier) and the engine is +rebuilt, because one domain can be fed several fixtures and several live +regions while `AssessParity` is called once, at report time, after all of them. + +**The holder's nil branch returns a suppression, not `ok=false`.** This is the +subtlety worth recording. `StorageParity`'s third result means "I declined to +look at all", and the sizer reads it literally: `ok=false` emits **neither** a +proposal **nor** a suppression, and it does not fall back to the +`no-storage-performance-model` refusal either — that lives in the *else* branch +of `cfg.Parity != nil`. A holder that returned `ok=false` while unfilled would +therefore produce a report with a missing dimension and **no line saying so**, +which is precisely the failure this seam exists to prevent. + +### 3.3 How its absence is surfaced + +Three states, three different reports, and none of them is silence. + +| State | What the report says | +|---|---| +| **no `--rds-parity`** | `rds.Config.Parity` is nil, and `pkg/rds`'s sizer refuses every assessed instance's storage with **`no-storage-performance-model`** — 4 of the shipped fixture's 27 refusals. A report that did not assess parity says so on every line it would have assessed. | +| **`--rds-parity`, no envelope** | `no-storage-performance-model` is gone and **`provisioning-envelope-unknown`** takes its place, plus a warning naming `DescribeValidDBInstanceModifications`. No ceiling is assumed. | +| **`--rds-parity` + envelope** | Real verdicts: `no-io-measurement`, `gp2-band-unpublished`, `storage-parity-not-cheaper`, `provisioned-performance-floors-at-baseline`, `storage-modification-cooldown`, or a proposal. | + +`TestParityIsOptInAndItsAbsenceIsVisible` asserts all three transitions, +including that the flag is not decorative: with `--rds-parity` the +not-evaluated refusal must be **gone** and at least one parity-only code must +be **present**. Concretely, on the shipped account: + +``` + without --rds-parity with --rds-parity + no-storage-performance-model 4 refused — + no-io-measurement — 4 refused + provisioning-envelope-unknown — 2 refused + gp2-band-unpublished — 1 refused + provisioned-performance-floors — 1 refused +``` + +### 3.4 The money gate + +`pkg/rds` does the whole arithmetic either way — the **magnitude** is reported +whatever the rate says. What provenance decides is whether that magnitude may +be called a *saving*. `pkg/rds/FINDINGS.md` §7 could not retrieve the RDS gp3 +storage or provisioned-performance rates from AWS, so every shipped figure is +`unverified` and the proposal is refused under `unverified-rate` with the +dollar figure attached — which is a strictly better report than +`no-storage-performance-model`, because it sizes the opportunity and names what +would unblock it. + +Supplying `--rds-rates` (storage) **and** `--rds-parity-rates` (the two gp3 +knobs) stamps both `operator-supplied` and unblocks the claim. +`TestParityReachesAProposalOnlyWithClaimableRates` runs both halves and asserts +the proposal that appears is advisory, never shrinks allocated storage, never +proposes below the striped regime's 12,000 IOPS / 500 MiB/s floor, and never +leaks into the actuatable recommendation stream. + +**`--rds-parity-rates` gives the file no way to name its own provenance.** +`rds.LoadRates` stamps every loaded row `operator-supplied` and offers no +`provenance` key, for the reason that provenance is the single gate between +"this sizes an opportunity" and "this goes in a business case". `PerformanceRates` +*does* carry a `provenance` json tag, so decoding straight into it would let a +file promote a guess to a claim by typing the word `verified`. The cmd/-side +projection omits the field and `DisallowUnknownFields` rejects it +(`TestTheParityRatesFileCannotDeclareItsOwnProvenance`). + +--- + +## 4. No live AWS call in any test + +Nineteen new test functions. **Not one** reads a credential, opens `~/.aws`, +sets an `AWS_*` variable or touches a socket. Every live test installs a seam +set built from `pkg/rds`'s own `Fixture` and `EnvelopeFixture` through +`newRDSLiveSeams`, a package-level constructor seam restored by `t.Cleanup`, so +the collector, the pagination, the window clamp, the access-denied retry and +the envelope collection all run for real while the SDK is never constructed. + +`provider.IsAccessDenied` matches a `smithy.APIError` rather than a string, so +the denial fakes implement that interface (`smithy-go` is already a direct +`go.mod` requirement; no module was added). + +One test — `TestABlankLiveRegionIsRefusedBeforeAnyCredentialIsRead` — reaches +the **real** `dialRDS`, and it is deliberately the one case that provably +returns before `LoadDefaultConfig`: `NewRDSAPI` rejects a blank region first, +because `CollectorConfig.Region` stamps `DBInstance.Region` and therefore +selects the rate-card row that prices every instance. It clears the `AWS_*` +environment for the duration anyway. + +A test that would try to reach the network on a developer laptop with a stale +profile is not a slow test; it is a failed unit. + +--- + +## 5. What is still NOT reachable in `pkg/rds`, and why + +### 5.1 The actuator — deliberately, and this unit must not change that + +`pkg/rds/actuate*.go` is 2,800 lines of merged, tested code: +`StorageActuateAPI`, `NewActuator`, `ApprovedStep`, `NewApproval`, +`PlanFingerprint`, `BoundActuator`, `LedgerEntry`, `Summarize`, the preflight +and the refusal types. **None of it is reachable from the binary and none of it +was made reachable here.** This unit wires read-only paths; making an actuator +reachable is a separate, separately-approved decision. + +Three independent walls stand in front of it and this unit added to none of +them and removed from none: + +1. `Registry.PlanSteps` checks `Health` before the domain is consulted, and + `pkg/rds`'s `Health` is unconditionally report-only. +2. `validateSteps` rejects a step labelled with another domain's kind. +3. `Registry.Execute` routes through the actuator table, which has no `rds` row. + +There is also **no SDK adapter for the mutating side**. `pkg/provider`'s +`rdsSDK` interface is six GETs, and `TestNoMutatingSDKSurface` fails on any +method whose name begins with a mutating verb. So even a wiring mistake in +`cmd/` could not reach `rds:ModifyDBInstance` — there is nothing to reach. +`TestNoActuatorBecomesReachableThroughTheLivePath` asserts the CLI end of this +for a live, parity-enabled domain. + +### 5.2 `AttributeStorageGrowth` — needs persistence between runs, which `kilter domains` has not got + +Trap 8's ledger rule compares an instance's allocated storage against the +**last** snapshot's, because storage autoscaling moves the floor on its own and +leaves no CloudTrail event. `Target.PriorAllocatedStorageGiB` is filled by +`Domain.Observe` from the domain's remembered previous observation — but +`kilter domains` builds a fresh registry every invocation and never calls the +domain's `Checkpoint`/`Restore`. So `PriorAllocatedStorageGiB` is always 0, and +`AttributeStorageGrowth` always answers `StorageUnchanged`. + +**Next job:** persist the RDS domain's checkpoint through `pkg/store` between +runs, keyed by scope. That is the same missing write side `WIRING-FINDINGS.md` +§6.2 and §6.3 both need, so the three jobs share most of their work. Wiring it +from `cmd/` alone is not possible: nothing in `kilter domains` writes state +today, and adding a store to it would be a new persistence surface rather than +a wiring change. + +### 5.3 Collector knobs with no CLI surface + +`CollectorConfig.MaxPages` and `PeriodSeconds`, +`EnvelopeCollectorConfig.MaxPages`, and `RDSAPI.SetCallTimeout` / +`CloudWatchAPI.SetCallTimeout` all take package defaults and no flag exposes +them. Defensible — each has a documented default with a reason — but an account +large enough to hit `DefaultMaxPages = 50` gets a truncation warning and no way +to raise the bound without rebuilding. + +`rds.CollectedMetrics()` is exported "so an operator can size that bill without +reading the source", and nothing in the CLI prints it. One line in +`--rds-detail` would close that. + +### 5.4 `pkg/domain/rds.Config` has no `Parity` field + +Because of it, `newRDSDomain` builds the domain through `rds.NewDomain` +directly and re-wraps the result as `&domrds.Domain{Domain: d}` so +`UsageLines` still contributes to the account-wide commitment baseline. That +works and is tested, but it duplicates four lines of config mapping and would +silently skip any state a future `domrds.Domain` grew. + +**One-field fix, in `pkg/domain/rds` (outside this unit's scope):** add +`Parity rds.StorageParity` to `domrds.Config` and assign it in `New`. `cmd/` +then calls `domrds.New` on both paths and the wrapper reconstruction goes away. + +### 5.5 The `pkg/rds` decisions this wiring inherits and cannot fix + +Carried forward from `RDS-ADAPTER-FINDINGS.md` §7, restated because they are +now reachable from the binary and therefore now affect real reports: + +- **§7.1 `GetMetricData` continuation results are dropped, not merged.** + `collect.go`'s page loop is first-write-wins per query ID, so a query whose + data spans a page boundary keeps only the first page's datapoints — and, + because that page's `StatusCode` is `Complete`, the series is **not** marked + partial. At 11 queries per instance and a 100,800-datapoint response cap, + one response holds about five series, so this is reachable on any account + with more than a handful of databases. This is the highest-value `pkg/rds` + fix on the list and it is one branch: append on a duplicate ID. +- **§7.2 `StorageEnvelope` cannot distinguish "no ceiling" from "unknown + ceiling".** The adapter picks omission to keep the safe reading, so a + partially-answered envelope produces a refusal that says the seam "was not + read" when it *was* supplied. Misleading, in the conservative direction. A + `MaxIOPSKnown bool` would fix it. +- **§7.3 `Envelope.Cooldown` silently ignores undated events**, which inverts + the recogniser's own stated bias toward over-counting. +- **§7.4 an empty `StatusCode` defaults to `Complete`.** The adapter resolves + it to `PartialData` on its side; the default belongs on the other side, or + every future CloudWatch adapter re-derives the same fix. +- **§7.5 an unaddressable instance is dropped without a warning.** The adapter + covers it with a `Note()` — which this unit renders — but the drop itself is + still silent inside `pkg/rds`. + +### 5.6 Smaller gaps this unit chose not to close + +- **`--scope` is one string for every `--rds-region`.** Scope is + `accountID/region`, so a two-region run stamps both regions' targets with one + scope. Correct for the single-region case that is the norm; wrong-looking for + a multi-region run. Deriving the scope per region needs the account ID, which + means `sts:GetCallerIdentity` — a new IAM action, and not one this unit will + add to a policy it is publishing as seven GETs. +- **No `--rds-no-metrics`.** `RDS-ADAPTER-FINDINGS.md` §6.3 offers it as the + alternative to the retry: it costs one fewer wasted collection and makes the + degradation an explicit statement rather than an inference. The retry was + chosen because it needs nothing from the operator and produces the documented + behaviour for a credential nobody described correctly. Both are honest; only + letting the run die is not. +- **The parity engine's thresholds are not exposed.** `MinWindow`, `Headroom`, + `MinConfidence` and `Percentile` stay at `pkg/rds`'s policy. A second set of + thresholds in `cmd/` would be a second answer to a question that must have + one. diff --git a/cmd/kilter/domains.go b/cmd/kilter/domains.go index 7de59bf..3a4c4b6 100644 --- a/cmd/kilter/domains.go +++ b/cmd/kilter/domains.go @@ -28,13 +28,15 @@ import ( // `kilter domains` is where eight packages of decision logic become reachable // from the binary. // -// Everything it reads is a RECORDED SNAPSHOT. No AWS SDK is linked on this -// path, no network call is made, and no clock is read inside a decision: the -// decision time comes from --now (defaulting to the wall clock once, at the -// top) and is threaded through every domain. That is not a testing -// convenience; it is design invariant 2 — the brain's decision path stays -// stdlib-and-intra-repo, and the SDK collectors that fill these snapshots run -// elsewhere. +// Everything it reads is a RECORDED SNAPSHOT, with exactly one exception: +// --rds-region dials AWS through pkg/provider's read-only SDK adapters to fill +// the RDS snapshot that would otherwise come from --rds-fixture. Without that +// flag no network call is made and no credential is read. No clock is read +// inside a decision either way: the decision time comes from --now (defaulting +// to the wall clock once, at the top) and is threaded through every domain. +// That is not a testing convenience; it is design invariant 2 — the brain's +// DECISION path stays stdlib-and-intra-repo, and collection is the only place +// an SDK appears. // // The command prints refusals as prominently as recommendations. On a real // account most of the output IS refusals — every Lambda function on a @@ -57,8 +59,12 @@ Flags: --snapshot PATH domain snapshot JSON; repeatable, routed by its "domain" field --kube-snapshot PATH cluster snapshot JSON (kilter analyze --dump-snapshot) for k8s-fargate --rds-fixture PATH recorded RDS account; runs the real rds collector (repeatable) + --rds-region REGION collect RDS LIVE from this region (needs AWS credentials; repeatable) --rds-rates PATH RDS rate override JSON; layered over the shipped unverified table --rds-window DUR RDS observation window (default 336h); clamped to CloudWatch retention + --rds-parity also assess gp2/gp3 storage parity (reads the modification envelope) + --rds-parity-rates P verified provisioned-IOPS/throughput rates; without it parity refuses + to call its arithmetic a saving --commitments PATH RI/Savings-Plan inventory JSON (kilter pricing sync-commitments) --catalog PATH pricing catalog JSON (default: embedded) --domain KIND restrict to one domain; repeatable (%s) @@ -116,16 +122,22 @@ type domainFlags struct { snapshots repeatedFlag kubeSnaps repeatedFlag rdsFixtures repeatedFlag - kinds repeatedFlag - rdsRates string - rdsWindow time.Duration - rdsDetail bool - commitments string - catalog string - scope string - region string - now string - jsonOut bool + // rdsRegions is the LIVE sibling of rdsFixtures: one collector, one + // RDSAPI and one CloudWatchAPI per region, merged into one domain. + rdsRegions repeatedFlag + kinds repeatedFlag + rdsRates string + // rdsParityRates prices the two gp3 knobs the rate card does not cover. + rdsParityRates string + rdsWindow time.Duration + rdsDetail bool + rdsParity bool + commitments string + catalog string + scope string + region string + now string + jsonOut bool maxSteps int window string @@ -146,9 +158,12 @@ func (df *domainFlags) bind(fs *flag.FlagSet, withPlan bool) { fs.Var(&df.snapshots, "snapshot", "domain snapshot JSON (repeatable)") fs.Var(&df.kubeSnaps, "kube-snapshot", "cluster snapshot JSON for k8s-fargate (repeatable)") fs.Var(&df.rdsFixtures, "rds-fixture", "recorded RDS account JSON, run through the real collector (repeatable)") + fs.Var(&df.rdsRegions, "rds-region", "collect RDS live from this region (requires AWS credentials; repeatable)") fs.StringVar(&df.rdsRates, "rds-rates", "", "RDS rate override JSON (pkg/rds LoadRates format)") fs.DurationVar(&df.rdsWindow, "rds-window", 14*24*time.Hour, "RDS observation window") fs.BoolVar(&df.rdsDetail, "rds-detail", false, "also print pkg/rds's own refusals-first report") + fs.BoolVar(&df.rdsParity, "rds-parity", false, "assess gp2/gp3 storage parity (reads the RDS modification envelope)") + fs.StringVar(&df.rdsParityRates, "rds-parity-rates", "", "verified provisioned-IOPS/throughput rates JSON") fs.Var(&df.kinds, "domain", "restrict to one domain kind (repeatable)") fs.StringVar(&df.commitments, "commitments", "", "RI/Savings-Plan inventory JSON") fs.StringVar(&df.catalog, "catalog", "", "pricing catalog JSON (default: embedded)") @@ -253,20 +268,22 @@ func buildRuntime(df *domainFlags) (*runtime, error) { // is and changing it is a failover, allocated storage cannot shrink, and // FreeableMemory is MemAvailable. Its Recommend() is empty by construction, // so the whole output arrives through the Refuser seam. + // + // --rds-parity additionally fills pkg/rds's StorageParity seam, which is + // nil by default. Nil is not a hole: the sizer then refuses every + // instance's storage with no-storage-performance-model, so a report that + // did not assess parity SAYS it did not on every line. var rdsDomain *domrds.Domain + var rdsParity *rdsParitySeam if wanted[domain.RDS] { - card, err := loadRDSRates(df.rdsRates) - if err != nil { - return nil, err - } - d, err := domrds.New(domrds.Config{Scope: df.scope, Region: df.region, Rates: card}) + d, seam, err := newRDSDomain(df, now) if err != nil { return nil, err } if err := rt.Registry.Register(d); err != nil { return nil, err } - rdsDomain, rt.rds = d, d + rdsDomain, rt.rds, rdsParity = d, d, seam } // Feed it. A snapshot that cannot be read is fatal (a path the operator @@ -314,30 +331,47 @@ func buildRuntime(df *domainFlags) (*runtime, error) { // flattened into samples arrives looking complete, and a truncated // DatabaseConnections series that looks complete is an idle verdict // manufactured out of silence. + // + // §6.5 (in absorbRDS): snap.Reservations is already + // []commit.ReservedDBInstance and goes straight into the account-wide + // inventory. An RDS line is absorbed by a Reserved DB Instance and by + // nothing else — no Savings Plan of any type covers RDS — so appending + // cannot disturb what --commitments contributed for the other domains. for _, path := range df.rdsFixtures { if rdsDomain == nil { rt.Warnings = append(rt.Warnings, fmt.Sprintf("%s: RDS fixture supplied, but the rds domain is not registered here", path)) continue } - snap, warns, err := collectRDS(context.Background(), path, df.scope, df.region, now, df.rdsWindow) + snap, envs, warns, err := collectRDSFixture(context.Background(), path, rdsOptions(df, df.region, now)) if err != nil { - return nil, err + return nil, rdsFailure(err, warns) } rt.Warnings = append(rt.Warnings, warns...) - if err := rdsDomain.Observe(snap); err != nil { - return nil, fmt.Errorf("%s: %w", path, err) + if inv, err = absorbRDS(rdsDomain, rdsParity, snap, envs, inv, path); err != nil { + return nil, err } - // §6.5: snap.Reservations is already []commit.ReservedDBInstance and - // goes straight into the account-wide inventory. An RDS line is - // absorbed by a Reserved DB Instance and by nothing else — no Savings - // Plan of any type covers RDS — so appending cannot disturb what - // --commitments contributed for the other domains. - if len(snap.Reservations) > 0 { - if inv == nil { - inv = &kcommit.Inventory{} - } - inv.ReservedDBs = append(inv.ReservedDBs, snap.Reservations...) + } + // The LIVE sibling of the loop above (cmd/WIRING-FINDINGS.md §6.1). One + // RDSAPI, one CloudWatchAPI and one collector per region, merged into the + // same domain — and everything downstream is identical, because a live + // snapshot is the same type as a recorded one. + for _, region := range df.rdsRegions { + if rdsDomain == nil { + rt.Warnings = append(rt.Warnings, + fmt.Sprintf("--rds-region %s: supplied, but the rds domain is not registered here", region)) + continue + } + snap, envs, warns, err := collectRDSLive(context.Background(), rdsOptions(df, region, now)) + if err != nil { + // A collection that failed halfway still learned which region and + // which permission. buildRuntime returns nil on error, so the only + // channel that survives is the error itself. + return nil, rdsFailure(err, warns) + } + rt.Warnings = append(rt.Warnings, warns...) + if inv, err = absorbRDS(rdsDomain, rdsParity, snap, envs, inv, "rds "+region); err != nil { + return nil, err } } diff --git a/cmd/kilter/rds.go b/cmd/kilter/rds.go index 12c5068..95512f9 100644 --- a/cmd/kilter/rds.go +++ b/cmd/kilter/rds.go @@ -11,28 +11,27 @@ import ( krds "github.com/agenticode/kilter/pkg/rds" ) -// The RDS wiring, and the exact line where it stops. +// The RDS wiring over a RECORDED account. // // pkg/rds/FINDINGS.md §6 owes cmd/ four things: the domain kind (landed in -// pkg/domain), an SDK adapter over three read seams, the collection loop, and -// the rate override. Three of the four are here. The fourth — the adapter over -// `*rds.Client` and `*cloudwatch.Client` — is NOT, and cannot be in this -// build: `github.com/aws/aws-sdk-go-v2/service/rds` and `.../service/cloudwatch` -// are not in go.mod, and adding them is a go.mod/go.sum change this unit may -// not make. See cmd/WIRING-FINDINGS.md. +// pkg/domain), an SDK adapter over the read seams, the collection loop, and +// the rate override. All four are now wired — the adapter landed in +// pkg/provider (PR#45) and cmd/kilter/rdslive.go drives it — and this file +// keeps the half that needs no account. // -// What replaces it is not a stub. `rds.Fixture` implements all three seams -// with real pagination, real truncation and real empty-account behaviour, and -// it is exported for exactly this reason — "the seams are the contract, and a -// contract nobody outside the package can exercise is not a contract". So -// --rds-fixture drives the REAL collector: rds.NewCollector over the recorded -// account, rds.Collector.Collect, the real window clamp, the real GetMetricData -// batching and ID routing. Every line of pkg/rds/collect.go that a live -// credential would exercise is exercised here, and the only thing missing is -// the field copy between an SDK struct and a struct with the same field names. +// It is not a stub and it never was. `rds.Fixture` implements the three +// collection seams with real pagination, real truncation and real +// empty-account behaviour, and it is exported for exactly this reason — "the +// seams are the contract, and a contract nobody outside the package can +// exercise is not a contract". So --rds-fixture drives the REAL collector: +// rds.NewCollector over the recorded account, rds.Collector.Collect, the real +// window clamp, the real GetMetricData batching and ID routing. `rds`'s +// EnvelopeFixture does the same for the U13 modification seam, so --rds-parity +// is exercisable without an AWS account too. // // No credential is read, no ~/.aws is opened, and no network call is made on -// this path — the same guarantee `kilter domains` already gives. +// THIS path. `--rds-region` is the path that does, and it is a sibling rather +// than a replacement: every test in this package drives the recorded one. // rdsFixtureFile is the on-disk shape of a recorded RDS account. // @@ -73,26 +72,59 @@ type rdsFixtureFile struct { // rds:DescribeReservedDBInstances. §6.2: nil ⇒ net == gross, which // under-claims and can never invent a saving. NoCommitmentAPI bool `json:"noCommitmentAPI,omitempty"` + + // --- the U13 modification seam, read only under --rds-parity ---------- + + // StorageOptions is the recorded rds:DescribeValidDBInstanceModifications + // answer per DBInstanceIdentifier: the ranges AWS says this instance can + // be provisioned within. An instance absent from this map has an UNKNOWN + // envelope, not an unlimited one, and every provisioning proposal for it + // is refused by name. + StorageOptions map[string][]krds.ValidStorageOptionRecord `json:"storageOptions,omitempty"` + // Events is the recorded rds:DescribeEvents answer per + // DBInstanceIdentifier. It is what the four-storage-modifications-per-24- + // hours limit is evaluated from, and an instance absent from this map has + // an empty history that WAS read — which is not the same as a history that + // could not be read (NoEnvelopeAPI). + Events map[string][]krds.EventRecord `json:"events,omitempty"` + // NoEnvelopeAPI models a caller holding rds:Describe* and NOT + // rds:DescribeValidDBInstanceModifications. nil ⇒ every envelope is + // unknown and every provisioning proposal refuses with + // provisioning-envelope-unknown. That is a complete report, not a failed + // one, and it is a DIFFERENT report from one where the seam answered and + // named no ceiling. + NoEnvelopeAPI bool `json:"noEnvelopeAPI,omitempty"` } // collectRDS runs the real collector over a recorded account and returns the -// native snapshot. +// native snapshot. It is collectRDSFixture without the U13 envelope, kept as +// the narrow entry point the generic-seam test drives. +func collectRDS(ctx context.Context, path, scope, region string, now time.Time, span time.Duration) (*krds.Snapshot, []string, error) { + snap, _, warns, err := collectRDSFixture(ctx, path, + rdsCollectOptions{Scope: scope, Region: region, Now: now, Span: span}) + return snap, warns, err +} + +// collectRDSFixture runs the real collector — and, under --rds-parity, the +// real envelope collector — over a recorded account. // // The window is [now-span, now] and is then CLAMPED by the collector, because // 1-minute CloudWatch datapoints live 15 days: a snapshot that claims a 30-day // window and holds 15 days of data is a lie told by omission, and every // downstream "insufficient window" gate reads the claim rather than the data. // The clamp is why c.Window() is rendered and the request is not. -func collectRDS(ctx context.Context, path, scope, region string, now time.Time, span time.Duration) (*krds.Snapshot, []string, error) { +func collectRDSFixture(ctx context.Context, path string, opts rdsCollectOptions) ( + *krds.Snapshot, []krds.Envelope, []string, error) { + raw, err := os.ReadFile(path) if err != nil { - return nil, nil, fmt.Errorf("--rds-fixture: %w", err) + return nil, nil, nil, fmt.Errorf("--rds-fixture: %w", err) } var ff rdsFixtureFile dec := json.NewDecoder(strings.NewReader(string(raw))) dec.DisallowUnknownFields() if err := dec.Decode(&ff); err != nil { - return nil, nil, fmt.Errorf("%s: %w", path, err) + return nil, nil, nil, fmt.Errorf("%s: %w", path, err) } fx := &krds.Fixture{ @@ -105,8 +137,8 @@ func collectRDS(ctx context.Context, path, scope, region string, now time.Time, DropResults: ff.DropResults, } - cfg := krds.DefaultCollectorConfig(krds.Window{Start: now.Add(-span), End: now}) - cfg.Scope, cfg.Region = scope, region + cfg := krds.DefaultCollectorConfig(krds.Window{Start: opts.Now.Add(-opts.Span), End: opts.Now}) + cfg.Scope, cfg.Region = opts.Scope, opts.Region // The three seams. Two of them are optional and their absence is a // DIFFERENT report rather than a failure — that is the whole reason @@ -122,11 +154,11 @@ func collectRDS(ctx context.Context, path, scope, region string, now time.Time, c, err := krds.NewCollector(fx, metrics, reserved, cfg) if err != nil { - return nil, nil, fmt.Errorf("%s: %w", path, err) + return nil, nil, nil, fmt.Errorf("%s: %w", path, err) } snap, err := c.Collect(ctx) if err != nil { - return nil, nil, fmt.Errorf("%s: %w", path, err) + return nil, nil, nil, fmt.Errorf("%s: %w", path, err) } var warnings []string @@ -138,7 +170,23 @@ func collectRDS(ctx context.Context, path, scope, region string, now time.Time, for _, w := range snap.Warnings { warnings = append(warnings, path+": "+w) } - return snap, warnings, nil + + // The fourth seam, read only when --rds-parity asked for it. NoEnvelopeAPI + // hands the collector a nil interface, which is legal and yields a wholly + // unknown envelope set — the recorded form of a caller who holds + // rds:Describe* and not rds:DescribeValidDBInstanceModifications. + var envAPI krds.ModificationEnvelopeAPI + if !ff.NoEnvelopeAPI { + envAPI = &krds.EnvelopeFixture{ + Options: ff.StorageOptions, Events: ff.Events, PageSize: ff.PageSize, + } + } + envs, ewarns, err := collectRDSEnvelopes(ctx, opts, envAPI, rdsIdentifiers(snap), path) + warnings = append(warnings, ewarns...) + if err != nil { + return nil, nil, warnings, fmt.Errorf("%s: %w", path, err) + } + return snap, envs, warnings, nil } // loadRDSRates resolves the rate card. diff --git a/cmd/kilter/rdslive.go b/cmd/kilter/rdslive.go new file mode 100644 index 0000000..8d0ca26 --- /dev/null +++ b/cmd/kilter/rdslive.go @@ -0,0 +1,507 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "os" + "strings" + "time" + + domrds "github.com/agenticode/kilter/pkg/domain/rds" + kcommit "github.com/agenticode/kilter/pkg/pricing/commit" + "github.com/agenticode/kilter/pkg/provider" + krds "github.com/agenticode/kilter/pkg/rds" +) + +// The two RDS paths cmd/WIRING-FINDINGS.md §6.1 and §6.4 left unreachable: a +// live account, and the storage-parity seam. +// +// Both are READ-ONLY. Seven AWS operations are reachable from here and every +// one of them is a GET — `pkg/provider`'s TestNoMutatingSDKSurface is the +// code-side proof, and nothing in this file imports pkg/rds's actuator, +// names ApprovedStep or ModifyStorage, or gives the binary any way to reach +// them. Making an actuator reachable is a separate, separately-approved +// decision. +// +// # What is a sibling of what +// +// `--rds-fixture` is NOT replaced. It is how the collector is exercised +// without an account, it is what every test in this package drives, and the +// live path is a second source feeding the same domain: +// +// --rds-fixture PATH a recorded account, through the real collector +// --rds-region REGION a live account, through pkg/provider's SDK adapters +// --rds-parity additionally read the modification envelope and run +// pkg/rds's storage-parity engine over it +// +// Everything after collection is identical for both: Observe, the reservation +// splice into the account-wide inventory, the parity envelope. absorbRDS is +// that shared tail, written once so the two loops cannot drift. +// +// # The one degradation cmd/ has to implement itself +// +// RDS-ADAPTER-FINDINGS.md §3 is not symmetric, and the asymmetry is the whole +// content of this file's error handling: a NIL MetricsAPI degrades (complete +// report, every instance refusing with no-metric-evidence) while a FAILING one +// aborts the collection. `cloudwatch:GetMetricData` is documented optional, so +// a credential without it must get the degraded report rather than an +// AccessDeniedException — which means one retry with the seam dropped, gated +// on provider.IsAccessDenied so a throttle or a timeout stays an error. +// +// Every other optional seam already degrades inside pkg/rds and is wired +// unconditionally: a denied DescribeReservedDBInstances warns and nets to +// gross, a denied DescribeDBClusters warns and falls back to the more cautious +// cluster-member exclusion, a denied ListTagsForResource warns by name and +// leaves the kilter.dev/mode guardrail unevaluated, and a denied +// DescribeValidDBInstanceModifications or DescribeEvents leaves that +// instance's envelope unknown so its provisioning proposals refuse. Not one of +// those is turned into a hard failure here, and not one of them is turned into +// silence either. + +// rdsEnvelopeWindow is the event history the parity seam reads. +// +// It must exceed StorageModificationWindow: the question the events answer is +// "have there been four storage modifications in the last 24 hours", and a +// window that cannot contain 24 hours cannot rule one out — pkg/rds's +// EnvelopeCollector says exactly that in a warning if you hand it a shorter +// one. 48 h is RDS-ADAPTER-FINDINGS.md §6.4's figure: twice the period, so the +// oldest modification that still blocks is comfortably inside it. +const rdsEnvelopeWindow = 48 * time.Hour + +// rdsCollectOptions is one RDS collection, recorded or live. +type rdsCollectOptions struct { + // Scope is the accountID/region every target ref is stamped with. + Scope string + // Region selects the rate-card row that prices every instance, and on the + // live path it is also the region the SDK clients talk to. The two must be + // the same value or every dollar in the report is confidently wrong. + Region string + // Now is the decision time. It comes from --now and is never a clock read + // inside a decision. + Now time.Time + // Span is the requested observation window. pkg/rds clamps it to + // CloudWatch retention; this package does not re-derive that. + Span time.Duration + // Parity requests the modification envelope and enables pkg/rds's + // storage-parity seam. Off by default, and its absence is visible in the + // report — see rdsParitySeam. + Parity bool +} + +func rdsOptions(df *domainFlags, region string, now time.Time) rdsCollectOptions { + return rdsCollectOptions{ + Scope: df.scope, Region: region, Now: now, Span: df.rdsWindow, Parity: df.rdsParity, + } +} + +// rdsLiveSeams is one region's live read surface: the four pkg/rds interfaces, +// plus the adapters' own Notes(). +// +// Three of the four are the SAME *provider.RDSAPI, because one credential +// answers all three rds: seams — exactly as rds.Fixture does. They stay +// separate fields because a caller may hold one permission and not another, +// and the right behaviour then is a degraded report rather than a missing one. +type rdsLiveSeams struct { + // Region is what the adapter reports back, not what was asked for, and it + // is what CollectorConfig.Region is set from. + Region string + Inventory krds.InventoryAPI + Metrics krds.MetricsAPI + Commitment krds.CommitmentAPI + Envelope krds.ModificationEnvelopeAPI + // Notes returns the degradations the seam structs have no field for — an + // unaddressable instance, a keyless tag, an undated event, a result + // CloudWatch did not identify. RDS-ADAPTER-FINDINGS.md §6.2: rendering + // them is not optional, because a degradation nobody can see is a + // degradation that did not happen. + Notes func() []string +} + +// newRDSLiveSeams is the constructor seam. +// +// It is a variable so the tests in this package can drive the whole live path +// — the retry, the notes, the envelope collection, the warning strings — over +// pkg/rds's own fixtures. No test in this package reads a credential, opens +// ~/.aws, sets an AWS_* variable or touches a socket, and a test that could +// reach the network from a developer laptop with a stale profile would be a +// failed unit rather than a slow one. +var newRDSLiveSeams = dialRDS + +// dialRDS builds the live adapters. This is the only function in cmd/ that +// reads an AWS credential for RDS. +func dialRDS(ctx context.Context, region string) (rdsLiveSeams, error) { + inv, err := provider.NewRDSAPI(ctx, region) + if err != nil { + return rdsLiveSeams{}, err + } + // RDS-ADAPTER-FINDINGS.md §6.5: the two adapters MUST be paired on the + // same region. A metric is published in the region its database lives in, + // so a cross-region pairing returns empty series for every instance — + // which reads as an account full of idle databases rather than as a + // misconfiguration. + cw, err := provider.NewCloudWatchAPI(ctx, inv.Region()) + if err != nil { + return rdsLiveSeams{}, err + } + return rdsLiveSeams{ + Region: inv.Region(), + Inventory: inv, Metrics: cw, Commitment: inv, Envelope: inv, + Notes: func() []string { return append(inv.Notes(), cw.Notes()...) }, + }, nil +} + +// collectRDSLive runs the real collector over a live account in one region. +// +// The window is [now-span, now] and is then CLAMPED by pkg/rds, because +// 1-minute CloudWatch datapoints live 15 days. The clamp is pkg/rds's job and +// this function does not re-derive it; it reports it, because a snapshot that +// claims a 30-day window and holds 15 days of data is a lie told by omission. +// +// Warnings are returned even on the error path. A collection that failed +// halfway still learned things — which region, which permission, which clamp — +// and dropping them because the run ended badly is how an operator ends up +// debugging an AccessDeniedException with no idea which of seven calls raised +// it. +func collectRDSLive(ctx context.Context, opts rdsCollectOptions) ( + *krds.Snapshot, []krds.Envelope, []string, error) { + + seams, err := newRDSLiveSeams(ctx, opts.Region) + if err != nil { + return nil, nil, nil, fmt.Errorf("--rds-region %s: %w", opts.Region, err) + } + label := "rds " + seams.Region + var warnings []string + note := func(format string, args ...any) { + warnings = append(warnings, label+": "+fmt.Sprintf(format, args...)) + } + + cfg := krds.DefaultCollectorConfig(krds.Window{Start: opts.Now.Add(-opts.Span), End: opts.Now}) + // The SAME region the clients talk to, per RDS-ADAPTER-FINDINGS.md §6.1. + cfg.Scope, cfg.Region = opts.Scope, seams.Region + + collect := func(metrics krds.MetricsAPI) (*krds.Snapshot, krds.Window, error) { + c, err := krds.NewCollector(seams.Inventory, metrics, seams.Commitment, cfg) + if err != nil { + return nil, krds.Window{}, err + } + snap, err := c.Collect(ctx) + return snap, c.Window(), err + } + + snap, observed, err := collect(seams.Metrics) + // The ONE seam that must be dropped rather than propagated. Everything + // else that can be denied already degrades inside pkg/rds, and a hard + // failure here would turn a documented degraded report into no report. + if err != nil && isMetricsAccessDenied(err) { + note("this credential does not hold cloudwatch:GetMetricData, so the metrics seam was " + + "dropped and every instance is reported without CloudWatch evidence and refuses with " + + krds.ReasonNoMetricEvidence + " — the inventory is complete and no verdict is drawn " + + "from the silence") + snap, observed, err = collect(nil) + } + if err != nil { + // Loud, by design. DescribeDBInstances is the one hard dependency: + // with no inventory there is nothing to report on, and a report that + // silently covered fewer databases than the account holds is worse + // than no report at all. + return nil, nil, warnings, fmt.Errorf("%s: %w", label, err) + } + + if observed != cfg.Window { + note("observation window clamped to %s (1-minute CloudWatch datapoints live %s)", + observed.String(), krds.RetentionAtOneMinute) + } + for _, n := range seams.Notes() { + note("%s", n) + } + for _, w := range snap.Warnings { + note("%s", w) + } + + envs, ewarns, err := collectRDSEnvelopes(ctx, opts, seams.Envelope, rdsIdentifiers(snap), label) + warnings = append(warnings, ewarns...) + if err != nil { + return nil, nil, warnings, fmt.Errorf("%s: %w", label, err) + } + return snap, envs, warnings, nil +} + +// isMetricsAccessDenied is the exact predicate the one retry is gated on. +// +// BOTH halves matter. provider.IsAccessDenied matches only permission denials, +// so a throttle or a timeout stays an error rather than being recorded as a +// permission the credential actually holds; and the message test keeps the +// retry pointed at the metrics seam, so a denied DescribeDBInstances — which +// must fail loudly — can never be quietly downgraded into "no CloudWatch". +// pkg/rds wraps that one failure as `rds: get metric data: %w`. +func isMetricsAccessDenied(err error) bool { + return err != nil && provider.IsAccessDenied(err) && + strings.Contains(err.Error(), "get metric data") +} + +// rdsIdentifiers is the instance list the envelope seam is asked about, in the +// snapshot's own order. The collector sorts and de-duplicates it. +func rdsIdentifiers(snap *krds.Snapshot) []string { + if snap == nil { + return nil + } + out := make([]string, 0, len(snap.Targets)) + for _, t := range snap.Targets { + if id := strings.TrimSpace(t.Instance.Identifier); id != "" { + out = append(out, id) + } + } + return out +} + +// collectRDSEnvelopes reads the provisioning envelope and the recent +// storage-modification history, and is a no-op unless --rds-parity asked for +// it. A nil api is legal and yields a wholly unknown set, which refuses every +// provisioning proposal by name rather than assuming a ceiling. +func collectRDSEnvelopes(ctx context.Context, opts rdsCollectOptions, + api krds.ModificationEnvelopeAPI, ids []string, label string) ([]krds.Envelope, []string, error) { + + if !opts.Parity { + return nil, nil, nil + } + c := krds.NewEnvelopeCollector(api, krds.EnvelopeCollectorConfig{ + Window: krds.Window{Start: opts.Now.Add(-rdsEnvelopeWindow), End: opts.Now}, + }) + envs, err := c.Collect(ctx, ids) + if err != nil { + return nil, nil, err + } + warns := make([]string, 0, len(envs.Warnings)) + for _, w := range envs.Warnings { + warns = append(warns, label+": "+w) + } + return envs.All(), warns, nil +} + +// --- the storage-parity seam ---------------------------------------------- + +// rdsParitySeam is the cmd/-side holder for pkg/rds's StorageParity seam +// (U13), and it exists to break one ordering problem. +// +// rds.Config.Parity is fixed when the domain is CONSTRUCTED, and the parity +// engine needs the modification envelopes, which can only be read once the +// inventory names the instances — which happens after the domain is +// registered. So the domain is registered holding this indirection, and every +// collected source folds its envelopes in through observe(). The envelopes are +// accumulated across sources and the engine is rebuilt, because one domain can +// be fed several fixtures and several live regions and AssessParity is called +// once at report time, after all of them. +// +// # Why the nil branch is not a nil return +// +// StorageParity's third result is "I declined to look at all", and the sizer +// reads it literally: ok=false emits NEITHER a proposal NOR a suppression, and +// it does not fall back to the no-storage-performance-model refusal either, +// because that lives in the else-branch of `cfg.Parity != nil`. A holder that +// returned ok=false while unfilled would therefore produce a report with a +// missing dimension and no line saying so — the exact failure this seam is +// supposed to make impossible. So it returns a suppression instead, and says +// which of the two states produced it. +type rdsParitySeam struct { + now time.Time + perf krds.PerformanceRates + // envelopes is every source's contribution, in collection order. + // krds.NewEnvelopes sorts and de-duplicates them. + envelopes []krds.Envelope + inner krds.StorageParity +} + +// observe folds one source's envelopes in and rebuilds the engine. A nil +// receiver is the "--rds-parity was not passed" case and is a no-op, so both +// collection loops can call it unconditionally. +func (s *rdsParitySeam) observe(envs []krds.Envelope) error { + if s == nil { + return nil + } + s.envelopes = append(s.envelopes, envs...) + p, err := krds.NewParity(krds.ParityConfig{ + Now: s.now, + Envelopes: krds.NewEnvelopes(s.envelopes), + // MinWindow, Headroom, MinConfidence and Percentile are pkg/rds's + // policy and are deliberately not re-stated here. A second set of + // thresholds in cmd/ would be a second answer to a question that must + // have one. + Performance: s.perf, + }) + if err != nil { + return err + } + s.inner = p + return nil +} + +// AssessParity implements krds.StorageParity by delegation. +func (s *rdsParitySeam) AssessParity(inst krds.DBInstance, e krds.Engine, series []krds.Series, + card krds.RateCard) (*krds.Proposal, []krds.Suppression, bool) { + + if s == nil || s.inner == nil { + return nil, []krds.Suppression{{ + Code: krds.ReasonNoStoragePerformanceModel, + Reason: fmt.Sprintf( + "storage-performance parity was requested for %s but no parity engine was built, so "+ + "this instance's storage is unassessed. This is a wiring bug rather than a finding, "+ + "and it is stated rather than skipped: a report missing a dimension it does not "+ + "mention reads as a report that looked and found nothing", inst.DisplayName()), + }}, true + } + return s.inner.AssessParity(inst, e, series, card) +} + +// rdsPerformanceRatesFile is the on-disk shape of --rds-parity-rates. +// +// It is a cmd/-side projection of rds.PerformanceRates with one field +// deliberately missing: `provenance`. rds.LoadRates stamps every loaded row +// operator-supplied and gives the file no way to name its own provenance, for +// the reason that provenance is the single gate between "this sizes an +// opportunity" and "this is a saving somebody can put in a business case". A +// second rate loader that let a file type the word "verified" would be a way +// to promote a guess to a claim, so an unknown field here is an error and the +// stamp is applied by this code. +type rdsPerformanceRatesFile struct { + // ProvisionedIOPSMonthUSD is charged per IOPS above the regime baseline. + ProvisionedIOPSMonthUSD float64 `json:"provisionedIOPSMonthUSD"` + // ProvisionedThroughputMonthUSD is charged per MiB/s above the regime + // baseline. + ProvisionedThroughputMonthUSD float64 `json:"provisionedThroughputMonthUSD"` +} + +// loadRDSPerformanceRates resolves the two gp3 knobs the RateCard does not +// price. +// +// The zero value means rds.DefaultPerformanceRates, every figure of which is +// `unverified` — pkg/rds/FINDINGS.md §7 could not retrieve the RDS +// provisioned-IOPS and provisioned-throughput rates from AWS. That is not a +// failure mode: parity still runs, still does the arithmetic and still reports +// the magnitude, and then refuses to call it a saving under +// `unverified-rate`. Supplying this file is what unblocks the claim. +func loadRDSPerformanceRates(path string) (krds.PerformanceRates, error) { + if path == "" { + return krds.PerformanceRates{}, nil + } + raw, err := os.ReadFile(path) + if err != nil { + return krds.PerformanceRates{}, fmt.Errorf("--rds-parity-rates: %w", err) + } + var f rdsPerformanceRatesFile + dec := json.NewDecoder(strings.NewReader(string(raw))) + dec.DisallowUnknownFields() + if err := dec.Decode(&f); err != nil { + return krds.PerformanceRates{}, fmt.Errorf("--rds-parity-rates %s: %w", path, err) + } + out := krds.PerformanceRates{ + ProvisionedIOPSMonthUSD: f.ProvisionedIOPSMonthUSD, + ProvisionedThroughputMonthUSD: f.ProvisionedThroughputMonthUSD, + Provenance: krds.RateOperator, + } + // Validated at the boundary, so a bad override fails where it was typed + // rather than producing a quietly wrong report. + if err := out.Validate(); err != nil { + return krds.PerformanceRates{}, fmt.Errorf("--rds-parity-rates %s: %w", path, err) + } + return out, nil +} + +// newRDSDomain builds the rds domain, with the parity seam when it was asked +// for. +// +// Without --rds-parity this is exactly domrds.New and rds.Config.Parity stays +// nil, which is what makes the seam's absence VISIBLE: pkg/rds's sizer emits +// no-storage-performance-model for every instance, so a report that did not +// assess parity says so on every line rather than looking complete. +// +// With it, the domain has to be built through rds.NewDomain directly, because +// domrds.Config carries Scope, Region and Rates and has no Parity field — +// pkg/domain/rds is outside this unit's scope. The wrapper is reconstructed +// around the result so Domain.UsageLines still contributes to the +// account-wide commitment baseline; see cmd/RDSLIVE-FINDINGS.md for the +// one-field change that would remove this. +func newRDSDomain(df *domainFlags, now time.Time) (*domrds.Domain, *rdsParitySeam, error) { + card, err := loadRDSRates(df.rdsRates) + if err != nil { + return nil, nil, err + } + if !df.rdsParity { + d, err := domrds.New(domrds.Config{Scope: df.scope, Region: df.region, Rates: card}) + return d, nil, err + } + perf, err := loadRDSPerformanceRates(df.rdsParityRates) + if err != nil { + return nil, nil, err + } + seam := &rdsParitySeam{now: now, perf: perf} + // Built before any collection, so the engine exists even if no source is + // ever supplied. Its envelopes are empty then, and an empty envelope set + // refuses every provisioning proposal with provisioning-envelope-unknown — + // which is the honest answer, and a louder one than silence. + if err := seam.observe(nil); err != nil { + return nil, nil, fmt.Errorf("--rds-parity: %w", err) + } + sc := krds.DefaultConfig() + sc.Scope, sc.Region = df.scope, df.region + if len(card.Classes) > 0 { + sc.Rates = card + } + sc.Parity = seam + d, err := krds.NewDomain(sc) + if err != nil { + return nil, nil, fmt.Errorf("domain/rds: %w", err) + } + return &domrds.Domain{Domain: d}, seam, nil +} + +// rdsFailure attaches what a failed collection had already learned to the +// error that ended it. +// +// buildRuntime returns nil on error, so anything appended to runtime.Warnings +// on the way to a failure is discarded — which is how an operator ends up +// staring at an AccessDeniedException with no idea which of seven calls raised +// it, or that the run had already fallen back from a denied GetMetricData +// before it died of something else. A collection that failed halfway still +// learned things, and they belong in the only channel that survives. +func rdsFailure(err error, warnings []string) error { + if err == nil || len(warnings) == 0 { + return err + } + var b strings.Builder + b.WriteString(err.Error()) + b.WriteString("\n\nwhat the collection had already learned before it failed:") + for _, w := range warnings { + b.WriteString("\n - ") + b.WriteString(w) + } + return errors.New(b.String()) +} + +// absorbRDS is the tail both collection loops share: observe the snapshot, +// fold in the envelopes, splice the reservations into the account-wide +// inventory. +// +// It is one function rather than two copies because the reservation splice is +// the part with a money consequence — an RDS line is absorbed by a Reserved DB +// Instance and by nothing else, no Savings Plan of any type covers RDS — and +// two copies of it would be two chances for the live path and the recorded +// path to disagree about the same account. +func absorbRDS(d *domrds.Domain, p *rdsParitySeam, snap *krds.Snapshot, + envs []krds.Envelope, inv *kcommit.Inventory, label string) (*kcommit.Inventory, error) { + + if err := d.Observe(snap); err != nil { + return inv, fmt.Errorf("%s: %w", label, err) + } + if err := p.observe(envs); err != nil { + return inv, fmt.Errorf("%s: %w", label, err) + } + if len(snap.Reservations) > 0 { + if inv == nil { + inv = &kcommit.Inventory{} + } + inv.ReservedDBs = append(inv.ReservedDBs, snap.Reservations...) + } + return inv, nil +} diff --git a/cmd/kilter/rdslive_test.go b/cmd/kilter/rdslive_test.go new file mode 100644 index 0000000..cf1eb92 --- /dev/null +++ b/cmd/kilter/rdslive_test.go @@ -0,0 +1,555 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "strings" + "testing" + "time" + + "github.com/aws/smithy-go" + + "github.com/agenticode/kilter/pkg/domain" + krds "github.com/agenticode/kilter/pkg/rds" +) + +// The live RDS path and the storage-parity seam, driven through the REAL CLI. +// +// NO TEST IN THIS FILE MAKES A LIVE AWS CALL. Not one reads a credential, +// opens ~/.aws, sets an AWS_* variable or touches a socket. Every live test +// installs a seam set built from pkg/rds's own Fixture and EnvelopeFixture, +// which is why the collector, the pagination, the window clamp, the +// access-denied retry and the envelope collection are all exercised for real +// while the SDK is never constructed. The single test that does reach +// dialRDS — TestABlankLiveRegionIsRefusedBeforeAnyCredentialIsRead — is +// deliberately the one case that provably returns before LoadDefaultConfig. +// +// A test that would try to reach the network on a developer laptop with a +// stale profile is not a slow test, it is a failed unit. + +// --- fakes ----------------------------------------------------------------- + +// deniedError is an AccessDeniedException in the shape provider.IsAccessDenied +// actually matches: a smithy.APIError, not a string. +type deniedError struct { + code string + msg string +} + +func (e deniedError) Error() string { return e.code + ": " + e.msg } +func (e deniedError) ErrorCode() string { return e.code } +func (e deniedError) ErrorMessage() string { return e.msg } +func (e deniedError) ErrorFault() smithy.ErrorFault { return smithy.FaultClient } + +func accessDenied(op string) error { + return deniedError{code: "AccessDeniedException", + msg: "User is not authorized to perform: " + op} +} + +// liveSeamsFrom builds the live seam set out of a recorded account. +// +// It is deliberately the SAME rdsFixtureFile the --rds-fixture path decodes, +// so the two sources can be compared field for field: anything the live path +// reports that the recorded path does not is a difference in the wiring rather +// than in the data. +func liveSeamsFrom(f rdsFixtureFile, notes []string) (rdsLiveSeams, *krds.Fixture) { + fx := &krds.Fixture{ + Instances: f.Instances, Clusters: f.Clusters, Tags: f.Tags, Metrics: f.Metrics, + Reservations: f.Reservations, PageSize: f.PageSize, DropResults: f.DropResults, + } + s := rdsLiveSeams{ + Region: fixtureRegion, Inventory: fx, Metrics: fx, Commitment: fx, + Envelope: &krds.EnvelopeFixture{ + Options: f.StorageOptions, Events: f.Events, PageSize: f.PageSize}, + Notes: func() []string { return notes }, + } + // The three optional seams, absent in the form pkg/rds documents: a nil + // interface, not a failing one. The difference is the subject of + // TestAFailingMetricsSeamIsNotTheSameAsAnAbsentOne. + if f.NoMetricsAPI { + s.Metrics = nil + } + if f.NoCommitmentAPI { + s.Commitment = nil + } + if f.NoEnvelopeAPI { + s.Envelope = nil + } + return s, fx +} + +// withLiveRDS installs a fake seam set for the duration of one test and +// restores the real dialer afterwards, so a later test can never inherit it. +func withLiveRDS(t *testing.T, s rdsLiveSeams) { + t.Helper() + prev := newRDSLiveSeams + newRDSLiveSeams = func(_ context.Context, region string) (rdsLiveSeams, error) { + out := s + if out.Region == "" { + out.Region = region + } + return out, nil + } + t.Cleanup(func() { newRDSLiveSeams = prev }) +} + +// liveArgs is the live sibling of rdsArgs: the same command over the same +// account, reached through --rds-region instead of --rds-fixture. +func liveArgs(sub string, extra ...string) []string { + args := []string{sub, + "--now", fixtureNow.Format(time.RFC3339), + "--scope", fixtureScope, + "--region", fixtureRegion, + "--domain", "rds", + "--rds-region", fixtureRegion, + } + return append(args, extra...) +} + +// runFails runs the command expecting failure and returns the error. +func runFails(t *testing.T, args ...string) error { + t.Helper() + var b strings.Builder + err := runDomainsTo(&b, args) + if err == nil { + t.Fatalf("kilter domains %s unexpectedly succeeded:\n%s", strings.Join(args, " "), b.String()) + } + return err +} + +// rdsRefusalCodes returns every RDS refusal code in an aggregate report, +// keyed by target and counted. +func rdsRefusalCodes(env reportEnvelope) map[string]int { + out := map[string]int{} + for _, ref := range env.Report.Refusals { + if ref.Target.Domain == domain.RDS { + out[ref.Code]++ + } + } + return out +} + +func warningsMentioning(env reportEnvelope, sub string) []string { + var out []string + for _, w := range env.Warnings { + if strings.Contains(w, sub) { + out = append(out, w) + } + } + return out +} + +// --- (a) the live collector ------------------------------------------------ + +// TestTheLiveRDSCollectorIsReachableFromTheBinary is the reason half of this +// unit exists. pkg/provider shipped both SDK adapters and nothing called them: +// --rds-fixture was the only way to drive pkg/rds, so a user with an AWS +// account could not run the RDS domain at all. +// +// The assertion is equality with the recorded path, because that is the +// strongest available statement: the live wiring passes the same seams to the +// same collector and changes nothing on the way through. A live path that +// merely "worked" could still be silently dropping a page, a tag or a series. +func TestTheLiveRDSCollectorIsReachableFromTheBinary(t *testing.T) { + seams, fx := liveSeamsFrom(buildRDSFixture(), nil) + withLiveRDS(t, seams) + + var live, recorded reportEnvelope + runJSON(t, &live, liveArgs("report")...) + runJSON(t, &recorded, rdsArgs(t, "report")...) + + dr, ok := live.Report.For(domain.RDS) + if !ok { + t.Fatal("the rds domain does not appear in a live report") + } + if !dr.Health.Ready || dr.Health.Targets != 7 { + t.Fatalf("live collection produced %d ready=%v targets, want 7 ready", dr.Health.Targets, dr.Health.Ready) + } + if dr.Refused == 0 { + t.Fatal("the live path produced no refusals; the refusals ARE this domain's output") + } + // Real pagination, not a single page: the recorded account uses pageSize 3 + // over 7 instances, and the live seams inherit it. + if fx.Calls.DescribeDBInstances < 3 { + t.Errorf("DescribeDBInstances called %d times over a 3-instance page size; "+ + "pagination did not survive the live wiring", fx.Calls.DescribeDBInstances) + } + if fx.Calls.ListTagsForResource == 0 { + t.Error("ListTagsForResource was never called, so the kilter.dev/mode guardrail is unreachable live") + } + if fx.Calls.GetMetricData == 0 { + t.Error("GetMetricData was never called; every instance would refuse for lack of evidence") + } + + gotLive, err := json.Marshal(live.Report) + if err != nil { + t.Fatal(err) + } + gotRec, err := json.Marshal(recorded.Report) + if err != nil { + t.Fatal(err) + } + if string(gotLive) != string(gotRec) { + t.Errorf("the live path and the recorded path disagree about the same account:\n--- live ---\n%s\n--- recorded ---\n%s", + gotLive, gotRec) + } +} + +// TestLiveReservationsReachTheAccountWideBaseline: the live snapshot's +// Reserved DB Instances are spliced into the commitment inventory exactly as +// the recorded path's are. An RDS line is absorbed by a Reserved DB Instance +// and by nothing else, so a live path that dropped them would understate +// coverage on every other domain's report too. +func TestLiveReservationsReachTheAccountWideBaseline(t *testing.T) { + seams, _ := liveSeamsFrom(buildRDSFixture(), nil) + withLiveRDS(t, seams) + + df := &domainFlags{ + now: fixtureNow.Format(time.RFC3339), scope: fixtureScope, region: fixtureRegion, + rdsWindow: 14 * 24 * time.Hour, + } + df.kinds = repeatedFlag{"rds"} + df.rdsRegions = repeatedFlag{fixtureRegion} + rt, err := buildRuntime(df) + if err != nil { + t.Fatal(err) + } + var rdsLines int + for _, l := range rt.Ledger.Baseline() { + if strings.Contains(l.ID, ":db:") { + rdsLines++ + } + } + if rdsLines == 0 { + t.Fatal("no live RDS usage line reached the account-wide baseline") + } + for _, w := range rt.Warnings { + if strings.Contains(w, "dropped a usage line") { + t.Errorf("a live RDS baseline line was dropped: %s", w) + } + } +} + +// TestTheAdaptersNotesAreRenderedBesideTheSnapshotWarnings. +// +// RDS-ADAPTER-FINDINGS.md §3.1 and §6.2: three degradations — an instance with +// neither ARN nor identifier, a tag with no key, an event with no date — are +// SILENT inside pkg/rds because its seam structs have no field for them. The +// adapters carry them on Notes(), and rendering that is not optional. A +// degradation nobody can see is a degradation that did not happen. +func TestTheAdaptersNotesAreRenderedBesideTheSnapshotWarnings(t *testing.T) { + const note = "dropped a tag with an empty key on db-legacy; if it was kilter.dev/mode " + + "the opt-out is not honoured" + seams, _ := liveSeamsFrom(buildRDSFixture(), []string{note}) + withLiveRDS(t, seams) + + var env reportEnvelope + runJSON(t, &env, liveArgs("report")...) + if len(warningsMentioning(env, note)) == 0 { + t.Errorf("the adapter's Notes() never reached the user: %v", env.Warnings) + } +} + +// TestAFailingMetricsSeamIsNotTheSameAsAnAbsentOne is the asymmetry +// RDS-ADAPTER-FINDINGS.md §3 puts in bold, and the one degradation cmd/ has to +// implement itself. +// +// cloudwatch:GetMetricData is documented OPTIONAL, and it is — but only in the +// nil form. A credential that lacks it and is wired anyway makes readMetrics +// return an error and Collect propagate it, so without the retry the operator +// gets an AccessDeniedException where the design promises a complete report in +// which every instance refuses with no-metric-evidence. +func TestAFailingMetricsSeamIsNotTheSameAsAnAbsentOne(t *testing.T) { + f := buildRDSFixture() + seams, fx := liveSeamsFrom(f, nil) + fx.MetricsErr = accessDenied("cloudwatch:GetMetricData") + withLiveRDS(t, seams) + + var env reportEnvelope + runJSON(t, &env, liveArgs("report")...) + + dr, ok := env.Report.For(domain.RDS) + if !ok || dr.Health.Targets != 7 { + t.Fatalf("the inventory did not survive a denied GetMetricData: %+v", dr) + } + if rdsRefusalCodes(env)[krds.ReasonNoMetricEvidence] == 0 { + t.Error("no instance refused with no-metric-evidence; silence was read as data") + } + if len(warningsMentioning(env, "cloudwatch:GetMetricData")) == 0 { + t.Errorf("the degradation is invisible; a report that quietly lost its evidence "+ + "reads as a report about quiet databases: %v", env.Warnings) + } + // The retry must not have manufactured an idle verdict out of the silence. + out := run(t, liveArgs("report", "--rds-detail")...) + if strings.Contains(out, krds.AdvisoryIdleInstance) || strings.Contains(out, krds.AdvisoryIdleReadReplica) { + t.Errorf("an idle verdict was drawn from an unanswered CloudWatch:\n%s", out) + } +} + +// TestAThrottleIsNotMistakenForAMissingPermission is the negative of the test +// above, and it is why the retry is gated on provider.IsAccessDenied rather +// than on "the metrics call failed". +// +// Swallowing a throttle or a timeout would turn a transient fault into a +// permanently degraded report that claims the credential lacks a permission it +// actually holds — and the operator would then go and grant a permission that +// was never missing. +func TestAThrottleIsNotMistakenForAMissingPermission(t *testing.T) { + seams, fx := liveSeamsFrom(buildRDSFixture(), nil) + fx.MetricsErr = deniedError{code: "ThrottlingException", msg: "Rate exceeded"} + withLiveRDS(t, seams) + + err := runFails(t, liveArgs("report")...) + if !strings.Contains(err.Error(), "Rate exceeded") { + t.Errorf("a throttle was reported as something else: %v", err) + } + if strings.Contains(err.Error(), "does not hold cloudwatch:GetMetricData") { + t.Errorf("a throttle was recorded as a missing permission: %v", err) + } +} + +// TestAMissingRequiredPermissionFailsLoudly. +// +// DescribeDBInstances is the one hard dependency in the whole seam set: with +// no inventory there is nothing to report on. The failure mode this guards +// against is the retry being written too broadly — an AccessDenied on the +// REQUIRED call being caught by the metrics branch and downgraded into "no +// CloudWatch", which would produce a confident, complete-looking report over +// zero databases. +func TestAMissingRequiredPermissionFailsLoudly(t *testing.T) { + seams, fx := liveSeamsFrom(buildRDSFixture(), nil) + fx.InstancesErr = accessDenied("rds:DescribeDBInstances") + withLiveRDS(t, seams) + + err := runFails(t, liveArgs("report")...) + if !strings.Contains(err.Error(), "rds:DescribeDBInstances") { + t.Errorf("the error does not name the permission that is missing: %v", err) + } + if strings.Contains(err.Error(), "GetMetricData") { + t.Errorf("a denied DescribeDBInstances was routed through the metrics degradation: %v", err) + } +} + +// TestOptionalPermissionsDegradeToAWarningAndNeverToAFailure covers the two +// optional seams that already degrade INSIDE pkg/rds, wired unconditionally +// for exactly that reason. +// +// A missing optional permission must produce a report WITH A NOTE — never a +// hard failure, and never a silently smaller report. +func TestOptionalPermissionsDegradeToAWarningAndNeverToAFailure(t *testing.T) { + for _, tc := range []struct { + name string + break_ func(*krds.Fixture) + want string + }{ + { + // rds:DescribeReservedDBInstances. Net savings equal gross, which + // UNDER-claims; a failure here would be a run lost to a permission + // that can only ever make a number smaller. + name: "DescribeReservedDBInstances", + break_: func(f *krds.Fixture) { f.ReservationsErr = accessDenied("rds:DescribeReservedDBInstances") }, + want: "under-claims", + }, + { + // rds:DescribeDBClusters. Members are still excluded, under the + // more cautious cluster-member-not-supported rather than Aurora's + // name. + name: "DescribeDBClusters", + break_: func(f *krds.Fixture) { f.ClustersErr = accessDenied("rds:DescribeDBClusters") }, + want: "cluster", + }, + } { + t.Run(tc.name, func(t *testing.T) { + seams, fx := liveSeamsFrom(buildRDSFixture(), nil) + tc.break_(fx) + withLiveRDS(t, seams) + + var env reportEnvelope + runJSON(t, &env, liveArgs("report")...) + dr, ok := env.Report.For(domain.RDS) + if !ok || dr.Health.Targets != 7 { + t.Fatalf("a denied %s shrank the report to %+v", tc.name, dr) + } + if len(warningsMentioning(env, tc.want)) == 0 { + t.Errorf("a denied %s degraded silently; the report looks complete and is not: %v", + tc.name, env.Warnings) + } + }) + } +} + +// TestAnUnreadableTagIsNotAnAbsentTag is the most dangerous class of bug in +// this codebase, asserted at the wiring. +// +// db-legacy carries kilter.dev/mode=off. If a denied ListTagsForResource +// collapsed into an empty tag map, "I could not look" would become "I looked +// and there was nothing", and an operator who tagged a database to be left +// alone would be silently disobeyed by a report that says nothing about it. +// +// The two halves are asserted together on purpose. The guardrail refusal +// DISAPPEARING is what makes the warning load-bearing: assert only the warning +// and the test still passes on a wiring that honours the tag anyway; assert +// only the refusal and the test cannot tell "unreadable" from "absent". +func TestAnUnreadableTagIsNotAnAbsentTag(t *testing.T) { + legacy := rdsARN("db-legacy") + + // Control: with the tag readable, the guardrail fires and nothing warns. + seams, _ := liveSeamsFrom(buildRDSFixture(), nil) + withLiveRDS(t, seams) + var honoured reportEnvelope + runJSON(t, &honoured, liveArgs("report")...) + if rdsRefusalCodes(honoured)[krds.ReasonModeOff] == 0 { + t.Fatal("the control is broken: kilter.dev/mode=off did not reach the report at all") + } + if len(warningsMentioning(honoured, krds.TagKilterMode)) != 0 { + t.Errorf("a readable tag produced a guardrail warning: %v", honoured.Warnings) + } + + // The subject: ListTagsForResource denied for that one instance. + denied, fx := liveSeamsFrom(buildRDSFixture(), nil) + fx.TagsErr = map[string]error{legacy: accessDenied("rds:ListTagsForResource")} + withLiveRDS(t, denied) + var unread reportEnvelope + runJSON(t, &unread, liveArgs("report")...) + + dr, ok := unread.Report.For(domain.RDS) + if !ok || dr.Health.Targets != 7 { + t.Fatalf("a denied ListTagsForResource shrank the report: %+v", dr) + } + // It says so, by name, naming the instance AND the guardrail — the two + // facts an operator needs to know their opt-out was not evaluated. + warns := warningsMentioning(unread, krds.TagKilterMode) + if len(warns) == 0 { + t.Fatalf("an unreadable tag was reported as no tag: %v", unread.Warnings) + } + var named bool + for _, w := range warns { + if strings.Contains(w, legacy) { + named = true + } + } + if !named { + t.Errorf("the warning does not name the instance whose guardrail went unevaluated: %v", warns) + } + // And the consequence is real rather than theoretical: the opt-out is NOT + // honoured. That is precisely why the warning has to exist. + if rdsRefusalCodes(unread)[krds.ReasonModeOff] != 0 { + t.Error("the mode=off guardrail fired without the tags being readable; " + + "this test can no longer distinguish an unreadable tag from an absent one") + } +} + +// TestTheLiveWindowIsClampedByPkgRDSAndTheClampIsSaidOutLoud. +// +// The clamp is pkg/rds's job and this wiring does not re-derive it — it +// reports it. A 30-day request returns 15 days of data inside a 30-day window, +// and silence read across the other 15 is how "this database had no +// connections for a month" gets manufactured. +func TestTheLiveWindowIsClampedByPkgRDSAndTheClampIsSaidOutLoud(t *testing.T) { + seams, _ := liveSeamsFrom(buildRDSFixture(), nil) + withLiveRDS(t, seams) + + var env reportEnvelope + runJSON(t, &env, liveArgs("report", "--rds-window", "720h")...) + if len(warningsMentioning(env, "clamped")) == 0 { + t.Errorf("a 30-day live window was accepted silently: %v", env.Warnings) + } + out := run(t, liveArgs("report", "--rds-window", "720h", "--rds-detail")...) + if strings.Contains(out, "720h0m0s window") { + t.Errorf("the live report renders the REQUESTED window rather than the observed one:\n%s", out) + } +} + +// TestABlankLiveRegionIsRefusedBeforeAnyCredentialIsRead exercises the REAL +// dialer — the only test here that does — and is safe precisely because +// provider.NewRDSAPI rejects a blank region before it calls LoadDefaultConfig. +// +// The region is not a tidiness check: CollectorConfig.Region stamps +// DBInstance.Region and therefore selects the rate-card row that prices every +// instance, so a client talking to one region under a config naming another +// produces a report whose every dollar is confidently wrong. +func TestABlankLiveRegionIsRefusedBeforeAnyCredentialIsRead(t *testing.T) { + for _, k := range []string{"AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY", "AWS_PROFILE", + "AWS_REGION", "AWS_DEFAULT_REGION", "AWS_CONFIG_FILE", "AWS_SHARED_CREDENTIALS_FILE"} { + t.Setenv(k, "") + } + err := runFails(t, "report", "--now", fixtureNow.Format(time.RFC3339), + "--scope", fixtureScope, "--domain", "rds", "--rds-region", "") + if !strings.Contains(err.Error(), "region required") { + t.Errorf("a blank --rds-region was not refused by name: %v", err) + } +} + +// TestNoActuatorBecomesReachableThroughTheLivePath. +// +// This unit wires READ-ONLY paths. pkg/rds's actuator exists and stays +// unreachable: Registry.PlanSteps checks Health before the domain is +// consulted, there is no rds row in the actuator table, and nothing in the +// live wiring imports ApprovedStep or ModifyStorage. Making an actuator +// reachable is a separate, separately-approved decision, and this asserts the +// live source did not smuggle one in. +func TestNoActuatorBecomesReachableThroughTheLivePath(t *testing.T) { + seams, _ := liveSeamsFrom(buildRDSFixture(), nil) + withLiveRDS(t, seams) + + var env struct { + Plans []domain.Plan `json:"plans"` + } + runJSON(t, &env, liveArgs("plan", "--rds-parity")...) + if len(env.Plans) != 1 { + t.Fatalf("got %d plans, want 1", len(env.Plans)) + } + p := env.Plans[0] + if p.Actuatable || len(p.Steps) != 0 || p.RefusalCode != domain.RefuseReportOnly { + t.Errorf("a live, parity-enabled rds domain produced actuatable=%v steps=%d refusal=%q", + p.Actuatable, len(p.Steps), p.RefusalCode) + } +} + +// flakyInventory fails DescribeDBInstances from the Nth call onward, so a +// collection can succeed once and fail on the retry — which is the only shape +// in which a warning exists before a fatal error. +type flakyInventory struct { + *krds.Fixture + failFrom int + calls int + err error +} + +func (f *flakyInventory) DescribeDBInstances(ctx context.Context, + in *krds.DescribeDBInstancesInput) (*krds.DescribeDBInstancesOutput, error) { + + f.calls++ + if f.calls >= f.failFrom { + return nil, f.err + } + return f.Fixture.DescribeDBInstances(ctx, in) +} + +// TestAFailedCollectionSaysWhatItHadAlreadyLearned. +// +// buildRuntime returns nil on error, so every warning appended to +// runtime.Warnings on the way to a failure is discarded — and an operator is +// left with one line where the run had already recorded that it fell back from +// a denied GetMetricData and then died of something else entirely. rdsFailure +// puts them in the only channel that survives an aborted build. +func TestAFailedCollectionSaysWhatItHadAlreadyLearned(t *testing.T) { + seams, fx := liveSeamsFrom(buildRDSFixture(), nil) + // Denied metrics: the first pass records the degradation and retries with + // the seam dropped. The retry then dies of a transient inventory fault, + // which is a different and fatal problem. The recorded account pages 7 + // instances 3 at a time, so the second pass starts at call 4. + fx.MetricsErr = accessDenied("cloudwatch:GetMetricData") + seams.Inventory = &flakyInventory{Fixture: fx, failFrom: 4, + err: errors.New("RequestTimeout: connection reset")} + withLiveRDS(t, seams) + + err := runFails(t, liveArgs("report")...) + if !strings.Contains(err.Error(), "connection reset") { + t.Errorf("the fatal cause is not named: %v", err) + } + if !strings.Contains(err.Error(), "cloudwatch:GetMetricData") { + t.Errorf("the degradation the run had already recorded was thrown away with the runtime: %v", err) + } +} diff --git a/cmd/kilter/rdsliveparity_test.go b/cmd/kilter/rdsliveparity_test.go new file mode 100644 index 0000000..cfaf873 --- /dev/null +++ b/cmd/kilter/rdsliveparity_test.go @@ -0,0 +1,416 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/agenticode/kilter/pkg/domain" + krds "github.com/agenticode/kilter/pkg/rds" +) + +// The StorageParity seam (U13), driven through the REAL CLI over a recorded +// account and over the live seams. +// +// cmd/WIRING-FINDINGS.md §6.4's last bullet: "pkg/rds's StorageParity seam is +// still nil, as U11 shipped it — --rds-fixture cannot enable it and no flag +// pretends to". Both halves are fixed here, and the harder half is the second: +// a flag that pretends to enable a seam is worse than no flag, so the tests +// below assert what the report SAYS in each of the three states — seam off, +// seam on with no envelope, seam on with an envelope. +// +// No AWS call, no credential, no socket. The envelope and the event history +// come from pkg/rds's own EnvelopeFixture through the real EnvelopeCollector. + +// parityFixture is the recorded account plus one instance the parity engine +// can actually reach a verdict about. +// +// db-gp3-fat is 1,000 GiB of gp3 provisioned at 20,000 IOPS / 900 MiB/s and +// measured at a fraction of it. That is the shape §2.4 says carries the money: +// a REDUCTION toward the striped regime's 12,000 / 500 floor, which is a +// different lever from the gp2→gp3 conversion and the only one on this fixture +// that clears every gate. +func parityFixture() rdsFixtureFile { + f := buildRDSFixture() + f.Instances = append(f.Instances, krds.DBInstanceRecord{ + DBInstanceIdentifier: "db-gp3-fat", DBInstanceArn: rdsARN("db-gp3-fat"), + DBInstanceClass: "db.r6i.xlarge", DBInstanceStatus: krds.StatusAvailable, + Engine: "mysql", EngineVersion: "8.0.39", LicenseModel: krds.LicenseGPL, + AvailabilityZone: fixtureRegion + "a", + AllocatedStorage: 1000, StorageType: krds.StorageGP3, + Iops: 20000, StorageThroughput: 900, + InstanceCreateTime: fixtureNow.Add(-90 * 24 * time.Hour), + }) + // The four series MeasureIO reads. All four, in full: a demand figure + // missing its write half is not a smaller demand, it is an unknown one, + // and pkg/rds refuses on exactly that. + f.Metrics["db-gp3-fat/"+krds.MetricCPUUtilization] = rdsSeries(22) + f.Metrics["db-gp3-fat/"+krds.MetricDatabaseConns] = rdsSeries(30) + f.Metrics["db-gp3-fat/"+krds.MetricReadIOPS] = rdsSeries(300) + f.Metrics["db-gp3-fat/"+krds.MetricWriteIOPS] = rdsSeries(200) + f.Metrics["db-gp3-fat/"+krds.MetricReadThroughput] = rdsSeries(20 * krds.MiB) + f.Metrics["db-gp3-fat/"+krds.MetricWriteThroughput] = rdsSeries(10 * krds.MiB) + + // The LIVE provisioning envelope, read rather than hardcoded: AWS + // publishes two contradictory gp3 ceilings and pkg/rds encodes neither. + f.StorageOptions = map[string][]krds.ValidStorageOptionRecord{ + "db-gp3-fat": {{ + StorageType: krds.StorageGP3, + MinIOPS: 12000, MaxIOPS: 64000, + MinStorageThroughputMBps: 500, MaxStorageThroughputMBps: 4000, + MinAllocatedStorageGiB: 20, MaxAllocatedStorageGiB: 65536, + }}, + } + return f +} + +// parityArgs runs the recorded account with whatever parity flags are added. +func parityArgs(t *testing.T, f rdsFixtureFile, extra ...string) []string { + t.Helper() + args := []string{"report", + "--now", fixtureNow.Format(time.RFC3339), + "--scope", fixtureScope, + "--region", fixtureRegion, + "--domain", "rds", + "--rds-fixture", writeRDSFixture(t, f), + } + return append(args, extra...) +} + +// writeJSONFile writes a rate override and returns its path. +func writeJSONFile(t *testing.T, name, body string) string { + t.Helper() + path := filepath.Join(t.TempDir(), name) + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + return path +} + +// claimableRates is a rate file whose STORAGE rows are operator-supplied, +// which is what makes a storage saving quotable at all. Every shipped row is +// `unverified` and can size a fact and never a saving. +const claimableRates = `{ + "region": "us-east-1", + "classes": {"open-source|db.r6i.xlarge": {"singleAZHourlyUSD": 0.48}}, + "storage": {"gp2GiBMonthUSD": 0.115, "gp3GiBMonthUSD": 0.092, + "io1GiBMonthUSD": 0.125, "io2GiBMonthUSD": 0.125} +}` + +const claimableParityRates = `{"provisionedIOPSMonthUSD": 0.02, "provisionedThroughputMonthUSD": 0.08}` + +// TestParityIsOptInAndItsAbsenceIsVisible is the whole of §6.4's last bullet. +// +// Two failure modes, and the second is the dangerous one: +// +// - a flag that does not enable the seam (the U11 state: the seam is nil and +// nothing can fill it); +// - a report that looks complete and is quietly missing a dimension. +// +// pkg/rds forecloses the second by construction — with Config.Parity nil the +// sizer refuses every instance's storage under no-storage-performance-model — +// and this asserts the CLI actually delivers that, and that --rds-parity +// actually replaces it with real verdicts rather than adding a flag that does +// nothing. +func TestParityIsOptInAndItsAbsenceIsVisible(t *testing.T) { + f := parityFixture() + + var off reportEnvelope + runJSON(t, &off, parityArgs(t, f)...) + codes := rdsRefusalCodes(off) + if codes[krds.ReasonNoStoragePerformanceModel] == 0 { + t.Fatal("a report that did not assess storage parity does not say so; " + + "a reader cannot tell a dimension that was skipped from one that found nothing") + } + for _, parityCode := range []string{ + krds.ReasonParityNoMeasurement, krds.ReasonParityEnvelopeUnknown, + krds.ReasonParityFloorsAtBaseline, krds.ReasonParityCooldown, + } { + if codes[parityCode] != 0 { + t.Errorf("the seam is off and %q was still emitted", parityCode) + } + } + + var on reportEnvelope + runJSON(t, &on, parityArgs(t, f, "--rds-parity")...) + onCodes := rdsRefusalCodes(on) + if onCodes[krds.ReasonNoStoragePerformanceModel] != 0 { + t.Errorf("--rds-parity was passed and the not-evaluated refusal is still there: " + + "the flag pretends to enable a seam it did not") + } + // The seam actually ran: at least one instance now carries a verdict that + // only the parity engine produces. + var reached int + for _, c := range []string{ + krds.ReasonParityNoMeasurement, krds.ReasonParityEnvelopeUnknown, + krds.ReasonParityGP2BandUnpublished, krds.ReasonParityFloorsAtBaseline, + krds.ReasonParityStorageTypeNotModelled, krds.ReasonParityNoCheaperConfig, + } { + reached += onCodes[c] + } + if reached == 0 { + t.Errorf("--rds-parity produced no parity verdict of any kind: %v", onCodes) + } + // And every instance still carries a reason. Silence is not an output. + dr, ok := on.Report.For(domain.RDS) + if !ok || dr.Health.Targets != 8 { + t.Fatalf("parity changed the inventory: %+v", dr) + } +} + +// TestParityWithoutTheEnvelopeSeamRefusesByNameRatherThanAssumingACeiling. +// +// RDS-ADAPTER-FINDINGS.md §4.4: StorageEnvelope.Known with MaxIOPS == 0 reads +// as "no ceiling to enforce" in pkg/rds/parity.go, so an unknown ceiling that +// leaked through as a zero one would let an 80,000-IOPS configuration pass +// validation on an instance capped at 16,000. The seam's absence therefore has +// to arrive as an UNKNOWN envelope and a named refusal — never as a permissive +// default and never as a silent hole. +func TestParityWithoutTheEnvelopeSeamRefusesByNameRatherThanAssumingACeiling(t *testing.T) { + f := parityFixture() + f.NoEnvelopeAPI = true + + var env reportEnvelope + runJSON(t, &env, parityArgs(t, f, "--rds-parity")...) + + dr, ok := env.Report.For(domain.RDS) + if !ok || dr.Health.Targets != 8 { + t.Fatalf("a missing rds:DescribeValidDBInstanceModifications shrank the report: %+v", dr) + } + if rdsRefusalCodes(env)[krds.ReasonParityEnvelopeUnknown] == 0 { + t.Errorf("no instance refused with %q; an unread envelope became an unlimited one: %v", + krds.ReasonParityEnvelopeUnknown, rdsRefusalCodes(env)) + } + if len(warningsMentioning(env, "DescribeValidDBInstanceModifications")) == 0 { + t.Errorf("the missing seam is not named anywhere in the warnings: %v", env.Warnings) + } + // No proposal can be produced without a ceiling to check it against. + for _, rec := range env.Report.Recommendations { + if rec.Target.Domain == domain.RDS { + t.Errorf("a proposal survived an unknown envelope: %s", rec.Target.ID) + } + } +} + +// TestParityReachesAProposalOnlyWithClaimableRates is the seam's happy path +// and its money gate in one test, because they are one decision. +// +// pkg/rds does the whole arithmetic either way — the magnitude is reported +// whatever the rate says. What the provenance decides is whether that +// magnitude is allowed to be called a saving. §7 could not retrieve the RDS +// gp3 storage and provisioned-performance rates from AWS, so every shipped +// figure is `unverified` and refuses under unverified-rate; supplying +// --rds-rates and --rds-parity-rates is what unblocks the claim. +func TestParityReachesAProposalOnlyWithClaimableRates(t *testing.T) { + f := parityFixture() + + // Unverified: the arithmetic runs, the opportunity is sized, the claim is + // refused by name. + var unverified reportEnvelope + runJSON(t, &unverified, parityArgs(t, f, "--rds-parity")...) + if rdsRefusalCodes(unverified)[krds.ReasonUnverifiedRate] == 0 { + t.Error("an unverified storage rate produced no unverified-rate refusal") + } + + rates := writeJSONFile(t, "rates.json", claimableRates) + perf := writeJSONFile(t, "parity-rates.json", claimableParityRates) + args := parityArgs(t, f, "--rds-parity", "--rds-rates", rates, "--rds-parity-rates", perf, "--rds-detail") + + var claimed struct { + Report *domain.Report `json:"report"` + RDS *krds.Report `json:"rds"` + Warnings []string `json:"warnings"` + } + runJSON(t, &claimed, args...) + if claimed.RDS == nil { + t.Fatal("--rds-detail produced no pkg/rds report") + } + if claimed.RDS.Totals.Proposals == 0 { + t.Fatalf("no proposal survived claimable rates; the seam can never produce one:\n%s", + mustJSON(t, claimed.RDS.Totals)) + } + // Report.Validate already ran inside the CLI (it refuses to print an + // invalid report), so reaching here means every clause in validateProposal + // held — including "allocated storage can only ever grow" and "an + // unverified rate is a magnitude, not a saving". + var found bool + for _, a := range claimed.RDS.Assessments { + if a.Proposal == nil { + continue + } + found = true + if a.Proposal.Action != domain.ActionAdvisory { + t.Errorf("%s proposes action %q; this domain is advisory only", a.Target.ID, a.Proposal.Action) + } + if a.Proposal.AllocatedStorageGiB < a.Instance.AllocatedStorageGiB { + t.Errorf("%s proposes shrinking allocated storage, which no RDS API can do", a.Target.ID) + } + if !a.Proposal.RateProvenance.Claimable() { + t.Errorf("%s claims %v/mo from a %q rate", a.Target.ID, + a.Proposal.NetSavingsMonthlyUSD, a.Proposal.RateProvenance) + } + if a.Proposal.IOPS < 12000 || a.Proposal.StorageThroughputMBps < 500 { + t.Errorf("%s proposes %d IOPS / %d MiB/s, below the striped regime's non-reducible floor", + a.Target.ID, a.Proposal.IOPS, a.Proposal.StorageThroughputMBps) + } + } + if !found { + t.Fatal("Totals.Proposals is non-zero and no assessment carries a proposal") + } + // The domain still recommends nothing through the generic seam: a + // domain.Recommendation must carry a Proposed resource shape and pkg/rds + // has none to give. The proposal is a REPORT line, not an executable one. + for _, rec := range claimed.Report.Recommendations { + if rec.Target.Domain == domain.RDS { + t.Errorf("a parity proposal leaked into the actuatable recommendation stream: %s", rec.Target.ID) + } + } +} + +// TestTheParityRatesFileCannotDeclareItsOwnProvenance. +// +// Provenance is the single gate between "this sizes an opportunity" and "this +// is a saving somebody can put in a business case". rds.LoadRates gives a rate +// file no way to name its own, and a second loader that did would be a way to +// promote a guess to a claim by typing a word. +func TestTheParityRatesFileCannotDeclareItsOwnProvenance(t *testing.T) { + bad := writeJSONFile(t, "perf.json", + `{"provisionedIOPSMonthUSD":0.02,"provisionedThroughputMonthUSD":0.08,"provenance":"verified"}`) + if _, err := loadRDSPerformanceRates(bad); err == nil { + t.Error("a parity rate file was allowed to declare itself verified") + } + // A non-positive rate is refused at the boundary rather than producing a + // quietly wrong report downstream. + for _, body := range []string{ + `{"provisionedIOPSMonthUSD":0,"provisionedThroughputMonthUSD":0.08}`, + `{"provisionedIOPSMonthUSD":0.02,"provisionedThroughputMonthUSD":-1}`, + `{"provisionedIOPSMonthUSD":0.02,"iopsRate":0.08}`, + } { + if _, err := loadRDSPerformanceRates(writeJSONFile(t, "perf.json", body)); err == nil { + t.Errorf("accepted %s", body) + } + } + // And the shipped default is what you get with no file: unverified, so it + // sizes and never claims. + got, err := loadRDSPerformanceRates("") + if err != nil { + t.Fatal(err) + } + if got != (krds.PerformanceRates{}) { + t.Errorf("an absent --rds-parity-rates invented rates %+v rather than deferring to pkg/rds", got) + } +} + +// TestTheStorageModificationCooldownIsReadFromRecordedEvents. +// +// "You can perform a maximum of four storage modifications on a DB instance +// within any 24-hour period." A fifth is not a recommendation, it is an API +// error with a dollar figure attached — so the events seam has to be wired, +// not merely declared, and the wiring has to hand it a window longer than 24 +// hours or the limit cannot be ruled out from it. +func TestTheStorageModificationCooldownIsReadFromRecordedEvents(t *testing.T) { + f := parityFixture() + f.Events = map[string][]krds.EventRecord{"db-gp3-fat": { + {SourceIdentifier: "db-gp3-fat", SourceType: krds.EventSourceDBInstance, + Categories: []string{krds.EventCategoryConfigurationChange}, + Message: "Finished applying modification to allocated storage", + Date: fixtureNow.Add(-2 * time.Hour)}, + {SourceIdentifier: "db-gp3-fat", SourceType: krds.EventSourceDBInstance, + Categories: []string{krds.EventCategoryConfigurationChange}, + Message: "Finished applying modification to allocated storage", + Date: fixtureNow.Add(-4 * time.Hour)}, + {SourceIdentifier: "db-gp3-fat", SourceType: krds.EventSourceDBInstance, + Categories: []string{krds.EventCategoryConfigurationChange}, + Message: "Finished applying modification to allocated storage", + Date: fixtureNow.Add(-6 * time.Hour)}, + {SourceIdentifier: "db-gp3-fat", SourceType: krds.EventSourceDBInstance, + Categories: []string{krds.EventCategoryConfigurationChange}, + Message: "Finished applying modification to allocated storage", + Date: fixtureNow.Add(-8 * time.Hour)}, + }} + rates := writeJSONFile(t, "rates.json", claimableRates) + perf := writeJSONFile(t, "parity-rates.json", claimableParityRates) + + var env reportEnvelope + runJSON(t, &env, parityArgs(t, f, "--rds-parity", + "--rds-rates", rates, "--rds-parity-rates", perf)...) + + if rdsRefusalCodes(env)[krds.ReasonParityCooldown] == 0 { + t.Fatalf("four storage modifications in eight hours did not block a fifth: %v", + rdsRefusalCodes(env)) + } + for _, ref := range env.Report.Refusals { + if ref.Code == krds.ReasonParityCooldown && !strings.Contains(ref.Reason, "24-hour") { + t.Errorf("the cooldown refusal does not state the limit it is enforcing: %s", ref.Reason) + } + } + // The window handed to the events seam must exceed the 24-hour period, or + // the collector warns that the limit cannot be ruled out. It must not. + if w := warningsMentioning(env, "shorter than the 24h0m0s"); len(w) != 0 { + t.Errorf("the event window is too short to answer the question it was asked: %v", w) + } +} + +// TestParityIsReachableOnTheLivePathToo. The envelope seam is the same +// *provider.RDSAPI as the inventory seam, so the flag has to work through +// --rds-region as well as --rds-fixture — and produce the same verdicts. +func TestParityIsReachableOnTheLivePathToo(t *testing.T) { + f := parityFixture() + seams, _ := liveSeamsFrom(f, nil) + withLiveRDS(t, seams) + + rates := writeJSONFile(t, "rates.json", claimableRates) + perf := writeJSONFile(t, "parity-rates.json", claimableParityRates) + + var live, recorded reportEnvelope + runJSON(t, &live, liveArgs("report", "--rds-parity", "--rds-rates", rates, "--rds-parity-rates", perf)...) + runJSON(t, &recorded, parityArgs(t, f, "--rds-parity", "--rds-rates", rates, "--rds-parity-rates", perf)...) + + if a, b := mustJSON(t, live.Report), mustJSON(t, recorded.Report); a != b { + t.Errorf("the live parity path and the recorded one disagree:\n--- live ---\n%s\n--- recorded ---\n%s", a, b) + } + if rdsRefusalCodes(live)[krds.ReasonNoStoragePerformanceModel] != 0 { + t.Error("--rds-parity did not reach the live path") + } +} + +// TestADeniedDescribeEventsLeavesTheLimitUnverifiedRatherThanCleared. +// +// rds:DescribeEvents is optional and degrades per instance: HistoryKnown goes +// false, so Cooldown answers "unknown" rather than "clear". That is the +// difference between "we checked and there is room for a fifth modification" +// and "we could not check" — and since the parity engine proceeds either way, +// the ONLY thing standing between an operator and the second being read as the +// first is the warning. So the warning is what this asserts. +func TestADeniedDescribeEventsLeavesTheLimitUnverifiedRatherThanCleared(t *testing.T) { + f := parityFixture() + seams, _ := liveSeamsFrom(f, nil) + seams.Envelope = &krds.EnvelopeFixture{ + Options: f.StorageOptions, + EventsErr: map[string]error{"db-gp3-fat": accessDenied("rds:DescribeEvents")}, + } + withLiveRDS(t, seams) + + var env reportEnvelope + runJSON(t, &env, liveArgs("report", "--rds-parity")...) + + dr, ok := env.Report.For(domain.RDS) + if !ok || dr.Health.Targets != 8 { + t.Fatalf("a denied DescribeEvents shrank the report: %+v", dr) + } + warns := warningsMentioning(env, "DescribeEvents") + if len(warns) == 0 { + t.Fatalf("an unreadable modification history was reported as an empty one: %v", env.Warnings) + } + var stated bool + for _, w := range warns { + if strings.Contains(w, "unverified rather than cleared") { + stated = true + } + } + if !stated { + t.Errorf("the warning does not say the limit is unverified rather than cleared: %v", warns) + } +}