add todos

Fix a bug where the post context was canceled before sending metrics to the server.

The interval time was decreased, and an optional environment variable NETBIRD_METRICS_INTERVAL_IN_SECONDS was added to control the interval time.

* update doc URL

client/ui/route.go

@@ -4,6 +4,7 @@ package main
 
 import (
 	"fmt"
+	"sort"
 	"strings"
 	"time"
 
@@ -24,6 +25,7 @@ const (
 	allRoutes             filter = "all"
 	overlappingRoutes     filter = "overlapping"
 	exitNodeRoutes        filter = "exit-node"
+	getClientFMT                 = "get client: %v"
 )
 
 type filter string
@@ -95,6 +97,8 @@ func (s *serviceClient) updateRoutes(grid *fyne.Container, f filter) {
 		return
 	}
 
+	sortRoutesByIDs(filteredRoutes)
+
 	for _, route := range filteredRoutes {
 		r := route
 
@@ -150,8 +154,8 @@ func (s *serviceClient) updateRoutes(grid *fyne.Container, f filter) {
 func (s *serviceClient) getFilteredRoutes(f filter) ([]*proto.Route, error) {
 	routes, err := s.fetchRoutes()
 	if err != nil {
-		log.Errorf("get client: %v", err)
-		s.showError(fmt.Errorf("get client: %v", err))
+		log.Errorf(getClientFMT, err)
+		s.showError(fmt.Errorf(getClientFMT, err))
 		return nil, err
 	}
 	switch f {
@@ -168,6 +172,9 @@ func getOverlappingRoutes(routes []*proto.Route) []*proto.Route {
 	var filteredRoutes []*proto.Route
 	existingRange := make(map[string][]*proto.Route)
 	for _, route := range routes {
+		if len(route.Domains) > 0 {
+			continue
+		}
 		if r, exists := existingRange[route.GetNetwork()]; exists {
 			r = append(r, route)
 			existingRange[route.GetNetwork()] = r
@@ -192,10 +199,17 @@ func getExitNodeRoutes(routes []*proto.Route) []*proto.Route {
 	}
 	return filteredRoutes
 }
+
+func sortRoutesByIDs(routes []*proto.Route) {
+	sort.Slice(routes, func(i, j int) bool {
+		return strings.ToLower(routes[i].GetID()) < strings.ToLower(routes[j].GetID())
+	})
+}
+
 func (s *serviceClient) fetchRoutes() ([]*proto.Route, error) {
 	conn, err := s.getSrvClient(defaultFailTimeout)
 	if err != nil {
-		return nil, fmt.Errorf("get client: %v", err)
+		return nil, fmt.Errorf(getClientFMT, err)
 	}
 
 	resp, err := conn.ListRoutes(s.ctx, &proto.ListRoutesRequest{})
@@ -209,8 +223,8 @@ func (s *serviceClient) fetchRoutes() ([]*proto.Route, error) {
 func (s *serviceClient) selectRoute(id string, checked bool) {
 	conn, err := s.getSrvClient(defaultFailTimeout)
 	if err != nil {
-		log.Errorf("get client: %v", err)
-		s.showError(fmt.Errorf("get client: %v", err))
+		log.Errorf(getClientFMT, err)
+		s.showError(fmt.Errorf(getClientFMT, err))
 		return
 	}
 
@@ -239,7 +253,7 @@ func (s *serviceClient) selectRoute(id string, checked bool) {
 func (s *serviceClient) selectAllFilteredRoutes(f filter) {
 	conn, err := s.getSrvClient(defaultFailTimeout)
 	if err != nil {
-		log.Errorf("get client: %v", err)
+		log.Errorf(getClientFMT, err)
 		return
 	}
 
@@ -256,7 +270,7 @@ func (s *serviceClient) selectAllFilteredRoutes(f filter) {
 func (s *serviceClient) deselectAllFilteredRoutes(f filter) {
 	conn, err := s.getSrvClient(defaultFailTimeout)
 	if err != nil {
-		log.Errorf("get client: %v", err)
+		log.Errorf(getClientFMT, err)
 		return
 	}
 

management/server/account.go

@@ -1726,6 +1726,7 @@ func (am *DefaultAccountManager) GetAccountFromToken(ctx context.Context, claims
 								log.WithContext(ctx).Errorf("failed to save account: %v", err)
 							} else {
 								log.WithContext(ctx).Tracef("user %s: JWT group membership changed, updating account peers", claims.UserId)
+								// todo: optimize this as part of the group optimizations
 								am.updateAccountPeers(ctx, account)
 								unlock()
 								alreadyUnlocked = true

management/server/dns.go

@@ -108,6 +108,7 @@ func (am *DefaultAccountManager) SaveDNSSettings(ctx context.Context, accountID
 		am.StoreEvent(ctx, userID, accountID, accountID, activity.GroupRemovedFromDisabledManagementGroups, meta)
 	}
 
+	// todo: check if before/after groups are in use by dns, acl, routes and if it has peers
 	am.updateAccountPeers(ctx, account)
 
 	return nil

management/server/group.go

@@ -155,6 +155,7 @@ func (am *DefaultAccountManager) SaveGroup(ctx context.Context, accountID, userI
 		return err
 	}
 
+	// todo: check if groups is in use by dns, acl, routes and before/after peers
 	am.updateAccountPeers(ctx, account)
 
 	// the following snippet tracks the activity and stores the group events in the event store.
@@ -322,6 +323,7 @@ func (am *DefaultAccountManager) DeleteGroup(ctx context.Context, accountId, use
 
 	am.StoreEvent(ctx, userId, groupID, accountId, activity.GroupDeleted, g.EventMeta())
 
+	// todo: check if groups is in use by dns, acl, routes and if it has peers
 	am.updateAccountPeers(ctx, account)
 
 	return nil
@@ -376,6 +378,7 @@ func (am *DefaultAccountManager) GroupAddPeer(ctx context.Context, accountID, gr
 		return err
 	}
 
+	// todo: check if groups is in use by dns, acl, routes
 	am.updateAccountPeers(ctx, account)
 
 	return nil
@@ -406,6 +409,7 @@ func (am *DefaultAccountManager) GroupDeletePeer(ctx context.Context, accountID,
 		}
 	}
 
+	// todo: check if groups is in use by dns, acl, routes
 	am.updateAccountPeers(ctx, account)
 
 	return nil

management/server/metrics/selfhosted.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"os"
 	"sort"
 	"strings"
 	"time"
@@ -24,7 +25,7 @@ const (
 	// payloadEndpoint metrics defaultEndpoint to send anonymous data
 	payloadEndpoint = "https://metrics.netbird.io"
 	// defaultPushInterval default interval to push metrics
-	defaultPushInterval = 24 * time.Hour
+	defaultPushInterval = 12 * time.Hour
 	// requestTimeout http request timeout
 	requestTimeout = 45 * time.Second
 )
@@ -82,7 +83,9 @@ func NewWorker(ctx context.Context, id string, dataSource DataSource, connManage
 
 // Run runs the metrics worker
 func (w *Worker) Run(ctx context.Context) {
-	pushTicker := time.NewTicker(defaultPushInterval)
+	interval := getMetricsInterval(ctx)
+
+	pushTicker := time.NewTicker(interval)
 	for {
 		select {
 		case <-w.ctx.Done():
@@ -97,6 +100,20 @@ func (w *Worker) Run(ctx context.Context) {
 	}
 }
 
+func getMetricsInterval(ctx context.Context) time.Duration {
+	interval := defaultPushInterval
+	if os.Getenv("NETBIRD_METRICS_INTERVAL_IN_SECONDS") != "" {
+		newInterval, err := time.ParseDuration(os.Getenv("NETBIRD_METRICS_INTERVAL_IN_SECONDS") + "s")
+		if err != nil {
+			log.WithContext(ctx).Errorf("unable to parse NETBIRD_METRICS_INTERVAL_IN_SECONDS, using default interval %v. Error: %v", defaultPushInterval, err)
+		} else {
+			log.WithContext(ctx).Infof("using NETBIRD_METRICS_INTERVAL_IN_SECONDS %s", newInterval)
+			interval = newInterval
+		}
+	}
+	return interval
+}
+
 func (w *Worker) sendMetrics(ctx context.Context) error {
 	apiKey, err := getAPIKey(w.ctx)
 	if err != nil {
@@ -112,10 +129,11 @@ func (w *Worker) sendMetrics(ctx context.Context) error {
 
 	httpClient := http.Client{}
 
-	exportJobReq, err := createPostRequest(w.ctx, payloadEndpoint+"/capture/", payloadString)
+	exportJobReq, cancelCTX, err := createPostRequest(w.ctx, payloadEndpoint+"/capture/", payloadString)
 	if err != nil {
 		return fmt.Errorf("unable to create metrics post request %v", err)
 	}
+	defer cancelCTX()
 
 	jobResp, err := httpClient.Do(exportJobReq)
 	if err != nil {
@@ -135,7 +153,7 @@ func (w *Worker) sendMetrics(ctx context.Context) error {
 
 	log.WithContext(ctx).Infof("sent anonymous metrics, next push will happen in %s. "+
 		"You can disable these metrics by running with flag --disable-anonymous-metrics,"+
-		" see more information at https://netbird.io/docs/FAQ/metrics-collection", defaultPushInterval)
+		" see more information at https://docs.netbird.io/about-netbird/faq#why-and-what-are-the-anonymous-usage-metrics", getMetricsInterval(ctx))
 
 	return nil
 }
@@ -373,20 +391,20 @@ func buildMetricsPayload(payload pushPayload) (string, error) {
 	return string(str), nil
 }
 
-func createPostRequest(ctx context.Context, endpoint string, payloadStr string) (*http.Request, error) {
+func createPostRequest(ctx context.Context, endpoint string, payloadStr string) (*http.Request, context.CancelFunc, error) {
 	ctx, cancel := context.WithTimeout(ctx, requestTimeout)
-	defer cancel()
 	reqURL := endpoint
 
 	payload := strings.NewReader(payloadStr)
 
 	req, err := http.NewRequestWithContext(ctx, "POST", reqURL, payload)
 	if err != nil {
-		return nil, err
+		cancel()
+		return nil, nil, err
 	}
 	req.Header.Add("content-type", "application/json")
 
-	return req, nil
+	return req, cancel, nil
 }
 
 func getMinMaxVersion(inputList []string) (string, string) {

management/server/nameserver.go

@@ -85,6 +85,7 @@ func (am *DefaultAccountManager) CreateNameServerGroup(ctx context.Context, acco
 		return nil, err
 	}
 
+	// todo: check distribution groups if they have any peers
 	am.updateAccountPeers(ctx, account)
 
 	am.StoreEvent(ctx, userID, newNSGroup.ID, accountID, activity.NameserverGroupCreated, newNSGroup.EventMeta())
@@ -120,6 +121,7 @@ func (am *DefaultAccountManager) SaveNameServerGroup(ctx context.Context, accoun
 		return err
 	}
 
+	// todo: check before and after distribution groups if they have any peers
 	am.updateAccountPeers(ctx, account)
 
 	am.StoreEvent(ctx, userID, nsGroupToSave.ID, accountID, activity.NameserverGroupUpdated, nsGroupToSave.EventMeta())
@@ -150,6 +152,7 @@ func (am *DefaultAccountManager) DeleteNameServerGroup(ctx context.Context, acco
 		return err
 	}
 
+	// todo: check distribution groups if they have any peers
 	am.updateAccountPeers(ctx, account)
 
 	am.StoreEvent(ctx, userID, nsGroup.ID, accountID, activity.NameserverGroupDeleted, nsGroup.EventMeta())

management/server/peer.go

@@ -7,10 +7,11 @@ import (
 	"strings"
 	"time"
 
-	"github.com/netbirdio/netbird/management/server/posture"
 	"github.com/rs/xid"
 	log "github.com/sirupsen/logrus"
 
+	"github.com/netbirdio/netbird/management/server/posture"
+
 	"github.com/netbirdio/netbird/management/proto"
 	"github.com/netbirdio/netbird/management/server/activity"
 	nbpeer "github.com/netbirdio/netbird/management/server/peer"
@@ -217,6 +218,7 @@ func (am *DefaultAccountManager) UpdatePeer(ctx context.Context, accountID, user
 		return nil, err
 	}
 
+	// todo: don't call it if peer is not expired and Peer.LoginExpirationEnabled was set to false
 	am.updateAccountPeers(ctx, account)
 
 	return peer, nil
@@ -289,6 +291,7 @@ func (am *DefaultAccountManager) DeletePeer(ctx context.Context, accountID, peer
 		return err
 	}
 
+	// todo: evaluate if peer was part of a group that has is used in a active dns, route, acl
 	am.updateAccountPeers(ctx, account)
 
 	return nil
@@ -509,6 +512,7 @@ func (am *DefaultAccountManager) AddPeer(ctx context.Context, setupKey, userID s
 
 	am.StoreEvent(ctx, opEvent.InitiatorID, opEvent.TargetID, opEvent.AccountID, opEvent.Activity, opEvent.Meta)
 
+	// todo: evaluate if peer is part of a group that has is used in a active dns, route, acl
 	am.updateAccountPeers(ctx, account)
 
 	approvedPeersMap, err := am.GetValidatedPeers(account)
@@ -544,6 +548,7 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync PeerSync, ac
 			return nil, nil, nil, err
 		}
 
+		// todo: review this logic
 		if sync.UpdateAccountPeers {
 			am.updateAccountPeers(ctx, account)
 		}
@@ -563,6 +568,7 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync PeerSync, ac
 		return peer, emptyMap, postureChecks, nil
 	}
 
+	// todo: review this logic and combine with the previous
 	if isStatusChanged {
 		am.updateAccountPeers(ctx, account)
 	}
@@ -802,11 +808,13 @@ func (am *DefaultAccountManager) checkAndUpdatePeerSSHKey(ctx context.Context, p
 	}
 
 	// trigger network map update
+	// todo: remove this since it is called by the caller function
 	am.updateAccountPeers(ctx, account)
 
 	return peer, nil
 }
 
+// todo: not in use, remove it
 // UpdatePeerSSHKey updates peer's public SSH key
 func (am *DefaultAccountManager) UpdatePeerSSHKey(ctx context.Context, peerID string, sshKey string) error {
 	if sshKey == "" {

management/server/policy.go

@@ -364,6 +364,7 @@ func (am *DefaultAccountManager) SavePolicy(ctx context.Context, accountID, user
 	}
 	am.StoreEvent(ctx, userID, policy.ID, accountID, action, policy.EventMeta())
 
+	// todo: call if before and after source and destination groups are not empty
 	am.updateAccountPeers(ctx, account)
 
 	return nil
@@ -391,6 +392,7 @@ func (am *DefaultAccountManager) DeletePolicy(ctx context.Context, accountID, po
 
 	am.StoreEvent(ctx, userID, policy.ID, accountID, activity.PolicyRemoved, policy.EventMeta())
 
+	// todo: call if source and destination groups are not empty
 	am.updateAccountPeers(ctx, account)
 
 	return nil

management/server/posture_checks.go

@@ -82,6 +82,7 @@ func (am *DefaultAccountManager) SavePostureChecks(ctx context.Context, accountI
 
 	am.StoreEvent(ctx, userID, postureChecks.ID, accountID, action, postureChecks.EventMeta())
 	if exists {
+		// todo: check if posture check is linked to a policy
 		am.updateAccountPeers(ctx, account)
 	}
 

management/server/route.go

@@ -204,7 +204,10 @@ func (am *DefaultAccountManager) CreateRoute(ctx context.Context, accountID stri
 	if err = am.Store.SaveAccount(ctx, account); err != nil {
 		return nil, err
 	}
-
+	// todo: call if one of the three is true:
+	// 1. distribution groups are not empty
+	// 2. routing groups are not empy
+	// 3. there is a routing peer
 	am.updateAccountPeers(ctx, account)
 
 	am.StoreEvent(ctx, userID, string(newRoute.ID), accountID, activity.RouteCreated, newRoute.EventMeta())
@@ -273,7 +276,10 @@ func (am *DefaultAccountManager) SaveRoute(ctx context.Context, accountID, userI
 	if err = am.Store.SaveAccount(ctx, account); err != nil {
 		return err
 	}
-
+	// todo: call if one of the three is true:
+	// 1. before and after distribution groups are not empty
+	// 2. before and after routing groups are not empy
+	// 3. there is a routing peer
 	am.updateAccountPeers(ctx, account)
 
 	am.StoreEvent(ctx, userID, string(routeToSave.ID), accountID, activity.RouteUpdated, routeToSave.EventMeta())
@@ -303,7 +309,10 @@ func (am *DefaultAccountManager) DeleteRoute(ctx context.Context, accountID stri
 	}
 
 	am.StoreEvent(ctx, userID, string(routy.ID), accountID, activity.RouteRemoved, routy.EventMeta())
-
+	// todo: call if one of the three is true:
+	// 1. distribution groups are not empty
+	// 2. routing groups are not empy
+	// 3. there is a routing peer
 	am.updateAccountPeers(ctx, account)
 
 	return nil

management/server/setupkey.go

@@ -320,7 +320,7 @@ func (am *DefaultAccountManager) SaveSetupKey(ctx context.Context, accountID str
 			}
 		}
 	}()
-
+	// todo: remove it, not needed here since we don't update anything else
 	am.updateAccountPeers(ctx, account)
 
 	return newKey, nil

management/server/user.go

@@ -517,6 +517,7 @@ func (am *DefaultAccountManager) deleteRegularUser(ctx context.Context, account
 	meta := map[string]any{"name": tuName, "email": tuEmail, "created_at": tuCreatedAt}
 	am.StoreEvent(ctx, initiatorUserID, targetUserID, account.Id, activity.UserDeleted, meta)
 
+	// todo: call only if user had a peer linked to it and peer propagation is enabled
 	am.updateAccountPeers(ctx, account)
 
 	return nil
@@ -851,7 +852,8 @@ func (am *DefaultAccountManager) SaveOrAddUser(ctx context.Context, accountID, i
 		if err = am.Store.SaveAccount(ctx, account); err != nil {
 			return nil, err
 		}
-
+		// todo: call only if is existing user, it has a peer linked to it and peer propagation is enabled
+		// new users don't need to call this
 		am.updateAccountPeers(ctx, account)
 	} else {
 		if err = am.Store.SaveAccount(ctx, account); err != nil {