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:
WalkthroughThis change introduces a Gateway API-based ingress architecture using Envoy Gateway, replacing traditional nginx ingress controllers. It renames and refactors the certificate Helm chart, creates a new gateway-config chart with Gateway and HTTPRoute resources, updates infrastructure configuration files to disable nginx ingress in favor of Gateway API routing, and adjusts Terraform deployments to manage the new Envoy Gateway infrastructure and DNS01 certificate handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
terraform/kubernetes.tf (2)
49-51: 🧹 Nitpick | 🔵 TrivialConsider constraining
random_passwordcharset to GitLab-acceptable characters.
random_password "gitlab_root"defaults to including punctuation. If GitLab’s root account or any consumer of this secret has restricted allowed characters (or if the value is later embedded in a URL/env without escaping), authentication can break in non-obvious ways. Consider settingspecial = false(oroverride_special = "...") and adding explicitmin_lower/min_upper/min_numericto make the generated password deterministic in shape.♻️ Proposed adjustment
resource "random_password" "gitlab_root" { - length = 64 + length = 64 + special = true + override_special = "!@#%^*-_=+" + min_lower = 2 + min_upper = 2 + min_numeric = 2 }
25-31:⚠️ Potential issue | 🟠 MajorMigration to Envoy Gateway is incomplete — DNS still points to ingress-nginx.
The PR deploys Envoy Gateway resources (Gateway, GatewayClass, HTTPRoute in the
gateway-configchart), but the public ingress is still wired to ingress-nginx:
terraform/dns.tf:2readslb_ipfromdata.kubernetes_service_v1.ingress_nginxterraform/dns.tf:18uses thislb_ipfor all DNS A records (gitlab_host, registry_host, etc.)helm_release.ingress_nginxis still deployed with a LoadBalancer service (lines 81–89 in helm.tf)time_sleep.wait_for_lbdepends onhelm_release.ingress_nginx(line 178)Public traffic and DNS resolution will continue routing through nginx, not Envoy Gateway. To complete the migration, either:
- Update
dns.tfto read the LoadBalancer IP from the Envoy Gateway service instead, or- If this is an intentional parallel rollout, document the transition plan and flag nginx for deprecation.
terraform/helm.tf (2)
120-138:⚠️ Potential issue | 🟠 MajorAdd Envoy Gateway / gateway-config to
helm_release.gitlab.depends_on.GitLab is now configured with
global.gatewayApi.enabled: trueand references aGatewaynamedgatewayinenvoy-gateway-system. Its renderedHTTPRoutes require both the Gateway API CRDs (fromhelm_release.envoy_gateway) and the actualGatewayresource (from the missing gateway-config release) to exist before apply, otherwise the install will fail or create dangling routes.🔧 Proposed fix
depends_on = [ digitalocean_database_cluster.postgres, ... helm_release.cert_manager, helm_release.dns01_certificate, helm_release.reflector, helm_release.cluster_issuer, - helm_release.ingress_nginx, + helm_release.envoy_gateway, + helm_release.gateway_config, digitalocean_record.main, ... ]The same
depends_onaddition applies tohelm_release.kube_prometheus_stack(Lines 141‑155), which currently has nodepends_onat all but renders a GrafanaHTTPRoute.
81-91: 🧹 Nitpick | 🔵 TrivialThe
ingress_nginxHelm release is orphaned and should be removed.
helm/gitlab/values.yamlconfirmsglobal.ingress.enabled: false,nginx-ingress.enabled: false, andinstallCertmanager: false. No Ingress resources in the codebase reference the nginx ingress controller, and routing has been migrated to Gateway API (evidenced by Gateway, GatewayClass, and HTTPRoute resources inhelm/gateway-config/). The release, its namespace, and values file are dead infrastructure unnecessarily maintaining a LoadBalancer and DNS/TLS surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 120eea1b-8335-48aa-b5e0-f3e03dea0bdd
📒 Files selected for processing (17)
helm/dns01-certificate/chart/.helmignorehelm/dns01-certificate/chart/Chart.yamlhelm/dns01-certificate/chart/templates/_helpers.tplhelm/dns01-certificate/chart/templates/dns01-certificate.yamlhelm/dns01-certificate/chart/values.yamlhelm/gateway-config/chart/.helmignorehelm/gateway-config/chart/Chart.yamlhelm/gateway-config/chart/templates/_helpers.tplhelm/gateway-config/chart/templates/gateway-class.yamlhelm/gateway-config/chart/templates/gateway.yamlhelm/gateway-config/chart/templates/http-to-https.yamlhelm/gateway-config/chart/values.yamlhelm/gitlab/values.yamlhelm/kube-prometheus-stack/values.yamlhelm/wildcard-certificate/values.yamlterraform/helm.tfterraform/kubernetes.tf
💤 Files with no reviewable changes (1)
- helm/wildcard-certificate/values.yaml
📜 Review details
🧰 Additional context used
🪛 YAMLlint (1.38.0)
helm/dns01-certificate/chart/templates/dns01-certificate.yaml
[error] 13-13: syntax error: expected the node content, but found '-'
(syntax)
[error] 14-14: too many spaces inside braces
(braces)
[error] 14-14: too many spaces inside braces
(braces)
[error] 15-15: too many spaces inside braces
(braces)
helm/gateway-config/chart/templates/http-to-https.yaml
[error] 5-5: too many spaces inside braces
(braces)
[error] 5-5: too many spaces inside braces
(braces)
[error] 8-8: too many spaces inside braces
(braces)
[error] 8-8: too many spaces inside braces
(braces)
helm/gateway-config/chart/templates/gateway.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 5-5: too many spaces inside braces
(braces)
[error] 5-5: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
🔇 Additional comments (7)
helm/dns01-certificate/chart/Chart.yaml (1)
2-3: LGTM — chart rename is consistent with downstream Terraform wiring.
name: dns01-certificatealigns with the new chart path${path.module}/../helm/dns01-certificate/chartreferenced interraform/helm.tf.helm/gateway-config/chart/.helmignore (1)
1-23: LGTM — standard.helmignore.helm/gateway-config/chart/Chart.yaml (1)
1-24: LGTM — new chart manifest is well-formed.helm/dns01-certificate/chart/templates/dns01-certificate.yaml (1)
13-15: Thetpl . $invocation is correct and will properly evaluate nested template expressions like{{ .Values.domain }}against the root context. The terraform helm_release only passesdomain,dnsNameTemplates, andreflectNamespaces, but this is not a problem: the chart'svalues.yamlprovides defaults forname(wildcard-certificate),secretName(wildcard-certificate), andissuer(letsencrypt). These will be used when the Kubernetes Certificate is rendered, so all required fields will be populated correctly.helm/gateway-config/chart/values.yaml (1)
1-2: Confirm that the gateway-config helm release will be deployed toenvoy-gateway-systemnamespace, or add a ReferenceGrant if deployed elsewhere.The
certificateName: wildcard-certificatecorrectly matches the Secret name produced by the dns01-certificate chart (which explicitly setssecretName: wildcard-certificate). However, the original concern remains unresolved:
- ✓ The Secret is correctly named and created in
envoy-gateway-system.- ✗ The gateway-config helm_release is not deployed via Terraform, so the target namespace is unclear. The Gateway resource requires the TLS Secret to be in its namespace; cross-namespace references need a
ReferenceGrant, which does not exist in the codebase.Verify where gateway-config will be deployed. If not in
envoy-gateway-system, add aReferenceGrantallowing the Gateway to reference the Secret from that namespace.terraform/helm.tf (1)
58-68: LGTM on thednsNameTemplatesindirection.Passing
{{ .Values.domain }}strings throughyamlencode(rather than interpolating${var.domain_name}here) is correct — the chart re-templates them viatplagainst its own values, which keeps the renderedCertificate.spec.dnsNamesdriven by a single source of truth (domain). The set covers apex, wildcard, and*.pages.<domain>, which lines up with GitLab Pages user content.helm/gateway-config/chart/templates/gateway.yaml (1)
19-24: TLS Secret will be properly created and reflected to all required namespaces.The
dns01-certificateHelm chart creates a cert-manager Certificate that generates a Secret namedwildcard-certificatein theenvoy-gateway-systemnamespace. The Secret is configured with reflector annotations:reflector.v1.k8s.emberstack.com/reflection-allowed: "true" reflector.v1.k8s.emberstack.com/reflection-auto-enabled: "true" reflector.v1.k8s.emberstack.com/reflection-allowed-namespaces: "{{ .Values.reflectNamespaces }}"This ensures automatic mirroring to target namespaces. The
gateway-configreferences this Secret via.Values.certificateName(default:wildcard-certificate), which matches the secret name produced bydns01-certificate. The listener will become Ready once cert-manager provisions the certificate and it is reflected to the required namespace.
| apiVersion: gateway.networking.k8s.io/v1 | ||
| kind: GatewayClass | ||
| metadata: | ||
| name: envoy | ||
| spec: | ||
| controllerName: gateway.envoyproxy.io/gatewayclass-controller No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
GatewayClass is cluster-scoped — beware of multi-release / uninstall coupling.
Packaging a cluster-scoped GatewayClass named envoy inside this chart means a helm uninstall (or a parallel install in another namespace) will delete/conflict on a cluster-global resource that may be in use by Gateways from other releases. Consider either:
- moving the
GatewayClassto a separate, single-instance chart (or directly into Terraform), or - gating it behind a value (e.g.,
{{- if .Values.createGatewayClass }}) so only one release owns it.
| gatewayApi: | ||
| enabled: true | ||
| installEnvoy: false | ||
| configureCertmanager: false | ||
| class: nginx | ||
| tls: | ||
| enabled: true | ||
| secretName: "wildcard-certificate" | ||
| gatewayClass: envoy | ||
| gateway: | ||
| name: gateway | ||
| namespace: envoy-gateway-system | ||
|
|
||
| ingress: | ||
| enabled: false |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitLab Helm chart 9.10.3 Gateway API support for Pages HTTPRoute and TLS
💡 Result:
Yes, the GitLab Helm chart version 9.10.3 supports Gateway API for GitLab Pages using HTTPRoute with TLS termination. Gateway API support was introduced starting from chart version 9.7 (corresponding to GitLab 18.7). In 9.10.3, enable it by setting global.gatewayApi.enabled=true in your values.yaml. The chart creates HTTPRoutes for services including Pages, referencing a pages-web listener in the Gateway with TLS mode: Terminate and certificateRefs to a pages-tls secret (wildcard certificate required for Pages domains *.pages.example.com). An optional HTTPRoute redirects HTTP to HTTPS. The chart can bundle Envoy Gateway (global.gatewayApi.installEnvoy=true) or reference an external Gateway API implementation like Envoy Gateway. GitLab Pages requires a user-provided wildcard TLS secret named <release>-pages-tls or customized via gitlab.gitlab-pages.ingress.tls.secretName. Providers must support HTTPRoutes, TCPRoutes (for SSH), RegularExpression matches, and ideally GRPCRoutes.
Citations:
- 1: https://gitlab.com/gitlab-org/charts/gitlab/raw/master/values.yaml
- 2: https://docs.gitlab.com/charts/advanced/gateway-api/
- 3: https://docs.gitlab.com/charts/installation/version_mappings/
- 4: https://gitlab.com/gitlab-org/charts/gitlab/-/tags
🏁 Script executed:
#!/bin/bash
# Look for any Pages-specific Gateway/HTTPRoute templates produced by the gitlab chart locally
# and any pages-related routing configured in this repo.
rg -nP --hidden -C2 'pages|HTTPRoute|gatewayApi' helm/ terraform/ 2>/dev/null | head -200Repository: clofour/gitlab-kubernetes
Length of output: 2896
🏁 Script executed:
#!/bin/bash
# Check pages configuration in values.yaml, especially TLS secret settings
rg -A 20 'pages:' helm/gitlab/values.yaml | head -100Repository: clofour/gitlab-kubernetes
Length of output: 1281
🏁 Script executed:
#!/bin/bash
# Search for pages.ingress or pages TLS configuration more comprehensively
rg -n 'pages.*tls|gitlab-pages|pages.*ingress' helm/gitlab/values.yaml -iRepository: clofour/gitlab-kubernetes
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Search for pages-tls or pages certificate secret creation anywhere
rg -r 'pages-tls|pages.*certificate|pages.*secret' --iglob '*.yaml' --iglob '*.tf' --iglob '*.md' helm/ terraform/Repository: clofour/gitlab-kubernetes
Length of output: 1047
🏁 Script executed:
#!/bin/bash
# Look for pages-tls in all files
rg 'pages-tls' helm/ terraform/ --type yaml --type tfRepository: clofour/gitlab-kubernetes
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Get the entire gitlab section to see gitlab-pages configuration
sed -n '/^gitlab:/,/^[a-z]/p' helm/gitlab/values.yaml | head -150Repository: clofour/gitlab-kubernetes
Length of output: 583
Provide wildcard TLS secret for GitLab Pages.
The GitLab Helm chart 9.10.3 does support Gateway API for Pages with HTTPRoute and TLS termination. However, the configuration is missing the required wildcard TLS certificate secret. The chart expects either a secret named <release>-pages-tls or explicit configuration via gitlab.gitlab-pages.ingress.tls.secretName, but neither is present in this values.yaml. The Terraform setup includes the wildcard domain *.pages.${domain} in the dns01 certificate, which is correct, but the secret must be created and referenced in the Helm values. Without it, Pages HTTPS will fail. Add the secret or configure gitlab.gitlab-pages.ingress.tls.secretName pointing to the certificate secret.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Infrastructure