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

Fix per-server outbound options not taking effect (part 2) #1491

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

stormynoct
Copy link
Contributor

I'm in a multi-WAN environment and I want the two WAN connections to backup each other. I tried to set outbound_fwmark for each server but it seems not working as expected.

By looking into the code I found two issues:

  1. PingBalancer does not set the fwmark correctly in connect_opts so it's not able to detect whether the second WAN is working or not.
  2. The outbound traffic seems to ignore the fwmark options in some cases. The connect_opts are modified correctly in some classes while not in others.

I drafted this change containing the following fixes:

  1. Add outbound_bind_addr and outbound_bind_interface into ServerInstanceConfig
  2. Refactor PingBalancer to hold a copy of per-server ConnectOpts
  3. Add new interfaces for specifying ConnectOpts in AutoProxyClientStream

It works for my case, but I don't have a good environment to be able to test all the logic I added. Also the related code pieces are quite a lot, so not sure if I missed any place.

@zonyitoo
Copy link
Collaborator

zonyitoo commented Apr 6, 2024

This is quite a big PR...

@stormynoct
Copy link
Contributor Author

Though 1 is needed in my environment, I feel like it's quite isolated from 2/3. If you think the PR is too large to review, maybe I can consider to break it down into two. One containing 2/3 and another on top containing only 1.

Maybe we can discuss first on whether you think the change 2/3 makes sense. There is some historical issues that fwmark is placed into Config while server params like addr/pass/method are placed into ServerConfig. ServerInstanceConfig looks like a bridge between these two and can move some additional parameters in Config to be per-server params.

But there is a disconnection that most places initiating an outgoing traffic uses the default ConnectOpts, they does not read ServerInstanceConfig which contains the correct information for outbound traffic. That's why I propose change 2. Both ServerInstanceConfig and cached ConnectOpts should retain a copy in PingBalancer.

@zonyitoo
Copy link
Collaborator

So your idea is to add a ConnectOpts in ServerConfig right? LGTM. Please resolve the conflicts and I am Ok with the other changes.

1. Add outbound_bind_addr and outbound_bind_interface into ServerInstanceConfig
2. Add new interfaces for specifying ConnectOpts in AutoProxyClientStream
@stormynoct stormynoct changed the title Fix per-server outbound options not taking effect Fix per-server outbound options not taking effect (part 2) Apr 14, 2024
@zonyitoo zonyitoo merged commit c68172b into shadowsocks:master Apr 18, 2024
9 checks passed
zonyitoo added a commit that referenced this pull request Nov 27, 2024
Per-server's specific outbound options should be set after cloning the
global connect opts.

- ref #1774
- ref #1491, #1493
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.

2 participants