Skip to content

Add GCP AlloyDB full-parity support - #304

Merged
thzgajendra merged 22 commits into
stackshy:developmentfrom
thzgajendra:feat/alloydb-full-parity
Jul 31, 2026
Merged

Add GCP AlloyDB full-parity support#304
thzgajendra merged 22 commits into
stackshy:developmentfrom
thzgajendra:feat/alloydb-full-parity

Conversation

@thzgajendra

Copy link
Copy Markdown
Collaborator

Objective

Add GCP AlloyDB as a first-class managed database server in cloudemu, at full parity (service + connectivity + metrics + cost + all child resources), reusing the relationaldb driver.

⚠️ Stacked on #303. This branch is cut from feat/sql-full-parity (PR #303), which carries the full relationaldb driver AlloyDB depends on (Users/Databases/etc.) and isn't on development yet. Review the single AlloyDB commit a37e2b2 (git show a37e2b2) — the rest of the diff is #303. Once #303 merges, this diff collapses to AlloyDB only.

What we found

DocumentDB/Neptune are already emulated via the RDS query-protocol handler; the genuinely-missing managed DB servers are MemoryDB/Timestream/Keyspaces (AWS), Spanner/Bigtable/AlloyDB (GCP), Managed Cassandra (Azure). AlloyDB is the first because it maps cleanly onto the relational driver.

How it's built

  • Provider providers/gcp/alloydb: implements RelationalDB (AlloyDB cluster→Cluster, instance PRIMARY/READ_POOL/SECONDARY→Instance, backup→ClusterSnapshot, restore) + Users + Databases, plus a new optional rdsdriver.AlloyDB capability (mirrors the ManagedInstances precedent) for AlloyDB-specifics: rich cluster/instance create, cross-region secondary + promote, instance failover/restart, continuous/automated-backup + maintenance config, and *Info accessors. Cloud Monitoring metrics on instance create.
  • Server server/gcp/alloydb: alloydb.googleapis.com/v1 REST handler (clusters[/instances|/users], backups, LRO operations, custom methods :promote/:createsecondary/:restore/:failover/:restart) using the SDK types for wire fidelity.
  • Wiring: providers/gcp/gcp.go (monitoring + combined CloudSQL+AlloyDB relational discovery adapter, TypeAlloyDBCluster); dedicated cost rates; docs updated.

Built to the #303 review lessons (from the start)

Copy-on-read (clone Tags/slices on every return path; aliasing test), copy-on-write + single-lock RMW, cascade delete, lifecycle state guards (promote/secondary preconditions, invalid instance-type + /-name rejection), metrics-on-create, dedicated cost keys, provider unit + SDK round-trip + wire-level error-mapping (404/409/400) tests.

Known limitation (documented)

AlloyDB's REST paths (/v1/projects/{p}/locations/{l}/clusters…) are identical to GKE's, and the SDK requests carry no service-distinguishing header — so the two can't be multiplexed on one server. Drivers.AlloyDB is therefore opt-in (left nil in DriversFrom; injected in place of GKE), exactly like the existing K8sAPI pattern. Standalone AlloyDB usage is fully functional and tested.

Test plan

  • go build ./..., go vet ./..., go test ./... green
  • golangci-lint clean on changed packages
  • Provider unit tests (lifecycle, aliasing, cascade, validation, secondary/promote, metrics) — ~83%
  • Server SDK round-trip + wire-error tests via the real google.golang.org/api/alloydb/v1 client

Risk & rollback

Additive: a new provider + server package + one optional driver capability + opt-in wiring. No existing behavior changes (AlloyDB nil by default). Rollback = revert a37e2b2.

Add databases, firewall rules and server configurations plus the server
failover action to Azure Database for MySQL Flexible Server, bringing it to
native parity with the ARM surface real armmysqlflexibleservers clients use.

Introduce Databases, FirewallRules, Configurations and Failover as optional
relationaldb driver capabilities (mirroring SubnetGroups), so the same
interfaces are reusable by the other managed-SQL services. The mock stores each
family per server and cascade-deletes children on server delete; the ARM handler
routes databases/firewallRules/configurations, updateConfigurations (batch) and
the failover action. Covered by real-SDK round-trip tests and mock-level
error-path/cascade tests.
…ources

