Repository navigation
fix(code-index): route Bedrock embedder through the system proxy #1773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
8280715
a8528d9
bdd38dd
cdf1993
82b3fee
5919f91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,35 @@ | ||
| import * as vscode from "vscode" | ||
| import { initializeNetworkProxy, getProxyConfig, isProxyEnabled, isDebugMode, getSystemProxyUrl } from "../networkProxy" | ||
| import { NodeHttpHandler } from "@smithy/node-http-handler" | ||
| import { HttpProxyAgent } from "http-proxy-agent" | ||
| import { HttpsProxyAgent } from "https-proxy-agent" | ||
| import { | ||
| initializeNetworkProxy, | ||
| getProxyConfig, | ||
| isProxyEnabled, | ||
| isDebugMode, | ||
| getSystemProxyUrl, | ||
| createProxyRoutingRequestHandler, | ||
| } from "../networkProxy" | ||
|
|
||
| // Mock global-agent | ||
| vi.mock("global-agent", () => ({ | ||
| bootstrap: vi.fn(), | ||
| })) | ||
|
|
||
| vi.mock("@smithy/node-http-handler", () => ({ | ||
| NodeHttpHandler: vi.fn().mockImplementation(function (options?: unknown) { | ||
| return { | ||
| options, | ||
| handle: vi.fn(), | ||
| updateHttpClientConfig: vi.fn(), | ||
| httpHandlerConfigs: vi.fn().mockReturnValue({}), | ||
| destroy: vi.fn(), | ||
| } | ||
| }), | ||
| })) | ||
| vi.mock("http-proxy-agent", () => ({ HttpProxyAgent: vi.fn() })) | ||
| vi.mock("https-proxy-agent", () => ({ HttpsProxyAgent: vi.fn() })) | ||
|
|
||
| // Mock vscode | ||
| vi.mock("vscode", () => ({ | ||
| workspace: { | ||
|
|
@@ -469,4 +493,118 @@ describe("networkProxy", () => { | |
| }) | ||
| }) | ||
| }) | ||
|
|
||
| describe("createProxyRoutingRequestHandler", () => { | ||
| type Handler = NonNullable<ReturnType<typeof createProxyRoutingRequestHandler>> | ||
|
|
||
| // The handler reads only these fields off the request. Building a real HttpRequest would | ||
| // pull in @smithy/protocol-http, which is not a direct dependency, so a literal stands in. | ||
| const requestTo = (hostname: string, port?: number) => | ||
| ({ protocol: "https:", hostname, port }) as unknown as Parameters<Handler["handle"]>[0] | ||
|
|
||
| // The two inner handlers are told apart by how they were built: only the proxied one | ||
| // receives agents. | ||
| const innerHandlers = () => { | ||
| const built = vi.mocked(NodeHttpHandler).mock.results.map((r) => r.value) | ||
| return { | ||
| direct: built.find((h) => h.options === undefined), | ||
| proxied: built.find((h) => h.options?.httpsAgent), | ||
| } | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| delete process.env.HTTPS_PROXY | ||
| delete process.env.https_proxy | ||
| delete process.env.HTTP_PROXY | ||
| delete process.env.http_proxy | ||
| delete process.env.NO_PROXY | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| delete process.env.no_proxy | ||
| mockConfig.get.mockReturnValue(undefined) | ||
| }) | ||
|
|
||
| it("should return undefined when no proxy is configured", () => { | ||
| expect(createProxyRoutingRequestHandler()).toBeUndefined() | ||
| expect(NodeHttpHandler).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("should build both proxy agents without keeping connections alive", () => { | ||
| process.env.HTTPS_PROXY = "http://proxy.corp:3128" | ||
|
|
||
| expect(createProxyRoutingRequestHandler()).toBeDefined() | ||
|
|
||
| // No agent options: an idle connection would outlive the request that opened it. | ||
| expect(HttpProxyAgent).toHaveBeenCalledWith("http://proxy.corp:3128") | ||
| expect(HttpsProxyAgent).toHaveBeenCalledWith("http://proxy.corp:3128") | ||
| const { proxied } = innerHandlers() | ||
| expect(proxied?.options?.httpAgent).toBe(vi.mocked(HttpProxyAgent).mock.instances[0]) | ||
| expect(proxied?.options?.httpsAgent).toBe(vi.mocked(HttpsProxyAgent).mock.instances[0]) | ||
| }) | ||
|
|
||
| it("should send a request through the proxy when NO_PROXY does not cover it", () => { | ||
| process.env.HTTPS_PROXY = "http://proxy.corp:3128" | ||
| process.env.NO_PROXY = "example.com" | ||
|
|
||
| const handler = createProxyRoutingRequestHandler() | ||
| const request = requestTo("bedrock-runtime.us-east-1.amazonaws.com") | ||
| handler?.handle(request) | ||
|
|
||
| const { direct, proxied } = innerHandlers() | ||
| expect(proxied?.handle).toHaveBeenCalledWith(request) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This asserts only the first argument. If |
||
| expect(direct?.handle).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("should send a request directly when NO_PROXY covers its host", () => { | ||
| process.env.HTTPS_PROXY = "http://proxy.corp:3128" | ||
| process.env.NO_PROXY = "amazonaws.com" | ||
|
|
||
| const handler = createProxyRoutingRequestHandler() | ||
| // The host is only known per request, which is the point of deciding here: indexed | ||
| // file contents must not reach a proxy the user excluded. | ||
| const request = requestTo("bedrock-runtime.eu-west-1.amazonaws.com") | ||
| handler?.handle(request) | ||
|
|
||
| const { direct, proxied } = innerHandlers() | ||
| expect(direct?.handle).toHaveBeenCalledWith(request) | ||
| expect(proxied?.handle).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("should send every request directly when NO_PROXY is '*'", () => { | ||
| process.env.HTTPS_PROXY = "http://proxy.corp:3128" | ||
| process.env.NO_PROXY = "*" | ||
|
|
||
| const handler = createProxyRoutingRequestHandler() | ||
| handler?.handle(requestTo("bedrock-runtime.us-east-1.amazonaws.com")) | ||
|
|
||
| const { direct, proxied } = innerHandlers() | ||
| expect(direct?.handle).toHaveBeenCalledOnce() | ||
| expect(proxied?.handle).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("should match NO_PROXY against the host when the request carries a port", () => { | ||
| process.env.HTTPS_PROXY = "http://proxy.corp:3128" | ||
| process.env.NO_PROXY = "amazonaws.com" | ||
|
|
||
| const handler = createProxyRoutingRequestHandler() | ||
| handler?.handle(requestTo("bedrock-runtime.us-east-1.amazonaws.com", 8443)) | ||
|
|
||
| const { direct, proxied } = innerHandlers() | ||
| expect(direct?.handle).toHaveBeenCalledOnce() | ||
| expect(proxied?.handle).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("should forward configuration updates and teardown to both routes", () => { | ||
| process.env.HTTPS_PROXY = "http://proxy.corp:3128" | ||
|
|
||
| const handler = createProxyRoutingRequestHandler() | ||
| handler?.updateHttpClientConfig("requestTimeout", 1234) | ||
| handler?.destroy() | ||
|
|
||
| const { direct, proxied } = innerHandlers() | ||
| expect(direct?.updateHttpClientConfig).toHaveBeenCalledWith("requestTimeout", 1234) | ||
| expect(proxied?.updateHttpClientConfig).toHaveBeenCalledWith("requestTimeout", 1234) | ||
| expect(direct?.destroy).toHaveBeenCalledOnce() | ||
| expect(proxied?.destroy).toHaveBeenCalledOnce() | ||
| }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,9 @@ | |
| */ | ||
|
|
||
| import * as vscode from "vscode" | ||
| import { NodeHttpHandler } from "@smithy/node-http-handler" | ||
| import { HttpProxyAgent } from "http-proxy-agent" | ||
| import { HttpsProxyAgent } from "https-proxy-agent" | ||
| import { Package } from "../shared/package" | ||
|
|
||
| /** | ||
|
|
@@ -419,6 +422,79 @@ export function getSystemProxyUrl(targetUrl?: string): string | undefined { | |
| return undefined | ||
| } | ||
|
|
||
| // Derived from the handler's own signatures so this file needs no @smithy/types dependency. | ||
| type HandleParameters = Parameters<NodeHttpHandler["handle"]> | ||
| type UpdateClientConfigParameters = Parameters<NodeHttpHandler["updateHttpClientConfig"]> | ||
|
|
||
| /** | ||
| * The subset of the AWS SDK request-handler contract implemented below. Exported so callers and | ||
| * their tests can name the type without reaching for @smithy/types. | ||
| */ | ||
| export interface ProxyRoutingHandler { | ||
| handle(...args: HandleParameters): ReturnType<NodeHttpHandler["handle"]> | ||
| updateHttpClientConfig(...args: UpdateClientConfigParameters): void | ||
| httpHandlerConfigs(): ReturnType<NodeHttpHandler["httpHandlerConfigs"]> | ||
| destroy(): void | ||
| } | ||
|
|
||
| /** | ||
| * AWS SDK request handler that chooses, per request, between the system proxy and a direct | ||
| * connection. | ||
| * | ||
| * The SDK resolves its own endpoint — region, partition, FIPS/dualstack flags, and any endpoint | ||
| * override — so the destination is only known once a request has been built. Choosing here is | ||
| * what makes NO_PROXY apply to the host actually called, rather than one guessed up front. | ||
| * | ||
| * The agents take no options, so no connection outlives its request. Both routes are HTTP/1.1: | ||
| * a client defaulting to NodeHttp2Handler drops to 1.1 once a proxy is configured, which is what | ||
| * the chat provider already does. | ||
| */ | ||
| class ProxyRoutingRequestHandler implements ProxyRoutingHandler { | ||
| private readonly direct = new NodeHttpHandler() | ||
| private readonly proxied: NodeHttpHandler | ||
|
|
||
| constructor(proxyUrl: string) { | ||
| this.proxied = new NodeHttpHandler({ | ||
| httpAgent: new HttpProxyAgent(proxyUrl), | ||
| httpsAgent: new HttpsProxyAgent(proxyUrl), | ||
|
Comment on lines
+458
to
+459
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since these agents are built with no options, |
||
| }) | ||
| } | ||
|
|
||
| private handlerFor(request: HandleParameters[0]): NodeHttpHandler { | ||
| // NO_PROXY entries match on host alone, so the port is left out. | ||
| return isNoProxyHost(`${request.protocol}//${request.hostname}`) ? this.direct : this.proxied | ||
| } | ||
|
|
||
| handle(...args: HandleParameters) { | ||
| return this.handlerFor(args[0]).handle(...args) | ||
| } | ||
|
|
||
| updateHttpClientConfig(...args: UpdateClientConfigParameters) { | ||
| this.direct.updateHttpClientConfig(...args) | ||
| this.proxied.updateHttpClientConfig(...args) | ||
| } | ||
|
|
||
| httpHandlerConfigs() { | ||
| return this.proxied.httpHandlerConfigs() | ||
| } | ||
|
|
||
| destroy() { | ||
| this.direct.destroy() | ||
| this.proxied.destroy() | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Build a request handler routing AWS SDK traffic through the system proxy, except for the | ||
| * destinations NO_PROXY excludes. | ||
| * | ||
| * Returns undefined when no proxy is configured, so the client keeps its own default handler. | ||
| */ | ||
| export function createProxyRoutingRequestHandler(): ProxyRoutingHandler | undefined { | ||
| const proxyUrl = getSystemProxyUrl() | ||
| return proxyUrl ? new ProxyRoutingRequestHandler(proxyUrl) : undefined | ||
| } | ||
|
|
||
| /** | ||
| * Log a message to the output channel if available. | ||
| */ | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.