fix: nginx 인증서 자동 갱신 안정화#55
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughmonitoring_stack 및 app_stack의 nginx 설정이 standalone Certbot에서 webroot 기반 HTTP-01 challenge 방식으로 전환되었다. monitoring_stack에는 IAM instance profile과 SSM RunCommand 기반 nginx 설정 재실행 경로가 추가되었으며, 관련 변수와 모듈 배선 및 secrets 서브프로젝트 참조가 갱신되었다. ChangesNginx SSL 갱신 및 SSM 재실행
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Terraform
participant SSM as AWS SSM
participant EC2 as monitoring_server
participant Nginx
participant Certbot
Terraform->>SSM: SSM 에이전트 온라인 대기
Terraform->>SSM: send-command(setup_nginx.sh)
SSM->>EC2: nginx 설정 스크립트 실행
EC2->>Nginx: challenge 설정 작성 및 reload
EC2->>Certbot: certonly --webroot 인증서 발급
EC2->>Nginx: full 설정 작성 및 reload
Terraform->>SSM: 실행 상태 폴링
SSM-->>Terraform: Success 또는 실패 상태
Certbot->>Nginx: renew deploy-hook reload
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Terraform Plan:
|
Terraform Plan:
|
Terraform Plan:
|
Terraform Plan:
|
Hexeong
requested review from
Gyuhyeok99,
lsy1307,
sukangpunch,
whqtker and
wibaek
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b05728b97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
modules/monitoring_stack/scripts/nginx_setup.sh.tftpl (1)
22-45: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win인증서가 있으면 80 전용 설정으로 먼저 덮어쓰지 마세요
modules/monitoring_stack/scripts/nginx_setup.sh.tftpl:22-45에서 매번 80 전용 설정으로reload한 뒤 full 설정을 다시 쓰고 있어서, 기존 인증서가 있는 재실행에서도 443 서버 블록이 잠깐 사라집니다. 인증서가 이미 있으면 처음부터 full 설정을 적용하고, challenge location만 추가하는 분기로 바꾸는 편이 좋습니다.🤖 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 `@modules/monitoring_stack/scripts/nginx_setup.sh.tftpl` around lines 22 - 45, The nginx setup flow in the script template is overwriting the existing config with an HTTP-only server block before restoring the full TLS config, which can temporarily drop the 443 block on reruns. Update the branching around the nginx config generation so the existing-cert path writes the full server configuration from the start, and only adds the ACME challenge location needed for certbot; keep the no-cert path on the current 80-only bootstrap. Use the nginx config generation and reload logic in the script template to locate the change.modules/app_stack/scripts/nginx_setup.sh.tftpl (1)
154-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value중복된 full 설정 작성/심볼릭 링크 처리.
인증서가 이미 존재하는 경로(154-162행)에서
write_full_nginx_conf+ 심볼릭 링크 + default 제거를 수행한 뒤, 176-180행에서 동일 작업이 다시 반복됩니다. challenge→full 전환이 필요한 최초 발급 경로를 제외하면 중복이므로, 발급이 실제로 필요한 경우에만 재작성하도록 정리하면 흐름이 단순해집니다.🤖 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 `@modules/app_stack/scripts/nginx_setup.sh.tftpl` around lines 154 - 180, The nginx setup flow is duplicating the full config write and site symlink/default cleanup in the certificate-present branch and again after certbot runs. Refactor the logic around write_full_nginx_conf, write_http_challenge_conf, the ln -sf /etc/nginx/sites-available/$CONF_NAME step, and reload_nginx so the full configuration and symlink cleanup happen only once after a successful issuance/renewal, while the challenge config remains only for the initial HTTP-01 path.
🤖 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 `@modules/monitoring_stack/scripts/nginx_setup.sh.tftpl`:
- Around line 102-108: Split the cron file name used in the certbot setup from
CONF_NAME, since the current /etc/cron.d/certbot-$CONF_NAME naming can inherit
dots from nginx_conf_name and be ignored by cron. Update the
nginx_setup.sh.tftpl block that writes the cron entry so the /etc/cron.d
filename is a fixed or sanitized value, while keeping CONF_NAME only for the
rest of the setup logic. Use the certbot renewal cron stanza as the location to
make this change.
---
Nitpick comments:
In `@modules/app_stack/scripts/nginx_setup.sh.tftpl`:
- Around line 154-180: The nginx setup flow is duplicating the full config write
and site symlink/default cleanup in the certificate-present branch and again
after certbot runs. Refactor the logic around write_full_nginx_conf,
write_http_challenge_conf, the ln -sf /etc/nginx/sites-available/$CONF_NAME
step, and reload_nginx so the full configuration and symlink cleanup happen only
once after a successful issuance/renewal, while the challenge config remains
only for the initial HTTP-01 path.
In `@modules/monitoring_stack/scripts/nginx_setup.sh.tftpl`:
- Around line 22-45: The nginx setup flow in the script template is overwriting
the existing config with an HTTP-only server block before restoring the full TLS
config, which can temporarily drop the 443 block on reruns. Update the branching
around the nginx config generation so the existing-cert path writes the full
server configuration from the start, and only adds the ACME challenge location
needed for certbot; keep the no-cert path on the current 80-only bootstrap. Use
the nginx config generation and reload logic in the script template to locate
the change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60bcfba8-8100-433c-8ed4-208cdfeb15f6
📒 Files selected for processing (7)
config/secretsenvironment/monitoring/main.tfenvironment/monitoring/variables.tfmodules/app_stack/scripts/nginx_setup.sh.tftplmodules/monitoring_stack/ec2.tfmodules/monitoring_stack/scripts/nginx_setup.sh.tftplmodules/monitoring_stack/variables.tf
|
👍 Caddy 사용도 고려해볼 수 있을 것 같네요 |
찾아본 결과, Caddy가 현재 nginx의 역할을 대체할 수 있다는 점 확인했습니다! 이번 PR이 반영되고 동일한 문제가 발생한다면 해당 작업 진행해보는 방향으로 하겠습니다! |
lsy1307
left a comment
lsy1307
left a comment
There was a problem hiding this comment.
전체 nginx/webroot 전환 흐름은 괜찮아 보입니다. 기존 인증서가 있으면 full nginx config를 유지한 상태에서 challenge location만 포함하고, 인증서가 없을 때만 HTTP challenge bootstrap config를 쓰는 분기도 적절해 보입니다.
다만 config/secrets submodule 포인터 변경에 monitoring.tfvars뿐 아니라 prod.tfvars 변경도 함께 포함되어 있습니다. prod plan 결과가 null_resource.update_nginx 교체만 나온다고 적어주셔서 실제 영향은 없어 보이지만, PR 설명에는 monitoring secrets 갱신만 언급되어 있어 prod tfvars 변경도 의도된 최신화인지 한 줄 남겨주면 더 명확할 것 같습니다.
해당 내용은 다른 PR이 머지됨과 함께 서브 모듈 포인터 충돌을 해결하고자 반영된 내용입니다. 포인터를 최신화하는 과정에서 반영되었기에 해당 PR의 작업내용은 아니지만, nginx 설정이 반영되는 과정에 있어 문제가 없게 최신화를 진행했습니다. |
3 participants
관련 이슈
작업 내용
Monitoring Nginx 인증서 발급/갱신 방식을
standalone에서webroot방식으로 변경했습니다./.well-known/acme-challenge/요청을 처리합니다./etc/cron.d/certbot-$CONF_NAME파일로 관리하도록 변경해 중복 등록을 방지했습니다.Monitoring EC2에 nginx 설정 변경 반영 경로를 추가했습니다.
nginx_setup.sh.tftpl렌더링 결과의 hash를terraform_data.update_nginx트리거로 사용합니다.ec2_iam_instance_profile변수를 추가했습니다.Prod/Stage 공용 app_stack Nginx 인증서 갱신도 동일하게 안정화했습니다.
certbot --standalone과systemctl stop nginx흐름을 제거했습니다.null_resource.update_nginx의 기존 script hash 기반 재실행 구조는 유지했습니다.Monitoring secrets submodule을 갱신했습니다.
특이 사항
reload로 수행되며, reload 실패 시에만restartfallback을 사용합니다.nginx_setup.sh.tftpl내용이 바뀌면 script hash 변경으로null_resource.update_nginx가 재실행됩니다.terraform_data.update_nginx를 통해 동일한 hash 기반 재실행 구조를 갖습니다.검증 결과:
bash -n modules/monitoring_stack/scripts/nginx_setup.sh.tftplbash -n modules/app_stack/scripts/nginx_setup.sh.tftplterraform validateforenvironment/monitoringterraform validateforenvironment/stageterraform validateforenvironment/prodterraform_data.update_nginx생성null_resource.update_nginxscript hash replacement onlynull_resource.update_nginxscript hash replacement only리뷰 요구사항 (선택)
Summary by CodeRabbit