feat: Server 레포지토리에서 분리된 사이드 인프라(Redis, Redis-exporter, Alloy)를 Infra 레포지토리에 통합 #11
Conversation
Hexeong
requested review from
Gyuhyeok99,
JAEHEE25,
lsy1307,
sukangpunch,
whqtker and
wibaek
as code owners
Hexeong
force-pushed
the
feat/6-integrate-side-infra
branch
from
2065e3a to
b003435
Compare
📝 WalkthroughWalkthroughThis change introduces Terraform-managed side infrastructure components (Redis, Redis Exporter, and Alloy) alongside updated module variables and provisioning scripts. EC2 instances now execute dynamic setup procedures via cloud-init and remote-exec triggers, with monitoring server integration for log aggregation. Changes
Sequence Diagram(s)sequenceDiagram
participant EC2 as EC2 Instance
participant CloudInit as Cloud-init
participant Trigger as Null Resource<br/>(Triggers)
participant Scripts as Setup Scripts
participant Services as Services<br/>(Docker)
participant Monitor as Monitoring Server
EC2->>CloudInit: Initialize instance
CloudInit->>Scripts: Execute docker_setup.sh (Part 1)
Scripts->>Services: Install Docker Engine
Services-->>EC2: Docker ready
Trigger->>Trigger: Detect nginx template change
Trigger->>Scripts: Execute nginx_setup.sh.tftpl
Scripts->>Services: Install Nginx + Certbot
Scripts->>Services: Configure SSL/TLS
Services-->>EC2: Nginx ready
Trigger->>Trigger: Detect side_infra template change
EC2->>Monitor: Query monitoring server IP
Monitor-->>EC2: Return monitoring IP
Trigger->>Scripts: Execute side_infra_setup.sh.tftpl
Scripts->>Scripts: Render Alloy config (inject<br/>monitoring server IP)
Scripts->>Services: Start redis via docker-compose
Scripts->>Services: Start redis-exporter
Scripts->>Services: Start alloy (logging agent)
Services->>Monitor: Alloy pushes logs to Loki
Monitor-->>Services: ✓ Logs ingested
Services-->>EC2: Side infrastructure ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 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 |
![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 (12)
modules/app_stack/variables.tf (1)
118-148: Add validation rules for the new variables.The new variables lack validation rules, which could lead to runtime errors when invalid values are provided. Consider adding validation for:
ssh_key_path: Validate the path exists and is readablework_dir: Validate it's a valid directory path format- Version variables (
redis_version,redis_exporter_version,alloy_version): Consider validating the format matches expected Docker tag patterns🔎 Example validation rules
variable "ssh_key_path" { description = "Path to the SSH private key file for remote-exec" type = string + + validation { + condition = can(file(var.ssh_key_path)) + error_message = "The ssh_key_path must be a valid file path." + } } variable "redis_version" { description = "Docker image tag for Redis" type = string + + validation { + condition = can(regex("^[0-9]+\\.[0-9]+\\.[0-9]+$", var.redis_version)) + error_message = "Redis version must follow semantic versioning (e.g., 7.2.0)." + } }config/side-infra/config.alloy.tftpl (1)
11-11: Consider making the log path configurable.The log path is hardcoded to
/var/log/spring/*.log. If different environments or deployments need different log locations, consider making this a template variable.🔎 Example parameterization
local.file_match "spring_logs" { - path_targets = [{ __path__ = "/var/log/spring/*.log" }] // 서비스 로그 파일 경로 + path_targets = [{ __path__ = "${log_path}" }] }Then pass
log_pathas a template variable when rendering this file.modules/app_stack/scripts/docker_setup.sh (1)
1-27: Add idempotency checks and error handling.The script lacks:
- Checks for existing Docker installation (will fail if Docker is already installed)
- Explicit error handling (
set -eorset -euo pipefail)- Retry logic for apt operations
These improvements would make the script more robust for re-runs and handle transient failures.
🔎 Suggested improvements
#!/bin/bash +set -euo pipefail + +# Check if Docker is already installed +if command -v docker &> /dev/null; then + echo "Docker is already installed. Skipping installation." + exit 0 +fi # 1. 필수 패키지 설치 apt-get update apt-get install -y ca-certificates curl gnupg lsb-releasemodules/app_stack/scripts/nginx_setup.sh.tftpl (2)
6-8: Add validation for template variables.The script doesn't validate that the template variables (
domain_name,conf_file_name) are non-empty or properly formatted. Invalid values could cause the script to fail in unexpected ways.🔎 Add input validation
# --- variables setting --- DOMAIN="${domain_name}" EMAIL="${email}" CONF_NAME="${conf_file_name}" + +# Validate required variables +if [[ -z "$DOMAIN" ]] || [[ -z "$EMAIL" ]] || [[ -z "$CONF_NAME" ]]; then + echo "Error: Required variables are not set" + exit 1 +fi
22-31: Make certificate issuance idempotent.The script will fail if the SSL certificate already exists for the domain. Consider checking if the certificate exists before attempting to issue a new one.
🔎 Add certificate existence check
# 3. Issue SSL certificate (Non-interactive mode) + +# Check if certificate already exists +if [ -d "/etc/letsencrypt/live/$DOMAIN" ]; then + echo "Certificate already exists for $DOMAIN. Skipping issuance." +else + systemctl stop nginx + + certbot certonly --standalone \ + --non-interactive \ + --agree-tos \ + --email "$EMAIL" \ + -d "$DOMAIN" + + echo "Certificate obtained successfully." +fi -systemctl stop nginx - -certbot certonly --standalone \ - --non-interactive \ - --agree-tos \ - --email "$EMAIL" \ - -d "$DOMAIN" - -echo "Certificate obtained successfully."modules/app_stack/ec2.tf (1)
87-87: Verify cloud-init completion timeout.The
cloud-init status --waitcommand will wait indefinitely for cloud-init to complete. If cloud-init hangs or fails, the provisioner will block forever. Consider adding a timeout.🔎 Add timeout to cloud-init wait
- "cloud-init status --wait > /dev/null", # Docker 설치 대기 + "timeout 300 cloud-init status --wait > /dev/null || (echo 'cloud-init timeout or failure' && exit 1)", # Docker 설치 대기 (5분 타임아웃)modules/app_stack/scripts/side_infra_setup.sh.tftpl (4)
1-13: Consider addingset -ufor safer error handling.While
set -eis present, addingset -uwould catch undefined variable references and prevent silent failures when template variables aren't properly substituted.🔎 Proposed enhancement
#!/bin/bash -set -e +set -eu
30-30: Remove deprecatedversionfield from docker-compose.yml.The
versionfield in Docker Compose files is deprecated and no longer necessary in recent versions of Docker Compose.🔎 Proposed fix
cat <<EOF > "$WORK_DIR/docker-compose.side-infra.yml" -version: '3.8' - services:
36-36: Verify the necessity of host networking mode.All three services use
network_mode: "host", which bypasses Docker's network isolation and directly exposes services on the host network. While this simplifies networking, it reduces security isolation and can lead to port conflicts.If host networking is required for performance or specific architectural reasons, ensure this is documented. Otherwise, consider using Docker networks with explicit port mappings for better isolation.
Also applies to: 47-47, 58-58
72-75: Health check verification is optional; Docker Compose V2 is already ensured.The script uses Docker Compose V2 syntax (
docker composewith a space), which is correctly installed bydocker_setup.shas a prerequisite before this script runs (viadocker-compose-pluginpackage). Verifying Docker Compose V2 availability is not necessary.Adding health checks after
docker compose up -dwould be a nice-to-have enhancement for better visibility, but the current implementation with error handling on cleanup operations (|| true) is sufficient for the script's purpose.environment/stage/variables.tf (1)
107-135: Add validation and improve variable documentation.The new variables lack validation rules and some have vague descriptions:
alloy_env_name(lines 117-120): The description "Alloy Env Name" is not descriptive enough. Clarify what values are expected (e.g., "production", "dev", "staging").Version variables (lines 122-135): Consider adding validation to ensure Docker image tags follow expected formats.
ssh_key_path(lines 107-110): Could benefit from validation to ensure it's a valid file path.🔎 Proposed enhancements
variable "alloy_env_name" { - description = "Alloy Env Name" + description = "Environment name for Alloy (e.g., 'production', 'dev', 'staging')" type = string + validation { + condition = contains(["production", "dev", "staging"], var.alloy_env_name) + error_message = "alloy_env_name must be one of: production, dev, staging" + } } variable "redis_version" { description = "Docker image tag for Redis" type = string + validation { + condition = can(regex("^[a-zA-Z0-9._-]+$", var.redis_version)) + error_message = "redis_version must be a valid Docker image tag" + } } variable "redis_exporter_version" { description = "Docker image tag for Redis Exporter" type = string + validation { + condition = can(regex("^[a-zA-Z0-9._-]+$", var.redis_exporter_version)) + error_message = "redis_exporter_version must be a valid Docker image tag" + } } variable "alloy_version" { description = "Docker image tag for Grafana Alloy" type = string + validation { + condition = can(regex("^[a-zA-Z0-9._-]+$", var.alloy_version)) + error_message = "alloy_version must be a valid Docker image tag" + } }environment/prod/variables.tf (1)
107-135: Add validation and improve variable documentation (same as stage).The new variables have the same issues as in
environment/stage/variables.tf:
alloy_env_namelacks clear documentation about expected values- Version variables lack validation for Docker tag formats
ssh_key_pathcould benefit from path validationAdditionally, consider whether these identical variable definitions across stage and prod environments could be DRYed up using a shared variables module or common configuration to reduce duplication and maintenance burden.
Refer to the proposed validation enhancements in
environment/stage/variables.tf. The same improvements should be applied here for consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
README.mdconfig/secretsconfig/side-infra/config.alloy.tftplenvironment/prod/main.tfenvironment/prod/variables.tfenvironment/stage/main.tfenvironment/stage/variables.tfmodules/app_stack/ec2.tfmodules/app_stack/scripts/docker_setup.shmodules/app_stack/scripts/nginx_setup.sh.tftplmodules/app_stack/scripts/side_infra_setup.sh.tftplmodules/app_stack/variables.tf
🔇 Additional comments (10)
config/secrets (1)
1-1: The review comment references incorrect commit hashes that do not exist in this repository's history. The old commit hashc1cf69a9de6f6b766750395875cd5bdcb16a0e96is not present in any branch or tag. Theconfig/secretssubmodule was first added in commitdf43044(feat: terraform으로 prod/stage 환경 IaC 구현) and subsequently updated in commite7760fc(refactor: private IP를 사용하여 접근하도록 수정). The current commit83f176821a253bb18f5ca36f1f24e8ce2e7c91d7is the result of these normal commits and is fully visible in the repository history. No hidden or obscured changes are present.Likely an incorrect or invalid review comment.
modules/app_stack/scripts/nginx_setup.sh.tftpl (1)
52-54: LGTM! Strong TLS configuration.The TLS configuration uses modern protocols (TLSv1.2+) and strong cipher suites, which aligns with current security best practices.
modules/app_stack/ec2.tf (3)
58-94: Consider null_resource re-execution behavior.The null_resource uses SHA256 hash of the rendered template as a trigger. This means:
- Any change to the script will trigger re-execution (intended)
- The script runs via remote-exec every time the hash changes
- If the script is not idempotent, this could cause issues
Since the nginx setup script has non-idempotent operations (like cert issuance), repeated executions could fail or cause rate limiting from Let's Encrypt.
Consider whether the scripts are designed to be re-run safely. Based on the earlier review of
nginx_setup.sh.tftpl, the certificate issuance step needs idempotency improvements.
97-143: Good approach to side infra provisioning.Using null_resource with remote-exec for side infrastructure deployment is appropriate here. The trigger based on script hash ensures updates are applied when configuration changes.
The nested templatefile() for Alloy config correctly injects the monitoring server's private IP, ensuring proper integration with the logging backend.
104-106: Verify monitoring server allows inbound traffic on port 3100.The Alloy configuration sends logs to Loki on port 3100 (
http://${loki_ip}:3100/loki/api/v1/push). Both the API server and monitoring server are in the same VPC, so routing is available. However, the monitoring server's security group (not managed in this module) must allow inbound port 3100 traffic from the API server's security group (api_sg). Verify this rule exists in the monitoring server's security group configuration.README.md (1)
6-42: LGTM! Clear documentation of new structure.The updated directory structure clearly documents the new side-infra configuration directory and the scripts for deployment. The organization is logical and makes it easy to understand where files are located.
environment/prod/main.tf (1)
49-58: LGTM! Proper variable propagation.The new variables are correctly passed through to the module. The comments clearly indicate the purpose of each section (SSH key path and Side Infra variables).
environment/stage/main.tf (1)
49-58: LGTM! Consistent with production environment.The stage environment correctly mirrors the production environment's variable propagation, ensuring consistency across environments.
config/side-infra/config.alloy.tftpl (1)
25-25: No action required. The ALLOY_ENV environment variable is properly configured in the deployment setup. Theside_infra_setup.sh.tftplscript correctly passesALLOY_ENV=$ALLOY_ENV_NAMEto the Alloy container via the docker-compose environment section (line 57), ensuring logs receive the environment label as intended.modules/app_stack/scripts/side_infra_setup.sh.tftpl (1)
23-26: LGTM!The heredoc approach for creating the Alloy configuration file is clean and appropriate.
| data "aws_instance" "monitoring_server" { | ||
| filter { | ||
| name = "tag:Name" | ||
| values = ["solid-connection-monitoring"] | ||
| } | ||
|
|
||
| filter { | ||
| name = "instance-state-name" | ||
| values = ["running"] | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for missing monitoring server.
The data source will fail the Terraform plan if no monitoring server is found or if multiple servers match the filters. Consider adding validation or documentation about this prerequisite.
🔎 Add validation check
You can add a validation step after the module that uses this data source:
# In the calling module (environment/prod or stage)
locals {
monitoring_server_id = module.prod_stack.monitoring_server_id
}
# Add output in modules/app_stack/outputs.tf
output "monitoring_server_id" {
value = data.aws_instance.monitoring_server.id
description = "ID of the monitoring server"
}Alternatively, add a precondition check in Terraform 1.2+:
data "aws_instance" "monitoring_server" {
+ lifecycle {
+ precondition {
+ condition = length(data.aws_instance.monitoring_server.*.id) == 1
+ error_message = "Exactly one monitoring server must exist with tag Name=solid-connection-monitoring in running state"
+ }
+ }
+
filter {
name = "tag:Name"
values = ["solid-connection-monitoring"]
}Committable suggestion skipped: line range outside the PR's diff.
| # 1. Create working and log directories | ||
| mkdir -p "$WORK_DIR/config/side-infra" | ||
| mkdir -p "$WORK_DIR/logs" | ||
| # Set log directory permissions (Shared by App and Alloy) | ||
| chmod 777 "$WORK_DIR/logs" | ||
| chown -R ubuntu:ubuntu "$WORK_DIR" |
There was a problem hiding this comment.
Reduce overly permissive directory permissions.
Setting the log directory to 777 grants read, write, and execute permissions to all users, which poses a security risk. If the Alloy container needs write access, consider using more restrictive permissions (e.g., 775) combined with proper user/group configuration in the container setup.
🔎 Proposed fix
mkdir -p "$WORK_DIR/config/side-infra"
mkdir -p "$WORK_DIR/logs"
# Set log directory permissions (Shared by App and Alloy)
-chmod 777 "$WORK_DIR/logs"
+chmod 775 "$WORK_DIR/logs"
chown -R ubuntu:ubuntu "$WORK_DIR"📝 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.
| # 1. Create working and log directories | |
| mkdir -p "$WORK_DIR/config/side-infra" | |
| mkdir -p "$WORK_DIR/logs" | |
| # Set log directory permissions (Shared by App and Alloy) | |
| chmod 777 "$WORK_DIR/logs" | |
| chown -R ubuntu:ubuntu "$WORK_DIR" | |
| # 1. Create working and log directories | |
| mkdir -p "$WORK_DIR/config/side-infra" | |
| mkdir -p "$WORK_DIR/logs" | |
| # Set log directory permissions (Shared by App and Alloy) | |
| chmod 775 "$WORK_DIR/logs" | |
| chown -R ubuntu:ubuntu "$WORK_DIR" |
🤖 Prompt for AI Agents
In modules/app_stack/scripts/side_infra_setup.sh.tftpl around lines 16 to 21,
the script sets overly-permissive permissions on the logs directory (chmod 777);
change this to a more restrictive mode such as chmod 775 and ensure group
ownership is set to the group the Alloy container runs as (e.g., chown -R
ubuntu:<alloy-group> "$WORK_DIR" or set the correct group name), so the
container has write access without granting world-writable permissions; update
any container/user docs or deployment config to use that group if needed.
| redis: | ||
| image: redis:$REDIS_VERSION | ||
| container_name: redis | ||
| network_mode: "host" | ||
| restart: always | ||
| command: redis-server --bind 127.0.0.1 --protected-mode yes | ||
| redis-exporter: | ||
| image: oliver006/redis_exporter:$REDIS_EXPORTER_VERSION | ||
| container_name: redis-exporter | ||
| environment: | ||
| REDIS_ADDR: "127.0.0.1:6379" | ||
| depends_on: | ||
| - redis | ||
| network_mode: "host" | ||
| restart: always |
There was a problem hiding this comment.
Add health checks and consider readiness dependencies.
The redis-exporter service uses depends_on which only ensures Redis starts before the exporter, but doesn't guarantee Redis is ready to accept connections. This could cause the exporter to fail or enter a restart loop during initialization.
🔎 Proposed enhancement with health checks
redis:
image: redis:$REDIS_VERSION
container_name: redis
network_mode: "host"
restart: always
command: redis-server --bind 127.0.0.1 --protected-mode yes
+ healthcheck:
+ test: ["CMD", "redis-cli", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
redis-exporter:
image: oliver006/redis_exporter:$REDIS_EXPORTER_VERSION
container_name: redis-exporter
environment:
REDIS_ADDR: "127.0.0.1:6379"
depends_on:
- - redis
+ redis:
+ condition: service_healthy
network_mode: "host"
restart: always📝 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.
| redis: | |
| image: redis:$REDIS_VERSION | |
| container_name: redis | |
| network_mode: "host" | |
| restart: always | |
| command: redis-server --bind 127.0.0.1 --protected-mode yes | |
| redis-exporter: | |
| image: oliver006/redis_exporter:$REDIS_EXPORTER_VERSION | |
| container_name: redis-exporter | |
| environment: | |
| REDIS_ADDR: "127.0.0.1:6379" | |
| depends_on: | |
| - redis | |
| network_mode: "host" | |
| restart: always | |
| redis: | |
| image: redis:$REDIS_VERSION | |
| container_name: redis | |
| network_mode: "host" | |
| restart: always | |
| command: redis-server --bind 127.0.0.1 --protected-mode yes | |
| healthcheck: | |
| test: ["CMD", "redis-cli", "ping"] | |
| interval: 5s | |
| timeout: 3s | |
| retries: 3 | |
| redis-exporter: | |
| image: oliver006/redis_exporter:$REDIS_EXPORTER_VERSION | |
| container_name: redis-exporter | |
| environment: | |
| REDIS_ADDR: "127.0.0.1:6379" | |
| depends_on: | |
| redis: | |
| condition: service_healthy | |
| network_mode: "host" | |
| restart: always |
🤖 Prompt for AI Agents
In modules/app_stack/scripts/side_infra_setup.sh.tftpl around lines 33-48, the
redis-exporter can start before Redis is ready because depends_on only orders
container start, not readiness; add a proper healthcheck to the redis service
(e.g., a redis-cli PING or tcp check with sensible interval/timeout/retries) and
change redis-exporter to wait for Redis healthy (use depends_on with condition:
service_healthy if your compose version supports it) or, if compose version does
not support health conditions, add a small wrapper/startup check for
redis-exporter that polls Redis until it responds before launching the exporter.
관련 이슈
작업 내용
특이 사항
리뷰 요구사항 (선택)
Summary by CodeRabbit
New Features
Infrastructure Updates
✏️ Tip: You can customize this high-level summary in your review settings.