diff --git a/management/internals/modules/agentnetwork/credentialcheck.go b/management/internals/modules/agentnetwork/credentialcheck.go new file mode 100644 index 000000000..dd62aaed4 --- /dev/null +++ b/management/internals/modules/agentnetwork/credentialcheck.go @@ -0,0 +1,120 @@ +package agentnetwork + +import ( + "context" + "errors" + "net/http" + + log "github.com/sirupsen/logrus" + + "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/modeldiscovery" + "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types" + "github.com/netbirdio/netbird/shared/management/status" +) + +// The provider form used to accept anything and find out later. A typo in the +// upstream URL, a key pasted with a character missing, an AWS access key in a +// field that wants a Bedrock API key — all saved cleanly, and surfaced as a +// failed request or an empty model picker some minutes later, with nothing +// pointing back at the record that caused it. +// +// checkProviderCredential closes that gap by spending the credential once, at +// save time, against the vendor's own model listing. + +// ModelLister is the vendor-facing half of the credential check. +// modeldiscovery.Client is the only production implementation; it is an +// interface because the check runs on a write path, so without a seam every +// test that saves a provider would reach a vendor over the network to do it. +type ModelLister interface { + Fetch(ctx context.Context, req modeldiscovery.Request) ([]modeldiscovery.Model, error) +} + +// checkProviderCredential asks the vendor whether this record's upstream and +// credential actually work, and refuses the write if they do not. +// +// Deliberately reuses the discovery Fetch rather than a lighter status probe: +// it exercises the exact path the model picker will take, so a URL that +// answers 200 with a login page fails here instead of passing a status check +// and producing an empty picker later. +// +// A provider the check cannot cover is saved, not blocked. That covers the +// eleven catalog entries with no listing endpoint, a Bedrock record whose +// upstream is proxied so no control-plane host can be derived, and a +// self-hosted endpoint on a private network that the proxy reaches through +// the tunnel but management cannot reach at all. None of those are evidence +// the record is wrong. +func (m *managerImpl) checkProviderCredential(ctx context.Context, provider *types.Provider) error { + _, err := m.modelDiscovery.Fetch(ctx, modeldiscovery.Request{ + CatalogID: provider.ProviderID, + UpstreamURL: provider.UpstreamURL, + APIKey: provider.APIKey, + }) + if err == nil { + return nil + } + + message, blocking := credentialCheckFailure(err) + if !blocking { + log.WithContext(ctx).Debugf("agent network provider %s not credential-checked: %v", provider.ProviderID, err) + return nil + } + + // The operator's message carries no status code, so the number lives here + // or nowhere. WriteError logs whatever we return, which is the message + // alone, so a support question about a 403 has nothing to go on without + // this line. + log.WithContext(ctx).Infof("agent network provider %s failed its credential check: %v", provider.ProviderID, err) + + return status.Errorf(status.InvalidArgument, "%s", message) +} + +// credentialCheckFailure renders a discovery failure as the sentence the +// provider form shows, and reports whether it should block the write. +// +// The strings are written to survive WriteError lowercasing them, and they +// never echo the operator's URL: paths are case-sensitive, so an echoed URL +// would come back altered and describe something they did not type. +func credentialCheckFailure(err error) (message string, blocking bool) { + // Not checkable. The record may be perfectly good; we simply have no way + // to ask, so saying nothing is more honest than reporting a failure. + switch { + case errors.Is(err, modeldiscovery.ErrNoDiscovery), + errors.Is(err, modeldiscovery.ErrNoDiscoveryHost), + errors.Is(err, modeldiscovery.ErrPrivateHost): + return "", false + } + + var vendor *modeldiscovery.VendorStatusError + if errors.As(err, &vendor) { + switch vendor.Status { + case http.StatusUnauthorized, http.StatusForbidden: + return "the provider rejected the credential", true + case http.StatusNotFound, http.StatusMethodNotAllowed: + return "the upstream url did not answer a model listing", true + default: + // Everything else the vendor chose to answer with, 5xx and 429 + // included. A vendor outage blocks the write: working around it is + // not this check's job, and saving a record we could not verify + // would put the operator back where they started. + return "the provider returned an error", true + } + } + + var unreachable *modeldiscovery.UnreachableError + if errors.As(err, &unreachable) { + if reason := unreachable.Reason(); reason != "" { + return "the upstream url could not be reached: " + reason, true + } + return "the upstream url could not be reached", true + } + + if errors.Is(err, modeldiscovery.ErrUnparseableListing) { + return "the upstream url answered, but not with a model listing", true + } + + // Anything left is ours, not theirs — a malformed request this code built, + // or a catalog entry that does not match its parser. Blocking is still + // right: we did not verify the record, and a save that silently skipped + // its check is the thing this feature exists to prevent. + return "the provider could not be checked", true +} diff --git a/management/internals/modules/agentnetwork/credentialcheck_test.go b/management/internals/modules/agentnetwork/credentialcheck_test.go new file mode 100644 index 000000000..d167a85a6 --- /dev/null +++ b/management/internals/modules/agentnetwork/credentialcheck_test.go @@ -0,0 +1,384 @@ +package agentnetwork + +import ( + "context" + "crypto/tls" + "errors" + "fmt" + "net" + "os" + "syscall" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/modeldiscovery" + "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types" + "github.com/netbirdio/netbird/management/server/permissions/modules" + "github.com/netbirdio/netbird/management/server/permissions/operations" + "github.com/netbirdio/netbird/management/server/store" + "github.com/netbirdio/netbird/shared/management/status" +) + +// stubLister stands in for the vendor on the write path. It records what it +// was asked so a test can assert not only that the check ran, but that it ran +// against the right upstream and the right credential — and, for an edit that +// touches neither, that it did not run at all. +type stubLister struct { + err error + requests []modeldiscovery.Request +} + +func (s *stubLister) Fetch(_ context.Context, req modeldiscovery.Request) ([]modeldiscovery.Model, error) { + s.requests = append(s.requests, req) + if s.err != nil { + return nil, s.err + } + return []modeldiscovery.Model{{ID: "a-model", PricingKnown: true}}, nil +} + +func (s *stubLister) calls() int { return len(s.requests) } + +func (s *stubLister) only(t *testing.T) modeldiscovery.Request { + t.Helper() + require.Len(t, s.requests, 1, "the vendor must be asked exactly once") + return s.requests[0] +} + +// TestCredentialCheckFailure_SeparatesTheUrlFromTheCredential is the contract +// the provider form is written against: an operator gets told which of the two +// fields they have to look at, and the message says so without a status code +// and without echoing the URL back at them. +func TestCredentialCheckFailure_SeparatesTheUrlFromTheCredential(t *testing.T) { + cases := []struct { + name string + err error + want string + }{ + { + name: "401 is the credential", + err: &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 401}, + want: "the provider rejected the credential", + }, + { + name: "403 is the credential", + err: &modeldiscovery.VendorStatusError{Provider: "Bedrock", Status: 403}, + want: "the provider rejected the credential", + }, + { + // The host authenticated us fine and then said it has no such + // endpoint, which is the URL being wrong rather than the key. + name: "404 is the url", + err: &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 404}, + want: "the upstream url did not answer a model listing", + }, + { + name: "405 is the url", + err: &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 405}, + want: "the upstream url did not answer a model listing", + }, + { + name: "500 is the vendor", + err: &modeldiscovery.VendorStatusError{Provider: "Anthropic", Status: 500}, + want: "the provider returned an error", + }, + { + name: "503 is the vendor", + err: &modeldiscovery.VendorStatusError{Provider: "Anthropic", Status: 503}, + want: "the provider returned an error", + }, + { + name: "429 is the vendor", + err: &modeldiscovery.VendorStatusError{Provider: "Anthropic", Status: 429}, + want: "the provider returned an error", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, blocking := credentialCheckFailure(tc.err) + require.True(t, blocking, "a vendor refusal must block the write") + require.Equal(t, tc.want, got) + }) + } +} + +// TestCredentialCheckFailure_NamesTheTransportFault covers the failures that +// never reached the vendor. The distinction inside them is worth keeping: a +// refused connection is a wrong port and an unknown host is a wrong hostname, +// and an operator staring at a URL they believe in needs to be told which. +func TestCredentialCheckFailure_NamesTheTransportFault(t *testing.T) { + cases := []struct { + name string + err error + want string + }{ + { + name: "unknown host", + err: &net.DNSError{Err: "no such host", Name: "api.exmaple.com", IsNotFound: true}, + want: "the upstream url could not be reached: no such host", + }, + { + name: "dns failure that is not a missing name", + err: &net.DNSError{Err: "server misbehaving", Name: "api.example.com"}, + want: "the upstream url could not be reached: dns lookup failed", + }, + { + name: "connection refused", + err: &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED}, + want: "the upstream url could not be reached: connection refused", + }, + { + name: "host unreachable", + err: &net.OpError{Op: "dial", Net: "tcp", Err: syscall.EHOSTUNREACH}, + want: "the upstream url could not be reached: host unreachable", + }, + { + name: "timeout", + err: fmt.Errorf("dial: %w", os.ErrDeadlineExceeded), + want: "the upstream url could not be reached: connection timed out", + }, + { + name: "context deadline", + err: fmt.Errorf("dial: %w", context.DeadlineExceeded), + want: "the upstream url could not be reached: connection timed out", + }, + { + name: "untrusted certificate", + err: &tls.CertificateVerificationError{}, + want: "the upstream url could not be reached: tls certificate not trusted", + }, + { + name: "plaintext service on an https url", + err: tls.RecordHeaderError{Msg: "first record does not look like a TLS handshake"}, + want: "the upstream url could not be reached: not a tls endpoint", + }, + { + // Nothing we recognise. Better to say only that it could not be + // reached than to paste a Go error into the provider form. + name: "cause we do not recognise", + err: errors.New("something went sideways"), + want: "the upstream url could not be reached", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + wrapped := &modeldiscovery.UnreachableError{Provider: "OpenAI", Err: tc.err} + got, blocking := credentialCheckFailure(wrapped) + require.True(t, blocking, "an unreachable upstream must block the write") + require.Equal(t, tc.want, got) + }) + } +} + +// TestCredentialCheckFailure_AnAnsweringUrlThatIsNotTheApi covers the case a +// status probe would wave through: the host is up, the credential was accepted +// or not required, and the body is a login page. Reusing the discovery parser +// for the check is what catches it. +func TestCredentialCheckFailure_AnAnsweringUrlThatIsNotTheApi(t *testing.T) { + err := fmt.Errorf("%w: decode model listing: unexpected token", modeldiscovery.ErrUnparseableListing) + + got, blocking := credentialCheckFailure(err) + require.True(t, blocking) + require.Equal(t, "the upstream url answered, but not with a model listing", got) +} + +// TestCredentialCheckFailure_WhatCannotBeCheckedIsNotAFailure pins the +// difference between "this record is wrong" and "we have no way to ask". A +// gateway with no listing endpoint, a Bedrock record pointed at a proxy, and a +// self-hosted endpoint the proxy reaches through the tunnel are all legitimate +// providers. Blocking them would make the feature a lockout. +func TestCredentialCheckFailure_WhatCannotBeCheckedIsNotAFailure(t *testing.T) { + cases := map[string]error{ + "no listing endpoint": modeldiscovery.ErrNoDiscovery, + "no derivable host": fmt.Errorf("%w: %w: bedrock", modeldiscovery.ErrInvalidRequest, modeldiscovery.ErrNoDiscoveryHost), + "private upstream": fmt.Errorf("%w: %w: 10.0.0.5", modeldiscovery.ErrInvalidRequest, modeldiscovery.ErrPrivateHost), + "private at dial time": fmt.Errorf("%w: discovery refused to dial non-public address 10.0.0.5", modeldiscovery.ErrPrivateHost), + "wrapped by unreachable": &modeldiscovery.UnreachableError{ + Provider: "vLLM", + Err: fmt.Errorf("%w: dial", modeldiscovery.ErrPrivateHost), + }, + } + + for name, err := range cases { + t.Run(name, func(t *testing.T) { + message, blocking := credentialCheckFailure(err) + require.False(t, blocking, "a provider we cannot check must still save") + require.Empty(t, message) + }) + } +} + +// TestCredentialCheckFailure_AnUnrecognisedFailureStillBlocks covers a fault of +// ours rather than the vendor's — a malformed request this code built, or a +// catalog entry whose parser does not match its endpoint. The record went +// unverified either way, and silently saving what we could not check is the +// thing this feature exists to prevent. +func TestCredentialCheckFailure_AnUnrecognisedFailureStillBlocks(t *testing.T) { + message, blocking := credentialCheckFailure(errors.New("no parser for listing shape \"\"")) + require.True(t, blocking) + require.Equal(t, "the provider could not be checked", message) +} + +// newCheckedProvider returns a record shaped the way the handler guarantees +// one: a known catalog id, a public upstream and a key. +func newCheckedProvider(accountID string) *types.Provider { + provider := types.NewProvider(accountID) + provider.ProviderID = "openai_api" + provider.Name = "openai" + provider.UpstreamURL = "https://api.openai.com" + provider.APIKey = "sk-good" + provider.Enabled = true + return provider +} + +// TestCreateProvider_RefusesARecordTheVendorRejects is the whole point of the +// feature: a key with a character missing used to save cleanly and surface +// minutes later as a failed request with nothing pointing back at the record. +func TestCreateProvider_RefusesARecordTheVendorRejects(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.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + + require.Error(t, err) + require.Contains(t, err.Error(), "the provider rejected the credential") + + var sErr *status.Error + require.ErrorAs(t, err, &sErr) + require.Equal(t, status.InvalidArgument, sErr.Type(), "the refusal must reach the caller as a 422") + + stored, err := f.store.GetAccountAgentNetworkProviders(ctx, store.LockingStrengthNone, "account1") + require.NoError(t, err) + require.Empty(t, stored, "a record that failed its check must not be written") +} + +// TestCreateProvider_ChecksTheCredentialItWasGiven pins what the vendor is +// asked with, since a check run against the wrong upstream or a stale key +// would pass while proving nothing. +func TestCreateProvider_ChecksTheCredentialItWasGiven(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + _, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + + asked := f.vendor.only(t) + require.Equal(t, "openai_api", asked.CatalogID) + require.Equal(t, "https://api.openai.com", asked.UpstreamURL) + require.Equal(t, "sk-good", asked.APIKey) +} + +// TestUpdateProvider_AUrlOnlyChangeIsCheckedAgainstTheStoredKey covers the +// case that shaped where the check sits. The key never returns to the browser, +// so an operator editing only the URL has none to offer — the stored one is +// the only credential there is, and the new URL still has to be proven with +// it. +func TestUpdateProvider_AUrlOnlyChangeIsCheckedAgainstTheStoredKey(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + created, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + f.vendor.requests = nil + + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Update, true) + edit := newCheckedProvider("account1") + edit.ID = created.ID + edit.UpstreamURL = "https://gateway.example.com" + edit.APIKey = "" // the form sends no key when it was not retyped + + _, err = f.manager.UpdateProvider(ctx, "user1", edit) + require.NoError(t, err) + + asked := f.vendor.only(t) + require.Equal(t, "https://gateway.example.com", asked.UpstreamURL, "the new url must be what gets tested") + require.Equal(t, "sk-good", asked.APIKey, "and the stored key must be what tests it") +} + +// TestUpdateProvider_AFailedRotationLeavesTheWorkingKeyInPlace is the +// half-applied state the check must never produce: refusing the new key while +// having already replaced the old one would take the provider down. +func TestUpdateProvider_AFailedRotationLeavesTheWorkingKeyInPlace(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + created, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + + f.vendor.err = &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 403} + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Update, true) + rotation := newCheckedProvider("account1") + rotation.ID = created.ID + rotation.APIKey = "sk-typo" + + _, err = f.manager.UpdateProvider(ctx, "user1", rotation) + require.Error(t, err) + require.Contains(t, err.Error(), "the provider rejected the credential") + + stored, err := f.store.GetAgentNetworkProviderByID(ctx, store.LockingStrengthNone, "account1", created.ID) + require.NoError(t, err) + require.Equal(t, "sk-good", stored.APIKey, "the rejected key must not have replaced the working one") +} + +// TestUpdateProvider_AnEditTouchingNeitherFieldAsksNoVendor keeps renames, +// model rows and price edits off the vendor's doorstep. They have nothing new +// to prove, and making them wait on a vendor — or fail because one is having a +// bad day — would be a tax on edits that carry no risk. +func TestUpdateProvider_AnEditTouchingNeitherFieldAsksNoVendor(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + created, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + f.vendor.requests = nil + // Any call at all now would fail the update, which is what makes the + // assertion below load-bearing rather than decorative. + f.vendor.err = &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 500} + + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Update, true) + rename := newCheckedProvider("account1") + rename.ID = created.ID + rename.Name = "openai-renamed" + rename.APIKey = "" + + _, err = f.manager.UpdateProvider(ctx, "user1", rename) + require.NoError(t, err, "an edit that changes neither url nor key must not be checked") + require.Zero(t, f.vendor.calls(), "and must not reach the vendor at all") +} + +// TestCreateProvider_AProviderWeCannotCheckStillSaves covers the eleven +// catalog entries with no listing endpoint, a Bedrock record behind a proxy, +// and a self-hosted endpoint on a private network. None of those are evidence +// the record is wrong, and refusing them would make this a lockout. +func TestCreateProvider_AProviderWeCannotCheckStillSaves(t *testing.T) { + cases := map[string]error{ + "gateway with no listing endpoint": modeldiscovery.ErrNoDiscovery, + "bedrock behind a proxy": fmt.Errorf("%w: %w", modeldiscovery.ErrInvalidRequest, modeldiscovery.ErrNoDiscoveryHost), + "self-hosted on a private network": fmt.Errorf("%w: %w", modeldiscovery.ErrInvalidRequest, modeldiscovery.ErrPrivateHost), + } + + for name, vendorErr := range cases { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.vendor.err = vendorErr + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + created, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + require.NotNil(t, created) + + stored, err := f.store.GetAccountAgentNetworkProviders(ctx, store.LockingStrengthNone, "account1") + require.NoError(t, err) + require.Len(t, stored, 1, "a provider we cannot check must still be written") + }) + } +} diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 41789195e..34e5c3220 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -126,13 +126,15 @@ type managerImpl struct { proxyController proxy.Controller // modelDiscovery queries vendors for the models a credential can reach. - // A field rather than a package call so tests can drive it without - // reaching the network. + // An interface rather than the concrete client because it is now on a + // write path: the credential check runs inside CreateProvider and + // UpdateProvider, so every test that saves a provider would otherwise + // reach a vendor over the network to do it. // // One instance serves every request for the process's lifetime, so its // fields must stay read-only after construction: lazy initialisation // inside Fetch or httpClient would race across request goroutines. - modelDiscovery *modeldiscovery.Client + modelDiscovery ModelLister // reconcileCache holds the last set of synthesised proxy mappings // per account, each paired with the proxy that served it, so a change @@ -146,6 +148,19 @@ type managerImpl struct { labelRng *rand.Rand } +// ManagerOption replaces a manager dependency at construction. Production +// passes none; each option exists for something a test cannot let run for +// real. +type ManagerOption func(*managerImpl) + +// WithModelLister replaces the vendor call behind the provider credential +// check. A test that saves a provider needs this — the check runs inside +// CreateProvider and UpdateProvider, so the write path reaches a vendor +// without it. +func WithModelLister(lister ModelLister) ManagerOption { + return func(m *managerImpl) { m.modelDiscovery = lister } +} + // NewManager constructs the persistent Agent Network manager. The // manager persists provider/policy/guardrail configuration and, on // every mutation, reconciles the in-memory synthesised reverse-proxy @@ -156,8 +171,9 @@ func NewManager( permissionsManager permissions.Manager, accountManager account.Manager, proxyController proxy.Controller, + opts ...ManagerOption, ) Manager { - return &managerImpl{ + m := &managerImpl{ store: store, accountManager: accountManager, permissionsManager: permissionsManager, @@ -166,6 +182,10 @@ func NewManager( reconcileCache: make(map[string]map[string]syntheticMapping), labelRng: rand.New(rand.NewSource(time.Now().UnixNano())), } + for _, opt := range opts { + opt(m) + } + return m } func (m *managerImpl) GetAllProviders(ctx context.Context, accountID, userID string) ([]*types.Provider, error) { @@ -230,6 +250,13 @@ func (m *managerImpl) CreateProvider(ctx context.Context, userID string, provide return nil, status.Errorf(status.InvalidArgument, "api_key is required when creating an agent network provider") } + // Before anything is persisted: a record whose upstream or credential does + // not work is rejected here rather than discovered later as a failed + // request with nothing pointing back at it. + if err := m.checkProviderCredential(ctx, provider); err != nil { + return nil, err + } + if provider.ID == "" { fresh := types.NewProvider(provider.AccountID) provider.ID = fresh.ID @@ -269,6 +296,20 @@ func (m *managerImpl) UpdateProvider(ctx context.Context, userID string, provide } else if strings.TrimSpace(provider.APIKey) == "" { return nil, status.Errorf(status.InvalidArgument, "api_key must be non-blank when rotating an agent network provider") } + + // Only the two fields the vendor would judge are worth a round-trip. This + // same call carries renames, model rows and price edits, and none of those + // should wait on a vendor — or be refused because one is having a bad day. + // + // The comparison runs after the merge above, so an update that changes only + // the URL reads as unchanged on the key and is checked against the stored + // one, which is the only credential the operator has to offer here. + if provider.UpstreamURL != existing.UpstreamURL || provider.APIKey != existing.APIKey { + if err := m.checkProviderCredential(ctx, provider); err != nil { + return nil, err + } + } + // Always preserve the session keypair across updates so existing // session cookies stay valid. The keys are server-managed and // never surfaced through the API. diff --git a/management/internals/modules/agentnetwork/modeldiscovery/discovery.go b/management/internals/modules/agentnetwork/modeldiscovery/discovery.go index 253cc63b3..42534eadb 100644 --- a/management/internals/modules/agentnetwork/modeldiscovery/discovery.go +++ b/management/internals/modules/agentnetwork/modeldiscovery/discovery.go @@ -146,7 +146,7 @@ func (c *Client) Fetch(ctx context.Context, req Request) ([]Model, error) { resp, err := c.httpClient().Do(httpReq) if err != nil { - return nil, fmt.Errorf("reach %s: %w", entry.Name, err) + return nil, &UnreachableError{Provider: entry.Name, Err: err} } defer func() { _ = resp.Body.Close() }() @@ -157,7 +157,7 @@ func (c *Client) Fetch(ctx context.Context, req Request) ([]Model, error) { if resp.StatusCode != http.StatusOK { // Surface the vendor's own status. An operator whose key lacks a scope // needs to see 403 rather than a generic failure. - return nil, fmt.Errorf("%s returned %d for its model listing", entry.Name, resp.StatusCode) + return nil, &VendorStatusError{Provider: entry.Name, Status: resp.StatusCode} } ids, err := parseListing(entry.Discovery.Shape, body) @@ -194,8 +194,8 @@ func (c *Client) discoveryURL(entry catalog.Provider, req Request) (string, erro region = RegionFromUpstream(entry, req.UpstreamURL) } if region == "" { - return "", fmt.Errorf("%w: %s discovery needs a region, and none could be read from the provider upstream", - ErrInvalidRequest, entry.Name) + return "", fmt.Errorf("%w: %w: %s discovery needs a region, and none could be read from the provider upstream", + ErrInvalidRequest, ErrNoDiscoveryHost, entry.Name) } host = strings.ReplaceAll(host, catalog.RegionPlaceholder, region) } @@ -266,7 +266,7 @@ func (c *Client) checkPublicHost(host string) error { // loopback address is still a way to reach loopback. for _, addr := range addrs { if !isPublic(addr) { - return fmt.Errorf("%w: discovery host %q resolves to a non-public address", ErrInvalidRequest, host) + return fmt.Errorf("%w: %w: discovery host %q resolves to a non-public address", ErrInvalidRequest, ErrPrivateHost, host) } } return nil @@ -463,7 +463,7 @@ func guardDialAddress(address string) error { return fmt.Errorf("discovery dial address %q is not an IP", host) } if !isPublic(addr) { - return fmt.Errorf("discovery refused to dial non-public address %s", addr) + return fmt.Errorf("%w: discovery refused to dial non-public address %s", ErrPrivateHost, addr) } return nil } diff --git a/management/internals/modules/agentnetwork/modeldiscovery/failure.go b/management/internals/modules/agentnetwork/modeldiscovery/failure.go new file mode 100644 index 000000000..89da0a40d --- /dev/null +++ b/management/internals/modules/agentnetwork/modeldiscovery/failure.go @@ -0,0 +1,117 @@ +package modeldiscovery + +import ( + "context" + "crypto/tls" + "errors" + "fmt" + "net" + "os" + "syscall" +) + +// The errors below exist so a caller can tell one discovery failure from +// another without reading the message. Fetch is used for two jobs now: filling +// the model picker, which only needs to know that it failed, and checking a +// provider's credential at save time, which has to tell the operator whether +// the URL or the key is the problem. A string is the wrong thing to branch on +// for the second, so each failure carries its own type. + +// VendorStatusError reports that the vendor answered the listing with +// something other than 200. Status is the vendor's own code: 401 and 403 mean +// the credential was refused, 404 and 405 mean the URL does not serve this +// API at all, and 5xx means the vendor is unwell — three different things to +// tell an operator, and only the number separates them. +type VendorStatusError struct { + Provider string + Status int +} + +func (e *VendorStatusError) Error() string { + return fmt.Sprintf("%s returned %d for its model listing", e.Provider, e.Status) +} + +// UnreachableError reports that the request never reached the vendor at all: +// the name did not resolve, the connection was refused, TLS failed, or the +// attempt timed out. Nothing was authenticated, so the credential is not +// implicated — only the URL is. +type UnreachableError struct { + Provider string + Err error +} + +func (e *UnreachableError) Error() string { + return fmt.Sprintf("reach %s: %v", e.Provider, e.Err) +} + +func (e *UnreachableError) Unwrap() error { return e.Err } + +// Reason names the transport failure in the words an operator can act on. +// "connection refused" and "no such host" are the difference between a wrong +// port and a wrong hostname, which is worth the few lines it takes to tell +// them apart. An empty string means the cause was not one we recognise, and +// the caller should say only that the host could not be reached rather than +// paste a Go error into the UI. +func (e *UnreachableError) Reason() string { + err := e.Err + + var dns *net.DNSError + if errors.As(err, &dns) { + if dns.IsNotFound { + return "no such host" + } + return "dns lookup failed" + } + + // Timeouts are checked before the syscall cases: a dial that times out is + // reported as a net.OpError wrapping a timeout, and the operator needs to + // hear "timed out" rather than the syscall underneath it. + if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, os.ErrDeadlineExceeded) { + return "connection timed out" + } + var netErr net.Error + if errors.As(err, &netErr) && netErr.Timeout() { + return "connection timed out" + } + + if errors.Is(err, syscall.ECONNREFUSED) { + return "connection refused" + } + if errors.Is(err, syscall.EHOSTUNREACH) || errors.Is(err, syscall.ENETUNREACH) { + return "host unreachable" + } + + var certErr *tls.CertificateVerificationError + if errors.As(err, &certErr) { + return "tls certificate not trusted" + } + var recordErr tls.RecordHeaderError + if errors.As(err, &recordErr) { + return "not a tls endpoint" + } + + return "" +} + +// ErrUnparseableListing marks a 200 whose body is not a model listing in the +// shape the catalog declared. It is a distinct outcome from a status refusal: +// the host answered and authenticated fine, it just is not the API we were +// aiming at — a login page or an unrelated service on the configured URL. +var ErrUnparseableListing = errors.New("response is not a model listing") + +// ErrNoDiscoveryHost marks a provider whose listing host cannot be worked out +// from the record. Bedrock's listing lives on a control-plane host derived +// from the region in the upstream, so an operator pointing the record at a +// proxied or self-hosted endpoint leaves nowhere to send it. Inventing a host +// would spend their credential somewhere they never configured. +// +// It wraps ErrInvalidRequest so the discovery endpoint keeps answering 400, +// while a credential check can recognise it as "cannot be checked" rather +// than "is broken". +var ErrNoDiscoveryHost = errors.New("provider has no derivable discovery host") + +// ErrPrivateHost marks an upstream that resolves somewhere the management +// server will not dial. A self-hosted vendor endpoint on a private network is +// a legitimate provider — the proxy reaches it through the tunnel — so this +// means the check cannot run, not that the record is wrong. +var ErrPrivateHost = errors.New("discovery host is not publicly routable") diff --git a/management/internals/modules/agentnetwork/modeldiscovery/parse.go b/management/internals/modules/agentnetwork/modeldiscovery/parse.go index 83048cb8a..67a10bf36 100644 --- a/management/internals/modules/agentnetwork/modeldiscovery/parse.go +++ b/management/internals/modules/agentnetwork/modeldiscovery/parse.go @@ -43,7 +43,7 @@ func parseOpenAIData(body []byte) ([]listedModel, error) { } `json:"data"` } if err := json.Unmarshal(body, &doc); err != nil { - return nil, fmt.Errorf("decode model listing: %w", err) + return nil, fmt.Errorf("%w: decode model listing: %w", ErrUnparseableListing, err) } out := make([]listedModel, 0, len(doc.Data)) for _, entry := range doc.Data { @@ -71,7 +71,7 @@ func parseBedrockInferenceProfiles(body []byte) ([]listedModel, error) { } `json:"inferenceProfileSummaries"` } if err := json.Unmarshal(body, &doc); err != nil { - return nil, fmt.Errorf("decode inference-profile listing: %w", err) + return nil, fmt.Errorf("%w: decode inference-profile listing: %w", ErrUnparseableListing, err) } out := make([]listedModel, 0, len(doc.Summaries)) for _, entry := range doc.Summaries { @@ -98,7 +98,7 @@ func parseVertexPublisherModels(body []byte) ([]listedModel, error) { } `json:"publisherModels"` } if err := json.Unmarshal(body, &doc); err != nil { - return nil, fmt.Errorf("decode publisher-model listing: %w", err) + return nil, fmt.Errorf("%w: decode publisher-model listing: %w", ErrUnparseableListing, err) } out := make([]listedModel, 0, len(doc.Models)) for _, entry := range doc.Models { diff --git a/management/internals/modules/agentnetwork/settings_bootstrap_test.go b/management/internals/modules/agentnetwork/settings_bootstrap_test.go index fc6fd8b82..fea62353e 100644 --- a/management/internals/modules/agentnetwork/settings_bootstrap_test.go +++ b/management/internals/modules/agentnetwork/settings_bootstrap_test.go @@ -26,6 +26,10 @@ type bootstrapFixture struct { manager Manager store store.Store perms *permissions.MockManager + // vendor stands in for the provider credential check's vendor call, which + // runs on every provider write. Without it these tests would reach a real + // vendor to save a record. + vendor *stubLister } func newBootstrapFixture(t *testing.T) *bootstrapFixture { @@ -47,10 +51,12 @@ func newBootstrapFixture(t *testing.T) *bootstrapFixture { accounts.EXPECT().UpdateAccountPeers(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes() accounts.EXPECT().BufferUpdateAccountPeers(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes() + vendor := &stubLister{} return &bootstrapFixture{ - manager: NewManager(st, perms, accounts, nil), + manager: NewManager(st, perms, accounts, nil, WithModelLister(vendor)), store: st, perms: perms, + vendor: vendor, } } @@ -211,6 +217,7 @@ func TestCreateProviderHasNoSettingsSideEffects(t *testing.T) { f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) provider := types.NewProvider("account1") + provider.ProviderID = "openai_api" provider.Name = "openai" provider.UpstreamURL = "https://api.openai.com" provider.APIKey = "sk-test"