diff --git a/go.mod b/go.mod index 2dae0c3..3900aee 100644 --- a/go.mod +++ b/go.mod @@ -5,9 +5,11 @@ go 1.26.4 require ( github.com/aws/aws-sdk-go-v2/config v1.32.29 github.com/aws/aws-sdk-go-v2/service/autoscaling v1.69.0 + github.com/aws/aws-sdk-go-v2/service/cloudwatch v1.67.1 github.com/aws/aws-sdk-go-v2/service/ec2 v1.316.0 github.com/aws/aws-sdk-go-v2/service/pricing v1.43.0 - github.com/aws/smithy-go v1.27.3 + github.com/aws/aws-sdk-go-v2/service/rds v1.124.5 + github.com/aws/smithy-go v1.27.10 github.com/prometheus/client_golang v1.23.2 go.etcd.io/bbolt v1.5.0 golang.org/x/term v0.45.0 @@ -18,14 +20,14 @@ require ( ) require ( - github.com/aws/aws-sdk-go-v2 v1.42.1 // indirect + github.com/aws/aws-sdk-go-v2 v1.43.8 // indirect github.com/aws/aws-sdk-go-v2/credentials v1.19.28 // indirect github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.18.30 // indirect - github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.30 // indirect - github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.30 // indirect + github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.39 // indirect + github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.39 // indirect github.com/aws/aws-sdk-go-v2/internal/v4a v1.4.31 // indirect - github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.13.13 // indirect - github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.13.30 // indirect + github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.13.18 // indirect + github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.13.39 // indirect github.com/aws/aws-sdk-go-v2/service/signin v1.4.0 // indirect github.com/aws/aws-sdk-go-v2/service/sso v1.32.0 // indirect github.com/aws/aws-sdk-go-v2/service/ssooidc v1.37.0 // indirect diff --git a/go.sum b/go.sum index f1ae776..f42a855 100644 --- a/go.sum +++ b/go.sum @@ -1,27 +1,31 @@ -github.com/aws/aws-sdk-go-v2 v1.42.1 h1:9eOTgu1z/dVtYpNZ3/8/XbbaX0x/BqE3HUzAzs6K0ek= -github.com/aws/aws-sdk-go-v2 v1.42.1/go.mod h1:5pKeft2eJj+gElQ38Jqg4ibCqh+/AK33/0X3hip7IjM= +github.com/aws/aws-sdk-go-v2 v1.43.8 h1:fpnrxwuwsoGIgjvgLeDU3y9w7YaHBxyF6AF3vQL8duw= +github.com/aws/aws-sdk-go-v2 v1.43.8/go.mod h1:j7gYSq8dL95QejkFXxvQNESH4I9WGHFI6iO+vhqEi5Q= github.com/aws/aws-sdk-go-v2/config v1.32.29 h1:BcMHHnpiWKogf+gGfpj3K1w+Sktz29XDo/cPSAPO3FU= github.com/aws/aws-sdk-go-v2/config v1.32.29/go.mod h1:+Kbhn8Es4kPUph3F/0W7avykytc+Jh2Ld9/msv9ljV4= github.com/aws/aws-sdk-go-v2/credentials v1.19.28 h1:zTXJSsNcoO91/mTXsZoYf0AK8dvNPiA58/VtyGXR+wM= github.com/aws/aws-sdk-go-v2/credentials v1.19.28/go.mod h1:Kd9E0JzDBW/q1xbsHFrev/GnbAf5J0Ng8xoyc7HZ91Q= github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.18.30 h1:/hi1JADLEW9YYryEz1w4GQu0EtP23pP553Cf9KgsDV4= github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.18.30/go.mod h1:/3AOgy4K17Dm4ucMZVC/MJkzy5kmfKUcINRHZyo0koQ= -github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.30 h1:xM/Is9cKMHa8Jj8zkvWhvrFkZsXJV9E+BB4g0HW0duQ= -github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.30/go.mod h1:WueJeNDZvK1fMYEWJIkcivBfEzUkTpBhzlrUKKY8EuA= -github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.30 h1:jn46zC9LdsVR/ZpMIJqMqb8hHv31BlLx3ulVqNspUOk= -github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.30/go.mod h1:1hTMsAgbdS/AtUi4bw8+gUuh1pceo+eXRLfpSuSQj3M= +github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.39 h1:YrEI22hVQcqMpq934ZoPQyJjGNzX4CGdrSDCjBD59sI= +github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.39/go.mod h1:N8qOX83LkaCeizvrfiNjwkBOXkxHt6a74CiZn8qz9F8= +github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.39 h1:Vo7UZzBjB6zS6feEOuBlpEgaj8iBTdiNlye+7w9ooGo= +github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.39/go.mod h1:JgxtAO/77e95Rs9WMWUzz99hT182gqdAh7/DHuEMA/k= github.com/aws/aws-sdk-go-v2/internal/v4a v1.4.31 h1:3GUprIsfmGcC5SACIyB0e7E0BM1O1b3Erl5CePYIAeQ= github.com/aws/aws-sdk-go-v2/internal/v4a v1.4.31/go.mod h1:7PuV1yl5e2xnUbm+RqvVg5i2iBM8EyijZNoI9wsOoOc= github.com/aws/aws-sdk-go-v2/service/autoscaling v1.69.0 h1:4FLdMjJIMLOM/yiwXRj250GvDNVy9rc3J1Ho3p8KGOk= github.com/aws/aws-sdk-go-v2/service/autoscaling v1.69.0/go.mod h1:sN7IK8djnxCOQDGVhOvUlIA83i1wIA5jYnzr2TlY9a8= +github.com/aws/aws-sdk-go-v2/service/cloudwatch v1.67.1 h1:qlTstHvBYwBv85OqvLYMM94u7KslWf3ky7F3Z4Rr1fs= +github.com/aws/aws-sdk-go-v2/service/cloudwatch v1.67.1/go.mod h1:/NFCPktZlN69wBh+I9F/+fhKwcIabJr8432iSX5vJNU= github.com/aws/aws-sdk-go-v2/service/ec2 v1.316.0 h1:LlxNun/oe5B2XMff8Mkh/3bJeHl1K7Fod+rMYtjagw4= github.com/aws/aws-sdk-go-v2/service/ec2 v1.316.0/go.mod h1:eoF0SIRbTgKWnTcTPYckiURPba/7ilfEkvwL4V1iHK4= -github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.13.13 h1:mbRIur/BiHK6SKPjoBIXSE/hJ6g6JGRLuxQy1jGjlN4= -github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.13.13/go.mod h1:ITg9em2KbJx1s0y4aqRX5OYWG6HBZ5TVR//OdpEZ2CQ= -github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.13.30 h1:/Z5jmNrKsSD7EmDjzAPsm/3L9IuOkzaynklJZ1qX7S4= -github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.13.30/go.mod h1:lEzEZnOosE7zi8Z6royW1cFJTD9fpab4Ul1SBrllewk= +github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.13.18 h1:+fiwOxNdE8bOK3SoVTln8hwP+OCyArbi2/InIr/A9AU= +github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.13.18/go.mod h1:aua4m7EZSvQra/96b8zJxWHwtHxuXQ8bx4DiM92V044= +github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.13.39 h1:inoUrqz4Lfpw1XwpUvQnBiAJ2tUzn3opZ0gduNLxo+8= +github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.13.39/go.mod h1:Yx+RrmAF+XGZTccwhQ3o4K5V8qkZBsTAcq148Y8g57k= github.com/aws/aws-sdk-go-v2/service/pricing v1.43.0 h1:hcoV2glaN+puMGz888vv+slu9mBT3BBAn6I6ZsiMcDg= github.com/aws/aws-sdk-go-v2/service/pricing v1.43.0/go.mod h1:fX2p8/1nehWSKdUTLWSp/gPCxOUSCaBk2r2wjDoyxf4= +github.com/aws/aws-sdk-go-v2/service/rds v1.124.5 h1:VvPoTSn3ChcV2YQ9lEbfdaTC7oqj8ceIu+C9YwtRn/M= +github.com/aws/aws-sdk-go-v2/service/rds v1.124.5/go.mod h1:6P8IPbrkZap+zDVKTQtcfNXP1n2sR9x5yHFQgt2iEkk= github.com/aws/aws-sdk-go-v2/service/signin v1.4.0 h1:sLzmJGCMv+C8KqiJgEqDLB6vxaJGmobRh4rr//ZpA3w= github.com/aws/aws-sdk-go-v2/service/signin v1.4.0/go.mod h1:mxC0nT/C8wMMS97DemZPzvUZxvIt+2Iq+eS3JdFZGgg= github.com/aws/aws-sdk-go-v2/service/sso v1.32.0 h1:qjMmry/cBDee1E/2gyvel0uRYCi3mwRZ2hf6N+GAodo= @@ -30,8 +34,8 @@ github.com/aws/aws-sdk-go-v2/service/ssooidc v1.37.0 h1:fpOlDPI55HdszaxapEGk6HsG github.com/aws/aws-sdk-go-v2/service/ssooidc v1.37.0/go.mod h1:DMPWJBjYs6+3+f/qhBFEFPPlQ6NlhWjai3dJNvipJ84= github.com/aws/aws-sdk-go-v2/service/sts v1.44.0 h1:bLZ0PolJ8J+HkJHztcXORUpHXBye2U8298lCEMi6ZCU= github.com/aws/aws-sdk-go-v2/service/sts v1.44.0/go.mod h1:9gdl4RrflIdpDb2TlXshWgR1F9TeCkvqDx77Vpr4Z/Q= -github.com/aws/smithy-go v1.27.3 h1:F3Zb497UhhskkfpJmfkXswyo+t0sh9OTBnIHjogWbVY= -github.com/aws/smithy-go v1.27.3/go.mod h1:YE2RhdIuDbA5E5bTdciG9KrW3+TiEONeUWCqxX9i1Fc= +github.com/aws/smithy-go v1.27.10 h1:bw56MIx8bhTQZSdzucEJSKWLpwX0ju7hU8cVoa75dg8= +github.com/aws/smithy-go v1.27.10/go.mod h1:YE2RhdIuDbA5E5bTdciG9KrW3+TiEONeUWCqxX9i1Fc= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= diff --git a/pkg/provider/RDS-ADAPTER-FINDINGS.md b/pkg/provider/RDS-ADAPTER-FINDINGS.md new file mode 100644 index 0000000..44ebabe --- /dev/null +++ b/pkg/provider/RDS-ADAPTER-FINDINGS.md @@ -0,0 +1,637 @@ +# Q4 — the live RDS SDK adapter: a field copy, and the places a field copy is not enough + +`pkg/rds` shipped a complete, tested, read-only collector whose only missing +piece was the translation between an AWS SDK struct and a struct with the same +field names. `cmd/WIRING-FINDINGS.md` §6.1 named the gap and named the blocker: +`github.com/aws/aws-sdk-go-v2/service/rds` and `.../service/cloudwatch` were not +in `go.mod`. This unit adds those two modules and writes the translation. + +Everything else was already proven. `--rds-fixture` drives the REAL collector — +real window clamp, real `GetMetricData` batching and ID routing, real pagination +across three pages, real truncation — so the value of this unit is precisely +that it adds no judgement. What it could not avoid adding is a decision about +what an *unset* SDK field means. §4 records every one; five of them change what +a report says, and two of those five change a verdict. + +**What landed** + +| File | What | +|---|---| +| `pkg/provider/rdsapi.go` | `RDSAPI` over `*rds.Client`. Implements `rds.InventoryAPI`, `rds.CommitmentAPI` and `rds.ModificationEnvelopeAPI` — six read operations. `IsAccessDenied`, `noteSet`. | +| `pkg/provider/cloudwatchapi.go` | `CloudWatchAPI` over `*cloudwatch.Client`. Implements `rds.MetricsAPI` — one read operation. | +| `pkg/provider/rdsapi_test.go` | 26 tests against a faked SDK client. No credential, no `~/.aws`, no `AWS_*`, no socket. | +| `pkg/provider/cloudwatchapi_test.go` | 11 tests, same rule, including the whole collector driven end to end through both adapters into `rds.Domain.Report`. | +| `go.mod` / `go.sum` | `service/rds v1.124.5`, `service/cloudwatch v1.67.1`, and the version bumps those two force on lines already present. Nothing else added, nothing removed. | + +--- + +## 1. The seams, and every operation on them + +`pkg/rds` declares four interfaces. `RDSAPI` implements three of them and +`CloudWatchAPI` the fourth. One concrete type covers the three `rds:` seams +because one credential answers all three — exactly as `rds.Fixture` does — and +they stay separate *interfaces* at the pkg/rds boundary for the reason pkg/rds +gives: a caller may hold one permission and not another, and the right +behaviour then is a degraded report, not a missing one. + +| Interface | Method | AWS API | Paginated | IAM action | Required? | +|---|---|---|---|---|---| +| `rds.InventoryAPI` | `DescribeDBInstances` | `rds:DescribeDBInstances` | Marker | `rds:DescribeDBInstances` | **yes** | +| | `DescribeDBClusters` | `rds:DescribeDBClusters` | Marker | `rds:DescribeDBClusters` | **yes** (degrades, §3) | +| | `ListTagsForResource` | `rds:ListTagsForResource` | no | `rds:ListTagsForResource` | **yes** | +| `rds.MetricsAPI` | `GetMetricData` | `cloudwatch:GetMetricData` | NextToken | `cloudwatch:GetMetricData` | optional | +| `rds.CommitmentAPI` | `DescribeReservedDBInstances` | `rds:DescribeReservedDBInstances` | Marker | `rds:DescribeReservedDBInstances` | optional | +| `rds.ModificationEnvelopeAPI` | `DescribeValidDBInstanceModifications` | same | no | `rds:DescribeValidDBInstanceModifications` | optional (U13) | +| | `DescribeEvents` | `rds:DescribeEvents` | Marker | `rds:DescribeEvents` | optional (U13) | + +`ModificationEnvelopeAPI` **does** exist — it landed in `pkg/rds/parity_envelope.go` +with U13, after `pkg/rds/FINDINGS.md` §7.5 said it would not — so it is +implemented here rather than deferred. U13 reads the envelope through this seam +and a missing envelope reaches the caller as "not answered": see §4.4, which is +the single most consequential line in this unit. + +Seven operations, all GETs. `TestNoMutatingSDKSurface` walks both SDK +interfaces by reflection and fails on any method whose name begins with a +mutating verb, so `rds:ModifyDBInstance` cannot arrive here by accident any more +than it can arrive in `pkg/rds`. + +### 1.1 Pagination is per-call, not internal + +Every list operation paginates, and the adapter's share of that is to propagate +the token faithfully in **both** directions: the caller's `Marker` in, AWS's +`Marker` out. The page **loop** stays in `pkg/rds`, which owns it along with +`CollectorConfig.MaxPages`, the per-page `ctx.Err()` check and the warning that +says an inventory was truncated. An adapter that looped internally would defeat +all three and would return one enormous page that no budget bounds. + +`TestPaginationIsPropagatedThroughEverySeam` drives the real collector over a +7-instance / 5-reservation fake at `pageSize: 3` and asserts 3 and 2 pages +respectively, with nothing lost. +`TestSwallowingAMarkerWouldTruncateSilently` is its negative: it shows that a +dropped `Marker` turns a partial inventory into a report that reads as complete. + +### 1.2 Every call carries a timeout + +`DefaultRDSCallTimeout = 30s`, `DefaultMetricsCallTimeout = 60s` — the metrics +call is longer because `pkg/rds` batches up to 500 queries into one request. +Both are `context.WithTimeout` on the caller's context, so the bound only ever +shortens the deadline and a cancelled parent still cancels +(`TestParentCancellationStillPropagates`). `SetCallTimeout` overrides; a +non-positive value restores the default rather than disabling the bound. +`TestEveryCallCarriesADeadline` asserts all six `rds:` calls arrive with +`ctx.Deadline()` set. + +--- + +## 2. The exact least-privilege IAM policy + +Two statements, because the two required-vs-optional halves are the whole point +of the seam split. Split them into separate policies if you want to grant the +required half and withhold the rest. + +```json +{ + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "KilterRDSInventoryRequired", + "Effect": "Allow", + "Action": [ + "rds:DescribeDBInstances", + "rds:DescribeDBClusters", + "rds:ListTagsForResource" + ], + "Resource": "*" + }, + { + "Sid": "KilterRDSOptionalDegradesWithoutThese", + "Effect": "Allow", + "Action": [ + "cloudwatch:GetMetricData", + "rds:DescribeReservedDBInstances", + "rds:DescribeValidDBInstanceModifications", + "rds:DescribeEvents" + ], + "Resource": "*" + } + ] +} +``` + +Notes on the policy, and what is deliberately not claimed: + +- **`rds:ListTagsForResource` is required, not a nicety.** The `kilter.dev/mode` + opt-out lives in a tag (`pkg/rds/rds.go:184`, `ModeOff`). Without this action + `readTags` fails, the collector emits the warning "the `kilter.dev/mode` + guardrail cannot be evaluated for it, so an opt-out tag on that instance would + not be honoured", and an operator who tagged a database to be left alone is + not obeyed. Granting the two describes without this one buys a report that + quietly ignores opt-outs. +- **`cloudwatch:GetMetricData` is not resource-scopable.** CloudWatch has no + resource-level permissions for it; `"Resource": "*"` is the only form that + works. It is also the only action here that can cost money at scale — one + `GetMetricData` request per 500 queries, and `pkg/rds` issues + `len(CollectedMetrics()) == 11` queries per instance. `rds.CollectedMetrics()` + is exported so an operator can size that bill without reading the source. +- `rds:ListTagsForResource` and `rds:DescribeValidDBInstanceModifications` act on + a single instance and can be narrowed to + `arn:aws:rds:::db:*`. Whether the *list* actions accept + resource-level ARNs varies by action and has changed over time + [unverified: which of the `rds:Describe*` actions accept resource-level ARNs + in the current IAM reference]. `"Resource": "*"` is correct for all of them, + so it is what is published here. +- No `rds:Modify*`, no `rds:Reboot*`, no `rds:Create*`, no `rds:Delete*`. This + policy grants nothing that can change an account. `TestNoMutatingSDKSurface` + is the code-side half of the same statement. + +--- + +## 3. What degrades, and what fails + +This is the table cmd/ has to read before it decides which seams to wire, and +it is not symmetric. Getting it wrong is how an optional permission becomes a +failed run. + +| Missing / failing | Behaviour today | Where | +|---|---|---| +| `DescribeDBInstances` fails | **Collection fails.** The one hard dependency: with no inventory there is nothing to report on. | `collect.go` `describeInstances` | +| `DescribeDBClusters` fails | Warning; members are still excluded, but under the more cautious `cluster-member-not-supported` rather than `aurora-not-supported`. | `describeClusters` | +| `ListTagsForResource` fails | Warning naming the instance; the `kilter.dev/mode` guardrail is unevaluated for it. | `readTags` | +| **`MetricsAPI` is nil** | Complete report; every instance refuses with `no-metric-evidence`. | `readMetrics` | +| **`GetMetricData` *fails* mid-run** | **Collection fails.** Not the same as nil. | `collect.go` `readMetrics`: `return fmt.Errorf("rds: get metric data: %w", err)` | +| `CommitmentAPI` is nil | Complete report; net == gross. | `describeReservations` | +| `DescribeReservedDBInstances` fails | Warning: "net savings equal gross in this report, which under-claims". | same | +| `ModificationEnvelopeAPI` is nil | Every provisioning proposal refuses with `provisioning-envelope-unknown`. | `parity_envelope.go` `Collect` | +| `DescribeValidDBInstanceModifications` fails | Per-instance warning; that instance's envelope stays unknown; its proposals refuse. | same | +| `DescribeEvents` fails | Per-instance warning; `HistoryKnown=false`; the four-per-24-hours limit is reported unverified rather than cleared. | `history` | + +**The asymmetry that matters: a nil `MetricsAPI` degrades, a failing one does +not.** `pkg/rds/FINDINGS.md` §6.2 calls `cloudwatch:GetMetricData` optional, and +it is — but only in the nil form. A credential that lacks the permission and is +wired anyway produces `AccessDeniedException` from the first call and +`Collect` returns an error, so the operator gets no report at all rather than +the documented degraded one. + +That is why `IsAccessDenied` is exported. It matches **only** permission +denials — `AccessDenied`, `AccessDeniedException`, `UnauthorizedOperation`, +`AuthorizationError`, `AuthFailure`, `NotAuthorized`, `Forbidden`. A throttle, a +timeout or a malformed request stays an error, because swallowing those would +turn a transient fault into a permanently degraded report that claims the +credential lacks a permission it actually holds +(`TestIsAccessDenied` pins both halves). + +§6.3 gives cmd/ the retry that turns the failure into the degradation. + +### 3.1 `Notes()` — degradations the seam has no field for + +Three facts the adapter learns cannot be expressed through the seam's return +types, because the seam structs have no field for them, and all three are +silent in `pkg/rds`: an instance with neither ARN nor identifier (dropped by +`recordID`), a tag with no key, an event with no date. Both adapters therefore +carry a deduplicated, sorted, mutex-guarded `Notes() []string`, and **cmd/ must +render it beside `Snapshot.Warnings`** (§6.2). A degradation nobody can see is a +degradation that did not happen. + +`Notes()` returns `nil` on a clean run, so wiring it costs nothing when there is +nothing to say. + +--- + +## 4. Every nilable field, and what its absence was decided to mean + +The rule: an unset SDK pointer is *unknown*, not zero. Where the destination +field can carry that distinction, it does. Where it cannot — and `pkg/rds`'s +records are deliberately plain scalars — the decision is stated, its direction +is argued, and it is tested. + +### 4.1 `DBInstance` — `rds:DescribeDBInstances` + +| SDK field | Type | nil ⇒ | Why that is the safe reading | +|---|---|---|---| +| `DBInstanceIdentifier`, `DBInstanceArn` | `*string` | `""` | With both empty the record is unaddressable and `pkg/rds` skips it. Faithful copy **plus a note**, because pkg/rds's skip is silent. | +| `DBInstanceClass`, `DBInstanceStatus`, `Engine`, `EngineVersion`, `LicenseModel`, `AvailabilityZone`, `StorageType` | `*string` | `""` | `pkg/rds` already refuses an unknown engine (`unknown-engine`) and an empty class by name. Empty is the refusing value, not a plausible one. | +| `MultiAZ` | `*bool` | `false` | **The one that moves money.** `DBInstance.Deployment()` maps false → `RDSSingleAZ`, a ×1 rather than ×2 multiplier. The destination is a `bool` and cannot hold "unknown". False *under*-states the instance line, and an under-stated line can only under-state a saving derived from it — the direction this package is built to fail in. `TestNilMultiAZUnderStatesRatherThanOverStates`. | +| `DBClusterIdentifier` | `*string` | `""` | Empty means "not a cluster member", which is what AWS's omission means. | +| `ReadReplicaSourceDBInstanceIdentifier` | `*string` | `""` | Empty on a primary, per the seam's own doc comment. | +| `ReadReplicaDBInstanceIdentifiers` | `[]string` | `nil`, blanks dropped | Carrying `""` forward would invent a replica named `""`. | +| `AllocatedStorage`, `MaxAllocatedStorage` | `*int32` | `0`, widened to `int64` | A nil `MaxAllocatedStorage` is AWS's own encoding of "storage autoscaling is off", which is exactly what 0 means to `pkg/rds`. The widening is lossless: no value RDS issues approaches the int32 ceiling. | +| `Iops`, `StorageThroughput` | `*int32` | `0` | Absent on gp2/standard because they are **not provisionable** there. 0 is the honest reading of that absence, and `pkg/rds` reads 0 as "not provisioned", never as "measured zero". | +| `InstanceCreateTime` | `*time.Time` | zero time | `pkg/rds` uses it only for an age display. | +| `TagList` | `[]Tag` | `nil` | An empty map and a nil map both trigger `pkg/rds`'s `ListTagsForResource` fallback, which is the behaviour you want. | +| `Tag.Key` | `*string` | entry dropped **+ note** | A tag named `""` is not a tag. The note names the instance and says out loud that if the dropped tag was `kilter.dev/mode`, the opt-out is not honoured. | +| `Tag.Value` | `*string` | `""` | A nil value is AWS's encoding of a legal empty-valued tag, and `""` is not `"off"`, so the guardrail reads it correctly. | + +`TestEveryNilableInstanceFieldHasADecision` asserts by reflection that every +field this adapter reads still exists on the SDK struct — so an upstream rename +breaks the test instead of silently zeroing a column — and then feeds an +entirely unset `DBInstance` through and requires the exact zero record plus the +unaddressable-instance note. + +### 4.2 `DBCluster` — `rds:DescribeDBClusters` + +`ServerlessV2ScalingConfiguration` is a nil **struct pointer** on every cluster +that is not Serverless v2, so `ServerlessV2MinCapacity`/`MaxCapacity` stay 0. +That is free: `ClusterInfo.ServerlessV2MinACU` is carried so the Aurora refusal +can *name* the lever a future unit would look at, and `pkg/rds` never does +arithmetic with it. `DBClusterMembers` entries with a nil +`DBInstanceIdentifier` are dropped for the same reason blank replica +identifiers are. + +### 4.3 `ReservedDBInstance` — `rds:DescribeReservedDBInstances` + +`FixedPrice`, `UsagePrice`, `Duration` and `MultiAZ` are copied **raw**. The +amortization (`EffectiveHourly = UsagePrice + FixedPrice ÷ term hours`), the +`active`/`payment-pending` filter and the topology mapping all live in +`pkg/rds.reservationFromRecord` and are not repeated here — that is +`FINDINGS.md` §6.2's explicit instruction, and +`TestReservationFieldsAreCopiedRawAndAmortizedOnlyByPkgRDS` proves the +arithmetic runs exactly once by asserting a raw `FixedPrice: 8760, Duration: +31536000` becomes `EffectiveHourlyUSD == 1.05`, not `2.05` and not `1.00`. + +Three nils get notes rather than repair, because in each case `pkg/rds` has +already chosen and its choice is defensible — but invisible: + +| nil | `pkg/rds` does | Note says | +|---|---|---| +| `DBInstanceCount` | `count <= 0` ⇒ the reservation is **dropped**, not guessed at 1 | "it is dropped rather than counted as one, so it contributes no coverage to this report" | +| `State` | `case "", "active", "payment-pending"` ⇒ treated as **billing** | "a retired reservation counted as live can only make a saving smaller, never larger" | +| `Duration` with a non-zero `FixedPrice` | keeps the usage price alone; the upfront is unamortizable and dropped | "under-stating this reservation's cost and therefore under-stating stranding" | + +`Duration` is `*int32` **seconds** in the SDK and `int64` in the record; the +longest RDS term is three years (94,608,000 s), so the widening is lossless. + +### 4.4 `ValidStorageOptions` — `rds:DescribeValidDBInstanceModifications` + +**This is the decision that changes a verdict, and it is the classic +nil-becomes-zero bug in its most expensive form.** + +AWS answers in ranges — `ProvisionedIops []Range` of `{From, To, Step}`, all +`*int32`. `rds.ValidStorageOptionRecord` carries one overall minimum and +maximum per dimension, and its own doc comment says that reduction is what an +adapter should perform ("AWS returns ranges; this reduces each to its overall +minimum and maximum"). `reduceRanges` does exactly that, skipping nil bounds +rather than reading them as 0. + +The trap is what happens next. `pkg/rds/parity.go:487` enforces the ceiling as: + +```go +if env.MaxIOPS > 0 && c.IOPS > env.MaxIOPS { ... refuse ... } +``` + +so a `StorageEnvelope` that is `Known` with `MaxIOPS == 0` has **no ceiling**, +not an unknown one. And `envelopesFromRecords` sets `Known = true` for any +record carrying a non-empty storage type. Emitting a record whose ranges AWS +never filled would therefore convert *"AWS did not tell us the ceiling"* into +*"this instance has no ceiling"* — and an 80,000-IOPS proposal would pass +validation on an instance capped at 16,000, which is precisely the contradiction +`FINDINGS.md` §2.4 refuses to resolve by guessing. + +So: **a storage type is emitted only when AWS named a positive upper bound for +both provisionable dimensions.** Otherwise the record is omitted, `Envelope.For` +returns `Known=false`, and the proposal is refused under its own name, +`provisioning-envelope-unknown`. `StorageEnvelope`'s doc comment asks for +exactly this — "A record whose ranges are empty is an UNKNOWN envelope, not a +zero one" — and omission is the only way the seam lets an adapter say it. + +This over-refuses for storage types where nothing is provisionable in the first +place (gp2, standard, and io1's throughput), which costs nothing: `pkg/rds` only +ever looks up gp3 (`env.For(StorageGP3)` at `parity_assess.go:472` and `:563`). +Every omission carries a note. + +`TestEnvelopeWithNoReadableCeilingStaysUnknown` covers four shapes — no ranges, +a nil upper bound, an absent throughput list, and `To: 0` — and asserts through +the real `EnvelopeCollector` that `Known` stays false in all four. Deleting the +gate makes all four fail. + +`Range.Step` has nowhere to go: the record carries a min and a max and nothing +else. A step larger than 1 is noted, because a value inside the range but off +the step passes this package's check and is rejected by AWS at apply time. + +An answer with a nil `ValidDBInstanceModificationsMessage` returns an **empty +output plus a note**, not an error: empty leaves every envelope unknown, which +is the refusing default. + +### 4.5 `MetricDataResult.StatusCode` — `cloudwatch:GetMetricData` + +The same trap in string form. `pkg/rds/collect.go` reads: + +```go +ser.Status = r.StatusCode +if ser.Status == "" { ser.Status = StatusComplete } +ser.Partial = ser.Status != StatusComplete +``` + +so an unset status becomes **Complete** and the series is treated as whole +evidence. Passing `""` through would turn "CloudWatch did not vouch for this +series" into "this series is complete", and a complete-looking +`DatabaseConnections` series is exactly what an idle verdict is made of. + +**Decision: an unset `StatusCode` is reported as `PartialData`**, plus a note. +This is not an invention — it is the same reading `pkg/rds` itself applies to a +result that never arrived at all (`StatusTruncated`: "a MISSING result means the +response was truncated, which is a fact about the response, not about the +metric"). `Complete`, `PartialData`, `InternalError` and `Forbidden` all pass +through verbatim; only the empty string is resolved, and it is resolved away +from evidence. `TestUnsetStatusBecomesPartialNotComplete` checks both the seam +value and the resulting `Series.Partial` through the real collector. + +`MetricDataResult.Id` nil is copied as `""` **plus a note**: `pkg/rds` routes by +ID, so an unidentified result lands in a slot no query owns and the real query +stays unanswered — which produces a `Truncated`, refusing series. Safe, and +invisible without the note. + +### 4.6 `Event.Date` — `rds:DescribeEvents` + +Nil becomes the zero time, which `Envelope.Cooldown` cannot place inside the +trailing 24-hour window, so an **undated storage modification does not count** +toward the four-per-24-hours limit. `pkg/rds` calls under-counting the worse of +the two errors by name — "over-counting delays a proposal by hours, +under-counting proposes a change AWS will reject" — so the adapter cannot repair +it (it has no date to supply) but it does say so. See §7.3 for the pkg/rds fix. + +### 4.7 Outbound nils + +Symmetric, and for the same reason: an empty seam field must stay unset on the +wire rather than become an explicit empty filter. + +- `Marker`/`NextToken`/`SourceIdentifier` empty ⇒ `nil`. +- `MaxRecords == 0` ⇒ `nil`. Sending 0 would have AWS reject a request the + caller never made — the valid range is 20–100 (`TestMaxRecordsIsOnlySentWhenAsked`). +- `StartTime`/`EndTime` zero ⇒ `nil` on `DescribeEvents`; sending a zero + `time.Time` means the year 1 and fails the whole request. +- `GetMetricData` **requires** both times, so a zero or inverted window is + refused client-side with a message that names the window, rather than sent + and answered with a validation error that reads like a permissions problem. + A zero `PeriodSeconds` is refused the same way. +- An empty query list makes **no call at all** (CloudWatch rejects it) and + returns an empty output. Nothing was asked, so nothing was answered. +- `MetricStat.Unit` is deliberately left unset. Naming a unit *filters* + datapoints to those published with it, so a wrong guess returns an empty + series — which reads as a quiet database. +- Dimensions are sorted by name. Go randomizes map iteration, and an unsorted + dimension list makes two runs over the same account send two different + requests, which defeats every replay and diff in this tree + (`TestDimensionsAreOrderedSoTwoIdenticalCollectionsIssueIdenticalRequests` + runs 25 iterations). + +--- + +## 5. What this adapter deliberately does not do + +- **No amortization.** §4.3. +- **No topology derivation.** `MultiAZ` in, `MultiAZ` out; + `DBInstance.Deployment()` decides. +- **No engine normalization.** `ProductDescription` is copied raw; + `commit.NormalizeRDSEngine` runs inside `pkg/rds`. +- **No retry, no backoff.** The SDK's own standard retryer handles throttling. + Adding a second layer here would multiply against the per-call timeout. +- **No `ScanBy`, no `MaxDatapoints`.** Both are left at CloudWatch's defaults — + see §7.1, where the reason is that choosing either would be answering a + `pkg/rds` question from the wrong side of the seam. +- **No live call anywhere in the test suite.** Every test drives a fake client. + `NewRDSAPI`/`NewCloudWatchAPI` reject a blank region *before* calling + `LoadDefaultConfig`, so even the constructor tests read no credential. + +--- + +## 6. The exact wiring `cmd/` must do + +`cmd/kilter/rds.go` currently holds `collectRDS(ctx, path, …)` over +`--rds-fixture`, and `cmd/kilter/domains.go:317` loops over `df.rdsFixtures`. +The live path is a sibling, not a replacement: **keep `--rds-fixture`**. It is +how the collector is tested without an account, and `cmd/kilter/rds_test.go` +depends on it. + +### 6.1 One new flag, one new function + +```go +// cmd/kilter/domains.go, beside --rds-fixture +fs.Var(&df.rdsRegions, "rds-region", + "collect RDS live from this region (requires AWS credentials; repeatable)") +``` + +```go +// cmd/kilter/rds.go +func collectRDSLive(ctx context.Context, scope, region string, + now time.Time, span time.Duration) (*krds.Snapshot, []string, error) { + + inv, err := provider.NewRDSAPI(ctx, region) + if err != nil { + return nil, nil, err + } + cw, err := provider.NewCloudWatchAPI(ctx, region) + if err != nil { + return nil, nil, err + } + + // pkg/rds/FINDINGS.md §6.3. The window is clamped by the collector, so + // c.Window() is what was observed and the request is not. + cfg := krds.DefaultCollectorConfig(krds.Window{Start: now.Add(-span), End: now}) + cfg.Scope, cfg.Region = scope, inv.Region() // the SAME region the client talks to + + collect := func(metrics krds.MetricsAPI) (*krds.Snapshot, error) { + // inv is passed twice: once as InventoryAPI, once as CommitmentAPI. + // A denied DescribeReservedDBInstances already degrades to a warning + // inside pkg/rds, so the commitment seam never needs to be dropped. + c, err := krds.NewCollector(inv, metrics, inv, cfg) + if err != nil { + return nil, err + } + return c.Collect(ctx) + } + + var warnings []string + snap, err := collect(cw) + // The ONE seam that must be dropped rather than retried: a nil MetricsAPI + // degrades, a denied one fails the whole collection (§3). + if err != nil && provider.IsAccessDenied(err) && + strings.Contains(err.Error(), "get metric data") { + + warnings = append(warnings, region+": no cloudwatch:GetMetricData; every instance is "+ + "reported without CloudWatch evidence and refuses with no-metric-evidence") + snap, err = collect(nil) + } + if err != nil { + return nil, nil, fmt.Errorf("rds %s: %w", region, err) + } + + if got := c.Window(); got != cfg.Window { + warnings = append(warnings, fmt.Sprintf( + "%s: observation window clamped to %s (1-minute CloudWatch datapoints live %s)", + region, got.String(), krds.RetentionAtOneMinute)) + } + // §6.2 — the adapters' own notes, which the seam structs cannot carry. + for _, n := range append(inv.Notes(), cw.Notes()...) { + warnings = append(warnings, region+": "+n) + } + for _, w := range snap.Warnings { + warnings = append(warnings, region+": "+w) + } + return snap, warnings, nil +} +``` + +(Hoist the `*krds.Collector` out of the closure if you want `c.Window()` as +written; the fixture path in `collectRDS` already does exactly that.) + +Everything downstream is unchanged: `rdsDomain.Observe(snap)`, +`inv.ReservedDBs = append(inv.ReservedDBs, snap.Reservations...)`, and +`rep.Refusals()` — `cmd/kilter/domains.go:317` already does all three for the +fixture path, and a live snapshot is the same type. + +### 6.2 Render `Notes()` + +Not optional. Three degradations — an unaddressable instance, a keyless tag, an +undated modification — are **silent in `pkg/rds`** because its seam structs have +no field for them (§3.1). `Notes()` is where they surface, it returns `nil` when +there is nothing to say, and the snippet above folds it into `rt.Warnings` +alongside `snap.Warnings`. + +### 6.3 The access-denied retry, and why only one seam needs it + +The two optional seams degrade differently, and the difference is the whole +reason this section exists. + +**`rds:DescribeReservedDBInstances` needs nothing.** `describeReservations` +returns a warning string and never an error, so a denied read already produces +a complete report with "net savings equal gross in this report, which +under-claims". Wire it unconditionally. + +**`cloudwatch:GetMetricData` needs the retry.** It is optional *in the nil form +only*: `readMetrics` returns `fmt.Errorf("rds: get metric data: %w", err)` and +`Collect` propagates it, so a credential that lacks the permission and is wired +anyway produces no report at all rather than the documented degraded one. The +single retry in §6.1 converts that into the promised behaviour — every instance +refusing with `no-metric-evidence` — and `IsAccessDenied` is what keeps a +throttle or a timeout from being mistaken for a missing permission. + +The alternative is to let the operator declare what they hold — +`--rds-no-metrics` — which costs one fewer wasted collection and makes the +degradation an explicit statement rather than an inference. Either is honest. +**What is not acceptable is wiring the seam and letting the run die**, because +the operator then sees `AccessDeniedException` where the design promises a +complete report. + +`rds:DescribeDBClusters` needs no treatment either: `pkg/rds` already degrades +it to a warning. + +### 6.4 U13's envelope, when a proposal path wants it + +```go +ec := krds.NewEnvelopeCollector(inv, krds.EnvelopeCollectorConfig{ + Window: krds.Window{Start: now.Add(-48 * time.Hour), End: now}, +}) +envs, err := ec.Collect(ctx, identifiersFrom(snap)) +``` + +`inv` is the same `*provider.RDSAPI`. A window shorter than 24 h cannot answer +the cooldown question and the collector warns about it. Passing `nil` instead of +`inv` is legal and refuses every provisioning proposal by name. + +### 6.5 One collector per region + +Unchanged from every shipped domain: `cmd/` runs one `RDSAPI` + +`CloudWatchAPI` + `Collector` per region and merges the reports. Pair the two +adapters on the **same** region — a metric is published in the region its +database lives in, and a cross-region pairing returns empty series for every +instance, which reads as an account full of idle databases. Both constructors +require an explicit region for this reason and reject a blank one. + +--- + +## 7. Findings for `pkg/rds` — decisions that belong on the other side of the seam + +Out of scope for this unit by construction. Each is stated as the failure it +produces, in the style §8 of `pkg/rds/FINDINGS.md` uses. + +### 7.1 `GetMetricData` continuation results are dropped, not merged + +`collect.go`'s page loop is first-write-wins per query ID: + +```go +for _, r := range res.Results { + if _, dup := got[r.ID]; dup { continue } + got[r.ID] = r +} +``` + +CloudWatch caps a response at 100,800 datapoints (`MaxDatapoints`) across all +queries and pages the remainder. `pkg/rds` issues 11 queries per instance; at +60 s over 14 days each series is ~20,160 datapoints, so **one response holds +about five series** — half an instance. Every subsequent page repeats IDs +already seen, and `continue` discards their datapoints. + +The failure: a query whose data spans a page boundary keeps only the datapoints +from the page it first appeared on, and — because that page's `StatusCode` is +`Complete` — the resulting `Series` is **not marked partial**. A truncated +`DatabaseConnections` series that looks complete is the exact shape +`StatusTruncated`'s doc comment exists to prevent. + +The fix is one branch: append `Timestamps`/`Values` on a duplicate ID instead of +dropping, and the existing `sort.SliceStable` by timestamp already handles the +ordering. Until it lands, this adapter leaves `ScanBy` and `MaxDatapoints` at +CloudWatch's defaults — choosing a scan order would be picking *which* half of +a split series survives, which is a `pkg/rds` question and not an adapter's to +answer. [unverified: whether CloudWatch marks the results of a paginated +response `PartialData`; if it does, the series is at least honestly partial and +only the datapoints are lost.] + +### 7.2 `StorageEnvelope` cannot distinguish "no ceiling" from "unknown ceiling" + +`parity.go:487`'s `env.MaxIOPS > 0 &&` guard resolves the ambiguity toward +*permissiveness*, while `StorageEnvelope.Known` resolves it toward refusal. The +two disagree, and this adapter has to pick omission (§4.4) to keep the safe one. + +The consequence of omission is a refusal message that reads "…needs the range +`rds:DescribeValidDBInstanceModifications` reports, and it was not read. … +Supply the seam to `rds.NewEnvelopeCollector` to unblock it" — which is +**misleading** when the seam *was* supplied and AWS simply returned a partial +envelope. A `MaxIOPSKnown bool` beside `MaxIOPS` (or a sentinel of −1) would let +the adapter say "AWS answered, and named no ceiling" and let the refusal say so +too. + +### 7.3 `Envelope.Cooldown` silently ignores undated events + +An `EventRecord` with a zero `Date` is recognised by +`IsStorageModificationEvent` and then filtered out by `Cooldown`'s window test, +so it does not count. That inverts the recogniser's own stated bias toward +over-counting. An undated storage-modification event should set +`HistoryKnown = false` — "we cannot rule the limit out from this" — rather than +be quietly dropped. + +### 7.4 An empty `StatusCode` defaults to `Complete` + +`collect.go`'s `if ser.Status == "" { ser.Status = StatusComplete }` is the +string form of the nil-pointer trap. This adapter resolves it on its side +(§4.5), but the default belongs on the other side: every future CloudWatch +adapter — and there are four other domains that will each write one — has to +re-derive the same fix. Defaulting to `StatusPartialData` in `pkg/rds` would +make the safe reading the one you get for free. + +### 7.5 An unaddressable instance is dropped without a warning + +`describeInstances` skips a record whose `recordID` is empty, alongside caps and +page budgets that each emit a warning naming what is absent. This one does not, +so an instance can vanish from a report that otherwise says out loud when it is +incomplete. One `warns = append(...)` line. + +--- + +## 8. Things that would falsify parts of this unit + +1. **If AWS begins returning `MultiAZ` as nil on real instances**, §4.1's + under-stating fallback stops being a theoretical guard and starts halving + real instance lines. The honest fix is a `MultiAZKnown` field and a + `unknown-deployment` refusal, which `pkg/rds` already has a reason code for + (`ReasonUnknownDeployment`) but cannot reach through the record. +2. **If `DescribeValidDBInstanceModifications` returns a gp3 entry with + `ProvisionedIops` filled and `ProvisionedStorageThroughput` empty on a real + instance**, §4.4's both-dimensions gate over-refuses on a case that matters, + and the gate should narrow to IOPS alone. Nothing in the AWS documentation + settles this [unverified], so the conservative gate is what shipped. +3. **If CloudWatch marks paginated continuation results `PartialData`**, §7.1's + failure is reduced from "silent truncation" to "lost datapoints on a + correctly-flagged partial series", which is much less serious and lowers the + priority of the `pkg/rds` fix. +4. **If a future `pkg/rds` release adds a fifth seam that writes**, + `TestNoMutatingSDKSurface` fails here first, which is the intended order. diff --git a/pkg/provider/cloudwatchapi.go b/pkg/provider/cloudwatchapi.go new file mode 100644 index 0000000..ccec06d --- /dev/null +++ b/pkg/provider/cloudwatchapi.go @@ -0,0 +1,261 @@ +package provider + +// The live CloudWatch adapter for pkg/rds's metrics seam. +// +// This is the fifth independent derivation of a CloudWatch seam in this tree +// (pkg/ec2, pkg/ebs, pkg/ecs, pkg/lambda, pkg/rds) and the first that talks to +// the SDK. pkg/rds owns the query construction, the 500-per-call batching, the +// ID routing and the page loop; this file owns the type translation and one +// decision the translation cannot avoid — what an EMPTY status code means. +// +// pkg/rds/collect.go reads a delivered series like this: +// +// ser.Status = r.StatusCode +// if ser.Status == "" { ser.Status = StatusComplete } +// ser.Partial = ser.Status != StatusComplete +// +// so an unset status becomes "Complete" and the series is treated as whole +// evidence. That is the string form of the nil-pointer trap: "CloudWatch did +// not say" would become "CloudWatch said the series is complete", and a +// complete-looking DatabaseConnections series is exactly what an idle verdict +// is made of. This adapter therefore reports an unset status as PartialData — +// the same reading pkg/rds itself applies to a result that never arrived. + +import ( + "context" + "fmt" + "sort" + "strings" + "time" + + awsconfig "github.com/aws/aws-sdk-go-v2/config" + "github.com/aws/aws-sdk-go-v2/service/cloudwatch" + cwtypes "github.com/aws/aws-sdk-go-v2/service/cloudwatch/types" + + krds "github.com/agenticode/kilter/pkg/rds" +) + +// DefaultMetricsCallTimeout bounds one cloudwatch:GetMetricData call. It is +// longer than [DefaultRDSCallTimeout] because the request is much larger: +// pkg/rds batches up to 500 queries per call, each spanning the whole +// observation window. +const DefaultMetricsCallTimeout = 60 * time.Second + +// cloudwatchSDK is the minimal CloudWatch surface pkg/rds needs, satisfied by +// *cloudwatch.Client and by test fakes. One operation, and it is a GET. +type cloudwatchSDK interface { + GetMetricData(ctx context.Context, in *cloudwatch.GetMetricDataInput, + opts ...func(*cloudwatch.Options)) (*cloudwatch.GetMetricDataOutput, error) +} + +// CloudWatchAPI adapts *cloudwatch.Client to [krds.MetricsAPI]. +// +// The seam is OPTIONAL: pkg/rds accepts a nil MetricsAPI and every instance +// then refuses with no-metric-evidence, which is a complete report rather than +// a failed one. Note that a mid-collection GetMetricData FAILURE is not the +// same thing — pkg/rds returns it as a hard error from Collect. cmd/ must +// therefore decide before collecting whether the credential holds +// cloudwatch:GetMetricData; see RDS-ADAPTER-FINDINGS.md §5. +type CloudWatchAPI struct { + api cloudwatchSDK + region string + timeout time.Duration + notes noteSet +} + +var _ krds.MetricsAPI = (*CloudWatchAPI)(nil) + +// NewCloudWatchAPI loads AWS credentials from the environment and targets one +// region — the SAME region as the [RDSAPI] it is paired with, because a metric +// is published in the region its database lives in and a cross-region pairing +// silently returns empty series for every instance. +func NewCloudWatchAPI(ctx context.Context, region string) (*CloudWatchAPI, error) { + region = strings.TrimSpace(region) + if region == "" { + return nil, fmt.Errorf("provider cloudwatch: region required: RDS metrics are published in " + + "the region the database lives in, and a mismatched region returns empty series that read " + + "as an idle database") + } + cfg, err := awsconfig.LoadDefaultConfig(ctx, awsconfig.WithRegion(region)) + if err != nil { + return nil, fmt.Errorf("provider cloudwatch: load aws config: %w", err) + } + return newCloudWatchAPI(cloudwatch.NewFromConfig(cfg), region), nil +} + +// newCloudWatchAPI is the test seam. +func newCloudWatchAPI(client cloudwatchSDK, region string) *CloudWatchAPI { + return &CloudWatchAPI{api: client, region: region, timeout: DefaultMetricsCallTimeout} +} + +// Region is the region this adapter reads. +func (a *CloudWatchAPI) Region() string { return a.region } + +// SetCallTimeout overrides the per-call deadline. A non-positive value +// restores [DefaultMetricsCallTimeout]. +func (a *CloudWatchAPI) SetCallTimeout(d time.Duration) { + if d <= 0 { + d = DefaultMetricsCallTimeout + } + a.timeout = d +} + +// Notes returns what this adapter observed that [krds.GetMetricDataOutput] has +// no field for — CloudWatch's own Messages list, unidentified results, and +// results delivered with no status. cmd/ must render them beside +// Snapshot.Warnings. +func (a *CloudWatchAPI) Notes() []string { return a.notes.list() } + +// GetMetricData implements [krds.MetricsAPI]: one page per call, with the +// caller's NextToken in and CloudWatch's NextToken out. +func (a *CloudWatchAPI) GetMetricData(ctx context.Context, + in *krds.GetMetricDataInput) (*krds.GetMetricDataOutput, error) { + + if in == nil { + in = &krds.GetMetricDataInput{} + } + if len(in.Queries) == 0 { + // CloudWatch rejects an empty query list. Nothing was asked, so + // nothing was answered — and no call is made. + return &krds.GetMetricDataOutput{}, nil + } + // StartTime and EndTime are required by the API. Refused here rather than + // sent, because AWS answers a zero time with a validation error that reads + // nothing like "the caller passed an empty window". + if in.StartTime.IsZero() || in.EndTime.IsZero() || !in.EndTime.After(in.StartTime) { + return nil, fmt.Errorf("provider cloudwatch: GetMetricData needs a positive window, got %s..%s", + in.StartTime.Format(time.RFC3339), in.EndTime.Format(time.RFC3339)) + } + + queries := make([]cwtypes.MetricDataQuery, 0, len(in.Queries)) + for _, q := range in.Queries { + if q.PeriodSeconds <= 0 { + return nil, fmt.Errorf("provider cloudwatch: query %q asks for a %d-second period", + q.ID, q.PeriodSeconds) + } + period, id, ns, name, stat := q.PeriodSeconds, q.ID, q.Namespace, q.MetricName, q.Stat + queries = append(queries, cwtypes.MetricDataQuery{ + Id: &id, + MetricStat: &cwtypes.MetricStat{ + Metric: &cwtypes.Metric{ + Namespace: &ns, + MetricName: &name, + Dimensions: dimensions(q.Dimensions), + }, + Period: &period, + Stat: &stat, + // Unit is deliberately unset. Naming a unit FILTERS datapoints + // to those published with it, so a wrong guess returns an + // empty series — which reads as a quiet database. + }, + ReturnData: boolPtr(true), + }) + } + + start, end, token := in.StartTime, in.EndTime, in.NextToken + cctx, cancel := context.WithTimeout(ctx, a.callTimeout()) + defer cancel() + res, err := a.api.GetMetricData(cctx, &cloudwatch.GetMetricDataInput{ + MetricDataQueries: queries, + StartTime: &start, + EndTime: &end, + NextToken: strPtr(token), + // ScanBy and MaxDatapoints are deliberately left at CloudWatch's + // defaults. See RDS-ADAPTER-FINDINGS.md §7.1: a response that exceeds + // the datapoint budget is paged, and pkg/rds keeps the FIRST result + // it sees per query ID, so which end of a split series survives is a + // pkg/rds question this adapter must not answer by picking a scan + // order. + }) + if err != nil { + return nil, fmt.Errorf("provider cloudwatch: GetMetricData (%d queries): %w", len(queries), err) + } + if res == nil { + return nil, nil + } + + out := &krds.GetMetricDataOutput{NextToken: str(res.NextToken)} + // CloudWatch reports over-large or malformed requests in Messages rather + // than as an error. GetMetricDataOutput has nowhere to put them. + a.noteMessages("GetMetricData", res.Messages) + for _, r := range res.MetricDataResults { + id := str(r.Id) + if id == "" { + // pkg/rds routes results BY ID — not by label, not by position. + // An unidentified result can only land in a slot no query owns, + // leaving the real query unanswered and its series Truncated, + // which is the safe outcome and an invisible one. + a.notes.add("cloudwatch:GetMetricData returned a result with no Id; it cannot be routed " + + "to the query that asked for it, so that query's series is reported as truncated " + + "rather than empty") + } + a.noteMessages("result "+orUnnamed(id), r.Messages) + out.Results = append(out.Results, krds.MetricDataResult{ + ID: id, + Timestamps: append([]time.Time(nil), r.Timestamps...), + Values: append([]float64(nil), r.Values...), + StatusCode: a.statusCode(r.StatusCode, id), + }) + } + return out, nil +} + +// statusCode translates CloudWatch's StatusCode enum, resolving the unset case +// AWAY from "Complete". +// +// CloudWatch defines Complete, PartialData, InternalError and Forbidden. +// Anything that is not Complete makes the series partial in pkg/rds, which is +// the correct handling for all three of the others, so they pass through +// verbatim. The empty string does NOT pass through: pkg/rds turns it into +// Complete, and a series nobody vouched for must not become evidence. +func (a *CloudWatchAPI) statusCode(sc cwtypes.StatusCode, id string) string { + if strings.TrimSpace(string(sc)) == "" { + a.notes.add("cloudwatch:GetMetricData returned a result for query %s with no status code; it "+ + "is reported as %s rather than complete, because an unvouched-for series must not become "+ + "evidence", orUnnamed(id), krds.StatusPartialData) + return krds.StatusPartialData + } + return string(sc) +} + +func (a *CloudWatchAPI) noteMessages(where string, msgs []cwtypes.MessageData) { + for _, m := range msgs { + code, val := str(m.Code), str(m.Value) + if code == "" && val == "" { + continue + } + a.notes.add("cloudwatch:%s reported %q: %s", where, code, val) + } +} + +func (a *CloudWatchAPI) callTimeout() time.Duration { + if a.timeout <= 0 { + return DefaultMetricsCallTimeout + } + return a.timeout +} + +// dimensions converts the seam's map to CloudWatch's list, sorted by name. +// The sort is not cosmetic: Go randomizes map iteration, and an unsorted +// dimension list makes two identical collections issue two different requests, +// which defeats every replay and every diff this tree relies on. +func dimensions(in map[string]string) []cwtypes.Dimension { + if len(in) == 0 { + return nil + } + names := make([]string, 0, len(in)) + for k := range in { + if k != "" { + names = append(names, k) + } + } + sort.Strings(names) + out := make([]cwtypes.Dimension, 0, len(names)) + for _, n := range names { + name, value := n, in[n] + out = append(out, cwtypes.Dimension{Name: &name, Value: &value}) + } + return out +} + +func boolPtr(b bool) *bool { return &b } diff --git a/pkg/provider/cloudwatchapi_test.go b/pkg/provider/cloudwatchapi_test.go new file mode 100644 index 0000000..ad7cbb7 --- /dev/null +++ b/pkg/provider/cloudwatchapi_test.go @@ -0,0 +1,515 @@ +package provider + +// As in rdsapi_test.go: a fake SDK client, no credential, no socket. + +import ( + "context" + "errors" + "reflect" + "sync" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/service/cloudwatch" + cwtypes "github.com/aws/aws-sdk-go-v2/service/cloudwatch/types" + rdstypes "github.com/aws/aws-sdk-go-v2/service/rds/types" + + krds "github.com/agenticode/kilter/pkg/rds" +) + +// ---- the fake cloudwatch: client ---- + +type fakeCW struct { + // results is what to answer, keyed by "/". + results map[string][]krds.Point + // status overrides the status code on every result; the zero value means + // "CloudWatch said nothing", which is the case this adapter resolves. + status cwtypes.StatusCode + // dropIDs omits the Id from every result. + dropIDs bool + // pages, when > 1, answers with that many pages of the same results. + pages int + // messages is CloudWatch's out-of-band complaint channel. + messages []cwtypes.MessageData + err error + + mu sync.Mutex + requests []*cloudwatch.GetMetricDataInput + deadline []bool +} + +func (f *fakeCW) GetMetricData(ctx context.Context, in *cloudwatch.GetMetricDataInput, + _ ...func(*cloudwatch.Options)) (*cloudwatch.GetMetricDataOutput, error) { + + f.mu.Lock() + _, ok := ctx.Deadline() + f.deadline = append(f.deadline, ok) + f.requests = append(f.requests, in) + page := len(f.requests) + f.mu.Unlock() + + if f.err != nil { + return nil, f.err + } + out := &cloudwatch.GetMetricDataOutput{Messages: f.messages} + if f.pages > 1 && page < f.pages { + tok := "page" + out.NextToken = &tok + } + for _, q := range in.MetricDataQueries { + r := cwtypes.MetricDataResult{StatusCode: f.status} + if !f.dropIDs { + r.Id = q.Id + } + key := "" + if q.MetricStat != nil && q.MetricStat.Metric != nil { + for _, d := range q.MetricStat.Metric.Dimensions { + if str(d.Name) == "DBInstanceIdentifier" { + key = str(d.Value) + } + } + key += "/" + str(q.MetricStat.Metric.MetricName) + } + for _, p := range f.results[key] { + r.Timestamps = append(r.Timestamps, p.At) + r.Values = append(r.Values, p.Value) + } + out.MetricDataResults = append(out.MetricDataResults, r) + } + return out, nil +} + +func (f *fakeCW) lastRequest() *cloudwatch.GetMetricDataInput { + f.mu.Lock() + defer f.mu.Unlock() + if len(f.requests) == 0 { + return nil + } + return f.requests[len(f.requests)-1] +} + +// ---- query construction ---- + +func TestGetMetricDataQueryIsBuiltFromTheSeamVerbatim(t *testing.T) { + f := &fakeCW{status: cwtypes.StatusCodeComplete} + a := newCloudWatchAPI(f, "us-east-1") + start, end := refNow.Add(-time.Hour), refNow + _, err := a.GetMetricData(context.Background(), &krds.GetMetricDataInput{ + Queries: []krds.MetricDataQuery{{ + ID: "q0_0", + Namespace: krds.NamespaceRDS, + MetricName: krds.MetricCPUUtilization, + Dimensions: map[string]string{"DBInstanceIdentifier": "db-1"}, + PeriodSeconds: 60, + Stat: "Average", + }}, + StartTime: start, EndTime: end, + }) + if err != nil { + t.Fatal(err) + } + in := f.lastRequest() + if len(in.MetricDataQueries) != 1 { + t.Fatalf("want 1 query, got %d", len(in.MetricDataQueries)) + } + q := in.MetricDataQueries[0] + if str(q.Id) != "q0_0" { + t.Fatalf("Id = %q; pkg/rds routes results BY ID, not by label or position", str(q.Id)) + } + if q.MetricStat == nil || q.MetricStat.Metric == nil { + t.Fatal("a MetricStat query is required") + } + if got := str(q.MetricStat.Metric.Namespace); got != "AWS/RDS" { + t.Fatalf("Namespace = %q", got) + } + if got := str(q.MetricStat.Metric.MetricName); got != krds.MetricCPUUtilization { + t.Fatalf("MetricName = %q", got) + } + if got := i32(q.MetricStat.Period); got != 60 { + t.Fatalf("Period = %d", got) + } + if got := str(q.MetricStat.Stat); got != "Average" { + t.Fatalf("Stat = %q", got) + } + // Naming a unit FILTERS datapoints to the ones published with it, so a + // wrong guess returns an empty series that reads as a quiet database. + if q.MetricStat.Unit != "" { + t.Fatalf("Unit must stay unset, got %q", q.MetricStat.Unit) + } + if q.ReturnData == nil || !*q.ReturnData { + t.Fatal("ReturnData must be explicitly true") + } + if q.Expression != nil { + t.Fatal("no metric math is issued by this adapter") + } + if in.StartTime == nil || !in.StartTime.Equal(start) || in.EndTime == nil || !in.EndTime.Equal(end) { + t.Fatalf("window not propagated: %v..%v", in.StartTime, in.EndTime) + } + if in.NextToken != nil { + t.Fatalf("an empty NextToken must be sent as unset, got %q", *in.NextToken) + } +} + +// TestDimensionsAreOrderedSoTwoIdenticalCollectionsIssueIdenticalRequests: +// Go randomizes map iteration, and an unordered dimension list makes two runs +// over the same account send two different requests, which defeats replay and +// diff. +func TestDimensionsAreOrderedSoTwoIdenticalCollectionsIssueIdenticalRequests(t *testing.T) { + dims := map[string]string{ + "DBInstanceIdentifier": "db-1", + "EngineName": "postgres", + "Az": "us-east-1a", + "": "dropped", + } + var first []string + for i := 0; i < 25; i++ { + f := &fakeCW{status: cwtypes.StatusCodeComplete} + a := newCloudWatchAPI(f, "us-east-1") + if _, err := a.GetMetricData(context.Background(), &krds.GetMetricDataInput{ + Queries: []krds.MetricDataQuery{{ + ID: "q0_0", Namespace: krds.NamespaceRDS, MetricName: "CPUUtilization", + Dimensions: dims, PeriodSeconds: 60, Stat: "Average", + }}, + StartTime: refNow.Add(-time.Hour), EndTime: refNow, + }); err != nil { + t.Fatal(err) + } + var names []string + for _, d := range f.lastRequest().MetricDataQueries[0].MetricStat.Metric.Dimensions { + names = append(names, str(d.Name)+"="+str(d.Value)) + } + if i == 0 { + first = names + continue + } + if !reflect.DeepEqual(names, first) { + t.Fatalf("dimension order is not stable: %v vs %v", names, first) + } + } + want := []string{"Az=us-east-1a", "DBInstanceIdentifier=db-1", "EngineName=postgres"} + if !reflect.DeepEqual(first, want) { + t.Fatalf("dimensions = %v, want %v (a blank name is dropped, not sent)", first, want) + } +} + +// ---- the status decision ---- + +// TestUnsetStatusBecomesPartialNotComplete is the load-bearing test for this +// seam. pkg/rds reads an empty status as Complete: +// +// ser.Status = r.StatusCode +// if ser.Status == "" { ser.Status = StatusComplete } +// +// so passing "" through would turn "CloudWatch did not vouch for this series" +// into "this series is whole evidence" — which is what an idle verdict is made +// of. +func TestUnsetStatusBecomesPartialNotComplete(t *testing.T) { + f := &fakeCW{status: ""} // CloudWatch said nothing + a := newCloudWatchAPI(f, "us-east-1") + out, err := a.GetMetricData(context.Background(), oneQuery()) + if err != nil { + t.Fatal(err) + } + if got := out.Results[0].StatusCode; got != krds.StatusPartialData { + t.Fatalf("StatusCode = %q, want %q", got, krds.StatusPartialData) + } + if !hasNote(a.Notes(), "no status code") { + t.Fatalf("the decision must be visible: %v", a.Notes()) + } + + // End to end: the series pkg/rds builds from it must be marked partial. + series := collectOneSeries(t, "") + if !series.Partial { + t.Fatal("a series CloudWatch did not vouch for must be partial in the snapshot") + } +} + +func TestKnownStatusCodesPassThroughVerbatim(t *testing.T) { + for _, sc := range []cwtypes.StatusCode{ + cwtypes.StatusCodeComplete, + cwtypes.StatusCodePartialData, + cwtypes.StatusCodeInternalError, + cwtypes.StatusCodeForbidden, + } { + t.Run(string(sc), func(t *testing.T) { + a := newCloudWatchAPI(&fakeCW{status: sc}, "us-east-1") + out, err := a.GetMetricData(context.Background(), oneQuery()) + if err != nil { + t.Fatal(err) + } + if got := out.Results[0].StatusCode; got != string(sc) { + t.Fatalf("StatusCode = %q, want %q", got, sc) + } + if len(a.Notes()) != 0 { + t.Fatalf("a vouched-for status needs no note: %v", a.Notes()) + } + // Everything but Complete makes the series partial in pkg/rds. + series := collectOneSeries(t, sc) + if want := sc != cwtypes.StatusCodeComplete; series.Partial != want { + t.Fatalf("Partial = %v for status %q, want %v", series.Partial, sc, want) + } + }) + } +} + +func TestResultWithNoIDIsNotedAndLeavesTheQueryTruncated(t *testing.T) { + a := newCloudWatchAPI(&fakeCW{status: cwtypes.StatusCodeComplete, dropIDs: true}, "us-east-1") + out, err := a.GetMetricData(context.Background(), oneQuery()) + if err != nil { + t.Fatal(err) + } + if out.Results[0].ID != "" { + t.Fatal("an unidentified result must not be given an ID") + } + if !hasNote(a.Notes(), "result with no Id") { + t.Fatalf("notes = %v", a.Notes()) + } + // pkg/rds iterates SLOTS, not results, so the query that was never + // answered becomes a truncated series rather than an empty metric. + snap := collectSnapshot(t, &fakeCW{status: cwtypes.StatusCodeComplete, dropIDs: true}) + for _, s := range snap.Targets[0].Series { + if !s.Partial || s.Status != krds.StatusTruncated { + t.Fatalf("%s: an unanswered query must be truncated, got %+v", s.Metric, s) + } + } +} + +// ---- window, queries, tokens ---- + +func TestEmptyQueryListMakesNoCall(t *testing.T) { + f := &fakeCW{} + a := newCloudWatchAPI(f, "us-east-1") + out, err := a.GetMetricData(context.Background(), &krds.GetMetricDataInput{ + StartTime: refNow.Add(-time.Hour), EndTime: refNow, + }) + if err != nil || out == nil || len(out.Results) != 0 { + t.Fatalf("an empty query list must answer empty without a call: %v %v", out, err) + } + if len(f.requests) != 0 { + t.Fatal("CloudWatch rejects an empty query list; no call should have been made") + } + if _, err := a.GetMetricData(context.Background(), nil); err != nil { + t.Fatalf("a nil input must be handled, got %v", err) + } +} + +func TestInvalidWindowAndPeriodAreRefusedClientSide(t *testing.T) { + f := &fakeCW{} + a := newCloudWatchAPI(f, "us-east-1") + q := []krds.MetricDataQuery{{ID: "q0_0", Namespace: krds.NamespaceRDS, + MetricName: "CPUUtilization", PeriodSeconds: 60, Stat: "Average"}} + for _, tc := range []struct { + name string + in *krds.GetMetricDataInput + }{ + {"zero start", &krds.GetMetricDataInput{Queries: q, EndTime: refNow}}, + {"zero end", &krds.GetMetricDataInput{Queries: q, StartTime: refNow}}, + {"inverted", &krds.GetMetricDataInput{Queries: q, StartTime: refNow, EndTime: refNow.Add(-time.Hour)}}, + {"empty window", &krds.GetMetricDataInput{Queries: q, StartTime: refNow, EndTime: refNow}}, + } { + t.Run(tc.name, func(t *testing.T) { + if _, err := a.GetMetricData(context.Background(), tc.in); err == nil { + t.Fatal("want a client-side refusal") + } + }) + } + bad := &krds.GetMetricDataInput{ + Queries: []krds.MetricDataQuery{{ID: "q0_0", Namespace: krds.NamespaceRDS, MetricName: "X", Stat: "Average"}}, + StartTime: refNow.Add(-time.Hour), EndTime: refNow, + } + if _, err := a.GetMetricData(context.Background(), bad); err == nil { + t.Fatal("a zero period must be refused") + } + if len(f.requests) != 0 { + t.Fatalf("nothing should have reached AWS, got %d requests", len(f.requests)) + } +} + +func TestNextTokenIsPropagatedInBothDirections(t *testing.T) { + f := &fakeCW{status: cwtypes.StatusCodeComplete, pages: 2} + a := newCloudWatchAPI(f, "us-east-1") + out, err := a.GetMetricData(context.Background(), oneQuery()) + if err != nil { + t.Fatal(err) + } + if out.NextToken == "" { + t.Fatal("CloudWatch said there is more; the adapter must say so or the series truncates silently") + } + in := oneQuery() + in.NextToken = out.NextToken + if _, err := a.GetMetricData(context.Background(), in); err != nil { + t.Fatal(err) + } + if got := f.lastRequest().NextToken; got == nil || *got != out.NextToken { + t.Fatalf("inbound NextToken not sent: %v", got) + } +} + +func TestCloudWatchMessagesBecomeNotes(t *testing.T) { + f := &fakeCW{ + status: cwtypes.StatusCodeComplete, + messages: []cwtypes.MessageData{{Code: sp("MaxMetricsExceeded"), Value: sp("too many metrics")}}, + } + a := newCloudWatchAPI(f, "us-east-1") + if _, err := a.GetMetricData(context.Background(), oneQuery()); err != nil { + t.Fatal(err) + } + if !hasNote(a.Notes(), "MaxMetricsExceeded") { + t.Fatalf("CloudWatch's out-of-band complaints have nowhere to go in the seam; "+ + "they must reach Notes(): %v", a.Notes()) + } +} + +func TestMetricsCallCarriesADeadlineAndHonoursCancellation(t *testing.T) { + f := &fakeCW{status: cwtypes.StatusCodeComplete} + a := newCloudWatchAPI(f, "us-east-1") + if _, err := a.GetMetricData(context.Background(), oneQuery()); err != nil { + t.Fatal(err) + } + if len(f.deadline) != 1 || !f.deadline[0] { + t.Fatal("GetMetricData was issued with no deadline") + } + a.SetCallTimeout(0) + if a.callTimeout() != DefaultMetricsCallTimeout { + t.Fatalf("timeout = %s, want the default", a.callTimeout()) + } + + blocked := newCloudWatchAPI(blockingCW{}, "us-east-1") + blocked.SetCallTimeout(20 * time.Millisecond) + if _, err := blocked.GetMetricData(context.Background(), oneQuery()); !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("want a deadline error, got %v", err) + } +} + +type blockingCW struct{} + +func (blockingCW) GetMetricData(ctx context.Context, _ *cloudwatch.GetMetricDataInput, + _ ...func(*cloudwatch.Options)) (*cloudwatch.GetMetricDataOutput, error) { + <-ctx.Done() + return nil, ctx.Err() +} + +// ---- end to end, through the real collector ---- + +// TestLiveAdaptersDriveTheRealCollector is the whole point of this unit: the +// same collector cmd/ drives against --rds-fixture, driven instead by the two +// SDK adapters, producing a snapshot and a report. +func TestLiveAdaptersDriveTheRealCollector(t *testing.T) { + inst := fullInstance() + inst.DBInstanceIdentifier = sp("db-1") + inst.DBInstanceArn = sp("arn:aws:rds:us-east-1:1:db:db-1") + inst.TagList = nil // force the ListTagsForResource fallback + + rf := &fakeRDS{ + instances: []rdstypes.DBInstance{inst}, + tags: map[string][]rdstypes.Tag{ + "arn:aws:rds:us-east-1:1:db:db-1": {{Key: sp(krds.TagKilterMode), Value: sp("off")}}, + }, + } + cw := &fakeCW{status: cwtypes.StatusCodeComplete, results: map[string][]krds.Point{ + "db-1/CPUUtilization": krds.SyntheticMetric(refNow.Add(-time.Hour), time.Minute, 60, 4), + }} + + ra := newRDSAPI(rf, "us-east-1") + ca := newCloudWatchAPI(cw, "us-east-1") + cfg := krds.DefaultCollectorConfig(krds.Window{Start: refNow.Add(-time.Hour), End: refNow}) + cfg.Scope, cfg.Region = "123456789012/us-east-1", ra.Region() + c, err := krds.NewCollector(ra, ca, ra, cfg) + if err != nil { + t.Fatal(err) + } + snap, err := c.Collect(context.Background()) + if err != nil { + t.Fatal(err) + } + if len(snap.Targets) != 1 { + t.Fatalf("want 1 target, got %d", len(snap.Targets)) + } + tgt := snap.Targets[0] + if tgt.Instance.Identifier != "db-1" || tgt.Instance.Class != "db.r6i.xlarge" { + t.Fatalf("instance did not survive the adapter: %+v", tgt.Instance) + } + if tgt.Instance.Region != "us-east-1" { + t.Fatalf("Region = %q; it must be the adapter's region", tgt.Instance.Region) + } + // The tag fallback ran, so the guardrail is reachable. + if tgt.Instance.Tags[krds.TagKilterMode] != "off" { + t.Fatalf("tags = %v; the kilter.dev/mode guardrail is unreachable without them", tgt.Instance.Tags) + } + // Every collected metric got a series, and the one with data has it. + if len(tgt.Series) != len(krds.CollectedMetrics()) { + t.Fatalf("want %d series, got %d", len(krds.CollectedMetrics()), len(tgt.Series)) + } + cpu, ok := tgt.SeriesFor(krds.MetricCPUUtilization) + if !ok || len(cpu.Points) != 60 { + t.Fatalf("CPU series did not survive: %+v", cpu) + } + if cpu.Partial { + t.Fatal("a Complete series must not be marked partial") + } + + // And the brain accepts it. + d, err := krds.NewDomain(krds.Config{Scope: cfg.Scope, Region: cfg.Region, Rates: krds.DefaultRates()}) + if err != nil { + t.Fatal(err) + } + if err := d.Observe(snap); err != nil { + t.Fatal(err) + } + rep := d.Report(refNow, nil) + if rep == nil || len(rep.Refusals()) == 0 { + t.Fatal("the report's refusals are the product; there must be at least one") + } + if len(d.Recommend(refNow, nil)) != 0 { + t.Fatal("this domain proposes nothing, always") + } +} + +// ---- helpers ---- + +func oneQuery() *krds.GetMetricDataInput { + return &krds.GetMetricDataInput{ + Queries: []krds.MetricDataQuery{{ + ID: "q0_0", Namespace: krds.NamespaceRDS, MetricName: krds.MetricCPUUtilization, + Dimensions: map[string]string{"DBInstanceIdentifier": "db-1"}, + PeriodSeconds: 60, Stat: "Average", + }}, + StartTime: refNow.Add(-time.Hour), EndTime: refNow, + } +} + +// collectSnapshot runs the real collector over one instance and the given +// CloudWatch fake. +func collectSnapshot(t *testing.T, cw *fakeCW) *krds.Snapshot { + t.Helper() + inst := fullInstance() + inst.DBInstanceIdentifier = sp("db-1") + inst.DBInstanceArn = sp("arn:aws:rds:us-east-1:1:db:db-1") + ra := newRDSAPI(&fakeRDS{instances: []rdstypes.DBInstance{inst}}, "us-east-1") + ca := newCloudWatchAPI(cw, "us-east-1") + cfg := krds.DefaultCollectorConfig(krds.Window{Start: refNow.Add(-time.Hour), End: refNow}) + cfg.Region = "us-east-1" + c, err := krds.NewCollector(ra, ca, nil, cfg) + if err != nil { + t.Fatal(err) + } + snap, err := c.Collect(context.Background()) + if err != nil { + t.Fatal(err) + } + return snap +} + +// collectOneSeries returns the CPU series pkg/rds builds when CloudWatch +// answers with the given status. +func collectOneSeries(t *testing.T, sc cwtypes.StatusCode) krds.Series { + t.Helper() + snap := collectSnapshot(t, &fakeCW{status: sc, results: map[string][]krds.Point{ + "db-1/CPUUtilization": krds.SyntheticMetric(refNow.Add(-time.Hour), time.Minute, 60, 4), + }}) + s, ok := snap.Targets[0].SeriesFor(krds.MetricCPUUtilization) + if !ok { + t.Fatal("no CPU series") + } + return s +} diff --git a/pkg/provider/rdsapi.go b/pkg/provider/rdsapi.go new file mode 100644 index 0000000..64baf7b --- /dev/null +++ b/pkg/provider/rdsapi.go @@ -0,0 +1,720 @@ +package provider + +// The live RDS SDK adapter: a field copy, and the four places where a field +// copy is not enough. +// +// pkg/rds declares four read seams over plain Go structs whose field names are +// the AWS API's own (pkg/rds/collect.go, pkg/rds/parity_envelope.go). It +// imports no SDK, on purpose — its decision path has to link into an air-gapped +// binary. This file is the other half: *rds.Client on one side, those structs +// on the other, and nothing in between that decides anything. +// +// Two conversions are already done INSIDE pkg/rds and are deliberately NOT +// redone here (pkg/rds/FINDINGS.md §6.2): +// +// - reservation amortization — EffectiveHourly = UsagePrice + FixedPrice ÷ +// term hours, plus the active/payment-pending filter (reservationFromRecord) +// - deployment topology — MultiAZ → commit.RDSMultiAZInstance +// (DBInstance.Deployment) +// +// So FixedPrice, UsagePrice, Duration and MultiAZ are copied raw. Amortizing +// or classifying them here would give the tree two implementations of an +// arithmetic that must have exactly one. +// +// What this file DOES decide, because a field copy cannot avoid it, is what an +// unset SDK pointer means. `*int32` → `int32` through a nil check that defaults +// to 0 turns "AWS did not say" into "AWS said zero", and downstream that reads +// as a measurement. Every nilable field's decision is recorded in +// RDS-ADAPTER-FINDINGS.md §4 and tested in rdsapi_test.go; the two that +// actually change a verdict are: +// +// - ValidStorageOptions with no readable ceiling is OMITTED, so the envelope +// stays Known=false. Emitting it would make StorageEnvelope.Known true with +// MaxIOPS==0, and pkg/rds/parity.go:487 reads MaxIOPS==0 as "no ceiling to +// enforce" — an unknown ceiling would silently become an unlimited one. +// - a GetMetricData result with no StatusCode is reported as PartialData +// (cloudwatchapi.go), because pkg/rds defaults an empty status to Complete. +// +// Facts that neither seam struct has a field for — an instance with no +// identifier, a tag with no key, an event with no date — go to Notes(), which +// cmd/ renders beside the snapshot's own warnings. A degradation nobody can +// see is a degradation that did not happen. + +import ( + "context" + "errors" + "fmt" + "sort" + "strings" + "sync" + "time" + + awsconfig "github.com/aws/aws-sdk-go-v2/config" + awsrds "github.com/aws/aws-sdk-go-v2/service/rds" + rdstypes "github.com/aws/aws-sdk-go-v2/service/rds/types" + "github.com/aws/smithy-go" + + krds "github.com/agenticode/kilter/pkg/rds" +) + +// DefaultRDSCallTimeout bounds one rds: API call. Every operation here is a +// single describe against a control-plane API that answers in well under a +// second; 30 s is generous enough that a slow region is not mistaken for a +// broken one, and short enough that a hung call cannot hold a collection open +// past its page budget. +const DefaultRDSCallTimeout = 30 * time.Second + +// rdsSDK is the minimal rds: surface this adapter needs, satisfied by +// *rds.Client and by test fakes. It is six operations, all of them GETs: this +// interface IS the write surface audit, and it is empty. +type rdsSDK interface { + DescribeDBInstances(ctx context.Context, in *awsrds.DescribeDBInstancesInput, + opts ...func(*awsrds.Options)) (*awsrds.DescribeDBInstancesOutput, error) + DescribeDBClusters(ctx context.Context, in *awsrds.DescribeDBClustersInput, + opts ...func(*awsrds.Options)) (*awsrds.DescribeDBClustersOutput, error) + ListTagsForResource(ctx context.Context, in *awsrds.ListTagsForResourceInput, + opts ...func(*awsrds.Options)) (*awsrds.ListTagsForResourceOutput, error) + DescribeReservedDBInstances(ctx context.Context, in *awsrds.DescribeReservedDBInstancesInput, + opts ...func(*awsrds.Options)) (*awsrds.DescribeReservedDBInstancesOutput, error) + DescribeValidDBInstanceModifications(ctx context.Context, + in *awsrds.DescribeValidDBInstanceModificationsInput, + opts ...func(*awsrds.Options)) (*awsrds.DescribeValidDBInstanceModificationsOutput, error) + DescribeEvents(ctx context.Context, in *awsrds.DescribeEventsInput, + opts ...func(*awsrds.Options)) (*awsrds.DescribeEventsOutput, error) +} + +// RDSAPI adapts *rds.Client to the three rds:-side seams pkg/rds declares. +// +// One concrete type implements all three because one credential answers all +// three, exactly as rds.Fixture does. They stay SEPARATE interfaces at the +// pkg/rds boundary for the reason pkg/rds gives: a caller may hold +// rds:DescribeDBInstances without rds:DescribeReservedDBInstances, and the +// right behaviour then is a complete report with net == gross rather than no +// report at all. cmd/ expresses that by passing nil for the optional +// parameter, NOT by passing an RDSAPI that fails — see +// RDS-ADAPTER-FINDINGS.md §5. +// +// Pagination is per-call and token-shaped, because that is the shape of the +// seam: pkg/rds owns the page loop and its page budget, and this adapter's job +// is to propagate Marker faithfully in both directions. Swallowing a Marker +// here would silently truncate an inventory into a report that reads as +// complete. +type RDSAPI struct { + api rdsSDK + region string + timeout time.Duration + notes noteSet +} + +var ( + _ krds.InventoryAPI = (*RDSAPI)(nil) + _ krds.CommitmentAPI = (*RDSAPI)(nil) + _ krds.ModificationEnvelopeAPI = (*RDSAPI)(nil) +) + +// NewRDSAPI loads AWS credentials from the environment (IRSA, instance +// profile, env vars, shared config) and targets one region. +// +// The region is required rather than inherited from the ambient config, and +// not for tidiness: CollectorConfig.Region is what stamps DBInstance.Region and +// therefore which rate-card row prices every instance. A client talking to +// us-west-2 under a config that says us-east-1 produces a report whose every +// dollar is confidently wrong. Read Region() back and pass the SAME value to +// CollectorConfig.Region. +func NewRDSAPI(ctx context.Context, region string) (*RDSAPI, error) { + region = strings.TrimSpace(region) + if region == "" { + return nil, fmt.Errorf("provider rds: region required: it stamps DBInstance.Region and so " + + "selects the rate-card row that prices every instance") + } + cfg, err := awsconfig.LoadDefaultConfig(ctx, awsconfig.WithRegion(region)) + if err != nil { + return nil, fmt.Errorf("provider rds: load aws config: %w", err) + } + return newRDSAPI(awsrds.NewFromConfig(cfg), region), nil +} + +// newRDSAPI is the test seam. +func newRDSAPI(client rdsSDK, region string) *RDSAPI { + return &RDSAPI{api: client, region: region, timeout: DefaultRDSCallTimeout} +} + +// Region is the region this adapter reads, to be handed to +// CollectorConfig.Region unchanged. +func (a *RDSAPI) Region() string { return a.region } + +// SetCallTimeout overrides the per-call deadline. A non-positive value +// restores [DefaultRDSCallTimeout]. +func (a *RDSAPI) SetCallTimeout(d time.Duration) { + if d <= 0 { + d = DefaultRDSCallTimeout + } + a.timeout = d +} + +// Notes returns what this adapter observed that the seam structs have no field +// for, deduplicated and in a stable order. cmd/ must render them beside +// Snapshot.Warnings; see RDS-ADAPTER-FINDINGS.md §6. +func (a *RDSAPI) Notes() []string { return a.notes.list() } + +// call bounds one SDK call. The parent context still cancels — this only ever +// shortens the deadline, never extends it. +func (a *RDSAPI) call(ctx context.Context) (context.Context, context.CancelFunc) { + d := a.timeout + if d <= 0 { + d = DefaultRDSCallTimeout + } + return context.WithTimeout(ctx, d) +} + +// --- InventoryAPI ---------------------------------------------------------- + +// DescribeDBInstances implements [krds.InventoryAPI]: one page per call, with +// the caller's Marker in and AWS's Marker out. +func (a *RDSAPI) DescribeDBInstances(ctx context.Context, + in *krds.DescribeDBInstancesInput) (*krds.DescribeDBInstancesOutput, error) { + + if in == nil { + in = &krds.DescribeDBInstancesInput{} + } + cctx, cancel := a.call(ctx) + defer cancel() + res, err := a.api.DescribeDBInstances(cctx, &awsrds.DescribeDBInstancesInput{ + Marker: strPtr(in.Marker), + // MaxRecords is passed only when the caller asked for one. Sending 0 + // would be rejected by AWS (the range is 20–100) on a request the + // caller never made. + MaxRecords: i32Ptr(in.MaxRecords), + }) + if err != nil { + return nil, fmt.Errorf("provider rds: DescribeDBInstances: %w", err) + } + if res == nil { + return nil, nil + } + out := &krds.DescribeDBInstancesOutput{Marker: str(res.Marker)} + for _, r := range res.DBInstances { + out.DBInstances = append(out.DBInstances, a.dbInstanceRecord(r)) + } + return out, nil +} + +func (a *RDSAPI) dbInstanceRecord(in rdstypes.DBInstance) krds.DBInstanceRecord { + r := krds.DBInstanceRecord{ + DBInstanceIdentifier: str(in.DBInstanceIdentifier), + DBInstanceArn: str(in.DBInstanceArn), + DBInstanceClass: str(in.DBInstanceClass), + DBInstanceStatus: str(in.DBInstanceStatus), + Engine: str(in.Engine), + EngineVersion: str(in.EngineVersion), + LicenseModel: str(in.LicenseModel), + + // MultiAZ nil ⇒ false ⇒ DBInstance.Deployment() reports Single-AZ, + // a ×1 rather than a ×2 multiplier. The destination field is a bool + // and cannot hold "unknown"; false is the under-stating direction, + // which under-states the instance line and every saving derived from + // it. See RDS-ADAPTER-FINDINGS.md §4. + MultiAZ: bval(in.MultiAZ), + DBClusterIdentifier: str(in.DBClusterIdentifier), + AvailabilityZone: str(in.AvailabilityZone), + + ReadReplicaSourceDBInstanceIdentifier: str(in.ReadReplicaSourceDBInstanceIdentifier), + ReadReplicaDBInstanceIdentifiers: nonEmpty(in.ReadReplicaDBInstanceIdentifiers), + + // AllocatedStorage and MaxAllocatedStorage widen *int32 → int64; no + // value RDS issues comes near the int32 ceiling, so the widening is + // lossless. A nil MaxAllocatedStorage is AWS's own encoding of + // "storage autoscaling is off", which is what 0 means here too. + AllocatedStorage: int64(i32(in.AllocatedStorage)), + MaxAllocatedStorage: int64(i32(in.MaxAllocatedStorage)), + StorageType: str(in.StorageType), + // Iops and StorageThroughput are absent on gp2/standard because they + // are not provisionable there; 0 is the honest reading of that + // absence, and pkg/rds treats 0 as "not provisioned", not as + // "measured zero". + Iops: i32(in.Iops), + StorageThroughput: i32(in.StorageThroughput), + + InstanceCreateTime: tval(in.InstanceCreateTime), + TagList: a.tagMap(in.TagList, str(in.DBInstanceArn)), + } + // pkg/rds keys an instance by ARN, falling back to the identifier, and + // silently skips a record with neither (collect.go recordID). It is + // silent because the seam has nowhere to say it; say it here. + if r.DBInstanceArn == "" && r.DBInstanceIdentifier == "" { + a.notes.add("rds:DescribeDBInstances returned a DB instance with neither DBInstanceArn nor " + + "DBInstanceIdentifier; it cannot be addressed and is ABSENT from this report rather than " + + "reported as having no findings") + } + return r +} + +func (a *RDSAPI) tagMap(in []rdstypes.Tag, owner string) map[string]string { + if len(in) == 0 { + return nil + } + out := make(map[string]string, len(in)) + for _, t := range in { + k := str(t.Key) + if k == "" { + a.notes.add("rds returned a tag with no key on %s; it is dropped, and if it was %s the "+ + "opt-out guardrail would not be honoured", orUnnamed(owner), krds.TagKilterMode) + continue + } + // A nil Value is AWS's encoding of an empty-valued tag, which is a + // legal RDS tag and is not "off" for the kilter.dev/mode guardrail. + out[k] = str(t.Value) + } + if len(out) == 0 { + return nil + } + return out +} + +// DescribeDBClusters implements [krds.InventoryAPI]. pkg/rds reads clusters +// for one reason — to tell an Aurora cluster from a Multi-AZ DB cluster +// without inferring it from a member's engine string — so the cluster's own +// Engine is the load-bearing field here. +func (a *RDSAPI) DescribeDBClusters(ctx context.Context, + in *krds.DescribeDBClustersInput) (*krds.DescribeDBClustersOutput, error) { + + if in == nil { + in = &krds.DescribeDBClustersInput{} + } + cctx, cancel := a.call(ctx) + defer cancel() + res, err := a.api.DescribeDBClusters(cctx, &awsrds.DescribeDBClustersInput{ + Marker: strPtr(in.Marker), + }) + if err != nil { + return nil, fmt.Errorf("provider rds: DescribeDBClusters: %w", err) + } + if res == nil { + return nil, nil + } + out := &krds.DescribeDBClustersOutput{Marker: str(res.Marker)} + for _, c := range res.DBClusters { + rec := krds.DBClusterRecord{ + DBClusterIdentifier: str(c.DBClusterIdentifier), + DBClusterArn: str(c.DBClusterArn), + Engine: str(c.Engine), + EngineMode: str(c.EngineMode), + } + for _, m := range c.DBClusterMembers { + if id := str(m.DBInstanceIdentifier); id != "" { + rec.DBClusterMembers = append(rec.DBClusterMembers, id) + } + } + // The ACU bounds are carried so the Aurora refusal can name the lever + // a future unit would look at; pkg/rds never does arithmetic with + // them, so a nil scaling configuration (every non-Serverless-v2 + // cluster) leaving them at 0 costs nothing. + if sv2 := c.ServerlessV2ScalingConfiguration; sv2 != nil { + rec.ServerlessV2MinCapacity = f64(sv2.MinCapacity) + rec.ServerlessV2MaxCapacity = f64(sv2.MaxCapacity) + } + out.DBClusters = append(out.DBClusters, rec) + } + return out, nil +} + +// ListTagsForResource implements [krds.InventoryAPI]. This is the required +// third operation: without it the kilter.dev/mode opt-out is unreachable and +// an operator who tagged a database to be left alone would not be obeyed. +func (a *RDSAPI) ListTagsForResource(ctx context.Context, + in *krds.ListTagsForResourceInput) (*krds.ListTagsForResourceOutput, error) { + + if in == nil || strings.TrimSpace(in.ResourceName) == "" { + // Refused client-side rather than sent: AWS would answer this with an + // InvalidParameterValue that reads like a permissions problem. + return nil, fmt.Errorf("provider rds: ListTagsForResource needs a resource ARN") + } + cctx, cancel := a.call(ctx) + defer cancel() + res, err := a.api.ListTagsForResource(cctx, &awsrds.ListTagsForResourceInput{ + ResourceName: strPtr(in.ResourceName), + }) + if err != nil { + return nil, fmt.Errorf("provider rds: ListTagsForResource(%s): %w", in.ResourceName, err) + } + if res == nil { + return nil, nil + } + return &krds.ListTagsForResourceOutput{TagList: a.tagMap(res.TagList, in.ResourceName)}, nil +} + +// --- CommitmentAPI --------------------------------------------------------- + +// DescribeReservedDBInstances implements [krds.CommitmentAPI]. +// +// FixedPrice, UsagePrice and Duration are copied RAW. The amortization +// (EffectiveHourly = UsagePrice + FixedPrice ÷ term hours) and the +// active/payment-pending filter live in pkg/rds.reservationFromRecord and are +// deliberately not repeated here. +func (a *RDSAPI) DescribeReservedDBInstances(ctx context.Context, + in *krds.DescribeReservedDBInstancesInput) (*krds.DescribeReservedDBInstancesOutput, error) { + + if in == nil { + in = &krds.DescribeReservedDBInstancesInput{} + } + cctx, cancel := a.call(ctx) + defer cancel() + res, err := a.api.DescribeReservedDBInstances(cctx, &awsrds.DescribeReservedDBInstancesInput{ + Marker: strPtr(in.Marker), + }) + if err != nil { + return nil, fmt.Errorf("provider rds: DescribeReservedDBInstances: %w", err) + } + if res == nil { + return nil, nil + } + out := &krds.DescribeReservedDBInstancesOutput{Marker: str(res.Marker)} + for _, r := range res.ReservedDBInstances { + id := str(r.ReservedDBInstanceId) + // A nil DBInstanceCount becomes 0, and pkg/rds drops a reservation + // with a non-positive count rather than guessing 1. That is the right + // call and it is invisible from inside the seam, so name it. + if r.DBInstanceCount == nil { + a.notes.add("reserved DB instance %s reported no DBInstanceCount; it is dropped rather "+ + "than counted as one, so it contributes no coverage to this report", orUnnamed(id)) + } + // A nil State becomes "", which pkg/rds accepts alongside "active". + if r.State == nil { + a.notes.add("reserved DB instance %s reported no State; it is treated as billing, which "+ + "is the conservative reading — a retired reservation counted as live can only make a "+ + "saving smaller, never larger", orUnnamed(id)) + } + // A nil Duration means the upfront cannot be amortized; pkg/rds keeps + // the usage price alone, which under-states the reservation. + if r.Duration == nil && r.FixedPrice != nil && *r.FixedPrice > 0 { + a.notes.add("reserved DB instance %s reported an upfront price but no Duration; the "+ + "upfront cannot be amortized and is dropped, under-stating this reservation's cost "+ + "and therefore under-stating stranding", orUnnamed(id)) + } + out.ReservedDBInstances = append(out.ReservedDBInstances, krds.ReservedDBInstanceRecord{ + ReservedDBInstanceId: id, + DBInstanceClass: str(r.DBInstanceClass), + DBInstanceCount: int(i32(r.DBInstanceCount)), + ProductDescription: str(r.ProductDescription), + MultiAZ: bval(r.MultiAZ), + OfferingType: str(r.OfferingType), + State: str(r.State), + FixedPrice: f64(r.FixedPrice), + UsagePrice: f64(r.UsagePrice), + // Duration is seconds and widens *int32 → int64 losslessly: the + // longest RDS term is three years, 94,608,000 s. + Duration: int64(i32(r.Duration)), + StartTime: tval(r.StartTime), + }) + } + return out, nil +} + +// --- ModificationEnvelopeAPI ----------------------------------------------- + +// DescribeValidDBInstanceModifications implements +// [krds.ModificationEnvelopeAPI]: the live provisioning envelope, read rather +// than hardcoded because AWS's own storage page states two contradictory gp3 +// IOPS ceilings. +// +// AWS answers in ranges ([]Range of From/To/Step); ValidStorageOptionRecord +// carries one overall minimum and maximum per dimension, which its own doc +// comment says is the reduction to perform. A storage type whose CEILING +// cannot be read is omitted entirely — see [storageOptionRecord]. +func (a *RDSAPI) DescribeValidDBInstanceModifications(ctx context.Context, + in *krds.DescribeValidDBInstanceModificationsInput) ( + *krds.DescribeValidDBInstanceModificationsOutput, error) { + + if in == nil || strings.TrimSpace(in.DBInstanceIdentifier) == "" { + return nil, fmt.Errorf("provider rds: DescribeValidDBInstanceModifications needs a DB instance identifier") + } + cctx, cancel := a.call(ctx) + defer cancel() + res, err := a.api.DescribeValidDBInstanceModifications(cctx, + &awsrds.DescribeValidDBInstanceModificationsInput{ + DBInstanceIdentifier: strPtr(in.DBInstanceIdentifier), + }) + if err != nil { + return nil, fmt.Errorf("provider rds: DescribeValidDBInstanceModifications(%s): %w", + in.DBInstanceIdentifier, err) + } + if res == nil || res.ValidDBInstanceModificationsMessage == nil { + // An answer with no message is not an empty envelope: returning an + // output with no options leaves every StorageEnvelope Known=false, + // which is the refusing default pkg/rds wants. + a.notes.add("rds:DescribeValidDBInstanceModifications(%s) answered without a modifications "+ + "message; that instance's provisioning envelope stays UNKNOWN and every provisioning "+ + "proposal for it is refused by name", in.DBInstanceIdentifier) + return &krds.DescribeValidDBInstanceModificationsOutput{}, nil + } + out := &krds.DescribeValidDBInstanceModificationsOutput{} + for _, so := range res.ValidDBInstanceModificationsMessage.Storage { + if rec, ok := a.storageOptionRecord(so, in.DBInstanceIdentifier); ok { + out.ValidStorageOptions = append(out.ValidStorageOptions, rec) + } + } + return out, nil +} + +// storageOptionRecord reduces one ValidStorageOptions to the record pkg/rds +// carries, and reports whether it may be emitted at all. +// +// The gate is the single most consequential decision in this file. +// pkg/rds/parity.go enforces the ceiling as `env.MaxIOPS > 0 && c.IOPS > +// env.MaxIOPS` — so a StorageEnvelope that is Known with MaxIOPS == 0 has NO +// ceiling, not an unknown one. And pkg/rds sets Known=true for any record +// carrying a storage type. Emitting a record whose ranges AWS did not fill +// would therefore convert "AWS did not tell us the ceiling" into "this +// instance has no ceiling", and a proposal of 80,000 IOPS against an instance +// capped at 16,000 would pass validation and be rejected by AWS at apply time. +// +// So a record is emitted only when AWS named an upper bound for BOTH +// provisionable dimensions. Otherwise the type is left out, Envelope.For +// returns Known=false, and the proposal is refused under the name +// provisioning-envelope-unknown. This over-refuses for storage types where +// nothing is provisionable in the first place (gp2, standard, and io1's +// throughput) — which costs nothing, because pkg/rds only ever looks up gp3. +func (a *RDSAPI) storageOptionRecord(in rdstypes.ValidStorageOptions, id string) ( + krds.ValidStorageOptionRecord, bool) { + + st := strings.ToLower(strings.TrimSpace(str(in.StorageType))) + if st == "" { + a.notes.add("rds:DescribeValidDBInstanceModifications(%s) returned a storage option with no "+ + "storage type; it is dropped", id) + return krds.ValidStorageOptionRecord{}, false + } + minIOPS, maxIOPS, iopsCapped, iopsStepped := reduceRanges(in.ProvisionedIops) + minTP, maxTP, tpCapped, tpStepped := reduceRanges(in.ProvisionedStorageThroughput) + minSize, maxSize, _, _ := reduceRanges(in.StorageSize) + + if !iopsCapped || !tpCapped { + a.notes.add("rds:DescribeValidDBInstanceModifications named no provisionable %s ceiling for "+ + "storage type %q on %s; that envelope is reported UNKNOWN rather than known-with-a-zero-"+ + "ceiling, because pkg/rds reads a zero maximum as \"no ceiling to enforce\"", + missingDimension(iopsCapped, tpCapped), st, id) + return krds.ValidStorageOptionRecord{}, false + } + if iopsStepped || tpStepped { + a.notes.add("rds:DescribeValidDBInstanceModifications reports storage type %q in steps larger "+ + "than 1; ValidStorageOptionRecord carries only an overall minimum and maximum, so a value "+ + "inside the range but off the step would pass this package's check and be rejected by AWS", st) + } + return krds.ValidStorageOptionRecord{ + StorageType: st, + MinIOPS: minIOPS, + MaxIOPS: maxIOPS, + MinStorageThroughputMBps: minTP, + MaxStorageThroughputMBps: maxTP, + MinAllocatedStorageGiB: int64(minSize), + MaxAllocatedStorageGiB: int64(maxSize), + }, true +} + +// reduceRanges collapses AWS's []Range into the single (min, max) pair +// ValidStorageOptionRecord carries. +// +// A nil From or To is skipped rather than read as 0: "AWS did not name a +// bound" is not "AWS named the bound zero". capped reports whether any range +// named a POSITIVE upper bound, which is the only reading under which the +// resulting maximum can be enforced. +func reduceRanges(rs []rdstypes.Range) (lo, hi int32, capped, stepped bool) { + haveLo := false + for _, r := range rs { + if r.From != nil && (!haveLo || *r.From < lo) { + lo, haveLo = *r.From, true + } + if r.To != nil && *r.To > hi { + hi, capped = *r.To, true + } + if r.Step != nil && *r.Step > 1 { + stepped = true + } + } + return lo, hi, capped, stepped +} + +func missingDimension(iopsCapped, tpCapped bool) string { + switch { + case !iopsCapped && !tpCapped: + return "IOPS or throughput" + case !iopsCapped: + return "IOPS" + default: + return "throughput" + } +} + +// DescribeEvents implements [krds.ModificationEnvelopeAPI]: the recent +// modification history behind the four-storage-modifications-per-24-hours +// limit. One page per call; the caller owns the loop and the page budget. +func (a *RDSAPI) DescribeEvents(ctx context.Context, + in *krds.DescribeEventsInput) (*krds.DescribeEventsOutput, error) { + + if in == nil { + in = &krds.DescribeEventsInput{} + } + cctx, cancel := a.call(ctx) + defer cancel() + res, err := a.api.DescribeEvents(cctx, &awsrds.DescribeEventsInput{ + SourceIdentifier: strPtr(in.SourceIdentifier), + SourceType: rdstypes.SourceType(in.SourceType), + // A zero time is "the caller did not bound this side", not the year 1; + // sending it would make AWS reject the whole request. + StartTime: timePtr(in.StartTime), + EndTime: timePtr(in.EndTime), + Marker: strPtr(in.Marker), + }) + if err != nil { + return nil, fmt.Errorf("provider rds: DescribeEvents(%s): %w", in.SourceIdentifier, err) + } + if res == nil { + return nil, nil + } + out := &krds.DescribeEventsOutput{Marker: str(res.Marker)} + for _, e := range res.Events { + rec := krds.EventRecord{ + SourceIdentifier: str(e.SourceIdentifier), + SourceType: string(e.SourceType), + Message: str(e.Message), + Categories: nonEmpty(e.EventCategories), + Date: tval(e.Date), + } + // An undated event keeps its zero time, and Envelope.Cooldown counts + // only events inside the trailing 24 hours — so an undated storage + // modification drops out of the count. Under-counting is the error + // pkg/rds explicitly calls the worse one ("under-counting proposes a + // change AWS will reject"), so it is named here. + if e.Date == nil && krds.IsStorageModificationEvent(rec) { + a.notes.add("rds:DescribeEvents returned a storage-modification event on %s with no date; "+ + "it cannot fall inside the 24-hour window and so does NOT count toward the "+ + "four-modifications limit, which under-counts", orUnnamed(rec.SourceIdentifier)) + } + out.Events = append(out.Events, rec) + } + return out, nil +} + +// --- Shared helpers -------------------------------------------------------- + +// IsAccessDenied reports whether err is AWS refusing for want of an IAM +// permission, as opposed to failing for any other reason. +// +// It exists so cmd/ can act on the difference the IAM table in +// pkg/rds/FINDINGS.md §6.2 draws: cloudwatch:GetMetricData and +// rds:DescribeReservedDBInstances are OPTIONAL, and the documented behaviour +// without them is a degraded report — which cmd/ produces by passing a nil +// seam, not by letting the call fail. See RDS-ADAPTER-FINDINGS.md §5. +// +// Only permission denials are matched. A throttle, a timeout or a malformed +// request must stay an error: swallowing those would turn a transient fault +// into a permanently degraded report that says the credential lacks a +// permission it actually holds. +func IsAccessDenied(err error) bool { + var ae smithy.APIError + if !errors.As(err, &ae) { + return false + } + switch ae.ErrorCode() { + case "AccessDenied", "AccessDeniedException", "UnauthorizedOperation", + "AuthorizationError", "AuthFailure", "NotAuthorized", "Forbidden": + return true + } + return false +} + +// noteSet collects the facts an adapter learned that its seam's structs have +// no field for. Deduplicated, because one malformed field usually repeats +// across a page, and sorted, because a report that reorders itself between +// runs cannot be diffed. +type noteSet struct { + mu sync.Mutex + seen map[string]bool +} + +func (n *noteSet) add(format string, args ...any) { + s := fmt.Sprintf(format, args...) + n.mu.Lock() + defer n.mu.Unlock() + if n.seen == nil { + n.seen = map[string]bool{} + } + n.seen[s] = true +} + +func (n *noteSet) list() []string { + n.mu.Lock() + defer n.mu.Unlock() + if len(n.seen) == 0 { + return nil + } + out := make([]string, 0, len(n.seen)) + for s := range n.seen { + out = append(out, s) + } + sort.Strings(out) + return out +} + +func orUnnamed(s string) string { + if strings.TrimSpace(s) == "" { + return "an unnamed resource" + } + return s +} + +// nonEmpty copies a string slice, dropping blanks. AWS occasionally lists an +// empty member identifier; carrying it forward would invent a replica, a +// cluster member or an event category named "". +func nonEmpty(in []string) []string { + var out []string + for _, s := range in { + if strings.TrimSpace(s) != "" { + out = append(out, s) + } + } + return out +} + +func f64(v *float64) float64 { + if v == nil { + return 0 + } + return *v +} + +func bval(v *bool) bool { + if v == nil { + return false + } + return *v +} + +func tval(v *time.Time) time.Time { + if v == nil { + return time.Time{} + } + return *v +} + +// strPtr returns nil for the empty string, so an unset seam field stays unset +// on the wire instead of becoming an explicit empty filter. +func strPtr(s string) *string { + if s == "" { + return nil + } + return &s +} + +func i32Ptr(v int32) *int32 { + if v == 0 { + return nil + } + return &v +} + +func timePtr(t time.Time) *time.Time { + if t.IsZero() { + return nil + } + return &t +} diff --git a/pkg/provider/rdsapi_test.go b/pkg/provider/rdsapi_test.go new file mode 100644 index 0000000..4bd5c17 --- /dev/null +++ b/pkg/provider/rdsapi_test.go @@ -0,0 +1,979 @@ +package provider + +// Every test here runs against a FAKE SDK client. Nothing in this file reads a +// credential, opens ~/.aws, reads an AWS_* variable or opens a socket — the +// same guarantee pkg/rds gives, for the same reason: an adapter you can only +// test against a real account is an adapter nobody tests. + +import ( + "context" + "errors" + "fmt" + "reflect" + "strconv" + "strings" + "sync" + "testing" + "time" + + awsrds "github.com/aws/aws-sdk-go-v2/service/rds" + rdstypes "github.com/aws/aws-sdk-go-v2/service/rds/types" + + "github.com/agenticode/kilter/pkg/pricing/commit" + krds "github.com/agenticode/kilter/pkg/rds" +) + +// ---- pointer helpers (sp/ip already exist in provider_test.go) ---- + +func bptr(b bool) *bool { return &b } +func fptr(f float64) *float64 { return &f } +func tptr(t time.Time) *time.Time { return &t } + +// ---- the fake rds: client ---- + +type fakeRDS struct { + instances []rdstypes.DBInstance + clusters []rdstypes.DBCluster + tags map[string][]rdstypes.Tag + reserved []rdstypes.ReservedDBInstance + options map[string]*rdstypes.ValidDBInstanceModificationsMessage + events map[string][]rdstypes.Event + // pageSize splits every paginated response; 0 means one page. + pageSize int + + err error + // deadlines records whether each call arrived with a deadline set. + mu sync.Mutex + deadlines []bool + calls []string + lastQuery any +} + +func (f *fakeRDS) record(op string, ctx context.Context, in any) { + f.mu.Lock() + defer f.mu.Unlock() + _, ok := ctx.Deadline() + f.deadlines = append(f.deadlines, ok) + f.calls = append(f.calls, op) + f.lastQuery = in +} + +// page slices a fake collection the way an AWS Marker does. +func (f *fakeRDS) page(total int, marker *string) (start, end int, next *string) { + start = 0 + if marker != nil { + start, _ = strconv.Atoi(*marker) + } + if start > total { + start = total + } + end = total + if f.pageSize > 0 && start+f.pageSize < total { + end = start + f.pageSize + tok := strconv.Itoa(end) + next = &tok + } + return start, end, next +} + +func (f *fakeRDS) DescribeDBInstances(ctx context.Context, in *awsrds.DescribeDBInstancesInput, + _ ...func(*awsrds.Options)) (*awsrds.DescribeDBInstancesOutput, error) { + f.record("DescribeDBInstances", ctx, in) + if f.err != nil { + return nil, f.err + } + s, e, next := f.page(len(f.instances), in.Marker) + return &awsrds.DescribeDBInstancesOutput{DBInstances: f.instances[s:e], Marker: next}, nil +} + +func (f *fakeRDS) DescribeDBClusters(ctx context.Context, in *awsrds.DescribeDBClustersInput, + _ ...func(*awsrds.Options)) (*awsrds.DescribeDBClustersOutput, error) { + f.record("DescribeDBClusters", ctx, in) + if f.err != nil { + return nil, f.err + } + s, e, next := f.page(len(f.clusters), in.Marker) + return &awsrds.DescribeDBClustersOutput{DBClusters: f.clusters[s:e], Marker: next}, nil +} + +func (f *fakeRDS) ListTagsForResource(ctx context.Context, in *awsrds.ListTagsForResourceInput, + _ ...func(*awsrds.Options)) (*awsrds.ListTagsForResourceOutput, error) { + f.record("ListTagsForResource", ctx, in) + if f.err != nil { + return nil, f.err + } + return &awsrds.ListTagsForResourceOutput{TagList: f.tags[str(in.ResourceName)]}, nil +} + +func (f *fakeRDS) DescribeReservedDBInstances(ctx context.Context, in *awsrds.DescribeReservedDBInstancesInput, + _ ...func(*awsrds.Options)) (*awsrds.DescribeReservedDBInstancesOutput, error) { + f.record("DescribeReservedDBInstances", ctx, in) + if f.err != nil { + return nil, f.err + } + s, e, next := f.page(len(f.reserved), in.Marker) + return &awsrds.DescribeReservedDBInstancesOutput{ReservedDBInstances: f.reserved[s:e], Marker: next}, nil +} + +func (f *fakeRDS) DescribeValidDBInstanceModifications(ctx context.Context, + in *awsrds.DescribeValidDBInstanceModificationsInput, + _ ...func(*awsrds.Options)) (*awsrds.DescribeValidDBInstanceModificationsOutput, error) { + f.record("DescribeValidDBInstanceModifications", ctx, in) + if f.err != nil { + return nil, f.err + } + return &awsrds.DescribeValidDBInstanceModificationsOutput{ + ValidDBInstanceModificationsMessage: f.options[str(in.DBInstanceIdentifier)]}, nil +} + +func (f *fakeRDS) DescribeEvents(ctx context.Context, in *awsrds.DescribeEventsInput, + _ ...func(*awsrds.Options)) (*awsrds.DescribeEventsOutput, error) { + f.record("DescribeEvents", ctx, in) + if f.err != nil { + return nil, f.err + } + all := f.events[str(in.SourceIdentifier)] + s, e, next := f.page(len(all), in.Marker) + return &awsrds.DescribeEventsOutput{Events: all[s:e], Marker: next}, nil +} + +// ---- the seams are satisfied, and the write surface is empty ---- + +func TestRDSAdapterSatisfiesEverySeamPkgRDSDeclares(t *testing.T) { + a := newRDSAPI(&fakeRDS{}, "us-east-1") + var ( + _ krds.InventoryAPI = a + _ krds.CommitmentAPI = a + _ krds.ModificationEnvelopeAPI = a + ) + if a.Region() != "us-east-1" { + t.Fatalf("Region() = %q", a.Region()) + } + // The collector is the real consumer; it must accept the adapter in all + // three positions at once. + cw := newCloudWatchAPI(&fakeCW{}, "us-east-1") + cfg := krds.DefaultCollectorConfig(krds.Window{Start: refNow.Add(-24 * time.Hour), End: refNow}) + if _, err := krds.NewCollector(a, cw, a, cfg); err != nil { + t.Fatalf("NewCollector over the live adapter: %v", err) + } +} + +// TestNoMutatingSDKSurface is this package's copy of pkg/rds's +// TestNoMutatingAPISurface: the SDK interfaces are the whole AWS surface the +// RDS path can reach, and a reviewer must be able to see that it is read-only +// without reading the bodies. +func TestNoMutatingSDKSurface(t *testing.T) { + mutating := []string{"create", "modify", "delete", "reboot", "start", "stop", + "promote", "restore", "failover", "apply", "purchase", "add", "remove", "put"} + for _, iface := range []reflect.Type{ + reflect.TypeOf((*rdsSDK)(nil)).Elem(), + reflect.TypeOf((*cloudwatchSDK)(nil)).Elem(), + } { + for i := 0; i < iface.NumMethod(); i++ { + name := strings.ToLower(iface.Method(i).Name) + for _, verb := range mutating { + if strings.HasPrefix(name, verb) { + t.Errorf("%s declares %s, which is not a read operation", + iface.Name(), iface.Method(i).Name) + } + } + } + } +} + +// ---- the field copy ---- + +var refNow = time.Date(2026, 3, 1, 12, 0, 0, 0, time.UTC) + +func fullInstance() rdstypes.DBInstance { + return rdstypes.DBInstance{ + DBInstanceIdentifier: sp("prod-orders"), + DBInstanceArn: sp("arn:aws:rds:us-east-1:123456789012:db:prod-orders"), + DBInstanceClass: sp("db.r6i.xlarge"), + DBInstanceStatus: sp("available"), + Engine: sp("postgres"), + EngineVersion: sp("15.5"), + LicenseModel: sp("postgresql-license"), + MultiAZ: bptr(true), + DBClusterIdentifier: sp(""), + AvailabilityZone: sp("us-east-1a"), + ReadReplicaSourceDBInstanceIdentifier: sp(""), + ReadReplicaDBInstanceIdentifiers: []string{"prod-orders-ro", "", "prod-orders-ro2"}, + AllocatedStorage: ip(500), + MaxAllocatedStorage: ip(1000), + StorageType: sp("gp3"), + Iops: ip(12000), + StorageThroughput: ip(500), + InstanceCreateTime: tptr(refNow.Add(-90 * 24 * time.Hour)), + TagList: []rdstypes.Tag{ + {Key: sp("env"), Value: sp("prod")}, + {Key: sp(krds.TagKilterMode), Value: sp("report")}, + }, + } +} + +func TestDBInstanceRecordIsAFieldForFieldCopy(t *testing.T) { + a := newRDSAPI(&fakeRDS{instances: []rdstypes.DBInstance{fullInstance()}}, "us-east-1") + out, err := a.DescribeDBInstances(context.Background(), &krds.DescribeDBInstancesInput{}) + if err != nil { + t.Fatal(err) + } + if len(out.DBInstances) != 1 { + t.Fatalf("want 1 record, got %d", len(out.DBInstances)) + } + got := out.DBInstances[0] + want := krds.DBInstanceRecord{ + DBInstanceIdentifier: "prod-orders", + DBInstanceArn: "arn:aws:rds:us-east-1:123456789012:db:prod-orders", + DBInstanceClass: "db.r6i.xlarge", + DBInstanceStatus: "available", + Engine: "postgres", + EngineVersion: "15.5", + LicenseModel: "postgresql-license", + MultiAZ: true, + AvailabilityZone: "us-east-1a", + // The blank entry in the SDK list is dropped rather than carried + // forward as a replica named "". + ReadReplicaDBInstanceIdentifiers: []string{"prod-orders-ro", "prod-orders-ro2"}, + AllocatedStorage: 500, + MaxAllocatedStorage: 1000, + StorageType: "gp3", + Iops: 12000, + StorageThroughput: 500, + InstanceCreateTime: refNow.Add(-90 * 24 * time.Hour), + TagList: map[string]string{"env": "prod", krds.TagKilterMode: "report"}, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("field copy differs:\n got %+v\nwant %+v", got, want) + } + if notes := a.Notes(); len(notes) != 0 { + t.Fatalf("a well-formed record must produce no notes: %v", notes) + } +} + +// TestEveryNilableInstanceFieldHasADecision walks the SDK struct with +// reflection so a new nilable field cannot be added upstream and quietly +// default to zero without anyone deciding what its absence means. +func TestEveryNilableInstanceFieldHasADecision(t *testing.T) { + // The fields this adapter reads. Any of them may arrive nil. + read := []string{ + "DBInstanceIdentifier", "DBInstanceArn", "DBInstanceClass", "DBInstanceStatus", + "Engine", "EngineVersion", "LicenseModel", "MultiAZ", "DBClusterIdentifier", + "AvailabilityZone", "ReadReplicaSourceDBInstanceIdentifier", + "ReadReplicaDBInstanceIdentifiers", "AllocatedStorage", "MaxAllocatedStorage", + "StorageType", "Iops", "StorageThroughput", "InstanceCreateTime", "TagList", + } + typ := reflect.TypeOf(rdstypes.DBInstance{}) + for _, name := range read { + if _, ok := typ.FieldByName(name); !ok { + t.Fatalf("the SDK no longer has DBInstance.%s; the field copy is stale", name) + } + } + + // An entirely unset instance: every pointer nil, every slice nil. + a := newRDSAPI(&fakeRDS{instances: []rdstypes.DBInstance{{}}}, "us-east-1") + out, err := a.DescribeDBInstances(context.Background(), nil) + if err != nil { + t.Fatal(err) + } + got := out.DBInstances[0] + if !reflect.DeepEqual(got, krds.DBInstanceRecord{}) { + t.Fatalf("an all-nil SDK instance must produce the zero record, got %+v", got) + } + // …and it must SAY so, because pkg/rds drops an unaddressable record + // silently. + if !hasNote(a.Notes(), "neither DBInstanceArn nor DBInstanceIdentifier") { + t.Fatalf("an unaddressable instance must be visible in Notes(): %v", a.Notes()) + } +} + +// TestNilMultiAZUnderStatesRatherThanOverStates pins the one nilable field +// whose destination cannot carry "unknown" and whose fallback moves money. +func TestNilMultiAZUnderStatesRatherThanOverStates(t *testing.T) { + inst := fullInstance() + inst.MultiAZ = nil + a := newRDSAPI(&fakeRDS{instances: []rdstypes.DBInstance{inst}}, "us-east-1") + out, _ := a.DescribeDBInstances(context.Background(), &krds.DescribeDBInstancesInput{}) + if out.DBInstances[0].MultiAZ { + t.Fatal("a nil MultiAZ must not become true") + } + // Single-AZ is a ×1 multiplier where Multi-AZ is ×2: the copy under-states + // the instance line, and an under-stated line can only under-state a + // saving. Prove pkg/rds reads it that way rather than refusing. + d := krds.DBInstance{Identifier: "x", MultiAZ: out.DBInstances[0].MultiAZ} + dep, ok := d.Deployment() + if !ok || dep != commit.RDSSingleAZ { + t.Fatalf("Deployment() = %v, %v; want single-AZ", dep, ok) + } +} + +func TestNilTagKeyIsDroppedAndNoted(t *testing.T) { + inst := fullInstance() + inst.TagList = []rdstypes.Tag{ + {Key: nil, Value: sp("off")}, + {Key: sp("keep"), Value: nil}, + } + a := newRDSAPI(&fakeRDS{instances: []rdstypes.DBInstance{inst}}, "us-east-1") + out, _ := a.DescribeDBInstances(context.Background(), &krds.DescribeDBInstancesInput{}) + tags := out.DBInstances[0].TagList + if _, bad := tags[""]; bad { + t.Fatal("a tag with no key must not become a tag named \"\"") + } + // A nil Value is an empty-valued tag, which is legal and is NOT "off". + if v, ok := tags["keep"]; !ok || v != "" { + t.Fatalf("tags = %v; want keep=\"\"", tags) + } + if !hasNote(a.Notes(), "tag with no key") { + t.Fatalf("a dropped tag must be visible: %v", a.Notes()) + } +} + +func TestListTagsForResourceRequiresAnARN(t *testing.T) { + a := newRDSAPI(&fakeRDS{}, "us-east-1") + if _, err := a.ListTagsForResource(context.Background(), &krds.ListTagsForResourceInput{}); err == nil { + t.Fatal("an empty resource name must be refused client-side") + } + if _, err := a.ListTagsForResource(context.Background(), nil); err == nil { + t.Fatal("a nil input must be refused client-side") + } +} + +func TestClusterRecordCopyAndNilServerlessConfig(t *testing.T) { + f := &fakeRDS{clusters: []rdstypes.DBCluster{ + { + DBClusterIdentifier: sp("aurora-1"), + DBClusterArn: sp("arn:aws:rds:us-east-1:1:cluster:aurora-1"), + Engine: sp("aurora-postgresql"), + EngineMode: sp("provisioned"), + DBClusterMembers: []rdstypes.DBClusterMember{ + {DBInstanceIdentifier: sp("aurora-1-a")}, + {DBInstanceIdentifier: nil}, + }, + ServerlessV2ScalingConfiguration: &rdstypes.ServerlessV2ScalingConfigurationInfo{ + MinCapacity: fptr(0.5), MaxCapacity: fptr(16), + }, + }, + // A provisioned Multi-AZ DB cluster: no serverless configuration at all. + {DBClusterIdentifier: sp("mysql-maz"), Engine: sp("mysql")}, + }} + a := newRDSAPI(f, "us-east-1") + out, err := a.DescribeDBClusters(context.Background(), &krds.DescribeDBClustersInput{}) + if err != nil { + t.Fatal(err) + } + got := out.DBClusters[0] + want := krds.DBClusterRecord{ + DBClusterIdentifier: "aurora-1", + DBClusterArn: "arn:aws:rds:us-east-1:1:cluster:aurora-1", + Engine: "aurora-postgresql", + EngineMode: "provisioned", + DBClusterMembers: []string{"aurora-1-a"}, + ServerlessV2MinCapacity: 0.5, + ServerlessV2MaxCapacity: 16, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("cluster copy:\n got %+v\nwant %+v", got, want) + } + // A cluster with no ServerlessV2 configuration leaves the ACU bounds at 0. + // pkg/rds never does arithmetic with them, so 0 costs nothing — but it + // must not become a nil-pointer dereference on the way through. + second := out.DBClusters[1] + if second.ServerlessV2MinCapacity != 0 || second.ServerlessV2MaxCapacity != 0 { + t.Fatalf("a cluster with no serverless config must leave the ACU bounds at 0: %+v", second) + } +} + +// ---- pagination, driven by the real collector ---- + +func TestPaginationIsPropagatedThroughEverySeam(t *testing.T) { + var instances []rdstypes.DBInstance + for i := 0; i < 7; i++ { + in := fullInstance() + id := fmt.Sprintf("db-%d", i) + in.DBInstanceIdentifier = sp(id) + in.DBInstanceArn = sp("arn:aws:rds:us-east-1:1:db:" + id) + instances = append(instances, in) + } + var reserved []rdstypes.ReservedDBInstance + for i := 0; i < 5; i++ { + reserved = append(reserved, rdstypes.ReservedDBInstance{ + ReservedDBInstanceId: sp(fmt.Sprintf("ri-%d", i)), + DBInstanceClass: sp("db.r6i.xlarge"), + DBInstanceCount: ip(1), + ProductDescription: sp("postgresql"), + State: sp("active"), + Duration: ip(31536000), + UsagePrice: fptr(0.1), + FixedPrice: fptr(876), + StartTime: tptr(refNow.Add(-30 * 24 * time.Hour)), + }) + } + f := &fakeRDS{instances: instances, reserved: reserved, pageSize: 3} + a := newRDSAPI(f, "us-east-1") + + cfg := krds.DefaultCollectorConfig(krds.Window{Start: refNow.Add(-24 * time.Hour), End: refNow}) + cfg.Scope, cfg.Region = "acct/us-east-1", "us-east-1" + c, err := krds.NewCollector(a, nil, a, cfg) + if err != nil { + t.Fatal(err) + } + snap, err := c.Collect(context.Background()) + if err != nil { + t.Fatal(err) + } + if len(snap.Targets) != 7 { + t.Fatalf("pagination lost instances: got %d of 7", len(snap.Targets)) + } + if len(snap.Reservations) != 5 { + t.Fatalf("pagination lost reservations: got %d of 5", len(snap.Reservations)) + } + // Three pages of instances (3+3+1) and two of reservations (3+2). + if n := countCalls(f, "DescribeDBInstances"); n != 3 { + t.Fatalf("want 3 instance pages, got %d", n) + } + if n := countCalls(f, "DescribeReservedDBInstances"); n != 2 { + t.Fatalf("want 2 reservation pages, got %d", n) + } +} + +// TestSwallowingAMarkerWouldTruncateSilently is the negative of the test +// above: it shows what a dropped Marker costs, so the propagation is not +// mistaken for incidental. +func TestSwallowingAMarkerWouldTruncateSilently(t *testing.T) { + f := &fakeRDS{pageSize: 3} + for i := 0; i < 7; i++ { + in := fullInstance() + in.DBInstanceIdentifier = sp(fmt.Sprintf("db-%d", i)) + in.DBInstanceArn = sp(fmt.Sprintf("arn:aws:rds:us-east-1:1:db:db-%d", i)) + f.instances = append(f.instances, in) + } + a := newRDSAPI(f, "us-east-1") + first, err := a.DescribeDBInstances(context.Background(), &krds.DescribeDBInstancesInput{}) + if err != nil { + t.Fatal(err) + } + if first.Marker == "" { + t.Fatal("AWS said there is more; the adapter must say so too, or the inventory truncates " + + "into a report that reads as complete") + } + second, err := a.DescribeDBInstances(context.Background(), + &krds.DescribeDBInstancesInput{Marker: first.Marker}) + if err != nil { + t.Fatal(err) + } + if second.DBInstances[0].DBInstanceIdentifier != "db-3" { + t.Fatalf("the inbound marker was not honoured: page 2 starts at %q", + second.DBInstances[0].DBInstanceIdentifier) + } +} + +func TestMaxRecordsIsOnlySentWhenAsked(t *testing.T) { + f := &fakeRDS{} + a := newRDSAPI(f, "us-east-1") + if _, err := a.DescribeDBInstances(context.Background(), &krds.DescribeDBInstancesInput{}); err != nil { + t.Fatal(err) + } + if in := f.lastQuery.(*awsrds.DescribeDBInstancesInput); in.MaxRecords != nil { + t.Fatalf("MaxRecords sent unasked as %d; AWS rejects anything outside 20–100", *in.MaxRecords) + } + if _, err := a.DescribeDBInstances(context.Background(), + &krds.DescribeDBInstancesInput{MaxRecords: 100}); err != nil { + t.Fatal(err) + } + if in := f.lastQuery.(*awsrds.DescribeDBInstancesInput); in.MaxRecords == nil || *in.MaxRecords != 100 { + t.Fatalf("MaxRecords not propagated: %v", in.MaxRecords) + } +} + +// ---- reservations: raw in, amortized by pkg/rds, never twice ---- + +func TestReservationFieldsAreCopiedRawAndAmortizedOnlyByPkgRDS(t *testing.T) { + f := &fakeRDS{reserved: []rdstypes.ReservedDBInstance{{ + ReservedDBInstanceId: sp("ri-1"), + DBInstanceClass: sp("db.r6i.xlarge"), + DBInstanceCount: ip(2), + ProductDescription: sp("postgresql"), + MultiAZ: bptr(true), + OfferingType: sp("Partial Upfront"), + State: sp("active"), + FixedPrice: fptr(8760), + UsagePrice: fptr(0.05), + Duration: ip(31536000), // 1 year in seconds → 8,760 hours + StartTime: tptr(refNow), + }}} + a := newRDSAPI(f, "us-east-1") + out, err := a.DescribeReservedDBInstances(context.Background(), + &krds.DescribeReservedDBInstancesInput{}) + if err != nil { + t.Fatal(err) + } + got := out.ReservedDBInstances[0] + want := krds.ReservedDBInstanceRecord{ + ReservedDBInstanceId: "ri-1", DBInstanceClass: "db.r6i.xlarge", DBInstanceCount: 2, + ProductDescription: "postgresql", MultiAZ: true, OfferingType: "Partial Upfront", + State: "active", FixedPrice: 8760, UsagePrice: 0.05, Duration: 31536000, StartTime: refNow, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("reservation copy:\n got %+v\nwant %+v", got, want) + } + + // Now prove pkg/rds does the amortization and this adapter did not: + // 8760 / 8760h = 1.00/h, plus the 0.05 usage price. + cfg := krds.DefaultCollectorConfig(krds.Window{Start: refNow.Add(-24 * time.Hour), End: refNow}) + cfg.Region = "us-east-1" + c, _ := krds.NewCollector(a, nil, a, cfg) + snap, err := c.Collect(context.Background()) + if err != nil { + t.Fatal(err) + } + if len(snap.Reservations) != 1 { + t.Fatalf("want 1 amortized reservation, got %d", len(snap.Reservations)) + } + r := snap.Reservations[0] + if diff := r.EffectiveHourlyUSD - 1.05; diff > 1e-9 || diff < -1e-9 { + t.Fatalf("EffectiveHourlyUSD = %v, want 1.05 (amortized exactly once)", r.EffectiveHourlyUSD) + } + // Deployment topology is likewise pkg/rds's, derived from the raw MultiAZ. + if r.Deployment != commit.RDSMultiAZInstance { + t.Fatalf("Deployment = %q; the adapter must not re-derive topology", r.Deployment) + } +} + +func TestReservationNilFieldsAreVisible(t *testing.T) { + f := &fakeRDS{reserved: []rdstypes.ReservedDBInstance{ + {ReservedDBInstanceId: sp("ri-nocount"), DBInstanceClass: sp("db.t3.small"), State: sp("active")}, + {ReservedDBInstanceId: sp("ri-nostate"), DBInstanceClass: sp("db.t3.small"), DBInstanceCount: ip(1)}, + {ReservedDBInstanceId: sp("ri-nodur"), DBInstanceClass: sp("db.t3.small"), + DBInstanceCount: ip(1), State: sp("active"), FixedPrice: fptr(100)}, + }} + a := newRDSAPI(f, "us-east-1") + if _, err := a.DescribeReservedDBInstances(context.Background(), nil); err != nil { + t.Fatal(err) + } + for _, want := range []string{ + "reported no DBInstanceCount", + "reported no State", + "upfront price but no Duration", + } { + if !hasNote(a.Notes(), want) { + t.Errorf("missing note %q in %v", want, a.Notes()) + } + } +} + +// ---- the provisioning envelope: the decision that moves a verdict ---- + +func TestEnvelopeRangesAreReducedToOverallMinAndMax(t *testing.T) { + f := &fakeRDS{options: map[string]*rdstypes.ValidDBInstanceModificationsMessage{ + "db-1": {Storage: []rdstypes.ValidStorageOptions{{ + StorageType: sp("GP3"), + ProvisionedIops: []rdstypes.Range{ + {From: ip(3000), To: ip(12000)}, + {From: ip(12001), To: ip(64000)}, + }, + ProvisionedStorageThroughput: []rdstypes.Range{ + {From: ip(125), To: ip(500)}, + {From: ip(501), To: ip(4000)}, + }, + StorageSize: []rdstypes.Range{{From: ip(20), To: ip(65536)}}, + }}}, + }} + a := newRDSAPI(f, "us-east-1") + out, err := a.DescribeValidDBInstanceModifications(context.Background(), + &krds.DescribeValidDBInstanceModificationsInput{DBInstanceIdentifier: "db-1"}) + if err != nil { + t.Fatal(err) + } + want := krds.ValidStorageOptionRecord{ + StorageType: "gp3", MinIOPS: 3000, MaxIOPS: 64000, + MinStorageThroughputMBps: 125, MaxStorageThroughputMBps: 4000, + MinAllocatedStorageGiB: 20, MaxAllocatedStorageGiB: 65536, + } + if len(out.ValidStorageOptions) != 1 || !reflect.DeepEqual(out.ValidStorageOptions[0], want) { + t.Fatalf("range reduction:\n got %+v\nwant %+v", out.ValidStorageOptions, want) + } + + // End to end: the envelope becomes Known and enforces its ceiling. + env := collectEnvelope(t, a, "db-1") + gp3 := env.Get("db-1").For("gp3") + if !gp3.Known || gp3.MaxIOPS != 64000 { + t.Fatalf("collected envelope = %+v", gp3) + } +} + +// TestEnvelopeWithNoReadableCeilingStaysUnknown is the load-bearing test in +// this file. +// +// pkg/rds enforces the ceiling as `env.MaxIOPS > 0 && c.IOPS > env.MaxIOPS`, +// so a Known envelope with a zero maximum has NO ceiling rather than an +// unknown one. If this adapter emitted a record whose ranges AWS never filled, +// "AWS did not tell us the ceiling" would silently become "this instance has +// no ceiling" and an 80,000-IOPS proposal would sail past validation on an +// instance capped at 16,000. +func TestEnvelopeWithNoReadableCeilingStaysUnknown(t *testing.T) { + cases := []struct { + name string + opt rdstypes.ValidStorageOptions + }{ + {"no ranges at all", rdstypes.ValidStorageOptions{StorageType: sp("gp3")}}, + {"iops range with a nil upper bound", rdstypes.ValidStorageOptions{ + StorageType: sp("gp3"), + ProvisionedIops: []rdstypes.Range{{From: ip(3000), To: nil}}, + ProvisionedStorageThroughput: []rdstypes.Range{{From: ip(125), To: ip(4000)}}, + }}, + {"throughput ranges absent", rdstypes.ValidStorageOptions{ + StorageType: sp("gp3"), + ProvisionedIops: []rdstypes.Range{{From: ip(3000), To: ip(64000)}}, + }}, + {"a zero upper bound is not a ceiling", rdstypes.ValidStorageOptions{ + StorageType: sp("gp3"), + ProvisionedIops: []rdstypes.Range{{From: ip(0), To: ip(0)}}, + ProvisionedStorageThroughput: []rdstypes.Range{{From: ip(0), To: ip(0)}}, + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := &fakeRDS{options: map[string]*rdstypes.ValidDBInstanceModificationsMessage{ + "db-1": {Storage: []rdstypes.ValidStorageOptions{tc.opt}}, + }} + a := newRDSAPI(f, "us-east-1") + out, err := a.DescribeValidDBInstanceModifications(context.Background(), + &krds.DescribeValidDBInstanceModificationsInput{DBInstanceIdentifier: "db-1"}) + if err != nil { + t.Fatal(err) + } + if len(out.ValidStorageOptions) != 0 { + t.Fatalf("an unreadable ceiling must not become a known envelope: %+v", + out.ValidStorageOptions) + } + if env := collectEnvelope(t, a, "db-1"); env.Get("db-1").For("gp3").Known { + t.Fatal("StorageEnvelope.Known must stay false, so the proposal is refused by name") + } + if !hasNote(a.Notes(), "reported UNKNOWN rather than known-with-a-zero-ceiling") { + t.Fatalf("the omission must be visible: %v", a.Notes()) + } + }) + } +} + +func TestEnvelopeAbsentMessageIsUnknownNotEmpty(t *testing.T) { + a := newRDSAPI(&fakeRDS{options: map[string]*rdstypes.ValidDBInstanceModificationsMessage{}}, "us-east-1") + out, err := a.DescribeValidDBInstanceModifications(context.Background(), + &krds.DescribeValidDBInstanceModificationsInput{DBInstanceIdentifier: "db-1"}) + if err != nil { + t.Fatal(err) + } + if out == nil || len(out.ValidStorageOptions) != 0 { + t.Fatalf("want an empty output, got %+v", out) + } + if !hasNote(a.Notes(), "answered without a modifications message") { + t.Fatalf("notes = %v", a.Notes()) + } + if _, err := a.DescribeValidDBInstanceModifications(context.Background(), + &krds.DescribeValidDBInstanceModificationsInput{}); err == nil { + t.Fatal("an empty identifier must be refused client-side") + } +} + +func TestEnvelopeStepIsNotedBecauseTheRecordCannotCarryIt(t *testing.T) { + f := &fakeRDS{options: map[string]*rdstypes.ValidDBInstanceModificationsMessage{ + "db-1": {Storage: []rdstypes.ValidStorageOptions{{ + StorageType: sp("gp3"), + ProvisionedIops: []rdstypes.Range{{From: ip(3000), To: ip(64000), Step: ip(1000)}}, + ProvisionedStorageThroughput: []rdstypes.Range{{From: ip(125), To: ip(4000)}}, + }}}, + }} + a := newRDSAPI(f, "us-east-1") + if _, err := a.DescribeValidDBInstanceModifications(context.Background(), + &krds.DescribeValidDBInstanceModificationsInput{DBInstanceIdentifier: "db-1"}); err != nil { + t.Fatal(err) + } + if !hasNote(a.Notes(), "in steps larger than 1") { + t.Fatalf("a step the record cannot carry must be visible: %v", a.Notes()) + } +} + +// ---- events ---- + +func TestEventCopyAndUndatedModificationIsNoted(t *testing.T) { + f := &fakeRDS{events: map[string][]rdstypes.Event{ + "db-1": { + { + SourceIdentifier: sp("db-1"), + SourceType: rdstypes.SourceTypeDbInstance, + Message: sp("Applying modification to allocated storage"), + EventCategories: []string{"configuration change", ""}, + Date: tptr(refNow.Add(-2 * time.Hour)), + }, + { + SourceIdentifier: sp("db-1"), + Message: sp("Finished applying modification to Provisioned IOPS"), + EventCategories: []string{"configuration change"}, + Date: nil, + }, + }, + }} + a := newRDSAPI(f, "us-east-1") + out, err := a.DescribeEvents(context.Background(), &krds.DescribeEventsInput{ + SourceIdentifier: "db-1", SourceType: krds.EventSourceDBInstance, + StartTime: refNow.Add(-24 * time.Hour), EndTime: refNow, + }) + if err != nil { + t.Fatal(err) + } + if len(out.Events) != 2 { + t.Fatalf("want 2 events, got %d", len(out.Events)) + } + first := out.Events[0] + if first.SourceType != string(rdstypes.SourceTypeDbInstance) { + t.Fatalf("SourceType = %q", first.SourceType) + } + if !reflect.DeepEqual(first.Categories, []string{"configuration change"}) { + t.Fatalf("a blank category must be dropped: %v", first.Categories) + } + if !out.Events[1].Date.IsZero() { + t.Fatal("an undated event must keep a zero date rather than being given one") + } + if !hasNote(a.Notes(), "with no date") { + t.Fatalf("an undated storage modification must be visible: %v", a.Notes()) + } + + // The zero-time input bounds must not be sent as year 1. + if _, err := a.DescribeEvents(context.Background(), &krds.DescribeEventsInput{SourceIdentifier: "db-1"}); err != nil { + t.Fatal(err) + } + in := f.lastQuery.(*awsrds.DescribeEventsInput) + if in.StartTime != nil || in.EndTime != nil { + t.Fatalf("a zero window must be sent as unset, got %v..%v", in.StartTime, in.EndTime) + } +} + +func TestEventPaginationFeedsTheCooldownVerdict(t *testing.T) { + var evs []rdstypes.Event + for i := 0; i < 4; i++ { + evs = append(evs, rdstypes.Event{ + SourceIdentifier: sp("db-1"), + SourceType: rdstypes.SourceTypeDbInstance, + Message: sp("Applying modification to allocated storage"), + EventCategories: []string{"configuration change"}, + Date: tptr(refNow.Add(-time.Duration(i+1) * time.Hour)), + }) + } + a := newRDSAPI(&fakeRDS{events: map[string][]rdstypes.Event{"db-1": evs}, pageSize: 2}, "us-east-1") + env := collectEnvelope(t, a, "db-1") + v := env.Get("db-1").Cooldown(refNow) + if !v.Known { + t.Fatal("the history was read; the cooldown must be Known") + } + if v.Recent != 4 || !v.Blocked { + t.Fatalf("pagination lost modifications: %+v", v) + } +} + +// ---- timeouts, cancellation and error propagation ---- + +func TestEveryCallCarriesADeadline(t *testing.T) { + f := &fakeRDS{ + instances: []rdstypes.DBInstance{fullInstance()}, + clusters: []rdstypes.DBCluster{{DBClusterIdentifier: sp("c1")}}, + reserved: []rdstypes.ReservedDBInstance{{ReservedDBInstanceId: sp("r1"), DBInstanceCount: ip(1), DBInstanceClass: sp("db.t3.small")}}, + options: map[string]*rdstypes.ValidDBInstanceModificationsMessage{"db-1": {}}, + events: map[string][]rdstypes.Event{"db-1": nil}, + tags: map[string][]rdstypes.Tag{"arn": {{Key: sp("k"), Value: sp("v")}}}, + } + a := newRDSAPI(f, "us-east-1") + ctx := context.Background() + a.DescribeDBInstances(ctx, &krds.DescribeDBInstancesInput{}) + a.DescribeDBClusters(ctx, &krds.DescribeDBClustersInput{}) + a.ListTagsForResource(ctx, &krds.ListTagsForResourceInput{ResourceName: "arn"}) + a.DescribeReservedDBInstances(ctx, &krds.DescribeReservedDBInstancesInput{}) + a.DescribeValidDBInstanceModifications(ctx, &krds.DescribeValidDBInstanceModificationsInput{DBInstanceIdentifier: "db-1"}) + a.DescribeEvents(ctx, &krds.DescribeEventsInput{SourceIdentifier: "db-1"}) + + f.mu.Lock() + defer f.mu.Unlock() + if len(f.deadlines) != 6 { + t.Fatalf("want 6 calls, got %d (%v)", len(f.deadlines), f.calls) + } + for i, ok := range f.deadlines { + if !ok { + t.Errorf("%s was issued with no deadline; a hung call would hold the collection open", + f.calls[i]) + } + } +} + +func TestSetCallTimeoutBoundsTheCall(t *testing.T) { + blocked := &blockingRDS{fakeRDS: &fakeRDS{}} + a := newRDSAPI(blocked, "us-east-1") + a.SetCallTimeout(20 * time.Millisecond) + start := time.Now() + _, err := a.DescribeDBInstances(context.Background(), &krds.DescribeDBInstancesInput{}) + if err == nil { + t.Fatal("a call that outlives its timeout must fail") + } + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("want a deadline error, got %v", err) + } + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Fatalf("the timeout did not bound the call: %s", elapsed) + } + // A non-positive value restores the default rather than disabling the bound. + a.SetCallTimeout(0) + if a.timeout != DefaultRDSCallTimeout { + t.Fatalf("timeout = %s, want the default", a.timeout) + } +} + +func TestParentCancellationStillPropagates(t *testing.T) { + blocked := &blockingRDS{fakeRDS: &fakeRDS{}} + a := newRDSAPI(blocked, "us-east-1") + ctx, cancel := context.WithCancel(context.Background()) + cancel() + if _, err := a.DescribeDBInstances(ctx, &krds.DescribeDBInstancesInput{}); !errors.Is(err, context.Canceled) { + t.Fatalf("want a cancellation error, got %v", err) + } +} + +type blockingRDS struct{ *fakeRDS } + +func (b *blockingRDS) DescribeDBInstances(ctx context.Context, _ *awsrds.DescribeDBInstancesInput, + _ ...func(*awsrds.Options)) (*awsrds.DescribeDBInstancesOutput, error) { + <-ctx.Done() + return nil, ctx.Err() +} + +func TestInventoryErrorFailsTheCollectionAndOptionalErrorsDoNot(t *testing.T) { + boom := errors.New("throttled") + + // The inventory is the one hard dependency. + a := newRDSAPI(&fakeRDS{err: boom}, "us-east-1") + cfg := krds.DefaultCollectorConfig(krds.Window{Start: refNow.Add(-24 * time.Hour), End: refNow}) + c, _ := krds.NewCollector(a, nil, nil, cfg) + if _, err := c.Collect(context.Background()); err == nil { + t.Fatal("a failed DescribeDBInstances must fail the collection") + } + + // A failed reservation read degrades into a warning instead. + f := &fakeRDS{instances: []rdstypes.DBInstance{fullInstance()}} + good := newRDSAPI(f, "us-east-1") + bad := newRDSAPI(&errAfterInventory{fakeRDS: f, err: boom}, "us-east-1") + c2, _ := krds.NewCollector(good, nil, bad, cfg) + snap, err := c2.Collect(context.Background()) + if err != nil { + t.Fatalf("a failed optional seam must not fail the collection: %v", err) + } + if !hasNote(snap.Warnings, "could not list Reserved DB Instances") { + t.Fatalf("the degradation must be visible in the snapshot: %v", snap.Warnings) + } +} + +type errAfterInventory struct { + *fakeRDS + err error +} + +func (e *errAfterInventory) DescribeReservedDBInstances(ctx context.Context, + _ *awsrds.DescribeReservedDBInstancesInput, + _ ...func(*awsrds.Options)) (*awsrds.DescribeReservedDBInstancesOutput, error) { + return nil, e.err +} + +// ---- access-denied classification ---- + +func TestIsAccessDenied(t *testing.T) { + cases := []struct { + name string + err error + denied bool + }{ + {"AccessDenied", apiErr{"AccessDenied", "not authorized"}, true}, + {"AccessDeniedException", apiErr{"AccessDeniedException", "no"}, true}, + {"wrapped", fmt.Errorf("provider rds: %w", apiErr{"AccessDenied", "no"}), true}, + // These must stay errors: a throttle turned into "you lack the + // permission" produces a permanently degraded report from a transient + // fault. + {"throttling", apiErr{"ThrottlingException", "slow down"}, false}, + {"validation", apiErr{"InvalidParameterValue", "bad marker"}, false}, + {"not found", apiErr{"DBInstanceNotFound", "gone"}, false}, + {"plain error", errors.New("dial tcp: connection refused"), false}, + {"nil", nil, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := IsAccessDenied(tc.err); got != tc.denied { + t.Fatalf("IsAccessDenied(%v) = %v, want %v", tc.err, got, tc.denied) + } + }) + } +} + +// ---- notes ---- + +func TestNotesAreDeduplicatedSortedAndRaceSafe(t *testing.T) { + a := newRDSAPI(&fakeRDS{}, "us-east-1") + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + a.notes.add("zebra") + a.notes.add("alpha") + a.notes.add("note %d", i%2) + _ = a.Notes() + }(i) + } + wg.Wait() + got := a.Notes() + want := []string{"alpha", "note 0", "note 1", "zebra"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("notes = %v, want %v", got, want) + } + if fresh := newRDSAPI(&fakeRDS{}, "x").Notes(); fresh != nil { + t.Fatalf("a clean run must report no notes, got %v", fresh) + } +} + +func TestNewRDSAPIRequiresARegion(t *testing.T) { + if _, err := NewRDSAPI(context.Background(), " "); err == nil { + t.Fatal("a blank region must be refused: it selects the rate-card row for every instance") + } + if _, err := NewCloudWatchAPI(context.Background(), ""); err == nil { + t.Fatal("a blank region must be refused") + } +} + +// ---- helpers ---- + +func collectEnvelope(t *testing.T, a *RDSAPI, ids ...string) krds.Envelopes { + t.Helper() + ec := krds.NewEnvelopeCollector(a, krds.EnvelopeCollectorConfig{ + Window: krds.Window{Start: refNow.Add(-48 * time.Hour), End: refNow}, + }) + env, err := ec.Collect(context.Background(), ids) + if err != nil { + t.Fatalf("envelope collect: %v", err) + } + return env +} + +func countCalls(f *fakeRDS, op string) int { + f.mu.Lock() + defer f.mu.Unlock() + n := 0 + for _, c := range f.calls { + if c == op { + n++ + } + } + return n +} + +func hasNote(notes []string, substr string) bool { + for _, n := range notes { + if strings.Contains(n, substr) { + return true + } + } + return false +}