Add databases, firewall rules and server configurations to Azure Database for
PostgreSQL Flexible Server, reusing the Databases/FirewallRules/Configurations
optional relationaldb capabilities introduced for MySQL Flex. Postgres Flex has
no failover action and no batch-configuration endpoint, and its configuration
resource accepts both PUT and PATCH; the handler and mock reflect that. The mock
cascade-deletes children on server delete. Covered by real-SDK round-trip tests
(the SDK has no ClientFactory, so each client is built from shared options) plus
mock-level default/error/cascade tests.
Add firewall rules, virtual-network rules, elastic pools, failover groups and
the Azure AD administrator to Azure SQL (Microsoft.Sql). Firewall rules reuse
the shared FirewallRules capability; the other four are added as optional
relationaldb capabilities (VNetRules, ElasticPools, FailoverGroups, AADAdmins)
alongside the existing SubnetGroups pattern. Failover-group failover flips the
local replication role between Primary and Secondary. The mock cascade-deletes
all child resources on server delete and returns isolated copies of the
slice-bearing failover-group state. Covered by real-SDK (armsql) round-trip
tests across all five families plus mock-level error/cascade/aliasing tests.
…ance ops

Add databases (via the shared Databases capability), users and client SSL certs
as instance child resources, plus the clone, failover, promote-replica and
start/stop-replica instance actions to GCP Cloud SQL. Users and SSL certs are
new optional relationaldb capabilities (Users, SslCerts); clone and replica
promotion are Clonable and ReplicaPromotion; failover reuses the shared Failover
capability. The REST path parser now recognizes the databases/users/sslCerts
sub-collections, and the users route honors Cloud SQL's ?name= query quirk for
delete and update. The mock cascade-deletes children on instance delete. Tiers
and flags catalogs are intentionally out of scope (separate path shapes, static
data). Covered by real sqladmin SDK round-trip tests plus mock-level
clone/cascade/error tests.
…oud Asset

Add a RelationalDatabases discovery capability to the resource-discovery engine
and a walkRelationalDB walker, mirroring the Kubernetes adapter pattern. Azure
wires an adapter that projects Azure SQL logical servers plus MySQL/PostgreSQL
Flexible Servers, and GCP projects Cloud SQL instances, so managed relational
databases appear in cross-service inventory. Resource Graph maps the portable
types to microsoft.sql/servers, microsoft.dbformysql/flexibleservers and
microsoft.dbforpostgresql/flexibleservers; Cloud Asset maps Cloud SQL to
sqladmin.googleapis.com/Instance. Both type-map switches become lookup tables to
stay under the cyclomatic-complexity gate. Covered by walker and type-map tests.
Add relationaldb:* entries to the cost rate catalog so provisioning a managed
database server/instance (RDS, Azure SQL, Azure MySQL/PostgreSQL Flexible
Server, Cloud SQL) is billed per instance-hour, clusters per cluster-hour, and
restores reuse the instance-hour, while snapshots and lifecycle actions are
free. The portable "relationaldb" service name means one catalog covers every
cloud. Covered by a cost-tracker test.
Document the native sub-resource capabilities added to Azure SQL, the Azure
MySQL/PostgreSQL Flexible Servers and Cloud SQL — databases, users, firewall/
vnet rules, configurations, elastic pools, failover groups, AAD admins, SSL
certs, clone/failover/replica actions — along with their discovery surfacing and
cost rates. Refresh the relational-database operation totals and the discovery
driver list.
…to feat/sql-full-parity

