netbirdio / netbird

Connect your devices into a secure WireGuard®-based overlay network with SSO, MFA and granular access controls.
https://netbird.io
BSD 3-Clause "New" or "Revised" License
11.16k stars 514 forks source link

Connected field is true after netbird down #2110

Open ctrl-zzz opened 5 months ago

ctrl-zzz commented 5 months ago

Describe the problem

I've been trying to implement a session-expire feature that checks the connected status of a peer; when a peer gets disconnected its login is marked as expired. The problem is that after netbird down the connected status seems to be stuck at true, so the session can never expire.

I've made sure my code doesn't interfere with the expected behavior and double checked without applying my changes that the problem persists.

To Reproduce

Steps to reproduce the behavior:

  1. In the management/server/account.go file print the peer.Status.Connected value immediately after the MarkPeerConnected call
  2. Deploy code
  3. Get NetBird started
  4. Perform netbird up and netbird down
  5. Check printed value of the connected field

Expected behavior

The connected field should be set to false.

Are you using NetBird Cloud?

No, I'm selfhosting it.

Additional context

When GETting a peer using the NetBird REST API its connected field behaves normally, being set at false after netbird down.

ctrl-zzz commented 5 months ago

Hello,

I've been looking for the issue and I noticed that the connected field is also set to true in the db, even after a disconnection. There's not a problem while retrieving the value, it just never gets updated after a netbird down.

The problem should be in how the field gets updated in the SavePeerStatus function in the management/server/sql_store.go file, particularly because of how GORM manages the update itself. There's an explanation on the GORM documentation on why this happens; updating the SavePeerStatus function accordingly solved the problem for me. Now the status field gets updated correctly and its value in the db stays consistent.

This could be an issue for other updates as well, so I wanted to give you a heads up about it.