From a531f5e42a18e67501a0226ca09452235f932795 Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Wed, 1 Jul 2026 15:31:58 +0900 Subject: [PATCH] fix(cfsetup): keep connector install in the host apply layer only Setup previously called runner.StartConnector (`cloudflared service install`) while the TUI applyCloudflareEdge separately installs cloudflared-felis.service for the same tunnel from the same config -- two managed services serving one tunnel from one connector config. Drop StartConnector from cfsetup: running a connector is a host-specific side effect (systemd/launchd/Windows service) that belongs with the caller, not in this host- and domain-agnostic package whose documented side effects are tunnel creation, DNS routing, and the Access app/policy calls. installCloudflaredService in the host layer stays the single connector installer, so the routed-but-dead 1033 is still closed; RouteDNS --overwrite-dns still closes the stale-DNS 1033. --- internal/cfsetup/cfsetup.go | 23 ++++------- internal/cfsetup/cfsetup_test.go | 70 -------------------------------- internal/cfsetup/runner.go | 24 ----------- 3 files changed, 7 insertions(+), 110 deletions(-) diff --git a/internal/cfsetup/cfsetup.go b/internal/cfsetup/cfsetup.go index 3c65f93..15718a3 100644 --- a/internal/cfsetup/cfsetup.go +++ b/internal/cfsetup/cfsetup.go @@ -339,13 +339,6 @@ type Runner interface { RouteDNS(ctx context.Context, tunnelID, hostname string) error // WriteTunnelConfig persists the rendered config.yml. WriteTunnelConfig(path string, contents []byte) error - // StartConnector installs and starts the local cloudflared connector bound to - // the written config so the tunnel actually has a running process serving it. - // Without this step the tunnel is created and the DNS is routed, but nothing - // runs the config — so every routed hostname returns Cloudflare error 1033 - // (tunnel has no connector), the other half of the 1033 failure mode that - // RouteDNS's --overwrite-dns closes. - StartConnector(ctx context.Context, configPath string) error // CreateAccessApplication creates the self-hosted Access app and returns its // id and the issued JWT `aud` (which felis [auth] access_jwt_aud must adopt). CreateAccessApplication(ctx context.Context, app AccessApplication) (appID, aud string, err error) @@ -460,15 +453,13 @@ func Setup(ctx context.Context, runner Runner, p Params) (*Result, error) { return nil, fmt.Errorf("cfsetup: write config: %w", err) } prog = append(prog, "Wrote "+p.ConfigPath) - // 6b. Install and start the connector for the config just written, so the - // tunnel is actually served rather than routed-but-dead (error 1033). - // Guarded by ConfigPath: with no config there is nothing to run, and a - // caller wanting only the Access config is not forced to install a service. - notify("Starting tunnel connector…") - if err := runner.StartConnector(ctx, p.ConfigPath); err != nil { - return nil, fmt.Errorf("cfsetup: start connector: %w", err) - } - prog = append(prog, "Started connector") + // Setup stops at writing the config: RUNNING a connector for it is a + // host-specific side effect (systemd/launchd/Windows service) that lives + // with the caller, not in this host-agnostic package. The felis TUI does it + // right after Setup returns (installCloudflaredService in tui_edge_apply.go), + // closing the routed-but-dead 1033 the same way RouteDNS's --overwrite-dns + // closes the stale-DNS 1033. A caller that skips that step gets a routed + // tunnel with no connector — Cloudflare error 1033 — by its own choice. } // 7. Front the admin face with a self-hosted Access app. notify("Creating Access application…") diff --git a/internal/cfsetup/cfsetup_test.go b/internal/cfsetup/cfsetup_test.go index 38af91b..3d3b0e2 100644 --- a/internal/cfsetup/cfsetup_test.go +++ b/internal/cfsetup/cfsetup_test.go @@ -69,11 +69,6 @@ func (r *recordingRunner) WriteTunnelConfig(path string, _ []byte) error { return nil } -func (r *recordingRunner) StartConnector(_ context.Context, configPath string) error { - r.calls = append(r.calls, "StartConnector:"+configPath) - return nil -} - func (r *recordingRunner) CreateAccessApplication(_ context.Context, app AccessApplication) (string, string, error) { r.calls = append(r.calls, "CreateAccessApplication:"+app.Domain) appID := r.appID @@ -336,71 +331,6 @@ func TestSetupSucceedsAndReportsAud(t *testing.T) { } } -// TestSetupStartsConnectorAfterWritingConfig pins the anti-1033 invariant, not a -// call order for its own sake: a successful Setup that wrote a config MUST also start -// a connector for it (a routed tunnel with no connector returns Cloudflare error -// 1033), and it must do so only AFTER the config exists (starting a connector for an -// unwritten config would serve nothing). This is the orchestration half of the fix; -// the actual `cloudflared service install` is INTEGRATION-ONLY (runner.go). -func TestSetupStartsConnectorAfterWritingConfig(t *testing.T) { - const cfgPath = "/etc/felis/cloudflared.yml" - runner := &recordingRunner{} - p := Params{ - PanelHostname: "console." + testRoot, - AdminHostname: "op.console." + testRoot, - TunnelName: "felis", - ConfigPath: cfgPath, - AccessIdentity: AccessIdentity{Emails: []string{"owner@example.net"}}, - Pre: goodPreconditions(), - } - if _, err := Setup(context.Background(), runner, p); err != nil { - t.Fatalf("Setup: %v", err) - } - write := indexOfCall(runner.calls, "WriteTunnelConfig:"+cfgPath) - start := indexOfCall(runner.calls, "StartConnector:"+cfgPath) - if write < 0 { - t.Fatalf("config was never written: %v", runner.calls) - } - if start < 0 { - t.Fatalf("connector was never started — a routed tunnel with no connector returns error 1033: %v", runner.calls) - } - if start < write { - t.Fatalf("connector started before its config was written (would serve nothing): %v", runner.calls) - } -} - -// TestSetupSkipsConnectorWhenNoConfigPath proves the connector step is gated on a -// written config: with ConfigPath unset the caller wants only the Access config, so -// no service is installed (and no config is written to install one around). -func TestSetupSkipsConnectorWhenNoConfigPath(t *testing.T) { - runner := &recordingRunner{} - p := Params{ - PanelHostname: "console." + testRoot, - AdminHostname: "op.console." + testRoot, - TunnelName: "felis", - AccessIdentity: AccessIdentity{Emails: []string{"owner@example.net"}}, - Pre: goodPreconditions(), - } - if _, err := Setup(context.Background(), runner, p); err != nil { - t.Fatalf("Setup: %v", err) - } - for _, c := range runner.calls { - if len(c) >= len("StartConnector") && c[:len("StartConnector")] == "StartConnector" { - t.Fatalf("connector was started with no config path to serve: %v", runner.calls) - } - } -} - -// indexOfCall returns the position of want in calls, or -1. -func indexOfCall(calls []string, want string) int { - for i, c := range calls { - if c == want { - return i - } - } - return -1 -} - // TestIngressSafetyInvariants parses the generated cloudflared config back and // asserts the properties that protect the origin: the final rule is the // hostname-less fail-shut 404 catch-all, every routed hostname points at the panel diff --git a/internal/cfsetup/runner.go b/internal/cfsetup/runner.go index 8d4074a..ee86ddf 100644 --- a/internal/cfsetup/runner.go +++ b/internal/cfsetup/runner.go @@ -171,30 +171,6 @@ func (r *ExecRunner) WriteTunnelConfig(path string, contents []byte) error { return os.WriteFile(path, contents, 0o644) } -// StartConnector installs and starts the cloudflared connector as a managed system -// service bound to configPath (`cloudflared --config service install`), -// so the tunnel written by WriteTunnelConfig has a running process serving it. On -// Linux this installs and starts a systemd unit; on macOS a launchd agent; on -// Windows a service. It is the step that turns a routed-but-dead tunnel (Cloudflare -// error 1033) into a reachable one, and it runs as the operator (root under the -// break-glass TUI) since installing a system service requires it. -// -// It is idempotent on re-run: an already-installed service is reported by cloudflared -// and treated as success rather than failing the whole setup. Re-applying a CHANGED -// config to an already-installed service would need a restart this method does not -// perform — a caveat noted honestly. INTEGRATION-ONLY. -func (r *ExecRunner) StartConnector(ctx context.Context, configPath string) error { - if strings.TrimSpace(configPath) == "" { - return fmt.Errorf("cfsetup: connector config path is required") - } - // The global --config flag must precede the `service install` subcommand. - _, err := r.runCloudflared(ctx, "--config", configPath, "service", "install") - if err != nil && strings.Contains(err.Error(), "already installed") { - return nil - } - return err -} - // CreateAccessApplication POSTs the self-hosted Access app and returns its id and // issued aud (spec §14: the aud felis [auth] access_jwt_aud must adopt). func (r *ExecRunner) CreateAccessApplication(ctx context.Context, app AccessApplication) (string, string, error) {