Fix the Inactivity Expiration problem.

[ismail0234]

Describe your changes

Even if inactivity is disabled before the change, the scheduled method can still be triggered. The method is never triggered when inactivity is disabled after the change. This is necessary to solve the error problem of SQL Tests.

Checklist

• Is it a bug fix
• Is a typo/documentation fix
• Is a feature enhancement
• It is a refactor
• Created tests that fail without the change (if possible)
• Extended the README / documentation, if necessary

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[ismail0234]

@pascal-fischer This fix solves multiple problems.

The value I add here, if there is a new value, we overwrite the old value. Because when the scheduled task runs, it always considers the old value as valid and triggers the scheduled task at the old time.

netbird/management/server/account.go

Line 1193 in 156fa32

oldSettings.PeerInactivityExpiration = newSettings.PeerInactivityExpiration

The method triggering order is as follows.

1. UpdateAccountSettings (account.go)
2. handleInactivityExpirationSettings (account.go)
3. checkAndSchedulePeerInactivityExpiration (account.go)
4. account.GetNextInactivePeerExpiration() (account.go)
5. a.Settings.PeerInactivityExpiration

netbird/management/server/account.go

Lines 1273 to 1279 in 669904c

	// checkAndSchedulePeerInactivityExpiration periodically checks for inactive peers to end their sessions
	func (am *DefaultAccountManager) checkAndSchedulePeerInactivityExpiration(ctx context.Context, account *Account) {
	am.peerInactivityExpiry.Cancel(ctx, []string{account.Id})
	if nextRun, ok := account.GetNextInactivePeerExpiration(); ok {
	go am.peerInactivityExpiry.Schedule(ctx, nextRun, account.Id, am.peerInactivityExpirationJob(ctx, account.Id))
	}
	}

The value “a.Settings.PeerInactivityExpiration” contains the old value.

netbird/management/server/account.go

Line 650 in 669904c

_, duration := peer.SessionExpired(a.Settings.PeerInactivityExpiration)

[ismail0234]

I also recommend you to look carefully at the old code. Even if you set “PeerInactivityExpirationEnabled” to false, the timer continues to be triggered if there is a different “PeerInactivityExpiration” value. There seems to be a bug here.

&Settings{
  PeerInactivityExpirationEnabled: false,
  PeerInactivityExpiration: 100,
}

[ismail0234]

This is the test code that gives an error.

The default values given when calling “manager.UpdateAccountSettings” are (PeerInactivityExpirationEnabled: false) and (PeerInactivityExpiration: 0). But in the current peer, these values are set to "true" and "10 minutes". Therefore, even if you set “PeerInactivityExpirationEnabled” to false, the “PeerInactivityExpiration” value is triggered.

netbird/management/server/account_test.go

Lines 1898 to 1944 in 669904c

	func TestDefaultAccountManager_MarkPeerConnected_PeerLoginExpiration(t *testing.T) {
	manager, err := createManager(t)
	require.NoError(t, err, "unable to create account manager")
	accountID, err := manager.GetAccountIDByUserID(context.Background(), userID, "")
	require.NoError(t, err, "unable to create an account")
	key, err := wgtypes.GenerateKey()
	require.NoError(t, err, "unable to generate WireGuard key")
	_, _, _, err = manager.AddPeer(context.Background(), "", userID, &nbpeer.Peer{
	Key: key.PublicKey().String(),
	Meta: nbpeer.PeerSystemMeta{Hostname: "test-peer"},
	LoginExpirationEnabled: true,
	})
	require.NoError(t, err, "unable to add peer")
	_, err = manager.UpdateAccountSettings(context.Background(), accountID, userID, &Settings{
	PeerLoginExpiration: time.Hour,
	PeerLoginExpirationEnabled: true,
	})
	require.NoError(t, err, "expecting to update account settings successfully but got error")
	wg := &sync.WaitGroup{}
	wg.Add(2)
	manager.peerLoginExpiry = &MockScheduler{
	CancelFunc: func(ctx context.Context, IDs []string) {
	wg.Done()
	},
	ScheduleFunc: func(ctx context.Context, in time.Duration, ID string, job func() (nextRunIn time.Duration, reschedule bool)) {
	wg.Done()
	},
	}
	accountID, err = manager.GetAccountIDByUserID(context.Background(), userID, "")
	require.NoError(t, err, "unable to get the account")
	account, err := manager.Store.GetAccount(context.Background(), accountID)
	require.NoError(t, err, "unable to get the account")
	// when we mark peer as connected, the peer login expiration routine should trigger
	err = manager.MarkPeerConnected(context.Background(), key.PublicKey().String(), true, nil, account)
	require.NoError(t, err, "unable to mark peer connected")
	failed := waitTimeout(wg, time.Second)
	if failed {
	t.Fatal("timeout while waiting for test to finish")
	}
	}

[ismail0234]

This ensures that “PeerInactivityExpirationEnabled” is only triggered when true. It also makes the new value “PeerInactivityExpiration” work.

[pascal-fischer]

Okay, you have me convinced. The PeerInactivityExpiration with 0 and the default is triggering this. It makes sense to disregard the timeout when the expiration is not enabled in the first place. I checked the code and tested the feature, and everything seems to be working fine.