[management] Skip account peers update if no changes affect peers #2310
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
…eature/validate-group-association
bcmmbaga
changed the title
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Refactor the condition for updating account peers to remove redundant checks Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
bcmmbaga
force-pushed
the
feature/validate-group-association
branch
from
beb8bbd
to
4e2cf9c
Compare
pascal-fischer
requested changes
Sep 12, 2024
management/server/group_test.go
Outdated
Comment on lines
480
to
495
| // adding a group to policy | ||
| err = manager.SavePolicy(context.Background(), account.Id, userID, &Policy{ | ||
| ID: "policy", | ||
| Enabled: true, | ||
| Rules: []*PolicyRule{ | ||
| { | ||
| Enabled: true, | ||
| Sources: []string{"groupA"}, | ||
| Destinations: []string{"groupA"}, | ||
| Bidirectional: true, | ||
| Action: PolicyTrafficActionAccept, | ||
| }, | ||
| }, | ||
| }) | ||
| assert.NoError(t, err) | ||
|
|
There was a problem hiding this comment.
It is ugly but I think we should copy the group chnaged tests for each resource type?
There was a problem hiding this comment.
Also for policy we should copy the tests for all 3 cases, source and dest group have peers and sorce has peers but not dest and dest has peers but not source
management/server/nameserver_test.go
Outdated
Comment on lines
960
to
997
| t.Run("creating nameserver group with distribution group no peers", func(t *testing.T) { | ||
| done := make(chan struct{}) | ||
| go func() { | ||
| peerShouldNotReceiveUpdate(t, updMsg) | ||
| close(done) | ||
| }() | ||
|
|
||
| newNameServerGroup, err = manager.CreateNameServerGroup( | ||
| context.Background(), account.Id, "ns-group-1", "ns-group-1", []nbdns.NameServer{{ | ||
| IP: netip.MustParseAddr(peer1.IP.String()), | ||
| NSType: nbdns.UDPNameServerType, | ||
| Port: nbdns.DefaultDNSPort, | ||
| }}, | ||
| []string{"group-id"}, | ||
| true, []string{}, true, userID, false, | ||
| ) | ||
| assert.NoError(t, err) | ||
|
|
||
| select { | ||
| case <-done: | ||
| case <-time.After(time.Second): | ||
| t.Error("timeout waiting for peerShouldNotReceiveUpdate") | ||
| } | ||
| }) | ||
|
|
||
| err = manager.GroupAddPeer(context.Background(), account.Id, "group-id", peer1.ID) | ||
| assert.NoError(t, err) | ||
|
|
||
| // saving a nameserver group with a distribution group with peers should update account peers and send peer update | ||
| t.Run("saving nameserver group with distribution group has peers", func(t *testing.T) { | ||
| done := make(chan struct{}) | ||
| go func() { | ||
| peerShouldReceiveUpdate(t, updMsg) | ||
| close(done) | ||
| }() | ||
|
|
||
| err = manager.SaveNameServerGroup(context.Background(), account.Id, userID, newNameServerGroup) | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
We should redo both tests for creating nameserver with group that has peers and saving nameserver with group that has no peers
management/server/peer_test.go
Outdated
Comment on lines
1091
to
1123
| // use the group-id in policy | ||
| err = manager.SavePolicy(context.Background(), account.Id, userID, &Policy{ | ||
| ID: "policy", | ||
| Enabled: true, | ||
| Rules: []*PolicyRule{ | ||
| { | ||
| Enabled: true, | ||
| Sources: []string{"group-id"}, | ||
| Destinations: []string{"group-id"}, | ||
| Bidirectional: true, | ||
| Action: PolicyTrafficActionAccept, | ||
| }, | ||
| }, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Adding peer with a used group in active dns, route or policy should update account peers and send peer update | ||
| t.Run("adding peer with used group", func(t *testing.T) { | ||
| done := make(chan struct{}) | ||
| go func() { | ||
| peerShouldReceiveUpdate(t, updMsg) | ||
| close(done) | ||
| }() | ||
|
|
||
| key, err := wgtypes.GeneratePrivateKey() | ||
| require.NoError(t, err) | ||
|
|
||
| expectedPeerKey := key.PublicKey().String() | ||
| peer4, _, _, err = manager.AddPeer(context.Background(), "", "regularUser1", &nbpeer.Peer{ | ||
| Key: expectedPeerKey, | ||
| LoginExpirationEnabled: true, | ||
| Meta: nbpeer.PeerSystemMeta{Hostname: expectedPeerKey}, | ||
| }) |
There was a problem hiding this comment.
Here we should also copy the tests for all resource types
Comment on lines
+1031
to
+1047
| t.Run("updating not expired peer and peer expiration is enabled", func(t *testing.T) { | ||
| done := make(chan struct{}) | ||
| go func() { | ||
| peerShouldNotReceiveUpdate(t, updMsg) | ||
| close(done) | ||
| }() | ||
|
|
||
| _, err := manager.UpdatePeer(context.Background(), account.Id, userID, peer2) | ||
| require.NoError(t, err) | ||
|
|
||
| select { | ||
| case <-done: | ||
| case <-time.After(time.Second): | ||
| t.Error("timeout waiting for peerShouldNotReceiveUpdate") | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
Here we need more tests. E.g. peer label chnaged and login expired and the related ones with expiration disabled maybe
management/server/user_test.go
Outdated
| require.NoError(t, err) | ||
|
|
||
| // updating user with linked peers should update account peers and send peer update | ||
| t.Run("updating user with no linked peers", func(t *testing.T) { |
There was a problem hiding this comment.
Suggested change
| t.Run("updating user with no linked peers", func(t *testing.T) { | |
| t.Run("updating user with linked peers", func(t *testing.T) { |
management/server/user_test.go
Outdated
| } | ||
| }) | ||
|
|
||
| _ = peer4 |
management/server/user_test.go
Outdated
| manager.peersUpdateManager.CloseChannel(context.Background(), peer4.ID) | ||
| }) | ||
|
|
||
| // deleting user with linked peers should update account peers and no send peer update |
There was a problem hiding this comment.
Suggested change
| // deleting user with linked peers should update account peers and no send peer update | |
| // deleting user with linked peers should update account peers and send peer update |
Comment on lines
+302
to
+313
| t.Run("Updating firewall rule", func(t *testing.T) { | ||
| newUpdateMessage1 := createMockUpdateMessage(t) | ||
| newUpdateMessage2 := createMockUpdateMessage(t) | ||
|
|
||
| newUpdateMessage2.NetworkMap.FirewallRules[0].Port = "443" | ||
| newUpdateMessage2.Update.NetworkMap.Serial++ | ||
|
|
||
| message, err := isNewPeerUpdateMessage(newUpdateMessage1, newUpdateMessage2) | ||
| assert.NoError(t, err) | ||
| assert.True(t, message) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Either we need to check each single field for changes in the test or preferably add tests for the diff classes. We need to test for diff in each possible field while making sure that if we add a field that the test is not covering we need to notice. Or we are able to do the diffs generic whitout the need of checking each field separately
bcmmbaga
force-pushed
the
feature/validate-group-association
branch
from
d37e482
to
4e2cf9c
Compare
…date-group-association # Conflicts: # management/server/account.go # management/server/peer.go # management/server/peer_test.go # management/server/policy.go # management/server/route.go # management/server/route_test.go
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
bcmmbaga
changed the title
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
|
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
This PR optimizes account peer updates to trigger only when necessary, reducing unnecessary network map processing and updates. These changes improve efficiency by avoiding redundant updates while ensuring all necessary updates are still performed.
Note: Network map updates are sent only if the new network map differs from the last one sent, except for TURN credential refresh updates. This occurs when the network map has been updated and has a higher serial number than the previous one.
Changes:
Posture Checks:
Policies:
Setup Keys:
Groups:
DNS Settings:
NameServer Groups:
Routes:
Users:
Peers:
Removed unused
UpdatePeerSSHKeymethod.Issue ticket number and link
Checklist