-
Notifications
You must be signed in to change notification settings - Fork 349
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Network: respect NO_PROXY settings (#6555)
Before JetBrains would pass proxy settings to the agent using HTTP_PROXY and HTTPS_PROXY. However these settings would not allow creating of a "proxyless" http agent since they can be used directly by node libraries. Instead now settings are passed under the CODY_NODE_DEFAULT_PROXY (which falls back to HTTP_PROXY and HTTPS_PROXY). In addition there's a new CODY_NODE_NO_PROXY environment variable that mirrors the NO_PROXY node environment variable. This now allows ignoring of proxy settings for particular endpoints. Lastly, before the NetworkDiagnostics would output extremely verbose logging. Including error logs on cancelled requests. The logging could also potentially leave dangling requests/sockets as event handlers weren't properly cleaned up. By moving to a weak-map for timing events and cleaning up of event handlers this is now prevented. The logging output has also been made a lot more concise and readable.  ## Test plan - Added unit tests to assert the correct re-use of agents depending on configuration and the new CODY_NODE_NO_PROXY setting - Manually verified working in JetBrains and VSCode
- Loading branch information
Showing
6 changed files
with
586 additions
and
167 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| import * as https from 'node:https' | ||
| import { HttpsProxyAgent } from 'hpagent' | ||
| import { describe, expect, test, vi } from 'vitest' | ||
| import { DelegatingAgent } from './DelegatingAgent' | ||
|
|
||
| vi.mock('@sourcegraph/cody-shared', async () => { | ||
| const actual = await vi.importActual('@sourcegraph/cody-shared') | ||
| return { | ||
| ...actual, | ||
| globalAgentRef: { | ||
| isSet: false, | ||
| agent: undefined, | ||
| }, | ||
| } | ||
| }) | ||
|
|
||
| // Mock Socket as before | ||
| vi.mock('node:net', () => ({ | ||
| Socket: vi.fn(() => ({ | ||
| connect: vi.fn(), | ||
| end: vi.fn(), | ||
| destroy: vi.fn(), | ||
| })), | ||
| })) | ||
|
|
||
| // Mock TLS as before | ||
| vi.mock('node:tls', () => ({ | ||
| connect: vi.fn(() => ({ | ||
| end: vi.fn(), | ||
| destroy: vi.fn(), | ||
| })), | ||
| rootCertificates: [], | ||
| })) | ||
|
|
||
| // Mock the entire https.request | ||
| vi.mock('node:https', async () => { | ||
| const actual = await vi.importActual('node:https') | ||
| return { | ||
| ...actual, | ||
| request: vi.fn(() => ({ | ||
| on: vi.fn(), | ||
| end: vi.fn(), | ||
| destroy: vi.fn(), | ||
| })), | ||
| } | ||
| }) | ||
|
|
||
| describe.sequential('DelegatingAgent caching', () => { | ||
| test('reuses cached agents for identical requests', async () => { | ||
| const agent = await DelegatingAgent.initialize({}) | ||
|
|
||
| const requestOptions = { | ||
| host: 'api.example.com', | ||
| port: 443, | ||
| protocol: 'https:', | ||
| method: 'GET', | ||
| path: '/', | ||
| headers: {}, | ||
| secureEndpoint: true, | ||
| } | ||
|
|
||
| // First connection should create a new agent | ||
| const agent1 = await agent.connect(https.request(requestOptions), requestOptions) | ||
|
|
||
| // Second connection to same endpoint should return cached agent | ||
| const agent2 = await agent.connect(https.request(requestOptions), requestOptions) | ||
|
|
||
| // Verify same agent instance is returned | ||
| expect(agent1).toBe(agent2) | ||
|
|
||
| // Different endpoint should create new agent | ||
| const differentRequestOptions = { | ||
| ...requestOptions, | ||
| host: 'different.example.com', | ||
| } | ||
| const agent3 = await agent.connect( | ||
| https.request(differentRequestOptions), | ||
| differentRequestOptions | ||
| ) | ||
|
|
||
| expect(agent3).not.toBe(agent1) | ||
|
|
||
| agent.dispose() | ||
| }) | ||
|
|
||
| test('respects CODY_NODE_DEFAULT_PROXY and CODY_NODE_NO_PROXY', async () => { | ||
| vi.stubEnv('CODY_NODE_DEFAULT_PROXY', 'http://proxy.example.com:8080') | ||
| vi.stubEnv('CODY_NODE_NO_PROXY', 'localhost,internal.example.com,*.other.com') | ||
|
|
||
| const agent = await DelegatingAgent.initialize({}) | ||
|
|
||
| // Test request that should be proxied | ||
| const proxyRequest = { | ||
| host: 'api.example.com', | ||
| port: 443, | ||
| protocol: 'https:', | ||
| method: 'GET', | ||
| path: '/', | ||
| headers: {}, | ||
| secureEndpoint: true, | ||
| } | ||
|
|
||
| // Test request that should not be proxied (matches NO_PROXY) | ||
| const noProxyRequest = { | ||
| host: 'internal.example.com', | ||
| port: 443, | ||
| protocol: 'https:', | ||
| method: 'GET', | ||
| path: '/', | ||
| headers: {}, | ||
| secureEndpoint: true, | ||
| } | ||
|
|
||
| const wildcardNoProxyRequest = { | ||
| host: 'subdomain.other.com', | ||
| port: 443, | ||
| protocol: 'https:', | ||
| method: 'GET', | ||
| path: '/subpath', | ||
| headers: {}, | ||
| secureEndpoint: true, | ||
| } | ||
|
|
||
| const proxiedAgent = await agent.connect(https.request(proxyRequest), proxyRequest) | ||
| const noProxiedAgent = await agent.connect(https.request(noProxyRequest), noProxyRequest) | ||
| const wildcardNoProxyAgent = await agent.connect( | ||
| https.request(wildcardNoProxyRequest), | ||
| wildcardNoProxyRequest | ||
| ) | ||
|
|
||
| // Verify different agents are used | ||
| expect(proxiedAgent).instanceOf(HttpsProxyAgent) | ||
|
|
||
| // Check that these are normal https Agents (e.g. not a subclass) | ||
| expect(noProxiedAgent.constructor).toBe(https.Agent) | ||
| expect(wildcardNoProxyAgent.constructor).toBe(https.Agent) | ||
|
|
||
| // They shouldn't use the same agent | ||
| expect(noProxiedAgent).not.toBe(wildcardNoProxyAgent) | ||
|
|
||
| // Clean up | ||
| agent.dispose() | ||
| vi.unstubAllEnvs() | ||
| }) | ||
| }) |
Oops, something went wrong.