feat: infra #88
feat: infra #88
Conversation
|
@CodeRabbit full review |
|
Warning Rate limit exceeded@0xkenj1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request establishes a new deployment framework using Terraform to provision AWS resources. A deployment-specific 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (17)
deployment/bastion_scripts/install_dependencies.sh (2)
1-1: Shebang and Error Handling Directive
The shebang is set correctly for bash. Consider adding an error-handling directive (e.g.,set -e) immediately after the shebang to ensure the script exits on any command failure, which improves overall reliability.-#!/bin/bash +#!/bin/bash +set -e
13-13: PNPM Installer Security Consideration
Piping the output of the installer script fromwgetdirectly toshis a common shortcut; however, it can introduce security risks if the remote script changes unexpectedly. Consider verifying the script’s integrity (for example, via a checksum or signature) or using a more secure method to install pnpm.deployment/modules/storage/variables.tf (1)
11-18: Database Credentials: Consider Marking Sensitive.
Whilerds_usernameis defined fine, forrds_password(lines 20-23) it is a good practice to addsensitive = trueto prevent the inadvertent exposure of secrets in Terraform’s output and logs.deployment/README.md (1)
32-57: Markdown List Indentation Adjustments Recommended.
Static analysis (markdownlint MD007) indicates that several unordered list items are indented using 4 spaces instead of the recommended 2 spaces. Adjusting the indentation will improve readability and consistency. For example, you can change:- - Contains environment-specific configurations + - Contains environment-specific configurationsApply similar adjustments to all list items flagged by markdownlint.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
36-36: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
deployment/modules/networking/main.tf (2)
1-28: VPC Module Configuration Looks Good
The VPC module is configured properly using the standard Terraform AWS VPC module. The naming, CIDR, subnets, and NAT gateway settings are clearly defined. One minor suggestion is to consider parameterizing the VPC CIDR (currently hardcoded as "10.0.0.0/16") for greater flexibility in different environments.
53-85: RDS Security Group Ingress Rules – Verify Exposure Intent
Two ingress blocks allow PostgreSQL traffic on port 5432—from both private and public subnets. While this may be intentional (to support both internal processing and API access), exposing RDS to public subnets can raise security concerns. Please double-check if public subnet access is required or if tighter restrictions (e.g., specific CIDR ranges) might be more appropriate.deployment/modules/compute/main.tf (1)
152-243: Processing Task Definitions Leverage Dynamic For-Each Mapping
The use of afor_eachovervar.CHAINSto define processing task definitions streamlines deployment for multiple chains.
- Environment variables include important configuration parameters, and the use of
jsonencodeensures proper formatting for the ECS container definitions.- The commented-out INDEXER_ADMIN_SECRET variable should be reviewed: if no longer needed, consider removing the commented code to reduce clutter.
deployment/environments/production/main.tf (1)
1-10: Clarify S3 Backend Configuration
The Terraform block is well structured; however, the backend "s3" block is currently empty. For robust state management, please consider specifying details such as the bucket name, key, region, and optionally encryption settings.deployment/modules/iam/main.tf (2)
61-78: RDS Access Policy – Consider Scope Refinement
The RDS access policy allows actions on resources formatted as
arn:aws:rds:${var.region}:${var.account_id}:db:*
While this might be necessary for dynamic environments, consider whether scoping this resource further is possible to adhere to least privilege principles.
118-136: Logger Access Policy – Review for Least Privilege
Similar to the ECR policy, the logger access policy currently allows actions on all resources. Consider if it’s possible to limit the scope to specific log groups or resources. Please verify that this wide permission is intentional.🧰 Tools
🪛 Checkov (3.2.334)
[HIGH] 119-136: Ensure IAM policies does not allow write access without constraints
(CKV_AWS_290)
[HIGH] 119-136: Ensure no IAM policies documents allow "*" as a statement's resource for restrictable actions
(CKV_AWS_355)
deployment/modules/compute/variables.tf (4)
1-3: Explicit Type for 'region' Variable?
Theregionvariable is declared without a description or an explicit type. Consider adding a description and a type (e.g.type = string) to improve clarity and type safety.
23-30: Consider Adding Type Constraints for 'app_name' and 'app_environment'
The variablesapp_nameandapp_environmenthave descriptions but no explicit type. Defining their type (e.g.type = string) will enhance type safety and consistency.
42-66: Other Basic Environment Variables Look Correct
Variables such asNODE_ENV, retry configuration parameters, and API service settings are straightforward. One consideration: if numerical operations are expected (e.g. for retry delays), you might evaluate whether a number type is more suitable than a string.
119-201: Datalayer Hasura and Other Environment Variables
The series of variables for configuring Hasura and related settings are detailed. The definitions are clear, but due to the volume of similar variables, you might consider grouping them into an object or map later for enhanced maintainability.deployment/environments/production/variables.tf (3)
94-155: Processing Environment Variables for Blue
The blue processing environment variables, includingBLUE_NODE_ENV, retry configurations, and chain settings, are clearly defined. Consider whether defaults like"http://localhost:8080/v1/graphql"for GraphQL URL are intentional for production or should be updated.
272-307: Datalayer Hasura API Settings for Blue
Most settings are straightforward. A few points to consider:
- The default for
BLUE_DATALAYER_HASURA_CORS_DOMAINis set to"*", which may be intentional, but in production environments you might want to restrict the allowed origins.- For
BLUE_DATALAYER_HASURA_CONSOLE_ASSETS_DIR, the default is"null"(as a string). If the intention is to represent a null value, consider using Terraform’s null (without quotes) for clarity.
358-392: Datalayer Hasura API Settings for Green – Consistent with Blue
The green environment settings largely mirror the blue ones. The same considerations regarding use of"*"for CORS and"null"for assets dir apply here.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
deployment/.gitignore(1 hunks)deployment/README.md(1 hunks)deployment/bastion_scripts/install_dependencies.sh(1 hunks)deployment/environments/production/main.tf(1 hunks)deployment/environments/production/outputs.tf(1 hunks)deployment/environments/production/variables.tf(1 hunks)deployment/modules/api-gw/main.tf(1 hunks)deployment/modules/api-gw/outputs.tf(1 hunks)deployment/modules/api-gw/variables.tf(1 hunks)deployment/modules/bastion/main.tf(1 hunks)deployment/modules/bastion/outputs.tf(1 hunks)deployment/modules/bastion/variables.tf(1 hunks)deployment/modules/compute/main.tf(1 hunks)deployment/modules/compute/outputs.tf(1 hunks)deployment/modules/compute/variables.tf(1 hunks)deployment/modules/container-registry/main.tf(1 hunks)deployment/modules/container-registry/outputs.tf(1 hunks)deployment/modules/container-registry/variables.tf(1 hunks)deployment/modules/iam/main.tf(1 hunks)deployment/modules/iam/outputs.tf(1 hunks)deployment/modules/iam/variables.tf(1 hunks)deployment/modules/load_balancer/main.tf(1 hunks)deployment/modules/load_balancer/outputs.tf(1 hunks)deployment/modules/load_balancer/variables.tf(1 hunks)deployment/modules/networking/main.tf(1 hunks)deployment/modules/networking/outputs.tf(1 hunks)deployment/modules/networking/variables.tf(1 hunks)deployment/modules/storage/main.tf(1 hunks)deployment/modules/storage/outputs.tf(1 hunks)deployment/modules/storage/variables.tf(1 hunks)deployment/registry/main.tf(1 hunks)deployment/registry/variables.tf(1 hunks)deployment/state/main.tf(1 hunks)deployment/state/variables.tf(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- deployment/modules/compute/outputs.tf
- deployment/modules/container-registry/outputs.tf
- deployment/modules/iam/variables.tf
- deployment/.gitignore
🧰 Additional context used
🪛 Checkov (3.2.334)
deployment/modules/container-registry/main.tf
[HIGH] 2-4: Ensure ECR image scanning on push is enabled
(CKV_AWS_163)
deployment/modules/bastion/main.tf
[HIGH] 4-14: Ensure Instance Metadata Service Version 1 is not enabled
(CKV_AWS_79)
deployment/modules/load_balancer/main.tf
[HIGH] 52-60: Ensure that load balancer is using at least TLS 1.2
(CKV_AWS_103)
deployment/modules/iam/main.tf
[HIGH] 98-116: Ensure no IAM policies documents allow "*" as a statement's resource for restrictable actions
(CKV_AWS_355)
[HIGH] 119-136: Ensure IAM policies does not allow write access without constraints
(CKV_AWS_290)
[HIGH] 119-136: Ensure no IAM policies documents allow "*" as a statement's resource for restrictable actions
(CKV_AWS_355)
🪛 markdownlint-cli2 (0.17.2)
deployment/README.md
36-36: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: test / Unit tests
🔇 Additional comments (96)
deployment/bastion_scripts/install_dependencies.sh (4)
2-4: System Update and Package Installation
The commands to update the package list (apt update) and install Node.js and npm (apt install -y nodejs npm) are correctly used. Ensure that the environment has the necessary privileges (or that the script is run with sudo) to perform these operations.
7-7: Global Installation of 'n'
The script installs the Node.js version manager "n" globally using npm, which is appropriate. Confirm that the executing user has the required permissions for global installs or run the command with sudo if needed.
9-10: Node.js Version Installation Using 'n'
The commandn i 20is used to install Node.js version 20 via the "n" version manager. Verify that this shorthand (ifor install) functions as expected in your environment and that any necessary elevated privileges (if required by "n") are handled appropriately.
15-15: Success Message Confirmation
The echo statement confirms that the dependencies have been installed successfully. This message provides clear user feedback and is appropriate as is.deployment/modules/storage/outputs.tf (1)
1-3: Verify RDS Module ReferenceThe new output
rds_endpointcorrectly exposes thedb_instance_endpointfrom therdsmodule. Please ensure that therdsmodule is defined and configured properly in your overall Terraform structure.deployment/modules/container-registry/variables.tf (1)
1-4: Approval for Variable DeclarationThe new
app_namevariable is clearly defined with an appropriate description and type. This will help ensure consistent naming of your ECR repository.deployment/modules/api-gw/outputs.tf (1)
1-3: Validate API Gateway Output ReferenceThe output
api_gateway_urlis set to captureaws_api_gateway_stage.this.invoke_url. Please confirm that the API Gateway stage resource is indeed labeled asthis, and that this output meets your integration requirements.deployment/registry/main.tf (2)
1-3: AWS Provider Configuration VerificationThe AWS provider block is straightforward and correctly uses
var.AWS_REGIONto set the region. Ensure thatvar.AWS_REGIONis properly defined in your variables.
5-8: Container Registry Module IntegrationThe module declaration for
container_registryuses a relative source and correctly passesapp_nameviavar.APP_NAME. Please verify thatvar.APP_NAMEis defined in this context and aligns with the expectations of your container registry module.deployment/modules/bastion/outputs.tf (1)
1-7: Clear and Consistent Outputs
The output declarations effectively expose the bastion instance's public IP and ID, facilitating integration with other modules. The implementation follows Terraform conventions.deployment/registry/variables.tf (1)
1-9: Well-Defined Application Variables
The addition ofAPP_NAMEandAWS_REGIONwith clear descriptions and explicit type definitions increases configuration clarity and supports reuse across different modules.deployment/state/variables.tf (1)
1-9: Consistent State Variables Addition
Integrating theAPP_NAMEandAWS_REGIONvariables in the state configuration ensures consistency with the registry variables. This centralized approach enhances environment management.deployment/modules/networking/variables.tf (1)
1-14: Well-Structured Networking Variables
The new variables (app_name,app_environment, andregion) are clearly defined with descriptive comments, enabling flexible and clear configuration for networking resources.deployment/environments/production/outputs.tf (3)
1-3: Output for active_deployment is clearly defined.
Exposingvar.ACTIVE_DEPLOYMENThere makes it easy to track which deployment is currently active.
5-7: Output for deployment_state is well configured.
Providing the deployment state viavar.DEPLOYMENT_STATEwill help in managing and debugging deployment flows.
9-11: API Gateway URL output is clearly exposed.
Referencingmodule.api_gateway.api_gateway_urlfacilitates integration with dependent modules. Ensure that theapi_gatewaymodule is configured correctly elsewhere.deployment/state/main.tf (4)
1-3: AWS provider configuration is standard and clear.
Usingvar.AWS_REGIONmakes the configuration flexible across environments.
4-4: AWS caller identity data block is appropriately used.
This block ensures that the current account information is available for later use, such as in policy definitions.
6-9: Locals block utilizes expressive naming.
Constructingbucket_nameas${var.APP_NAME}-terraform-stateand capturing theaccount_idare both clear and maintainable. Consider verifying that the constructed bucket name complies with AWS naming restrictions.
11-47: S3 Terraform state module configuration is comprehensive.
Key security settings (blocking public ACLs/policies, etc.) are in place and a JSON-encoded policy explicitly defines permitted S3 actions. The tagging strategy usinglocal.bucket_nameis also clear.deployment/modules/load_balancer/outputs.tf (3)
1-3: Output for lb_green_target_group_arn is precise.
Referencingaws_lb_target_group.green_api_target_group.arnfollows a clear convention and supports further integrations.
5-7: Output for lb_blue_target_group_arn is defined correctly.
It mirrors the structure of the green target group output, ensuring consistency across target groups.
9-11: Output for lb_dns_name properly exposes the load balancer’s DNS.
This output is crucial for routing purposes in modules like the API gateway.deployment/modules/iam/outputs.tf (3)
1-3: Output for processing_service_role_arn is correctly set.
Exposing the IAM role ARN for processing services is essential for cross-module role referencing.
5-7: Output for api_service_role_arn is well defined.
This facilitates proper linkage for API service tasks.
9-11: Output for bastion_instance_profile_name is appropriately configured.
This output will enable other modules or operations to reference the bastion’s instance profile easily.deployment/modules/bastion/variables.tf (5)
1-4: Variable app_name is defined clearly.
The description and type specification provide good context for the application naming conventions.
6-9: Variable app_environment is well documented.
Describing the environment (e.g., dev, staging, prod) makes it easier for users to configure deployments correctly.
11-14: Variable subnet_id is straightforward and descriptive.
This variable will ensure that the bastion host is deployed within the intended subnet.
16-19: Variable bastion_instance_profile_name is accurately defined.
Providing a clear description helps in linking the bastion with the corresponding IAM instance profile.
21-24: Variable bastion_security_group_id is clearly specified.
This variable is essential for associating the correct security group with the bastion instance.deployment/modules/storage/main.tf (4)
1-4: Module Declaration is Correct and Version Locked.
The module is defined using the official Terraform AWS RDS module with an explicit version (“~> 6.10.0”), which helps ensure stability.
5-13: Identifier Construction and RDS Parameters Are Clear.
The concatenated identifier ("${var.app_name}-${var.app_environment}-rds") and the main RDS parameters (engine, engine_version, instance_class, allocated_storage) are defined as expected. No issues noted here.
14-20: Credentials and Networking Configuration.
Settingmanage_master_user_passwordto false forces the master credentials to be provided externally, which is appropriate if managed securely. The use of variables forusername,password, VPC security group IDs, and subnet IDs makes the module flexible. Ensure that sensitive data are handled via secure backends or secret management.
21-34: Backup, Maintenance, and Tagging Settings.
The backup retention, backup window, maintenance window, accessibility, encryption, and tagging parameters are well defined. Consider parameterizing values like the backup window and maintenance window if future flexibility is required.deployment/modules/storage/variables.tf (3)
1-9: Core Variables for Application Identification.
The variablesapp_name,app_environment, andregionare clearly defined with appropriate descriptions and types, helping ensure consistent naming and usage throughout the deployment.
25-33: Networking Variables Are Clearly Defined.
Bothrds_security_group_idandrds_subnet_idsare set with the proper types. This ensures that network configuration for the RDS instance is consistent.
35-38: Subnet Group Name is Explicitly Defined.
Therds_subnet_group_namevariable is defined in a clear and straightforward manner, which makes it easy to reference in the module.deployment/README.md (2)
1-8: Introduction and Overview Are Well Documented.
The header and overview sections clearly explain the purpose of the deployment directory and provide context for the infrastructure configuration.
9-31: Directory Structure Clarity.
The documentation nicely delineates the organization intoenvironments/,modules/, andstate/directories. This breakdown aids in understanding the separation of concerns in the deployment.deployment/modules/api-gw/variables.tf (1)
1-33: API Gateway Variables Are Configured Appropriately.
All variables—includingapp_name,app_environment,lb_dns_name, and the throttling/quota parameters—are defined with clear descriptions, types, and sensible defaults. This setup facilitates a robust and easily configurable API Gateway deployment.deployment/modules/load_balancer/variables.tf (3)
1-10: Load Balancer Base Variables Are Well Defined.
The variablesapp_nameandapp_environmentare clear and consistent with other modules, helping maintain uniform naming conventions across the configuration.
11-18: Active Deployment Validation is Well Implemented.
Theactive_deploymentvariable includes a validation block that restricts the value to either "green" or "blue", with a meaningful error message. This is a good practice to ensure predictable blue-green deployment behavior.
20-33: Networking and Security Group Configurations.
The definitions forvpc_id,public_subnets, andload_balancer_security_group_idhave the appropriate types and descriptions. This clarity helps ensure that the load balancer module receives the correctly formatted input.deployment/modules/networking/main.tf (4)
33-49: Processing Security Group is Configured Appropriately
The security group for processing is configured with an open egress rule and proper tagging. If the open outbound rule is intentional for your use case, this configuration is acceptable.
87-110: Load Balancer Security Group Configuration Is Clear
This security group permits HTTP traffic (port 80) and allows all outbound connections. The configuration and naming are consistent.
112-134: API Security Group Ingress Rule – Dependency on Load Balancer
The API security group only allows incoming traffic from the load balancer security group, which is a good security practice. Ensure that the referenced security group resource (aws_security_group.load_balancer.id) remains consistent with any changes in the load balancer configuration.
136-144: Database Subnet Group Definition is Correct
The DB subnet group correctly groups the private subnets for RDS deployment. Tags have been applied consistently.deployment/modules/load_balancer/main.tf (3)
1-7: Application Load Balancer Resource Configuration
The ALB is appropriately configured as external, using the provided public subnets and security group. The naming and parameters align with best practices.
9-29: Target Groups for Blue/Green Deployments are Set Up Properly
Both target groups include well‑defined health check settings and tagging. The use of separate resources for blue (“blue_api_target_group”) and green (“green_api_target_group”) deployments is ideal for blue-green strategies.
52-60:❓ Verification inconclusive
Load Balancer Listener – Enforce TLS 1.2?
The listener currently uses HTTP (port 80) without any TLS configuration. Note that static analysis (CKV_AWS_103) recommends enforcing at least TLS 1.2. Consider switching to an HTTPS listener by updating the protocol to "HTTPS", adding the appropriatessl_policy(e.g., "ELBSecurityPolicy-TLS-1-2-2017-01"), and providing a certificate ARN.A potential diff for switching to HTTPS might be:
-resource "aws_lb_listener" "api_listener" { - load_balancer_arn = aws_lb.load_balancer.arn - port = 80 - protocol = "HTTP" - default_action { - type = "forward" - target_group_arn = var.active_deployment == "green" ? aws_lb_target_group.green_api_target_group.arn : aws_lb_target_group.blue_api_target_group.arn - } -} +resource "aws_lb_listener" "api_listener" { + load_balancer_arn = aws_lb.load_balancer.arn + port = 443 + protocol = "HTTPS" + ssl_policy = "ELBSecurityPolicy-TLS-1-2-2017-01" + certificate_arn = var.certificate_arn + default_action { + type = "forward" + target_group_arn = var.active_deployment == "green" ? aws_lb_target_group.green_api_target_group.arn : aws_lb_target_group.blue_api_target_group.arn + } +}If using HTTPS is not desired in this context, please document why HTTP was preferred.
Action: Enforce HTTPS in the Load Balancer Listener Configuration
The current configuration in
deployment/modules/load_balancer/main.tf(lines 52–60) defines theaws_lb_listenerfor the API using HTTP on port 80 without any TLS parameters. As noted by the CKV_AWS_103 recommendation, enforcing TLS 1.2 via an HTTPS listener is preferred.
Issue: Use of HTTP (port 80) without TLS exposes data in transit.
Recommendation: Switch to an HTTPS listener. This involves:
- Changing the port from 80 to 443.
- Updating the protocol from
"HTTP"to"HTTPS".- Adding an
ssl_policy(e.g.,"ELBSecurityPolicy-TLS-1-2-2017-01").- Specifying a
certificate_arnfor TLS termination.Actionable Diff:
-resource "aws_lb_listener" "api_listener" { - load_balancer_arn = aws_lb.load_balancer.arn - port = 80 - protocol = "HTTP" - default_action { - type = "forward" - target_group_arn = var.active_deployment == "green" ? aws_lb_target_group.green_api_target_group.arn : aws_lb_target_group.blue_api_target_group.arn - } -} +resource "aws_lb_listener" "api_listener" { + load_balancer_arn = aws_lb.load_balancer.arn + port = 443 + protocol = "HTTPS" + ssl_policy = "ELBSecurityPolicy-TLS-1-2-2017-01" + certificate_arn = var.certificate_arn + default_action { + type = "forward" + target_group_arn = var.active_deployment == "green" ? aws_lb_target_group.green_api_target_group.arn : aws_lb_target_group.blue_api_target_group.arn + } +}If an HTTP listener is required for specific reasons, please document the rationale for not adopting HTTPS.
🏁 Script executed:
#!/bin/bash echo "Verifying 'aws_lb_listener' configuration in deployment/modules/load_balancer/main.tf..." if [ -f deployment/modules/load_balancer/main.tf ]; then grep -A 15 'resource "aws_lb_listener" "api_listener"' deployment/modules/load_balancer/main.tf echo "Extracting protocol configuration..." grep -A 5 'resource "aws_lb_listener" "api_listener"' deployment/modules/load_balancer/main.tf | grep "protocol" else echo "File deployment/modules/load_balancer/main.tf not found." fiLength of output: 1047
Enforce HTTPS for API Listener Configuration
The current configuration indeployment/modules/load_balancer/main.tf(lines 52–60) sets up the API listener on port 80 using HTTP without TLS, which conflicts with the CKV_AWS_103 recommendation to enforce at least TLS 1.2. Please consider making the following updates:
- Switch Protocol: Change the listener's protocol from
"HTTP"to"HTTPS".- Update Port: Change the port from
80to443.- Add TLS Settings: Include an
ssl_policy(e.g.,"ELBSecurityPolicy-TLS-1-2-2017-01") and acertificate_arn, which you can source from a variable likevar.certificate_arn.A potential diff for the update is:
-resource "aws_lb_listener" "api_listener" { - load_balancer_arn = aws_lb.load_balancer.arn - port = 80 - protocol = "HTTP" - default_action { - type = "forward" - target_group_arn = var.active_deployment == "green" ? aws_lb_target_group.green_api_target_group.arn : aws_lb_target_group.blue_api_target_group.arn - } -} +resource "aws_lb_listener" "api_listener" { + load_balancer_arn = aws_lb.load_balancer.arn + port = 443 + protocol = "HTTPS" + ssl_policy = "ELBSecurityPolicy-TLS-1-2-2017-01" + certificate_arn = var.certificate_arn + default_action { + type = "forward" + target_group_arn = var.active_deployment == "green" ? aws_lb_target_group.green_api_target_group.arn : aws_lb_target_group.blue_api_target_group.arn + } +}If there is a deliberate reason to continue using HTTP, please document the rationale clearly.
🧰 Tools
🪛 Checkov (3.2.334)
[HIGH] 52-60: Ensure that load balancer is using at least TLS 1.2
(CKV_AWS_103)
deployment/modules/networking/outputs.tf (1)
1-32: Networking Outputs are Well Defined
All critical outputs (VPC ID, subnets, security group IDs, etc.) are exported consistently, which facilitates cross-module integrations. Ensure that consumers of these outputs are updated accordingly if any resource names change in the future.deployment/modules/api-gw/main.tf (9)
1-3: Local Variable for Stage Name is Clear
The local variablestage_nameis defined appropriately, ensuring consistency across the API Gateway resources.
5-11: API Gateway REST API Resource Configured Correctly
The REST API is set up with a "REGIONAL" endpoint, which fits well for most production scenarios.
13-17: Proxy Resource for API Gateway is Properly Defined
Capturing all paths using "{proxy+}" effectively supports a proxy integration setup.
19-28: Proxy Method Configuration for API Gateway
The "ANY" method with no authorization and proper request parameter mapping is correctly implemented for the proxy.
30-42: API Gateway Integration for Proxy is Set Up Adequately
The integration uses "HTTP_PROXY" with correct mapping of the proxy path parameter. Double-check that the backend load balancer (referenced byvar.lb_dns_name) is accessible via the configured URI scheme.
44-49: OPTIONS Method for Preflight Requests is Configured
The dedicated OPTIONS method without authorization is implemented to support CORS preflight requests.
51-61: Deployment Resource with Create-Before-Destroy Lifecycle is Sound
Using a dependency on the integration and a lifecycle block to minimize downtime during updates is appropriate.
63-67: API Gateway Stage is Clearly Defined
The stage ties the deployment and REST API together correctly, using the localstage_namefor consistency.
69-86: Usage Plan Configuration Provides Required API Throttling and Quota
The usage plan effectively groups rate limiting and quota settings along with referencing the API stage. This ensures controlled API usage.deployment/modules/compute/main.tf (3)
1-4: Local Variables for Logging and Container Naming are Set Up Well
Defininglog_group_nameandapi_container_namelocally supports consistent resource naming.
6-86: ECS Module Configuration is Comprehensive
The module block leverages the Terraform AWS ECS module effectively. Merging service definitions for the API and processing services is a neat approach.
- The API service is conditionally created with appropriate settings (subnets, security groups, autoscaling parameters, etc.).
- The blue-green load balancer mapping within the API service is clear.
- For the processing services, the loop iterates over
var.CHAINSto define distinct services.
89-150: API Task Definition is Configured Correctly
The API task definition for FARGATE includes proper resource allocation (CPU, memory), networking mode, and container definitions.
- Environment variables are dynamically built using a for-expression which improves maintainability.
- Health checks, port mappings, and logging configurations are appropriately defined.
Be sure that the image tag and repository values (fromvar.api_repository_urlandvar.api_image_tag) are correctly provided in the environment, as any mismatch could lead to deployment errors.deployment/environments/production/main.tf (10)
11-13: AWS Provider Block Looks Good
The provider declaration correctly sets the AWS region.
16-17: Caller Identity Data Source – LGTM
Using theaws_caller_identitydata source to dynamically retrieve the account ID is appropriate.
19-24: Networking Module Configuration Verified
Module "networking" is configured with clear variable mappings. Ensure that the source path (../../modules/networking) is correct relative to this file.
26-32: IAM Module Configuration Looks Appropriate
The IAM module parameters correctly pass required variables including the account ID from the caller identity.
34-44: Storage Module Setup is Clear
The storage module includes necessary parameters for RDS configuration. Confirm that the referenced outputs (e.g.module.networking.rds_security_group_id) exist and are correctly exported from the networking module.
46-53: Bastion Module Parameters are Consistent
The bastion module’s parameters (subnet, instance profile, and security group) are correctly configured.
55-63: Load Balancer Module Declaration – Verified
Parameters for the load balancer module appear complete. It’s good to see that the active deployment flag is passed.
65-71: API Gateway Module Looks Correct
The API gateway module receives the load balancer DNS name and related identifiers properly.
73-123: Review Deployment Logic in Blue Compute Module
The "blue_compute" module defines many environment variables and deployment conditions. In particular, the expression forshould_deploy_moduleuses
var.ACTIVE_DEPLOYMENT == "blue" || var.DEPLOYMENT_STATE == "deploying"
which means the blue deployment may run even when not explicitly active if the state is “deploying”. Please verify if this logic is intentional and documented.
125-175: Review Deployment Logic in Green Compute Module
Similarly, the "green_compute" module configures deployment flags and environment settings. The symmetry with the blue module is good; however, ensure that the conditions (usingvar.ACTIVE_DEPLOYMENT == "green" || var.DEPLOYMENT_STATE == "deploying") match your deployment strategy. Consider documenting how the "deploying" state interacts with both blue and green modules.deployment/modules/iam/main.tf (7)
1-17: Processing Service Role Configuration – LGTM
Theaws_iam_role.processing_service_roleresource is correctly defined with an assume role policy for ECS tasks.
19-35: API Service Role Configuration Looks Correct
The resource foraws_iam_role.api_service_roleis set up similarly to the processing role.
37-52: Bastion Role Configuration Verified
The bastion role is declared with an EC2 service principal and a proper trust relationship.
56-59: IAM Instance Profile for Bastion – Looks Good
The instance profile resource binds the bastion role correctly.
80-95: Secrets Access Policy is Appropriately Scoped
The secrets access policy restricts access to a specific secret resource, which is good practice.
138-143: SSM Policy Attachment Confirmed
Attaching the managed policyAmazonSSMManagedInstanceCoreto the bastion role is standard practice for EC2 instances.
144-190: IAM Role Policy Attachments are Consistent
All role policy attachments (RDS, Secrets, ECR, Logger) are correctly linking policies to the intended roles. Double-check that the roles and policies align with your application’s security requirements.deployment/modules/compute/variables.tf (7)
4-11: 'color' Variable Validation is Clear
The validation condition ensures the value is either "blue" or "green". Good use of validation.
13-16: Module Deployment Flags – Well Defined
should_deploy_moduleandis_active_deploymentare clearly documented with descriptions and types.
31-40: Subnet Variables are Appropriately Typed
public_subnetsandprivate_subnetsare declared with clear types and descriptions.
67-91: API and Load Balancer Variables are Defined Properly
The configuration for API repository, image tags, and load balancer target group ARNs is clear.
93-117: Processing Service Settings are Consistent
All processing-related variables are declared with appropriate descriptions and types.
204-213: CHAINS Variable Structure is Clear
TheCHAINSvariable is defined as a list of objects with a clear schema. Verify that the type for theidfield (string) is consistent with its usage in other modules.
216-261: Remaining Variables for Database and Configuration Settings
The declarations forDATABASE_URL,INDEXER_GRAPHQL_URL,METADATA_SOURCE,PUBLIC_GATEWAY_URLS,PRICING_SOURCE,COINGECKO_API_KEY,COINGECKO_API_TYPE, andLOG_LEVELfollow expected patterns.deployment/environments/production/variables.tf (8)
5-12: DEPLOYMENT_STATE Variable – Validation is Solid
The validation rule restricts the value to either "single" or "deploying", ensuring correct state management.
14-21: ACTIVE_DEPLOYMENT Variable Enforces Allowed Values
The validation forACTIVE_DEPLOYMENTis appropriately constrained to "blue" or "green".
28-53: Shared Variables are Clear and Well-Defined
Variables such asAWS_REGION,APP_NAME,APP_ENVIRONMENT, along with sensitive database credentials, are declared with clarity.
59-73: Blue Environment Image and API Settings Look Correct
Ensure that these variables are overridden appropriately in production and that the defaults (if any) meet your deployment requirements.
76-90: Green Environment Image Settings are Consistent
The green environment settings mirror the blue ones correctly.
156-173: Blue Environment - Additional Settings
The settings for metadata, public gateways, pricing, and Coingecko API details for the blue environment are comprehensive.
174-195: Green Environment - Processing Variables
The green counterparts are similarly defined. Check that any defaults (if provided) align with production configurations.
252-266: Datalayer Postgres Variables – Sensitive and Clear
The Postgres DB name variables are marked sensitive, which is good.
| resource "aws_ecr_repository" "processing_repository" { | ||
| name = "${var.app_name}-processing" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enable Image Scanning on Push for ECR
Incorporate image scanning on push as recommended by Checkov [CKV_AWS_163] to enhance repository security. Consider adding an image_scanning_configuration block. For example:
-resource "aws_ecr_repository" "processing_repository" {
- name = "${var.app_name}-processing"
-}
+resource "aws_ecr_repository" "processing_repository" {
+ name = "${var.app_name}-processing"
+ image_scanning_configuration {
+ scan_on_push = true
+ }
+}This improvement will help ensure that vulnerabilities in container images are caught early.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resource "aws_ecr_repository" "processing_repository" { | |
| name = "${var.app_name}-processing" | |
| } | |
| resource "aws_ecr_repository" "processing_repository" { | |
| name = "${var.app_name}-processing" | |
| image_scanning_configuration { | |
| scan_on_push = true | |
| } | |
| } |
🧰 Tools
🪛 Checkov (3.2.334)
[HIGH] 2-4: Ensure ECR image scanning on push is enabled
(CKV_AWS_163)
There was a problem hiding this comment.
will avoid this for now
There was a problem hiding this comment.
@0xkenj1 Understood. I'll respect your decision to proceed without image scanning for now. Let me know if you need any other assistance with your Terraform infrastructure.
| resource "aws_instance" "bastion" { | ||
| ami = "ami-0884d2865dbe9de4b" | ||
| instance_type = "t3.large" | ||
| subnet_id = var.subnet_id | ||
| iam_instance_profile = var.bastion_instance_profile_name | ||
| vpc_security_group_ids = [var.bastion_security_group_id] | ||
|
|
||
| tags = { | ||
| Name = "${var.app_name}-${var.app_environment}-bastion" | ||
| } | ||
| } |
There was a problem hiding this comment.
Enforce Instance Metadata Service v2 for Enhanced Security
The current configuration for the bastion EC2 instance does not explicitly enforce the use of Instance Metadata Service Version 2, which is recommended to mitigate certain security risks (see CVK_AWS_79). Consider adding a metadata_options block to require IMDSv2.
Below is an example diff to address this:
resource "aws_instance" "bastion" {
ami = "ami-0884d2865dbe9de4b"
instance_type = "t3.large"
subnet_id = var.subnet_id
iam_instance_profile = var.bastion_instance_profile_name
vpc_security_group_ids = [var.bastion_security_group_id]
+ metadata_options {
+ http_tokens = "required"
+ http_endpoint = "enabled"
+ http_put_response_hop_limit = 2
+ }
tags = {
Name = "${var.app_name}-${var.app_environment}-bastion"
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resource "aws_instance" "bastion" { | |
| ami = "ami-0884d2865dbe9de4b" | |
| instance_type = "t3.large" | |
| subnet_id = var.subnet_id | |
| iam_instance_profile = var.bastion_instance_profile_name | |
| vpc_security_group_ids = [var.bastion_security_group_id] | |
| tags = { | |
| Name = "${var.app_name}-${var.app_environment}-bastion" | |
| } | |
| } | |
| resource "aws_instance" "bastion" { | |
| ami = "ami-0884d2865dbe9de4b" | |
| instance_type = "t3.large" | |
| subnet_id = var.subnet_id | |
| iam_instance_profile = var.bastion_instance_profile_name | |
| vpc_security_group_ids = [var.bastion_security_group_id] | |
| metadata_options { | |
| http_tokens = "required" | |
| http_endpoint = "enabled" | |
| http_put_response_hop_limit = 2 | |
| } | |
| tags = { | |
| Name = "${var.app_name}-${var.app_environment}-bastion" | |
| } | |
| } |
🧰 Tools
🪛 Checkov (3.2.334)
[HIGH] 4-14: Ensure Instance Metadata Service Version 1 is not enabled
(CKV_AWS_79)
There was a problem hiding this comment.
prefer to avoid small modifications that can introduce potential issues
There was a problem hiding this comment.
@0xkenj1 I understand your preference to avoid small modifications that might introduce potential issues, even security-related ones like enforcing IMDSv2. That's a valid approach, especially when working with infrastructure code where stability is critical.
If you decide to address this security recommendation in the future, the metadata_options block can be added as part of a more comprehensive update to the bastion configuration. For now, I'll respect your decision to keep the current implementation as is.
✏️ Learnings added
Learnt from: 0xkenj1
PR: defi-wonderland/grants-stack-indexer-v2#88
File: deployment/modules/bastion/main.tf:4-14
Timestamp: 2025-03-05T19:31:03.262Z
Learning: For infrastructure code, 0xkenj1 prefers to avoid small modifications, even security-related ones like enforcing IMDSv2 on EC2 instances, to prevent introducing potential issues. Security improvements should be part of more comprehensive updates.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| # ECR access policy | ||
| resource "aws_iam_policy" "ecr_access_policy" { | ||
| name = "${var.app_name}-${var.app_environment}-ECRAccessPolicy" | ||
|
|
||
| policy = jsonencode({ | ||
| Version = "2012-10-17", | ||
| Statement = [ | ||
| { | ||
| Effect = "Allow", | ||
| Action = [ | ||
| "ecr:GetAuthorizationToken", | ||
| "ecr:BatchCheckLayerAvailability", | ||
| "ecr:GetDownloadUrlForLayer", | ||
| "ecr:BatchGetImage" | ||
| ], | ||
| Resource = "*" | ||
| } | ||
| ] | ||
| }) | ||
| } |
There was a problem hiding this comment.
ECR Access Policy – Potential Over-Permission
The ECR access policy uses "Resource": "*". Static analysis recommends narrowing resource scope where possible. Evaluate if you can restrict actions to a specific repository ARN. If broader access is necessary, document the justification for using "*".
🧰 Tools
🪛 Checkov (3.2.334)
[HIGH] 98-116: Ensure no IAM policies documents allow "*" as a statement's resource for restrictable actions
(CKV_AWS_355)
There was a problem hiding this comment.
ditto , ECR is created on a different terraform workflow, improving this could be tricky, will keep it as it is
There was a problem hiding this comment.
@0xkenj1 Understood. You're right that managing permissions across different Terraform workflows can be challenging. Since the ECR repositories are created in a separate workflow, using the wildcard resource specifier makes sense from a practical standpoint, even though it's not ideal from a strict security perspective.
Just as a note for the future, if you ever decide to revisit this, one potential approach could be to use data sources to look up the existing ECR repositories and then reference their ARNs specifically. But I understand that's an optimization that may not be worth the added complexity right now.
✏️ Learnings added
Learnt from: 0xkenj1
PR: defi-wonderland/grants-stack-indexer-v2#88
File: deployment/modules/iam/main.tf:97-116
Timestamp: 2025-03-05T19:34:26.734Z
Learning: In this project, ECR repositories are created in a separate Terraform workflow from the IAM policies that need to access them, making it challenging to restrict ECR access policies to specific repository ARNs. The team has decided to use "Resource": "*" in the ECR access policy for practical reasons.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
|
||
| DATABASE_URL = "postgresql://${var.DATALAYER_PG_USER}:${var.DATALAYER_PG_PASSWORD}@${module.storage.rds_endpoint}/${var.BLUE_DATALAYER_PG_DB_NAME}" | ||
| INDEXER_GRAPHQL_URL = var.BLUE_INDEXER_GRAPHQL_URL | ||
| # INDEXER_ADMIN_SECRET = var.INDEXER_ADMIN_SECRET |
There was a problem hiding this comment.
is this fine to be commented out?
There was a problem hiding this comment.
Yep, we don't need an admin secret to query envio graphql api. We will whitelist IP on envio hosting service
| type = string | ||
| } | ||
|
|
||
| # variable "INDEXER_ADMIN_SECRET" { |
| # We'll only have a single NAT Gateway instead of 1 per AZ | ||
| single_nat_gateway = true |
There was a problem hiding this comment.
for any particular reason?
| account_id = data.aws_caller_identity.current.account_id | ||
| } | ||
|
|
||
| module "s3_terraform_state" { |
There was a problem hiding this comment.
i would enable bucket versioning
as nth, add object lock and sse encryption
https://github.com/terraform-aws-modules/terraform-aws-s3-bucket/tree/master
Description
Terraform infrastructure for Grants stack indexer v2
Checklist before requesting a review
Summary by CodeRabbit
Summary by CodeRabbit
New Features
.gitignorefile to manage ignored files in the deployment process.Documentation
Chores