Repository navigation
feat(mcp): EMR Spark Troubleshooting MCP integration for AWS DevOps Agent - #103
Conversation
There was a problem hiding this comment.
What needs to change
1. The PrivateLink claim doesn't match what the template does. Your own comment at integration-bring-your-own-vpc.yaml:449 and integration-with-new-vpc.yaml:550 says SigV4 services don't accept PrivateConnectionName and that DevOps Agent reaches SMUS over AWS-internal routing. I agree — ServiceDetails.MCPServerSigV4 takes only Endpoint, Region, Service, McpRoleArn, so the DependsOn is pure ordering. So the VPC endpoint, its SG, and the PrivateConnection are off the data path, and in the new-VPC template so are the VPC, both subnets, and the AZ-discovery Lambda — there's no IGW, NAT, or route table, so that VPC only hosts an endpoint nothing traverses. That's ~$15/month of interface endpoint across two AZs, plus ENIs, for an unused path.
The README claims it more strongly than the PR body: "registers the MCP endpoint as a capability provider bound to the private connection above." Same claim in the README opening paragraph, the CHANGELOG, and the PR title — and that opening paragraph is lifted verbatim into mcp-data.json by docs/hooks/mcp_catalog.py, so it becomes the public one-liner on the docs site. The troubleshooting bullet telling operators to dig the hostname from inside the VPC points at the wrong thing for the same reason. Either drop the VPC-side resources from the DevOps Agent path, or keep them and relabel consistently as retained for local Kiro CLI and future non-SigV4 services. I'd lean toward keeping them if you want one template serving both, but I don't want us advertising in-VPC traffic that doesn't happen — that line tends to end up in a compliance narrative.
2. CallPrivilegedTool is granted unconditionally in MCPInvokeAccess, next to CallReadOnlyTool as its explicit counterpart, while every other grant on this role is read-only. Resource: '*' is unavoidable since these actions have no ARN form, so the action list is the only control. Which tools does it gate, and can any mutate EMR or Glue state? If it isn't needed for all flows, put it behind a parameter defaulting to false, matching the existing EnableEMREC2 / EnableGlue pattern. This one needs EMR/Spark SME sign-off — the blast radius isn't visible from the templates and I can't assess it from here.
Nits
None of these are blocking.
- Version
0.1.0; our guidance starts new work at1.0.0, and the repo's only other MCP CHANGELOG is at1.1.0. VpcCidrForIngressdefaults to10.0.0.0/8, wider than any real VPC — exposure is VPC-local either way, but it undercuts the README's "restricts ingress to your VPC CIDR."- Partition hygiene is inconsistent within each file: trust policy uses
${AWS::Partition}, the S3/logs/managed-policy ARNs hardcodearn:aws:. Moot given theRulesblock.
Thanks
One of the better-documented contributions I've reviewed — specific enough that I could verify most of it mechanically, with every cfn-lint and cfn-guard suppression carrying a written reason. The AZ-discovery Lambda is careful work: rejects a non-HTTPS ResponseURL, sets reserved concurrency, handles Delete, reports FAILED instead of hanging the stack. Scoping S3 tighter than the canonical template is a real improvement, not a cosmetic one.
Verified myself against b75ce7e: cfn-lint clean on both; Rules present in both with exactly the 11 documented regions; S3 ARN construction correct (bucket ARNs for ListBucket, /* for GetObject); trust policy carries both aws:SourceAccount and aws:SourceArn guards; SG ingress 443 from the VPC CIDR only, not 0.0.0.0/0; iam:PassRole carries iam:PassedToService: glue.amazonaws.com in both copies, so I'm not flagging it; logs scoped to /aws/emr-serverless/* and /aws-glue/*; no hardcoded account IDs or secrets; strict docs build passes because mcp_catalog.py rewrites the relative links. No llms.txt entry needed — that file indexes skills, not MCP integrations.
No privilege-escalation path found. Happy to re-review once those two land.
b75ce7e to
633b9a9
Compare
|
Thank you for the thorough review — every concern was on the mark, and the investigation your comment triggered went further than I expected. Pushed a rewrite (commit What changed1. Stripped the VPC endpoint, security group, PrivateConnection, and (in the new-VPC template) the VPC + subnets + AZ discovery LambdaDigging into your first point turned up something I didn't know when I opened this PR: the DevOps Agent API explicitly refuses
Combined with a
The console/API inconsistency is now filed as its own internal ticket for the DevOps Agent service team. Given that, "relabel and keep for future non-SigV4 flows" felt too generous — the resources add ~$15/month of interface endpoint and complicate delete ordering for no benefit on the SigV4 path. Ripped them out. The template collapses to a single file: 2.
|
633b9a9 to
5987395
Compare
ams-thakkar
left a comment
There was a problem hiding this comment.
Re-reviewed at 5987395. Item 1 from my last pass is fully resolved — see below. I'm no longer holding on item 2; it's moved to a non-blocking note with what I found. Two smaller things need fixing, one of which I got wrong last time.
What needs to change
1. Add the llms.txt entry — and I was wrong last time. I told you "no llms.txt entry needed — that file indexes skills, not MCP integrations." That is incorrect. llms.txt has an ## Available MCP Servers section, and it already listed all five MCP servers at this branch's merge-base (3a5dd7f), so this isn't drift from rebasing. Adding a sixth server without an entry leaves the index incomplete. One line, same format as the others. Sorry for sending you the wrong way on that.
2. The PR body still describes the deleted design. It says the integration works "over AWS PrivateLink", the What deploys table still lists the Interface VPC Endpoint and PrivateConnection, the Two templates section describes two files that no longer exist, and Testing reports results for those two files. Since we merge with merge commits, the body persists as the record of what landed — and asserting in-VPC traffic is exactly the claim item 1 was about. Please refresh it. I'm not asking for a re-deploy: the surviving resources are unchanged from the revision you tested, apart from two tightenings I checked myself.
Nits
None of these are blocking.
CallPrivilegedToolis still granted unconditionally, and I'm not holding the PR on it, because the AWS canonicalspark-troubleshooting-mcp-setup.yamlgrants the same three actions identically at lines 101-103. This is AWS's own shape for this MCP, not something you introduced. Worth knowing what you're inheriting, though: the Service Authorization Reference forsagemaker-unified-studio-mcpdocuments exactly one action —AuthorizeVpce— and states the service "has no API operations that can be used in theActionselement", supports no resource ARNs, and has no service-specific condition keys. SoInvokeMcp,CallReadOnlyToolandCallPrivilegedToolare undocumented (that's also why cfn-lint's W3037 only knowsauthorizevpce), and with no ARN form and no condition keys the action list is the entire control surface. If you ever learn it isn't needed for all flows, a parameter defaulting tofalsewould fit your existingEnableEMREC2/EnableGluepattern.- The
E3006suppression onSparkTroubleshootingServiceis dead. cfn-lint 1.57.1 passes without it across all 11 regions in yourRulesblock, so the stated reason — not in the provider schema — is stale. TheW3037suppression is load-bearing (confirmed: removing it yields three findings), keep that one. CloudWatchKmsKeyArn'sAllowedPatternstill hardcodesarn:aws:kms:while the rest of the file moved to${AWS::Partition}. Moot under theRulesblock.SparkArtifactsBucketNameshas noAllowedPattern. An empty value buildsarn:aws:s3:::and fails at IAM policy creation rather than at parameter validation, which is a confusing place to land.- The same service exposes a sibling endpoint,
spark-code-recommendation/mcp, per the EMR setup page. This role and action set would serve it too — possible follow-up, not this PR.
Thanks
You addressed item 1 properly rather than minimally. Removing the four VPC resources and their four parameters was the easy half; you also fixed every downstream claim it had leaked into — the README opening paragraph, the IAM summary, the troubleshooting bullet, the CHANGELOG, and the inline comment on the Service resource, which now states plainly that no private connection is used and links the investigation. That matters because the README's first paragraph is lifted verbatim into mcp-data.json and becomes the public card on the docs site; it now reads accurately. You also tightened aws:SourceArn from :* to :service/* and moved the two CloudWatch Logs ARNs onto ${AWS::Partition} — neither of which I asked for.
Verified myself against 5987395: cfn-lint clean, both as submitted and across all 11 regions. Parsed both revisions and diffed resource-by-resource — the new template is exactly the old bring-your-own-VPC template minus SMUSVPCEndpoint, SMUSVPCESecurityGroup, SparkTroubleshootingPrivateConnection and the AZ Lambda, with nothing added; all seven surviving resources are byte-identical bar the two tightenings above. Endpoint URL and Service: sagemaker-unified-studio-mcp match the EMR setup page character for character. mkdocs build --strict on your branch merged onto current main: zero warnings, same count as main alone, and the catalog stub generates. S3 ARN construction still correct through the !Join/!Sub/!Split chain for both statements. Version now 1.0.0; non-production disclaimer present. Merges into current main cleanly despite being 58 commits behind.
One corroboration for item 1 that I should have caught last time: AuthorizeVpce — the only documented action on this service, described as granting permission to use the MCP service VPC endpoint — was never granted in the VPC revision either. Independent evidence that the endpoint was off the data path.
So: the llms.txt line and the PR body refresh, and I'm happy to approve. Mergeable state is BLOCKED on the parked fork workflow run plus one approval, both of which I'll handle once those land.
…gent Adds mcp/emr-spark-troubleshooting-mcp-connect/ — a single CloudFormation template that registers the AWS-managed Apache Spark Troubleshooting MCP server (SageMaker Unified Studio) with AWS DevOps Agent using SigV4 auth. What deploys ------------ - IAM role trusted by aidevops.amazonaws.com with SourceAccount / SourceArn conditions scoped to arn:aws:aidevops:<region>:<account>: service/* (matches the console-suggested scope). - Conditional inline policies for MCP invoke, EMR on EC2, EMR Serverless, Glue, scoped S3 read on caller-supplied Spark artifacts buckets, and optional CloudWatch Logs KMS decrypt. IAM shape mirrors the AWS canonical spark-troubleshooting-mcp-setup template, tightened so S3 access is scoped to caller-supplied bucket names instead of Resource: '*'. - AWS::DevOpsAgent::Service (native CFN) with MCPServerSigV4 details — registers the MCP endpoint as a capability provider. Attaching the registered Service to an Agent Space is a one-time manual step performed once via the DevOps Agent console — this is intentional and out of scope for the template. No VPC, VPC endpoint, or Private Connection ------------------------------------------- Earlier drafts of this template deployed a VPC endpoint for sagemaker-unified-studio-mcp and an AWS::DevOpsAgent::PrivateConnection. Both were removed after PR review confirmed AWS DevOps Agent reaches SigV4-authenticated MCPs over the AWS backbone using each MCP's publicly-resolvable endpoint. The DevOps Agent API also explicitly rejects PrivateConnectionName on SigV4 Services. Any VPC endpoint / Private Connection alongside a SigV4 MCP is inert for the DevOps Agent → MCP data path. Region support -------------- CFN Rules block fail-fasts outside the 11 regions where both AWS DevOps Agent and the SageMaker Unified Studio MCP service are available. Tested ------ - cfn-lint clean. - Stack CREATE_COMPLETE in ~30 seconds, DELETE_COMPLETE in ~20 seconds. - Verified Service registered with the correct SigV4 details and role ARN via aws cloudformation describe-stacks.
5987395 to
260f4aa
Compare
|
Thanks for the re-review (and the honest correction on
Not changed:
Verified again: cfn-lint clean, deploy / delete cycle still ~30s / ~20s. Ready whenever. |
ams-thakkar
left a comment
There was a problem hiding this comment.
Approving. Both items from my last pass landed, and so did two of the nits.
What needs to change
Nothing blocking.
Nits
- The "inert" claim is broader than the evidence. The body says "Any Interface VPC Endpoint or
AWS::DevOpsAgent::PrivateConnectiondeployed alongside a SigV4 MCP is inert for that path," which reads as a statement about the service. What you demonstrated is narrower and more useful: the CloudFormation/API path refusesPrivateConnectionNameonmcpserversigv4. The console path does not appear to — the DevOps Agent guide puts "Connect to endpoint using private connection" in step 1, ahead of auth-method selection in step 2, with no SigV4 carve-out, and your own Big Data blog routes a SigV4 MCP over PrivateLink with that checkbox selected. A reader who finds the blog first will think one of the two is wrong. Narrowing the sentence to the API path and linking the internal ticket would settle it, and would stop an operator who needs in-VPC routing concluding it is unavailable. Non-blocking, and a docs-only change. - Also my own error to correct, since it's in the record of my last review: I offered
AuthorizeVpcenever being granted in the VPC revision as "independent evidence that the endpoint was off the data path." That reasoning runs backwards.AuthorizeVpceis documented as granting permission to use the MCP service VPC endpoint, so its absence is at least as consistent with the VPC revision simply having been under-permissioned. Your API observation is the real evidence; my corroboration was not. CloudWatchKmsKeyArn'sAllowedPatternstill hardcodesarn:aws:kms:while the rest of the file uses${AWS::Partition}. Moot under theRulesblock.- The sibling
spark-code-recommendation/mcpendpoint would be served by this same role and action set — still a reasonable follow-up.
Thanks
You fixed both asks properly and went past them: the llms.txt entry describes the integration accurately ("creates the IAM role and the Service (capability provider) only — no MCP server code is deployed"), and the rewritten body replaces the deleted design rather than patching around it, so the merge commit will read as what actually landed. You also picked up two nits I'd marked non-blocking — the dead E3006 suppression and the missing AllowedPattern — neither of which you had to touch.
This is scoped maintainer approval. What I verified myself at 260f4aada:
- cfn-lint 1.57.2 clean in all 11 regions in your
Rulesblock, individually and combined, withE3006now gone. ConfirmedW3037is still load-bearing: strip it and you get exactly three findings, so that suppression stays earned. - The new
AllowedPatterndoes not break comma-separated input, because the parameter isCommaDelimitedListand CloudFormation applies the pattern per item rather than to the whole string. The regex admits 3–63 characters, matching yourConstraintDescription. - Diffed the whole action set against the canonical
spark-troubleshooting-mcp-setup.yaml. The only deltas are in your favour: you grants3:GetBucketLocationands3:GetObjectVersionwhere canonical grants the wildcardss3:GetBucket*,s3:GetObject*ands3:List*— so you are strictly tighter than canonical on S3, on top of scopingResourceto named buckets instead of*. - One thing I got wrong in my first review and should correct: I wrote that "every other grant on this role is read-only." Two are not —
elasticmapreduce:CreatePersistentAppUIandglue:RequestLogParsing, both onResource: '*', both behind toggles that default totrue. The conclusion is unchanged: both appear verbatim in the canonical template, and the EMR trio matches the set SageMaker's own EMR permissions reference documents underAllowPresignedUrlfor Spark UI access. They create a read-only view, not workload state. - Identifier scanner: exit 0, 0 findings across 522 added lines. No 12-digit values or secret-shaped tokens in any of the four files.
- Merges cleanly onto current
main(69e1ce4), withllms.txtauto-merging.mkdocs build --strictzero warnings on the merged tree, and the generated card inmcp-data.jsoncarries the corrected text with no PrivateLink claim. Version1.0.0; non-production disclaimer present. - I released the parked fork workflow run;
validate-skill-evalsnow passes.
What I am relying on you for, and did not verify: the CREATE_COMPLETE / DELETE_COMPLETE deploy and teardown results, and the API refusal of PrivateConnectionName. On the SigV4 and private-connection question you are the subject-matter authority here as the blog's author, so I am taking that as SME-reviewed rather than treating my reading of the docs as the tiebreaker.
Merging now.
Summary
Adds
mcp/emr-spark-troubleshooting-mcp-connect/— a single CloudFormation template that registers the AWS-managed Apache Spark Troubleshooting Agent for Amazon EMR (a SageMaker Unified Studio MCP server) with AWS DevOps Agent using SigV4 auth.No MCP server code is deployed by this template — the MCP is AWS-hosted. The template provisions only the integration surface: the SigV4 IAM role AWS DevOps Agent assumes to invoke the MCP, and the registered capability provider (
AWS::DevOpsAgent::Service).What deploys
AWS::IAM::Roletrusted byaidevops.amazonaws.comaws:SourceAccount+aws:SourceArnconditions scoped toarn:aws:aidevops:<region>:<account>:service/*AWS::DevOpsAgent::Service(native CFN)Attaching the registered Service to an Agent Space is a one-time manual step in the DevOps Agent console — intentionally out of scope so operators keep control of their Agent Space membership.
No VPC endpoint or private connection
The template deploys no VPC-side resources. During review, two independent signals confirmed that AWS DevOps Agent reaches SigV4-authenticated MCPs over the AWS backbone using the MCP's publicly-resolvable endpoint:
PrivateConnectionNameonmcpserversigv4Services ("Private connections are not supported for SigV4 auth").nslookupofsagemaker-unified-studio-mcp.<region>.api.awsreturns public AWS IPs.The DevOps Agent service team's own ServiceAuthorizationReference lists
AuthorizeVpceas the only documented action and states the service has no API operations usable in theActionselement and no resource ARNs — further corroborating that there is no customer-side network path on the DevOps Agent → MCP hop for SigV4 auth.Any Interface VPC Endpoint or
AWS::DevOpsAgent::PrivateConnectiondeployed alongside a SigV4 MCP is inert for that path. Earlier drafts of this PR deployed those resources; they were removed. The console/API inconsistency that implied otherwise has been filed internally.IAM shape
Inline policies mirror the AWS canonical
spark-troubleshooting-mcp-setup.yamltemplate, with one tightening: S3 access is scoped to caller-supplied bucket names via the requiredSparkArtifactsBucketNamesparameter, instead of the canonical template'sResource: '*'.The trust policy targets
aidevops.amazonaws.com(not account root) since the audience here is DevOps Agent.Region support
CFN
Rulesblock fail-fasts outside the 11 regions where both AWS DevOps Agent and the SageMaker Unified Studio MCP service are available:us-east-1, us-west-2, ap-northeast-1, eu-west-1, ap-southeast-1, ap-southeast-2, ca-central-1, sa-east-1, eu-central-1, eu-west-2, ap-south-1Testing
cfn-lint1.57.2: exit 0, clean across all 11 supported regionsCREATE_COMPLETEin ~30 seconds (deploy)DELETE_COMPLETEin ~20 seconds (teardown)ServiceandRoleresources verified viaaws cloudformation describe-stacks;TroubleshootingRoleArn,MCPEndpointURL,ServiceArnoutputs confirmedNot in scope
spark-code-recommendation/mcpendpoint — same role and action set would serve it; possible follow-upReferences