Proactively expire peers' login per account #698
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
braginini
changed the title
braginini
requested review from
mlsmaycon,
gigovich,
pappz and
pascal-fischer
and removed request for
mlsmaycon
pascal-fischer
requested changes
Feb 23, 2023
management/server/account.go
Outdated
| @@ -307,6 +308,53 @@ func (a *Account) GetGroup(groupID string) *Group { | |||
| return a.Groups[groupID] | |||
| } | |||
|
|
|||
| // GetExpiredPeers returns peers tha have been expired | |||
There was a problem hiding this comment.
Missing a 't' in that
management/server/account.go
Outdated
| } | ||
|
|
||
| // GetNextPeerExpiration returns the minimum duration in which the next peer of the account will expire and true if it was found. | ||
| // If there is no peer that expires this function returns false and a zero time.Duration |
There was a problem hiding this comment.
we return max value not 0 if no peer is found
If there is no peer that is not yet expired, this function returns false and the max time.Duration value as default
management/server/account.go
Outdated
Comment on lines
330
to
344
| if len(peersWithExpiry) == 0 { | ||
| return nextExpiry, false | ||
| } | ||
| for _, peer := range peersWithExpiry { | ||
| // consider only connected peers because others will require login on connecting to the management server | ||
| if peer.Status.LoginExpired || !peer.Status.Connected { | ||
| continue | ||
| } | ||
| _, duration := peer.LoginExpired(a.Settings.PeerLoginExpiration) | ||
| if duration < nextExpiry { | ||
| nextExpiry = duration | ||
| } | ||
| } | ||
|
|
||
| return nextExpiry, nextExpiry != time.Duration(1<<63-1) |
There was a problem hiding this comment.
Maybe go with nil instead of max value for nextExpiry if no more expiring peers are available as it is more clean from a coding point of view to not return a value if there is no value available.
management/server/account.go
Outdated
Comment on lines
727
to
737
| for _, peer := range account.GetExpiredPeers() { | ||
| peerIDs = append(peerIDs, peer.ID) | ||
| peer.MarkLoginExpired(true) | ||
| account.UpdatePeer(peer) | ||
| err = am.Store.SavePeerStatus(account.Id, peer.ID, *peer.Status) | ||
| if err != nil { | ||
| log.Errorf("failed saving peer status while expiring peer %s", peer.ID) | ||
| // todo return retry? | ||
| return false, 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure on this one but I think we do way to much computing overhead here.
account.GetExpiredPeers() will return the complete list of expired peers. Even peers that have been expired for months. So in case we we have lots of zombies in the account that are not used anymore but are doing database overrides and peer updates for all of them on any run with the same (unchanged) data.
I would add additional check for status changes at the top of the loop:
if peer.Status.LoginExpired {
continue
}
| } | ||
| } | ||
|
|
||
| log.Debugf("discovered %d peers to expire for account %s", len(peerIDs), account.Id) |
There was a problem hiding this comment.
Log message should also show wrong number of peers. Not newly expired but total expired in account
pascal-fischer
approved these changes
Feb 27, 2023
pulsastrix
pushed a commit
to pulsastrix/netbird
that referenced
this pull request
Dec 24, 2023
Goals: Enable peer login expiration when adding new peer Expire peer's login when the time comes The account manager triggers peer expiration routine in future if the following conditions are true: peer expiration is enabled for the account there is at least one peer that has expiration enabled and is connected The time of the next expiration check is based on the nearest peer expiration. Account manager finds a peer with the oldest last login (auth) timestamp and calculates the time when it has to run the routine as a sum of the configured peer login expiration duration and the peer's last login time. When triggered, the expiration routine checks whether there are expired peers. The management server closes the update channel of these peers and updates network map of other peers to exclude expired peers so that the expired peers are not able to connect anywhere. The account manager can reschedule or cancel peer expiration in the following cases: when admin changes account setting (peer expiration enable/disable) when admin updates the expiration duration of the account when admin updates peer expiration (enable/disable) when peer connects (Sync) P.S. The network map calculation was updated to exclude peers that have login expired.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes
Goals:
The account manager triggers peer expiration routine in future if the
following conditions are true:
The time of the next expiration check is based on the nearest peer expiration.
Account manager finds a peer with the oldest last login (auth) timestamp and
calculates the time when it has to run the routine as a sum of the configured
peer login expiration duration and the peer's last login time.
When triggered, the expiration routine checks whether there are expired peers.
The management server closes the update channel of these peers and updates
network map of other peers to exclude expired peers so that the expired peers
are not able to connect anywhere.
The account manager can reschedule or cancel peer expiration in the following cases:
P.S. The network map calculation was updated to exclude peers that have login expired.
Issue ticket number and link
Checklist