-
Notifications
You must be signed in to change notification settings - Fork 55
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: revamp config #230
feat: revamp config #230
Conversation
BTW, this is part of my Websockets work. Websockets is stream-based, but we will implement a Packet Dialer on top of a Stream Dialer. |
@fortuna super cool! I am going through the proposed changes. Just to clarify, with this PR, we can have a dialer that sends UDP datagrams through a TCP stream? I guess the reverse is also possible: a TCP stream that is built on the top of a UDP based connection? |
This allows our config library to support those use cases. Yes, we will be able to do TCP streams over h3 (QUIC/UDP), or UDP streams over Websockets/Connect-UDP over h1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have further comments on the code. All looks good to me. Overall, I think the users of the SDK can benefit from an example section in the doc.go that includes a bunch of concrete examples of various dialer configurations and their combinations (e.g. do53:address=8.8.8.8|ss://info@user:pass
, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this is getting more complicated. While it's fine for now, I'd prefer simplifying the APIs or adding helper functions to make it easier for new developers in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this is getting more complicated. While it's fine for now, I'd prefer simplifying the APIs or adding helper functions to make it easier for new developers in the future.
@jyyi1 Could you elaborate on that?
If you look at the changes to the tools, they are not really more complicated.
In config.go, we no longer need to provide implementations saying a protocol is not supported. We get that for free with the on-demand constuction.
Ultimately, the domain is more complicated than our previous model. We can model more things now, which is important. If you have ideas on how to simplify it, please let us know.
With this change a config can use a stream dialer, a packet dialer, both or none. The needs are resolved at dialer time and it depends on whether you are building a stream dialer or a packet dialer. That's why the dialer builder takes functions instead of the actual inner dialers.
This enables new configs that were not possible before. I added a Do53 config, which uses both a stream and packet dialer to create a stream dialer. SOCKS5 UDP is not implemented but is an example that needs a Packet and Stream dialer. QUIC can be a StreamDialer that uses a Packet Dialer.
I also fixed the SS sanitization to not output the prefix when absent and properly encode it.