Support using proxy dial-up connection to GitHub. #46
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes add proxy configuration support by introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as Config Loader
participant Main as main.go
participant Proxy as Proxy Module
participant HTTP as http.Transport
Loader->>Main: Load configuration (includes proxy settings)
Main->>Proxy: InitReq(config)
Proxy->>HTTP: initTransport(config, transport)
HTTP-->>Proxy: Transport configured
Proxy-->>Main: Return initialized client
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
go.mod (1)
3-3: Fix invalid Go version.The specified Go version "1.23.6" is invalid. The latest stable version is 1.22.
Update to a valid Go version:
-go 1.23.6 +go 1.22
🧹 Nitpick comments (4)
proxy/gitreq.go (1)
20-30: Consider improving HTTP client initialization.While the changes are functionally correct, consider:
- Moving the transport configuration to a separate function
- Adding error handling for transport initialization
Apply this diff to improve the code:
func initGitHTTPClient(cfg *config.Config) { + if cfg == nil { + panic("config cannot be nil") + } gtr = &http.Transport{ MaxIdleConns: 30, MaxConnsPerHost: 30, IdleConnTimeout: 30 * time.Second, } initTransport(cfg, gtr) gclient = &http.Client{ Transport: gtr, + Timeout: 30 * time.Second, } }proxy/chunkreq.go (1)
169-171: Consider adjusting buffer pool size based on proxy usage.The fixed 32KB buffer size might not be optimal for all proxy scenarios. Consider making it configurable or adjusting based on proxy usage patterns.
🧰 Tools
🪛 golangci-lint (1.62.2)
171-171: SA6002: argument should be pointer-like to avoid allocations
(staticcheck)
config/config.toml (1)
40-41: Document proxy authentication requirements.Consider adding documentation about proxy authentication configuration if supported.
Add a comment explaining if and how proxy authentication should be configured:
enabled = false -url = "socks5://127.0.0.1:1080" # "http://127.0.0.1:7890" +url = "socks5://127.0.0.1:1080" # Format: [scheme]://[user:pass@]host:port + # Supports: socks5://, http://, https://README.md (1)
118-120: Add security considerations for proxy usage.The proxy configuration documentation should include security best practices and warnings.
Add a security note:
[proxy] enabled = false # 是否使用代理连接 url = "socks5://127.0.0.1:1080" # "http://127.0.0.1:7890" 支持HTTP代理和SOCKS5代理 支持多级SOCKS5代理 +# 安全提示： +# 1. 确保代理服务器可信任 +# 2. 建议使用加密代理协议（如SOCKS5） +# 3. 在生产环境中避免使用无认证的公共代理
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(1 hunks)config/config.go(2 hunks)config/config.toml(1 hunks)go.mod(1 hunks)main.go(1 hunks)proxy/chunkreq.go(3 hunks)proxy/dial.go(1 hunks)proxy/gitreq.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
proxy/dial.go
51-51: SA1019: transport.Dial has been deprecated since Go 1.7: Use DialContext instead, which allows the transport to cancel dials as soon as they are no longer needed. If both are set, DialContext takes priority.
(staticcheck)
🔇 Additional comments (6)
config/config.go (1)
16-16: LGTM!The proxy configuration structure follows the existing pattern and is well-integrated into the configuration system.
Also applies to: 65-68
main.go (1)
94-96: LGTM!The changes correctly integrate the proxy configuration into the initialization sequence.
Also applies to: 103-103
proxy/chunkreq.go (2)
25-28: LGTM! Function signature changes look good.The InitReq function now properly accepts the config parameter needed for proxy configuration.
37-50: Verify proxy transport initialization.While the transport configuration looks good, we need to ensure the initTransport function properly handles proxy configuration.
Let's verify the implementation of initTransport:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for initTransport implementation ast-grep --pattern 'func initTransport(cfg *config.Config, tr *http.Transport) { $$$ }'Length of output: 91
We'll now run an additional script to locate the definition of initTransport using a broader search tool (rg) to ensure we capture any variant definitions that might be present in the repository.
🏁 Script executed:
#!/bin/bash rg -n "func initTransport" .Length of output: 117
We'll now run a script to display the full implementation of initTransport from "proxy/dial.go" (lines 30–120) to confirm how proxy configuration is handled.
🏁 Script executed:
#!/bin/bash sed -n '30,120p' proxy/dial.goLength of output: 605
Proxy Transport Initialization Verified
The initTransport implementation in proxy/dial.go properly manages proxy configuration as follows:
- Checks whether proxying is enabled via cfg.Proxy.Enabled.
- When enabled but no URL is provided, it falls back to http.ProxyFromEnvironment.
- When a URL is available, it attempts to parse it and, if successful, configures the transport:
- For URLs starting with "http," sets the proxy using http.ProxyURL.
- For other URL types, it creates a custom dialer with newProxyDial and assigns the Dial and DialContext fields accordingly.
Based on this thorough review, the proxy configuration is correctly handled.
config/config.toml (1)
39-41: LGTM! Proxy configuration is well-structured.The proxy configuration is disabled by default, which is a good security practice.
go.mod (1)
9-9: Verify golang.org/x/net version compatibility.Let's ensure we're using a secure and compatible version of the net package.
✅ Verification successful
🌐 Web query:
What is the latest version of golang.org/x/net?💡 Result:
The latest version of `golang.org/x/net` as of February 9, 2025, is **v0.0.0-20250106045124-913f4cffc9e4**, based on its January 6, 2025 commit timestamp[5][6][7]. This pseudo-version follows Go’s module versioning convention (`v0.0.0-<timestamp>-<commit-hash>`), indicating it is the most recent unreleased commit from the repository’s main branch. To install or update: ```bash go get -u golang.org/x/net@latest ``` The package includes networking libraries like HTTP/2 support[6], HTML parsing[7], and other supplementary utilities[1]. For production use, consider pinning to a specific tagged release if available, as pseudo-versions represent unreleased development states. Citations: - 1: https://github.com/juelite/golang.org-x-net - 2: https://go.dev/doc/devel/release - 3: https://go.dev/dl/ - 4: https://tip.golang.org/doc/go1.20 - 5: https://deps.dev/go/golang.org%2Fx%2Fnet/v0.0.0-20191004110552-13f9640d40b9/versions - 6: https://pkg.go.dev/golang.org/x/net/http2 - 7: https://pkg.go.dev/golang.org/x/net/html - 8: https://tip.golang.org/doc/go1.18 - 9: https://pkg.go.dev/golang.org/x/net - 10: https://github.com/golang/net
golang.org/x/net version verified for compatibility
- The go.mod file currently pins golang.org/x/net to v0.34.0, which is a stable, tagged release.
- The latest available commit (as a pseudo-version) is v0.0.0-20250106045124-913f4cffc9e4. This version reflects more recent commits from the main branch but represents an unreleased development state.
- For production environments, sticking with a tagged release like v0.34.0 is typically recommended for stability—unless you require specific fixes/features from the latest commit.
|
感谢您的PR, 请讲述此PR带来的改进与预期功能 |
|
明白了, 请处理coderabbitai审查的意见 |
预期功能：ghproxy可以通过代理服务进行拨号连接至GitHub 带来的改进：ghproxy不需要部署在国外的云主机上，可以跑在nas或国内站点上，使用代理拨号的方式连接GitHub |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
proxy/dial.go (1)
42-61:⚠️ Potential issueFix transport initialization issues.
The function has several critical issues that need to be addressed:
- Using deprecated
Dialfield (use onlyDialContext)- Unsafe type assertion for
proxy.ContextDialer- Improper error handling
- Missing validation for empty URL
Apply this diff to fix the issues:
func initTransport(cfg *config.Config, transport *http.Transport) { if !cfg.Proxy.Enabled { return } - if cfg.Proxy.Url == "" { + if strings.TrimSpace(cfg.Proxy.Url) == "" { transport.Proxy = http.ProxyFromEnvironment return } proxyInfo, err := url.Parse(cfg.Proxy.Url) - if err == nil { - if strings.HasPrefix(cfg.Proxy.Url, "http") { - transport.Proxy = http.ProxyURL(proxyInfo) - } else { - proxyDialer := newProxyDial(cfg.Proxy.Url) - transport.Dial = proxyDialer.Dial - transport.DialContext = proxyDialer.(proxy.ContextDialer).DialContext - } + if err != nil { + log.Printf("Failed to parse proxy URL %s: %v", cfg.Proxy.Url, err) + return + } + if strings.HasPrefix(cfg.Proxy.Url, "http") { + transport.Proxy = http.ProxyURL(proxyInfo) + return + } + proxyDialer := newProxyDial(cfg.Proxy.Url) + contextDialer, ok := proxyDialer.(proxy.ContextDialer) + if !ok { + log.Printf("Failed to create context dialer for proxy URL %s", cfg.Proxy.Url) + return + } + transport.DialContext = contextDialer.DialContext } }🧰 Tools
🪛 golangci-lint (1.62.2)
57-57: SA1019: transport.Dial has been deprecated since Go 1.7: Use DialContext instead, which allows the transport to cancel dials as soon as they are no longer needed. If both are set, DialContext takes priority.
(staticcheck)
🧹 Nitpick comments (1)
proxy/dial.go (1)
12-40: Consider adding error logging and connection timeout.While the core functionality is solid, consider these improvements for better operability:
- Add error logging for failed proxy configurations to aid debugging
- Validate host format (should contain both host and port)
- Add connection timeout for proxy dialing
Here's a suggested implementation:
+import ( + "fmt" + "log" + "time" +) func newProxyDial(proxyUrls string) proxy.Dialer { var proxyDialer proxy.Dialer = proxy.Direct for _, proxyUrl := range strings.Split(proxyUrls, ",") { urlInfo, err := url.Parse(proxyUrl) if err != nil { + log.Printf("Failed to parse proxy URL %s: %v", proxyUrl, err) continue } if urlInfo.Scheme != "socks5" { + log.Printf("Unsupported proxy scheme %s in URL %s", urlInfo.Scheme, proxyUrl) continue } + host, port, err := net.SplitHostPort(urlInfo.Host) + if err != nil { + log.Printf("Invalid host format in URL %s: %v", proxyUrl, err) + continue + } var auth *proxy.Auth = nil if urlInfo.User != nil { pwd, ok := urlInfo.User.Password() if !ok { + log.Printf("Failed to get password from URL %s", proxyUrl) continue } auth = &proxy.Auth{ User: urlInfo.User.Username(), Password: pwd, } } - dialer, err := proxy.SOCKS5("tcp", urlInfo.Host, auth, proxyDialer) + baseDialer := &net.Dialer{Timeout: 30 * time.Second} + dialer, err := proxy.SOCKS5("tcp", fmt.Sprintf("%s:%s", host, port), auth, baseDialer) if err == nil { proxyDialer = dialer + } else { + log.Printf("Failed to create SOCKS5 dialer for URL %s: %v", proxyUrl, err) } } return proxyDialer }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proxy/dial.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
proxy/dial.go
57-57: SA1019: transport.Dial has been deprecated since Go 1.7: Use DialContext instead, which allows the transport to cancel dials as soon as they are no longer needed. If both are set, DialContext takes priority.
(staticcheck)
🔇 Additional comments (1)
proxy/dial.go (1)
1-10: LGTM!Package structure and imports are well-organized and appropriate for the functionality.
我们决定采纳此PR |
|
请将其pr到dev分支, 谢谢 |
|
25w13a已加入 |
|
错误: gitreq 使用了 chuncked transport |
Summary by CodeRabbit
New Features
Chores