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) + } +}