feat: Add ECS security analysis tool with interactive workflow (PR #1) #1457
Conversation
nzuresh
requested review from
a team,
biagic,
guitar80ep,
matthewgoodman13,
nineonine and
tusharbabbar
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1457 +/- ##
========================================
Coverage 89.45% 89.45%
========================================
Files 726 728 +2
Lines 50305 50434 +129
Branches 7942 7957 +15
========================================
+ Hits 44998 45117 +119
- Misses 3452 3461 +9
- Partials 1855 1856 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add comprehensive security analysis functionality for ECS clusters with interactive user workflow and AWS Trusted Advisor-style recommendations. Features: - Interactive cluster selection workflow (prevents automatic analysis) - Security checks for Container Insights, exec logging, CloudWatch encryption - Color-coded recommendations (red/yellow/green) with severity levels - Two-section output: Critical/High priority + Medium/Low priority - Comprehensive error handling and user guidance Implementation: - Module layer: FastMCP tool registration with detailed documentation - API layer: DataAdapter and SecurityAnalyzer classes - Test suite: 75 tests with 97% coverage using parameterized tests Files: - awslabs/ecs_mcp_server/api/security_analysis.py (478 lines) - awslabs/ecs_mcp_server/modules/security_analysis.py (198 lines) - tests/unit/test_security_analysis.py (405 lines) - awslabs/ecs_mcp_server/main.py (2 lines added) All quality checks passed: formatting, linting, type checking, DRY principle
nzuresh
force-pushed
the
feature/pr1-foundation-cluster
branch
from
4c4ffdb to
1675f82
Compare
| "Example: ['my-cluster', 'prod-cluster']" | ||
| ), | ||
| ), | ||
| regions: Optional[List[str]] = Field( # noqa: B008 |
There was a problem hiding this comment.
Why do we have param regions? We should assume the region that the MCP is configured with. This would be consistent with other tools.
I also experienced the following bug during this workflow:
- Customer configures ECS MCP with region "us-west-2"
- Customer asks "analyze security of clusterA in region us-east-1"
- Even though cluster exists, it does not find it since the mcp clients are configured for us-west-2
There was a problem hiding this comment.
Additional comments on above image:
- is total_clusters_analyzed expected to be 1, even though the cluster was not found? No clusters were analyzed.
- is status expected to be success even when no clusters were found?
| STEP 4 - CALL THIS TOOL: | ||
| Only after completing steps 1-3, call this tool with selected clusters and region | ||
|
|
||
| STEP 5 - AFTER ANALYSIS: |
There was a problem hiding this comment.
After testing with KiloCode, I never got the question to check another cluster after the first analysis.
Do we even need this? Customers can ask themselves if they want to analyze another cluster.
| 3. Analyze same-named clusters across multiple regions: | ||
| cluster_names: ["prod-cluster"] | ||
| regions: ["us-east-1", "us-west-2"] | ||
| Note: This will look for "prod-cluster" in both regions | ||
|
|
||
| MULTI-REGION WORKFLOW: | ||
| - Analyze one region at a time for better user experience | ||
| - After showing results, ask: "Would you like to analyze another region?" | ||
| - If yes, repeat the workflow for the new region | ||
| - This allows users to focus on one region's issues before moving to the next |
There was a problem hiding this comment.
I don't think this will work with how our credentials work. Lets only support single-cluster analysis for now. This is also inconsistent with instructions above.
| - If yes, repeat the workflow for the new region | ||
| - This allows users to focus on one region's issues before moving to the next | ||
|
|
||
| WORKFLOW: |
There was a problem hiding this comment.
This is a repeat of above?
| - CloudWatch logging configuration | ||
| - Log encryption settings | ||
|
|
||
| Parameters: |
There was a problem hiding this comment.
Do we need this repeated? We already define this above in the params section?
| return response | ||
|
|
||
|
|
||
| async def _discover_clusters(region: str) -> Dict[str, Any]: |
There was a problem hiding this comment.
is this used anywhere?
|
|
||
|
|
||
| class DataAdapter: | ||
| """Adapter that uses existing MCP tools to collect ECS data.""" |
There was a problem hiding this comment.
What is this DataAdapter class? Description says that it uses existing MCP tools, but in the class, it does not do any of that? Naming is confusing.
EDIT: I see, you are trying to reuse the methods within the ecs_resource_management tool. We shouldn't have tools relying on each other. But instead, lets re-use the methods in utils/aws.py.
| Dictionary with cluster data or error | ||
| """ | ||
| try: | ||
| response = await ecs_api_operation( |
There was a problem hiding this comment.
Lets reuse the get_aws_client("ecs") from utils
| cluster_data = ecs_data.get("cluster", {}) | ||
|
|
||
| # Run security checks (will be implemented in subsequent subtasks) | ||
| self._analyze_cluster_security(cluster_data) |
There was a problem hiding this comment.
I'm worried about how are hardcoding the analysis + recommendations. How will this be maintained or be maintainable over time? How can the analysis, recommendations, remediation steps be updated/changed as ECS evolves and new features are released.
What are alternatives here? Is it possible to use the AWS Knowledge tools to get best recommendations instead? Able to get recommendations directly from Trusted Advisor?
How did you come up with these specific analysis, recommendations that we should be checking for?
| cluster_data = await adapter.collect_cluster_data(cluster_name) | ||
|
|
||
| # Analyze security | ||
| analyzer = SecurityAnalyzer(cluster_name, region) |
There was a problem hiding this comment.
Once we simplify to checking only one region, why not let the analyzer also retrieve cluster data?
Then we have:
result = SecurityAnalyzer.analyze(clusterName) # Analyzes cluster or returns error if cluster does not exist
2 participants
Fixes
Summary
Changes
This PR adds a comprehensive ECS security analysis tool that provides AWS Trusted Advisor-style security recommendations for ECS clusters.
Key additions:
analyze_ecs_securitytool with interactive cluster selection workflowFiles added:
awslabs/ecs_mcp_server/api/security_analysis.py(478 lines) - Core security analysis logicawslabs/ecs_mcp_server/modules/security_analysis.py(198 lines) - MCP tool registrationtests/unit/test_security_analysis.py(405 lines) - Test suite with 97% coverageawslabs/ecs_mcp_server/main.py(2 lines modified) - Module registrationTotal: ~1,083 lines added
User experience
Before this change:
After this change:
Example workflow:
User: "Analyze ECS security"
Tool: Discovers clusters and prompts user to select
User: Selects clusters to analyze
Tool: Returns comprehensive security analysis with recommendations
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change? N
RFC issue number: N/A
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.