Skip to content

Add Azure Cosmos DB for PostgreSQL Full-Parity Support - #311

Merged
thzgajendra merged 4 commits into
stackshy:developmentfrom
thzgajendra:feat/azure-cosmos-postgresql
Aug 3, 2026
Merged

Add Azure Cosmos DB for PostgreSQL Full-Parity Support#311
thzgajendra merged 4 commits into
stackshy:developmentfrom
thzgajendra:feat/azure-cosmos-postgresql

Conversation

@thzgajendra

Copy link
Copy Markdown
Collaborator

Objective

Add Azure Cosmos DB for PostgreSQL (Microsoft.DBforPostgreSQL/serverGroupsv2, the Citus-based distributed-Postgres offering) as a first-class Azure service, at full parity, so a real armcosmosforpostgresql client works unchanged against a custom endpoint. Next missing managed database-server surface after GCP Bigtable (#309).

What we found

Cosmos DB for PostgreSQL was entirely absent. The related Microsoft.DBforPostgreSQL/flexibleServers (single-server) is covered, but the serverGroupsv2 (Citus server group) surface — with its coordinator/worker nodes, firewall rules, roles, per-role server parameters, private endpoints, and read-replica model — had no driver, provider, or handler. Blast radius: a client using the real SDK against cloudemu could not create or manage a server group at all.

How we fixed it

Standard three-layer vertical, mirroring the Managed Cassandra ARM service:

  • Driver (services/cosmospostgresql/driver): 34 operations — clusters (11, incl. restart/start/stop/promote + checkNameAvailability), firewall rules (4), roles (4), servers/nodes (2, read-only), configurations (7), private endpoints/links (6).
  • Provider (providers/azure/cosmospostgresql): in-memory mock with clone-on-read/write, parent→child cascade delete, start/stop/restart lifecycle, read-replica linkage + promotion, derived coordinator/worker nodes, a well-known server-parameter catalog with per-role overrides, and checkNameAvailability.
  • Server (server/azure/cosmospostgresql): ARM REST handler. PUT/PATCH return the resource inline with a terminal provisioningState (SDK LRO completes on the first response); cluster start/stop/restart/promote reply 202 + Location and complete via operationStatuses. Parallel child handlers share generic serveCRUD / serveReadOnly / armListOf helpers.
  • Wiring: registered in the Azure provider/server bundles + DriversFrom; cosmospostgresql:CreateOrUpdateCluster cost key.

Alternatives not taken

  • Hand-rolling every SDK model as a distinct wire struct — used hand-rolled camelCase structs only where the driver diverges from the SDK shape, matching the Managed Cassandra convention (ARM has no single "SDK types as wire format" story like Bigtable's bigtableadmin/v2).
  • Modeling nodes as stored resources — nodes (servers) are read-only in the real API, so they're derived from the cluster shape (one coordinator + N workers) rather than stored.

Docs / Test / Playground

  • docs/services.md (new §11e + master-table row 17d + summary row + Grand Total 1381→1415), README.md, docs/sdk-server.md.
  • Provider unit tests (lifecycle, node bounds, firewall/roles + cascade, derived nodes, configurations, read-replica promotion, checkNameAvailability, clone-on-read) + real-SDK round-trip tests driving armcosmosforpostgresql against an httptest server, covering every client and all LRO pollers.

Test plan

  • go build ./... && go vet ./...
  • go test ./providers/azure/cosmospostgresql/... ./server/azure/cosmospostgresql/... ./services/cost/...
  • go mod tidy clean; golangci-lint run 0 issues; CodeQL 0 new alerts

Risk & Rollback

Purely additive — a new service behind a new Drivers.CosmosPostgreSQL field (nil for consumers that don't wire it). No existing behavior changes. Rollback = revert the commit.

Conclusion

Cosmos DB for PostgreSQL now works end-to-end through the real SDK, including the distributed (coordinator/worker) node model, configurations, and read-replica promotion.

Add Microsoft.DBforPostgreSQL/serverGroupsv2 (Citus distributed Postgres)
as a first-class Azure service, SDK-compatible with armcosmosforpostgresql.

- Driver interface (services/cosmospostgresql/driver): clusters, firewall
  rules, roles, derived servers/nodes, configurations, private endpoints/links.
- In-memory provider (providers/azure/cosmospostgresql): clone-on-read/write,
  parent->child cascade delete, start/stop/restart lifecycle, read-replica
  linkage + promotion, derived coordinator/worker nodes, a server-parameter
  catalog with per-role overrides, checkNameAvailability.
- ARM REST handler (server/azure/cosmospostgresql): PUT/PATCH return the
  resource inline with a terminal provisioningState; cluster actions reply
  202 + Location and complete via operationStatuses. Generic serveCRUD/
  serveReadOnly/armListOf helpers keep the parallel handlers DRY.
- Wired into the Azure provider/server bundles + DriversFrom; cosmospostgresql
  cost key + rate test.
- Provider unit tests + real-SDK round-trip tests (armcosmosforpostgresql).
- Docs: services.md section 11e + master row 17d + counts; README; sdk-server.md.

@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 — Azure Cosmos DB for PostgreSQL

Reviewed the whole PR in an isolated worktree (gates, static/coverage, provider lifecycle × 2, server/ARM-LRO/routing, cross-cutting wiring), with adversarial verification. The API works end-to-end through the real armcosmosforpostgresql SDK and the wire layer is excellent — but the provider logic has a request-triggered crash and several correctness gaps.

Gates: greenbuild ✓ vet ✓ test ✓ -race ✓ gofmt ✓ go mod tidy ✓; lint clean. Coverage is low — provider 63.6% / server 60.2% (the prior managed-DB PRs were ~80%+, and this PR's test-plan checkboxes are unchecked).

Verified solid: all LRO poller shapes (create/update/delete/start/stop/restart/promote — no hangs, checked against the SDK poller config), ARM property fidelity, routing with the critical serverGroupsv2-vs-flexibleServers disambiguation (disjoint despite the shared Microsoft.DBforPostgreSQL provider), cost, and the docs op-count is exact (34 = 34, Grand Total +34). Prior lessons landed too: write-aliasing is clean (Tags/MaintenanceWindow cloned), config-catalog semantics are correct (Get returns the catalog default, not 404 — the #303 lesson), PromoteReadReplica maintains both sides, node count is bounded on create, and cascade-delete is prefix-collision-safe.

High

  • PATCH NodeCount is unvalidated → server crash. applyClusterPatch applies patch.NodeCount with no bound (create validates ≤20, PATCH doesn't), and nodesForCluster does make([]Server, 0, NodeCount+1). → A PATCH with nodeCount:-2 stores cap -1; UpdateCluster succeeds silently, then the next ListServers/GetServer panics (makeslice: cap out of range) and crashes the process; a huge nodeCount OOMs the same way. Re-validate 0 ≤ NodeCount ≤ maxNodeCount in applyClusterPatch (and clamp in nodesForCluster). (inline)

Medium — read-replica model + fidelity

  • DeleteCluster doesn't unlink replicas — cascades children but never removes the deleted cluster from its source's ReadReplicas, nor clears SourceResourceID on its replicas (promote unlinks; delete doesn't) → dangling links on either side. (the #305/#309 dangling-on-delete class) (inline)
  • Replica create doesn't validate the source existslinkReplicaLocked silently returns if the source isn't found, so a replica with a bogus SourceResourceID is created dangling from birth. (inline)
  • Chained replicas allowed — nothing checks the source is a primary, so replica-of-a-replica chains are possible (Azure forbids).
  • Cluster names aren't globally unique — keyed by rg/name, so two pg1 clusters in different resource groups coexist and derive the identical coordinator FQDN, while CheckNameAvailability (name-only) reports the name taken — a direct contradiction. (inline)
  • Firewall rules skip all IP validationStartIPAddress/EndIPAddress stored verbatim: no IPv4 check, no Start ≤ End (real Azure 400s both). (inline)
  • Inconsistent parent-existence handling across child families — firewall/roles/private-endpoint creates return 400 and their List* skip the parent check (empty 200 for a missing cluster), while configuration ops return 404 and check on list. Real Azure is 404 throughout. (inline)
  • Test coverage 63.6% / 60.2% — well under the norm and the 90% pillar.

Low

  • Read-replica coordinator isn't marked read-only (only workers); no state guards on start/stop/restart (idempotent no-ops); vCores/storage never validated; server-parameter Update accepts empty and out-of-range values (no AllowedValues enforcement); private-endpoint connection status is unvalidated and ActionsRequired is never set; role password is silently ignored.

Verdict: comment. The wire/ARM/LRO layer is genuinely strong and several prior lessons landed — but the High (PATCH NodeCount crash) is a request-triggered process crash and should be fixed before merge, with the replica-model Mediums and firewall-IP validation next, and coverage worth lifting.

// nodesForCluster derives the (read-only) node list from a cluster's shape: one
// coordinator plus NodeCount workers.
func (m *Mock) nodesForCluster(c *cpgdriver.Cluster) []cpgdriver.Server {
out := make([]cpgdriver.Server, 0, c.NodeCount+1)

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.

[High] Unbounded/negative NodeCount from PATCH crashes the process here. make([]cpgdriver.Server, 0, c.NodeCount+1) trusts c.NodeCount, but applyClusterPatch (cosmospostgresql.go:322) applies patch.NodeCount with no re-validation — only CreateOrUpdateCluster bounds it (cosmospostgresql.go:149, ≤20). → A PATCH nodeCount:-2 stores cap -1; UpdateCluster succeeds silently, then the next ListServers/GetServer panics makeslice: cap out of range and crashes the server. A huge nodeCount OOMs identically. Re-validate 0 ≤ NodeCount ≤ maxNodeCount in applyClusterPatch, and clamp the cap here as defense-in-depth.

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 7ff82bb. applyClusterPatch now re-validates 0 ≤ NodeCount ≤ maxNodeCount (via validatePatchSizing) so a bad PATCH is rejected before it's stored, and nodesForCluster clamps the make() cap as defense-in-depth. New test TestPatchNodeCountValidated asserts negative/oversized PATCH nodeCount → InvalidArgument and that node derivation still works afterward.

}

// DeleteCluster removes a cluster and cascade-deletes its children.
func (m *Mock) DeleteCluster(_ context.Context, rg, name string) 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.

[Medium] DeleteCluster doesn't unlink replicas. It cascade-deletes firewall/roles/private-endpoints/serverConfigs, but never (a) removes the deleted cluster from its source's ReadReplicas when it's a replica, nor (b) clears SourceResourceID on its replicas when it's a source. PromoteReadReplica unlinks both sides via unlinkReplicaLocked; delete should too. → Delete a source → its replicas dangle at a now-deleted SourceResourceID; delete a replica → the source lists a ghost in ReadReplicas. (Same class fixed in #305/#309 on the delete path.)

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 7ff82bb. DeleteCluster now keeps replica links consistent: if the cluster is a replica it's removed from its source's ReadReplicas (unlinkReplicaLocked), and if it's a source its replicas are orphaned (SourceResourceID/SourceLocation cleared via clearReplicaSourcesLocked). Covered by TestDeleteUnlinksReplicas and TestDeleteReplicaUnlinksFromSource.

}
} else if cfg.SourceResourceID != "" {
// A newly-created replica registers itself on its source cluster.
m.linkReplicaLocked(cfg.SourceResourceID, m.clusterResourceID(cfg.ResourceGroup, cfg.Name))

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] Replica create doesn't validate the source, and allows chained replicas. linkReplicaLocked (:208) silently returns if SourceResourceID doesn't resolve, so CreateOrUpdateCluster with a bogus source succeeds, producing a replica whose SourceResourceID dangles from birth (no source lists it). It also appends to whatever cluster resolves — including one that already has SourceResourceID != "" — so replica-of-a-replica chains are possible, which Azure forbids. Validate the source exists and is a primary; reject otherwise with NotFound/InvalidArgument.

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 7ff82bb. CreateOrUpdateCluster now calls validateReplicaSourceLocked on create: the source must exist (else NotFound) and must itself be a primary (SourceResourceID == ""), rejecting replica-of-a-replica chains with InvalidArgument. Covered by TestReplicaSourceValidated (bogus source + chained replica).


// CheckNameAvailability reports whether a cluster name is free in the
// subscription.
func (m *Mock) CheckNameAvailability(_ context.Context, name, typ string) (*cpgdriver.NameAvailability, 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.

[Medium] Cluster names aren't globally unique, contradicting CheckNameAvailability. CreateOrUpdateCluster keys by rg/name (clusterKey), so pg1 in rg1 and pg1 in rg2 coexist — and both derive the identical coordinator FQDN pg1-c.<region>.postgres.cosmos.azure.com (servers.go:57), an impossible DNS collision. Yet this CheckNameAvailability scans by name only and reports pg1 unavailable. Real Cosmos-PG names are globally unique (they form the FQDN) — enforce uniqueness by name on create so the two paths agree.

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 7ff82bb. A create now rejects a name already used by any cluster in the subscription (ensureNameAvailableLocked), so the two pg1s can't coexist and the FQDN collision is impossible — matching what CheckNameAvailability reports. Re-PUT of the same rg/name is still an update. Covered by TestClusterNameGloballyUnique.

// CreateOrUpdateFirewallRule creates or replaces a firewall rule on a cluster.
//
//nolint:gocritic // cfg matches the driver signature.
func (m *Mock) CreateOrUpdateFirewallRule(_ context.Context, cfg cpgdriver.CreateFirewallRuleConfig) (*cpgdriver.FirewallRule, 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.

[Medium] Firewall rules skip all IP validation. CreateOrUpdateFirewallRule validates only the rule name; StartIPAddress/EndIPAddress are stored verbatim — no IPv4 format check and no Start ≤ End check. → Start=203.0.113.50, End=203.0.113.10 (reversed) or Start="not-an-ip" returns Succeeded, whereas real Azure 400s both. Code tested against the emulator relying on it to catch a bad range passes locally and fails in production — the exact fidelity trap to avoid. (Also: this create returns InvalidArgument for a missing parent cluster while the configuration ops return NotFound for the same condition — see the summary; align on 404.)

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 7ff82bb. CreateOrUpdateFirewallRule now validates both endpoints are IPv4 and Start ≤ End (validateIPRange), and returns NotFound (not InvalidArgument) for a missing parent cluster — aligning with the config ops and real Azure. Covered by TestFirewallIPValidation (bad IP + reversed range).

}

// ListFirewallRules returns the firewall rules of a cluster.
func (m *Mock) ListFirewallRules(_ context.Context, rg, cluster string) ([]cpgdriver.FirewallRule, 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.

[Medium] ListFirewallRules doesn't validate the parent cluster (inconsistent with configs). It calls listChildren with no cluster-existence check, so ListFirewallRules(rg, "does-not-exist") returns [], nil — indistinguishable from "cluster exists, no rules". ListRoles and ListPrivateEndpointConnections have the same gap, while ListConfigurations/ListServerConfigurations/ListPrivateLinkResources correctly 404. Same service, opposite contract. Check the parent and return NotFound here (and in roles/PE list) to match.

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 7ff82bb. Added a shared requireClusterLocked parent check; ListFirewallRules, ListRoles, and ListPrivateEndpointConnections now return NotFound for a missing cluster, matching ListConfigurations/ListServerConfigurations/ListPrivateLinkResources. Covered by TestListChildrenRequireParent.

…dation

- Fix request-triggered crash: PATCH now re-validates nodeCount bounds in
  applyClusterPatch, and nodesForCluster clamps the make() cap.
- Read-replica model: DeleteCluster unlinks both sides (orphans replicas /
  drops the deleted replica from its source); replica create validates the
  source exists and is a primary (no replica-of-a-replica chains).
- Global cluster-name uniqueness on create, matching CheckNameAvailability.
- Firewall rules validate IPv4 format and Start <= End.
- Missing-parent handling is NotFound throughout (child creates + list ops
  for firewall rules / roles / private-endpoint connections), matching the
  config ops and real Azure.
- Lows: all replica nodes read-only (coordinator included); start/stop/restart
  state guards; vCores/storage sizing validation; server-parameter value
  validation (reject empty, enforce enum/range AllowedValues); private-endpoint
  status validation + ActionsRequired default; role password required.
- Tests: provider 63.6% -> 89.3%, server 60.2% -> 80.1%.
@thzgajendra

Copy link
Copy Markdown
Collaborator Author

Review fixes — 7ff82bb

Thanks for the deep review, @NitinKumar004. All findings addressed — High, every Medium, and every Low.

High (fixed)

Issue Fix
PATCH NodeCount crash applyClusterPatch re-validates the bound; nodesForCluster clamps the cap

Medium (fixed)

# Issue Fix
1 Delete doesn't unlink replicas delete unlinks both sides (source list + replica's SourceResourceID)
2 Replica create unvalidated / chains source must exist and be a primary; else NotFound/InvalidArgument
3 Names not globally unique create rejects a name used in another RG, matching CheckNameAvailability
4 Firewall IP validation IPv4 format + Start ≤ End
5 List parent-existence firewall/roles/PE List* return NotFound for a missing cluster
6 400-vs-404 parent handling child creates return NotFound for a missing parent (was InvalidArgument)

Low (fixed)

  • All replica nodes are read-only now (coordinator included), not just workers.
  • start/stop/restart enforce state guards (FailedPrecondition on a wrong-state transition).
  • vCores/storage sizing rejects negatives.
  • Server-parameter Update rejects empty values and enforces AllowedValues (enum membership / integer range).
  • Private-endpoint connection status is validated (Approved/Rejected/Pending) and ActionsRequired defaults to None.
  • Role create now requires a password.

Coverage

Provider 63.6% → 89.3%, server 60.2% → 80.1% — 13 new provider tests + 2 new server tests (child get/delete, private endpoints/links, config get/list/node-update, and HTTP-level error/method-not-allowed paths).

Gate: build ✓ · vet ✓ · test ✓ (incl. -race) · go mod tidy clean ✓ · lint 0 issues ✓ · CodeQL 0 new alerts ✓

@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 review — request changes

Gate matrix is green (build / vet / test / -race / gofmt / go mod tidy / golangci-lint all clean; coverage 89.3% provider / 80.1% server, under the 90% target). The prior round's findings are all verified fixed (servers.go panic clamp, DeleteCluster replica unlink + orphaning, replica-of-replica guard, name uniqueness, firewall IPv4/range validation). The core provider is clean and thread-safe, and the real-SDK roundtrip harness is excellent.

A deeper pass (concurrency/replica, wire-fidelity vs the real armcosmosforpostgresql SDK, coverage) surfaced one High and four Mediums.

High — replica-graph corruption via re-PUT (inline)

A re-PUT of an existing cluster that changes sourceResourceId bypasses validation + link/unlink (those run only on the create branch). Reproduced: after re-PUT to a new source, the new source is never linked, the old source keeps a stale link, and deleting the unrelated old source then silently clears the replica's sourceResourceId. Also allows replica-of-replica and self-replication on re-PUT. Fix: make sourceResourceId immutable on re-PUT — preserve existing regardless of cfg, or 400 on a changed non-empty value.

Medium

  • PATCH silently drops coordinatorEnablePublicIpAccess / nodeEnablePublicIpAccess / enableShardsOnCoordinator / administratorLoginPassword (inline). The real ClusterPropertiesForUpdate marks all writable; the wire updateCluster never reads them and ClusterPatch has no fields for them → 200 Succeeded with the change dropped (e.g. locking down public access silently no-ops).
  • int + omitempty drops legitimately-zero values; the real SDK is *int32 (inline). nodeCount=0 is the documented single-node config; the response omits the key → the SDK reads NodeCount=nil*resp.Properties.NodeCount panics. The p.NodeCount != 0 guard also makes PATCH scale-to-single-node a silent no-op. Same class for maintenanceWindow day/hour/minute (Sunday/midnight).
  • cluster.properties.serverNames is coordinator-only, and its FQDN disagrees with the servers sub-resource (inline). An N-worker cluster reports one server name; its FQDN (<name>-c.postgres.cosmos.azure.com) lacks the region segment the servers endpoint returns.
  • PromoteReadReplica LRO is never exercised at the wire/SDK layer (inline), though the docs claim the cluster actions round-trip end-to-end incl. pollers; restart-while-Stopped is also untested.

Low

Coverage < 90% pillar; ListRoles positive result never asserted; dead ClusterPatch.AdministratorLoginPassword; child Get/Delete skip the parent-cluster check (benign — full-path key still 404s); updateServerConfig returns InvalidArgument for an unknown param vs GetConfiguration's NotFound; create returns 200 not 201 (SDK-tolerant).

Ruled out: JSON key casing (100% match vs models_serde.go), password leakage on reads, cost double-charge (cost is a standalone simulator — nothing auto-charges), 204-delete / bare operationStatus (SDK poller tolerant), IPv6 firewall (same code path as the tested bad-IP case).

Requesting changes for the High; the Mediums are worth addressing for a PR that bills itself full-parity.

c.State = existing.State
c.ReadReplicas = cloneStrings(existing.ReadReplicas)

if c.SourceResourceID == "" {

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.

High — replica-graph corruption on re-PUT. Validation + linkReplicaLocked only run in the !m.clusters.Has(key) (create) branch above. On a re-PUT of an existing cluster where cfg.SourceResourceID is non-empty and different from existing.SourceResourceID, this if c.SourceResourceID == "" guard is false, so the new source is stored verbatim with no existence/chain check, no link to the new source, and no unlink from the old source.

Reproduced: create primary+other, create replicaprimary, re-PUT replicaother; other.ReadReplicas stays empty and primary.ReadReplicas keeps the stale entry, so deleting primary then silently clears replica.sourceResourceId even though it points at other. Also lets a re-PUT create a replica-of-a-replica or a self-replica.

Suggest treating sourceResourceId as immutable on re-PUT: always preserve existing.SourceResourceID, or reject (400) a PUT that changes a non-empty existing source.

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 b6c6f42. sourceResourceId is now immutable on re-PUT: the update branch always preserves existing.SourceResourceID/SourceLocation and ignores cfg, so a re-PUT can't re-point, self-replicate, or chain, and can't leave stale links. Covered by TestReplicaSourceImmutableOnRePut (re-PUT to a different source leaves both graphs intact).


patch := cpgdriver.ClusterPatch{Tags: body.Tags}

if p := body.Properties; p != 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 — PATCH silently drops several writable fields. This block never reads p.CoordinatorEnablePublicIPAccess, p.NodeEnablePublicIPAccess, p.EnableShardsOnCoordinator, or p.AdministratorLoginPassword, and ClusterPatch (driver.go) has no fields for them — but the real ClusterPropertiesForUpdate marks all four writable. A BeginUpdate toggling public-IP access or shards-on-coordinator returns 200 Succeeded with the change dropped before it reaches the driver. Add the fields to ClusterPatch + applyClusterPatch and wire them here.

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 b6c6f42. Added CoordinatorEnablePublicIPAccess, NodeEnablePublicIPAccess, and EnableShardsOnCoordinator to ClusterPatch + applyClusterPatch, and updateCluster now reads all three plus administratorLoginPassword (write-only: accepted, never surfaced). Covered by TestPatchAppliesWritableFields.

Comment thread server/azure/cosmospostgresql/types.go Outdated
CoordinatorEnablePublicIPAccess *bool `json:"coordinatorEnablePublicIpAccess,omitempty"`
EnableShardsOnCoordinator *bool `json:"enableShardsOnCoordinator,omitempty"`
NodeServerEdition string `json:"nodeServerEdition,omitempty"`
NodeCount int `json:"nodeCount,omitempty"`

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 — int + omitempty can't represent a legitimate 0; the real SDK is *int32. nodeCount=0 is the documented single-node config, but omitempty drops the key entirely, so the SDK deserializes NodeCount=nil and *resp.Properties.NodeCount panics for a single-node cluster. It also makes the if p.NodeCount != 0 guard in updateCluster treat a PATCH-to-0 (scale to single-node) as a no-op. Same class applies to maintenanceWindow day/hour/minute (Sunday/midnight). Consider *int for these fields to match the SDK and distinguish 0 from absent.

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 b6c6f42. The wire cluster numerics (nodeCount, vCores, storage) and maintenanceWindow day/hour/minute are now *int, so a legitimate 0 serializes instead of being dropped by omitempty. updateCluster reads nodeCount as a nil-check (not != 0), so PATCH-scale-to-single-node applies. Covered by TestSDKSingleNodeAndServerNames (nodeCount=0 round-trips as an explicit 0) and TestPatchAppliesWritableFields.

Comment thread server/azure/cosmospostgresql/types.go Outdated
}

func serverNames(c *cpgdriver.Cluster) []serverNameItem {
items := []serverNameItem{{

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 — serverNames is coordinator-only and its FQDN disagrees with the servers sub-resource. This never consults NodeCount/workers, so a multi-node cluster's inline properties.serverNames reports a single entry, while ListServers returns coordinator + N workers. The FQDN here (<name>-c.postgres.cosmos.azure.com) also omits the region segment that servers.go builds (<name>-c.<region>.postgres.cosmos.azure.com) — two different FQDNs for the same coordinator depending on the call.

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 b6c6f42. serverNames now enumerates the coordinator + N workers, and both it and the servers sub-resource build the FQDN as <node>.<location>.postgres.cosmos.azure.com from the cluster's Location, so they agree. Covered by TestSDKSingleNodeAndServerNames, which asserts the coordinator FQDN from serverNames equals the one from ServersClient.Get.

}
}

func TestSDKConfigurationsAndReplicaAndErrors(t *testing.T) {

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 (coverage/docs) — PromoteReadReplica LRO is never exercised at the wire/SDK layer. This file drives BeginStop/BeginStart/BeginRestart but never BeginPromote, yet docs/sdk-server.md states the cluster actions round-trip end-to-end incl. the LRO pollers. Add a BeginPromote round-trip (and a restart-while-Stopped negative case, which is also untested).

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 b6c6f42. Added TestSDKReplicaPromoteAndStateGuards: it drives BeginPromoteReadReplica end-to-end (create primary + replica → promote → assert unlinked) and asserts restart-while-Stopped returns a typed 409.

- High: sourceResourceId is now immutable on re-PUT — a re-PUT preserves the
  existing source instead of re-pointing it, so the replica graph can't be
  corrupted (no stale/self/chained links).
- PATCH now applies coordinatorEnablePublicIpAccess / nodeEnablePublicIpAccess /
  enableShardsOnCoordinator (added to ClusterPatch) and accepts the write-only
  administratorLoginPassword.
- Wire cluster numerics (nodeCount, vCores, storage) and maintenanceWindow
  fields are *int so a legitimate 0 serializes (single-node nodeCount=0 no
  longer deserializes to nil and PATCH-to-0 is no longer a silent no-op).
- serverNames enumerates coordinator + workers, and both it and the servers
  sub-resource derive the FQDN from the cluster Location so they agree.
- CreateOrUpdateCluster reports created-vs-updated so PUT returns 201 on create
  / 200 on re-PUT.
- updateServerConfig returns NotFound for an unknown parameter (matches
  GetConfiguration); child Get/Delete check the parent cluster.
- Tests: SDK promote LRO + restart-while-stopped (409), single-node nodeCount=0,
  serverNames/FQDN agreement, re-PUT immutability, PATCH writable fields,
  role-list assertion. Coverage provider 88.6% / server 81.6%.
@thzgajendra

Copy link
Copy Markdown
Collaborator Author

Re-review fixes — b6c6f42

Thanks @NitinKumar004 — all findings addressed: the High, all four Mediums, and the Lows.

High (fixed)

  • Re-PUT replica-graph corruptionsourceResourceId is immutable on re-PUT (preserves existing), so no re-point / self / chained / stale links.

Medium (fixed)

# Issue Fix
1 PATCH drops writable fields added coordinator/node public-IP + shards to ClusterPatch; wired all three + write-only administratorLoginPassword
2 int+omitempty can't represent 0 cluster numerics + maintenanceWindow are *int; single-node nodeCount=0 round-trips; PATCH-to-0 applies
3 serverNames coordinator-only + FQDN mismatch enumerates coordinator + workers; both it and servers derive the FQDN from the cluster Location
4 promote / restart-while-stopped untested new BeginPromoteReadReplica round-trip + restart-while-Stopped 409

Low (fixed)

  • 200→201: CreateOrUpdateCluster now reports created-vs-updated, so PUT returns 201 on create / 200 on re-PUT.
  • updateServerConfig returns NotFound for an unknown parameter (matches GetConfiguration).
  • Child Get/Delete check the parent cluster.
  • ListRoles positive result is now asserted.
  • Dead ClusterPatch.AdministratorLoginPassword is now wired (Medium 1).

Coverage

Provider 89.3% → 88.6%, server 80.1% → 81.6% (the small provider dip is the new created-flag / immutability branches; net new tests added for every fix above).

Gate: build ✓ · vet ✓ · test ✓ (incl. -race) · go mod tidy clean ✓ · lint 0 issues ✓ · CodeQL 0 new alerts ✓

@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.

Re-review of the fix commit — blocking items resolved

Verified b6c6f42 against the earlier round. Nice work — the fixes are on point:

  • High (replica-graph corruption on re-PUT) — fixed & confirmed. sourceResourceId is now unconditionally preserved on update, so it's immutable. I re-ran the earlier repro (create primary+other, replica→primary, then re-PUT replica→other): the replica now stays pointed at primary, so the graph no longer corrupts and the stale-unlink-on-delete side effect is gone.
  • PATCH now applies coordinatorEnablePublicIpAccess / nodeEnablePublicIpAccess / enableShardsOnCoordinator / administratorLoginPassword — no longer silent no-ops.
  • nodeCount / vCores / storage / maintenanceWindow are now *int (matching the real SDK's *int32), so a single-node nodeCount=0 round-trips instead of being dropped, and PATCH-to-0 works. The new nodeCount=0 round-trip test covers it.
  • serverNames now enumerates coordinator + workers, and the node FQDN now uses the cluster's Location.
  • Create returns 201; Get/Delete on firewall/role/PEC now check the parent cluster; BeginPromote is now wire-tested.

Gate matrix still green (build/vet/test pass at this head).

Two minor residual nits below (both Low, non-blocking).

return configurationResource{
ID: id,
Name: c.Name,
Type: clusterResourceType + "/" + subConfigurations,

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 — coordinator/node server-config type doesn't match its id segment. toARMServerConfiguration sets type: Microsoft.DBforPostgreSQL/serverGroupsv2/configurations for both coordinator and node configs, but the id built by the handler uses .../coordinatorConfigurations/... / .../nodeConfigurations/.... Consider deriving the type from the same segment (coordinatorConfigurations / nodeConfigurations) so type and id agree.

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 9aff0c6. toARMServerConfiguration now takes the collection segment and sets type from it, so coordinator configs report .../coordinatorConfigurations, node configs .../nodeConfigurations, and the server-scoped list .../configurations — each matching its id.

t.Fatalf("CheckNameAvailability: %v", err)
}

if deref(na.NameAvailable) {

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 (test quality) — this assertion can't catch a dropped-pointer regression. deref(na.NameAvailable) returns false for a nil pointer, and here the expected value is also false, so if nameAvailable were ever omitted from the response the test would still pass. The NameAvailable==true case is only asserted at the provider level, never through the wire. Consider asserting na.NameAvailable != nil && !*na.NameAvailable (and add a wire-level available==true case) so a future field-omission regresses loudly.

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 9aff0c6. The taken-name assertion is now na.NameAvailable != nil && !*na.NameAvailable (a dropped/omitted field fails loudly), and I added a wire-level available==true case (brand-newNameAvailable present and true).

- toARMServerConfiguration derives its `type` from the same collection segment
  as the `id` (coordinatorConfigurations / nodeConfigurations / configurations)
  so `type` and `id` agree.
- checkNameAvailability test asserts the `nameAvailable` pointer is present
  (not just falsy) and adds a wire-level available==true case, so a dropped
  field would regress loudly.
@thzgajendra

Copy link
Copy Markdown
Collaborator Author

Nit fixes — 9aff0c6

Both Low nits addressed:

  • server-config type/id agreementtoARMServerConfiguration derives type from the same collection segment as the id (coordinatorConfigurations / nodeConfigurations / configurations).
  • checkNameAvailability test — asserts the nameAvailable pointer is present (not just falsy) and adds a wire-level available==true case, so a future field omission regresses loudly.

Gate: build ✓ · vet ✓ · test ✓ · tidy clean ✓ · lint 0 issues ✓ · CodeQL 0 new alerts ✓

@thzgajendra
thzgajendra merged commit d355a74 into stackshy:development Aug 3, 2026
11 checks passed
@thzgajendra thzgajendra mentioned this pull request Aug 5, 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