# Conflicts:
#	docs/features.md
#	docs/services.md
#	services/relationaldb/driver/driver.go
#	services/resourcediscovery/engine.go
#	services/resourcediscovery/walkers.go
Close the remaining managed-SQL parity gaps: serve the Cloud SQL machine-tier
catalog (GET /v1/projects/{p}/tiers) and the database-flag catalog
(GET /v1/flags, which is project-less) as static reference data, and emit
Microsoft.Sql/servers/elasticpools metrics (cpu/storage/workers percent) when an
Azure SQL elastic pool is created. Also annotate the ModifyInstance/ModifyCluster
methods across the four SQL mocks with the driver-interface hugeParam nolint now
that the shared ModifyInstanceInput grew. Covered by SDK tiers/flags round-trip
and an elastic-pool metric-emission test.
Add the Microsoft.Sql/managedInstances resource type and its managed databases
as a ManagedInstances optional relationaldb capability: managed-instance CRUD +
list + start/stop/failover actions, and managed-database CRUD + list. The
handler now matches the managedInstances resource type alongside servers and
routes both, and the mock cascade-deletes managed databases when their instance
is removed. Covered by an armsql managed-instance/database SDK round-trip test
(create → get → list → failover → cascade delete) and a mock-level lifecycle
test.
Update the relational-database section for the now-complete managed-SQL parity:
add the ManagedInstances capability row, note Cloud SQL's tiers/flags reference
catalogs and Azure SQL's Managed Instance family, and record the elastic-pool
metric namespace. Refresh the optional-operation totals (25 capability
interfaces, +109 relational / +118 grand-total optional).
…ring

Address review MEDIUM stackshy#1 and the list-ordering LOW: GetManagedInstance /
ListManagedInstances now clone the stored Tags map so a caller mutating the
returned map can't corrupt the store (the copy-on-read hole flagged as the
concurrent-map panic class), and every List* mock method iterates
memstore.SortedValues() instead of All() for deterministic SDK list ordering,
matching the documented convention. Large-value list loops use index iteration
to avoid per-element copies.
Address review MEDIUM stackshy#2/stackshy#3/stackshy#4 (Azure update paths) and stackshy#7 (MI discovery):

- PUT on an existing Azure SQL server / database / managed instance now applies
  the request body (upsert) instead of returning the stale record.
- PATCH is a genuine merge — elastic pools, failover groups and managed
  instances gain Update* capability methods that overlay only the fields the
  request supplied, so a partial PATCH no longer wipes unspecified fields; MI
  PATCH now decodes and applies its body instead of being a no-op.
- Managed instances are projected into cross-service discovery
  (microsoft.sql/managedinstances) via sqlDiscovery + the Resource Graph type
  map, so a created MI appears in inventory.

Covered by SDK PATCH-merge round-trip tests (elastic pool keeps its SKU when
only maxSizeBytes is patched; MI keeps administratorLogin when only vCores is
patched) and a managed-instance type-map test.
Address review MEDIUM stackshy#5. Databases now carry an ElasticPoolID (added to the
shared InstanceConfig/Instance/ModifyInstanceInput and surfaced as the
elasticPoolId ARM database property), so an SDK client placing a database into a
pool round-trips instead of silently dropping it. DeleteElasticPool now returns
a precondition error while the pool still contains databases, matching real
Azure's 409. Covered by a membership/delete-guard test.
Address review MEDIUM stackshy#6. A Cloud SQL insert with masterInstanceName now creates
an actual read replica: the replica records its master (ReadReplicaSource) and
the primary lists it (replicaNames), both surfaced in the instance body. Replica
lifecycle is faithful — start/stopReplica require the target to be a replica and
leave it RUNNABLE (no SUSPENDED state change); promoteReplica detaches it from
the primary (and rejects a non-replica); failover is a primary-only operation
and rejects a replica. Covered by SDK and mock replica-lifecycle tests.
…y tests

- Cloud SQL backup-run IDs are generated by the mock from the clock + a
  monotonic counter (was time.Now().UnixNano(), violating the Clock determinism
  rule and collision-prone); CloneInstance now clones the source's databases and
  resets replica linkage; UpdateUser doc corrected.
- Firewall rules validate IPv4 start/end (Azure SQL + both Flexible Servers);
  Azure SQL Managed Instance requires subnetId, as real Azure does.
- Added a real-SDK Resource Graph indexing test that drives sqlDiscovery
  end-to-end (logical server + managed instance appear and filter), and a
  -race concurrency test that mutates a returned managed-instance Tags map to
  pin the copy-on-read fix.
…idation

- Cloud SQL DeleteInstance now unlinks replica<->master before cascading
  children, so no dangling ReadReplicaSource/ReadReplicaTargets remain.
