Skip to content
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

♻️ refactor geoip match logic #1157

Merged
merged 1 commit into from
Sep 10, 2021
Merged

♻️ refactor geoip match logic #1157

merged 1 commit into from
Sep 10, 2021

Conversation

Loyalsoldier
Copy link
Contributor

cc @maskedeken

@rurirei
Copy link
Contributor

rurirei commented Jul 22, 2021

sorry is it too frequently to introduce third-party dependencies recently?

@Loyalsoldier
Copy link
Contributor Author

sorry is it too frequently to introduce third-party dependencies recently?

Any problem?

@xiaokangwang
Copy link
Contributor

xiaokangwang commented Sep 2, 2021

The issue here is that a third party dependency has been introduced on a critical module. Router, unlike transport or protocol, is not interchangeable, so if something goes wrong, there is no easy way to avoid it.

There are some issues with third party libraries referenced in a critical module:

  • They may have a more restrictive license. (PROXY protocol parser pires/go-proxyproto, protocol buffer parser jhump/protoreflect use Apache 2.0, not MIT or BSD. I intended to remove these dependencies when possible.)
  • They are not integrated with V2Ray's infrastructure and not well optimized, like the lack of support for shared reusable buffer or chained proxy. (all of them)
  • If the original owner decided to stop maintenance, we will need to take over the maintenance to keep using it. (utls)
  • If there is a change to the behaviour or interface. We will need to update our code or fork their project to keep using it. This may end very badly. (Go stdlib's change to HMAC have broken V2Ray's VMess, and our initial "fix" to that problem have broken the protocol.)

@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2021

Codecov Report

Merging #1157 (8ec4095) into master (c4a6686) will decrease coverage by 0.01%.
The diff coverage is 77.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1157      +/-   ##
==========================================
- Coverage   44.77%   44.76%   -0.02%     
==========================================
  Files         485      485              
  Lines       29539    29466      -73     
==========================================
- Hits        13226    13189      -37     
+ Misses      14909    14878      -31     
+ Partials     1404     1399       -5     
Impacted Files Coverage Δ
app/router/config.go 59.03% <ø> (-6.97%) ⬇️
app/router/condition_geoip.go 78.57% <77.14%> (-11.61%) ⬇️
app/router/config.pb.go 23.79% <0.00%> (+1.16%) ⬆️
transport/internet/headers/http/http.go 85.44% <0.00%> (+1.89%) ⬆️
transport/internet/kcp/listener.go 83.16% <0.00%> (+1.98%) ⬆️
proxy/freedom/freedom.go 49.00% <0.00%> (+4.00%) ⬆️
app/router/command/command.go 57.77% <0.00%> (+4.44%) ⬆️
common/buf/reader.go 88.23% <0.00%> (+5.88%) ⬆️
transport/internet/udp/dispatcher.go 77.14% <0.00%> (+12.38%) ⬆️
app/router/command/errors.generated.go 100.00% <0.00%> (+100.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7e52c2...8ec4095. Read the comment docs.

@kslr kslr changed the title Refactor: geoip match logic ♻️ refactor geoip match logic Sep 10, 2021
@kslr kslr merged commit f190216 into v2fly:master Sep 10, 2021
xiaokangwang pushed a commit that referenced this pull request Sep 27, 2021
xiaokangwang added a commit that referenced this pull request Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants