feat: allow granting rds_replication - #20
Conversation
Greptile SummaryThis PR introduces a PostgreSQL role-membership custom resource so constructs can grant predefined roles such as
Confidence Score: 3/5The PR should not merge until membership identities cannot collide and same-stack role creation is reliably ordered before the grant. Distinct valid memberships can be treated as the same resource and silently retain incorrect privileges, while missing construct dependencies can make deployments invoke GRANT before the referenced roles exist. Files Needing Attention: cdk-postgresql/lib/role-membership.handler.ts, cdk-postgresql/lib/role-membership.ts
|
| Filename | Overview |
|---|---|
| cdk-postgresql/lib/role-membership.handler.ts | Implements membership lifecycle operations, but ambiguous physical-ID construction can silently skip valid membership updates. |
| cdk-postgresql/lib/role-membership.ts | Defines the public construct, but same-stack roles supplied by name have no inferred deployment ordering. |
| cdk-postgresql/lib/postgres.ts | Adds correctly identifier-escaped GRANT and REVOKE helpers. |
| cdk-postgresql/lib/handler.ts | Correctly routes the new custom-resource type to its handler. |
| cdk-postgresql/lib/index.ts | Exports the new public RoleMembership API. |
Reviews (1): Last reviewed commit: "feat: allow granting rds_replication" | Re-trigger Greptile
There was a problem hiding this comment.
Pull request overview
Adds first-class support for PostgreSQL role membership grants/revokes via a new CDK Construct + Lambda custom resource, enabling use cases like granting rds_replication on RDS/Aurora where direct replication privileges can’t be granted.
Changes:
- Introduces
RoleMembershipConstruct andPredefinedRoleNameunion for common built-in roles. - Adds a new custom resource handler (
Custom::Postgresql-RoleMembership) and routes it from the shared Lambda entry handler. - Adds SQL helpers in
postgres.tsto GRANT/REVOKE role memberships.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cdk-postgresql/lib/role-membership.ts | New CDK Construct defining the RoleMembership custom resource and its typed props. |
| cdk-postgresql/lib/role-membership.handler.ts | New Lambda handler implementing Create/Update/Delete for role membership grants/revokes. |
| cdk-postgresql/lib/postgres.ts | Adds grantRoleMembership / revokeRoleMembership SQL helpers. |
| cdk-postgresql/lib/index.ts | Exports the new RoleMembership Construct from the library entrypoint. |
| cdk-postgresql/lib/handler.ts | Routes the new Custom::Postgresql-RoleMembership resource type to its handler. |
Suppressed comments (2)
cdk-postgresql/lib/postgres.ts:81
- revokeRoleMembership is not idempotent: if the membership was already removed (manual intervention/drift) the stack delete or replacement cleanup can fail. Consider treating "not a member" as success, while still throwing on unexpected errors.
await client.query(
`REVOKE ${escapeIdentifier(role)} FROM ${escapeIdentifier(member)}`
);
};
cdk-postgresql/lib/role-membership.handler.ts:109
- If postgres.revokeRoleMembership throws, the PG client connection will never be closed. Wrapping the query in a try/finally avoids leaking connections and reduces the risk of Lambda hangs/timeouts during stack deletion/replacement.
console.log(`Revoking ${role} from ${member}`);
const client = await getConnectedClient(connection);
await postgres.revokeRoleMembership({ client, role, member });
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e1d9ba6 to
e53f9b1
Compare
e53f9b1 to
35df407
Compare
| await client.query( | ||
| `REVOKE ${escapeIdentifier(role)} FROM ${escapeIdentifier(member)}` | ||
| ); |
There was a problem hiding this comment.
REVOKE is idempotent for a membership that isn't there, but it is not tolerant of a role that isn't there — that case is a hard error, so the Delete path can wedge a stack.
Verified against real containers (PG 16.14 and PG 13), quoted the same way escapeIdentifier quotes:
REVOKE "replicator" FROM "myuser" (not a member) -> WARNING: ... has not been granted ... exit=0
REVOKE "ghostrole" FROM "myuser" (no such role) -> ERROR: role "ghostrole" does not exist exit=1
REVOKE "replicator" FROM "ghostmember" -> ERROR: role "ghostmember" does not exist exit=1
So handleDelete fails in exactly the situation the new doc comment on RoleMembership describes — no CloudFormation dependency between a Role and its membership:
- Stack teardown. With no dependency between the two, CFN's deletion order between them is unspecified. If
DROP USERruns first, theREVOKEerrors and the resource lands inDELETE_FAILED— manual--skip-resourcesto get out. - Rollback of a grant that ran too early. Create fails with
role "..." does not exist, CFN rolls back with a Delete, and theREVOKEhits the same error. A recoverable deploy failure turns into a stuck rollback.
Suggest treating 42704 as non-fatal, mirroring the 0LP01 handling already in createDatabase above:
| await client.query( | |
| `REVOKE ${escapeIdentifier(role)} FROM ${escapeIdentifier(member)}` | |
| ); | |
| try { | |
| await client.query( | |
| `REVOKE ${escapeIdentifier(role)} FROM ${escapeIdentifier(member)}` | |
| ); | |
| } catch (e) { | |
| if (!util.types.isNativeError(e)) { | |
| throw e; | |
| } | |
| if (!isDatabaseError(e) || e.code !== "42704") { | |
| throw new VError(e, "unexpected error while revoking role membership"); | |
| } | |
| console.warn(e.message); | |
| } |
Nothing is needed on the GRANT side: a missing role there should fail the deploy, and re-granting an existing membership is already a NOTICE (exit=0) on both 13 and 16 — so the earlier idempotency comment on grantRoleMembership was a false positive.
There was a problem hiding this comment.
Thanks, it's now fixed
Contributes to KKN-927