- Managed Instance lifecycle gains a state guard (transitionManagedInstance):
  Start/Stop respect current state, Failover requires Ready; MI create emits
  representative metrics and clones returned Tags; adds CreateManagedInstance
  cost rate.
- Cloud SQL RestoreBackup restores in place onto the existing instance via a
  new optional BackupRestorer capability (was create-new -> 409).
- Failover-group failover requires a partner server; firewall rules validate
  Start <= End; Azure Flex SetConfiguration rejects unknown params and empty
  values; ARM parser captures the action verb so forced vs planned failover
  and unknown POST verbs are distinguished; Cloud SQL selfLink/targetLink use
  the served /v1 prefix.
- Broaden new-package test coverage (sub-resource CRUD, MI lifecycle, Users
  update, backup get, error paths, server PATCH, raw MI start/stop).
…idation

- Deep-copy Tags/slice fields on every Instance/Cluster/Snapshot return path in
  azuresql and cloudsql (Describe/Create/Modify/Restore/Clone), so a returned
  value never aliases the memstore — fixes a potential concurrent-map crash and
  matches the managedinstance/FailoverGroup discipline.
- Azure Flex GetConfiguration/ListConfigurations return catalog defaults for a
  known-but-unset parameter (real Azure behavior) instead of NotFound/empty; the
  catalog now maps names to defaults.
- MySQL Flex batch config update is atomic: all entries are validated before any
  is applied, via a new optional BatchConfigurations capability.
- Azure SQL database create/modify validates the referenced elastic pool exists
  (symmetry with the pool-delete member guard).
- Cloud SQL rejects child (database/user) names containing '/' that would orphan
  a row; get-paths add length guards on single-ID Describe lookups.
- Flex firewall create validates start <= end (parity with Azure SQL).
- Tests: aliasing, config defaults, batch atomicity, elastic-pool validation,
  child-name rejection, FG update/list, managed-database delete, and Azure
  wire-level error-mapping (404/400) across all three Azure SQL services.
…to feat/sql-full-parity

# Conflicts:
#	docs/services.md
Adds GCP AlloyDB as a first-class managed database server, reusing the
relationaldb driver.

- Provider providers/gcp/alloydb: implements RelationalDB (clusters,
  instances, cluster backups→ClusterSnapshot, restore) + Users + Databases,
  plus a new optional rdsdriver.AlloyDB capability for AlloyDB-specific
  behavior — rich cluster/instance create, cross-region secondary + promote,
  instance failover/restart, continuous/automated-backup + maintenance config,
  and *Info accessors. Cloud Monitoring metrics on instance create.
  Copy-on-read (clone Tags/slices), '/'-name and instance-type validation,
  cascade delete, and lifecycle state guards throughout.
- Server server/gcp/alloydb: alloydb.googleapis.com/v1 REST handler
  (clusters[/instances|/users], backups, LRO operations, custom methods
  :promote/:createsecondary/:restore/:failover/:restart) using the SDK types
  for wire fidelity; SDK round-trip + wire-error (404/409/400) tests.
