Repository navigation
Move the Terraform templates here and align their sizing - #60
David Larsen (dc-larsen) wants to merge 3 commits into
Conversation
…oud Run Move the per-platform Terraform templates from the socketdev-demo org so every Registry Firewall deployment template lives in this repo. Files are unchanged in this commit; follow-up commits update links and sizing defaults. Sources (socketdev-demo, copied verbatim): - socketdev-demo/socket-firewall-aws-ecs-fargate @ fb0eccb -> terraform/aws-ecs-fargate - socketdev-demo/socket-firewall-azure-container-apps @ 91ddad4 -> terraform/azure-container-apps - socketdev-demo/socket-firewall-gcp-cloud-run @ 7126b69 -> terraform/gcp-cloud-run
Default to 3 replicas of 4 vCPU and 8 GiB, set nginx workers from the CPU allocation with 4096 connections and an 8192 open-file limit, set socket.api_read_timeout to 60, raise liveness probe timeouts to 10 s, add Redis sizing notes, and point READMEs at their new paths and the public docs.
Add a templates table covering Helm, CloudFormation, and the three Terraform templates, link the sizing guide, and link the Terraform siblings from the CloudFormation README.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Domain tokens corrupt package tarball URLs
- Azure and ECS now set path_routing.domain to a single hostname and put extra names (including localhost) in allowed_domain so rewritten package URLs stay valid.
- ✅ Fixed: Ingress protocol mismatches container port
- Azure Container Apps ingress now targets HTTP port 8080, matching TLS termination at the platform instead of forwarding cleartext to the TLS listener on 8443.
Or push these changes by commenting:
@cursor push 99bea5bac4
Preview (99bea5bac4)
diff --git a/terraform/aws-ecs-fargate/main.tf b/terraform/aws-ecs-fargate/main.tf
--- a/terraform/aws-ecs-fargate/main.tf
+++ b/terraform/aws-ecs-fargate/main.tf
@@ -49,8 +49,11 @@
}
path_routing = {
enabled = true
- domain = "${aws_lb.firewall.dns_name} localhost"
- routes = local.routes
+ # Must be a single hostname. Multiple space-separated tokens corrupt
+ # the URLs the firewall rewrites into npm metadata responses.
+ domain = aws_lb.firewall.dns_name
+ allowed_domain = ["localhost"]
+ routes = local.routes
}
})
}
diff --git a/terraform/azure-container-apps/main.tf b/terraform/azure-container-apps/main.tf
--- a/terraform/azure-container-apps/main.tf
+++ b/terraform/azure-container-apps/main.tf
@@ -45,6 +45,11 @@
registry = name
}]
+ # path_routing.domain is substituted into rewritten package URLs, so it
+ # must be one hostname. Extra tokens from var.domain (cert SANs) and
+ # localhost belong in allowed_domain.
+ domain_tokens = compact(split(" ", var.domain))
+
socket_yml = yamlencode(merge(
{
ports = {
@@ -71,8 +76,12 @@
}
path_routing = {
enabled = true
- domain = "${var.domain} localhost"
- routes = local.routes
+ domain = local.domain_tokens[0]
+ allowed_domain = distinct(concat(
+ slice(local.domain_tokens, 1, length(local.domain_tokens)),
+ ["localhost"],
+ ))
+ routes = local.routes
}
},
var.debug_logging_enabled ? {
@@ -366,8 +375,10 @@
ingress {
external_enabled = false
- target_port = 8443
- transport = "http"
+ # Ingress terminates TLS and forwards HTTP. Send that to the HTTP
+ # listener (8080); 8443 is nginx's TLS port and rejects cleartext.
+ target_port = 8080
+ transport = "http"
traffic_weight {
percentage = 100You can send follow-ups to the cloud agent here.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit ac8e307. Configure here.
| path_routing = { | ||
| enabled = true | ||
| domain = "${var.domain} localhost" | ||
| routes = local.routes |
There was a problem hiding this comment.
Domain tokens corrupt package tarball URLs
High Severity
path_routing.domain is built as a space-separated string that always appends localhost, and on Azure it can include extra Front Door hostnames as well. The firewall rewrites registry metadata using that whole value, so tarball URLs become invalid and npm install fails. The Cloud Run template already treats this field as a single hostname for that reason.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ac8e307. Configure here.
| ingress { | ||
| external_enabled = false | ||
| target_port = 8443 | ||
| transport = "http" |
There was a problem hiding this comment.
Ingress protocol mismatches container port
High Severity
Container Apps ingress uses transport = "http" and target_port 8443, so Envoy forwards cleartext HTTP to the firewall's TLS listener. Client requests through the FQDN fail, while HTTPS probes still pass because they reach the container directly instead of going through ingress.
Reviewed by Cursor Bugbot for commit ac8e307. Configure here.
| # ── Ingress (internal only) ───────────────────────────────────────────── | ||
|
|
||
| ingress { | ||
| external_enabled = false |
There was a problem hiding this comment.
With internal_load_balancer_enabled = true (line 282) and external_enabled = false here, the app ingress is scoped to the Container Apps environment, not the VNet. Microsoft's ingress table for an internal environment with internal app ingress: "Reachable only from other apps in the same environment. Clients elsewhere in the virtual network receive an HTTP 404 response." No variable in the template controls external_enabled and there is no environment-level HTTP route config, so nothing re-exposes it.
Combined with the http transport to the TLS listener on 8443 that Bugbot flagged at line 370, the revision reports healthy (the HTTPS probes at lines 471-502 hit the container directly) while nobody in the VNet can reach it, including the README's "Verify the deployment" step (README lines 99-105). Bugbot's autofix only moves target_port to 8080 and keeps external_enabled = false, so it does not clear the 404.
The ingress block is unchanged from upstream socketdev-demo at 91ddad4; the same upstream commit was already imported on migrate-azure-container-apps-terraform, where f8af311 (2026-09-05, "make ingress reachable from the VNet") sets external_enabled = true and target_port = 8080 for exactly this reason. This PR does not carry it.
Repro (documented behavior, not run here): apply with the three required inputs, then from a VM in the VNet run curl -k --resolve <fqdn>:443:<environment static IP> https://<fqdn>/health (the template creates no private DNS zone for the environment's default domain, so without --resolve the name does not resolve from the VM). The ingress answers 404 while az containerapp revision list shows the revision healthy.
| routes = [for name, upstream in var.registries : { | ||
| path = "/${name}" | ||
| upstream = upstream | ||
| registry = name |
There was a problem hiding this comment.
registry = name passes the registries map key straight through as the firewall's ecosystem selector (same pattern at aws-ecs-fargate/main.tf:28 and gcp-cloud-run/main.tf:53). The config-tool dispatches on that string with no normalization and, for any value it does not recognize, emits its streaming fallback location (path_routing/__init__.py:781-783 at firewall main), whose only Lua calls are capture_trace_id(), check_bearer_token() and streaming_proxy.proxy_request(<upstream base URL>). No parser or Socket check is attached, so no SOCKET_DECISION line can be produced for the route, and the config-tool prints no warning.
The fallback also never forwards the client's path: proxy_request requests exactly the path of the URL it is handed, so every request on the route becomes GET /artifactory/api/npm/npm-remote upstream regardless of what the client asked for.
This README's own Artifactory example (lines 74-88, key "repository/npm-remote"), the comment at lines 33-40 and terraform.tfvars.example lines 38-42 all produce exactly that shape. The registry_overrides lookup from 7a55a02 on migrate-azure-container-apps-terraform handled this for the Azure module and is not in this PR; the AWS and GCP modules never had it.
Repro: run the firewall config-tool on the README's registries block and inspect the emitted location ~ ^/repository/npm-remote(/|$): it contains only the three calls above, versus npm_parser.proxy_metadata/proxy_tarball/proxy_default when the route has registry = "npm". End to end, npm install <pkg> --registry https://<fqdn>/repository/npm-remote fails to resolve the package while the deployment boots cleanly and the logs show no SOCKET_DECISION lines.
| variable "firewall_image" { | ||
| description = "Docker image for the Socket Registry Firewall" | ||
| type = string | ||
| default = "socketdev/socket-registry-firewall:latest" |
There was a problem hiding this comment.
Non-blocking. The image default is socketdev/socket-registry-firewall:latest here, in aws-ecs-fargate/variables.tf:56, gcp-cloud-run/variables.tf:21 and all three tfvars examples. The PR body notes this is unchanged, but the two templates already in this repo are pinned to 2.11.1 and chart-checks.yaml gates those pins; nothing gates terraform/, and this repo already pinned this same Azure template once for this reason (ae2dab1 on migrate-azure-container-apps-terraform, to 2.4.1).
Cloud Run resolves the tag to a digest when the revision is created; ECS does the same at the start of each service deployment (versionConsistency defaults to enabled on Fargate, which this template uses); Container Apps revisions are immutable and Microsoft advises against static tags like latest. So the running version is whatever latest pointed at when the last deployment or revision was created, and any later terraform apply that touches the task definition or template silently rolls to a different firewall build.
These templates already depend on firewall behavior that changed under them: since v1.1.345 (2026-06-26) the firewall rewrites package URLs to the configured path_routing.domain string instead of the request Host, which is what turned the two-token domain Bugbot flagged into broken tarball URLs.
Repro on ECS: curl -k https://<alb>/health returns SocketFirewall/<version> - Health OK; after a firewall release, run aws ecs update-service --force-new-deployment without changing firewall_image and curl again.
Andre Coetzee (Andre153)
left a comment
Andre Coetzee (Andre153)
left a comment
There was a problem hiding this comment.
The sizing changes are sound: fmt/validate pass, the generated socket.yml keys all reach the firewall, and the worker expressions behave on edge inputs. Before merge, the Azure template needs the ingress fix and the registry-key mapping that already exist on migrate-azure-container-apps-terraform: the first makes the deployment unreachable from the VNet, the second makes the documented Artifactory example a route that serves nothing and scans nothing. The :latest default is out of line with the rest of the repo.
This branch has not been deployed
2 participants
Summary
Moves the three per-platform Terraform templates from the
socketdev-demoorg into this repo, so every Registry Firewall deployment template lives in one place. It also aligns their default sizing with the new Sizing and High Availability docs page (SocketDev/docs#146).Move (commit 1, files unchanged):
socketdev-demo/socket-firewall-aws-ecs-fargatefb0eccbterraform/aws-ecs-fargate/socketdev-demo/socket-firewall-azure-container-apps91ddad4terraform/azure-container-apps/socketdev-demo/socket-firewall-gcp-cloud-run7126b69terraform/gcp-cloud-run/Sizing defaults (commit 2):
socket.ymlfiles now setnginx.worker_processesfrom the CPU allocation. Without it,autocounts the host's CPUs, so a 1-CPU Container Apps replica could start one worker per host core. They also setworker_connections: 4096,worker_rlimit_nofile: 8192, andsocket.api_read_timeout: 60(the default falls back to 300 s).cache.t4g.mediumor larger, and Memorystore Standard 2 GB or larger. Each note also says to use non-cluster mode andvolatile-lru.Links (commits 2 and 3):
Notes
socketdev/socket-registry-firewall:latest, as before. The release bot doesn't pin these templates.Test plan
terraform fmt -checkpasses on all three templatesterraform init -backend=false && terraform validatepasses on all three (run on throwaway copies)terraform consolegives the expected worker counts for the new expressions:cpu = 4.0(Azure)cpu = 0.5(Azure)4096CPU units (Fargate)256CPU units (Fargate)"4"(Cloud Run)"500m"(Cloud Run)CONFIG_FILEis honored by the image entrypoint (entrypoint.sh:${CONFIG_FILE:-/app/socket.yml}), so the generatednginxblock takes effect on Container Apps and Cloud Runterraform applyin a sandbox subscription or project for each platformNote
Medium Risk
Default scale and generated firewall config change what new applies provision; existing stacks only change on the next
terraform applyif they rely on defaults, with higher cost and different nginx behavior.Overview
Consolidates the AWS ECS Fargate, Azure Container Apps, and GCP Cloud Run Socket Registry Firewall Terraform modules into
terraform/under this repo, and updates the root and CloudFormation READMEs with a platform template table, sizing guidance, and cross-links between deployment options.Raises baseline sizing on all three modules to match the enterprise sizing guide: 3 replicas/instances at 4 vCPU and 8 GiB each (up from smaller prior defaults). Example
terraform.tfvarsand variable defaults are updated accordingly, with Redis tier notes added where relevant.Tightens generated
socket.ymlin each module:nginx.worker_processesis derived from allocated CPU (per-platform expression), plusworker_connections,worker_rlimit_nofile, andsocket.api_read_timeout: 60. Azure and Cloud Run liveness probe timeouts increase to 10s to avoid restarts under load. Azure addsmovedblocks for Key Vault secret resources that now usecount.Template READMEs link to each other, the Helm chart, CloudFormation, and public Socket docs instead of the old standalone repos.
Reviewed by Cursor Bugbot for commit ac8e307. Configure here.