diff --git a/architecture/layering/allowlist_test.go b/architecture/layering/allowlist_test.go index 0792a0d28..d6c7d468c 100644 --- a/architecture/layering/allowlist_test.go +++ b/architecture/layering/allowlist_test.go @@ -122,9 +122,6 @@ var allowlist = []allowance{ {From: "gateway/sts", To: "handlers/iam", Debt: "IAM implementation consumers in STS, Ochre and ELBv2", Reason: "GetCallerIdentity takes IAMService; needs an IAM capability"}, - {From: "gateway/tagging", To: "domains/ec2/tags", Debt: "close-out: gateway/tagging", Reason: "cross-service GetResources; clears when domains/tagging aggregates through per-domain capabilities"}, - {From: "gateway/tagging", To: "handlers/elbv2", Debt: "close-out: gateway/tagging", Reason: "cross-service GetResources; clears when domains/tagging aggregates through per-domain capabilities"}, - {From: "handlers/ecs", To: "domains/ec2/ebs/metadata", Debt: "close-out: handlers/ecs (ADR-0004 blocked-dependency list; inventory-ecs boundary imports)", Reason: "legacy ECS calls another domain's implementation as an internal API"}, {From: "handlers/ecs", To: "domains/ec2/eip", Debt: "close-out: handlers/ecs (ADR-0004 blocked-dependency list; inventory-ecs boundary imports)", Reason: "legacy ECS calls another domain's implementation as an internal API"}, {From: "handlers/ecs", To: "domains/ec2/instancetypes", Debt: "close-out: handlers/ecs (ADR-0004 blocked-dependency list; inventory-ecs boundary imports)", Reason: "legacy ECS calls another domain's implementation as an internal API"}, diff --git a/architecture/layering/layers_test.go b/architecture/layering/layers_test.go index 060ab5f51..46c0df198 100644 --- a/architecture/layering/layers_test.go +++ b/architecture/layering/layers_test.go @@ -67,7 +67,7 @@ var legacyRoots = map[string]bool{ // Bedrock is the AWS surface of the Ochre domain. var gatewayDomains = map[string]string{ "bedrock": "ochre", "ecs": "ecs", "eks": "eks", "elbv2": "elbv2", "iam": "iam", - "rds": "rds", "sts": "sts", "tagging": "tagging", + "rds": "rds", "sts": "sts", } // runtimeLegacy and operatorLegacy are the legacy roots whose ADR-0001 target diff --git a/architecture/layering/legacy_test.go b/architecture/layering/legacy_test.go index b413f386f..04b8d309e 100644 --- a/architecture/layering/legacy_test.go +++ b/architecture/layering/legacy_test.go @@ -21,7 +21,6 @@ var frozenLegacyPackages = []string{ "gateway/rds", "gateway/spx", "gateway/sts", - "gateway/tagging", "handlers/ecs", "handlers/eks", "handlers/elbv2", diff --git a/docs/PACKAGE_BOUNDARY_MIGRATION.md b/docs/PACKAGE_BOUNDARY_MIGRATION.md index 82338cab2..1a5f75c21 100644 --- a/docs/PACKAGE_BOUNDARY_MIGRATION.md +++ b/docs/PACKAGE_BOUNDARY_MIGRATION.md @@ -337,6 +337,9 @@ while the branch is in flight; they do not describe the umbrella repository's | `3fdaa54d9` | `spinifex/admin` (`DefaultAccountID`, `DefaultAccountName`) | `spinifex/foundation/aws/identifiers` | The fixed, platform-reserved bootstrap account identifiers join `GlobalAccountID`: the ID and name of the first human-facing account created during bootstrap, used by `daemon`, `gateway`, `runtime/roles/awsgw`, `accountteardown` and `operator/cli`. Function names and values are unchanged, and they stay fixed rather than configurable. Callers use the existing `awsidentifiers` alias; `daemon`, `gateway`, `runtime/roles/awsgw`, `accountteardown` and the other `operator/cli` files drop their `admin` import, and `operator/cli/admin.go` keeps `legacyadmin` only for the credential generators. The `TestDefaultAccountID` assertion moves with the function, plus a name pin and a check that the ID is valid and distinct from `GlobalAccountID`. `admin` now holds only the IAM credential generators. | | `254d898a3` | `spinifex/handlers/sts` (`oidc_jwks.go`: the `handlers_eks.AccountBucketName`/`OIDCJWKSKey` import) | `spinifex/handlers/eks` (`OIDCJWKSReader`, a `ClusterJWKSResolver` implementation), wired by `spinifex/runtime/roles/awsgw` | The Q-22 STS-to-EKS dependency is cut by a consumer-owned resolver: `handlers/sts` declares `ClusterJWKSResolver` (raw-bytes lookup, the same found/error split `FetchClusterJWKS` already had), and `handlers/eks` implements it beside `AccountBucketName`/`OIDCJWKSKey` with the same bucket-open-then-get, timeouts and not-found classification `oidc_jwks.go` used. STS still decodes the JWKS, applies the empty-keys rule and maps every outcome to `InvalidIdentityToken`, unchanged. `runtime/roles/awsgw` and `tests/integration/harness.go` construct the resolver and pass it to `NewSTSServiceImpl`, now a required, nil-checked argument; the compile-time interface check lives in `awsgw` because `handlers/eks` must not import `handlers/sts`. The literal bucket/key, KV-dependency-failure and cluster-status-not-consulted characterisation tests move to `handlers/eks`, next to the layout they pin; every remaining `handlers/sts` test, including the web-identity fixtures and `newTestSetup`, is rewritten against a test-owned `fakeJWKSResolver` and imports no EKS package. The one test proving an EKS-minted issuer URL parses through STS's `ParseEKSIssuerURL` moves to `tests/integration` as a composition test. The stale `handlers/sts` → `handlers/eks` allowlist entry is deleted. | | `0368e861a` | `spinifex/admin` (`accounts.go`: `GenerateAWSAccessKey`, `GenerateAWSSecretKey`, and their format and uniqueness tests) | `spinifex/foundation/aws/credentials` (`awscredentials`) | A stateless AWS credential-format primitive: it generates an `AKIA` + 16-alphanumeric access key ID and a 40-character base64 secret key, and issues, stores and authorises nothing. It is not IAM issuance and does not implement ADR-0005 S5. Output, randomness source, errors and names are unchanged; only the package line and doc comment changed. It is deliberately not unified with `handlers/iam`'s internal `generateAccessKeyID`, whose IDs are `AKIA` + 20 uppercase hex characters. `handlers/iam` and `operator/cli/admin.go` import `awscredentials`; `operator/cli` no longer imports any `legacyadmin`. The `spinifex/admin` directory is deleted: `admin` leaves the legacy-root, frozen-package and operator-legacy lists, joins the retired roots so it cannot return, and its two allowlist entries (`handlers/iam` → `admin`, `operator/cli` → `admin`) are removed. Operator bootstrap still generates the admin and Northstar credentials directly, recorded as debt below. | +| `0d830e901` | — | `spinifex/domains/tagging` (`TaggedLister`, `TaggedResource`) | The read-only list capability for the Resource Groups Tagging aggregation, declared by its consumer per ADR-0001. `ListTagged` takes the caller's account and the lower-cased `ResourceTypeFilters` set, where an entry is a full `service:type` or a bare service, and returns ARNs with read-only tag maps. The package imports no domain, handler or contract package and the layering ratchet classifies it with no allowlist row. It carries no mutation, limit or ever-tagged surface. | +| `f3545e3ea` | `spinifex/gateway/tagging` (`listEC2`, `buildEC2Mappings`, `ec2ARN`, `elbv2Resources`, `matchesType`), copied | `spinifex/runtime/roles/awsgw` (`ec2IndexLister`, `elbv2Lister`), `spinifex/domains/tagging` (`MatchesType`) | AWSGW-side NATS client adapters implement `tagging.TaggedLister`, each with its compile-time check beside it in the composition root; no owner package carries one. The EC2 index adapter wraps `ec2.DescribeTags` and builds each ARN in the gateway's region from the stored resource type, including the unresolved-ID shapes today's code produces. The ELBv2 adapter lists load balancers and target groups admitted by the type filter, tagged or not, and reads tags in `DescribeTags` batches of 20. Both reuse existing subjects and clients, so no wire contract is added, and each call builds fresh tag maps. Type matching is exported once from `domains/tagging` so no copy survives the move. Embedded-JetStream tests cover type filters including a bare service, untagged ELBv2 resources, EC2 ARN mapping, account scoping, batch sizes and owner error propagation. | +| `aedd0b2c4` | `spinifex/gateway/tagging` (`GetResources` parsing, tag filtering, sorting, offset token, authorization, response writing) | `spinifex/domains/tagging` (`Aggregator`, `NewAggregator`, `HasScope`, `ScopedActions`), wired by `spinifex/runtime/roles/awsgw` | The aggregation moves unchanged and consumes `[]TaggedLister`: ELBv2 is called before EC2, the non-stable ARN sort is kept, lister errors propagate with the same codes, and tag maps return as key-sorted tag lists with `[]` for untagged resources. AWSGW builds the aggregator with both adapters and sets the new `GatewayConfig.TaggingResources` field; the dispatch in `gateway/tagging.go` stays and still answers `ServerInternal` when NATS or the aggregator is not wired. The `TestCurrentBehaviour_Tagging_GetResources*` tests move to `runtime/roles/awsgw` with unchanged assertions, seeding EC2 tags through the existing EC2 tags client over NATS; pure aggregator tests move to `domains/tagging` against fake listers; the gateway keeps dispatch and error-path tests. The gateway's private copies of the listing helpers and the legacy NATS lister are deleted with their tests, `gateway/tagging` is removed, and its two allowlist rows (`gateway/tagging` → `domains/ec2/tags`, `gateway/tagging` → `handlers/elbv2`) and its frozen-legacy entry go with it. | The EC2 contract row is intentionally different from the directory moves: it creates a compatibility boundary. `ec2.cmd.*` retains its deployed one-token NATS subscription and permission shape, while callers use @@ -417,6 +420,7 @@ These residuals were recorded by individual slices and must be resolved once the - `domains/dns`: imports `runtime/compute/vm` (domain-to-runtime), `domains/network/reconcile` for the reconcile lock and `bootstrap/config` for node configuration. - ACM: the IAM AES-GCM helper import (ACM relocation residual). - `runtime/roles/awsgw`: the quota reconcile loop belongs to `domains/admission/quota`, and the EKS token-verify responder belongs with STS or EKS identity. +- `runtime/roles/awsgw` → `handlers/elbv2` (inherited from `gateway/tagging`): this is a new physical import in AWSGW that inherits the former `gateway/tagging` dependency, because the `elbv2Lister` adapter builds its client with `handlers_elbv2.NewNATSELBv2Service` and calls the `ELBv2Service` interface. It is permitted temporarily because AWSGW is the composition root, which may wire legacy packages, so the layering test needs no allowlist row. Removal condition: it must clear when ELBv2 exposes a domain-owned boundary (its NATS client or contract under its domain owner) and the adapter consumes that instead. - `runtime/roles/vpcd` (placement): the role still mixes process/role supervision with network-controller responsibilities. `lease_reaper.go` (orphaned external-pool DHCP lease sweep), `ovn_controller_watchdog.go` (OVN controller liveness and restart), `host_eip_loop.go` (host-EIP reconcile loop) and `lease_datapath_reconcile.go` (lease datapath reconcile) are periodic network-control loops, not role wiring; `vpcd.go` itself also carries bridge-mode detection and verification, chassis-list parsing, external-CIDR resolution, DHCP manager startup and lease/IP-change handling, and gateway/host-EIP allocator and neighbour flush/prime construction alongside `Service.Start`. The future placement question is whether these belong in `domains/network`. Removal condition: an accepted decision on network-controller placement, and extraction of those loops and helpers behind the role. - ECR control-plane registration: `awsapi.Deps.NATS` is a raw `*nats.Conn`, not a narrow capability. Every NATS-backed `Handler` passes it to `domains/ecr.NewNATSMetaStore`, which calls `natsmsg.NATSRequest`, so narrowing it means changing the exported `Handler` type, every handler and the meta store; it must not be broadened further meanwhile. - ACM control-plane registration: `domains/acm/awsapi.Deps.NATS` (`registration.go`) is likewise a raw `*nats.Conn` carried by every `Handler`, for the same reason; it must not be broadened further meanwhile. @@ -657,7 +661,7 @@ series above; "needs decision" is open. | `gateway/bedrock` | The Ochre domain core, its stores and the inference engine, together with the Bedrock action layer, in one package with internal cycles. | Deferred: needs the split plan recorded above — domain core and stores to `domains/ochre/`, inference to an Ochre-owned package, and only the action layer to `domains/ochre/awsapi`. | | `gateway/{ecs,eks,rds,iam,sts,elbv2}` | Each service's AWS action adapters and dispatch, not yet registered through ADR-0002. | Deferred: the ADR-0002 registration contract has only reached ECR and ACM so far; each service needs its own registration slice. | | `gateway/spx` | `promote_image.go` and `spx.go` dispatch (`storage.go` already consumes `contracts/operator/v1`). | Needs decision: which package owns operator-plane action dispatch registration — an `ingress/aws`-style seam for the operator plane, or `gateway` stays its home. | -| `gateway/tagging` | The cross-service tagging API (`GetResources`) and its authorization. | Decided: a new `domains/tagging` boundary owns the cross-resource tagging API and aggregation; individual resource domains keep ownership of their own tag storage and expose narrow tagging capabilities. Stays debt until those per-domain capabilities exist; clears when `domains/tagging` aggregates only through them. | +| `gateway/tagging` | — | Done. `domains/tagging` owns the read-only cross-resource `GetResources` aggregation and its API semantics (`0d830e901`, `aedd0b2c4`); AWSGW composes the owner listers, the EC2 tag index and ELBv2 (`f3545e3ea`); the root `gateway` keeps only the legacy dispatch in `gateway/tagging.go`. The package is removed, and its two allowlist rows and frozen-legacy entry are gone. Owner tag mutation capabilities remain future direction, so the RDS and ECS tag-writing edges above stay open. | | `services/viperblockd` | — | Done. The adapter is `providers/ebs/viperblock` (with the read-only `dirty` reader), the shared vocabulary is `contracts/viperblockd/legacy/v1`, and the role is `runtime/roles/viperblockd`. `services/` is empty. | | `utils` | — | Done and deleted. `ec2_subject.go` went to `contracts/ec2/v1`, the vpcd protocol to `contracts/network/v1` with the typed client in `domains/network/projection`, and the master-key loader to `domains/ec2/ebs/encryption`. | | `vpcd` | — | Done. The IMDS local-file reader and record-space opener moved to `domains/ec2/instance/state` and are injected through `guestmetadata.LocalStateReader`/`RecordLoader` (recorded above), so `vpcd` no longer imports `daemon` directly; its inter-process boundary now exists (`contracts/network/v1`, with `domains/network/subscribers` as the consumer and `domains/network/projection` as the producer client). The package moved unchanged to `runtime/roles/vpcd` (recorded below), and the legacy directory is removed. | @@ -677,7 +681,7 @@ ADR-0001 completion is structural, so none of these is cleared by remediating se |---|---|---| | Explicitly deferred by ADR-0001 | `daemon` (Q-28), `lbagent` (Q-30) | Closed for this refactor; the ADR names the prerequisite. | | No missing boundary (candidate moves) | `handlers/ecs/bus` → `contracts/ecs/v1` (`a130f6b8e`); `admin/images_promote.go` → `domains/ec2/image` (`263c58167`); `admin` bootstrap account identifiers → `foundation/aws/identifiers` (`3fdaa54d9`); `admin` credential-format generators → `foundation/aws/credentials` `0368e861a`, retiring `admin` | Done. | -| One narrow missing boundary | Done: `handlers/sts` EKS JWKS lookup → `handlers/eks` `ClusterJWKSResolver` (`254d898a3`). Open: `gateway/tagging` (per-domain tagging capabilities, owner decided); `gateway/spx` (owner of operator-plane dispatch registration) | Each needs one decision or one capability, then a mechanical move. | +| One narrow missing boundary | Done: `handlers/sts` EKS JWKS lookup → `handlers/eks` `ClusterJWKSResolver` (`254d898a3`); `gateway/tagging` → `domains/tagging` with AWSGW-composed listers (`0d830e901`, `f3545e3ea`, `aedd0b2c4`). Open: `gateway/spx` (owner of operator-plane dispatch registration) | Each needs one decision or one capability, then a mechanical move. | | Broader boundary or follows another package | `handlers/iam` and `handlers/iam/mock` (per-consumer IAM capabilities); `handlers/{ecs,eks,rds}` (ADR-0004 blocked-dependency lists); `handlers/elbv2` (data-plane boundary evidence); `accountteardown` (domain cleanup capabilities); `gateway` root (ADR-0002 auth and request-context phase); `gateway/{iam,sts,ecs,eks,rds,elbv2}` (each follows its `handlers/`, because a new-path `awsapi` cannot import a legacy handler); `gateway/bedrock` (the recorded Ochre split plan) | Recorded debt with removal conditions above. | Per-consumer IAM capabilities are the widest unblocker: they gate `handlers/iam` itself, STS's IAM reads, Ochre's IAM reads, the ELBv2 role path and the ECS, EKS and RDS ensurers. diff --git a/docs/package-boundary/tagging-capabilities-discovery.md b/docs/package-boundary/tagging-capabilities-discovery.md new file mode 100644 index 000000000..e27e6c619 --- /dev/null +++ b/docs/package-boundary/tagging-capabilities-discovery.md @@ -0,0 +1,516 @@ +# Tagging capabilities discovery + +**Status:** Discovery only. +Nothing here is a decision. +It records current behaviour at the head of the ADR-0001 checkpoint branch and proposes a capability shape for review. +The ADR-0001 structural cut in [section 4](#4-proposal-the-adr-0001-structural-cut) has since been implemented; the [future direction](#future-direction-outside-the-adr-0001-structural-cut) has not. + +## Scope and method + +This covers every Spinifex path that reads or writes resource tags, grouped by the owner of the record the tags live on. +File references are relative to the repository root at that revision. +References to `gateway/tagging` describe the code before the structural cut moved `GetResources` to `domains/tagging` and the listers to `runtime/roles/awsgw`. + +What was measured: + +- Code reading of every tag entry point found by searching for `Tag`/`Untag`/`AddTags`/`RemoveTags`/`ListTags`/`DescribeTags` handlers and the EC2 record-mirror fan-out. +- Sixteen characterisation tests named `TestCurrentBehaviour_Tagging_*` (listed under [Tests](#tests)), run through real handlers on embedded JetStream. +- AWS documentation fetched on 2026-10-13 for the pages cited in [AWS parity](#aws-parity). + +What was not measured: + +- No live cluster run, so multi-node interleavings are shown by injecting a write between a handler's read and its write, not by real concurrent nodes. +- No test drives the full daemon CreateTags path including record mirrors on a multi-node cluster. +- Predastore's own S3 bucket tagging was not exercised; Spinifex does not read it. + +Related work this does not duplicate: + +- `mulga-6maav`: CreateTags accepts unvalidated or nonexistent resource IDs. +- `mulga-5eh43`: IAM CreatePolicy rejects `aws:RequestTag` condition keys. +- `mulga-lmq4l`: tag-based ABAC condition keys. +- `docs/development/bugs/concurrent-createtags-lost-updates.md` (mulga repo): moved the EC2 central index to JetStream KV under CAS (`mulga-tt77o`, closed). + That plan's "Not done" list leaves the two-store split in place, which is where the record-mirror finding below sits. +- `docs/development/bugs/creation-time-tags-describetags.md` (mulga repo): creation-time tags reach the central index only for some EC2 owners. + +## 1. Map of tag reads and writes by owner + +### EC2 and the VPC family + +EC2 has two tag stores. + +- **Central index:** JetStream KV bucket `ec2-tags`, key `{accountID}.{resourceID}`, value the whole tag map, owned by `TagsServiceImpl` (`spinifex/spinifex/domains/ec2/tags/service_impl.go:50`). + A legacy Predastore object `tags/{accountID}/{resourceID}.json` is read through and deleted on teardown (`service_impl.go:129`, `:156`). + `DescribeTags` and `GetResources` read only this store. +- **Resource record:** each owning service's own KV or metadata record carries a `Tags` map. + Each resource's own describe call and its tag filters read only this. + +Write paths: + +| Path | Package | Writes | Concurrency | +| --- | --- | --- | --- | +| AWSGW `CreateTags` validation | `domains/ec2/awsapi/tags/CreateTags.go:15` | nothing; refuses empty key and `aws:` prefix | n/a | +| Daemon `createTags` / `deleteTags` | `daemon/daemon_handlers_tag.go:89`, `:125` | central index for non-instance IDs, then every record mirror, then instance owners | sequential, first error returns | +| Central index merge | `domains/ec2/tags/service_impl.go:195`, `:377`; `store.go:77` | `ec2-tags` | CAS via `kvutil.Update` | +| Central index overwrite | `domains/ec2/tags/store.go:102` (`PutResourceTags`) | `ec2-tags` | CAS put | +| Record mirrors (`ApplyRecordTags`/`RemoveRecordTags`) | vpc `domains/ec2/vpc/service_impl.go:1091`, `:1110`, `:1166`; image `domains/ec2/image/service_impl.go:681`; volume `domains/ec2/volume/service_impl.go:1175`, `:1208`; snapshot `domains/ec2/snapshot/service_impl.go:997`; route table `domains/ec2/routetable/service_impl.go:1588`; igw `domains/ec2/igw/service_impl.go:598`; eigw `domains/ec2/eigw/service_impl.go:277`; eip `domains/ec2/eip/service_impl.go:938`; natgw `domains/ec2/natgw/service_impl.go:538`; key pair `domains/ec2/key/service_impl.go:1032` | each owner's record | plain read then `Put` (`domains/ec2/tagmirror/tagmirror.go:83`), no revision | +| Instance tags | `daemon/daemon_handlers_tag.go:194` routes to the owning daemon over `ec2.cmd.`, or `TagStoppedInstance` when no responder | instance record and central index together | ownership-gated | +| Creation-time `TagSpecifications` | each create path | record; central index only for instance, vpc/subnet/sg/eni (`domains/ec2/vpc/service_impl.go`, `projectRecordTags`) and key pair (`domains/ec2/key/service_impl.go:55`) | central write failure is logged and swallowed | + +The daemon fan-out (`daemon/daemon_handlers_tag.go:34`) calls every mirror with every non-instance ID, and each mirror skips IDs whose prefix it does not own. +`launchtemplate` and `placementgroup` are not in the mirror list, so a `CreateTags` on them reaches only the central index. +That last point is from code and is not tested. + +Read paths: + +- `DescribeTags` lists every key in the bucket and filters by account prefix (`domains/ec2/tags/service_impl.go:243`, `:290`). + An ID with no recognised prefix is reported with resource type `unknown` (`:84`). +- Resource describes filter on their own record's tags (for example `domains/ec2/instance/service_impl.go:2040`). + +Cross-owner reach: the daemon is the composition point that writes the central index and then reaches every record owner; the record mirrors each write only their own record. + +### ELBv2 + +- **Store:** load balancer, target group, listener and rule records, each with a `Tags` map (`spinifex/spinifex/handlers/elbv2/service_impl_tags.go:25`). +- **Writes:** `AddTags` and `RemoveTags` (`service_impl_tags.go:101`, `:149`). + Load balancer, listener and rule tag writes re-put the whole record read earlier (`:41`, `:74`, `:88`). + Target group writes go through `UpdateTargetGroupByArn` CAS, but the merged map is computed from the earlier read and assigned inside the CAS closure (`:57`). +- **Reads:** `DescribeTags` returns one entry per ARN, tagged or not (`handlers/elbv2/service_impl.go:3041`). +- **Cross-owner reach:** `gateway/tagging` calls `DescribeLoadBalancers`, `DescribeTargetGroups` and `DescribeTags` over NATS (`spinifex/spinifex/gateway/tagging/GetResources.go:106`). + +### RDS + +- **Store:** DB instance, DB snapshot, subnet group and parameter group records in the account bucket (`spinifex/spinifex/handlers/rds/tags.go:56`). +- **Writes:** `AddTagsToResource`, `RemoveTagsFromResource` (`tags.go:104`, `:141`) through a registry of `TaggedRecord` kinds, with a revision-checked update retried up to 16 times (`tags.go:207`). +- **Validation:** 50 tags after merge, 128-character key, 256-character value, `aws:` refused (`tags.go:21`, `:236`). +- **ARN check:** region and account must match the caller (`handlers/rds/arn.go:66`, `:69`). +- **Cross-owner reach:** `handlers/rds/tags.go` writes `domains/rds/subnetgroup` and `domains/rds/parametergroup` records directly (`tags.go:53`, `:67`, `:72`). + These are the `handlers/rds` → `domains/rds/subnetgroup` and `handlers/rds` → `domains/rds/parametergroup` legacy edges in the `docs/PACKAGE_BOUNDARY_MIGRATION.md` "Dependency-rule debt to clear after the moves" list. + +### ECS + +- **Store:** cluster, task definition revision, service, task and container instance records in the account bucket (`spinifex/spinifex/handlers/ecs/tags.go:77`). +- **Writes:** `TagResource`, `UntagResource` (`tags.go:228`, `:255`) via `mutateResourceTags` (`tags.go:141`), which re-puts the whole record with no revision. +- **ARN check:** `parseResourceARN` keeps only the resource segment (`tags.go:41`); the ARN's account and region are not compared with the caller's. +- **Validation:** none on count, length or prefix. +- **Cross-owner reach:** reads and writes `domains/ecs/taskdefinition` records directly (`tags.go:89`, `:154`), the `handlers/ecs` → `domains/ecs/taskdefinition` legacy edge in the `docs/PACKAGE_BOUNDARY_MIGRATION.md` "Dependency-rule debt to clear after the moves" list. + +### EKS + +- **Store:** cluster meta and nodegroup records (`spinifex/spinifex/handlers/eks/service_impl.go:1763`). +- **Writes:** `TagResource`, `UntagResource` (`:1763`, `:1806`) under CAS (`casUpdateMeta`, `casUpdateNodegroupTags`). + Other EKS ARNs return NotImplemented. +- **ARN check:** `clusterNameFromARN` and `nodegroupRefFromARN` discard account and region (`:1925`, `:1940`). +- **Validation:** none on count, length or prefix. + +### ECR + +- **Store:** repository meta record (`RepoMeta.Tags`) in the account's ECR KV bucket (`spinifex/spinifex/domains/ecr/meta.go:163`). +- **Writes:** `TagResource`, `UntagResource` in the AWS adapter (`spinifex/spinifex/domains/ecr/awsapi/tags.go:59`, `:90`), which reads the whole meta over NATS, edits it in the gateway process, and puts it back with a plain `kv.Put`. +- **ARN check:** `RepositoryNameFromResourceARN` keeps only the repository name (`domains/ecr/awsapi/authz.go:217`). +- **Validation:** missing or empty key only (`domains/ecr/awsapi/validation.go:64`). + +### ACM + +- **Store:** certificate record (`CertRecord.Tags`) (`spinifex/spinifex/domains/acm/store.go:223`). +- **Writes:** `AddTagsToCertificate`, `RemoveTagsFromCertificate` (`spinifex/spinifex/domains/acm/service_impl.go:724`, `:751`), through `Store.UpdateTags` (`store.go:502`). + Both check ownership through `lookupOwned`, then change only `Tags` through `Store.UpdateTags`, a revision-checked update of the raw record that leaves the encrypted private key untouched. + Before `mulga-ncn9f` was fixed they wrote back the `GetCertMetadata` copy through `PutCert`, which replaced the stored key with an empty one. +- **Validation:** none on count, length or prefix. + +### IAM + +- **Store:** role, user, customer-managed policy, instance profile and OIDC provider records in IAM KV buckets. +- **Writes:** `TagRole`/`UntagRole` (`spinifex/spinifex/handlers/iam/roles.go:521`, `:545`), `TagPolicy`/`UntagPolicy` (`handlers/iam/service_impl.go:1942`, `:1965`) and `TagInstanceProfile`/`UntagInstanceProfile` (`handlers/iam/instance_profiles.go:300`, `:325`) use CAS. + `TagUser`/`UntagUser` (`handlers/iam/service_impl.go:1871`, `:1893`, `:1919`) and `TagOpenIDConnectProvider`/`UntagOpenIDConnectProvider` (`handlers/iam/oidc_providers.go:293`, `:311`, `:334`) read and then plain `Put`. +- **Validation:** count, key and value length, pattern, `aws:` prefix and duplicate keys (`handlers/iam/service_impl.go:279`). + +### Resources with no Spinifex tag path + +- Lambda and CloudWatch Logs have no package under `spinifex/spinifex/`. +- Bedrock (`gateway/bedrock*`, `domains/ochre`) has no tag action. +- S3 bucket tagging is served by Predastore (`predastore/internal/gate/handlers/bucket_tagging.go`); Spinifex's `GetResources` does not list buckets. + +### Cross-service tagging API + +- `gateway/tagging.go:19` serves only `GetResources`; any other action, including `TagResources`, `UntagResources`, `GetTagKeys` and `GetTagValues`, returns `InvalidAction` (`gateway/tagging.go:41`). +- Authorization evaluates `tagging:GetResources` against `*` (`spinifex/spinifex/gateway/tagging/authz.go:28`). +- `GetResources` aggregates ELBv2 load balancers and target groups plus the EC2 central index (`gateway/tagging/GetResources.go:45`), then filters by tag, sorts by ARN and slices by an integer offset token (`:264`). +- The layering ratchet records the two cross-owner imports as debt (two `gateway/tagging` rows in `architecture/layering/allowlist_test.go`), and `gateway/tagging` is a frozen legacy package (`architecture/layering/legacy_test.go`). + +### Authorization against tags + +- The policy evaluator allowlists five condition keys and none read a tag (`bluebottle/pkg/iampolicy/conditions.go`). +- IAM `CreatePolicy` refuses any unsupported key at write time (`handlers/iam/service_impl.go:2496`), with a specific reason for `aws:RequestTag/` (`:2514`). +- No code path reads a resource's tags to evaluate a request. + +### Summary table + +| Owner | Tag store | Service write API | Write concurrency | Limits enforced | ARN account checked | Listed by `GetResources` | Cross-owner reach | +| --- | --- | --- | --- | --- | --- | --- | --- | +| EC2 central index | `ec2-tags` KV | `CreateTags`/`DeleteTags` | CAS | `aws:` prefix only, at AWSGW | IDs are not ARNs; stored under caller account | yes | daemon fans out to record owners | +| EC2 record owners (11 services) | own record | via daemon mirror | plain read then put | none | n/a | no (index only) | none | +| EC2 instance | instance record and index | via owning daemon | ownership-gated | `aws:` at AWSGW | n/a | yes, via index | none | +| ELBv2 | LB/TG/listener/rule records | `AddTags`/`RemoveTags` | LB, listener, rule plain put; TG CAS over a stale merge | empty key only | owner field compared | LB and TG only | `gateway/tagging` reads over NATS | +| RDS | four record kinds | `AddTagsToResource`/`RemoveTagsFromResource` | CAS, 16 attempts | 50/128/256, `aws:` | yes, and region | no | writes `domains/rds` subnet and parameter group records | +| ECS | five record kinds | `TagResource`/`UntagResource` | plain read then put | none | no | no | writes `domains/ecs/taskdefinition` records | +| EKS | cluster meta, nodegroup | `TagResource`/`UntagResource` | CAS | none | no | no | none | +| ECR | repo meta | `TagResource`/`UntagResource` | plain read then put, from the gateway process | empty key only | no | no | none | +| ACM | certificate record | `AddTagsToCertificate`/`RemoveTagsFromCertificate` | revision-checked update of the tag field only | none | owner checked via `lookupOwned` | no | none | +| IAM | five record kinds | `Tag*`/`Untag*` | CAS for role, policy, instance profile; plain put for user, OIDC provider | full | names, not ARNs, except policy and OIDC | no | none | + +## 2. Error and atomicity behaviour + +Each line states current behaviour and how it was established. +"Test" names a `TestCurrentBehaviour_Tagging_*` test listed under [Tests](#tests); "code" means read from source and not tested here; "INFERRED" means a consequence of code that no test demonstrates. + +### Read-modify-write and revision checks + +- EC2 record mirror: a tag write and a non-tag record change that both land between the mirror's read and its put are reverted, and every call reports success. + Test: `RecordMirrorRevertsInterleavedWrites`. + Every record owner that routes through `tagmirror.UpdateKVRecordTags` shares this; volume and image use their own read-then-put with the same shape (INFERRED for those two). +- EC2 central index: CAS, so concurrent merges on one resource keep each other's keys. + Code, and the closed plan's tests. +- ECS: the tag write reverts a record change committed between its read and its put. + Test: `ECSTagWriteRevertsInterleavedRecordWrite` (an interleaved `DesiredCount` change is lost). +- ACM: tag writes are revision-checked and change only the tag field; the stored private key survives (resolved by `mulga-ncn9f`). + Test: `ACMTagWritesPreserveStoredPrivateKey`. + A certificate that lost its key before the fix can no longer be served by any listener that loads it (INFERRED); recovery is `mulga-o436p`. +- ELBv2: load balancer, listener and rule tag writes can revert a concurrent record change, and the target group CAS assigns a map merged from an earlier read, so two concurrent `AddTags` on one target group can lose a key. + INFERRED from `service_impl_tags.go:41`, `:57`, `:74`, `:88`. +- ECR, IAM user, IAM OIDC provider: read then plain put; concurrent tag writes can lose a key. + INFERRED. +- RDS, EKS, IAM role, policy and instance profile: revision-checked; code. + +### Partial failure across resources in one call + +- ELBv2 `AddTags` writes each ARN before resolving the next, so an unknown ARN after a valid one fails the call with the first resource already tagged. + Test: `ELBv2AddTagsPartiallyAppliesBeforeUnknownARN`. +- EC2 `CreateTags`/`DeleteTags`: the central loop stops at the first failing resource after committing earlier ones; the daemon then writes the record mirrors only if the central write succeeded, and a mirror error is returned after the central index is already committed. + Code (`daemon/daemon_handlers_tag.go:98`); the resulting index/record divergence is INFERRED. +- Instance IDs are tagged concurrently and the first error in input order is returned; other instances may already be tagged. + Code (`daemon/daemon_handlers_tag.go:172`). + +### Validation limits + +- EC2: `aws:` refused at AWSGW with `InvalidParameterValue`; no count, key-length or value-length limit. + Test: `EC2CreateTagsAppliesNoLengthOrCountLimits` (51 tags, a 129-character key and a 257-character value all stored). +- ELBv2, ECS, ACM: no count, length or `aws:` rule. + Tests: `ELBv2AddTagsAppliesNoTagLimits`, `ECSTagResourceAppliesNoTagLimits`, `ACMAddTagsAppliesNoTagLimits`. +- ECR: empty key only; EKS: none. + Code. +- RDS and IAM: AWS limits enforced. + Code. + +### Unknown, nonexistent and cross-account resources + +- EC2: a well-formed ID that names nothing, an ID with no recognised prefix, and another account's ID are all accepted and stored under the caller's account. + Test: `EC2CreateTagsStoresUnresolvedIDsInCallerNamespace` (extends `mulga-6maav`). +- `GetResources` then lists those IDs, an unrecognised one as `arn:aws:ec2:::unknown/`. + Test: `GetResourcesListsUnresolvedEC2IDs`. +- ECS: an ARN naming another account and region resolves to the caller's same-named resource and tags it. + Test: `ECSForeignAccountARNTagsCallerResource`. +- EKS and ECR discard the ARN account the same way. + Code; the effect is INFERRED. +- ELBv2: unknown or foreign ARNs return the per-type not-found code; RDS returns `InvalidParameterValue` for a foreign account or region. + Code. + +### Error codes + +| Owner | Unknown resource | Bad tag | Store failure | +| --- | --- | --- | --- | +| EC2 | none (accepted) | `ErrorInvalidParameterValue` (`aws:` and empty key only) | `ErrorServerInternal` | +| ELBv2 | `ErrorELBv2LoadBalancerNotFound`, `ErrorELBv2TargetGroupNotFound`, `ErrorELBv2ListenerNotFound`, `ErrorELBv2RuleNotFound` | `ErrorInvalidParameterValue` (empty key only) | `ErrorServerInternal` | +| RDS | the kind's not-found fault (`tags.go:60`) | `ErrorInvalidParameterValue` | error passed through | +| ECS | `ErrorECSClusterNotFound`, `ErrorECSServiceNotFound`, otherwise `ErrorECSInvalidParameter` | none | error passed through | +| EKS | `ErrorEKSResourceNotFound` for clusters (`service_impl.go:1958`); other ARNs `ErrorNotImplemented` | none | error passed through | +| ECR | `RepositoryNotFoundError` | `ErrorInvalidTagParameter` (empty key only) | error passed through | +| ACM | `ErrorACMInvalidArn` or `ErrorResourceNotFound` (`service_impl.go:614`; also when the certificate is deleted between the ownership check and the write) | none | `ErrorInternalError` | +| IAM | `ErrorIAMNoSuchEntity` | `validationError`, `ErrorIAMInvalidInput`, `ErrorIAMLimitExceeded` | error passed through | + +Names are `awserrors` constants or package helpers. +This table is from code. + +### GetResources during and after writes + +- A resource whose tags were all removed drops out of the result. + Test: `GetResourcesOmitsResourceWhoseTagsWereAllRemoved`. +- The pagination token is an integer offset into a set recomputed per call, so a resource tagged between page reads that sorts ahead of the offset makes the next page repeat an entry. + Test: `GetResourcesOffsetTokenRepeatsAfterInterleavedWrite`. + The same mechanism can skip an entry when a resource ahead of the offset is untagged (INFERRED). +- A non-integer token returns `InvalidParameterValue`, and `ResourcesPerPage` of 500 is accepted. + Test: `GetResourcesPaginationValidation`. +- Only EC2 index entries and ELBv2 load balancers and target groups are aggregated; any other type filter returns an empty list without error. + Test: `GetResourcesAggregatesOnlyEC2AndELBv2`. +- ELBv2 load balancers and target groups with no tags are included, because ELBv2 `DescribeTags` returns one entry per ARN. + Code (`handlers/elbv2/service_impl.go:3041`). + +### Authorization + +- No path supplies resource tags, request tags or tag keys to the policy evaluator. + Code. +- IAM `CreatePolicy` refuses policies conditioned on `aws:ResourceTag/`, `aws:RequestTag/`, `aws:TagKeys` and `ec2:ResourceTag/` with `MalformedPolicyDocument`. + Test: `CreatePolicyRefusesTagConditionKeys` (links `mulga-5eh43`, `mulga-lmq4l`). + +## 3. AWS parity + +Each item states AWS behaviour first, then ours. +Sources were fetched on 2026-10-13. + +- **Tag limits.** + AWS: 50 tags per resource, 128-character keys, 256-character values, keys and values case-sensitive, `aws:` reserved ([EC2 tag restrictions](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html#tag-restrictions); [ECS TagResource](https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_TagResource.html); [EKS TagResource](https://docs.aws.amazon.com/eks/latest/APIReference/API_TagResource.html) gives 50 entries, key 1..128, value up to 256; [ECR TagResource](https://docs.aws.amazon.com/AmazonECR/latest/APIReference/API_TagResource.html) gives `TooManyTagsException` at 50 and `InvalidTagParameterException`; [ACM AddTagsToCertificate](https://docs.aws.amazon.com/acm/latest/APIReference/API_AddTagsToCertificate.html) gives 1..50 items, `InvalidTagException` for `aws:`, `TooManyTagsException`; [ELBv2 AddTags](https://docs.aws.amazon.com/elasticloadbalancing/latest/APIReference/API_AddTags.html) lists `TooManyTags` and `DuplicateTagKeys`). + Ours: EC2, ELBv2, ECS, EKS, ECR and ACM do not enforce the count or lengths; ELBv2, ECS, EKS, ECR and ACM do not refuse `aws:`. + Divergence, verified by the limit tests. +- **EC2 nonexistent or malformed IDs.** + AWS: the error table defines `InvalidID` as "The specified ID for the resource you are trying to tag is not valid" ([EC2 error codes](https://docs.aws.amazon.com/AWSEC2/latest/APIReference/errors-overview.html)). + What AWS returns for a well-formed ID naming no resource on `CreateTags` is unverified. + Ours: accepted and stored. + The malformed-ID case is a divergence, verified by test. +- **EC2 cross-account IDs.** + AWS: "When you tag public or shared resources, the tags you assign are available only to your AWS account" ([EC2 tag restrictions](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html#tag-restrictions)). + AWS behaviour for an ID owned by another account and not shared with the caller is unverified. + Ours stores under the caller in both cases, which matches the documented shared case and is unverified for the unshared case. +- **EC2 consistency.** + AWS: the EC2 API "follows an eventual consistency model" ([EC2 error codes, Eventual consistency](https://docs.aws.amazon.com/AWSEC2/latest/APIReference/errors-overview.html#api-eventual-consistency)). + A lost tag write is not eventual consistency; the record-mirror revert is a defect regardless of this model. +- **Multi-resource atomicity.** + AWS: Resource Groups Tagging `TagResources` reports per-ARN failures in `FailedResourcesMap` ([TagResources](https://docs.aws.amazon.com/resourcegroupstagging/latest/APIReference/API_TagResources.html)), so partial success is part of that contract. + Whether ELBv2 `AddTags` or EC2 `CreateTags` is all-or-nothing across resources is unverified. + Ours: both apply partially before failing. +- **GetResources content.** + AWS: "`GetResources` does not return untagged resources", and without a tag filter "Resources that were previously tagged, but do not currently have tags, are shown with an empty tag set" ([GetResources](https://docs.aws.amazon.com/resourcegroupstagging/latest/APIReference/API_GetResources.html)). + Ours drops previously tagged resources with no tags (test) and includes never-tagged ELBv2 load balancers and target groups (code). + Both are divergences. +- **GetResources coverage.** + AWS: "All tagged resources, whether the tagging used the Resource Groups Tagging API or not, are returned" (same page). + Ours returns EC2 and two ELBv2 types only. + Divergence, verified by test. +- **GetResources pagination.** + AWS: `ResourcesPerPage` "minimum of 1 and a maximum value of 100"; invalid input is `InvalidParameterException`; a token is valid for at most 15 minutes, after which `PaginationTokenExpiredException` (same page). + Ours accepts 500 and returns `InvalidParameterValue` for a bad token. + Divergence, verified by test. + AWS's page consistency under concurrent writes is unverified, so the repeat on interleaved writes is reported as current behaviour, not as a parity gap. +- **TagResources and UntagResources.** + AWS: requires `tag:TagResources` and the owning service's tagging permission, for example `ec2:CreateTags`; at most 20 ARNs and 50 tags per call (TagResources page). + Ours: not served (code). +- **Tag condition keys.** + AWS: `aws:ResourceTag/` compares the tag on the resource the request is made against; `aws:RequestTag/` the tag passed in the request; `aws:TagKeys` the list of keys passed ([IAM global condition keys](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_condition-keys.html)). + Ours: refused at policy write and never evaluated. + Divergence, verified by test; tracked by `mulga-5eh43` and `mulga-lmq4l`. +- **ECS, EKS and ECR ARN with another account.** + AWS error for that case is unverified. + Ours acts on the caller's same-named resource (ECS by test, EKS and ECR by code). +- **ACM private key.** + No AWS documentation addresses this; tagging is metadata only on every AWS page fetched. + The key-loss defect is resolved (`mulga-ncn9f`). + +## 4. Proposal: the ADR-0001 structural cut + +**Status:** Done. +Steps 1 to 4 of the [migration order](#migration-order-and-exit-criteria) are implemented and recorded in `docs/PACKAGE_BOUNDARY_MIGRATION.md`. + +ADR-0001 (accepted 2026-09-28) gives the rule this proposal works within: a consumer declares its own narrow in-process capability interface, an owner owns any versioned process contract under `contracts//v1`, and a composition root wires them. +The ADR's package map row for `tags` sends generic helpers to `foundation/aws/tags` and says each resource owner retains canonical tag state. + +### Scope + +Decided: `domains/tagging` is a stateless aggregator composed in the AWSGW process, not a new daemon. +Decided: any owner wire contract belongs under that owner's `contracts//v1`, and that there is no shared tagging wire format. + +Inside the cut: + +- `domains/tagging` owns the current `GetResources` aggregation path and nothing else. + Today `gateway/tagging` serves only `GetResources` (`spinifex/spinifex/gateway/tagging.go:19`), so this is a structural move with no new API. +- `domains/tagging` declares one read/list capability, enough to reach the EC2 tag index and ELBv2 without importing either owner. +- The AWSGW composition root supplies one adapter per owner that implements that capability. +- `GetResources` behaviour is unchanged, including every behaviour recorded in [Findings](#findings). + +Outside the cut, listed under [Future direction](#future-direction-outside-the-adr-0001-structural-cut): pagination semantics, supported-resource scope, tag limits, `aws:` handling, ever-tagged tracking, tag-policy evaluation (`ResourceTagReader`), and every mutation API (`TagResources`, `UntagResources` and the per-service tag writes). + +### Capability + +`domains/tagging` is the consumer, so it declares the interface: + +```go +package tagging + +// TaggedLister returns one owner's resources for GetResources aggregation. +type TaggedLister interface { + ListTagged(ctx context.Context, accountID string, typeFilters map[string]bool) ([]TaggedResource, error) +} + +type TaggedResource struct { + ARN string + Tags map[string]string +} +``` + +`typeFilters` keeps today's meaning: the lower-cased `ResourceTypeFilters` set, where an entry may be a full `service:type` or a bare service (`gateway/tagging/GetResources.go:206`). +Each adapter applies it to the types it serves, as `listELBv2` and `listEC2` do now. +Tag filtering, ARN sorting and the offset token stay in the aggregator unchanged. +The interface carries no mutation, no limits and no ever-tagged flag; those belong to the future direction. + +### Adapters and compile-time checks + +The owners run in daemons and the aggregator runs in AWSGW. +A daemon-side owner cannot satisfy a Go interface declared in the AWSGW process, so the capability is implemented by an AWSGW-side NATS client adapter. + +- **EC2 index adapter:** wraps today's `listEC2` path, `ec2tags.NewNATSTagsService(...).DescribeTags` plus `buildEC2Mappings` (`gateway/tagging/GetResources.go:95`). + It is constructed with the region, since EC2 IDs become ARNs there. +- **ELBv2 adapter:** wraps today's `elbv2Resources` path, `DescribeLoadBalancers`, `DescribeTargetGroups` and batched `DescribeTags` through `handlers_elbv2.NewNATSELBv2Service` (`gateway/tagging/GetResources.go:106`). +- **Placement:** the adapters live under the AWSGW composition root, `runtime/roles/awsgw`, which the ratchet already names as the role allowed to wire domain registrations (`architecture/layering/rules_test.go:17`) and which already imports `handlers/*` and `domains/*` packages (`runtime/roles/awsgw/awsgw.go:28` to `:46`). +- **Compile-time checks:** `var _ tagging.TaggedLister = (*ec2IndexLister)(nil)` and `var _ tagging.TaggedLister = (*elbv2Lister)(nil)` sit beside the adapters in AWSGW. + No owner package carries a check against a `domains/tagging` interface. +- **Wire contracts:** both adapters reuse subjects and clients that exist today, so the cut defines no new wire contract. + The remote owner of each subject owns its request/reply contract and handler; the EC2 `ec2.DescribeTags` and ELBv2 `elbv2.Describe*` subjects are where that contract would be formalised, under `contracts/ec2/v1` and `contracts/elbv2/v1`, if a later step needs it. +- **Wiring:** `runtime/roles/awsgw` builds the aggregator with both adapters and hands it to the gateway, which today calls `gateway_tagging.GetResources` with its NATS connection (`gateway/tagging.go:21`). + +### Ratchet rows removed + +- `architecture/layering/allowlist_test.go` row `gateway/tagging` → `domains/ec2/tags`. +- `architecture/layering/allowlist_test.go` row `gateway/tagging` → `handlers/elbv2`. +- `architecture/layering/legacy_test.go` entry `gateway/tagging` as a frozen legacy package. +- The `gateway/tagging` row of the `docs/PACKAGE_BOUNDARY_MIGRATION.md` "Close-out status" table and the "One narrow missing boundary" row of its "Remaining legacy roots by blocker class" matrix record `gateway/tagging` as open debt; both rows are updated when it clears. + +`domains/tagging` starts with no allowlist rows and imports only `foundation` packages and the AWS SDK model types it already uses. + +### Migration order and exit criteria + +1. **Interfaces in `domains/tagging`, read/list only.** + Exit: `TaggedLister` and `TaggedResource` exist, the package imports no domain, handler or contract package, and the ratchet classifies it with zero allowlist rows. + Done in `0d830e901`. +2. **EC2 index and ELBv2 adapters in AWSGW.** + Exit: both adapters compile with their `var _ tagging.TaggedLister` checks, each has a test against embedded JetStream equivalent to today's `listEC2` and `elbv2Resources`, and nothing calls them yet. + Done in `f3545e3ea`; type matching is exported once as `tagging.MatchesType`. +3. **Move `GetResources` aggregation from `gateway/tagging` to `domains/tagging`.** + The request parsing, type and tag filtering, sorting, offset token and `authz.go` move; the gateway dispatch in `gateway/tagging.go` calls the aggregator wired by AWSGW. + Exit: the `TestCurrentBehaviour_Tagging_GetResources*` tests and the existing `gateway/tagging` unit tests pass with unchanged assertions after moving with the code. + Done in `aedd0b2c4`: AWSGW sets `GatewayConfig.TaggingResources`, the `GetResources` characterisation tests run in `runtime/roles/awsgw`, and the aggregator unit tests run in `domains/tagging`. +4. **Remove the ratchet rows.** + Exit: the two `gateway/tagging` allowlist rows and its `legacy_test.go` frozen-legacy entry are deleted, the `gateway/tagging` package is gone, `make preflight`'s layering tests pass, and the "Close-out status" `gateway/tagging` row and the "One narrow missing boundary" matrix row record the debt as cleared. + Done: the package and ratchet rows went with `aedd0b2c4`, and the migration record now marks `gateway/tagging` cleared. + +The RDS and ECS record-writing edges in the `docs/PACKAGE_BOUNDARY_MIGRATION.md` "Dependency-rule debt to clear after the moves" list are not cleared by this cut. +They need owner tag mutation capabilities, which are future direction. + +## Future direction (outside the ADR-0001 structural cut) + +Nothing in this section is part of the ADR-0001 track. +It records a possible shape for later work so the cut above does not block it. + +### Owner tag mutation + +A later `domains/tagging` could declare a mutation capability: + +```go +type Tagger interface { + ResourceTypes() []string // "service:type", e.g. "ec2:security-group" + TagResource(ctx context.Context, accountID, arn string, add map[string]string) error + UntagResource(ctx context.Context, accountID, arn string, keys []string) error + ListTags(ctx context.Context, accountID, arn string) (map[string]string, error) +} +``` + +For owners in another process, the same model as the cut applies: the owner defines a request/reply contract under `contracts//v1` and serves it, and an AWSGW-side adapter implements `Tagger` and carries the `var _ tagging.Tagger` check. +An owner in the same process as its consumer can implement the interface directly. + +Errors would be sentinels declared in `domains/tagging` (`ErrNotFound`, `ErrInvalidARN`, `ErrInvalidTag`, `ErrTooManyTags`, `ErrConflict`), wrapping the owner's own `awserrors` code so per-service APIs keep today's codes. + +Guarantees each owner would give: + +1. Per-resource atomic: validate first, then one revision-checked write that changes only the tag field, retried on conflict with the mutation re-applied to the fresh record. +2. Existence and ownership: the ARN's partition, region and account must match the caller, and the resource must exist, before any write. +3. Limits from `foundation/aws/tags` with the owner's documented variations. +4. A tag write never rewrites a non-tag field. + +No owner would be asked for cross-resource atomicity. + +### Work that would use it + +- `TagResources` and `UntagResources` in `domains/tagging`, which are an API expansion, reporting per-ARN failures in `FailedResourcesMap`. +- Clearing the RDS subnet-group and parameter-group edges and the ECS task-definition edge (the `handlers/rds` and `handlers/ecs` legacy edges in the `docs/PACKAGE_BOUNDARY_MIGRATION.md` "Dependency-rule debt to clear after the moves" list) by calling owner mutation capabilities. +- ACM, EKS, ECR and IAM owners; ACM's key-loss defect is already fixed (`mulga-ncn9f`). +- EC2 record owners, once the canonical EC2 tag store is decided (`mulga-36wn6`, `mulga-6maav`). +- `foundation/aws/tags` validation helpers shared by every owner. +- `GetResources` pagination, supported-resource scope and ever-tagged tracking (`mulga-t6cuv`). + +### Tag-policy evaluation + +The policy door would declare its own read-only capability: + +```go +type ResourceTagReader interface { + ResourceTags(ctx context.Context, accountID, arn string) (map[string]string, bool, error) +} +``` + +`aws:RequestTag` and `aws:TagKeys` come from the parsed request and need no owner call. +This depends on `mulga-lmq4l` and `mulga-5eh43`. + +## Open decisions + +These need an owner's decision and are not settled by this document. +None of them blocks the ADR-0001 cut. + +1. **EC2 canonical store.** Whether the `ec2-tags` index becomes the canonical store for every EC2 type, or records stay canonical and the index becomes a projection each owner maintains. +2. **Pagination token format and lifetime.** Keyset token versus snapshot, and whether to mirror AWS's 15-minute expiry. +3. **Cross-account ARN behaviour** for ECS, EKS and ECR, given that AWS behaviour for these is unverified here. +4. **Ever-tagged tracking.** Whether owners must remember that a resource was once tagged, to match `GetResources` returning `"Tags": []`. +5. **Which `TagResources` permission check runs where.** The gateway door, the aggregator, or each owner. +6. **Internal callers and `aws:` keys.** EC2 refuses `aws:` only at AWSGW; whether internal writers (system VPC, EKS controllers) may keep writing reserved keys through a mutation capability. + +Decided by the user and removed from this list: where `domains/tagging` runs (AWSGW, stateless) and the process contract shape (owner contracts only, no shared format). + +## Findings + +Current behaviour only; none is fixed except finding 1, marked resolved. + +| # | Finding | Evidence | Bead | +| --- | --- | --- | --- | +| 1 | Resolved: ACM tag writes erased the stored private key; they now preserve it. Certificates tagged before the fix may have lost their key and need re-import or re-issuance. | Test: `ACMTagWritesPreserveStoredPrivateKey` | `mulga-ncn9f` (P1, closed); remediation `mulga-o436p` (P1, open) | +| 2 | EC2 record mirrors revert concurrent tag and non-tag writes. | Test: `RecordMirrorRevertsInterleavedWrites` | `mulga-36wn6` (P1) | +| 3 | ECS tag writes revert concurrent record writes, including a `DesiredCount` change. | Test: `ECSTagWriteRevertsInterleavedRecordWrite` | `mulga-c0d5x` (P1) | +| 4 | ECS acts on the caller's resource for an ARN naming another account. | Test: `ECSForeignAccountARNTagsCallerResource`; EKS and ECR INFERRED | `mulga-k8d9b` (P2) | +| 5 | EC2 stores tags for unresolved and other-account IDs. | Test: `EC2CreateTagsStoresUnresolvedIDsInCallerNamespace` | `mulga-6maav` | +| 6 | EC2, ELBv2, ECS and ACM enforce no tag count or length limits. | Tests: `EC2CreateTagsAppliesNoLengthOrCountLimits`, `ELBv2AddTagsAppliesNoTagLimits`, `ECSTagResourceAppliesNoTagLimits`, `ACMAddTagsAppliesNoTagLimits`; EKS and ECR code | none | +| 7 | ELBv2 `AddTags` partially applies before an unknown ARN. | Test: `ELBv2AddTagsPartiallyAppliesBeforeUnknownARN` | `mulga-liyz5` (P2) | +| 8 | `GetResources` lists unresolved EC2 IDs, including type `unknown`. | Test: `GetResourcesListsUnresolvedEC2IDs` | `mulga-t6cuv` (P2), `mulga-6maav` | +| 9 | `GetResources` drops resources whose tags were all removed. | Test: `GetResourcesOmitsResourceWhoseTagsWereAllRemoved` | `mulga-t6cuv` (P2) | +| 10 | `GetResources` offset token repeats an entry after an interleaved write. | Test: `GetResourcesOffsetTokenRepeatsAfterInterleavedWrite` | `mulga-t6cuv` (P2) | +| 11 | `GetResources` accepts page sizes over 100 and returns `InvalidParameterValue` for a bad token. | Test: `GetResourcesPaginationValidation` | `mulga-t6cuv` (P2) | +| 12 | `GetResources` covers only EC2 and ELBv2 load balancers and target groups. | Test: `GetResourcesAggregatesOnlyEC2AndELBv2` | `mulga-t6cuv` (P2) | +| 13 | `GetResources` includes untagged ELBv2 load balancers and target groups. | Code | `mulga-t6cuv` (P2) | +| 14 | `TagResources`, `UntagResources`, `GetTagKeys`, `GetTagValues` are not served. | Code | none | +| 15 | Tag condition keys are refused at policy write and never evaluated. | Test: `CreatePolicyRefusesTagConditionKeys` | `mulga-5eh43`, `mulga-lmq4l` | +| 16 | ELBv2 target group tag CAS merges from a stale read; LB, listener and rule tag writes are whole-record puts. | INFERRED | none | +| 17 | ECR, IAM user and IAM OIDC provider tag writes are read then plain put. | INFERRED | none | +| 18 | The EC2 daemon writes the central index before record mirrors, and a mirror failure leaves them divergent. | Code; divergence INFERRED | none | +| 19 | Creation-time tags reach the central index only for instance, vpc/subnet/sg/eni and key pair. | Code; matches `creation-time-tags-describetags.md` | none | +| 20 | `launchtemplate` and `placementgroup` tag writes reach only the central index. | Code | none | + +`mulga-t6cuv` groups the `GetResources` parity gaps; whether it covers findings 8 and 13 is for its owner to confirm. + +## Tests + +Each asserts current behaviour: + +| Test | File | +| --- | --- | +| `TestCurrentBehaviour_Tagging_RecordMirrorRevertsInterleavedWrites` | `spinifex/spinifex/domains/ec2/tagmirror/tagging_current_behaviour_test.go` | +| `TestCurrentBehaviour_Tagging_EC2CreateTagsStoresUnresolvedIDsInCallerNamespace` | `spinifex/spinifex/daemon/tagging_current_behaviour_test.go` | +| `TestCurrentBehaviour_Tagging_EC2CreateTagsAppliesNoLengthOrCountLimits` | `spinifex/spinifex/gateway/tagging_current_behaviour_test.go` | +| `TestCurrentBehaviour_Tagging_GetResourcesListsUnresolvedEC2IDs` | `spinifex/spinifex/runtime/roles/awsgw/tagging_current_behaviour_test.go` | +| `TestCurrentBehaviour_Tagging_GetResourcesOmitsResourceWhoseTagsWereAllRemoved` | same | +| `TestCurrentBehaviour_Tagging_GetResourcesOffsetTokenRepeatsAfterInterleavedWrite` | same | +| `TestCurrentBehaviour_Tagging_GetResourcesPaginationValidation` | same | +| `TestCurrentBehaviour_Tagging_GetResourcesAggregatesOnlyEC2AndELBv2` | same | +| `TestCurrentBehaviour_Tagging_ELBv2AddTagsPartiallyAppliesBeforeUnknownARN` | `spinifex/spinifex/handlers/elbv2/tagging_current_behaviour_test.go` | +| `TestCurrentBehaviour_Tagging_ELBv2AddTagsAppliesNoTagLimits` | same | +| `TestCurrentBehaviour_Tagging_ECSTagWriteRevertsInterleavedRecordWrite` | `spinifex/spinifex/handlers/ecs/tagging_current_behaviour_test.go` | +| `TestCurrentBehaviour_Tagging_ECSForeignAccountARNTagsCallerResource` | same | +| `TestCurrentBehaviour_Tagging_ECSTagResourceAppliesNoTagLimits` | same | +| `TestCurrentBehaviour_Tagging_ACMTagWritesPreserveStoredPrivateKey` | `spinifex/spinifex/domains/acm/tagging_current_behaviour_test.go` | +| `TestCurrentBehaviour_Tagging_ACMAddTagsAppliesNoTagLimits` | same | +| `TestCurrentBehaviour_Tagging_CreatePolicyRefusesTagConditionKeys` | `spinifex/spinifex/handlers/iam/tagging_current_behaviour_test.go` | + +The gateway and AWSGW tests serve the EC2 tag subjects from a real `TagsServiceImpl` without the daemon's record-mirror fan-out, so they cover the central index only. +The interleaving tests wrap the KV handle to run one write between the handler's read and its write; they show the window exists, not how often real nodes hit it. diff --git a/spinifex/daemon/tagging_current_behaviour_test.go b/spinifex/daemon/tagging_current_behaviour_test.go new file mode 100644 index 000000000..298f3a86e --- /dev/null +++ b/spinifex/daemon/tagging_current_behaviour_test.go @@ -0,0 +1,47 @@ +package daemon + +import ( + "context" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The TestCurrentBehaviour_Tagging_ tests pin what the daemon's EC2 tag +// handlers accept today. They describe current behaviour, not the target +// design, and a failure means the behaviour changed. + +// A non-instance ID is never resolved to a resource before the central write. +// Another account's ID, a well-formed ID naming nothing, and an ID with no +// recognised prefix all succeed, and each is stored under the caller's account. +func TestCurrentBehaviour_Tagging_EC2CreateTagsStoresUnresolvedIDsInCallerNamespace(t *testing.T) { + const caller = "999999999999" + d := tagTestDaemon(t, "i-tag-cb-unrelated", nil) + + ids := []string{"sg-0a1b2c3d4e5f60718", "vol-00000000000000000", "not-a-real-id"} + resources := make([]*string, 0, len(ids)) + for _, id := range ids { + resources = append(resources, aws.String(id)) + } + + _, err := d.createTags(context.Background(), &ec2.CreateTagsInput{ + Resources: resources, + Tags: []*ec2.Tag{{Key: aws.String("env"), Value: aws.String("prod")}}, + }, caller) + require.NoError(t, err) + + for _, id := range ids { + assert.Equal(t, map[string]string{"env": "prod"}, centralTags(t, d, caller, id), id) + assert.Empty(t, centralTags(t, d, testAccountID, id), id) + } + + out, err := d.tagsService.DescribeTags(t.Context(), &ec2.DescribeTagsInput{ + Filters: []*ec2.Filter{{Name: aws.String("resource-id"), Values: []*string{aws.String("not-a-real-id")}}}, + }, caller) + require.NoError(t, err) + require.Len(t, out.Tags, 1) + assert.Equal(t, "unknown", aws.StringValue(out.Tags[0].ResourceType)) +} diff --git a/spinifex/domains/acm/tagging_current_behaviour_test.go b/spinifex/domains/acm/tagging_current_behaviour_test.go new file mode 100644 index 000000000..04705f197 --- /dev/null +++ b/spinifex/domains/acm/tagging_current_behaviour_test.go @@ -0,0 +1,86 @@ +package acm + +import ( + "context" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/acm" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The TestCurrentBehaviour_Tagging_ tests pin what the ACM tag writers do to +// the certificate record today. They describe current behaviour, not the +// target design, and a failure means the behaviour changed. + +// The tag writers change only the stored tag field, so the private key and +// certificate body survive a tag or untag unchanged. +func TestCurrentBehaviour_Tagging_ACMTagWritesPreserveStoredPrivateKey(t *testing.T) { + cases := map[string]func(svc *ACMServiceImpl, certArn string) error{ + "AddTagsToCertificate": func(svc *ACMServiceImpl, certArn string) error { + _, err := svc.AddTagsToCertificate(context.Background(), &acm.AddTagsToCertificateInput{ + CertificateArn: aws.String(certArn), + Tags: []*acm.Tag{{Key: aws.String("env"), Value: aws.String("dev")}}, + }, testAccountID) + return err + }, + "RemoveTagsFromCertificate": func(svc *ACMServiceImpl, certArn string) error { + _, err := svc.RemoveTagsFromCertificate(context.Background(), &acm.RemoveTagsFromCertificateInput{ + CertificateArn: aws.String(certArn), + Tags: []*acm.Tag{{Key: aws.String("Name")}}, + }, testAccountID) + return err + }, + } + for name, write := range cases { + t.Run(name, func(t *testing.T) { + svc := setupACMService(t) + certArn := importTagged(t, svc, &acm.Tag{Key: aws.String("Name"), Value: aws.String("ingress")}) + + before, err := svc.store.GetCert(context.Background(), certArn) + require.NoError(t, err) + require.NotEmpty(t, before.PrivateKey, "import must store a usable key") + + require.NoError(t, write(svc, certArn)) + + after, err := svc.store.GetCert(context.Background(), certArn) + require.NoError(t, err) + assert.Equal(t, before.PrivateKey, after.PrivateKey, "a tag write keeps the stored private key") + assert.Equal(t, before.Certificate, after.Certificate, "the certificate body survives") + }) + } +} + +// Neither tag writer bounds the number of tags, the key or value length, or +// refuses the reserved aws: prefix; every tag supplied is stored. +func TestCurrentBehaviour_Tagging_ACMAddTagsAppliesNoTagLimits(t *testing.T) { + svc := setupACMService(t) + certArn := importTagged(t, svc) + + tags := []*acm.Tag{ + {Key: aws.String("aws:reserved"), Value: aws.String("v")}, + {Key: aws.String(longString(129)), Value: aws.String(longString(257))}, + } + for i := range 50 { + tags = append(tags, &acm.Tag{Key: aws.String("k" + longString(i+1)), Value: aws.String("v")}) + } + _, err := svc.AddTagsToCertificate(context.Background(), &acm.AddTagsToCertificateInput{ + CertificateArn: aws.String(certArn), + Tags: tags, + }, testAccountID) + require.NoError(t, err) + + got := listTagMap(t, svc, certArn) + assert.Len(t, got, 52) + assert.Contains(t, got, "aws:reserved") + assert.Equal(t, longString(257), got[longString(129)]) +} + +func longString(n int) string { + b := make([]byte, n) + for i := range b { + b[i] = 'a' + } + return string(b) +} diff --git a/spinifex/domains/ec2/tagmirror/tagging_current_behaviour_test.go b/spinifex/domains/ec2/tagmirror/tagging_current_behaviour_test.go new file mode 100644 index 000000000..2312d9b09 --- /dev/null +++ b/spinifex/domains/ec2/tagmirror/tagging_current_behaviour_test.go @@ -0,0 +1,65 @@ +package tagmirror + +import ( + "context" + "encoding/json" + "sync" + "testing" + + "github.com/mulgadc/spinifex/spinifex/foundation/state/kvutil" + "github.com/nats-io/nats.go/jetstream" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The TestCurrentBehaviour_Tagging_ tests pin how the EC2 record-side tag +// mirror writes resource records today. They describe current behaviour, not +// the target design, and a failure means the behaviour changed. + +// interleavingKV runs between once, after the first read of key returns and +// before the caller can write, standing in for a writer on another node. +type interleavingKV struct { + jetstream.KeyValue + + key string + once sync.Once + between func() +} + +func (k *interleavingKV) Get(ctx context.Context, key string) (jetstream.KeyValueEntry, error) { + entry, err := k.KeyValue.Get(ctx, key) + if key == k.key { + k.once.Do(k.between) + } + return entry, err +} + +// UpdateKVRecordTags reads the record and puts it back without a revision +// check. A second tag write and a non-tag record change that both commit +// between that read and that write are reverted, and every call reports success. +func TestCurrentBehaviour_Tagging_RecordMirrorRevertsInterleavedWrites(t *testing.T) { + const acct, id = "123456789012", "sg-0123456789abcdef0" + key := kvutil.AccountKey(acct, id) + seed, err := json.Marshal(tagMirrorRecord{Name: "original", Tags: map[string]string{}}) + require.NoError(t, err) + kv := seedKV(t, "tagmirror-cb", map[string][]byte{key: seed}) + ctx := context.Background() + + wrapped := &interleavingKV{KeyValue: kv, key: key, between: func() { + require.NoError(t, UpdateKVRecordTags(ctx, kv, acct, id, func(r *tagMirrorRecord) { + r.Name = "renamed" + r.Tags["b"] = "2" + })) + }} + + require.NoError(t, UpdateKVRecordTags(ctx, wrapped, acct, id, func(r *tagMirrorRecord) { + r.Tags["a"] = "1" + })) + + entry, err := kv.Get(ctx, key) + require.NoError(t, err) + var final tagMirrorRecord + require.NoError(t, json.Unmarshal(entry.Value(), &final)) + assert.Equal(t, map[string]string{"a": "1"}, final.Tags, "the interleaved tag b is lost") + assert.Equal(t, "original", final.Name, "the interleaved rename is reverted") +} diff --git a/spinifex/gateway/tagging/authz.go b/spinifex/domains/tagging/authz.go similarity index 98% rename from spinifex/gateway/tagging/authz.go rename to spinifex/domains/tagging/authz.go index 6a3852e9c..e36462900 100644 --- a/spinifex/gateway/tagging/authz.go +++ b/spinifex/domains/tagging/authz.go @@ -1,4 +1,4 @@ -package gateway_tagging +package tagging import ( "errors" diff --git a/spinifex/gateway/tagging/authz_test.go b/spinifex/domains/tagging/authz_test.go similarity index 65% rename from spinifex/gateway/tagging/authz_test.go rename to spinifex/domains/tagging/authz_test.go index 72a021873..8f65eaad1 100644 --- a/spinifex/gateway/tagging/authz_test.go +++ b/spinifex/domains/tagging/authz_test.go @@ -1,10 +1,10 @@ -package gateway_tagging_test +package tagging_test import ( "testing" + "github.com/mulgadc/spinifex/spinifex/domains/tagging" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" - gateway_tagging "github.com/mulgadc/spinifex/spinifex/gateway/tagging" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -13,8 +13,8 @@ import ( // serves is evaluated against "*". Scoping GetResources to the resources it // happens to return would deny calls AWS permits. func TestResourceARNsAreAccountLevel(t *testing.T) { - for _, action := range gateway_tagging.ScopedActions() { - resources, err := gateway_tagging.ResourceARNs(action) + for _, action := range tagging.ScopedActions() { + resources, err := tagging.ResourceARNs(action) require.NoError(t, err, "action %q", action) assert.Equal(t, []string{"*"}, resources, "action %q", action) } @@ -23,13 +23,13 @@ func TestResourceARNsAreAccountLevel(t *testing.T) { // An action absent from the dispatch table cannot reach the resolver, but if one // ever did it fails closed rather than authorizing account-wide. func TestResourceARNsUnknownAction(t *testing.T) { - _, err := gateway_tagging.ResourceARNs("BogusAction") + _, err := tagging.ResourceARNs("BogusAction") require.Error(t, err) assert.Equal(t, awserrors.ErrorInvalidAction, err.Error()) } func TestHasScope(t *testing.T) { - assert.True(t, gateway_tagging.HasScope("GetResources")) - assert.False(t, gateway_tagging.HasScope("TagResources")) - assert.Equal(t, []string{"GetResources"}, gateway_tagging.ScopedActions()) + assert.True(t, tagging.HasScope("GetResources")) + assert.False(t, tagging.HasScope("TagResources")) + assert.Equal(t, []string{"GetResources"}, tagging.ScopedActions()) } diff --git a/spinifex/domains/tagging/getresources.go b/spinifex/domains/tagging/getresources.go new file mode 100644 index 000000000..1da613c98 --- /dev/null +++ b/spinifex/domains/tagging/getresources.go @@ -0,0 +1,161 @@ +package tagging + +import ( + "context" + "errors" + "maps" + "slices" + "sort" + "strconv" + "strings" + + "github.com/aws/aws-sdk-go/aws" + rgt "github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi" + "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" +) + +// defaultResourcesPerPage is used when the caller omits ResourcesPerPage (AWS max is 100). +const defaultResourcesPerPage = 100 + +// Aggregator serves GetResources over the owner listers the composition root +// wires. Listers are called in the order given; it holds no state of its own. +type Aggregator struct { + listers []TaggedLister +} + +// NewAggregator returns an Aggregator that lists from each lister in order. +func NewAggregator(listers ...TaggedLister) *Aggregator { + return &Aggregator{listers: listers} +} + +// GetResources implements the RGT GetResources call, aggregating the account's +// listed resources with type and tag filtering. Pagination uses an opaque +// integer offset into a deterministically ARN-sorted result set. +func (a *Aggregator) GetResources(ctx context.Context, accountID string, body []byte) (any, error) { + var input rgt.GetResourcesInput + if err := unmarshalIfBody(body, &input); err != nil { + return nil, errors.New(awserrors.ErrorInvalidParameterValue) + } + + typeFilters := lowerStringSlice(input.ResourceTypeFilters) + + var mappings []*rgt.ResourceTagMapping + for _, l := range a.listers { + resources, err := l.ListTagged(ctx, accountID, typeFilters) + if err != nil { + return nil, err + } + for _, r := range resources { + mappings = append(mappings, &rgt.ResourceTagMapping{ + ResourceARN: aws.String(r.ARN), + Tags: tagMapToRGT(r.Tags), + }) + } + } + + mappings = filterByTags(mappings, input.TagFilters) + + // Sort by ARN before pagination. + sort.Slice(mappings, func(i, j int) bool { + return aws.StringValue(mappings[i].ResourceARN) < aws.StringValue(mappings[j].ResourceARN) + }) + + page, next, err := paginate(mappings, input.PaginationToken, input.ResourcesPerPage) + if err != nil { + return nil, err + } + + out := &rgt.GetResourcesOutput{ResourceTagMappingList: page} + out.PaginationToken = aws.String(next) // AWS expects empty string, not nil, on last page + return out, nil +} + +// filterByTags keeps resources matching every TagFilter (AND semantics). +// A non-empty Values list within a filter is an OR; an empty list matches any value. +func filterByTags(mappings []*rgt.ResourceTagMapping, filters []*rgt.TagFilter) []*rgt.ResourceTagMapping { + if len(filters) == 0 { + return mappings + } + out := mappings[:0] + for _, m := range mappings { + if matchesAllTagFilters(m.Tags, filters) { + out = append(out, m) + } + } + return out +} + +func matchesAllTagFilters(tags []*rgt.Tag, filters []*rgt.TagFilter) bool { + tagMap := make(map[string]string, len(tags)) + for _, t := range tags { + tagMap[aws.StringValue(t.Key)] = aws.StringValue(t.Value) + } + for _, f := range filters { + key := aws.StringValue(f.Key) + if key == "" { + continue + } + val, ok := tagMap[key] + if !ok { + return false + } + if len(f.Values) == 0 { + continue + } + if !slices.ContainsFunc(f.Values, func(v *string) bool { + return aws.StringValue(v) == val + }) { + return false + } + } + return true +} + +// paginate slices a page from the sorted mappings and returns the page and the +// next token (empty when exhausted). +func paginate(mappings []*rgt.ResourceTagMapping, token *string, perPage *int64) ([]*rgt.ResourceTagMapping, string, error) { + start := 0 + if token != nil && *token != "" { + n, err := strconv.Atoi(*token) + if err != nil || n < 0 { + return nil, "", errors.New(awserrors.ErrorInvalidParameterValue) + } + start = n + } + if start > len(mappings) { + start = len(mappings) + } + + size := defaultResourcesPerPage + if perPage != nil && *perPage > 0 { + size = int(*perPage) + } + + end := start + size + if end >= len(mappings) { + return mappings[start:], "", nil + } + return mappings[start:end], strconv.Itoa(end), nil +} + +func tagMapToRGT(tags map[string]string) []*rgt.Tag { + keys := slices.Sorted(maps.Keys(tags)) + out := make([]*rgt.Tag, 0, len(keys)) + for _, k := range keys { + out = append(out, &rgt.Tag{Key: aws.String(k), Value: aws.String(tags[k])}) + } + return out +} + +func lowerStringSlice(in []*string) map[string]bool { + if len(in) == 0 { + return nil + } + out := make(map[string]bool, len(in)) + for _, s := range in { + if s != nil { + out[strings.ToLower(*s)] = true + } + } + return out +} diff --git a/spinifex/gateway/tagging/GetResources_test.go b/spinifex/domains/tagging/getresources_test.go similarity index 59% rename from spinifex/gateway/tagging/GetResources_test.go rename to spinifex/domains/tagging/getresources_test.go index 2526a1a16..d0d9ed51a 100644 --- a/spinifex/gateway/tagging/GetResources_test.go +++ b/spinifex/domains/tagging/getresources_test.go @@ -1,4 +1,7 @@ -package gateway_tagging +//test:in-package — filterByTags, paginate and tagMapToRGT are unexported +// helpers of the aggregator and are tested directly. + +package tagging import ( "context" @@ -6,29 +9,26 @@ import ( "testing" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/ec2" rgt "github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// fakeLister returns canned mappings so getResources can be exercised without a -// NATS backend. +const testAccountID = "123456789012" + +// fakeLister returns canned resources so GetResources can be exercised without +// a NATS backend. type fakeLister struct { - elbv2 []*rgt.ResourceTagMapping - ec2 []*rgt.ResourceTagMapping - elbv2Err error - ec2Err error + out []TaggedResource + err error gotFilter map[string]bool } -func (f *fakeLister) listELBv2(_ context.Context, typeFilters map[string]bool) ([]*rgt.ResourceTagMapping, error) { - f.gotFilter = typeFilters - return f.elbv2, f.elbv2Err -} +var _ TaggedLister = (*fakeLister)(nil) -func (f *fakeLister) listEC2(_ context.Context, typeFilters map[string]bool) ([]*rgt.ResourceTagMapping, error) { - return f.ec2, f.ec2Err +func (f *fakeLister) ListTagged(_ context.Context, _ string, typeFilters map[string]bool) ([]TaggedResource, error) { + f.gotFilter = typeFilters + return f.out, f.err } func mustJSON(t *testing.T, v any) []byte { @@ -38,28 +38,6 @@ func mustJSON(t *testing.T, v any) []byte { return b } -func TestMatchesType(t *testing.T) { - // Empty filter set admits everything. - assert.True(t, matchesType(nil, "elasticloadbalancing:loadbalancer")) - - exact := map[string]bool{"elasticloadbalancing:loadbalancer": true} - assert.True(t, matchesType(exact, "elasticloadbalancing:loadbalancer")) - assert.False(t, matchesType(exact, "elasticloadbalancing:targetgroup")) - assert.False(t, matchesType(exact, "ec2:subnet")) - - // Service-only filter admits all of that service's types. - svcOnly := map[string]bool{"ec2": true} - assert.True(t, matchesType(svcOnly, "ec2:subnet")) - assert.True(t, matchesType(svcOnly, "ec2:security-group")) - assert.False(t, matchesType(svcOnly, "elasticloadbalancing:loadbalancer")) -} - -func TestEC2ARN(t *testing.T) { - assert.Equal(t, - "arn:aws:ec2:us-east-1:123456789012:subnet/subnet-abc", - ec2ARN("us-east-1", "123456789012", "ec2:subnet", "subnet-abc")) -} - func mapping(arn string, kv ...string) *rgt.ResourceTagMapping { var tags []*rgt.Tag for i := 0; i+1 < len(kv); i += 2 { @@ -136,17 +114,15 @@ func TestPaginate(t *testing.T) { _, _, err = paginate(all, aws.String("not-a-number"), nil) require.Error(t, err) } - func TestGetResources_MergesFiltersSortsPaginates(t *testing.T) { - lister := &fakeLister{ - elbv2: []*rgt.ResourceTagMapping{ - mapping("arn:elb:c", "elbv2.k8s.aws/cluster", "prod"), - mapping("arn:elb:a", "elbv2.k8s.aws/cluster", "dev"), - }, - ec2: []*rgt.ResourceTagMapping{ - mapping("arn:ec2:b", "elbv2.k8s.aws/cluster", "prod"), - }, - } + elbv2 := &fakeLister{out: []TaggedResource{ + {ARN: "arn:elb:c", Tags: map[string]string{"elbv2.k8s.aws/cluster": "prod"}}, + {ARN: "arn:elb:a", Tags: map[string]string{"elbv2.k8s.aws/cluster": "dev"}}, + }} + ec2 := &fakeLister{out: []TaggedResource{ + {ARN: "arn:ec2:b", Tags: map[string]string{"elbv2.k8s.aws/cluster": "prod"}}, + }} + agg := NewAggregator(elbv2, ec2) // Tag-filter to prod + page size 1: two matches (arn:ec2:b, arn:elb:c), // ARN-sorted, first page returns arn:ec2:b with a next token. @@ -154,7 +130,7 @@ func TestGetResources_MergesFiltersSortsPaginates(t *testing.T) { TagFilters: []*rgt.TagFilter{tagFilter("elbv2.k8s.aws/cluster", "prod")}, ResourcesPerPage: aws.Int64(1), }) - out, err := getResources(context.Background(), lister, body) + out, err := agg.GetResources(context.Background(), testAccountID, body) require.NoError(t, err) res, ok := out.(*rgt.GetResourcesOutput) require.True(t, ok) @@ -168,7 +144,7 @@ func TestGetResources_MergesFiltersSortsPaginates(t *testing.T) { ResourcesPerPage: aws.Int64(1), PaginationToken: aws.String("1"), }) - out, err = getResources(context.Background(), lister, body) + out, err = agg.GetResources(context.Background(), testAccountID, body) require.NoError(t, err) res, ok = out.(*rgt.GetResourcesOutput) require.True(t, ok) @@ -182,47 +158,22 @@ func TestGetResources_ResourceTypeFiltersLowercasedAndPassed(t *testing.T) { body := mustJSON(t, rgt.GetResourcesInput{ ResourceTypeFilters: aws.StringSlice([]string{"ElasticLoadBalancing:LoadBalancer"}), }) - _, err := getResources(context.Background(), lister, body) + _, err := NewAggregator(lister).GetResources(context.Background(), testAccountID, body) require.NoError(t, err) assert.True(t, lister.gotFilter["elasticloadbalancing:loadbalancer"]) } func TestGetResources_ListerErrorPropagates(t *testing.T) { - lister := &fakeLister{elbv2Err: assert.AnError} - _, err := getResources(context.Background(), lister, []byte("{}")) + agg := NewAggregator(&fakeLister{err: assert.AnError}, &fakeLister{}) + _, err := agg.GetResources(context.Background(), testAccountID, []byte("{}")) require.ErrorIs(t, err, assert.AnError) } func TestGetResources_InvalidBody(t *testing.T) { - _, err := getResources(context.Background(), &fakeLister{}, []byte("not-json")) + _, err := NewAggregator(&fakeLister{}).GetResources(context.Background(), testAccountID, []byte("not-json")) require.Error(t, err) } -func TestBuildEC2Mappings(t *testing.T) { - tags := []*ec2.TagDescription{ - {ResourceId: aws.String("subnet-1"), ResourceType: aws.String("subnet"), Key: aws.String("Name"), Value: aws.String("a")}, - {ResourceId: aws.String("subnet-1"), ResourceType: aws.String("subnet"), Key: aws.String("env"), Value: aws.String("prod")}, - {ResourceId: aws.String("sg-1"), ResourceType: aws.String("security-group"), Key: aws.String("Name"), Value: aws.String("b")}, - {ResourceId: aws.String(""), ResourceType: aws.String("subnet"), Key: aws.String("skip"), Value: aws.String("me")}, - } - - // No type filter: both resources, tags merged per resource. - all := buildEC2Mappings(tags, "us-east-1", "123456789012", nil) - require.Len(t, all, 2) - bySubnet := map[string]*rgt.ResourceTagMapping{} - for _, m := range all { - bySubnet[aws.StringValue(m.ResourceARN)] = m - } - subnet := bySubnet["arn:aws:ec2:us-east-1:123456789012:subnet/subnet-1"] - require.NotNil(t, subnet) - assert.Len(t, subnet.Tags, 2) - - // Type filter narrows to subnets only. - onlySubnets := buildEC2Mappings(tags, "us-east-1", "123456789012", map[string]bool{"ec2:subnet": true}) - require.Len(t, onlySubnets, 1) - assert.Equal(t, "arn:aws:ec2:us-east-1:123456789012:subnet/subnet-1", aws.StringValue(onlySubnets[0].ResourceARN)) -} - func TestTagMapToRGT_SortedDeterministic(t *testing.T) { out := tagMapToRGT(map[string]string{"b": "2", "a": "1", "c": "3"}) require.Len(t, out, 3) diff --git a/spinifex/domains/tagging/lister.go b/spinifex/domains/tagging/lister.go new file mode 100644 index 000000000..a6aab1b92 --- /dev/null +++ b/spinifex/domains/tagging/lister.go @@ -0,0 +1,20 @@ +// Package tagging owns the read-only Resource Groups Tagging aggregation +// capability, composed into AWSGW. Each resource owner keeps its canonical +// tags; this package reads them only through the TaggedLister it declares. +package tagging + +import "context" + +// TaggedLister returns one owner's resources for GetResources aggregation. +// typeFilters is the lower-cased ResourceTypeFilters set, each entry a full +// "service:type" or a bare service; an empty set admits every type served. +type TaggedLister interface { + ListTagged(ctx context.Context, accountID string, typeFilters map[string]bool) ([]TaggedResource, error) +} + +// TaggedResource is one listed resource; Tags may be empty for an untagged +// resource. Tags is read-only: a lister must not mutate a map it has returned. +type TaggedResource struct { + ARN string + Tags map[string]string +} diff --git a/spinifex/gateway/tagging/handler.go b/spinifex/domains/tagging/response.go similarity index 80% rename from spinifex/gateway/tagging/handler.go rename to spinifex/domains/tagging/response.go index 54684e7bc..4916c5866 100644 --- a/spinifex/gateway/tagging/handler.go +++ b/spinifex/domains/tagging/response.go @@ -1,7 +1,4 @@ -// Package gateway_tagging is the HTTP-side glue between awsgw and the Resource -// Groups Tagging API. Speaks AWS JSON 1.1; GetResources aggregates the elbv2 -// and ec2 tag stores over NATS. Errors are handled by the shared gateway ErrorHandler. -package gateway_tagging +package tagging import ( "encoding/json" diff --git a/spinifex/domains/tagging/typefilter.go b/spinifex/domains/tagging/typefilter.go new file mode 100644 index 000000000..2e02f5c0e --- /dev/null +++ b/spinifex/domains/tagging/typefilter.go @@ -0,0 +1,21 @@ +package tagging + +import "strings" + +// MatchesType reports whether fullType ("service:type") passes typeFilters. +// An empty filter set admits everything; a service-only filter (e.g. "ec2") +// admits all of that service's types. +func MatchesType(typeFilters map[string]bool, fullType string) bool { + if len(typeFilters) == 0 { + return true + } + if typeFilters[fullType] { + return true + } + if svc, _, ok := strings.Cut(fullType, ":"); ok { + if typeFilters[svc] { + return true + } + } + return false +} diff --git a/spinifex/domains/tagging/typefilter_test.go b/spinifex/domains/tagging/typefilter_test.go new file mode 100644 index 000000000..66bb92547 --- /dev/null +++ b/spinifex/domains/tagging/typefilter_test.go @@ -0,0 +1,25 @@ +package tagging_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/mulgadc/spinifex/spinifex/domains/tagging" +) + +func TestMatchesType(t *testing.T) { + // Empty filter set admits everything. + assert.True(t, tagging.MatchesType(nil, "elasticloadbalancing:loadbalancer")) + + exact := map[string]bool{"elasticloadbalancing:loadbalancer": true} + assert.True(t, tagging.MatchesType(exact, "elasticloadbalancing:loadbalancer")) + assert.False(t, tagging.MatchesType(exact, "elasticloadbalancing:targetgroup")) + assert.False(t, tagging.MatchesType(exact, "ec2:subnet")) + + // Service-only filter admits all of that service's types. + svcOnly := map[string]bool{"ec2": true} + assert.True(t, tagging.MatchesType(svcOnly, "ec2:subnet")) + assert.True(t, tagging.MatchesType(svcOnly, "ec2:security-group")) + assert.False(t, tagging.MatchesType(svcOnly, "elasticloadbalancing:loadbalancer")) +} diff --git a/spinifex/gateway/gateway.go b/spinifex/gateway/gateway.go index 56b0e220f..f53f020aa 100644 --- a/spinifex/gateway/gateway.go +++ b/spinifex/gateway/gateway.go @@ -34,6 +34,7 @@ import ( ecrauth "github.com/mulgadc/spinifex/spinifex/domains/ecr/auth" ecrregistry "github.com/mulgadc/spinifex/spinifex/domains/ecr/registry" ochrevector "github.com/mulgadc/spinifex/spinifex/domains/ochre/vector" + "github.com/mulgadc/spinifex/spinifex/domains/tagging" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" "github.com/mulgadc/spinifex/spinifex/foundation/aws/policy" "github.com/mulgadc/spinifex/spinifex/foundation/telemetry" @@ -234,6 +235,11 @@ type GatewayConfig struct { // connection. BedrockAgentVector ochrevector.VectorService + // TaggingResources serves Resource Groups Tagging API GetResources over the + // owner listers AWSGW wires. Nil fails Tagging_Request with ServerInternal, + // the same as an unconfigured gw.NATSConn. + TaggingResources *tagging.Aggregator + // Services holds the signed AWS services dispatched through the // registration seam. Nil means nothing is registered, so every request // falls through to the legacy switch in Request. diff --git a/spinifex/gateway/tagging.go b/spinifex/gateway/tagging.go index 93397d554..5070d2071 100644 --- a/spinifex/gateway/tagging.go +++ b/spinifex/gateway/tagging.go @@ -7,8 +7,8 @@ import ( "net/http" "strings" + "github.com/mulgadc/spinifex/spinifex/domains/tagging" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" - gateway_tagging "github.com/mulgadc/spinifex/spinifex/gateway/tagging" ) // taggingHandler invokes a per-action Resource Groups Tagging API function. @@ -18,7 +18,7 @@ type taggingHandler func(ctx context.Context, gw *GatewayConfig, accountID strin // The action is carried in "X-Amz-Target: ResourceGroupsTaggingAPI_20170126.". var taggingActions = map[string]taggingHandler{ "GetResources": func(ctx context.Context, gw *GatewayConfig, acct string, b []byte) (any, error) { - return gateway_tagging.GetResources(ctx, gw.NATSConn, gw.Region, acct, b) + return gw.TaggingResources.GetResources(ctx, acct, b) }, } @@ -44,7 +44,7 @@ func (gw *GatewayConfig) Tagging_Request(w http.ResponseWriter, r *http.Request) return errors.New(awserrors.ErrorInvalidAction) } - resources, err := gateway_tagging.ResourceARNs(action) + resources, err := tagging.ResourceARNs(action) if err != nil { return err } @@ -66,7 +66,7 @@ func (gw *GatewayConfig) Tagging_Request(w http.ResponseWriter, r *http.Request) return err } - if gw.NATSConn == nil { + if gw.NATSConn == nil || gw.TaggingResources == nil { return errors.New(awserrors.ErrorServerInternal) } @@ -75,6 +75,6 @@ func (gw *GatewayConfig) Tagging_Request(w http.ResponseWriter, r *http.Request) return err } - gateway_tagging.WriteJSONResponse(w, output) + tagging.WriteJSONResponse(w, output) return nil } diff --git a/spinifex/gateway/tagging/GetResources.go b/spinifex/gateway/tagging/GetResources.go deleted file mode 100644 index 474c5fffb..000000000 --- a/spinifex/gateway/tagging/GetResources.go +++ /dev/null @@ -1,317 +0,0 @@ -package gateway_tagging - -import ( - "context" - "errors" - "maps" - "slices" - "sort" - "strconv" - "strings" - - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/ec2" - "github.com/aws/aws-sdk-go/service/elbv2" - rgt "github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi" - ec2tags "github.com/mulgadc/spinifex/spinifex/domains/ec2/tags" - "github.com/mulgadc/spinifex/spinifex/foundation/aws/arn" - "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" - handlers_elbv2 "github.com/mulgadc/spinifex/spinifex/handlers/elbv2" - "github.com/nats-io/nats.go" -) - -// describeTagsBatchSize is the AWS ceiling on ARNs per ELBv2 DescribeTags call. -const describeTagsBatchSize = 20 - -// defaultResourcesPerPage is used when the caller omits ResourcesPerPage (AWS max is 100). -const defaultResourcesPerPage = 100 - -// resourceLister fetches an account's tagged resources shaped as RGT mappings, -// split out so filter/sort/paginate logic can be tested without a live NATS backend. -type resourceLister interface { - listELBv2(ctx context.Context, typeFilters map[string]bool) ([]*rgt.ResourceTagMapping, error) - listEC2(ctx context.Context, typeFilters map[string]bool) ([]*rgt.ResourceTagMapping, error) -} - -// GetResources implements the RGT GetResources call, aggregating tagged ELBv2 -// and EC2 resources for the account with type and tag filtering. Pagination uses -// an opaque integer offset into a deterministically ARN-sorted result set. -func GetResources(ctx context.Context, natsConn *nats.Conn, region, accountID string, body []byte) (any, error) { - return getResources(ctx, &natsLister{natsConn: natsConn, region: region, accountID: accountID}, body) -} - -// getResources is the protocol-independent core: parses the request, collects -// resource families from the lister, then filters, sorts, and paginates. -func getResources(ctx context.Context, lister resourceLister, body []byte) (any, error) { - var input rgt.GetResourcesInput - if err := unmarshalIfBody(body, &input); err != nil { - return nil, errors.New(awserrors.ErrorInvalidParameterValue) - } - - typeFilters := lowerStringSlice(input.ResourceTypeFilters) - - var mappings []*rgt.ResourceTagMapping - - elbMappings, err := lister.listELBv2(ctx, typeFilters) - if err != nil { - return nil, err - } - mappings = append(mappings, elbMappings...) - - ec2Mappings, err := lister.listEC2(ctx, typeFilters) - if err != nil { - return nil, err - } - mappings = append(mappings, ec2Mappings...) - - mappings = filterByTags(mappings, input.TagFilters) - - // Stable sort so pagination tokens are consistent across pages. - sort.Slice(mappings, func(i, j int) bool { - return aws.StringValue(mappings[i].ResourceARN) < aws.StringValue(mappings[j].ResourceARN) - }) - - page, next, err := paginate(mappings, input.PaginationToken, input.ResourcesPerPage) - if err != nil { - return nil, err - } - - out := &rgt.GetResourcesOutput{ResourceTagMappingList: page} - out.PaginationToken = aws.String(next) // AWS expects empty string, not nil, on last page - return out, nil -} - -// natsLister fetches tagged resources from the elbv2 and ec2 tag stores via NATS. -type natsLister struct { - natsConn *nats.Conn - region string - accountID string -} - -func (l *natsLister) listELBv2(ctx context.Context, typeFilters map[string]bool) ([]*rgt.ResourceTagMapping, error) { - return elbv2Resources(ctx, l.natsConn, l.accountID, typeFilters) -} - -func (l *natsLister) listEC2(ctx context.Context, typeFilters map[string]bool) ([]*rgt.ResourceTagMapping, error) { - svc := ec2tags.NewNATSTagsService(l.natsConn) - tagsOut, err := svc.DescribeTags(ctx, &ec2.DescribeTagsInput{}, l.accountID) - if err != nil { - return nil, err - } - return buildEC2Mappings(tagsOut.Tags, l.region, l.accountID, typeFilters), nil -} - -// elbv2Resources lists load balancers and target groups admitted by typeFilters, -// attaching tags via batched DescribeTags. -func elbv2Resources(ctx context.Context, natsConn *nats.Conn, accountID string, typeFilters map[string]bool) ([]*rgt.ResourceTagMapping, error) { - wantLB := matchesType(typeFilters, "elasticloadbalancing:loadbalancer") - wantTG := matchesType(typeFilters, "elasticloadbalancing:targetgroup") - if !wantLB && !wantTG { - return nil, nil - } - - svc := handlers_elbv2.NewNATSELBv2Service(natsConn) - var arns []string - - if wantLB { - lbs, err := svc.DescribeLoadBalancers(ctx, &elbv2.DescribeLoadBalancersInput{}, accountID) - if err != nil { - return nil, err - } - for _, lb := range lbs.LoadBalancers { - if lb.LoadBalancerArn != nil { - arns = append(arns, *lb.LoadBalancerArn) - } - } - } - if wantTG { - tgs, err := svc.DescribeTargetGroups(ctx, &elbv2.DescribeTargetGroupsInput{}, accountID) - if err != nil { - return nil, err - } - for _, tg := range tgs.TargetGroups { - if tg.TargetGroupArn != nil { - arns = append(arns, *tg.TargetGroupArn) - } - } - } - if len(arns) == 0 { - return nil, nil - } - - mappings := make([]*rgt.ResourceTagMapping, 0, len(arns)) - for start := 0; start < len(arns); start += describeTagsBatchSize { - end := min(start+describeTagsBatchSize, len(arns)) - tagsOut, err := svc.DescribeTags(ctx, &elbv2.DescribeTagsInput{ - ResourceArns: aws.StringSlice(arns[start:end]), - }, accountID) - if err != nil { - return nil, err - } - for _, td := range tagsOut.TagDescriptions { - mappings = append(mappings, &rgt.ResourceTagMapping{ - ResourceARN: td.ResourceArn, - Tags: elbv2TagsToRGT(td.Tags), - }) - } - } - return mappings, nil -} - -// buildEC2Mappings groups EC2 tag descriptions by resource, builds an ARN per -// resource, and filters by type. EC2 tags use a bare resource ID and type word -// (e.g. "subnet"); RGT uses "ec2:". Pure function, no NATS dependency. -func buildEC2Mappings(tagDescriptions []*ec2.TagDescription, region, accountID string, typeFilters map[string]bool) []*rgt.ResourceTagMapping { - type resourceTags struct { - fullType string - tags map[string]string - } - byResource := make(map[string]*resourceTags) - for _, td := range tagDescriptions { - id := aws.StringValue(td.ResourceId) - if id == "" { - continue - } - fullType := "ec2:" + aws.StringValue(td.ResourceType) - rt := byResource[id] - if rt == nil { - rt = &resourceTags{fullType: fullType, tags: map[string]string{}} - byResource[id] = rt - } - rt.tags[aws.StringValue(td.Key)] = aws.StringValue(td.Value) - } - - var mappings []*rgt.ResourceTagMapping - for id, rt := range byResource { - if !matchesType(typeFilters, rt.fullType) { - continue - } - mappings = append(mappings, &rgt.ResourceTagMapping{ - ResourceARN: aws.String(ec2ARN(region, accountID, rt.fullType, id)), - Tags: tagMapToRGT(rt.tags), - }) - } - return mappings -} - -// ec2ARN builds an arn:aws:ec2 ARN for a resource. fullType is "ec2:", -// read from the stored tag record rather than from the id. -func ec2ARN(region, accountID, fullType, id string) string { - kind := arn.EC2ResourceType(strings.TrimPrefix(fullType, "ec2:")) - return arn.FormatEC2(kind, region, accountID, id) -} - -// matchesType reports whether fullType passes typeFilters. An empty filter set -// admits everything; a service-only filter (e.g. "ec2") admits all subtypes. -func matchesType(typeFilters map[string]bool, fullType string) bool { - if len(typeFilters) == 0 { - return true - } - if typeFilters[fullType] { - return true - } - if svc, _, ok := strings.Cut(fullType, ":"); ok { - if typeFilters[svc] { - return true - } - } - return false -} - -// filterByTags keeps resources matching every TagFilter (AND semantics). -// A non-empty Values list within a filter is an OR; an empty list matches any value. -func filterByTags(mappings []*rgt.ResourceTagMapping, filters []*rgt.TagFilter) []*rgt.ResourceTagMapping { - if len(filters) == 0 { - return mappings - } - out := mappings[:0] - for _, m := range mappings { - if matchesAllTagFilters(m.Tags, filters) { - out = append(out, m) - } - } - return out -} - -func matchesAllTagFilters(tags []*rgt.Tag, filters []*rgt.TagFilter) bool { - tagMap := make(map[string]string, len(tags)) - for _, t := range tags { - tagMap[aws.StringValue(t.Key)] = aws.StringValue(t.Value) - } - for _, f := range filters { - key := aws.StringValue(f.Key) - if key == "" { - continue - } - val, ok := tagMap[key] - if !ok { - return false - } - if len(f.Values) == 0 { - continue - } - if !slices.ContainsFunc(f.Values, func(v *string) bool { - return aws.StringValue(v) == val - }) { - return false - } - } - return true -} - -// paginate slices a page from the sorted mappings and returns the page and the -// next token (empty when exhausted). -func paginate(mappings []*rgt.ResourceTagMapping, token *string, perPage *int64) ([]*rgt.ResourceTagMapping, string, error) { - start := 0 - if token != nil && *token != "" { - n, err := strconv.Atoi(*token) - if err != nil || n < 0 { - return nil, "", errors.New(awserrors.ErrorInvalidParameterValue) - } - start = n - } - if start > len(mappings) { - start = len(mappings) - } - - size := defaultResourcesPerPage - if perPage != nil && *perPage > 0 { - size = int(*perPage) - } - - end := start + size - if end >= len(mappings) { - return mappings[start:], "", nil - } - return mappings[start:end], strconv.Itoa(end), nil -} - -func elbv2TagsToRGT(tags []*elbv2.Tag) []*rgt.Tag { - out := make([]*rgt.Tag, 0, len(tags)) - for _, t := range tags { - out = append(out, &rgt.Tag{Key: t.Key, Value: t.Value}) - } - return out -} - -func tagMapToRGT(tags map[string]string) []*rgt.Tag { - keys := slices.Sorted(maps.Keys(tags)) - out := make([]*rgt.Tag, 0, len(keys)) - for _, k := range keys { - out = append(out, &rgt.Tag{Key: aws.String(k), Value: aws.String(tags[k])}) - } - return out -} - -func lowerStringSlice(in []*string) map[string]bool { - if len(in) == 0 { - return nil - } - out := make(map[string]bool, len(in)) - for _, s := range in { - if s != nil { - out[strings.ToLower(*s)] = true - } - } - return out -} diff --git a/spinifex/gateway/tagging_authz_completeness_test.go b/spinifex/gateway/tagging_authz_completeness_test.go index 65ad56ea4..5e5f1ce98 100644 --- a/spinifex/gateway/tagging_authz_completeness_test.go +++ b/spinifex/gateway/tagging_authz_completeness_test.go @@ -6,7 +6,7 @@ package gateway import ( "testing" - gateway_tagging "github.com/mulgadc/spinifex/spinifex/gateway/tagging" + "github.com/mulgadc/spinifex/spinifex/domains/tagging" "github.com/stretchr/testify/assert" ) @@ -15,13 +15,13 @@ import ( // left behind by a deleted or renamed action fails too. func TestTaggingScopeTableIsExhaustive(t *testing.T) { for action := range taggingActions { - assert.True(t, gateway_tagging.HasScope(action), - "tagging action %q has no resource scope entry: add one to taggingScopes in gateway/tagging/authz.go", action) + assert.True(t, tagging.HasScope(action), + "tagging action %q has no resource scope entry: add one to taggingScopes in domains/tagging/authz.go", action) } - for _, action := range gateway_tagging.ScopedActions() { + for _, action := range tagging.ScopedActions() { _, ok := taggingActions[action] assert.True(t, ok, - "taggingScopes has an entry for %q, which the dispatch table does not serve: remove it from gateway/tagging/authz.go", action) + "taggingScopes has an entry for %q, which the dispatch table does not serve: remove it from domains/tagging/authz.go", action) } } diff --git a/spinifex/gateway/tagging_authz_test.go b/spinifex/gateway/tagging_authz_test.go index ecf3a879f..7e7b30c79 100644 --- a/spinifex/gateway/tagging_authz_test.go +++ b/spinifex/gateway/tagging_authz_test.go @@ -9,8 +9,8 @@ import ( "testing" "github.com/mulgadc/bluebottle/pkg/sigv4" + "github.com/mulgadc/spinifex/spinifex/domains/tagging" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" - gateway_tagging "github.com/mulgadc/spinifex/spinifex/gateway/tagging" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -56,7 +56,7 @@ func TestTaggingRequest_ScopedDenyDoesNotFire(t *testing.T) { func TestTaggingRequest_WildcardPolicyStillPermitsEveryAction(t *testing.T) { gw := scopedPolicyGateway(statement("Allow", "tagging:*", "*")) - for _, action := range gateway_tagging.ScopedActions() { + for _, action := range tagging.ScopedActions() { t.Run(action, func(t *testing.T) { assertPermitted(t, dispatchTagging(t, gw, action, `{}`)) }) diff --git a/spinifex/gateway/tagging_current_behaviour_test.go b/spinifex/gateway/tagging_current_behaviour_test.go new file mode 100644 index 000000000..7e5ea44e7 --- /dev/null +++ b/spinifex/gateway/tagging_current_behaviour_test.go @@ -0,0 +1,104 @@ +//test:in-package — drives EC2_Request through the gateway's unexported test +// helper setupEC2Request. + +package gateway + +import ( + "context" + "fmt" + "net/http" + "net/url" + "strings" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/nats-io/nats.go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mulgadc/spinifex/internal/testkit" + "github.com/mulgadc/spinifex/spinifex/bootstrap/config" + ec2tags "github.com/mulgadc/spinifex/spinifex/domains/ec2/tags" + awserrors "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" + natsmsg "github.com/mulgadc/spinifex/spinifex/foundation/messaging/nats" +) + +// The TestCurrentBehaviour_Tagging_ tests here pin today's EC2 tag-write +// outcomes at the AWSGW door. They drive the real router over the real EC2 tag +// service on an embedded JetStream, and describe current behaviour, not the +// target design. The daemon's record-mirror fan-out is not wired here, so only +// the central tag index is exercised. + +// newTaggingCBGateway serves ec2.CreateTags from a real TagsServiceImpl +// over a fresh JetStream bucket, with no legacy object store. +func newTaggingCBGateway(t *testing.T) (*GatewayConfig, *ec2tags.TagsServiceImpl) { + t.Helper() + _, nc, js := testutil.StartTestJetStream(t) + kv, err := ec2tags.GetOrCreateTagsBucket(t.Context(), js) + require.NoError(t, err) + svc := ec2tags.NewTagsServiceImplWithStore(&config.Config{}, nil, kv) + + serve := func(subject string, fn func(*nats.Msg)) { + sub, err := nc.Subscribe(subject, fn) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) + } + serve("ec2.CreateTags", func(msg *nats.Msg) { + natsmsg.ServeNATSRequestCtx(msg, func(ctx context.Context, in *ec2.CreateTagsInput) (*ec2.CreateTagsOutput, error) { + return svc.CreateTags(ctx, in, natsmsg.AccountIDFromMsg(msg)) + }) + }) + + gw := &GatewayConfig{DisableLogging: true, Region: authzRegion, IAMService: allowAllIAMService(), NATSConn: nc} + return gw, svc +} + +// ec2TagQuery builds a CreateTags or DeleteTags query body. +func ec2TagQuery(action string, resources []string, tags map[string]string) string { + q := url.Values{"Action": {action}} + for i, r := range resources { + q.Set(fmt.Sprintf("ResourceId.%d", i+1), r) + } + i := 0 + for k, v := range tags { + i++ + q.Set(fmt.Sprintf("Tag.%d.Key", i), k) + q.Set(fmt.Sprintf("Tag.%d.Value", i), v) + } + return q.Encode() +} + +func requireEC2OK(t *testing.T, gw *GatewayConfig, body string) { + t.Helper() + w := serveEC2(gw, body) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) +} + +func centralTagCount(t *testing.T, svc *ec2tags.TagsServiceImpl, resourceID string) int { + t.Helper() + out, err := svc.DescribeTags(t.Context(), &ec2.DescribeTagsInput{ + Filters: []*ec2.Filter{{Name: aws.String("resource-id"), Values: []*string{aws.String(resourceID)}}}, + }, authzAccountID) + require.NoError(t, err) + return len(out.Tags) +} + +// CreateTags refuses an aws: key but applies no tag-count, key-length or +// value-length limit: 51 tags, a 129-character key and a 257-character value +// are all stored. +func TestCurrentBehaviour_Tagging_EC2CreateTagsAppliesNoLengthOrCountLimits(t *testing.T) { + gw, svc := newTaggingCBGateway(t) + const sg = "sg-0a1b2c3d4e5f60718" + + tags := map[string]string{strings.Repeat("k", 129): strings.Repeat("v", 257)} + for i := range 50 { + tags[fmt.Sprintf("key-%02d", i)] = "v" + } + requireEC2OK(t, gw, ec2TagQuery("CreateTags", []string{sg}, tags)) + assert.Equal(t, 51, centralTagCount(t, svc, sg)) + + w := serveEC2(gw, ec2TagQuery("CreateTags", []string{sg}, map[string]string{"aws:reserved": "v"})) + assert.Equal(t, http.StatusBadRequest, w.Code) + assert.Contains(t, w.Body.String(), ""+awserrors.ErrorInvalidParameterValue+"") +} diff --git a/spinifex/gateway/tagging_test.go b/spinifex/gateway/tagging_test.go index ee21fd8cd..041ed77fd 100644 --- a/spinifex/gateway/tagging_test.go +++ b/spinifex/gateway/tagging_test.go @@ -7,6 +7,8 @@ import ( "strings" "testing" + "github.com/mulgadc/spinifex/internal/testkit" + "github.com/mulgadc/spinifex/spinifex/domains/tagging" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -64,3 +66,27 @@ func TestTaggingRequest_KnownActionNoNATS(t *testing.T) { require.Error(t, err) assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) } + +// A connected gateway with no GetResources aggregator wired fails the same way +// as one with no NATS connection. +func TestTaggingRequest_KnownActionNoAggregator(t *testing.T) { + _, nc, _ := testutil.StartTestJetStream(t) + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService(), NATSConn: nc} + w := httptest.NewRecorder() + err := gw.Tagging_Request(w, setupTaggingRequest("ResourceGroupsTaggingAPI_20170126.GetResources", "{}")) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) +} + +// A wired aggregator is what GetResources dispatches to, and its output is +// written as an AWS JSON 1.1 response. +func TestTaggingRequest_WiredAggregatorServesGetResources(t *testing.T) { + _, nc, _ := testutil.StartTestJetStream(t) + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService(), NATSConn: nc, TaggingResources: tagging.NewAggregator()} + w := httptest.NewRecorder() + err := gw.Tagging_Request(w, setupTaggingRequest("ResourceGroupsTaggingAPI_20170126.GetResources", "{}")) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, w.Code) + assert.Equal(t, tagging.JSONContentType, w.Header().Get("Content-Type")) + assert.JSONEq(t, `{"PaginationToken":""}`, w.Body.String()) +} diff --git a/spinifex/handlers/ecs/tagging_current_behaviour_test.go b/spinifex/handlers/ecs/tagging_current_behaviour_test.go new file mode 100644 index 000000000..7471598b5 --- /dev/null +++ b/spinifex/handlers/ecs/tagging_current_behaviour_test.go @@ -0,0 +1,131 @@ +package handlers_ecs + +import ( + "context" + "strings" + "sync" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ecs" + "github.com/nats-io/nats.go/jetstream" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The TestCurrentBehaviour_Tagging_ tests pin how the ECS tag actions read and +// write resource records today. They describe current behaviour, not the +// target design, and a failure means the behaviour changed. + +// interleavingKV runs between once, after the first read of key returns and +// before the caller can write, standing in for a concurrent writer. +type interleavingKV struct { + jetstream.KeyValue + + key string + once sync.Once + between func() +} + +func (k *interleavingKV) Get(ctx context.Context, key string) (jetstream.KeyValueEntry, error) { + entry, err := k.KeyValue.Get(ctx, key) + if key == k.key { + k.once.Do(k.between) + } + return entry, err +} + +// A tag write re-puts the whole record it read without a revision check, so a +// record change committed between its read and its write is reverted. +func TestCurrentBehaviour_Tagging_ECSTagWriteRevertsInterleavedRecordWrite(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + + _, err := svc.CreateCluster(ctx, &ecs.CreateClusterInput{ClusterName: aws.String("web")}, testAccountID) + require.NoError(t, err) + registerTaskDef(t, svc, "nginx", 128, 256) + _, err = svc.CreateService(ctx, &ecs.CreateServiceInput{ + Cluster: aws.String("web"), + ServiceName: aws.String("api"), + TaskDefinition: aws.String("nginx"), + DesiredCount: aws.Int64(0), + }, testAccountID) + require.NoError(t, err) + + kv, err := svc.bucket(ctx, testAccountID) + require.NoError(t, err) + key := ServiceKey("web", "api") + wrapped := &interleavingKV{KeyValue: kv, key: key, between: func() { + var rec ServiceRecord + found, gerr := getJSON(ctx, kv, key, &rec) + require.NoError(t, gerr) + require.True(t, found) + rec.DesiredCount = 5 + require.NoError(t, putJSON(ctx, kv, key, &rec)) + }} + + err = svc.mutateResourceTags(ctx, wrapped, ecsResourceService, "web", "api", func(existing map[string]string) map[string]string { + if existing == nil { + existing = map[string]string{} + } + existing["team"] = "infra" + return existing + }) + require.NoError(t, err) + + var final ServiceRecord + found, err := getJSON(ctx, kv, key, &final) + require.NoError(t, err) + require.True(t, found) + assert.Equal(t, 0, final.DesiredCount, "the interleaved DesiredCount=5 write is lost") + assert.Equal(t, map[string]string{"team": "infra"}, final.Tags) +} + +// The ARN's account and region are not compared with the caller's: the +// resource segment is resolved in the caller's own bucket, so a foreign ARN +// tags the caller's same-named cluster. +func TestCurrentBehaviour_Tagging_ECSForeignAccountARNTagsCallerResource(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + + _, err := svc.CreateCluster(ctx, &ecs.CreateClusterInput{ClusterName: aws.String("web")}, testAccountID) + require.NoError(t, err) + + foreign := "arn:aws:ecs:eu-west-9:999999999999:cluster/web" + _, err = svc.TagResource(ctx, &ecs.TagResourceInput{ + ResourceArn: aws.String(foreign), + Tags: []*ecs.Tag{{Key: aws.String("via"), Value: aws.String("foreign-arn")}}, + }, testAccountID) + require.NoError(t, err) + + own := "arn:aws:ecs:" + testRegion + ":" + testAccountID + ":cluster/web" + listed, err := svc.ListTagsForResource(ctx, &ecs.ListTagsForResourceInput{ResourceArn: aws.String(own)}, testAccountID) + require.NoError(t, err) + require.Len(t, listed.Tags, 1) + assert.Equal(t, "foreign-arn", aws.StringValue(listed.Tags[0].Value)) +} + +// TagResource applies no tag-count, key-length, value-length or aws: prefix +// rule: all 52 tags below are stored. +func TestCurrentBehaviour_Tagging_ECSTagResourceAppliesNoTagLimits(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + + created, err := svc.CreateCluster(ctx, &ecs.CreateClusterInput{ClusterName: aws.String("web")}, testAccountID) + require.NoError(t, err) + arn := aws.StringValue(created.Cluster.ClusterArn) + + tags := []*ecs.Tag{ + {Key: aws.String("aws:reserved"), Value: aws.String("v")}, + {Key: aws.String(strings.Repeat("k", 129)), Value: aws.String(strings.Repeat("v", 257))}, + } + for i := range 50 { + tags = append(tags, &ecs.Tag{Key: aws.String("k" + strings.Repeat("x", i+1)), Value: aws.String("v")}) + } + _, err = svc.TagResource(ctx, &ecs.TagResourceInput{ResourceArn: aws.String(arn), Tags: tags}, testAccountID) + require.NoError(t, err) + + listed, err := svc.ListTagsForResource(ctx, &ecs.ListTagsForResourceInput{ResourceArn: aws.String(arn)}, testAccountID) + require.NoError(t, err) + assert.Len(t, listed.Tags, 52) +} diff --git a/spinifex/handlers/elbv2/tagging_current_behaviour_test.go b/spinifex/handlers/elbv2/tagging_current_behaviour_test.go new file mode 100644 index 000000000..5d74845c1 --- /dev/null +++ b/spinifex/handlers/elbv2/tagging_current_behaviour_test.go @@ -0,0 +1,61 @@ +package handlers_elbv2 + +import ( + "context" + "fmt" + "strings" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/elbv2" + "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The TestCurrentBehaviour_Tagging_ tests pin how ELBv2 AddTags applies a +// multi-resource request today. They describe current behaviour, not the +// target design, and a failure means the behaviour changed. + +// AddTags walks ResourceArns in order and writes each before resolving the +// next, so an unknown ARN after a valid one fails the call with the earlier +// resource already tagged. +func TestCurrentBehaviour_Tagging_ELBv2AddTagsPartiallyAppliesBeforeUnknownARN(t *testing.T) { + svc := setupTestService(t) + lbOut, err := svc.CreateLoadBalancer(context.Background(), &elbv2.CreateLoadBalancerInput{Name: aws.String("cb-partial-lb")}, testAccountID) + require.NoError(t, err) + arn := *lbOut.LoadBalancers[0].LoadBalancerArn + missing := "arn:aws:elasticloadbalancing:us-east-1:" + testAccountID + ":targetgroup/missing/0123456789abcdef" + + _, err = svc.AddTags(context.Background(), &elbv2.AddTagsInput{ + ResourceArns: []*string{aws.String(arn), aws.String(missing)}, + Tags: []*elbv2.Tag{{Key: aws.String("env"), Value: aws.String("prod")}}, + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorELBv2TargetGroupNotFound, err.Error()) + + assert.Equal(t, map[string]string{"env": "prod"}, tagsFor(t, svc, arn), "the first resource keeps the tag") +} + +// AddTags refuses only an empty key: 51 tags, a 129-character key, a +// 257-character value and an aws: key are all stored. +func TestCurrentBehaviour_Tagging_ELBv2AddTagsAppliesNoTagLimits(t *testing.T) { + svc := setupTestService(t) + lbOut, err := svc.CreateLoadBalancer(context.Background(), &elbv2.CreateLoadBalancerInput{Name: aws.String("cb-limits-lb")}, testAccountID) + require.NoError(t, err) + arn := *lbOut.LoadBalancers[0].LoadBalancerArn + + tags := []*elbv2.Tag{ + {Key: aws.String("aws:reserved"), Value: aws.String("v")}, + {Key: aws.String(strings.Repeat("k", 129)), Value: aws.String(strings.Repeat("v", 257))}, + } + for i := range 49 { + tags = append(tags, &elbv2.Tag{Key: aws.String(fmt.Sprintf("key-%02d", i)), Value: aws.String("v")}) + } + _, err = svc.AddTags(context.Background(), &elbv2.AddTagsInput{ResourceArns: []*string{aws.String(arn)}, Tags: tags}, testAccountID) + require.NoError(t, err) + + got := tagsFor(t, svc, arn) + assert.Len(t, got, 51) + assert.Contains(t, got, "aws:reserved") +} diff --git a/spinifex/handlers/iam/tagging_current_behaviour_test.go b/spinifex/handlers/iam/tagging_current_behaviour_test.go new file mode 100644 index 000000000..b39544e82 --- /dev/null +++ b/spinifex/handlers/iam/tagging_current_behaviour_test.go @@ -0,0 +1,41 @@ +package handlers_iam + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/iam" + "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The TestCurrentBehaviour_Tagging_ tests pin how IAM treats tag-based +// authorization today. They describe current behaviour, not the target design, +// and a failure means the behaviour changed. + +// A customer-managed policy conditioned on any tag key family is refused at +// write time, so no stored policy can carry a tag condition the evaluator +// would then have to enforce. +func TestCurrentBehaviour_Tagging_CreatePolicyRefusesTagConditionKeys(t *testing.T) { + svc := setupTestIAMService(t) + + conditions := map[string]struct{ key, condition string }{ + "ResourceTag": {"aws:ResourceTag/team", `{"StringEquals":{"aws:ResourceTag/team":"infra"}}`}, + "RequestTag": {"aws:RequestTag/team", `{"StringEquals":{"aws:RequestTag/team":"infra"}}`}, + "TagKeys": {"aws:TagKeys", `{"ForAllValues:StringEquals":{"aws:TagKeys":["team"]}}`}, + "EC2Resource": {"ec2:ResourceTag/team", `{"StringEquals":{"ec2:ResourceTag/team":"infra"}}`}, + } + for name, tc := range conditions { + t.Run(name, func(t *testing.T) { + doc := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"ec2:CreateTags","Resource":"*","Condition":` + tc.condition + `}]}` + _, err := svc.CreatePolicy(testAccountID, &iam.CreatePolicyInput{ + PolicyName: aws.String("cb-tag-" + name), + PolicyDocument: aws.String(doc), + }) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorIAMMalformedPolicyDocument) + assert.Contains(t, err.Error(), `key "`+tc.key+`" is not supported`) + }) + } +} diff --git a/spinifex/runtime/roles/awsgw/awsgw.go b/spinifex/runtime/roles/awsgw/awsgw.go index 0b4cd04b2..af18c4733 100644 --- a/spinifex/runtime/roles/awsgw/awsgw.go +++ b/spinifex/runtime/roles/awsgw/awsgw.go @@ -515,6 +515,7 @@ func launchService(config *config.ClusterConfig) error { BedrockAgentKB: bedrockAgentKB, BedrockAgentDataSources: bedrockAgentDataSources, BedrockAgentVector: bedrockAgentVector, + TaggingResources: newTaggingAggregator(natsConn, nodeConfig.Region), } ecrControlPlane := awsapi.NewRegistration(awsapi.Deps{ diff --git a/spinifex/runtime/roles/awsgw/tagging_current_behaviour_test.go b/spinifex/runtime/roles/awsgw/tagging_current_behaviour_test.go new file mode 100644 index 000000000..85ee4e1d1 --- /dev/null +++ b/spinifex/runtime/roles/awsgw/tagging_current_behaviour_test.go @@ -0,0 +1,208 @@ +//test:in-package — drives GetResources through newTaggingAggregator, the +// composition root's own wiring, which is unexported. + +package awsgw + +import ( + "context" + "encoding/json" + "net/http/httptest" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/nats-io/nats.go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + testutil "github.com/mulgadc/spinifex/internal/testkit" + "github.com/mulgadc/spinifex/spinifex/bootstrap/config" + ec2tags "github.com/mulgadc/spinifex/spinifex/domains/ec2/tags" + "github.com/mulgadc/spinifex/spinifex/domains/tagging" + awserrors "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" + natsmsg "github.com/mulgadc/spinifex/spinifex/foundation/messaging/nats" +) + +// The TestCurrentBehaviour_Tagging_GetResources tests pin today's Resource +// Groups Tagging API outcomes through the aggregator AWSGW wires. They run the +// real EC2 tag service on an embedded JetStream, and describe current +// behaviour, not the target design. The daemon's record-mirror fan-out is not +// wired here, so only the central tag index is exercised. + +const ( + authzAccountID = "123456789012" + authzRegion = "ap-southeast-2" +) + +// newTaggingCBAggregator serves the EC2 tag subjects from a real +// TagsServiceImpl over a fresh JetStream bucket, and returns AWSGW's +// GetResources wiring with the NATS client that writes tags as the gateway does. +func newTaggingCBAggregator(t *testing.T) (*tagging.Aggregator, ec2tags.TagsService) { + t.Helper() + _, nc, js := testutil.StartTestJetStream(t) + kv, err := ec2tags.GetOrCreateTagsBucket(t.Context(), js) + require.NoError(t, err) + svc := ec2tags.NewTagsServiceImplWithStore(&config.Config{}, nil, kv) + + serve := func(subject string, fn func(*nats.Msg)) { + sub, err := nc.Subscribe(subject, fn) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) + } + serve("ec2.CreateTags", func(msg *nats.Msg) { + natsmsg.ServeNATSRequestCtx(msg, func(ctx context.Context, in *ec2.CreateTagsInput) (*ec2.CreateTagsOutput, error) { + return svc.CreateTags(ctx, in, natsmsg.AccountIDFromMsg(msg)) + }) + }) + serve("ec2.DeleteTags", func(msg *nats.Msg) { + natsmsg.ServeNATSRequestCtx(msg, func(ctx context.Context, in *ec2.DeleteTagsInput) (*ec2.DeleteTagsOutput, error) { + return svc.DeleteTags(ctx, in, natsmsg.AccountIDFromMsg(msg)) + }) + }) + serve("ec2.DescribeTags", func(msg *nats.Msg) { + natsmsg.ServeNATSRequestCtx(msg, func(ctx context.Context, in *ec2.DescribeTagsInput) (*ec2.DescribeTagsOutput, error) { + return svc.DescribeTags(ctx, in, natsmsg.AccountIDFromMsg(msg)) + }) + }) + + return newTaggingAggregator(nc, authzRegion), ec2tags.NewNATSTagsService(nc) +} + +// createTags writes tags over the ec2.CreateTags subject the gateway's EC2 +// CreateTags dispatch uses. +func createTags(t *testing.T, tags ec2tags.TagsService, resources []string, kv map[string]string) { + t.Helper() + in := &ec2.CreateTagsInput{Resources: aws.StringSlice(resources)} + for k, v := range kv { + in.Tags = append(in.Tags, &ec2.Tag{Key: aws.String(k), Value: aws.String(v)}) + } + _, err := tags.CreateTags(t.Context(), in, authzAccountID) + require.NoError(t, err) +} + +// deleteAllTags sends DeleteTags with no Tag list, which clears every tag. +func deleteAllTags(t *testing.T, tags ec2tags.TagsService, resource string) { + t.Helper() + _, err := tags.DeleteTags(t.Context(), &ec2.DeleteTagsInput{Resources: aws.StringSlice([]string{resource})}, authzAccountID) + require.NoError(t, err) +} + +type getResourcesPage struct { + ResourceTagMappingList []struct { + ResourceARN string `json:"ResourceARN"` + Tags []struct { + Key string `json:"Key"` + Value string `json:"Value"` + } `json:"Tags"` + } `json:"ResourceTagMappingList"` + PaginationToken string `json:"PaginationToken"` +} + +// getResources runs GetResources for the test account and decodes the body the +// gateway would write for it. +func getResources(t *testing.T, agg *tagging.Aggregator, body string) (getResourcesPage, error) { + t.Helper() + out, err := agg.GetResources(t.Context(), authzAccountID, []byte(body)) + if err != nil { + return getResourcesPage{}, err + } + w := httptest.NewRecorder() + tagging.WriteJSONResponse(w, out) + var page getResourcesPage + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &page)) + return page, nil +} + +func pageARNs(p getResourcesPage) []string { + out := make([]string, 0, len(p.ResourceTagMappingList)) + for _, m := range p.ResourceTagMappingList { + out = append(out, m.ResourceARN) + } + return out +} + +// GetResources lists whatever the central index holds: an ID with no +// recognised prefix is reported with resource type "unknown", and a +// well-formed ID naming no resource is reported as that resource. +func TestCurrentBehaviour_Tagging_GetResourcesListsUnresolvedEC2IDs(t *testing.T) { + agg, tags := newTaggingCBAggregator(t) + + createTags(t, tags, []string{"not-a-real-id", "vol-00000000000000000"}, map[string]string{"env": "prod"}) + + page, err := getResources(t, agg, `{"ResourceTypeFilters":["ec2"]}`) + require.NoError(t, err) + prefix := "arn:aws:ec2:" + authzRegion + ":" + authzAccountID + ":" + assert.ElementsMatch(t, []string{ + prefix + "unknown/not-a-real-id", + prefix + "volume/vol-00000000000000000", + }, pageARNs(page)) +} + +// DeleteTags with no Tag list clears every tag, and the resource then drops +// out of GetResources even though it was tagged before. +func TestCurrentBehaviour_Tagging_GetResourcesOmitsResourceWhoseTagsWereAllRemoved(t *testing.T) { + agg, tags := newTaggingCBAggregator(t) + const sg = "sg-0a1b2c3d4e5f60718" + + createTags(t, tags, []string{sg}, map[string]string{"env": "prod"}) + page, err := getResources(t, agg, `{"ResourceTypeFilters":["ec2"]}`) + require.NoError(t, err) + require.Len(t, page.ResourceTagMappingList, 1) + + deleteAllTags(t, tags, sg) + page, err = getResources(t, agg, `{"ResourceTypeFilters":["ec2"]}`) + require.NoError(t, err) + assert.Empty(t, page.ResourceTagMappingList) +} + +// The pagination token is an offset into the result set recomputed on every +// call. A resource tagged between two page reads that sorts ahead of the +// offset shifts the set, so the second page repeats the first page's resource. +func TestCurrentBehaviour_Tagging_GetResourcesOffsetTokenRepeatsAfterInterleavedWrite(t *testing.T) { + agg, tags := newTaggingCBAggregator(t) + + createTags(t, tags, []string{"sg-0000000000000000b", "sg-0000000000000000c"}, map[string]string{"env": "prod"}) + + first, err := getResources(t, agg, `{"ResourceTypeFilters":["ec2"],"ResourcesPerPage":1}`) + require.NoError(t, err) + require.Len(t, first.ResourceTagMappingList, 1) + require.Equal(t, "1", first.PaginationToken) + + createTags(t, tags, []string{"sg-0000000000000000a"}, map[string]string{"env": "prod"}) + + second, err := getResources(t, agg, `{"ResourceTypeFilters":["ec2"],"ResourcesPerPage":1,"PaginationToken":"1"}`) + require.NoError(t, err) + require.Len(t, second.ResourceTagMappingList, 1) + assert.Equal(t, pageARNs(first), pageARNs(second), "the second page repeats the first") +} + +// A token that is not a non-negative integer is InvalidParameterValue, and a +// page size above 100 is accepted rather than refused. +func TestCurrentBehaviour_Tagging_GetResourcesPaginationValidation(t *testing.T) { + agg, tags := newTaggingCBAggregator(t) + createTags(t, tags, []string{"sg-0000000000000000a"}, map[string]string{"env": "prod"}) + + _, err := getResources(t, agg, `{"PaginationToken":"not-a-token","ResourceTypeFilters":["ec2"]}`) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error()) + + page, err := getResources(t, agg, `{"ResourcesPerPage":500,"ResourceTypeFilters":["ec2"]}`) + require.NoError(t, err) + assert.Len(t, page.ResourceTagMappingList, 1) +} + +// GetResources aggregates only the EC2 central index and ELBv2 load balancers +// and target groups. A type filter naming any other service returns an empty +// list without error, whatever that service holds. +func TestCurrentBehaviour_Tagging_GetResourcesAggregatesOnlyEC2AndELBv2(t *testing.T) { + agg, tags := newTaggingCBAggregator(t) + createTags(t, tags, []string{"sg-0000000000000000a"}, map[string]string{"env": "prod"}) + + for _, filter := range []string{"rds", "ecs:cluster", "eks:cluster", "ecr:repository", "acm:certificate", "iam:role", "elasticloadbalancing:listener"} { + t.Run(filter, func(t *testing.T) { + page, err := getResources(t, agg, `{"ResourceTypeFilters":["`+filter+`"]}`) + require.NoError(t, err) + assert.Empty(t, page.ResourceTagMappingList) + }) + } +} diff --git a/spinifex/runtime/roles/awsgw/tagging_listers.go b/spinifex/runtime/roles/awsgw/tagging_listers.go new file mode 100644 index 000000000..632188e92 --- /dev/null +++ b/spinifex/runtime/roles/awsgw/tagging_listers.go @@ -0,0 +1,150 @@ +package awsgw + +import ( + "context" + "strings" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/elbv2" + ec2tags "github.com/mulgadc/spinifex/spinifex/domains/ec2/tags" + "github.com/mulgadc/spinifex/spinifex/domains/tagging" + "github.com/mulgadc/spinifex/spinifex/foundation/aws/arn" + handlers_elbv2 "github.com/mulgadc/spinifex/spinifex/handlers/elbv2" + "github.com/nats-io/nats.go" +) + +// newTaggingAggregator wires GetResources over ELBv2 then the EC2 tag index, +// the order results have always been gathered in. +func newTaggingAggregator(nc *nats.Conn, region string) *tagging.Aggregator { + return tagging.NewAggregator(newELBv2Lister(nc), newEC2IndexLister(nc, region)) +} + +// elbv2DescribeTagsBatchSize is the AWS ceiling on ARNs per ELBv2 DescribeTags call. +const elbv2DescribeTagsBatchSize = 20 + +// ec2IndexLister lists EC2 resources from the central tag index over the +// ec2.DescribeTags subject, building each ARN in the gateway's region. +type ec2IndexLister struct { + svc ec2tags.TagsService + region string +} + +var _ tagging.TaggedLister = (*ec2IndexLister)(nil) + +func newEC2IndexLister(nc *nats.Conn, region string) *ec2IndexLister { + return &ec2IndexLister{svc: ec2tags.NewNATSTagsService(nc), region: region} +} + +func (l *ec2IndexLister) ListTagged(ctx context.Context, accountID string, typeFilters map[string]bool) ([]tagging.TaggedResource, error) { + out, err := l.svc.DescribeTags(ctx, &ec2.DescribeTagsInput{}, accountID) + if err != nil { + return nil, err + } + return ec2TaggedResources(out.Tags, l.region, accountID, typeFilters), nil +} + +// ec2TaggedResources groups tag descriptions by resource ID and keeps those +// whose "ec2:" passes typeFilters. The type comes from the first stored +// tag record for the ID, and an empty ID is skipped. +func ec2TaggedResources(tagDescriptions []*ec2.TagDescription, region, accountID string, typeFilters map[string]bool) []tagging.TaggedResource { + type resourceTags struct { + fullType string + tags map[string]string + } + byResource := make(map[string]*resourceTags) + for _, td := range tagDescriptions { + id := aws.StringValue(td.ResourceId) + if id == "" { + continue + } + rt := byResource[id] + if rt == nil { + rt = &resourceTags{fullType: "ec2:" + aws.StringValue(td.ResourceType), tags: map[string]string{}} + byResource[id] = rt + } + rt.tags[aws.StringValue(td.Key)] = aws.StringValue(td.Value) + } + + var out []tagging.TaggedResource + for id, rt := range byResource { + if !tagging.MatchesType(typeFilters, rt.fullType) { + continue + } + out = append(out, tagging.TaggedResource{ARN: ec2ARN(region, accountID, rt.fullType, id), Tags: rt.tags}) + } + return out +} + +// ec2ARN builds an arn:aws:ec2 ARN for a resource. fullType is "ec2:", +// read from the stored tag record rather than from the id. +func ec2ARN(region, accountID, fullType, id string) string { + kind := arn.EC2ResourceType(strings.TrimPrefix(fullType, "ec2:")) + return arn.FormatEC2(kind, region, accountID, id) +} + +// elbv2Lister lists ELBv2 load balancers and target groups admitted by the +// type filters, tagged or not, with tags from batched DescribeTags calls. +type elbv2Lister struct { + svc handlers_elbv2.ELBv2Service +} + +var _ tagging.TaggedLister = (*elbv2Lister)(nil) + +func newELBv2Lister(nc *nats.Conn) *elbv2Lister { + return &elbv2Lister{svc: handlers_elbv2.NewNATSELBv2Service(nc)} +} + +func (l *elbv2Lister) ListTagged(ctx context.Context, accountID string, typeFilters map[string]bool) ([]tagging.TaggedResource, error) { + wantLB := tagging.MatchesType(typeFilters, "elasticloadbalancing:loadbalancer") + wantTG := tagging.MatchesType(typeFilters, "elasticloadbalancing:targetgroup") + if !wantLB && !wantTG { + return nil, nil + } + + var arns []string + if wantLB { + lbs, err := l.svc.DescribeLoadBalancers(ctx, &elbv2.DescribeLoadBalancersInput{}, accountID) + if err != nil { + return nil, err + } + for _, lb := range lbs.LoadBalancers { + if lb.LoadBalancerArn != nil { + arns = append(arns, *lb.LoadBalancerArn) + } + } + } + if wantTG { + tgs, err := l.svc.DescribeTargetGroups(ctx, &elbv2.DescribeTargetGroupsInput{}, accountID) + if err != nil { + return nil, err + } + for _, tg := range tgs.TargetGroups { + if tg.TargetGroupArn != nil { + arns = append(arns, *tg.TargetGroupArn) + } + } + } + if len(arns) == 0 { + return nil, nil + } + + out := make([]tagging.TaggedResource, 0, len(arns)) + for start := 0; start < len(arns); start += elbv2DescribeTagsBatchSize { + end := min(start+elbv2DescribeTagsBatchSize, len(arns)) + tagsOut, err := l.svc.DescribeTags(ctx, &elbv2.DescribeTagsInput{ + ResourceArns: aws.StringSlice(arns[start:end]), + }, accountID) + if err != nil { + return nil, err + } + for _, td := range tagsOut.TagDescriptions { + tags := make(map[string]string, len(td.Tags)) + for _, t := range td.Tags { + tags[aws.StringValue(t.Key)] = aws.StringValue(t.Value) + } + out = append(out, tagging.TaggedResource{ARN: aws.StringValue(td.ResourceArn), Tags: tags}) + } + } + return out, nil +} diff --git a/spinifex/runtime/roles/awsgw/tagging_listers_test.go b/spinifex/runtime/roles/awsgw/tagging_listers_test.go new file mode 100644 index 000000000..0a6756e95 --- /dev/null +++ b/spinifex/runtime/roles/awsgw/tagging_listers_test.go @@ -0,0 +1,362 @@ +//test:in-package — the listers are unexported composition-root adapters with +// no production caller to reach them through yet. + +package awsgw + +import ( + "context" + "errors" + "fmt" + "slices" + "strconv" + "sync" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/elbv2" + "github.com/nats-io/nats.go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + testutil "github.com/mulgadc/spinifex/internal/testkit" + "github.com/mulgadc/spinifex/spinifex/bootstrap/config" + ec2tags "github.com/mulgadc/spinifex/spinifex/domains/ec2/tags" + "github.com/mulgadc/spinifex/spinifex/domains/tagging" + "github.com/mulgadc/spinifex/spinifex/foundation/aws/arn" + awserrors "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" + natsmsg "github.com/mulgadc/spinifex/spinifex/foundation/messaging/nats" + handlers_elbv2 "github.com/mulgadc/spinifex/spinifex/handlers/elbv2" + handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" +) + +const ( + listerRegion = "ap-southeast-2" + listerAccount = "111122223333" + foreignAccount = "444455556666" +) + +func serveSubject(t *testing.T, nc *nats.Conn, subject string, fn func(*nats.Msg)) { + t.Helper() + sub, err := nc.Subscribe(subject, fn) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) +} + +// failSubject answers every request on subject with err, as an owner would. +func failSubject(t *testing.T, nc *nats.Conn, subject string, err error) { + t.Helper() + serveSubject(t, nc, subject, func(msg *nats.Msg) { + natsmsg.ServeNATSRequestCtx(msg, func(context.Context, *map[string]any) (*struct{}, error) { return nil, err }) + }) +} + +func byARN(t *testing.T, got []tagging.TaggedResource) map[string]map[string]string { + t.Helper() + out := make(map[string]map[string]string, len(got)) + for _, r := range got { + _, dup := out[r.ARN] + require.False(t, dup, "ARN listed twice: %s", r.ARN) + out[r.ARN] = r.Tags + } + return out +} + +func sortedARNs(got []tagging.TaggedResource) []string { + out := make([]string, 0, len(got)) + for _, r := range got { + out = append(out, r.ARN) + } + slices.Sort(out) + return out +} + +// newEC2IndexFixture serves ec2.DescribeTags from a real tag index on an +// embedded JetStream and returns the index for seeding. +func newEC2IndexFixture(t *testing.T) (*nats.Conn, *ec2tags.TagsServiceImpl) { + t.Helper() + _, nc, js := testutil.StartTestJetStream(t) + kv, err := ec2tags.GetOrCreateTagsBucket(t.Context(), js) + require.NoError(t, err) + svc := ec2tags.NewTagsServiceImplWithStore(&config.Config{}, nil, kv) + serveSubject(t, nc, "ec2.DescribeTags", func(msg *nats.Msg) { + natsmsg.ServeNATSRequestCtx(msg, func(ctx context.Context, in *ec2.DescribeTagsInput) (*ec2.DescribeTagsOutput, error) { + return svc.DescribeTags(ctx, in, natsmsg.AccountIDFromMsg(msg)) + }) + }) + return nc, svc +} + +func seedEC2Tags(t *testing.T, svc *ec2tags.TagsServiceImpl, accountID string, ids []string, tags map[string]string) { + t.Helper() + in := &ec2.CreateTagsInput{Resources: aws.StringSlice(ids)} + for k, v := range tags { + in.Tags = append(in.Tags, &ec2.Tag{Key: aws.String(k), Value: aws.String(v)}) + } + _, err := svc.CreateTags(t.Context(), in, accountID) + require.NoError(t, err) +} + +func TestEC2IndexLister_ListsCallerIndexEntriesAsRegionalARNs(t *testing.T) { + nc, svc := newEC2IndexFixture(t) + seedEC2Tags(t, svc, listerAccount, []string{"sg-0000000000000000a"}, map[string]string{"env": "prod", "Name": "web"}) + seedEC2Tags(t, svc, listerAccount, []string{"subnet-0000000000000000b"}, map[string]string{"env": "dev"}) + seedEC2Tags(t, svc, listerAccount, []string{"not-a-real-id", "vol-00000000000000000"}, map[string]string{"env": "orphan"}) + seedEC2Tags(t, svc, foreignAccount, []string{"sg-0000000000000000f"}, map[string]string{"env": "other"}) + + got, err := newEC2IndexLister(nc, listerRegion).ListTagged(t.Context(), listerAccount, nil) + require.NoError(t, err) + + // Unresolved IDs are listed: an unrecognised prefix as type "unknown", a + // well-formed ID naming no resource as that resource's type. + prefix := "arn:aws:ec2:" + listerRegion + ":" + listerAccount + ":" + assert.Equal(t, map[string]map[string]string{ + prefix + "security-group/sg-0000000000000000a": {"env": "prod", "Name": "web"}, + prefix + "subnet/subnet-0000000000000000b": {"env": "dev"}, + prefix + "unknown/not-a-real-id": {"env": "orphan"}, + prefix + "volume/vol-00000000000000000": {"env": "orphan"}, + }, byARN(t, got)) +} + +func TestEC2IndexLister_TypeFilters(t *testing.T) { + nc, svc := newEC2IndexFixture(t) + seedEC2Tags(t, svc, listerAccount, []string{"sg-0000000000000000a"}, map[string]string{"env": "prod"}) + seedEC2Tags(t, svc, listerAccount, []string{"subnet-0000000000000000b"}, map[string]string{"env": "prod"}) + lister := newEC2IndexLister(nc, listerRegion) + prefix := "arn:aws:ec2:" + listerRegion + ":" + listerAccount + ":" + + for _, tc := range []struct { + filters map[string]bool + want []string + }{ + {map[string]bool{"ec2": true}, []string{prefix + "security-group/sg-0000000000000000a", prefix + "subnet/subnet-0000000000000000b"}}, + {map[string]bool{"ec2:subnet": true}, []string{prefix + "subnet/subnet-0000000000000000b"}}, + {map[string]bool{"ec2:subnet": true, "ec2:security-group": true}, []string{prefix + "security-group/sg-0000000000000000a", prefix + "subnet/subnet-0000000000000000b"}}, + {map[string]bool{"elasticloadbalancing": true}, []string{}}, + {map[string]bool{"ec2:instance": true}, []string{}}, + } { + t.Run(fmt.Sprint(tc.filters), func(t *testing.T) { + got, err := lister.ListTagged(t.Context(), listerAccount, tc.filters) + require.NoError(t, err) + assert.Equal(t, tc.want, sortedARNs(got)) + }) + } +} + +func TestEC2IndexLister_ReturnsFreshTagMapsPerCall(t *testing.T) { + nc, svc := newEC2IndexFixture(t) + seedEC2Tags(t, svc, listerAccount, []string{"sg-0000000000000000a"}, map[string]string{"env": "prod"}) + lister := newEC2IndexLister(nc, listerRegion) + + first, err := lister.ListTagged(t.Context(), listerAccount, nil) + require.NoError(t, err) + require.Len(t, first, 1) + first[0].Tags["env"] = "mutated" + + second, err := lister.ListTagged(t.Context(), listerAccount, nil) + require.NoError(t, err) + require.Len(t, second, 1) + assert.Equal(t, map[string]string{"env": "prod"}, second[0].Tags) +} + +func TestEC2IndexLister_PropagatesOwnerError(t *testing.T) { + _, nc, _ := testutil.StartTestJetStream(t) + failSubject(t, nc, "ec2.DescribeTags", errors.New(awserrors.ErrorServerInternal)) + + got, err := newEC2IndexLister(nc, listerRegion).ListTagged(t.Context(), listerAccount, nil) + require.EqualError(t, err, awserrors.ErrorServerInternal) + assert.Nil(t, got) +} + +func TestEC2TaggedResources(t *testing.T) { + tags := []*ec2.TagDescription{ + {ResourceId: aws.String("subnet-1"), ResourceType: aws.String("subnet"), Key: aws.String("Name"), Value: aws.String("a")}, + {ResourceId: aws.String("subnet-1"), ResourceType: aws.String("subnet"), Key: aws.String("env"), Value: aws.String("prod")}, + {ResourceId: aws.String("sg-1"), ResourceType: aws.String("security-group"), Key: aws.String("Name"), Value: aws.String("b")}, + {ResourceId: aws.String(""), ResourceType: aws.String("subnet"), Key: aws.String("skip"), Value: aws.String("me")}, + } + + // No type filter: both resources, tags merged per resource, empty ID skipped. + all := ec2TaggedResources(tags, "us-east-1", "123456789012", nil) + assert.Equal(t, map[string]map[string]string{ + "arn:aws:ec2:us-east-1:123456789012:subnet/subnet-1": {"Name": "a", "env": "prod"}, + "arn:aws:ec2:us-east-1:123456789012:security-group/sg-1": {"Name": "b"}, + }, byARN(t, all)) + + // Type filter narrows to subnets only. + onlySubnets := ec2TaggedResources(tags, "us-east-1", "123456789012", map[string]bool{"ec2:subnet": true}) + assert.Equal(t, []string{"arn:aws:ec2:us-east-1:123456789012:subnet/subnet-1"}, sortedARNs(onlySubnets)) +} + +func TestEC2ARN(t *testing.T) { + assert.Equal(t, + "arn:aws:ec2:us-east-1:123456789012:subnet/subnet-abc", + ec2ARN("us-east-1", "123456789012", "ec2:subnet", "subnet-abc")) +} + +// elbv2Fixture serves the ELBv2 read subjects from a real ELBv2 service on an +// embedded JetStream, recording the ARN count of every DescribeTags request. +type elbv2Fixture struct { + nc *nats.Conn + store *handlers_elbv2.Store + + mu sync.Mutex + describeCalls int + tagBatches []int +} + +func newELBv2Fixture(t *testing.T) *elbv2Fixture { + t.Helper() + _, nc, _ := testutil.StartTestJetStream(t) + masterKey, err := handlers_iam.GenerateMasterKey() + require.NoError(t, err) + svc, err := handlers_elbv2.NewELBv2ServiceImplWithNATS(nil, nc, masterKey) + require.NoError(t, err) + t.Cleanup(svc.Close) + store, err := handlers_elbv2.NewStore(t.Context(), nc) + require.NoError(t, err) + + f := &elbv2Fixture{nc: nc, store: store} + serveSubject(t, nc, "elbv2.DescribeLoadBalancers", func(msg *nats.Msg) { + f.countDescribe() + natsmsg.ServeNATSRequestCtx(msg, func(ctx context.Context, in *elbv2.DescribeLoadBalancersInput) (*elbv2.DescribeLoadBalancersOutput, error) { + return svc.DescribeLoadBalancers(ctx, in, natsmsg.AccountIDFromMsg(msg)) + }) + }) + serveSubject(t, nc, "elbv2.DescribeTargetGroups", func(msg *nats.Msg) { + f.countDescribe() + natsmsg.ServeNATSRequestCtx(msg, func(ctx context.Context, in *elbv2.DescribeTargetGroupsInput) (*elbv2.DescribeTargetGroupsOutput, error) { + return svc.DescribeTargetGroups(ctx, in, natsmsg.AccountIDFromMsg(msg)) + }) + }) + serveSubject(t, nc, "elbv2.DescribeTags", func(msg *nats.Msg) { + natsmsg.ServeNATSRequestCtx(msg, func(ctx context.Context, in *elbv2.DescribeTagsInput) (*elbv2.DescribeTagsOutput, error) { + f.mu.Lock() + f.tagBatches = append(f.tagBatches, len(in.ResourceArns)) + f.mu.Unlock() + return svc.DescribeTags(ctx, in, natsmsg.AccountIDFromMsg(msg)) + }) + }) + return f +} + +func (f *elbv2Fixture) countDescribe() { + f.mu.Lock() + defer f.mu.Unlock() + f.describeCalls++ +} + +func (f *elbv2Fixture) calls() (describes int, batches []int) { + f.mu.Lock() + defer f.mu.Unlock() + return f.describeCalls, slices.Clone(f.tagBatches) +} + +func (f *elbv2Fixture) putLB(t *testing.T, accountID, name, id string, tags map[string]string) string { + t.Helper() + lbARN := arn.FormatELBv2LoadBalancer(listerRegion, accountID, name, id, "application") + require.NoError(t, f.store.PutLoadBalancer(t.Context(), &handlers_elbv2.LoadBalancerRecord{ + LoadBalancerArn: lbARN, LoadBalancerID: id, Name: name, Type: "application", AccountID: accountID, Tags: tags, + })) + return lbARN +} + +func (f *elbv2Fixture) putTG(t *testing.T, accountID, name, id string, tags map[string]string) string { + t.Helper() + tgARN := arn.FormatELBv2TargetGroup(listerRegion, accountID, name, id) + require.NoError(t, f.store.PutTargetGroup(t.Context(), &handlers_elbv2.TargetGroupRecord{ + TargetGroupArn: tgARN, TargetGroupID: id, Name: name, AccountID: accountID, Tags: tags, + })) + return tgARN +} + +func TestELBv2Lister_ListsCallerLoadBalancersAndTargetGroupsTaggedOrNot(t *testing.T) { + f := newELBv2Fixture(t) + taggedLB := f.putLB(t, listerAccount, "tagged", "00000000000000a1", map[string]string{"env": "prod", "team": "web"}) + untaggedLB := f.putLB(t, listerAccount, "untagged", "00000000000000a2", nil) + taggedTG := f.putTG(t, listerAccount, "tg", "00000000000000b1", map[string]string{"env": "prod"}) + f.putLB(t, foreignAccount, "foreign", "00000000000000c1", map[string]string{"env": "other"}) + f.putTG(t, foreignAccount, "foreign-tg", "00000000000000c2", nil) + + got, err := newELBv2Lister(f.nc).ListTagged(t.Context(), listerAccount, nil) + require.NoError(t, err) + assert.Equal(t, map[string]map[string]string{ + taggedLB: {"env": "prod", "team": "web"}, + untaggedLB: {}, + taggedTG: {"env": "prod"}, + }, byARN(t, got)) +} + +func TestELBv2Lister_TypeFilters(t *testing.T) { + f := newELBv2Fixture(t) + lb := f.putLB(t, listerAccount, "lb", "00000000000000a1", map[string]string{"env": "prod"}) + tg := f.putTG(t, listerAccount, "tg", "00000000000000b1", map[string]string{"env": "prod"}) + lister := newELBv2Lister(f.nc) + + for _, tc := range []struct { + filters map[string]bool + want []string + wantDescribe int + }{ + {map[string]bool{"elasticloadbalancing": true}, []string{lb, tg}, 2}, + {map[string]bool{"elasticloadbalancing:loadbalancer": true}, []string{lb}, 1}, + {map[string]bool{"elasticloadbalancing:targetgroup": true}, []string{tg}, 1}, + // Neither served type admitted: no owner call is made. + {map[string]bool{"elasticloadbalancing:listener": true}, []string{}, 0}, + {map[string]bool{"ec2": true}, []string{}, 0}, + } { + t.Run(fmt.Sprint(tc.filters), func(t *testing.T) { + before, _ := f.calls() + got, err := lister.ListTagged(t.Context(), listerAccount, tc.filters) + require.NoError(t, err) + assert.Equal(t, tc.want, sortedARNs(got)) + after, _ := f.calls() + assert.Equal(t, tc.wantDescribe, after-before) + }) + } +} + +func TestELBv2Lister_BatchesDescribeTagsByTwenty(t *testing.T) { + f := newELBv2Fixture(t) + want := make([]string, 0, 45) + for i := range 45 { + want = append(want, f.putLB(t, listerAccount, fmt.Sprintf("lb-%02d", i), fmt.Sprintf("%016x", i+1), map[string]string{"n": strconv.Itoa(i)})) + } + slices.Sort(want) + + got, err := newELBv2Lister(f.nc).ListTagged(t.Context(), listerAccount, map[string]bool{"elasticloadbalancing:loadbalancer": true}) + require.NoError(t, err) + assert.Equal(t, want, sortedARNs(got)) + _, batches := f.calls() + assert.Equal(t, []int{20, 20, 5}, batches) +} + +func TestELBv2Lister_PropagatesOwnerErrors(t *testing.T) { + for _, subject := range []string{"elbv2.DescribeLoadBalancers", "elbv2.DescribeTargetGroups", "elbv2.DescribeTags"} { + t.Run(subject, func(t *testing.T) { + _, nc, _ := testutil.StartTestJetStream(t) + ok := func(out any) func(*nats.Msg) { + return func(msg *nats.Msg) { + natsmsg.ServeNATSRequestCtx(msg, func(context.Context, *map[string]any) (*any, error) { return &out, nil }) + } + } + lbs := &elbv2.DescribeLoadBalancersOutput{LoadBalancers: []*elbv2.LoadBalancer{{LoadBalancerArn: aws.String(arn.FormatELBv2LoadBalancer(listerRegion, listerAccount, "lb", "00000000000000a1", "application"))}}} + handlers := map[string]func(*nats.Msg){ + "elbv2.DescribeLoadBalancers": ok(lbs), + "elbv2.DescribeTargetGroups": ok(&elbv2.DescribeTargetGroupsOutput{}), + "elbv2.DescribeTags": ok(&elbv2.DescribeTagsOutput{}), + } + for s, h := range handlers { + if s == subject { + failSubject(t, nc, s, errors.New(awserrors.ErrorServerInternal)) + continue + } + serveSubject(t, nc, s, h) + } + + got, err := newELBv2Lister(nc).ListTagged(t.Context(), listerAccount, nil) + require.EqualError(t, err, awserrors.ErrorServerInternal) + assert.Nil(t, got) + }) + } +}