Fix cloudflared quick tunnel startup in Docker#783
Fix cloudflared quick tunnel startup in Docker#783diegosouzapw merged 1 commit intodiegosouzapw:mainfrom
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Overview
Analysis SummaryThe PR correctly addresses the root cause described in the PR description - isolating the managed cloudflared runtime from ambient environment variables that could point it to an existing Changes reviewed:
Fix VerificationThe fix correctly isolates cloudflared by:
Files Reviewed (2 files)
|
There was a problem hiding this comment.
Code Review
This pull request implements runtime directory isolation for the cloudflared process by defining specific directories for configuration, cache, and data, and mapping them to environment variables. It also refactors the tunnel startup arguments to support protocol auto-negotiation. The review feedback suggests using the logical OR assignment operator (||=) for more concise assignment of temporary directory environment variables.
| if (!childEnv.TMPDIR) childEnv.TMPDIR = runtimeDirs.tempDir; | ||
| if (!childEnv.TMP) childEnv.TMP = runtimeDirs.tempDir; | ||
| if (!childEnv.TEMP) childEnv.TEMP = runtimeDirs.tempDir; |
There was a problem hiding this comment.
For conciseness, you can use the logical OR assignment operator (||=) to set these temporary directory environment variables. This operator assigns the right-hand side value only if the left-hand side is falsy (e.g., undefined or an empty string), which matches the current logic.
| if (!childEnv.TMPDIR) childEnv.TMPDIR = runtimeDirs.tempDir; | |
| if (!childEnv.TMP) childEnv.TMP = runtimeDirs.tempDir; | |
| if (!childEnv.TEMP) childEnv.TEMP = runtimeDirs.tempDir; | |
| childEnv.TMPDIR ||= runtimeDirs.tempDir; | |
| childEnv.TMP ||= runtimeDirs.tempDir; | |
| childEnv.TEMP ||= runtimeDirs.tempDir; |
There was a problem hiding this comment.
Pull request overview
This PR fixes Cloudflare cloudflared Quick Tunnel startup in Docker by running cloudflared under an isolated, managed runtime environment (so ambient HOME/XDG_* config can’t interfere) and by letting cloudflared auto-negotiate its protocol rather than forcing http2.
Changes:
- Add a managed runtime directory layout and ensure it exists before spawning
cloudflared. - Build a sanitized child process environment that overrides
HOMEand relevantXDG_*/Windows profile directories to point at the managed runtime. - Factor tunnel start arguments into a helper and remove the forced
--protocol http2, with unit coverage for env/args.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/lib/cloudflaredTunnel.ts |
Introduces managed runtime dirs, isolates cloudflared child env, and updates spawn args to rely on protocol auto-negotiation. |
tests/unit/cloudflaredTunnel.test.mjs |
Adds/updates unit tests covering the isolated child env and start argument construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks @rdself for this excellent contribution! 🎉 The cloudflared quick tunnel isolation fix is clean, well-tested, and solves a real Docker deployment issue. The decision to remove the hardcoded This is now merged and will be part of the next release. We appreciate your effort! |
Summary
HOME/XDG_*config so Quick Tunnel startup is not broken by existingconfig.ymlhttp2Root cause
Quick Tunnel startup can fail early when the parent process environment points cloudflared at an existing
.cloudflared/config.yml. I reproduced the failure locally with an invalid config file inHOME, which exited immediately with code 1.Validation
node --import tsx/esm --test tests/unit/cloudflaredTunnel.test.mjsnpm run typecheck:corenpx eslint src/lib/cloudflaredTunnel.ts tests/unit/cloudflaredTunnel.test.mjsstartCloudflaredTunnel()with an invalid parentHOME/.cloudflared/config.ymland confirm it still returns atrycloudflare.com/v1URL after the fix