mirror of
https://github.com/netbirdio/netbird.git
synced 2026-10-27 14:59:09 +01:00
[management] Say what went wrong when loading a provider's models fails
Pressing "Load models from provider" against a key the vendor refuses answered "internal server error". Every outcome on that path is the operator's own key or upstream, so a 500 was wrong twice over: it told them the server broke, and it named nothing they could act on. The failures are already typed and already have sentences written for them — the save-time check translates the same set. Discovery now runs them through the same classifier, so the button reports a refused credential or an unreachable url in the words the form uses elsewhere. ErrNoDiscovery and ErrInvalidRequest pass through untouched. The handler maps both already, and a provider with no listing endpoint is a fact about the catalog entry rather than a failure — the caller falls back to the catalog's own models instead of showing an error at all.
This commit is contained in:
@@ -49,6 +49,32 @@ func (m *managerImpl) checkProviderCredential(ctx context.Context, provider *typ
|
|||||||
return status.Errorf(status.InvalidArgument, "%s", message)
|
return status.Errorf(status.InvalidArgument, "%s", message)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// discoveryFailure renders a failed model listing for the operator who pressed
|
||||||
|
// the button. Every outcome here is something they did or configured — a key
|
||||||
|
// the vendor refused, an upstream that does not answer — so it owes them the
|
||||||
|
// same sentence a refused save gives, not the generic 500 an unclassified
|
||||||
|
// error turns into.
|
||||||
|
//
|
||||||
|
// ErrNoDiscovery and ErrInvalidRequest pass through untouched: the handler
|
||||||
|
// already maps them, and "this provider has no listing endpoint" is a fact
|
||||||
|
// about the catalog rather than a failure to report as one.
|
||||||
|
func discoveryFailure(ctx context.Context, catalogID string, err error) error {
|
||||||
|
if errors.Is(err, modeldiscovery.ErrNoDiscovery) || errors.Is(err, modeldiscovery.ErrInvalidRequest) {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
|
||||||
|
message, _ := credentialCheckFailure(err)
|
||||||
|
if message == "" {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
|
||||||
|
// The operator's message carries no status code, so the vendor's number is
|
||||||
|
// recorded here or nowhere.
|
||||||
|
log.WithContext(ctx).Infof("agent network model discovery for %s failed: %v", catalogID, err)
|
||||||
|
|
||||||
|
return status.Errorf(status.InvalidArgument, "%s", message)
|
||||||
|
}
|
||||||
|
|
||||||
// credentialCheckFailure renders a discovery failure as the sentence the
|
// credentialCheckFailure renders a discovery failure as the sentence the
|
||||||
// provider form shows, and reports whether it should block the write.
|
// provider form shows, and reports whether it should block the write.
|
||||||
//
|
//
|
||||||
|
|||||||
@@ -377,3 +377,82 @@ func TestCreateProvider_AProviderWeCannotCheckStillSaves(t *testing.T) {
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestDiscoveryFailure_TellsTheOperatorWhatWentWrong covers the button, not the
|
||||||
|
// save. Pressing "Load models from provider" against a bad key used to answer
|
||||||
|
// "internal server error", which names neither the thing that failed nor
|
||||||
|
// anything the operator could act on — every outcome here is their key or their
|
||||||
|
// URL.
|
||||||
|
func TestDiscoveryFailure_TellsTheOperatorWhatWentWrong(t *testing.T) {
|
||||||
|
cases := map[string]struct {
|
||||||
|
err error
|
||||||
|
want string
|
||||||
|
}{
|
||||||
|
"refused credential": {
|
||||||
|
err: &modeldiscovery.VendorStatusError{Provider: "Bedrock", Status: 403},
|
||||||
|
want: "the provider rejected the credential",
|
||||||
|
},
|
||||||
|
"upstream that is not the api": {
|
||||||
|
err: &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 404},
|
||||||
|
want: "the upstream url did not answer a model listing",
|
||||||
|
},
|
||||||
|
"upstream that does not resolve": {
|
||||||
|
err: &modeldiscovery.UnreachableError{
|
||||||
|
Provider: "OpenAI",
|
||||||
|
Err: &net.DNSError{Err: "no such host", Name: "api.example.com", IsNotFound: true},
|
||||||
|
},
|
||||||
|
want: "the upstream url could not be reached: no such host",
|
||||||
|
},
|
||||||
|
"vendor having a bad day": {
|
||||||
|
err: &modeldiscovery.VendorStatusError{Provider: "Anthropic", Status: 503},
|
||||||
|
want: "the provider returned an error",
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
for name, tc := range cases {
|
||||||
|
t.Run(name, func(t *testing.T) {
|
||||||
|
err := discoveryFailure(context.Background(), "openai_api", tc.err)
|
||||||
|
require.EqualError(t, err, tc.want)
|
||||||
|
|
||||||
|
var sErr *status.Error
|
||||||
|
require.ErrorAs(t, err, &sErr)
|
||||||
|
require.Equal(t, status.InvalidArgument, sErr.Type(),
|
||||||
|
"a failure the operator caused must not read as a server fault")
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestDiscoveryFailure_LeavesTheCatalogFactsAlone keeps the two outcomes the
|
||||||
|
// handler already maps. A provider with no listing endpoint is a fact about the
|
||||||
|
// catalog entry, and the caller falls back to the catalog's own models rather
|
||||||
|
// than showing an error at all — rewriting it as a refusal would turn a normal
|
||||||
|
// path into one.
|
||||||
|
func TestDiscoveryFailure_LeavesTheCatalogFactsAlone(t *testing.T) {
|
||||||
|
for name, err := range map[string]error{
|
||||||
|
"no listing endpoint": modeldiscovery.ErrNoDiscovery,
|
||||||
|
"bad request": fmt.Errorf("%w: unknown catalog provider", modeldiscovery.ErrInvalidRequest),
|
||||||
|
} {
|
||||||
|
t.Run(name, func(t *testing.T) {
|
||||||
|
require.Equal(t, err, discoveryFailure(context.Background(), "openai_api", err),
|
||||||
|
"the handler's own mapping must still see the original error")
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestDiscoverProviderModels_SurfacesTheVendorRefusal drives the manager rather
|
||||||
|
// than the classifier, so a future refactor that stops translating on this path
|
||||||
|
// fails here rather than silently going back to 500s.
|
||||||
|
func TestDiscoverProviderModels_SurfacesTheVendorRefusal(t *testing.T) {
|
||||||
|
ctx := context.Background()
|
||||||
|
f := newBootstrapFixture(t)
|
||||||
|
f.vendor.err = &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 401}
|
||||||
|
f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true)
|
||||||
|
|
||||||
|
_, err := f.manager.DiscoverProviderModels(ctx, "account1", "user1", modeldiscovery.Request{
|
||||||
|
CatalogID: "openai_api",
|
||||||
|
UpstreamURL: "https://api.openai.com",
|
||||||
|
APIKey: "sk-wrong",
|
||||||
|
}, "")
|
||||||
|
|
||||||
|
require.EqualError(t, err, "the provider rejected the credential")
|
||||||
|
}
|
||||||
|
|||||||
@@ -231,7 +231,11 @@ func (m *managerImpl) DiscoverProviderModels(ctx context.Context, accountID, use
|
|||||||
req.APIKey = record.APIKey
|
req.APIKey = record.APIKey
|
||||||
}
|
}
|
||||||
|
|
||||||
return m.modelDiscovery.Fetch(ctx, req)
|
models, err := m.modelDiscovery.Fetch(ctx, req)
|
||||||
|
if err != nil {
|
||||||
|
return nil, discoveryFailure(ctx, req.CatalogID, err)
|
||||||
|
}
|
||||||
|
return models, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// CreateProvider persists a new provider for the account. Providers have no
|
// CreateProvider persists a new provider for the account. Providers have no
|
||||||
|
|||||||
Reference in New Issue
Block a user