Fix bedrock DNS resolution when behind a corporate proxy#906
Conversation
When behind a corporate proxy, Node.js resolves DNS locally before contacting the proxy. If the proxy is the only path to the endpoint, the request fails with ENOTFOUND. HttpsProxyAgent uses CONNECT tunneling so the proxy handles DNS resolution. Configure NodeHttpHandler with HttpsProxyAgent when a system proxy is detected.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughBedrock now resolves proxy settings from environment variables or VS Code configuration and routes AWS requests through an HTTPS proxy agent when configured. Tests cover proxy discovery, proxied and direct requests, API-key authentication, dependencies, and the patch changeset. ChangesBedrock proxy routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AwsBedrockHandler
participant getSystemProxyUrl
participant HttpsProxyAgent
participant NodeHttpHandler
participant BedrockRuntimeClient
AwsBedrockHandler->>getSystemProxyUrl: Resolve system proxy URL
getSystemProxyUrl-->>AwsBedrockHandler: Return URL or undefined
AwsBedrockHandler->>HttpsProxyAgent: Create proxy agent
AwsBedrockHandler->>NodeHttpHandler: Create request handler
AwsBedrockHandler->>BedrockRuntimeClient: Set requestHandler
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ad39470 to
204a5da
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/networkProxy.ts`:
- Around line 354-360: Update the environment proxy selection in the surrounding
proxy-resolution function to trim the chosen
HTTPS_PROXY/https_proxy/HTTP_PROXY/http_proxy value before returning it, and
treat the trimmed result as unset when blank so resolution can continue to the
fallback path. Preserve the existing HTTPS-over-HTTP precedence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6711a1da-2a4c-48ec-a911-3f1c9a030d08
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.changeset/itchy-moles-thank.mdsrc/api/providers/__tests__/bedrock.spec.tssrc/api/providers/bedrock.tssrc/package.jsonsrc/utils/networkProxy.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- .changeset/itchy-moles-thank.md
- src/package.json
- src/api/providers/bedrock.ts
- src/api/providers/tests/bedrock.spec.ts
| // Standard proxy environment variables (HTTPS takes precedence over HTTP) | ||
| const fromEnv = | ||
| process.env.HTTPS_PROXY || | ||
| process.env.https_proxy || | ||
| process.env.HTTP_PROXY || | ||
| process.env.http_proxy | ||
| if (fromEnv) return fromEnv |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim environment proxy values before returning them.
Unlike the VS Code fallback, environment values are not trimmed. A whitespace-only HTTPS_PROXY/HTTP_PROXY value is truthy and will be passed to HttpsProxyAgent as an invalid proxy URL, preventing Bedrock client initialization. Normalize the selected environment value and treat blank values as unset.
Proposed fix
- const fromEnv =
+ const fromEnv = (
process.env.HTTPS_PROXY ||
process.env.https_proxy ||
process.env.HTTP_PROXY ||
process.env.http_proxy
- if (fromEnv) return fromEnv
+ )?.trim()
+ if (fromEnv) return fromEnv📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Standard proxy environment variables (HTTPS takes precedence over HTTP) | |
| const fromEnv = | |
| process.env.HTTPS_PROXY || | |
| process.env.https_proxy || | |
| process.env.HTTP_PROXY || | |
| process.env.http_proxy | |
| if (fromEnv) return fromEnv | |
| // Standard proxy environment variables (HTTPS takes precedence over HTTP) | |
| const fromEnv = ( | |
| process.env.HTTPS_PROXY || | |
| process.env.https_proxy || | |
| process.env.HTTP_PROXY || | |
| process.env.http_proxy | |
| )?.trim() | |
| if (fromEnv) return fromEnv |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/networkProxy.ts` around lines 354 - 360, Update the environment
proxy selection in the surrounding proxy-resolution function to trim the chosen
HTTPS_PROXY/https_proxy/HTTP_PROXY/http_proxy value before returning it, and
treat the trimmed result as unset when blank so resolution can continue to the
fallback path. Preserve the existing HTTPS-over-HTTP precedence.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
awaiting-review
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/utils/__tests__/networkProxy.spec.ts (2)
351-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrevent potential test pollution by using
mockImplementationOnce.Mocking
vscode.workspace.getConfigurationwithmockImplementationwithout explicitly restoring it might cause subsequent tests (like "should not return empty string proxy") to fail or skip their actual configuration checks if they rely on it, depending on the test runner's configuration for mock restoration.Using
mockImplementationOnceensures the mock only applies to this specific test.♻️ Proposed refactor
it("should handle VS Code API errors gracefully", () => { - vi.mocked(vscode.workspace.getConfiguration).mockImplementation(() => { + vi.mocked(vscode.workspace.getConfiguration).mockImplementationOnce(() => { throw new Error("VS Code API unavailable") }) const result = getSystemProxyUrl()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/__tests__/networkProxy.spec.ts` around lines 351 - 357, Update the getSystemProxyUrl error test to configure vscode.workspace.getConfiguration with mockImplementationOnce instead of mockImplementation, ensuring the thrown API error applies only to that invocation and does not pollute subsequent tests.
310-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep proxy env vars isolated Deleting and reassigning
process.envhere mutates the shared worker environment. Save and restore the original values inafterEach, or switch these cases tovi.stubEnv()/vi.unstubAllEnvs()so this suite doesn’t leak state into other tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/__tests__/networkProxy.spec.ts` around lines 310 - 334, Isolate environment changes in the proxy tests instead of directly deleting and reassigning shared process.env values. Update the beforeEach/afterEach setup around getSystemProxyUrl to use vi.stubEnv with vi.unstubAllEnvs, or save and restore the original HTTPS_PROXY, https_proxy, HTTP_PROXY, and http_proxy values after each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/utils/__tests__/networkProxy.spec.ts`:
- Around line 351-357: Update the getSystemProxyUrl error test to configure
vscode.workspace.getConfiguration with mockImplementationOnce instead of
mockImplementation, ensuring the thrown API error applies only to that
invocation and does not pollute subsequent tests.
- Around line 310-334: Isolate environment changes in the proxy tests instead of
directly deleting and reassigning shared process.env values. Update the
beforeEach/afterEach setup around getSystemProxyUrl to use vi.stubEnv with
vi.unstubAllEnvs, or save and restore the original HTTPS_PROXY, https_proxy,
HTTP_PROXY, and http_proxy values after each test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aa135631-358f-48ea-b61a-eda1f1366357
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
src/api/providers/bedrock.tssrc/package.jsonsrc/utils/__tests__/networkProxy.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/package.json
- src/api/providers/bedrock.ts
efa8f35 to
2c3fd28
Compare
github-actions
Bot
added
awaiting-review
1 participant
When behind a corporate proxy, Node.js resolves DNS locally before contacting the proxy. If the proxy is the only path to the endpoint, the request fails with ENOTFOUND.
HttpsProxyAgent uses CONNECT tunneling so the proxy handles DNS resolution. Configure NodeHttpHandler with HttpsProxyAgent when a system proxy is detected.
I had the need to use bedrock behind a corporate proxy, as there was no fix available, I made one so that I could use it, using Claude code. Here is a proposed PR if you want to merge it to the main branch ...
I reviewed the changes myself, and it seems to be good, but I am no typescript expert so ...
Related GitHub Issue
Closes: #905
Description
Node.js resolves DNS locally before contacting the proxy. When behind a corporate proxy that blocks external DNS, Bedrock calls fail with
ENOTFOUND.This configures
NodeHttpHandlerwithHttpsProxyAgentinBedrockRuntimeClient, which uses CONNECT tunneling so the proxy handles DNS resolution.Proxy is detected from
HTTPS_PROXY/HTTP_PROXYenv vars or VS Codehttp.proxysettingTest Procedure
Test Procedure
pnpm test -- bedrock.spec→ 102 passingPre-Submission Checklist
Documentation Updates
[x] No documentation updates are required.
Additional Notes
Get in Touch
I have no discord username for the moment.
Thanks
Summary by CodeRabbit
Summary by CodeRabbit
zoo-codepackage with this fix.