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

Properly indicate whether a connection is "LAN" or not in the GUI #8686

Closed
bt90 opened this issue Nov 19, 2022 · 3 comments · Fixed by #8694
Closed

Properly indicate whether a connection is "LAN" or not in the GUI #8686

bt90 opened this issue Nov 19, 2022 · 3 comments · Fixed by #8694
Assignees
@acolomb
Labels
bug A problem with current functionality, as opposed to missing functionality (enhancement) frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Milestone
v1.23.0

Comments

@bt90
Copy link
Contributor

bt90 commented Nov 19, 2022

IPv6 LAN connections currently show up as WAN connections because they use public IPv6 addresses. We should check if the other address is in the same subnet to determine if it's a LAN connection.

Apart from the wrong connection type in the UI, this might also affect bandwidth limitation logic.

https://forum.syncthing.net/t/towards-better-connection-type-icons/19333/23
@bt90 bt90 added enhancement New features or improvements of some kind, as opposed to a problem (bug) needs-triage New issues needed to be validated labels Nov 19, 2022
@bt90
Copy link
Contributor Author

bt90 commented Nov 19, 2022

We should also treat ULA IPs as LAN:

#7456

@calmh
Copy link
Member

calmh commented Nov 19, 2022 •
edited

We do compare against all directly connected networks already. If there is extra logic in the GUI that should probably be changed to just look at an isLAN flag from the backend, which already has that flag (but maybe doesn't expose it).

https://github.com/calmh/syncthing/blob/a7a87cd10ba63cc63a47cef3cb9733292f768746/lib/connections/service.go#L746-L751

@acolomb
Copy link
Member

acolomb commented Nov 21, 2022

I've been poking around in the code, trying to expose that flag to the GUI. Not sure I've got the cleanest possible solution, but at least it does fix the wrong classifications in my tests. Will send a PR soonish to discuss the implementation.

@acolomb acolomb self-assigned this Nov 21, 2022
@acolomb acolomb linked a pull request Nov 27, 2022 that will close this issue
gui: Let the backend decide whether connection is local (fixes #8686) #8694
Merged
1 task
calmh pushed a commit that referenced this issue Nov 28, 2022
@acolomb
gui, lib/connections: Let the backend decide whether connection is lo…
ab0eb90 
…cal (fixes #8686) (#8694)

* lib/connections: Cache isLAN decision for later external access.

The check whether a remote device's address is on a local network
currently happens when handling the Hello message, to configure the
limiters.  Save the result to the ConnectionInfo and pass it out as
part of the model's ConnectionInfo struct in ConnectionStats().

* gui: Use provided connection attribute to distinguish LAN / WAN.

Replace the dumb IP address check which didn't catch common cases and
actually could contradict what the backend decided.  That could have
been confusing if the GUI says WAN, but the limiter is not actually
applied because the backend thinks it's a LAN.

Add strings for QUIC and relay connections to also differentiate
between LAN and WAN.

* gui: Redefine reception level icons for all connection types.

Move the mapping to the JS code, as it is much easier to handle
multiple switch cases by fall-through there.

QUIC is regarded no less than TCP anymore.  LAN and WAN make the
difference between levels 4 / 3 and 2 / 1:

{TCP,QUIC} LAN --> {TCP,QUIC} WAN --> Relay LAN --> Relay WAN -->
Disconnected.
@calmh calmh added this to the v1.22.2 milestone Nov 29, 2022
@calmh calmh modified the milestones: v1.22.2, v1.22.3 Dec 13, 2022
@calmh calmh changed the title LAN detection via subnetmask Properly indicate whether a connection is "LAN" or not in the GUI Dec 13, 2022
@calmh calmh added bug A problem with current functionality, as opposed to missing functionality (enhancement) and removed enhancement New features or improvements of some kind, as opposed to a problem (bug) needs-triage New issues needed to be validated labels Dec 13, 2022
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Nov 28, 2023
@syncthing syncthing locked and limited conversation to collaborators Nov 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@acolomb acolomb

Labels
bug A problem with current functionality, as opposed to missing functionality (enhancement) frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Milestone
v1.23.0
Development

Successfully merging a pull request may close this issue.

gui: Let the backend decide whether connection is local (fixes #8686) acolomb/syncthing
4 participants
@calmh @bt90 @acolomb @st-review