Skip to content

feat: API Gateway transport + dynamic multi-cluster MCP - #57

Open
kiranmam wants to merge 1 commit into
aws:mainfrom
kiranmam:feat/mcp-dynamic-apigateway
Open

feat: API Gateway transport + dynamic multi-cluster MCP#57
kiranmam wants to merge 1 commit into
aws:mainfrom
kiranmam:feat/mcp-dynamic-apigateway

Conversation

@kiranmam

Copy link
Copy Markdown
Contributor

Transport: Function URL → API Gateway (execute-api SigV4). Data-plane tools now accept cluster_identifier and auto-discover engine + credentials from MasterUserSecret. Added includeResultMetadata + label fallback for Aurora MySQL column names. Pinned mcp==1.29.0, mcp-proxy==0.12.0, fastmcp==3.4.7. Verified end-to-end against live Aurora MySQL and PostgreSQL clusters through DevOps Agent.

@chetanandikanti

chetanandikanti commented Aug 17, 2026

Copy link
Copy Markdown

Adding myself as a reviewer on this PR. @kiranmam

@ams-thakkar

Copy link
Copy Markdown
Contributor

Nice change overall — auto-discovering engine and credentials from MasterUserSecret removes real per-cluster config burden, and pinning the MCP dependencies is a solid improvement over the previous open ranges.

Below, Part 1 is things needing action from you, Part 2 is verification results with nothing to do — included so you know what the review did and didn't cover.


Part 1 — Action needed

# Action Blocking?
1 Restore rds:DescribeDBProxies + rds:DescribeDBProxyTargets, or remove get_proxy_health and its docs Yes — regression
2 Set Timeout: 29, or raise TimeoutInMillis No, but it's a real bug
3 Decide how ApiGatewayAccount should behave, and document it No — your call
4 Consider blocking AllowedClusters: '*' when StageName: prod No — your call
5 Optional: reconcile the unused StageName parameter No
6 Optional: two formatting nits No

1. get_proxy_health loses the permissions it needs — blocking

Action: either restore the two IAM actions, or delete the tool along with its README row, its docstring mention, and the "RDS Proxy health" claim in the mcp instructions string.

The template narrows the RDS describe statement:

- Action: [rds:DescribeDBClusters, rds:DescribeDBInstances, rds:DescribeDBEngineVersions, rds:DescribeDBProxies, rds:DescribeDBProxyTargets]
+ Action: [rds:DescribeDBClusters, rds:DescribeDBInstances, rds:DescribeDBEngineVersions]

The tool is still live and still calls both removed actions:

  • src/server.py:646@mcp.tool() on get_proxy_health, so it's exposed to the agent
  • src/server.py:658rds_client.describe_db_proxies(DBProxyName=proxy_name)
  • src/server.py:671rds_client.describe_db_proxy_targets(DBProxyName=proxy_name)
  • README.md:17 — lists it in the tools table
  • Module docstring and mcp instructions both still advertise "RDS Proxy health"

main grants these today, so post-merge the tool returns AccessDenied while staying documented and advertised. Removing it is a perfectly fine resolution if proxy support is being dropped — I'm only flagging that the current state is inconsistent either way.

2. Lambda timeout exceeds the API Gateway integration timeout

Action: set Timeout: 29 to match, or set TimeoutInMillis higher (needs a quota increase).

Timeout moved 180 → 60, but a REST API integration times out at 29s by default. A slow query gets a 504 from API Gateway at 29s while the Lambda runs on to 60s and keeps billing.

3. ApiGatewayAccount changes account-wide config

Action: your call — either add a prominent README warning, or default to not managing the account setting.

AWS::ApiGateway::Account sets the CloudWatch role for every API Gateway API in the account, not just this one. Deploying this stack overwrites whatever the account already had, and a stack delete can reset it for unrelated APIs. The ExistingApiGatewayCloudWatchRoleArn parameter shows this was on your radar — the gap is that the default path mutates account-level config silently, which matters for a sample people drop into existing accounts.

4. The access boundary moved from IAM into application code

Action: your call — consider a condition that fails the deploy when StageName: prod and AllowedClusters is '*'.

Data access widened from a single cluster ARN to cluster:*, and from one secret to all rds!cluster-*:

- Resource: !Ref ClusterArn
+ Resource: !Sub 'arn:aws:rds:${AWS::Region}:${AWS::AccountId}:cluster:*'

That's inherent to "deploy once, diagnose any allowlisted cluster", and scoping the secret pattern to rds!cluster-* instead of secret:* is a good call. The part worth a second thought: the real restriction is now ALLOWED_CLUSTERS, enforced in Python, and it defaults to '*'. The parameter description says prod requires an explicit list, but nothing enforces it — so a default deploy gets account-wide Aurora data access plus every managed cluster secret. A condition would turn that guidance into a guarantee.

5 and 6 — optional

  • StageName (dev/staging/prod) no longer controls the API stage; the API hardcodes StageName: Prod and the output URL is always /Prod/. The parameter now only feeds an env var, which reads as leftover.
  • sql=sql,includeResultMetadata=True, is missing a space after the comma, and the columns = [...] continuation line below it is misaligned.

Part 2 — Verified, no action needed

Recording these so it's clear what was checked, and so a future reader doesn't have to re-derive them:

  • No SQL injection path. Queries come only from the fixed MYSQL_QUERIES / PG_QUERIES dicts, cluster_identifier passes through validate_cluster before any use and never reaches SQL, and validate_database still runs inside _execute_sql. Worth stating explicitly on a PR that introduces a user-supplied cluster_identifier to data-plane tools.
  • The secret_arn override is IAM-backstopped. A caller can't reach a secret the execution role couldn't already read, and the error text says so.
  • All three dependency pins exist and are installable — checked against PyPI: mcp 1.29.0, mcp-proxy 0.12.0, fastmcp 3.4.7. Holding mcp at 1.29.0 while 2.0.0 is current looks deliberate rather than accidental.
  • includeResultMetadata plus the label-then-name fallback is the right fix for Aurora MySQL column naming.
  • run.sh dropping the explicit cd /var/task is fine — that's already Lambda's working directory.

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.

3 participants