- Wiring: providers/gcp/gcp.go (field, monitoring, combined CloudSQL+AlloyDB
  relational discovery adapter, TypeAlloyDBCluster); server/gcp Drivers gains
  an opt-in AlloyDB field (left nil in DriversFrom — its paths collide with
  GKE's, so the two are mutually exclusive on one server).
- Dedicated AlloyDB cost rates; docs/services.md updated (capabilities, GCP
  column, operation counts).
…to feat/alloydb-full-parity

# Conflicts:
#	docs/services.md
#	go.mod
#	providers/gcp/gcp.go
#	services/cost/cost.go
#	services/resourcediscovery/walkers.go

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep end-to-end lifecycle review — GCP AlloyDB (commit a37e2b2)

Reviewed the single AlloyDB commit in an isolated worktree (gates, static/coverage, provider lifecycle, server/SDK/LRO, and cross-cutting wiring, with adversarial verification). The whole lifecycle is implemented and round-trips through the real alloydb/v1 SDK — but it is not gap-free.

Gates: greenbuild ✓ vet ✓ test ✓ -race ✓ gofmt ✓ go mod tidy ✓; lint clean on the AlloyDB files. Discovery has no CloudSQL regression (the combined adapter delegates to the unchanged CloudSQL path then appends AlloyDB); cost keys are consistent with the Aurora/managed-SQL precedent; docs op-counts match the 8-method capability; copy-on-read, single-lock RMW, promote consistency, LRO fidelity, and body decoding are all verified solid.

Lifecycle scorecard

Stage Status
Cluster create/get/list/patch/delete works — but delete doesn't detach secondaries
Instance create/get/list/patch/delete works — but no one-primary rule
Secondary + promote promote guards correct; dangling link on delete
Failover / restart no PRIMARY guard (no-op; invalid calls accepted)
Backup / restore complete (backups correctly survive cluster delete)
Users / Databases complete except UpdateUser route missing
LRO / custom methods / wire errors round-trip via real SDK (404/409 tested; 400 not)

Medium (should fix before merge)

  • GKE-vs-AlloyDB routing footgun — the "AlloyDB in place of GKE" contract is comment-only; the most-likely wiring silently breaks GKE. (inline)
  • Dangling SECONDARY link on PRIMARY delete — the exact #303 replica class the PR aimed to avoid; promote is fine, delete isn't. (inline)
  • One-primary-per-cluster not enforced — two PRIMARY instances can be created in one cluster. (inline)
  • UpdateUser is dead — the driver method is wired but no PATCH route, so Users.Patch → 405. (inline)

Low

  • FailoverInstance/RestartInstance have no PRIMARY/type guard — failover on a READ_POOL silently succeeds. (inline)
  • Unknown :verb mis-dispatches, and promoteCluster ignores r.MethodGET .../clusters/{c}:promote promotes (a state change on a read).
  • DescribeClusters/DescribeInstances list-all use .All() (unordered) while snapshots/users/dbs sort — inconsistent determinism within the same PR (clusters.go:76, instances.go:122).
  • 8 banner-comment blocks (// ---- X ----) across the AlloyDB files — contradicts the project's no-banner-comments convention.
  • Coverage below the 90% pillar: server/gcp/alloydb 70.2%, provider 82.8%; the 400 error path isn't wire-tested; the aliasing test covers only DescribeClusters.
  • patchCluster/patchInstance ignore the capability-assertion ok (nil-panic if a plain RelationalDB were ever registered — unreachable today); toWireUser.Name is the bare id, not the full resource path; operations GET returns a bare done with no response.
  • go.mod: the google.golang.org/api v0.277→v0.291 bump drags a broad transitive update (otel, grpc, golang.org/x/*) — tidy-clean and building, but wider blast radius than AlloyDB.

Verdict: comment. Additive and gate-green, with the full lifecycle working — but the GKE routing footgun (top priority) and the dangling-secondary / two-primaries / dead-UpdateUser Mediums are worth closing before merge.

Comment thread server/gcp/gcp.go
// operations}/... — the cluster/operations paths are identical to GKE's, so
// the two are mutually exclusive on one server. Registered before GKE so an
// AlloyDB-configured server (GKE nil) works; DriversFrom leaves AlloyDB nil.
if d.AlloyDB != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] GKE-vs-AlloyDB routing footgun. AlloyDB and GKE are registered as independent if != nil blocks (AlloyDB first), and dispatch is first-match-wins. But DriversFrom returns GKE: p.GKE (non-nil) and AlloyDB: nil (fromprovider.go:22,37), and AlloyDB's Matches claims the same /locations/.../clusters + /operations paths as GKE. → The natural enable — d := DriversFrom(p); d.AlloyDB = p.AlloyDB — leaves both non-nil, so AlloyDB shadows GKE's cluster/operations paths and the GKE control plane silently breaks. The "in place of GKE" contract lives only in comments; there's no helper/guard/test. Suggest a DriversFromWithAlloyDB that nils GKE, or a guard in New rejecting both non-nil.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c1cfafd. New now panics fast if both AlloyDB and GKE are non-nil (they share /locations/.../clusters paths), and added DriversFromWithAlloyDB(p) which enables AlloyDB and nils GKE — so the natural enable path is safe and the ambiguous one fails loudly instead of silently shadowing GKE.

// deleteClusterChildren removes instances, users and databases under a cluster.
// The caller holds the write lock. The trailing '/' prefix guard prevents
// "foo" from sweeping "foo2/…".
func (m *Mock) deleteClusterChildren(cluster string) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] Dangling SECONDARY link on PRIMARY delete. deleteClusterChildren removes prefix-keyed instances/users/databases but never scans clusterExtra for other clusters whose PrimaryCluster == id. → CreateSecondaryCluster(PrimaryCluster="P") then DeleteCluster("P") leaves the secondary with clusterExtra[S].PrimaryCluster == "P" pointing at a deleted cluster; AlloyDBClusterInfo(S) then reports a ghost primary. This is the same replica-linkage class fixed in #303 — real AlloyDB blocks deleting a primary with live secondaries or detaches them. Detach (or reject) on delete, and add a test for primary-delete-with-live-secondary.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c1cfafd. DeleteCluster now calls detachSecondaries(id), which scans clusterExtra and clears PrimaryCluster on any SECONDARY pointing at the deleted primary — so AlloyDBClusterInfo(secondary) no longer reports a ghost primary. Covered by TestPrimaryDeleteDetachesSecondaries.

// CreateAlloyDBInstance creates an instance of the given type under a cluster.
//
//nolint:gocritic // cfg matches the AlloyDB capability signature.
func (m *Mock) CreateAlloyDBInstance(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] One-primary-per-cluster not enforced. CreateAlloyDBInstance (and the base CreateInstance, which stamps InstanceType: PRIMARY unconditionally) checks duplicate ID and valid type but not whether the cluster already has a PRIMARY. → Creating two PRIMARY instances in one cluster both succeed; real AlloyDB permits exactly one primary. Also worth guarding: a SECONDARY instance type should only live in a SECONDARY cluster, and READ_POOL NodeCount is stored unvalidated (0 accepted).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c1cfafd. Both native CreateAlloyDBInstance and base CreateInstance now reject a second PRIMARY (clusterHasPrimary → FailedPrecondition). Also added: a SECONDARY instance requires a SECONDARY cluster, and READ_POOL requires nodeCount > 0. Covered by TestOnePrimaryPerCluster.

rdsdriver "github.com/stackshy/cloudemu/v2/services/relationaldb/driver"
)

func (h *Handler) serveUsers(w http.ResponseWriter, r *http.Request, p *alloyPath) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] UpdateUser is unreachable (dead capability). driver.Users.UpdateUser is implemented and functional, and the real SDK exposes Users.Patch, but the item-level router here only handles GET/DELETE. → svc.Projects.Locations.Clusters.Users.Patch(...) → 405 METHOD_NOT_ALLOWED. Add an http.MethodPatch case that calls UpdateUser.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c1cfafd. Added an http.MethodPatch case in the user item router → UpdateUser; Users.Patch(...) now returns 200. Covered by TestSDKAlloyDBUserPatchAndGuards.


// FailoverInstance triggers a failover of a REGIONAL primary instance. The
// instance stays available; the action just verifies it exists.
func (m *Mock) FailoverInstance(_ context.Context, clusterID, instanceID string) (*rdsdriver.Instance, error) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] FailoverInstance/RestartInstance have no PRIMARY/type guard. Both share instanceAction, which only verifies the instance exists and returns a clone — no instance-type check, no state transition. → FailoverInstance on a READ_POOL instance silently succeeds, whereas real AlloyDB instances.failover is valid only on the PRIMARY. The no-op is defensible, but the missing precondition means an invalid call is accepted.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c1cfafd. FailoverInstance now requires the target be a PRIMARY (FailedPrecondition otherwise); RestartInstance stays valid on any type. Covered by TestFailoverRequiresPrimary.

- One PRIMARY per cluster enforced (base + native create); SECONDARY instance
  requires a SECONDARY cluster; READ_POOL requires nodeCount > 0.
- Detach dangling SECONDARY links when their PRIMARY cluster is deleted (the
  replica-linkage class from stackshy#303).
- FailoverInstance requires a PRIMARY target; RestartInstance stays valid on
  any type.
- UpdateUser is now reachable via Users.Patch (was 405).
- Custom methods (:promote / :failover / :restart) are POST-only and unknown
  verbs 404 instead of misrouting to CRUD; GET on :promote no longer promotes.
- Deterministic list order: DescribeClusters/DescribeInstances use SortedValues.
- patchCluster/patchInstance handle the capability assertion; toWireUser returns
  the full resource path; removed banner comments.
- Wiring: guard New against AlloyDB+GKE both enabled (they share paths) and add
  DriversFromWithAlloyDB helper (enables AlloyDB in place of GKE).
- Tests for every guard + user PATCH + GET-:promote rejection + 400 wire error +
  instance output aliasing.
@thzgajendra

Copy link
Copy Markdown
Collaborator Author

Thanks for the deep lifecycle pass — all findings addressed in c1cfafd (per-comment replies inline). Beyond the 4 Mediums + the failover Low, I also closed the other Lows you flagged:

  • Unknown :verb mis-dispatch / GET-promotes-on-read — custom methods (:promote/:failover/:restart) are now POST-only; a non-POST is 405 and an unknown verb 404, so GET .../clusters/{c}:promote no longer promotes.
  • List determinismDescribeClusters/DescribeInstances now use SortedValues(), matching the snapshot/user/db paths.
  • Banner comments — removed all 8 // ---- X ---- blocks (project no-banner convention).
  • patchCluster/patchInstance ignored the capability ok — now return 501 UNIMPLEMENTED instead of risking a nil-panic.
  • toWireUser.Name — now the full resource path (projects/…/clusters/…/users/…), not the bare id.
  • Coverage — added tests for one-primary, secondary-in-secondary-cluster, READ_POOL nodeCount, failover-requires-primary, primary-delete-detaches-secondary, Users.Patch, GET-:promote rejection, the 400 wire path, and instance output-aliasing. Provider ~84%, server ~70%.

Deliberately left as-is (noted, not bugs):

  • operations GET returns bare done:true with no response — the mutating calls already return a terminal operation carrying the resource, so the poll is only a fallback; fine for SDK round-trips.
  • go.mod transitive bumpalloydb/v1 requires google.golang.org/api ≥ 0.291, which pulls the wider update; unavoidable for the new SDK, tidy-clean and green.

Gates: build ✓ vet ✓ test ✓ (full) golangci-lint ✓ (0 issues on changed pkgs).

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review comments addressed — approving ✅

Verified c1cfafd 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 AlloyDB files).

Medium — all fixed:

  • GKE-vs-AlloyDB routing footgunDriversFromWithAlloyDB helper that nils GKE, plus a fail-fast panic in the server wiring if both are enabled — the ambiguous state can no longer silently shadow GKE.
  • Dangling SECONDARY link on PRIMARY deletedetachSecondaries clears the primary link on every secondary before delete (the #303 replica class, now closed on the delete path too).
  • One-primary-per-clusterclusterHasPrimary guard rejects a second PRIMARY instance.
  • UpdateUser was unreachableUsers.Patch now routes to patchUserUpdateUser.

Low — fixed:

  • FailoverInstance now requires a PRIMARY; RestartInstance correctly stays unrestricted.
  • :promote/:failover/:restart now 405 on a non-POST and 404 on an unknown verb (a GET …:promote no longer triggers a state change).
  • DescribeClusters/DescribeInstances list-all now use SortedValues() (deterministic, consistent with the rest of the package).
  • Banner-comment blocks removed.

Full lifecycle (cluster → instances → users → databases → backup → restore → secondary → promote → failover → restart → delete) is implemented, wired (metrics/cost/discovery, no CloudSQL regression), and round-trips through the real alloydb/v1 SDK. Non-blocking follow-up: server/gcp/alloydb coverage is ~70% (consistent with the other GCP server handlers; broad wire-level fault-injection would lift it). LGTM.

@thzgajendra
thzgajendra merged commit 45616a0 into stackshy:development Jul 31, 2026
16 of 17 checks passed
@thzgajendra thzgajendra mentioned this pull request Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants