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

feat: support bandwidth configuration #645

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

mnixry
Copy link
Contributor

@mnixry mnixry commented Sep 19, 2024

Background

ref: daeuniverse/outbound#21

Checklist

Full Changelogs

  • Implement bandwidth config parsing in dae:

     global {
     	...
     
         # The maximum bandwidth for accessing the Internet. It is useful for some specific protocols (e.g., Hysteria2), 
         # which will perform better with bandwith information provided. The unit is **byte** per second.
         bandwidth_max_tx: 26214400 # 200Mbps == 25MB/s == 26214400 B/s uplink
         bandwidth_max_rx: 131072000 # 1Gbps == 125MB/s == 131072000 B/s downlink
     }
    
  • Improve handling for insecure parameter in hysteria2 URI parsing.

Issue Reference

Closes #[issue number]

Test Result

@mzz2017
Copy link
Contributor

mzz2017 commented Sep 19, 2024

提问,

  1. 当设置了带宽时,是否所有 hy2 节点都会使用 brutal?
  2. 有 hy2 节点不支持 brutal 的可能性吗

@mnixry
Copy link
Contributor Author

mnixry commented Sep 19, 2024

  1. 当设置了带宽时,是否所有 hy2 节点都会使用 brutal?

否,Hysteria2 协议会在握手时协商双方带宽。我们实现的行为是:如果服务端和客户端任意一方不提供带宽或者服务端选择忽略客户端带宽,那么它将会选择启用 BBR。只有当双方均设置了带宽的前提下才会取双方最小的带宽进行通信,具体流程在 上游 Hysteria 文档中有描述

  1. 有 hy2 节点不支持 brutal 的可能性吗

实际上确实有可能存在「服务端和客户端都提供了带宽参数,但是实际上不支持 Brutal」的情况,且根据协议这似乎是一个 Unspecified Behavior。但是由于我们的协商行为和官方客户端一致,所以当这种这种情况存在时,则该服务端将和一般通过官方客户端的互操作性也会难以保证,所以我们大概可以忽略这种情况。

@mzz2017
Copy link
Contributor

mzz2017 commented Sep 19, 2024

@mnixry 我不是一个 hy2 用户,但你解释的非常清楚,且对于我了解 PR 相关的风险非常重要,多谢解答

@mzz2017
Copy link
Contributor

mzz2017 commented Sep 19, 2024

@mnixry 麻烦在 pr 描述中也加入配置文件的示例,谢谢

@mnixry
Copy link
Contributor Author

mnixry commented Sep 19, 2024

麻烦在 pr 描述中也加入配置文件的示例,谢谢

done


# The maximum bandwidth for accessing the Internet. It is useful for some specific protocols (e.g., Hysteria2),
# which will perform better with bandwith information provided. The unit is **byte** per second.
bandwidth_max_tx: 26214400 # 200Mbps == 25MB/s == 26214400 B/s uplink
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not easily add global parameters. Besides, there are not many protocols that support bandwidth settings (as far as I know). It is sufficient to design a bandwidth query parameter in the URI schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not easily add global parameters. Besides, there are not many protocols that support bandwidth settings (as far as I know). It is sufficient to design a bandwidth query parameter in the URI schema.

@douglarek Thanks for the review.

The upstream PR also includes supports for importing maxTx and maxRx from the URI scheme, and these have higher priority over global configurations.

There are several reasons to consider adding bandwidth settings as a global option:

  1. When using subscriptions with URIs provided by external sources, you might not have control over the URI parameters they include.
  2. In principle, a URI should contain only server-related details, ensuring it can be shared across devices without modifications. Bandwidth preferences, on the other hand, are client-specific and shouldn’t be universally shared.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not easily add global parameters. Besides, there are not many protocols that support bandwidth settings (as far as I know). It is sufficient to design a bandwidth query parameter in the URI schema.

@douglarek Thanks for the review.

The upstream PR also includes supports for importing maxTx and maxRx from the URI scheme, and these have higher priority over global configurations.

There are several reasons to consider adding bandwidth settings as a global option:

1. When using subscriptions with URIs provided by external sources, you might not have control over the URI parameters they include.

2. In principle, a URI should contain only server-related details, ensuring it can be shared across devices without modifications. Bandwidth preferences, on the other hand, are client-specific and shouldn’t be universally shared.

Regarding what you mentioned in point 1, I think the idea is good, but its actual implementation may not be perfect. If you have two subscriptions and their bandwidths are different, this global setting would not be useful. Regarding 2, this is actually intentional and cannot be considered a general rule. This is essentially a major pitfall set by the protocol designer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants