From 403bb68882b5c4b90467bf1a78ad159987b9c18e Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Mon, 10 Aug 2026 14:49:12 -0400 Subject: [PATCH] Suggest `cortex serve` when a context named "cortex" refuses the connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A user who points a context at a local `rossoctl cortex serve` and forgets to start it gets only the transport error: "dial tcp [::1]:9097: connect: connection refused". That is accurate and says nothing about the remedy, which in this one case the CLI can name, because `cortex serve` is the only server rossoctl can start itself. The hint goes in errorHint, the single place every command's error is already printed from Execute, so it covers agents, tools, status, envvars, and anything added later rather than being repeated at the eleven call sites that reach a client. The condition is errors.Is(err, syscall.ECONNREFUSED), not the words "connection refused" in the message. apiclient wraps the dial failure with %w, so the errno survives in the chain, and matching the syscall distinguishes a refused connection from a timeout or an unresolvable host — neither of which starting a local server would fix, and the second of which is the likelier typo. Text matching would also have broken an existing test that requires a bare errors.New("connection refused") to produce no hint; that case still passes, and a new test pins the DNS-failure distinction by resolving an RFC 2606 .invalid name. The hint fires only for a context named "cortex". Everywhere else the advice would be misdirection: a production API that is down is not fixed by running a local server, and saying so would send the user after the wrong problem. That a name is the marker is a convention rather than a guarantee, which is why this is a hint appended to the real error and never a replacement for it. An explicit --server suppresses it, since --server overrides every context and the current context's name then says nothing about what was actually dialed. The request was originally to key on config.TypeCortex, and that constant is removed here instead. It could not work: the type was declared but never set. All four context-creating sites hardcode TypeAPI, and create-context has no --type flag, so no condition keyed on it could ever have fired. Type is an unvalidated bare string, so a config.yaml written when "cortex" existed still parses and carries the value as-is — no migration, and a test keeps a literal "cortex" string to pin that such a context still routes to the HTTP client. What a local cortex is, is an ordinary HTTP server reached by pointing a context at its address, which "api" already describes. Reading the context to check its name can itself fail. It happens while an error is already being reported, so a config that will not load leaves the hint silent rather than replacing the failure the user is trying to read; a test writes a malformed config to require that. The ECONNREFUSED test chains come from a real dial to a closed port, wrapped the way apiclient wraps it, rather than from a constructed syscall.Errno. A fake errno would keep passing if net/http stopped surfacing the syscall, which is precisely the change that would silently disable this hint in production. `rossoctl login` still gets no hint: fetchAuthConfigForLogin wraps with %v, severing the chain. That is pre-existing, deliberate at that site for the 401 circularity its comment describes, and left alone here. Verified end to end. With a context named cortex and nothing listening, the raw error is followed by the hint; with a context named prod and nothing listening, there is no hint; with `cortex serve` running, the same command succeeds and the hint is gone. go build, go vet, gofmt, and the full test suite pass. staticcheck was not run, as the installed build is Go 1.25 and this module requires 1.26.4. Neither README documents the context type field, so no documentation changed. Assisted by Claude. Signed-off-by: Ed Snible --- cmd/root.go | 69 ++++++++- cmd/root_test.go | 138 ++++++++++++++++++ cmd/ui_test.go | 2 +- internal/config/config.go | 23 +-- internal/rossoctlclient/rossoctlclient.go | 2 +- .../rossoctlclient/rossoctlclient_test.go | 18 ++- 6 files changed, 224 insertions(+), 28 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index e357ca1..72a9446 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -12,6 +12,7 @@ import ( "fmt" "net/http" "os" + "syscall" "github.com/spf13/cobra" @@ -30,6 +31,15 @@ var ( // defaultServer is the API endpoint used when --server is not supplied. const defaultServer = "http://rossoctl-ui.localtest.me:8080/api/v1/" +// cortexContextName is the context name that marks a context as pointing at a +// local `rossoctl cortex serve`, which is the one server rossoctl can start +// itself. It is the trigger for the hint in connectionRefusedHint. +// +// A name is the marker because nothing else distinguishes such a context: a +// cortex is reached over HTTP like any other server, and the config's type field +// is "api" for every context every command creates. +const cortexContextName = "cortex" + // Persistent flags shared by every command. var ( verbose bool @@ -210,21 +220,64 @@ func Execute() { // each call site: a 401 can come from any command that reaches the API, and // eleven files call one of these clients. Adding it here covers agents, tools, // status, envvars, and anything added later for free. +// Two shapes of failure carry a remedy: a response the server sent (a +// StatusError) and never reaching the server at all (a refused connection). func errorHint(err error) string { var statusErr *apiclient.StatusError - if !errors.As(err, &statusErr) { + if errors.As(err, &statusErr) { + switch statusErr.StatusCode { + case http.StatusUnauthorized: + // Deliberately not suggested for 403: that is an authenticated + // identity lacking permission, where signing in again changes + // nothing and the advice would send the user in a circle. + return "Hint: the server rejected the credentials. Run `rossoctl login` to sign in." + default: + return "" + } + } + + if hint := connectionRefusedHint(err); hint != "" { + return hint + } + return "" +} + +// connectionRefusedHint suggests starting the local API when the context that +// could not be reached is the one that names it. +// +// The test is the syscall, via errors.Is, not the words "connection refused" in +// the message. apiclient wraps the dial failure with %w, so the errno survives +// in the chain, and matching on it distinguishes a refused connection from a +// timeout or an unresolvable host — neither of which a local server would fix. +// It also leaves an unrelated error that merely says "connection refused" alone. +// +// The suggestion is offered only for a context named "cortex". `cortex serve` is +// the one server rossoctl can start itself, so it is a real remedy there and +// misdirection anywhere else: a production API that is down is not fixed by +// running a local server, and saying so would send the user after the wrong +// problem. Naming the context is a convention rather than a guarantee, which is +// why this is a hint appended to the real error rather than a replacement for it. +func connectionRefusedHint(err error) string { + if !errors.Is(err, syscall.ECONNREFUSED) { return "" } - switch statusErr.StatusCode { - case http.StatusUnauthorized: - // Deliberately not suggested for 403: that is an authenticated - // identity lacking permission, where signing in again changes - // nothing and the advice would send the user in a circle. - return "Hint: the server rejected the credentials. Run `rossoctl login` to sign in." - default: + // An explicit --server overrides every context (see resolveServer), so the + // current context's name says nothing about what was actually dialed. + if server != "" { return "" } + + // Read the context defensively. This runs while an error is already being + // reported, so a config that cannot be loaded must leave the hint silent + // rather than replace the failure the user is trying to read. + ctx, ctxErr := resolveContext() + if ctxErr != nil || ctx == nil || ctx.Name != cortexContextName { + return "" + } + + return "Hint: nothing is listening at " + ctx.Server + + ". Run `rossoctl cortex serve` to start the local API." } func init() { diff --git a/cmd/root_test.go b/cmd/root_test.go index f984222..b2c8e4b 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "strings" + "syscall" "testing" "time" @@ -15,6 +16,7 @@ import ( "github.com/spf13/pflag" "github.com/rossoctl/rossoctl-cli/internal/apiclient" + "github.com/rossoctl/rossoctl-cli/internal/config" ) // TestMain isolates HOME to a throwaway directory for the whole cmd test @@ -303,3 +305,139 @@ func TestErrorHintQuietForOtherErrors(t *testing.T) { }) } } + +// refusedError produces the error chain a real dial failure yields, by asking +// net/http for a port nothing is listening on and wrapping it the way +// apiclient.doJSON does. +// +// It is built from a real connection rather than a hand-made syscall.Errno so the +// test proves errorHint matches the chain the client actually produces. A +// constructed errno would pass even if net/http stopped surfacing the syscall. +func refusedError(t *testing.T) error { + t.Helper() + + // Port 1 on loopback: privileged, so nothing is listening, and the connection + // is refused rather than timing out as a filtered address would. + _, err := (&http.Client{Timeout: 5 * time.Second}).Get("http://127.0.0.1:1/api/v1/agents") + if err == nil { + t.Fatal("expected a dial failure against 127.0.0.1:1") + } + if !errors.Is(err, syscall.ECONNREFUSED) { + t.Fatalf("test precondition: %v is not ECONNREFUSED", err) + } + return fmt.Errorf("requesting %s: %w", "http://127.0.0.1:1/api/v1/agents", err) +} + +// seedContext writes a single named context and makes it current, bypassing +// create-context so the name is exactly as given. +func seedContext(t *testing.T, name, server string) { + t.Helper() + path := isolateHome(t) + cfg, err := config.Load(path) + if err != nil { + t.Fatalf("load config: %v", err) + } + cfg.Upsert(config.Context{Name: name, Type: config.TypeAPI, Server: server}) + if err := cfg.SetCurrent(name); err != nil { + t.Fatalf("set current: %v", err) + } + if err := cfg.Save(); err != nil { + t.Fatalf("save config: %v", err) + } +} + +// TestErrorHintSuggestsCortexServeOnRefusedConnection is the case this hint +// exists for: a context pointed at a local `cortex serve` that was never started. +func TestErrorHintSuggestsCortexServeOnRefusedConnection(t *testing.T) { + resetFlags(rootCmd) + seedContext(t, "cortex", "http://localhost:9097/api/v1/") + + hint := errorHint(refusedError(t)) + if hint == "" { + t.Fatal("a refused connection on the cortex context should produce a hint") + } + if !strings.Contains(hint, "rossoctl cortex serve") { + t.Errorf("hint %q should name `rossoctl cortex serve`", hint) + } + // The server is named so the user can see which address was unreachable. + if !strings.Contains(hint, "http://localhost:9097/api/v1/") { + t.Errorf("hint %q should name the context's server", hint) + } +} + +// TestErrorHintQuietForRefusedConnectionOnOtherContexts keeps the suggestion from +// becoming misdirection. A production API that is down is not fixed by starting a +// local server, so only the context that names one gets the advice. +func TestErrorHintQuietForRefusedConnectionOnOtherContexts(t *testing.T) { + resetFlags(rootCmd) + seedContext(t, "prod", "http://rossoctl.example.com/api/v1/") + + if hint := errorHint(refusedError(t)); hint != "" { + t.Errorf("errorHint on a non-cortex context = %q, want no hint", hint) + } +} + +// TestErrorHintQuietForRefusedConnectionWithExplicitServer covers --server, which +// overrides every context: the current context's name then says nothing about +// what was actually dialed. +func TestErrorHintQuietForRefusedConnectionWithExplicitServer(t *testing.T) { + resetFlags(rootCmd) + seedContext(t, "cortex", "http://localhost:9097/api/v1/") + + server = "http://elsewhere.example.com/api/v1/" + t.Cleanup(func() { server = "" }) + + if hint := errorHint(refusedError(t)); hint != "" { + t.Errorf("errorHint with an explicit --server = %q, want no hint", hint) + } +} + +// TestErrorHintDistinguishesRefusedFromOtherTransportFailures pins the choice of +// errors.Is over message matching. An unresolvable host is not a refused +// connection, and starting a local server would not fix it. +func TestErrorHintDistinguishesRefusedFromOtherTransportFailures(t *testing.T) { + resetFlags(rootCmd) + seedContext(t, "cortex", "http://localhost:9097/api/v1/") + + // .invalid is reserved by RFC 2606 and never resolves. + _, dnsErr := (&http.Client{Timeout: 5 * time.Second}).Get("http://nonexistent.invalid./x") + if dnsErr == nil { + t.Skip("this network resolves nonexistent.invalid.; cannot test a DNS failure") + } + wrapped := fmt.Errorf("requesting %s: %w", "http://nonexistent.invalid./x", dnsErr) + + if hint := errorHint(wrapped); hint != "" { + t.Errorf("errorHint on a DNS failure = %q, want no hint", hint) + } +} + +// TestErrorHintRefusedSurvivesWrapping verifies the hint survives the extra +// context commands add on the way up, as the 401 hint does. +func TestErrorHintRefusedSurvivesWrapping(t *testing.T) { + resetFlags(rootCmd) + seedContext(t, "cortex", "http://localhost:9097/api/v1/") + + wrapped := fmt.Errorf("listing agents in namespace %q: %w", "team1", refusedError(t)) + if hint := errorHint(wrapped); hint == "" { + t.Error("a wrapped refused connection should still produce a hint") + } +} + +// TestErrorHintQuietWhenConfigIsUnreadable covers the defensive context lookup: +// errorHint runs while an error is already being reported, so a broken config +// must leave the hint silent rather than obscure the real failure. +func TestErrorHintQuietWhenConfigIsUnreadable(t *testing.T) { + resetFlags(rootCmd) + path := isolateHome(t) + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(path, []byte("{{ not yaml"), 0o600); err != nil { + t.Fatalf("write bad config: %v", err) + } + + // Must not panic, and must not produce a hint. + if hint := errorHint(refusedError(t)); hint != "" { + t.Errorf("errorHint with an unreadable config = %q, want no hint", hint) + } +} diff --git a/cmd/ui_test.go b/cmd/ui_test.go index 862be84..b5a9ad8 100644 --- a/cmd/ui_test.go +++ b/cmd/ui_test.go @@ -51,7 +51,7 @@ func TestUIOpenBlankServerFails(t *testing.T) { if err != nil { t.Fatalf("load config: %v", err) } - cfg.Upsert(config.Context{Name: "empty", Type: config.TypeCortex}) + cfg.Upsert(config.Context{Name: "empty", Type: config.TypeAPI}) if err := cfg.SetCurrent("empty"); err != nil { t.Fatalf("set current: %v", err) } diff --git a/internal/config/config.go b/internal/config/config.go index 8e4308f..1778a8d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -23,21 +23,24 @@ const ( filePerm os.FileMode = 0o600 ) -// Type records what kind of server a Context targets: a hosted Rossoctl API -// ("api") or a local Cortex ("cortex"). +// Type records what kind of server a Context targets. Every context is reached +// the same way, over HTTP at its server URI, so the type does not select a +// client implementation — it is a label describing what is at the other end. // -// Both are reached the same way, over HTTP at the context's server URI, so the -// type does not select a client implementation — it is a label describing what -// is at the other end. +// There was once a second value, "cortex", for a context served by `rossoctl +// cortex serve`. Nothing ever set it: every context-creating path assigned +// "api", so no condition keyed on it could fire. A local cortex is an ordinary +// HTTP server reached by pointing a context at its address, which "api" already +// describes. +// +// Type is not validated on load, so a config file written when "cortex" existed +// still parses; the value is simply carried as-is. type Type string const ( - // TypeAPI is a context served by the Rossoctl HTTP API (backed by a - // Kubernetes cluster). + // TypeAPI is a context served by the Rossoctl HTTP API — whether that is a + // backend fronting a Kubernetes cluster or a local `rossoctl cortex serve`. TypeAPI Type = "api" - // TypeCortex is a context served by a local Cortex, as started by - // `rossoctl cortex serve`. - TypeCortex Type = "cortex" ) // Context is a single named target: a type, a server URI, an optional default diff --git a/internal/rossoctlclient/rossoctlclient.go b/internal/rossoctlclient/rossoctlclient.go index d0f6af4..bd5551d 100644 --- a/internal/rossoctlclient/rossoctlclient.go +++ b/internal/rossoctlclient/rossoctlclient.go @@ -74,7 +74,7 @@ var _ Rossoctl = (*apiclient.Client)(nil) // NewClient builds a Rossoctl backend for ctx: an HTTP apiclient.Client for the // context's server and bearer token. // -// Every context type reaches the API over HTTP, including TypeCortex. A cortex +// Every context reaches the API over HTTP, a local cortex included. A cortex // context once had its own file-backed client reading agents.json directly; that // backend is gone, and a cortex is now reached the same way as any other server — // by pointing the context at a `rossoctl cortex serve` address. diff --git a/internal/rossoctlclient/rossoctlclient_test.go b/internal/rossoctlclient/rossoctlclient_test.go index 40a737c..89f7758 100644 --- a/internal/rossoctlclient/rossoctlclient_test.go +++ b/internal/rossoctlclient/rossoctlclient_test.go @@ -7,14 +7,16 @@ import ( "github.com/rossoctl/rossoctl-cli/internal/config" ) -// TestNewClientIsAlwaysHTTP verifies every context type yields the HTTP client. -// A cortex context is no exception: it is reached by pointing it at a -// `rossoctl cortex serve` address, not by a separate file-backed client. +// TestNewClientIsAlwaysHTTP verifies every context yields the HTTP client, +// including one whose type is unset or holds a value this build no longer +// defines — such as the "cortex" type a config file may still carry from an +// older release. A cortex is reached by pointing a context at a `rossoctl cortex +// serve` address, not by a separate file-backed client. func TestNewClientIsAlwaysHTTP(t *testing.T) { for _, ctxType := range []config.Type{ config.TypeAPI, - config.TypeCortex, - "", // unset + "cortex", // retired from this build; still valid on disk + "", // unset } { t.Run(string(ctxType), func(t *testing.T) { c := NewClient(&config.Context{Type: ctxType, Server: "http://x/api/v1/"}) @@ -38,9 +40,9 @@ func TestNewClientCarriesContextFields(t *testing.T) { t.Errorf("BearerToken = %q, want %q", c.BearerToken, ctx.BearerToken) } - // A cortex context's server is honored the same way, so a context pointed at - // a local `cortex serve` reaches it rather than being routed elsewhere. - cortex := &config.Context{Type: config.TypeCortex, Name: "mycortex", Server: "http://localhost:9097/api/v1/"} + // A localhost server is honored the same way, so a context pointed at a local + // `cortex serve` reaches it rather than being routed elsewhere. + cortex := &config.Context{Type: config.TypeAPI, Name: "cortex", Server: "http://localhost:9097/api/v1/"} cc, ok := NewClient(cortex).(*apiclient.Client) if !ok { t.Fatalf("expected *apiclient.Client for a cortex context, got %T", cc)