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)