[management] Trim the credential-check comments to the repo budget

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
 }

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")