Skip to content

chore(infra): route electric through ingest's ALB instead of its own - #1266

Open
Makisuo wants to merge 1 commit into
mainfrom
chore/electric-shares-ingest-alb
Open

Makisuo wants to merge 1 commit into
mainfrom
chore/electric-shares-ingest-alb

Conversation

@Makisuo

@Makisuo Makisuo commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

What

Electric stops owning an ALB. In each region, ingest's ALB now serves both:

  • apps/ingest/alchemy.run.ts composes the ALB and HTTPS listener itself (previously owned implicitly by the ECS service via public: true). Ingest's rule is the catch-all at priority 50000.
  • apps/electric/alchemy.run.ts attaches a Host: electric[.eu].maple.dev rule at priority 10 plus its own certificate via ELBv2.ListenerCertificate (SNI). Its ALB security group is removed; the task group now admits only the shared ALB's group.
  • alchemy.run.ts passes the listener and ALB group through, and only creates electric when domains.electric is set (prd only, which always has it).

The listener uses certificateArn, not certificates: the declarative list would strip electric's ListenerCertificate on every ingest reconcile.

Why

Over 3 days the US electric ALB served ~29k requests and the EU one ~2k, versus ~45M on the US ingest ALB. Each dedicated ALB costs $16-20/mo in hourly charges alone, so this saves roughly $36/mo across both regions (out of a ~$500/mo AWS bill).

Deploy notes

  • Ingest's ALB is replaced. publishProxiedCname repoints ingest.maple.dev / ingest.eu.maple.dev at the new ALB in the same deploy; the old one is reaped afterwards. Clients only ever see the proxied hostname.
  • New target groups for both services. An ALB target group can belong to only one load balancer, so the rules use an explicit forward (new logical ids) instead of reusing the owned-ALB ones. Expect one rolling deploy of ingest (same path as the Sep 21 EC2 rollout) and electric's usual ~60s singleton gap.
  • Electric is now Cloudflare-only at the network layer. Its old ALB admitted 0.0.0.0/0; the shared one admits only Cloudflare ranges. electric-sync uses https://electric.maple.dev through the proxy, so it is unaffected.
  • Possible retry: deleting electric's old security group can hit DependencyViolation while the old task ENIs detach. Re-running the deploy finishes it.

Verification

  • tsc -p tsconfig.alchemy.json passes; oxfmt clean.
  • Not deployed. After the prd deploy: curl https://electric.maple.dev/v1/health (and the EU host), a shape through electric-sync, and an OTLP POST to both ingest hosts.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Devin Review

Summary by CodeRabbit

  • New Features
    • Ingest and Electric traffic now routes through a shared load balancer, with requests directed to the appropriate service based on hostname.
    • Ingest uses HTTPS when a domain and certificate are configured; otherwise, it uses HTTP.
    • Electric’s hostname is published as a proxied CNAME.

Each region ran a dedicated ALB for electric that served a few thousand
requests a day, costing about $16-20/mo per region for its hourly charge
alone. Ingest now composes its ALB and listener explicitly; electric
attaches a host rule (priority 10) and its own SNI certificate to that
listener, while ingest keeps the catch-all (priority 50000).

Both services move to new target groups, since an ALB target group can
only belong to one load balancer: the first deploy rolls ingest and gives
electric its usual ~60s gap. Electric's ALB security group goes away; the
shared ALB admits only Cloudflare, which is how electric-sync reaches it.
@maple-review-bot

maple-review-bot Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Maple review

🟢 Confidence 4/5 · likely safe to merge
service.url must still resolve without public: true, since both hostnames' CNAMEs come from it and the vendored alchemy source cannot confirm it.
quality 98/100 · 1 note · tests not needed · risk high · 2/2 new units observable

