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.

Sign up for GitHub

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

Jump to bottom

Add ws_proxy option for connecting gateway through a proxy. #2709

Open
wants to merge 98 commits into
base: next
Choose a base branch
from
Open

Add ws_proxy option for connecting gateway through a proxy. #2709

wants to merge 98 commits into from

Conversation

zzzzRuby
Copy link

@zzzzRuby zzzzRuby commented Jan 12, 2024 •
edited

@github-actions github-actions bot added client Related to the `client` module. gateway Related to the `gateway` module. labels Jan 12, 2024
@cheesycod
Copy link
Contributor

This would help me as well. I forked nirn to hackily replace the ws url.

@GnomedDev
Copy link
Member

I'm kinda wondering why this is required at all, can't the gateway url just be exposed (if not already) to point to the proxy, instead of having this proxy option, and why is the manual http-over-tcp required? None of this is commented or explained.

@zzzzRuby
Copy link
Author

I'm kinda wondering why this is required at all, can't the gateway url just be exposed (if not already) to point to the proxy, instead of having this proxy option, and why is the manual http-over-tcp required? None of this is commented or explained.

Sorry I'm new to opensource.
In some network environments like Chinese networks, there must be a proxy to connect with discord gateway. There're two options to do this, the first one is open a tunnel to the proxy likes this pr does, the second one is creating a custom gateway and redirect all message through a tunnel to the proxy. The second one seems too complex.
As to the manual http-over-tcp, I'm not sure if I can add a new dependency in Cargo.toml.

@GnomedDev
Copy link
Member

Can you not perform this proxying system wide, instead of coding support for it into every application? I'm honestly not familar with this.

@zzzzRuby
Copy link
Author

Can you not perform this proxying system wide, instead of coding support for it into every application? I'm honestly not familar with this.

Normally, libraries like libcurl or reqwest get a default proxy from environment or system. But tungstenite doesn't support this and they seem have no interest in this (snapview/tungstenite-rs#177), the only choice to perform a proxy system wide is proxychains-like-tools. But I don't think proxychains is a good idea, it depends on linking libc dynamically, so on some distro like alpine it's hard to use with rust program and sometimes not work.

GnomedDev
GnomedDev reviewed Jan 13, 2024
View reviewed changes
Copy link
Member

@GnomedDev GnomedDev left a comment

Choose a reason for hiding this comment

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

Okay, I understand now, the situation sucks (this should be on tungstenite) but if we can make a positive impact lets go for this

