diff --git a/management/internals/modules/agentnetwork/modeldiscovery/discovery.go b/management/internals/modules/agentnetwork/modeldiscovery/discovery.go index 21d4656ec..c9af43919 100644 --- a/management/internals/modules/agentnetwork/modeldiscovery/discovery.go +++ b/management/internals/modules/agentnetwork/modeldiscovery/discovery.go @@ -124,6 +124,17 @@ func (c *Client) Fetch(ctx context.Context, req Request) ([]Model, error) { return nil, ErrNoDiscovery } + // An entry with a listing host of its own answers from somewhere other + // than the upstream on the record — Bedrock lists from the control plane + // and infers on the runtime host. Reaching the listing therefore proves + // nothing about the host requests will actually go to, so that one is + // checked separately or not at all. + if entry.Discovery.Host != "" { + if err := c.checkUpstreamHost(entry, req.UpstreamURL); err != nil { + return nil, err + } + } + endpoint, err := c.discoveryURL(entry, req) if err != nil { return nil, err @@ -201,20 +212,40 @@ func (c *Client) discoveryURL(entry catalog.Provider, req Request) (string, erro } target := &url.URL{Scheme: "https", Host: host, Path: entry.Discovery.Path, RawQuery: entry.Discovery.Query} - if err := c.checkPublicHost(target.Hostname()); err != nil { - // A host that refuses to resolve fails here, before any request is - // built, and it is the commonest way for an upstream to be wrong. It - // has to reach the caller as unreachable rather than as an - // unclassified fault. ErrPrivateHost is the other outcome and means - // something else entirely — not a bad host, one we decline to dial. - if errors.Is(err, ErrPrivateHost) { - return "", err - } - return "", &UnreachableError{Provider: entry.Name, Err: err} + if err := c.classifyHost(entry, target.Hostname()); err != nil { + return "", err } return target.String(), nil } +// checkUpstreamHost verifies the host the operator configured, for entries +// whose listing lives elsewhere and so cannot vouch for it. +// +// A name that does not resolve is the record being wrong. One that resolves +// privately is not: an upstream behind a proxy is a supported configuration, +// and ErrPrivateHost carries that difference on to the caller, which treats it +// as unverifiable rather than as a failure. +func (c *Client) checkUpstreamHost(entry catalog.Provider, upstreamURL string) error { + parsed, err := url.Parse(strings.TrimSpace(upstreamURL)) + if err != nil || parsed.Hostname() == "" { + return fmt.Errorf("%w: provider upstream %q is not a usable URL", ErrInvalidRequest, upstreamURL) + } + return c.classifyHost(entry, parsed.Hostname()) +} + +// classifyHost renders a failed host check as the two outcomes the caller +// distinguishes. A host that refuses to resolve is the commonest way for an +// upstream to be wrong and has to arrive as unreachable rather than as an +// unclassified fault. ErrPrivateHost means something else entirely — not a bad +// host, one we decline to dial. +func (c *Client) classifyHost(entry catalog.Provider, host string) error { + err := c.checkPublicHost(host) + if err == nil || errors.Is(err, ErrPrivateHost) { + return err + } + return &UnreachableError{Provider: entry.Name, Err: err} +} + // RegionFromUpstream recovers the region an operator embedded in the provider // upstream, by matching it against the catalog's own host template. Bedrock's // template is "bedrock-runtime..amazonaws.com" and Vertex's is diff --git a/management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go b/management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go index 47862b573..2d3921c3e 100644 --- a/management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go +++ b/management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go @@ -597,3 +597,50 @@ func TestFetch_AProxyInThePathDoesNotSilentlyDisableTheCheck(t *testing.T) { type roundTripFunc func(*http.Request) (*http.Response, error) func (f roundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } + +// TestFetch_TheUpstreamIsCheckedWhenTheListingCannotVouchForIt covers the hole +// a separate listing host leaves. Bedrock lists from the control plane, so a +// record whose runtime upstream does not exist reaches a perfectly good +// listing and saves — the requests it then serves go nowhere. +// +// Both halves matter. A runtime host that cannot be resolved is the record +// being wrong, and blocks. A proxied one resolves and only leaves the region +// underivable, which stays the unverifiable outcome it already was. +func TestFetch_TheUpstreamIsCheckedWhenTheListingCannotVouchForIt(t *testing.T) { + refusing := &net.Resolver{ + PreferGo: true, + Dial: func(ctx context.Context, network, address string) (net.Conn, error) { + return nil, errors.New("resolver unavailable") + }, + } + client := &Client{Resolver: refusing} + + _, err := client.Fetch(context.Background(), Request{ + CatalogID: "bedrock_api", + // Matches no catalog template, so nothing here reaches the control + // plane the listing comes from: without its own check this upstream + // was never contacted at all. + UpstreamURL: "https://bedrock.typo.example.invalid", + APIKey: "aws-bearer", + }) + + require.Error(t, err) + var unreachable *UnreachableError + require.ErrorAs(t, err, &unreachable, "a runtime host that will not resolve must block the save") +} + +// TestFetch_AListingHostOfItsOwnDoesNotReachThroughTheUpstream keeps the check +// above from reading the operator's upstream as the place to list from. +func TestFetch_AListingHostOfItsOwnDoesNotReachThroughTheUpstream(t *testing.T) { + cl, tr := newStubClient(http.StatusOK, bedrockListing) + + _, err := cl.Fetch(context.Background(), Request{ + CatalogID: "bedrock_api", + UpstreamURL: "https://bedrock-runtime.eu-central-1.amazonaws.com", + APIKey: "aws-bearer", + }) + require.NoError(t, err) + + assert.Equal(t, "bedrock.eu-central-1.amazonaws.com", tr.got.URL.Host, + "checking the runtime host must not turn it into the listing host") +}