diff --git a/management/internals/modules/agentnetwork/credentialcheck.go b/management/internals/modules/agentnetwork/credentialcheck.go index dd62aaed4..a49cc1d41 100644 --- a/management/internals/modules/agentnetwork/credentialcheck.go +++ b/management/internals/modules/agentnetwork/credentialcheck.go @@ -12,37 +12,20 @@ import ( "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. +// test that saves a provider would reach a vendor 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. +// checkProviderCredential refuses a record whose upstream or credential the +// vendor will not accept. // -// 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. +// It reuses the discovery Fetch rather than a lighter status probe so it +// exercises the path the model picker takes: a URL answering 200 with a login +// page fails here instead of producing an empty picker later. func (m *managerImpl) checkProviderCredential(ctx context.Context, provider *types.Provider) error { _, err := m.modelDiscovery.Fetch(ctx, modeldiscovery.Request{ CatalogID: provider.ProviderID, @@ -59,10 +42,8 @@ func (m *managerImpl) checkProviderCredential(ctx context.Context, provider *typ 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. + // WriteError logs only what we return, and that carries no status code, + // so the vendor's number is recorded here or nowhere. log.WithContext(ctx).Infof("agent network provider %s failed its credential check: %v", provider.ProviderID, err) return status.Errorf(status.InvalidArgument, "%s", message) @@ -71,12 +52,12 @@ func (m *managerImpl) checkProviderCredential(ctx context.Context, provider *typ // 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. +// The strings survive WriteError lowercasing them, and never echo the +// operator's URL: paths are case-sensitive, so an echoed URL comes back +// altered and describes 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. + // Not checkable. The record may be perfectly good and we have no way to + // ask, so reporting a failure would be a guess. switch { case errors.Is(err, modeldiscovery.ErrNoDiscovery), errors.Is(err, modeldiscovery.ErrNoDiscoveryHost), @@ -92,10 +73,8 @@ func credentialCheckFailure(err error) (message string, blocking bool) { 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. + // 5xx and 429 included: an outage still leaves the record + // unverified, which is what this refuses to save. return "the provider returned an error", true } } @@ -112,9 +91,8 @@ func credentialCheckFailure(err error) (message string, blocking bool) { 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. + // Ours rather than the vendor's — a request this code built badly, or a + // catalog entry that does not match its parser. Still unverified, so it + // still blocks. return "the provider could not be checked", true } diff --git a/management/internals/modules/agentnetwork/modeldiscovery/failure.go b/management/internals/modules/agentnetwork/modeldiscovery/failure.go index 89da0a40d..2b351d75f 100644 --- a/management/internals/modules/agentnetwork/modeldiscovery/failure.go +++ b/management/internals/modules/agentnetwork/modeldiscovery/failure.go @@ -10,18 +10,14 @@ import ( "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. +// Fetch serves two callers with different needs: the model picker, which only +// needs to know it failed, and the provider credential check, which has to +// tell an operator whether the URL or the key is at fault. Each failure +// carries a type so the second does not have to branch on a message. -// 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. +// VendorStatusError reports a listing answered with something other than 200. +// Only the vendor's own code separates a refused credential (401, 403) from a +// URL that does not serve this API (404, 405) from an unwell vendor (5xx). type VendorStatusError struct { Provider string Status int @@ -31,10 +27,9 @@ 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. +// UnreachableError reports that the request never reached the vendor: the +// name did not resolve, the connection was refused, TLS failed, or it timed +// out. Nothing was authenticated, so only the URL is implicated. type UnreachableError struct { Provider string Err error @@ -46,12 +41,10 @@ func (e *UnreachableError) Error() string { 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. +// Reason names the transport failure in words an operator can act on: a wrong +// port and a wrong hostname fail differently and are worth telling apart. +// Empty means unrecognised, 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 @@ -93,25 +86,22 @@ func (e *UnreachableError) Reason() string { 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. +// ErrUnparseableListing marks a 200 whose body is not a listing in the shape +// the catalog declared. Distinct from a status refusal: the host answered and +// authenticated fine, it is just not the API — a login page, say. 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. +// ErrNoDiscoveryHost marks a provider whose listing host cannot be derived +// from the record: Bedrock's control-plane host comes from the region in the +// upstream, so a proxied endpoint leaves nowhere to send it, and inventing one +// would spend the credential somewhere 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". +// Wraps ErrInvalidRequest so the discovery endpoint still answers 400, while a +// credential check can read it as "cannot be checked" rather than "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. +// ErrPrivateHost marks an upstream resolving somewhere management will not +// dial. A self-hosted endpoint on a private network is a legitimate provider +// the proxy reaches 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")