Moves both regional ingress paths onto ingest's single ALB: ingest owns the load balancer and listener, electric attaches a host rule and its own SNI certificate. The wiring reads correctly and the doc drift is the only defect I could confirm; the alchemy assumptions are unverifiable here.

  • createMapleIngest now owns AWS.ELBv2.LoadBalancer/Listener (ingest-lb, ingest-listener) and forwards to ingest at priority 50000
  • createMapleElectric takes ingest's listener, albSecurityGroupId and a required hostname, dropping public: true and its own ALB group
  • alchemy.run.ts creates electric only when domains.electric is set, and does not deploy it in previews or dev

Findings

🔵 Note · F1 · Runbook this file points to still documents electric's own ALB and group

maintainability · apps/electric/alchemy.run.ts:33

The header comment still points at docs/electric-sync.md as the runbook, but that doc (lines 176-179) still describes Electric "with its own cluster, ALB, security groups and certificate inside the ingest fleet's VPC", and docs/infra.md's ECS.Service pitfall note still explains the certificateArn/listenerPort pairing this diff removes. The next operator or on-call following it will reason about ALBs and a security group that no longer exist, including the manual electric-lb cleanup the deploy notes call out. Update those two sections with the shared listener, the host rule and the Cloudflare-only group.

🤖 Prompt to fix this finding with an AI agent
Findings from an automated review of commit 53fb08b9bfc5d871a92d25a9b2777e9bc3f260f6. Verify each one against the current code before changing anything, fix only those that still apply, and keep each fix to the lines it names.

---

F1 · Note · maintainability · apps/electric/alchemy.run.ts:33
Runbook this file points to still documents electric's own ALB and group
The header comment still points at `docs/electric-sync.md` as the runbook, but that doc (lines 176-179) still describes Electric "with its own cluster, ALB, security groups and certificate inside the ingest fleet's VPC", and `docs/infra.md`'s `ECS.Service` pitfall note still explains the `certificateArn`/`listenerPort` pairing this diff removes. The next operator or on-call following it will reason about ALBs and a security group that no longer exist, including the manual `electric-lb` cleanup the deploy notes call out. Update those two sections with the shared listener, the host rule and the Cloudflare-only group.
What was checked
  • Electric's rule (priority 10) still wins over ingest's catch-all (apps/ingest/alchemy.run.ts:510), and every host reaching the 443 listener still forwards to ingest
  • domains.electric and domains.ingest are set together for prd and prd-eu only, so the HTTP-80 listener path never carries electric's hostname
  • Both ingest and electric synced to Maple in the last 6h, so the shared ALB's path stays observable
Observability coverage: 2 of 2 changes observable
Change Kind Observable Evidence
ingest ALB listener + electric host rule (shared ingress) load balancer entrypoint yes ALB is infrastructure in front of already-instrumented apps; ingest (247.8k req) and electric-sync reported spans in the last 6h
cloudflare CNAMEs for ingest and electric hostnames DNS wiring yes Traffic still arrives at the same instrumented services through the proxied hostnames

53fb08b · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d2ce5e04-1bc4-4995-87c9-460c13861d8e
📥 Commits

Reviewing files that changed from the base of the PR and between 8e2e9a4 and 53fb08b.

📒 Files selected for processing (3)
  • alchemy.run.ts
  • apps/electric/alchemy.run.ts
  • apps/ingest/alchemy.run.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

Ingest now creates and exposes an ALB listener and security-group ID. When an Electric domain is configured, Electric uses that listener for hostname-based routing and publishes a proxied CNAME for the hostname.

Changes

Shared ALB routing

Layer / File(s) Summary
Create and expose the ingest listener
apps/ingest/alchemy.run.ts
Ingest creates an internet-facing ALB listener, routes its ECS service through a target group, and returns the listener and ALB security-group ID.
Route Electric through the shared listener
apps/electric/alchemy.run.ts
Electric uses the shared listener and security group. It adds a hostname rule, attaches an issued certificate when available, and publishes a proxied CNAME for the hostname.
Pass the shared listener to Electric
alchemy.run.ts
Electric creation requires domains.electric and receives that hostname, the ingest listener, and the ALB security-group ID.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Other

