Skip to content

Conversation

sitole
Copy link
Member

@sitole sitole commented Oct 7, 2025

  • Deploys Traefik v3.5 as a Nomad job with configuration for auto-discovery of Nomad services.
  • Ingress load balancer that will handle traffic for services behind Traefik.
  • Move "additional domains" to the security store so they can be taken from other IaC projects and correctly propagate DNS for each domain that should be routed with Traefik

Note

Deploys a Traefik-based ingress via Nomad and GCP HTTPS load balancer, and moves additional routing domains to Secret Manager with module-wide wiring.

  • Ingress (Traefik)
    • Add Nomad job ingress running traefik:v3.5 with Nomad/Consul providers; new ingress_count and ingress_port variables.
    • Provision GCP External Managed HTTPS LB for ingress: health check, backend service to api_instance_group, URL map, target HTTPS proxy, global IP/forwarding rule; reference existing certificate map.
    • Wire ingress_port across modules (variables.tf, main.tf), add API instance group named port, and open firewall for the ingress port.
  • Routing domains via secrets
    • Create Secret Manager entries routing-domains and initial version; output routing_domains_secret_name.
    • Read and merge secret-stored domains with env additional_domains in root module and pass downstream.
  • Misc
    • Makefile: propagate INGRESS_COUNT TF var.

Written by Cursor Bugbot for commit 9e24600. This will update automatically on new commits. Configure here.

@sitole sitole added the improvement Improvement for current functionality label Oct 7, 2025
Copy link

linear bot commented Oct 7, 2025

@sitole
Copy link
Member Author

sitole commented Oct 7, 2025

This is a prerequisite for https://github.com/e2b-dev/belt/pull/217 that shows exposing the service via an ingress service.

@sitole sitole marked this pull request as ready for review October 7, 2025 15:42
cursor[bot]

This comment was marked as outdated.

}
}

resource "google_compute_url_map" "ingress" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get a better name here? I think we now have two load balancers, "ingress" and "orch_map", neither of their names help understand what they do. Maybe "traefik" and "direct"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like ingress as its common name and it describes what it is. Yes, "orch_map" is, in my opinion, a mistake, as it no longer makes sense. Ideally, I would like to transition away from the current load balancer once the migration is complete/rename it to something like "ingress-sandboxes" or a similar name to distinguish better.

I don't like to call it Traefik, as we can switch the ingress backend at any time in the future, but I'm okay with you coming up with a better name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a note here, if we want to rename the orch_map to ingress-sandboxes, maybe we should name this ingress something like ingress-api or ingress-management or ingress-services

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don’t like that we would need two load balancers just because we cannot filter sandbox traffic. Will look into again tomorrow

Yep, we can rename ingress to something else. Iam not sure about management/api as we can use it for something different in future. Ingress services sounds okay to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though it's actually quite nice to have separate LBs for user's sandbox traffic and our services traffic (different limitations, limits, HTTP support, etc), but maybe it's unnecessary

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we should be able to match sandbox traffic to different rules (now it's catch-all fallback) so we can apply different limits/armor rules to them, then we don't need to have different LBs.

For supporting newer versions of HTTP, etc, we can still relatively easily migrate everything, and I'm not sure if we would need some special LB that cannot handle both sandbox and services traffic.

@sitole sitole requested a review from djeebus October 8, 2025 09:11
@jakubno jakubno self-assigned this Oct 8, 2025
Copy link
Contributor

@dobrac dobrac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to confirm, this doesn't route any traffic yet and it's just a preparation?

Comment on lines +76 to +77
cpu_count = 1
memory_mb = 512
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these enough for our traffic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, more than enough for now. When we proceed and start routing sandbox traffic, we need to revisit this, with memory ideally set to 1024 min and 2048 max.

$(call tfvar, GCP_REGION) \
$(call tfvar, GCP_ZONE) \
$(call tfvar, DOMAIN_NAME) \
$(call tfvar, ADDITIONAL_DOMAINS) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note here, this is a breaking change and should be handled with proper migration path

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any? This type transformation @jakubno we handles manually.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I can push attribute before this pr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we maybe do the secret default value from the env var? Is that a bad idea?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imho during migration period we can merge server + tf var lists.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added compatibility during the migration period that will accept both env and secret and merge them together.

"--providers.consulcatalog=true",
"--providers.consulcatalog.exposedByDefault=false",
"--providers.consulcatalog.endpoint.address=${consul_endpoint}",
"--providers.consulcatalog.endpoint.token=${consul_token}",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Token Exposure and Network Resolution Issues

Sensitive Nomad and Consul tokens are passed as command-line arguments to the Traefik container, exposing them in process lists and logs. Additionally, if the nomad_endpoint is configured as localhost, it may not resolve correctly from within the container using host networking, potentially causing service discovery failures.

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement for current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants