Add AWS MemoryDB Full-Parity Support - #305
Conversation
MemoryDB for Redis/Valkey is a durable, in-VPC cluster service. Unlike ElastiCache it is control-plane only (no Set/Get data plane), so it gets a dedicated driver (services/memorydb/driver) rather than reusing the cache driver, which mandates data-plane methods. - Driver: 33 core operations (clusters/shards/nodes, ACLs & users, parameter groups, subnet groups, snapshots, tags, engine-version & event catalogs) plus two type-asserted optional capabilities: MultiRegion (7) and ReservedNodes (3). - Provider (providers/aws/memorydb): in-memory Mock with shard/node/endpoint topology, reference validation, restore-from-snapshot, failover, CloudWatch metric emission, and clone-on-read on every return path. - Server (server/aws/memorydb): AWS JSON 1.1 handler on the "AmazonMemoryDB." target prefix, with typed faults (*NotFoundFault / *AlreadyExistsFault / InvalidParameterValueException). Verified with real aws-sdk-go-v2/memorydb round-trip tests plus wire-error assertions. - Wiring: registered in the AWS provider and server bundles; dedicated memorydb:* cost rate keys; docs/services.md section and counts updated.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Reject NumShards/ReplicasPerShard outside the MemoryDB service limits (500 shards, 5 replicas) with InvalidArgument before the mock allocates the shard/node topology, so an oversized create/update request cannot drive unbounded memory use. Fixes the CodeQL go/uncontrolled-allocation-size alert.
The caller-side validateShardTopology guard isn't recognized across the function boundary by CodeQL's uncontrolled-allocation-size analysis. Add a defensive upper-bound clamp in buildShards itself, right before the slice allocations, so the bound dominates the make() in-function. Behavior is unchanged for valid input (callers still reject out-of-range counts first).
CodeQL's uncontrolled-allocation-size analysis does not treat the clamp reassignment as a sanitizer, so it kept flagging make([]T, 0, n) where n derives from caller input. Allocate the shard/node slices as nil and let append grow them; the clamped loop bounds keep growth bounded. No behavior change for valid input.
NitinKumar004
left a comment
NitinKumar004
left a comment
There was a problem hiding this comment.
Deep end-to-end lifecycle review — AWS MemoryDB (full parity)
Reviewed the whole PR in an isolated worktree (gates, static/coverage, provider lifecycle, server/SDK/typed-faults, cross-cutting wiring), with adversarial verification. The whole MemoryDB lifecycle is supported and working end-to-end — in notably good shape — but not fully gap-free.
Gates: green — build ✓ vet ✓ test ✓ -race ✓ gofmt ✓ go mod tidy ✓; lint clean. All 43 driver ops route, 42 are SDK-round-trip-tested against the real aws-sdk-go-v2/service/memorydb client. No High issues. Verified solid: clone-on-read (all paths), single-lock RMW, cascade delete, determinism, create-path reference validation, the buildShards allocation bound (validates ≤500 shards/≤5 replicas before allocation — a genuine fix, not CodeQL-silencing), disjoint X-Amz-Target dispatch (no collision), metrics + cost (tested), and — notably — the documented timestamp hazard is properly mitigated (all timestamps are *time.Time → nil → null, so nothing is ever emitted as an SDK-rejecting RFC3339 string).
Lifecycle scorecard
| Stage | Status |
|---|---|
| Cluster create/topology/describe/update/delete + ref validation | complete |
| ACLs & Users (bidirectional linkage + guards) | complete |
| Parameter groups / Subnet groups (in-use + default guards) | complete |
| Snapshots (create/copy/delete/restore) | works — restore loses replica count |
| Shard failover (state guards) | works — checks wrong shard index (harmless) |
| Reserved nodes | works — duplicate-purchase fault type wrong |
| Multi-region clusters | CRUD works — parent linkage absent |
Medium
- Multi-region parent linkage is absent —
DeleteMultiRegionClusterguardslen(c.Members) > 0, butMembersis never populated (CreateCluster storesMultiRegionClusterNamewithout registering the cluster as a member or validating the MRC exists). The guard is dead code, so an MRC can be deleted while regional clusters still reference it → danglingMultiRegionClusterName(the #303/#304 dangling-link class, in the multi-region path). (inline) - Reserved-node duplicate purchase → unmodeled fault — the purchase path maps AlreadyExists via the
"ReservedNodesOffering"noun →ReservedNodesOfferingAlreadyExistsFault, which the SDK does not model (onlyReservedNodeAlreadyExistsFaultexists), soerrors.As(&types.ReservedNodeAlreadyExistsFault{})fails to match. (inline)
Low
- Restore loses replica count —
ClusterConfigurationhasNumShardsbut no replica field, so a restored cluster is replica-less unless re-specified. (inline) UpdateClusterreturnsNotFoundfor a dangling ACL/param-group whileCreateClusterreturnsInvalidArgument— inconsistent code for the same error class (clusters.go).FailoverShardchecksShards[0].NumberOfNodesregardless of the requested shard (harmless while topology is uniform);DeleteClusterwith an existingfinalSnapshotNamesilently overwrites it;CreateACLretains duplicate user names.- Paging is single-shot (
MaxResults/NextTokenignored);DescribeMultiRegionParametershandled-but-untested; the MR param-group error noun is wrong (latent-dead);BatchUpdateCluster/DescribeServiceUpdatesunrouted (out of scope, graceful default). - The aliasing test covers only the cluster path (code is correct for all); two
docs/services.mdsignature typos (DescribeEngineVersions/DescribeEvents); non-deterministicListTagsorder (AWS leaves it unspecified). - Coverage below the 90% pillar: provider 73.8%, server 71.8% (disclosed in the PR body).
Verdict: comment. The core cluster/ACL/user/param-group/subnet-group/snapshot lifecycle is complete, correct, and SDK-tested; the two Mediums are fidelity gaps confined to the AWS-only optional surfaces (multi-region linkage, reserved-node fault type).
| return nil, cerrors.Newf(cerrors.NotFound, "multi-region cluster %q not found", name) | ||
| } | ||
|
|
||
| if len(c.Members) > 0 { |
There was a problem hiding this comment.
[Medium] Multi-region parent linkage is absent — this guard is dead code. len(c.Members) > 0 never triggers because Members is never populated: CreateCluster stores cfg.MultiRegionClusterName on the cluster but never appends the cluster to the MRC's Members, and never validates the MRC exists (validateClusterRefs doesn't check it). → Create MRC "m" → create a regional cluster with MultiRegionClusterName="m" → DeleteMultiRegionCluster("m") succeeds, leaving the regional cluster with a dangling MultiRegionClusterName pointing at a deleted MRC. Real MemoryDB rejects deleting an MRC with attached regional clusters. Register the cluster into Members on create (and validate the MRC exists), so the guard is live.
There was a problem hiding this comment.
Fixed in 7631821. CreateCluster now validates the referenced MRC exists (rejecting a dangling reference with InvalidParameterValueException) and registers the regional cluster into the MRC's Members; DeleteCluster unregisters it. The delete guard is now live — deleting an MRC with attached regional clusters returns FailedPrecondition. Covered by TestMultiRegionParentLinkage (provider).
| node, err := rn.PurchaseReservedNodesOffering(r.Context(), | ||
| aws.ToString(in.ReservedNodesOfferingId), aws.ToString(in.ReservationId), int(aws.ToInt32(in.NodeCount))) | ||
| if err != nil { | ||
| writeErr(w, "ReservedNodesOffering", err) |
There was a problem hiding this comment.
[Medium] Reserved-node duplicate purchase emits an unmodeled fault. On PurchaseReservedNodesOffering, the provider returns AlreadyExists for a duplicate ReservationId, and this maps via the "ReservedNodesOffering" noun → __type: ReservedNodesOfferingAlreadyExistsFault — which the SDK does not model (its reserved-node faults are ReservedNodeAlreadyExistsFault, ReservedNodeNotFoundFault, ReservedNodeQuotaExceededFault, ReservedNodesOfferingNotFoundFault). → A second purchase with the same ReservationId returns a generic smithy.GenericAPIError, so errors.As(err, &types.ReservedNodeAlreadyExistsFault{}) fails. Map the purchase AlreadyExists to the "ReservedNode" noun (real AWS returns ReservedNodeAlreadyExistsFault); the "ReservedNodesOffering" noun is only correct for the NotFound case.
There was a problem hiding this comment.
Fixed in 7631821. The purchase path now maps AlreadyExists (duplicate ReservationId) to the ReservedNode noun → ReservedNodeAlreadyExistsFault (SDK-modeled); the NotFound (missing offering) case keeps the ReservedNodesOffering noun. Verified with TestSDKReservedNodeDuplicateFault, which asserts errors.As(&types.ReservedNodeAlreadyExistsFault{}).
| } | ||
|
|
||
| // ClusterConfiguration captures a cluster's shape at snapshot time. | ||
| type ClusterConfiguration struct { |
There was a problem hiding this comment.
[Low] Restore loses the replica count. ClusterConfiguration (the snapshot's stored config) carries NumShards but no replica field, while CreateClusterConfig has NumReplicasPerShard and UpdateCluster has ReplicaCount. → Snapshot a cluster with N replicas → restore via CreateCluster{SnapshotName} → the restored cluster is replica-less (NumberOfNodes=1 per shard, no HA), because the replica count was never captured. Add a replica field to ClusterConfiguration and honor it on restore. (Restore also ignores the snapshot's SubnetGroupName/ParameterGroupName/TLSEnabled.)
There was a problem hiding this comment.
Fixed in 7631821. Added ReplicasPerShard (and TLSEnabled) to ClusterConfiguration; snapshotFromCluster now captures the replica count and TLS, and CreateCluster's restore path honors ReplicasPerShard/SubnetGroupName/ParameterGroupName/TLSEnabled for any field the caller leaves unset (subnet/param groups only when they still exist). Covered by TestRestorePreservesTopology.
…pagination, service updates Fixes from NitinKumar004's review plus the two larger parity items: Medium - Multi-region parent linkage: CreateCluster now validates the referenced MRC exists and registers the regional cluster into its Members; DeleteCluster unregisters it. The delete guard is now live — an MRC with attached regional clusters can no longer be deleted, and a dangling MultiRegionClusterName can no longer be created. - Reserved-node duplicate purchase now maps AlreadyExists to the ReservedNode noun (ReservedNodeAlreadyExistsFault, SDK-modeled) instead of the unmodeled ReservedNodesOfferingAlreadyExistsFault; NotFound keeps the Offering noun. Low - Restore preserves replica count (new ClusterConfiguration.ReplicasPerShard) plus SubnetGroupName/ParameterGroupName/TLSEnabled when the caller omits them. - UpdateCluster returns InvalidArgument (not NotFound) for a dangling ACL/parameter-group, consistent with CreateCluster. - FailoverShard checks the requested shard's node count, not shard[0]. - DeleteCluster rejects a finalSnapshotName that already exists. - CreateACL de-duplicates user names. - Multi-region parameter-group error noun corrected; ListTags returns tags in deterministic (sorted) order; docs signature typos fixed. Full parity additions - Pagination: every Describe* op honors MaxResults/NextToken via a server-side opaque base64 offset token over the deterministic result set; a bad token yields InvalidParameterValueException. - Service updates: new ServiceUpdates optional capability (DescribeServiceUpdates + BatchUpdateCluster with partial-success UnprocessedClusters), routed through the JSON 1.1 handler. Edge-case tests added across provider and SDK round-trip layers; docs and counts updated.
|
Thanks for the deep review, @NitinKumar004 — all Mediums and Lows are addressed in 7631821, and I took the "add both extras" call on the two out-of-scope items. Medium
Low
Extras added this round (previously flagged out-of-scope)
Edge-case tests added across the provider and SDK round-trip layers. Coverage: provider 75.5%, server 71.3%. Full local gate green (build/vet/ Not changed: |
NitinKumar004
left a comment
NitinKumar004
left a comment
There was a problem hiding this comment.
Review comments addressed — approving ✅
Verified 7631821 against the earlier review; every finding is resolved, correctly and with tests. Gates green (build ✓ vet ✓ test ✓ -race ✓ gofmt ✓ go mod tidy ✓; lint clean on the MemoryDB files).
Medium — fixed:
- Multi-region parent linkage →
CreateClusternow validates the MRC exists andregisterMRCMember/unregisterMRCMemberpopulateMembers, so the delete-guard is live and danglingMultiRegionClusterNamereferences can no longer occur. - Reserved-node duplicate-purchase fault → a duplicate reservation now maps to
ReservedNodeAlreadyExistsFault(the SDK-modeled type), soerrors.Asmatches; the*Offering*noun is kept only for the NotFound case.
Low — fixed:
- Restore now preserves the replica count (
ClusterConfiguration.ReplicasPerShard+ honored on restore, withAvailabilityMode). FailoverShardchecks the requested shard (c.Shards[shard]), notShards[0].- Pagination implemented (
paging.go, offset overSortedValues— deterministic) withNextTokenon the Describe ops. - Multi-region parameter-group error noun corrected to
MultiRegionParameterGroup. DescribeServiceUpdates+BatchUpdateClusterimplemented (newServiceUpdatescapability) — beyond what the review flagged.
Full MemoryDB lifecycle (cluster/topology → ACLs/users → parameter/subnet groups → snapshots/restore → failover → multi-region → reserved nodes → service updates) is implemented, wired (metrics/cost, disjoint X-Amz-Target dispatch), and round-trips through the real aws-sdk-go-v2/service/memorydb client. Test suite is stable (verified across repeated full runs and a -count=10 stress of the MemoryDB packages). Non-blocking follow-up: provider/server coverage ~72–75% (below the 90% pillar, as disclosed). LGTM.
3 participants
Objective
Add full-parity support for AWS MemoryDB for Redis/Valkey — the next missing managed database-server surface after AlloyDB (#304). Covers every control-plane resource, connection, child resource, metric, cost dimension, pagination, and fleet-maintenance operation, so a real
aws-sdk-go-v2/service/memorydbclient works unchanged against a custom endpoint.What we found
Set/Getdata plane), so it does not fitservices/cache/driver. It gets its own driver.How we fixed it
services/memorydb/driver): 33 core operations + three type-asserted optional capabilities —MultiRegion(7),ReservedNodes(3),ServiceUpdates(2).providers/aws/memorydb): in-memoryMockwith shard/node/endpoint topology, reference validation, restore-from-snapshot (full shape incl. replicas/subnet/param/TLS), shard failover, multi-region parent linkage (members registered on create, guarded on delete), CloudWatch metrics, and clone-on-read on every return path.server/aws/memorydb): AWS JSON 1.1 handler on theAmazonMemoryDB.target prefix; canonical errors map to typed faults; pagination (MaxResults/NextToken) on everyDescribe*op via an opaque base64 offset token over the deterministic result set.memorydb:*cost keys;docs/services.mdsection + counts.Review round (thanks @NitinKumar004)
Medium: multi-region parent linkage now live; reserved-node duplicate purchase →
ReservedNodeAlreadyExistsFault.Low: restore preserves replica count (+subnet/param/TLS);
UpdateClusterdangling refs →InvalidArgument;FailoverShardchecks the requested shard;DeleteClusterrejects a duplicate final snapshot;CreateACLde-dupes users; MR param-group noun fixed;ListTagsdeterministic; doc typos fixed.Extras (opted in): pagination across all
Describe*ops; service updates (DescribeServiceUpdates+BatchUpdateClusterwith partial-successUnprocessedClusters).Alternatives not taken
CreateTime, reserved-nodeStartTime, eventDate, service-update dates): AWS JSON 1.1 encodes timestamps as epoch numbers, whichencoding/jsoncan't produce for atime.Time; those nullable fields are omitted so SDK deserialization stays clean. All other fields round-trip through the real SDK.Docs / Test / Playground
docs/services.md: MemoryDB section (per-resource tables incl. Service Updates + a pagination note) + recomputed totals (Grand Total 1310, +137 optional).ReservedNodeAlreadyExistsFault). Cost rate-catalog test.Test plan
go build ./...,go vet ./...go test ./...(full module, green)golangci-lint— 0 issues on all new/changed packagesgo mod tidycleanRisk & Rollback
Additive: new driver capability + provider/server packages plus opt-in bundle registration (nil driver ⇒ handler not registered). Behavioral changes are limited to more-accurate error codes (dangling refs → InvalidArgument; duplicate final snapshot rejected). Rollback = revert the branch.
Conclusion
MemoryDB reaches full parity — core control plane, multi-region, reserved nodes, service updates, and pagination — with the established driver→provider→server layering. Follow-ups (separate PRs, tracked in the missing-database-server initiative): AWS Timestream & Keyspaces, GCP Spanner & Bigtable, Azure Managed Cassandra.