Sequence Diagram(s)

sequenceDiagram
  participant Stack as Maple stack
  participant Ingest as Ingest ALB listener
  participant Electric as Electric ECS service
  participant DNS
  Stack->>Ingest: Create listener and ingest target-group rule
  Ingest-->>Stack: Return listener and ALB security-group ID
  Stack->>Electric: Pass listener, security group, and hostname
  Ingest->>Electric: Forward hostname-matched traffic
  Stack->>DNS: Publish proxied CNAME for hostname
Loading

Suggested reviewers: jeremyfunk

Merge Risk: ⚪ Minimal · up to 53fb0

The shared ingress configuration has no established merge-blocking issue. Proceed with the planned deployment and post-deployment health checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 53fb0

The change narrows Electric’s network exposure and preserves its configured application secret and database authority. It also couples both services to one regional ingress boundary. Recovery from an interrupted load-balancer and DNS migration remains unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The shared ingress boundary covers both services in each configured production region, rather than one service per ALB. This broadens infrastructure failure scope but does not, in the compared definitions, broaden Electric’s database authority or move its database credentials into the shared ingress component.

Security Findings and Attack Paths

  • observed — The base already exposes Electric publicly and configures ELECTRIC_SECRET as its application control. The head removes the dedicated ALB’s 0.0.0.0/0 admission and preserves the secret injection. The source comparison therefore supports narrower origin reachability, not an introduced authentication bypass; running-image enforcement was not verified.

Trust Boundaries and Controls

  • observed — Cloudflare network admission and ALB hostname routing are reachability controls, not tenant authorization. Electric continues to receive its application secret and SSL-configured database connection through task-side secrets. Its root-created replication role and inherited database role are unchanged by this PR.

Resilience and Maintainability Implications

  • observed — Certificate ownership is explicitly split between ingest’s default listener certificate and Electric’s additional SNI attachment. Current production domain maps supply both hostnames, avoiding the unsupported combination of an HTTP ingest listener with an Electric certificate attachment.

Hardening Proposals

  • proposed — Before production cutover, establish recovery behavior for failures during target-group migration, certificate attachment, and DNS publication. Verify that live ingress remains recoverable until new targets are healthy, and that disabling or renaming Electric removes its obsolete route, certificate attachment, and DNS record without affecting ingest.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: routing Electric through Ingest’s ALB instead of using a dedicated ALB.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

Comment on lines +159 to +163
yield* publishProxiedCname({
id: "electric-public-cname",
hostname,
serviceUrl: service.url,
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Electric DNS outruns its TLS certificate

During ALB replacement, publishProxiedCname can repoint Electric before ListenerCertificate attaches its certificate. Both depend on listener, but neither the service nor DNS record depends on ListenerCertificate. Electric requests then fail TLS until attachment finishes.

Learn more

Alchemy orders resources through the Outputs referenced in their properties, not the order in which the factory yields them. The Electric certificate attachment and the ECS service both reference the shared listener, but the CNAME references only the service URL. On the initial ALB migration, the service can become ready and update DNS while the listener still offers only ingest's default certificate. Cloudflare connects to the Electric hostname over TLS and rejects the wrong origin certificate until the attachment completes.

Example: On a production redeploy, the new ALB serves the Electric target group at 12:00:00 and the Electric CNAME changes at 12:00:01. If the SNI certificate attaches at 12:00:15, requests to electric.maple.dev fail during those 14 seconds instead of continuing to reach the old ALB.

Recommended fix: Carry an Output from AWS.ELBv2.ListenerCertificate into the DNS record's dependency chain so the CNAME cannot reconcile before SNI attachment. Keep the certificate on the listener before switching the public name; validate with an initial ALB replacement plan.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant