feat: add CloudWatch IAM role, shielded deploy target, and CI for testnet-shielded-outputs - #395
Conversation
📝 WalkthroughWalkthroughAdds a Makefile target and CI job for shielded testnet deployments, declares ChangesShielded deployment and tooling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds infrastructure/config updates to support deploying a new shielded environment by having Serverless manage the API Gateway → CloudWatch IAM role and by adding a dedicated Makefile deploy target.
Changes:
- Adds an
AWS::IAM::RoleCloudFormation resource for API Gateway CloudWatch logging. - Introduces
make deploy-lambdas-shieldedtargeting stageshieldedineu-central-1with thetestnet-shielded-outputsAWS profile. - Removes the TODO comment related to manually pre-creating the API Gateway CloudWatch role.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| serverless.yml | Adds CloudFormation resources for the API Gateway CloudWatch logs role and adjusts the related logs config comment. |
| Makefile | Adds a new deploy target for the shielded stage using a specific AWS profile/region. |
Comments suppressed due to low confidence (1)
serverless.yml:1009
CustomApiGatewayAccountCloudWatchRoleis incomplete (onlyDependsOnis defined). As-is, this is invalid CloudFormation/YAML for a resource and will fail deployment. Either remove this stub resource or add the requiredTypeandProperties(e.g., anAWS::ApiGateway::AccountwithCloudWatchRoleArn) so the template is valid.
CustomApiGatewayAccountCloudWatchRole:
DependsOn: ApiGatewayCloudWatchLogsRole
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2cbee0e to
e3429b0
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
serverless.yml (1)
1044-1046:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
CustomApiGatewayAccountCloudWatchRoleis incomplete and will break deployment.Line 1044 defines a resource without
Type/Properties, so CloudFormation will reject the template.🔧 Proposed fix
CustomApiGatewayAccountCloudWatchRole: DependsOn: ApiGatewayCloudWatchLogsRole + Type: AWS::ApiGateway::Account + Properties: + CloudWatchRoleArn: + Fn::GetAtt: + - ApiGatewayCloudWatchLogsRole + - Arn🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@serverless.yml` around lines 1044 - 1046, The resource CustomApiGatewayAccountCloudWatchRole is missing required CloudFormation fields and will fail deployment; update the serverless.yml to define this resource as an AWS::IAM::Role named CustomApiGatewayAccountCloudWatchRole (keeping the existing DependsOn: ApiGatewayCloudWatchLogsRole) and provide the necessary Properties including AssumeRolePolicyDocument (trust policy for apigateway.amazonaws.com), RoleName or logical name, and inline Policies or ManagedPolicyArns granting CloudWatch/logs permissions needed by API Gateway (e.g., logs:CreateLogGroup, logs:CreateLogStream, logs:PutLogEvents); ensure the property names match CloudFormation (Type and Properties) so the template validates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@serverless.yml`:
- Around line 1044-1046: The resource CustomApiGatewayAccountCloudWatchRole is
missing required CloudFormation fields and will fail deployment; update the
serverless.yml to define this resource as an AWS::IAM::Role named
CustomApiGatewayAccountCloudWatchRole (keeping the existing DependsOn:
ApiGatewayCloudWatchLogsRole) and provide the necessary Properties including
AssumeRolePolicyDocument (trust policy for apigateway.amazonaws.com), RoleName
or logical name, and inline Policies or ManagedPolicyArns granting
CloudWatch/logs permissions needed by API Gateway (e.g., logs:CreateLogGroup,
logs:CreateLogStream, logs:PutLogEvents); ensure the property names match
CloudFormation (Type and Properties) so the template validates.
- Add CloudFormation resources to serverless.yml to create the APIGatewayPushToCloudWatchLogs IAM role automatically on deploy, removing the need to pre-create it manually (drops the TODO comment) - Add Makefile target `deploy-lambdas-shielded` for deploying to the shielded stage in eu-central-1 using the testnet-shielded-outputs AWS profile Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3887875 to
600c49d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@serverless.yml`:
- Around line 1029-1047: The stack creates an account-scoped
AWS::ApiGateway::Account resource (CustomApiGatewayAccountCloudWatchRole)
pointing CloudWatchRoleArn at a stage-owned IAM role
(ApiGatewayCloudWatchLogsRole), which can cause race/deletion issues across
multiple stage stacks; to fix, remove creation of AWS::ApiGateway::Account from
per-stage stacks and instead provision a single account-level resource in a
dedicated foundation stack that exports the CloudWatchRoleArn, update per-stage
stacks to import/reference that exported CloudWatchRoleArn (or a parameter)
rather than creating CustomApiGatewayAccountCloudWatchRole, and if you must keep
the IAM role in CloudFormation, add DeletionPolicy: Retain to
ApiGatewayCloudWatchLogsRole to avoid accidental deletion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c1a551b-ed24-40e4-97b3-4ce2ea72a5cb
📒 Files selected for processing (4)
.github/workflows/deploy.ymlMakefilepyproject.tomlserverless.yml
…ternally AWS::ApiGateway::Account is a singleton per account+region. Managing it in every CloudFormation stack causes stacks to overwrite each other, and deleting any stack resets the account-level logging config globally. Move the IAM role and account association to Terraform (ops-tools), and use roleManagedExternally: true so Serverless never generates its own AWS::ApiGateway::Account resource. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Depends on: https://github.com/HathorNetwork/ops-tools/pull/1344
Summary
This PR adds infrastructure and CI improvements for the
testnet-shielded-outputsenvironment:CloudWatch IAM Role (
serverless.yml)The
APIGatewayPushToCloudWatchLogsIAM role was previously required to exist manually before deployment. This PR adds it as a CloudFormation resource in theresourcessection, so Serverless Framework creates/manages it automatically. The TODO comment noting this gap has been removed.Shielded Deploy Target (
Makefile)Adds a
deploy-lambdas-shieldedMakefile target to streamline deployments to theshieldedstage:eu-central-1testnet-shielded-outputsCI/CD for testnet-shielded-outputs (
.github/workflows/deploy.yml)Adds automated deployment workflow for the
testnet-shielded-outputsenvironment.Fix: Poetry Export (
pyproject.toml,Makefile)Ensures
poetrycan properly export dependencies for deployment.How to deploy
Summary by CodeRabbit