diff --git a/internal/gateway/discover.go b/internal/gateway/discover.go index ea0c277..e951a12 100644 --- a/internal/gateway/discover.go +++ b/internal/gateway/discover.go @@ -3,6 +3,7 @@ package gateway import ( "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -127,6 +128,26 @@ type discoveryCandidate struct { prefer int } +// discoveryFailure carries the HTTP status the caller should answer with. +// +// The status is part of the error rather than left to the handler because the +// whole point of classifying a failure is that the operator must not have to +// guess whether the key, the endpoint, or the vendor is at fault. +type discoveryFailure struct { + msg string + status int +} + +func (e *discoveryFailure) Error() string { return e.msg } + +// StatusOrDefault reports the intended HTTP status. +func (e *discoveryFailure) StatusOrDefault() int { + if e.status == 0 { + return http.StatusBadGateway + } + return e.status +} + // discoveryCandidates enumerates the URL forms worth trying for a raw input. // // The ranking encodes a real observation rather than a guess: the amr-link @@ -146,13 +167,20 @@ func discoveryCandidates(raw string) []discoveryCandidate { if raw == "" { return nil } - seen := map[string]bool{} + // Deduplicate on the URL that will actually be requested, not on the + // (base, path) pair. base "https://h/v1" + path "/models" and base "https://h" + // + path "/v1/models" both request the same address, and listing it twice in + // an error message makes the operator think two things were tried. + seenURL := map[string]bool{} + seenPair := map[string]bool{} var out []discoveryCandidate add := func(base, path string, prefer int) { - if seen[base+"|"+path] { + pair := base + "|" + path + if seenPair[pair] || seenURL[base+path] { return } - seen[base+"|"+path] = true + seenPair[pair] = true + seenURL[base+path] = true out = append(out, discoveryCandidate{baseURL: base, modelsPath: path, prefer: prefer}) } @@ -321,8 +349,10 @@ func DiscoverModels(ctx context.Context, rawURL, apiKey string) (*discoveryResul client := &http.Client{Timeout: discoveryTimeout} var tried []string var firstErr string + var authErr string typedBase := strings.TrimRight(strings.TrimSpace(rawURL), "/") + var lastErr string for _, c := range cands { full := c.baseURL + c.modelsPath tried = append(tried, full) @@ -340,7 +370,10 @@ func DiscoverModels(ctx context.Context, rawURL, apiKey string) (*discoveryResul start := time.Now() resp, err := client.Do(req) if err != nil { - firstErr = err.Error() + lastErr = classifyTransport(err) + if lastErr == "" { + lastErr = err.Error() + } continue } body, readErr := io.ReadAll(io.LimitReader(resp.Body, discoveryMaxBody)) @@ -351,6 +384,13 @@ func DiscoverModels(ctx context.Context, rawURL, apiKey string) (*discoveryResul continue } if resp.StatusCode != http.StatusOK { + // An auth rejection is the operator's key being wrong, not the + // upstream being down. Keep it: it is the single most common + // failure and 502 would send them to the wrong place. + if af := classifyAuthStatus(resp.StatusCode, body); af != "" { + authErr = af + continue + } firstErr = fmt.Sprintf("%s returned %d", full, resp.StatusCode) continue } @@ -406,7 +446,74 @@ func DiscoverModels(ctx context.Context, rawURL, apiKey string) (*discoveryResul } return res, nil } - return nil, fmt.Errorf("no model catalog found; tried %s; last error: %s", strings.Join(tried, ", "), firstErr) + return nil, buildDiscoveryError(tried, authErr, firstErr, lastErr) +} + +// classifyAuthStatus turns a 401/403 from the catalog endpoint into a message +// that names the credential as the cause. +// +// This distinction is the whole point: a wrong key and an unreachable vendor +// are the two most common discovery failures, and they need opposite actions +// (re-type the key vs. go check the vendor). Reporting both as "upstream +// error" makes the operator guess. +func classifyAuthStatus(status int, body []byte) string { + if status != http.StatusUnauthorized && status != http.StatusForbidden { + return "" + } + snippet := strings.TrimSpace(string(body)) + // Vendors put the reason in wildly different shapes; a compact quote is + // more useful than a parse attempt that fails on most of them. + snippet = strings.Join(strings.Fields(snippet), " ") + if len(snippet) > 200 { + snippet = snippet[:200] + "…" + } + return fmt.Sprintf("upstream rejected the api key (HTTP %d): %s — "+ + "check the key, not the endpoint", status, snippet) +} + +// classifyTransport turns low-level network failures into something an +// operator can act on. An empty result means "no specific advice", and the +// caller keeps the raw error. +func classifyTransport(err error) string { + if err == nil { + return "" + } + msg := err.Error() + lower := strings.ToLower(msg) + switch { + case strings.Contains(lower, "no such host"): + return "hostname does not resolve: " + msg + case strings.Contains(lower, "certificate"), strings.Contains(lower, "tls"): + return "TLS verification failed: " + msg + case strings.Contains(lower, "connection refused"): + return "connection refused: " + msg + } + return "" +} + +// buildDiscoveryError assembles the final error, preferring the most specific +// diagnosis available. Every URL tried is listed: an operator staring at a +// failed probe needs to know the search actually happened, not just the last +// attempt. +func buildDiscoveryError(tried []string, authErr, firstErr, lastErr string) error { + triedPart := "no model catalog found" + if len(tried) > 0 { + triedPart = "no model catalog found; tried " + strings.Join(tried, ", ") + } + if authErr != "" { + return &discoveryFailure{status: http.StatusUnauthorized, msg: authErr + " (" + triedPart + ")"} + } + detail := firstErr + if detail == "" { + detail = lastErr + } + if detail == "" { + return errors.New(triedPart) + } + if lastErr != "" && lastErr != firstErr { + detail += " (last: " + lastErr + ")" + } + return &discoveryFailure{status: http.StatusBadGateway, msg: triedPart + "; " + detail} } // upstreamModel is the subset of a /models entry this code reads. Vendors add diff --git a/internal/gateway/discover_api.go b/internal/gateway/discover_api.go index 2ba5379..0840954 100644 --- a/internal/gateway/discover_api.go +++ b/internal/gateway/discover_api.go @@ -3,6 +3,7 @@ package gateway import ( "context" "encoding/json" + "errors" "io" "net/http" "strings" @@ -131,8 +132,15 @@ func (g *Gateway) handleSourceDiscoverAPI(w http.ResponseWriter, r *http.Request res, err := DiscoverModels(ctx, req.BaseURL, req.APIKey) if err != nil { - // 502: the upstream is what failed. The message lists every URL tried. - writeError(w, http.StatusBadGateway, "discover_failed", err.Error()) + // A rejected key must not be reported as an upstream outage: the two + // need opposite actions from the operator, and 502 sends them to the + // wrong one. + status := http.StatusBadGateway + var f *discoveryFailure + if errors.As(err, &f) { + status = f.StatusOrDefault() + } + writeError(w, status, "discover_failed", err.Error()) return } diff --git a/internal/gateway/discover_api_test.go b/internal/gateway/discover_api_test.go index 9dbd854..1fa6983 100644 --- a/internal/gateway/discover_api_test.go +++ b/internal/gateway/discover_api_test.go @@ -3,6 +3,7 @@ package gateway import ( "context" "encoding/json" + "errors" "net/http" "net/http/httptest" "strings" @@ -168,6 +169,100 @@ func TestDiscoverModels_SendsAuthorizationHeader(t *testing.T) { } } +// TestDiscoverModels_WrongKeyIsUnauthorizedNotBadGateway pins the distinction +// that decides whether the operator knows what to do next. +// +// A rejected key and an unreachable vendor are the two most common discovery +// failures, and they need opposite actions: re-type the key, versus go check the +// vendor's status page. Both used to surface as one generic error, so the +// operator had to guess. +func TestDiscoverModels_WrongKeyIsUnauthorizedNotBadGateway(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"error":{"message":"invalid api key","code":"invalid_api_key"}}`)) + })) + defer srv.Close() + + _, err := DiscoverModels(context.Background(), srv.URL+"/v1", "sk-wrong") + if err == nil { + t.Fatal("expected a failure against a 401 endpoint") + } + var f *discoveryFailure + if !errors.As(err, &f) { + t.Fatalf("error should be classified, got %T: %v", err, err) + } + if got := f.StatusOrDefault(); got != http.StatusUnauthorized { + t.Errorf("status = %d, want %d — a wrong key must not read as an upstream outage", + got, http.StatusUnauthorized) + } + // The message has to name which credential is at fault, or the status code + // alone still sends the operator to the wrong place. + if !strings.Contains(strings.ToLower(f.Error()), "api key") { + t.Errorf("message %q should name the api key as the cause", f.Error()) + } + if !strings.Contains(f.Error(), "/v1/models") { + t.Errorf("message %q should list what was tried", f.Error()) + } +} + +// TestDiscoverModels_UnreachableStaysBadGateway: the classification must not +// over-correct — an unresolvable host is still the vendor's or the network's +// problem, not the key's. +func TestDiscoverModels_UnreachableStaysBadGateway(t *testing.T) { + _, err := DiscoverModels(context.Background(), "https://no-such-host-xyz.invalid/v1", "k") + if err == nil { + t.Fatal("expected a failure for an unresolvable host") + } + var f *discoveryFailure + if !errors.As(err, &f) { + t.Fatalf("error should be classified, got %T", err) + } + if got := f.StatusOrDefault(); got != http.StatusBadGateway { + t.Errorf("status = %d, want %d", got, http.StatusBadGateway) + } + if !strings.Contains(strings.ToLower(f.Error()), "resolve") { + t.Errorf("message %q should say the hostname does not resolve", f.Error()) + } +} + +// TestDiscoveryCandidates_NoDuplicateRequests: base "https://h/v1" with path +// "/models" and base "https://h" with path "/v1/models" request the SAME URL. +// Listing it twice in a failure message reads as "two things were tried" when +// only one was. +func TestDiscoveryCandidates_NoDuplicateRequests(t *testing.T) { + seen := map[string]bool{} + for _, c := range discoveryCandidates("https://h/v1") { + url := c.baseURL + c.modelsPath + if seen[url] { + t.Errorf("candidate %q would be requested twice", url) + } + seen[url] = true + } + if len(seen) < 2 { + t.Errorf("only %d distinct URLs; the search collapsed", len(seen)) + } +} + +// TestDiscoverModels_FailureListsEachURLOnce is the end-to-end form of the same +// rule, on the error text an operator actually reads. +func TestDiscoverModels_FailureListsEachURLOnce(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + + _, err := DiscoverModels(context.Background(), srv.URL+"/v1", "k") + if err == nil { + t.Fatal("expected failure") + } + msg := err.Error() + needle := srv.URL + "/v1/models" + if n := strings.Count(msg, needle); n > 1 { + t.Errorf("URL %s appears %d times in %q", needle, n, msg) + } +} + func TestDiscoverModels_EmptyKeyOmitsHeader(t *testing.T) { var present bool srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/gateway/discover_mutation_test.go b/internal/gateway/discover_mutation_test.go index 54b9cf0..4afa3a3 100644 --- a/internal/gateway/discover_mutation_test.go +++ b/internal/gateway/discover_mutation_test.go @@ -11,6 +11,7 @@ package gateway import ( "encoding/json" + "net/http" "strings" "testing" ) @@ -116,10 +117,47 @@ func TestMutantsAreDetectable(t *testing.T) { } }) + t.Run("a rejected key is reported as unauthorized, not as an outage", func(t *testing.T) { + // The mutant: collapse every failure into one generic error, which is + // what made a typo'd key look like the vendor being down. + if classifyAuthStatus(http.StatusUnauthorized, []byte(`{"error":"invalid api key"}`)) == "" { + t.Error("mutant survived: 401 is not classified") + } + if classifyAuthStatus(http.StatusNotFound, []byte("nope")) != "" { + t.Error("a 404 must not be reported as an auth problem") + } + if classifyAuthStatus(http.StatusOK, nil) != "" { + t.Error("a 200 must not be reported as an auth problem") + } + }) + + t.Run("duplicate candidate URLs are collapsed", func(t *testing.T) { + seen := map[string]bool{} + for _, c := range discoveryCandidates("https://h/v1") { + u := c.baseURL + c.modelsPath + if seen[u] { + t.Errorf("mutant survived: %s requested twice", u) + } + seen[u] = true + } + }) + t.Run("discovery falls back across URL forms", func(t *testing.T) { cands := discoveryCandidates("https://h/v1") - if len(cands) < 3 { - t.Errorf("mutant survived: only %d candidates, fallback search removed", len(cands)) + // Two distinct URLs is the correct count after deduping: the two + // spellings of "https://h/v1/models" collapse into one request. An + // earlier expectation of three was measuring the bug, not the coverage. + distinct := map[string]bool{} + for _, c := range cands { + distinct[c.baseURL+c.modelsPath] = true + } + if len(distinct) != 2 { + t.Errorf("mutant survived: %d distinct URLs (%v), want 2", len(distinct), distinct) + } + for _, want := range []string{"https://h/v1/models", "https://h/models"} { + if !distinct[want] { + t.Errorf("mutant survived: %s is not searched", want) + } } }) diff --git a/internal/gateway/discover_test.go b/internal/gateway/discover_test.go index 1ee1c57..c937a0d 100644 --- a/internal/gateway/discover_test.go +++ b/internal/gateway/discover_test.go @@ -315,14 +315,24 @@ func TestDiscoveryCandidates_DedupesAndPrefersTyped(t *testing.T) { } seen := map[string]bool{} for _, c := range cands { - k := c.baseURL + "|" + c.modelsPath + // Dedup is on the requested URL, so that is the key to assert on. + k := c.baseURL + c.modelsPath if seen[k] { - t.Errorf("duplicate candidate %s", k) + t.Errorf("duplicate request for %s", k) } seen[k] = true } - if !seen["https://h|/v1/models"] { - t.Error("host root with the canonical path was never tried; that is the shape the real amr-link endpoint uses") + if !seen["https://h/v1/models"] { + t.Error("the typed base with a bare /models path must be tried; it is the first candidate and the one that answers on the real amr-link endpoint") + } + if !seen["https://h/models"] { + t.Error("host root with a bare /models path was never tried; vendors that imply the version would be missed") + } + // The two candidate PAIRS that both spell "https://h/v1/models" collapse to + // one request. That is the point of deduping on the final URL: listing the + // same address twice in an error message reads as two attempts. + if seen["https://h|v1/models"] && seen["https://h/models"] { + t.Error("both spellings of the versioned URL are present; one is redundant") } // The bare "/models" form must exist too — some gateways serve it there. hasBare := false