src/client/mod.rs Outdated Show resolved Hide resolved
src/client/mod.rs
@@ -351,6 +371,7 @@ impl IntoFuture for ClientBuilder {
#[cfg(feature = "voice")]
voice_manager: voice_manager.as_ref().map(Arc::clone),
ws_url: Arc::clone(&ws_url),
ws_proxy,
Copy link
Member

Choose a reason for hiding this comment

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

Can this be wrapped in Arc like ws_url, and preprocessed into Url to fail here if the user provides an invalid URL.

Copy link
Author

Choose a reason for hiding this comment

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

Yes it can. Finished

Copy link
Collaborator

Choose a reason for hiding this comment

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

ws_url is only parsed into a Url inside of the gateway::shard::connect helper function. I think for consistency's sake the same should be done with ws_proxy - a followup PR could move the parsing logic for both to be earlier in the initialization stage.

src/gateway/ws.rs
@@ -101,13 +108,72 @@ const TIMEOUT: Duration = Duration::from_millis(500);
const DECOMPRESSION_MULTIPLIER: usize = 3;

impl WsClient {
pub(crate) async fn connect(url: Url) -> Result<Self> {
async fn connect_with_proxy_async(
Copy link
Member

Choose a reason for hiding this comment

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

This whole method should be replaced with something using reqwest, which is already in the library.

Copy link

Choose a reason for hiding this comment

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

This whole method should be replaced with something using reqwest, which is already in the library.

Reqwest doesn't expose API that can be used in websocket proxy. But there is an implementation that borrows from reqwest's design.
snapview/tungstenite-rs#177 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

Just like Itsusinn said. I replaced the http connect code with async-http-proxy[https://crates.io/crates/async-http-proxy] which is mentioned in that issue.

@arqunis arqunis added the enhancement An improvement to Serenity. label Jan 13, 2024
@github-actions github-actions bot added framework Related to the `framework` and `framework::standard` modules and/or the `command_attr` crate model Related to the `model` module. builder Related to the `builder` module. cache Related to the `cache`-feature. http Related to the `http` module. utils Related to the `utils` module. command_attr Related to the `command_attr` crate. examples Related to Serenity's examples. labels Jan 20, 2024
@zzzzRuby zzzzRuby force-pushed the akira-bot-ws-proxy branch 2 times, most recently from 782a253 to 15d9f25 Compare January 20, 2024 07:50
@github-actions github-actions bot removed framework Related to the `framework` and `framework::standard` modules and/or the `command_attr` crate model Related to the `model` module. builder Related to the `builder` module. cache Related to the `cache`-feature. http Related to the `http` module. utils Related to the `utils` module. command_attr Related to the `command_attr` crate. examples Related to Serenity's examples. labels Jan 20, 2024
arqunis and others added 12 commits March 13, 2024 11:08
@arqunis @GnomedDev
Synchronise README example from the current branch
643ad87
@mkrasnitski @GnomedDev
Minor adjustments
3b83005
@GnomedDev
Fix clippy::assigning_clones (serenity-rs#2784)
ddfdb74
@cheesycod @GnomedDev
Apply Strum derives for FullEvent and Event (serenity-rs#2788)
e5eae3a
@GnomedDev
Remove Channel::name (serenity-rs#2787)
ba4f89d 
`Channel::name` just called `to_channel` (cloning the entire `Channel`!) and
`PrivateChannel::name` or `GuildChannel::name`.

`GuildChannel::name` just returned the field of the same name, for consistency
with `PrivateChannel::name`.

`PrivateChannel::name` returned a non-standard format string that would be very
easy to replicate in user code.

I also tweaked the signature of `OnlineStatus::name` to stop passing an 8 byte
reference to a 1 byte struct, and return `&'static str`.
@GnomedDev
Use join_to_string throughout serenity (serenity-rs#2785)
b0abeac
@GnomedDev
Use ToArrayString for int -> str (serenity-rs#2786)
e146b47
@cheesycod @GnomedDev
Rename unstable_discord_api to unstable and disable it by default (…
e19b5c3 
…serenity-rs#2789)

This also disables the `#[non_exhaustive]` attribute on `FullEvent` if
`unstable` is enabled as a feature. 

It is already expected that any change to Serenity with `unstable` enabled may
or may not break code using this feature and making this enum exhaustive allows
for use cases such as fine-grained audit logging, where new variants should be
a compiler error.

Co-authored-by: Gnome! <david2005thomas@Gmail.com>
@GnomedDev
Remove deprecated Channel::is_nsfw methods (serenity-rs#2792)
1cb2757
@GnomedDev
Replace manual Hash/Eq implementation on User/Role (serenity-rs#2793)
0c53087
@GnomedDev
Remove useless method wrappers (serenity-rs#2795)
1890018
@GnomedDev
Fix unused variable warning from rebase
6cfbba0
@GnomedDev GnomedDev force-pushed the next branch 3 times, most recently from 7ad183d to 52f2822 Compare May 14, 2024 20:34
@GnomedDev GnomedDev force-pushed the next branch 2 times, most recently from bda6e4e to 6561f4b Compare May 23, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Itsusinn Itsusinn Itsusinn left review comments

@mkrasnitski mkrasnitski mkrasnitski left review comments

@GnomedDev GnomedDev GnomedDev left review comments

Assignees
No one assigned
Labels
client Related to the `client` module. enhancement An improvement to Serenity. gateway Related to the `gateway` module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet