Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
terraform/helm.tf (1)
108-125:⚠️ Potential issue | 🟠 MajorAdd
kubernetes_secret_v1.gitlab_sendgrid_secrettodepends_on.The Helm values reference the
gitlab-sendgrid-secretfor SMTP password, but it isn't listed in thisdepends_on. On a clean apply, Terraform may schedule the Helm release before the secret exists, causing pod startup failures or a transient reconcile error.🔧 Proposed fix
kubernetes_secret_v1.gitlab_redis, - kubernetes_secret_v1.gitlab_s3_main + kubernetes_secret_v1.gitlab_s3_main, + kubernetes_secret_v1.gitlab_sendgrid_secret ]terraform/outputs.tf (1)
44-50:⚠️ Potential issue | 🟠 MajorStale output naming and endpoint after R2 migration.
output "spaces_endpoint"still emits a DigitalOcean Spaces URL, but the buckets it logically pairs with are now Cloudflare R2. Any consumer using both will end up pointing at the wrong endpoint. Update to the R2 S3-compatible endpoint (e.g.,https://${var.cloudflare_account_id}.r2.cloudflarestorage.comor the EU jurisdiction variant).output "spaces_buckets"should be renamed (e.g.,r2_buckets) to reflect the new backing resource and avoid confusing downstream consumers.terraform/kubernetes.tf (1)
103-115:⚠️ Potential issue | 🔴 CriticalCritical: GitLab S3 connection is broken — wrong endpoint and wrong credentials for R2.
This secret will not authenticate against Cloudflare R2:
endpointstill points at DigitalOcean Spaces (https://${var.region}.digitaloceanspaces.com). For R2 it must behttps://<account_id>.r2.cloudflarestorage.com(or jurisdiction-specific variant such as.eu.r2.cloudflarestorage.com).aws_access_key_id/aws_secret_access_keymust be an R2 Access Key ID and Secret Access Key generated via the R2 dashboard/API.var.cloudflare_account_idis an account identifier (not a key) andvar.cloudflare_api_tokenis a Cloudflare API token used for the Terraform provider — not the R2 S3 secret. R2's S3-compatible API uses SigV4 with R2-specific keys.- For R2,
regionshould beauto.The GitHub Actions workflow already injects
R2_ACCESS_KEY_ID/R2_SECRET_ACCESS_KEYasAWS_*env vars (for the TF backend). Plumb those through as newTF_VAR_r2_access_key_id/TF_VAR_r2_secret_access_keyenvironment variables and use them here. Apply the same fix togitlab_s3_backup(lines 136–154).Proposed fix for
gitlab_s3_maindata = { connection = yamlencode({ provider = "AWS" - region = var.region - endpoint = "https://${var.region}.digitaloceanspaces.com" - aws_access_key_id = var.cloudflare_account_id - aws_secret_access_key = var.cloudflare_api_token + region = "auto" + endpoint = "https://${var.cloudflare_account_id}.r2.cloudflarestorage.com" + aws_access_key_id = var.r2_access_key_id + aws_secret_access_key = var.r2_secret_access_key path_style = true }) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41670b00-09f1-4557-9d22-8d275b82c86d
📒 Files selected for processing (11)
.github/workflows/terraform.yaml.gitignorehelm/gitlab/values.yamlterraform/helm.tfterraform/kubernetes.tfterraform/outputs.tfterraform/providers.tfterraform/s3.tfterraform/spaces.tfterraform/variables.tfterraform/versions.tf
💤 Files with no reviewable changes (1)
- terraform/spaces.tf
📜 Review details
🧰 Additional context used
🪛 Checkov (3.2.524)
helm/gitlab/values.yaml
[low] 89-90: Base64 High Entropy String
(CKV_SECRET_6)
🔇 Additional comments (6)
.gitignore (1)
2-2: Looks good — ignore rule is valid.Adding
TODO.mdto.gitignoreis straightforward and safe.terraform/kubernetes.tf (2)
142-151: Same issue applies togitlab_s3_backup.Mirror the fix from
gitlab_s3_main: switch theendpoint,region, and access key variables to R2 values. Otherwise GitLab backups silently fail to upload.
156-167: Sendgrid secret wiring looks correct.Matches
helm/gitlab/values.yamlSMTP config (secret: gitlab-sendgrid-secret,key: password).terraform/providers.tf (1)
5-7: LGTM.Cloudflare provider initialized with the API token variable; matches
versions.tfandvariables.tf.terraform/versions.tf (1)
9-12: No action required—attributes are correct for Cloudflare provider 5.19.0.The
cloudflare_r2_bucketresource interraform/s3.tfcorrectly usesaccount_id,name, andjurisdictionattributes for provider version ~> 5.19.0. All three attributes are valid in this version, withaccount_idandnameas required fields andjurisdictionas optional.helm/gitlab/values.yaml (1)
77-93: No issues found. Configuration keys forglobal.emailandglobal.smtpare correct per GitLab Helm chart 9.10.3 documentation, includingdisplay_name,from,reply_to, anduser_namein snake_case. The SendGrid SMTP settings with STARTTLS on port 587 are properly configured.
| resource "random_id" "suffix" { | ||
| byte_length = 3 | ||
| keepers = { | ||
| cluster_name = var.cluster_name | ||
| } | ||
| lifecycle { | ||
| prevent_destroy = true | ||
| } | ||
| } |
There was a problem hiding this comment.
keepers + prevent_destroy on random_id is contradictory.
If var.cluster_name ever changes, keepers will mark this resource for replacement, but prevent_destroy = true will then abort the plan with an error. If the goal is to lock the suffix permanently, drop keepers. If the goal is to rotate it with cluster name changes, drop prevent_destroy.
| resource "cloudflare_r2_bucket" "gitlab" { | ||
| for_each = local.buckets | ||
| account_id = var.cloudflare_account_id | ||
| name = "${var.cluster_name}-${each.key}-${random_id.suffix.hex}" | ||
| jurisdiction = var.r2_jurisdiction | ||
| } No newline at end of file |
There was a problem hiding this comment.
Bucket destroy/rename risk and naming-length concerns.
cloudflare_r2_bucket.gitlablackslifecycle { prevent_destroy = true }even thoughrandom_id.suffixhas it. Any change tovar.cluster_name,each.key, orvar.r2_jurisdictionrecreates the bucket — R2 cannot rename, so this means destroy + recreate and data loss. Addprevent_destroyhere too, given these buckets back GitLab artifacts/registry/backups.- R2 bucket names must be 3–63 chars, lowercase alphanumerics + hyphens. With keys like
ci-secure-files(15) anddependency-proxy(16) plus 6 hex chars and two dashes,cluster_nameis effectively capped at ~40 chars. Consider validatingcluster_namelength, or hashing instead of concatenating.
🔧 Proposed fix
resource "cloudflare_r2_bucket" "gitlab" {
for_each = local.buckets
account_id = var.cloudflare_account_id
name = "${var.cluster_name}-${each.key}-${random_id.suffix.hex}"
jurisdiction = var.r2_jurisdiction
+
+ lifecycle {
+ prevent_destroy = true
+ }
}| variable "r2_jurisdiction" { | ||
| type = string | ||
| default = "eu" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
Cloudflare R2 supported jurisdictions list (default, eu, fedramp)
💡 Result:
Cloudflare R2 supports three jurisdictions for data storage: default (no restriction, data stored globally), eu (European Union), and fedramp (FedRAMP, requires Enterprise customers).
Citations:
- 1: https://developers.cloudflare.com/r2/reference/data-location/
- 2: https://developers.cloudflare.com/r2/buckets/data-location/
- 3: https://alos.no/cfnet/api/Cloudflare.NET.Accounts.Models.R2Jurisdiction.html
Add validation block to constrain r2_jurisdiction to supported values.
Cloudflare R2 only accepts three jurisdictions: default, eu, and fedramp. Without a validation block, an invalid value will only fail at terraform apply time with an API error. Adding validation catches misconfiguration earlier.
♻️ Proposed validation
variable "r2_jurisdiction" {
type = string
default = "eu"
+ validation {
+ condition = contains(["default", "eu", "fedramp"], var.r2_jurisdiction)
+ error_message = "r2_jurisdiction must be one of: default, eu, fedramp."
+ }
}📝 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.
| variable "r2_jurisdiction" { | |
| type = string | |
| default = "eu" | |
| } | |
| variable "r2_jurisdiction" { | |
| type = string | |
| default = "eu" | |
| validation { | |
| condition = contains(["default", "eu", "fedramp"], var.r2_jurisdiction) | |
| error_message = "r2_jurisdiction must be one of: default, eu, fedramp." | |
| } | |
| } |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 participant
Summary by CodeRabbit
Release Notes