OLS-3736 fix IPv6 addresses in no-proxy causing invalid port error - #3033
OLS-3736 fix IPv6 addresses in no-proxy causing invalid port error#3033thoraxe wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe provider adds IPv6-aware ChangesIPv6 no-proxy handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@ols/src/llms/providers/provider.py`:
- Around line 239-260: The _no_proxy_mount_key function incorrectly treats IPv6
CIDR entries as bare IPv6 literals. Detect CIDR values separately and return the
existing all://*<host> wildcard pattern for them, while retaining bracketed
exact-match handling for bare IPv6 addresses; add a regression test covering an
IPv6 CIDR such as 2001:db8::/32.
In `@tests/unit/llms/providers/test_providers.py`:
- Around line 173-205: Update each test_no_proxy_mount_key_* function with a ->
None return annotation and rewrite its docstring in imperative form. Also update
the nested MyProvider override methods to include appropriate parameter and
return type annotations plus Google-style imperative docstrings, preserving the
existing test behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7161237a-a481-47bd-a3da-e86ab6eee50a
📒 Files selected for processing (2)
ols/src/llms/providers/provider.pytests/unit/llms/providers/test_providers.py
sriroopar
left a comment
sriroopar
left a comment
There was a problem hiding this comment.
Clean, well-scoped IPv6 fix with thorough test coverage — CIDR-before-IPv6 ordering is correct and no regressions.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sriroopar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
All PipelineRuns for this commit have already succeeded. Use |
|
/hold Revision 058836a was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/test e2e-ols-cluster |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
sriroopar
left a comment
sriroopar
left a comment
There was a problem hiding this comment.
Root cause
PR #3033 (fix/OLS-3736-ipv6-no-proxy) branched off main at commit 1430287. Since then, a different PR merged into main:
PR #3033's branch was never rebased onto that change, so:
The pre-existing test at line 164 still calls the old 2-arg form:
_construct_httpx_client(False, False).
The new regression test this PR adds (test_construct_httpx_client_with_ipv6_in_no_proxy_hosts)
copied that same old 2-arg call pattern. When CI runs the PR's branch merged against current main, both calls now pass 3 positional args (self + 2) into a method that only accepts 2 (self + use_async), producing exactly the TypeError you're seeing. This is also why the PR's own test-plan checkbox for make test-unit is unchecked — the author likely knows it's not green yet.
Fix :
Rebase the branch onto latest main, then update both call sites to drop the removed argument:
|
/hold Revision 058836a was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
When ::1 (or any bare IPv6 address) appears in the cluster no-proxy list, the HTTP client mount-dict key was constructed as all://*::1, which httpx misparses — treating :1 as a port — and raises "Invalid port: ':1'", crashing the LLM connection health check. Extract _no_proxy_mount_key() in provider.py to wrap bare IPv6 addresses (detected by two or more colons) in brackets per RFC 3986 section 3.2.2, producing all://[::1]. host:port entries (exactly one colon) and plain hostnames continue to use the all://*<host> pattern. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Fix _no_proxy_mount_key to detect CIDR entries (e.g. 2001:db8::/32) before the IPv6 colon-count check, so IPv6 CIDRs use the wildcard suffix pattern instead of being incorrectly bracketed as IPv6 literals - Add test_no_proxy_mount_key_ipv6_cidr regression test for IPv6 CIDR - Add -> None return annotations to all test_no_proxy_mount_key_* fns - Rewrite test docstrings in imperative mood (fixes ruff D403 CI failure) - Add type annotations and docstrings to nested MyProvider override methods Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Rebasing onto main picked up a prior change that dropped an argument from _construct_httpx_client (now only takes use_async). The new IPv6 regression test still called it with two positional args, causing a TypeError in CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
058836a to
0ad0edd
Compare
|
Rebased onto current main and fixed the stale |
|
/hold cancel |
|
New changes are detected. LGTM label has been removed. |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
@thoraxe: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary
::1(or any bare IPv6 address) is present in the cluster no-proxy list, the httpx mount-dict key was constructed asall://*::1, which httpx misparses — treating:1as a port — raising"Invalid port: ':1'"and crashing the LLM connection health check._no_proxy_mount_key()inprovider.pyto wrap bare IPv6 addresses (detected byhost.count(":") >= 2) in brackets per RFC 3986 § 3.2.2, producingall://[::1].host:portentries (exactly one colon) and plain hostnames continue to useall://*<host>.Test plan
make test-unitpasses (1160 tests, 15 new/modified in providers file)::1, bare full IPv6, already-bracketed[::1],host:port, hostname, IPv4, CIDR, and mixed-list httpx client constructionFixes: OLS-3736
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests