From f9462eea274d2205908ceb8d3b27dff1ce19149b Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Thu, 20 Jun 2024 13:52:32 +0200 Subject: [PATCH] Fix dns route retrieval condition (#2165) * Fix route retrieval condition * Make error messages take domains into account --- management/server/account.go | 15 ++++++++------- management/server/route.go | 27 ++++++++++++++++++--------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index 99ed654a21..5dd3c8b266 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -18,6 +18,11 @@ import ( "github.com/eko/gocache/v3/cache" cacheStore "github.com/eko/gocache/v3/store" + gocache "github.com/patrickmn/go-cache" + "github.com/rs/xid" + log "github.com/sirupsen/logrus" + "golang.org/x/exp/maps" + "github.com/netbirdio/netbird/base62" nbdns "github.com/netbirdio/netbird/dns" "github.com/netbirdio/netbird/management/domain" @@ -33,10 +38,6 @@ import ( "github.com/netbirdio/netbird/management/server/posture" "github.com/netbirdio/netbird/management/server/status" "github.com/netbirdio/netbird/route" - gocache "github.com/patrickmn/go-cache" - "github.com/rs/xid" - log "github.com/sirupsen/logrus" - "golang.org/x/exp/maps" ) const ( @@ -383,9 +384,9 @@ func (a *Account) getRoutingPeerRoutes(peerID string) (enabledRoutes []*route.Ro func (a *Account) GetRoutesByPrefixOrDomains(prefix netip.Prefix, domains domain.List) []*route.Route { var routes []*route.Route for _, r := range a.Routes { - if r.IsDynamic() && r.Domains.PunycodeString() == domains.PunycodeString() { - routes = append(routes, r) - } else if r.Network.String() == prefix.String() { + dynamic := r.IsDynamic() + if dynamic && r.Domains.PunycodeString() == domains.PunycodeString() || + !dynamic && r.Network.String() == prefix.String() { routes = append(routes, r) } } diff --git a/management/server/route.go b/management/server/route.go index 8741cc47de..2fae8ab8de 100644 --- a/management/server/route.go +++ b/management/server/route.go @@ -1,6 +1,7 @@ package server import ( + "fmt" "net/netip" "unicode/utf8" @@ -51,7 +52,7 @@ func (am *DefaultAccountManager) checkRoutePrefixOrDomainsExistForPeers(account for _, prefixRoute := range routesWithPrefix { // we skip route(s) with the same network ID as we want to allow updating of the existing route - // when create a new route routeID is newly generated so nothing will be skipped + // when creating a new route routeID is newly generated so nothing will be skipped if routeID == prefixRoute.ID { continue } @@ -65,8 +66,9 @@ func (am *DefaultAccountManager) checkRoutePrefixOrDomainsExistForPeers(account group := account.GetGroup(groupID) if group == nil { return status.Errorf( - status.InvalidArgument, "failed to add route with prefix %s - peer group %s doesn't exist", - prefix.String(), groupID) + status.InvalidArgument, "failed to add route with %s - peer group %s doesn't exist", + getRouteDescriptor(prefix, domains), groupID, + ) } for _, pID := range group.Peers { @@ -83,18 +85,18 @@ func (am *DefaultAccountManager) checkRoutePrefixOrDomainsExistForPeers(account } if _, ok := seenPeers[peerID]; ok { return status.Errorf(status.AlreadyExists, - "failed to add route with prefix %s - peer %s already has this route", prefix.String(), peerID) + "failed to add route with %s - peer %s already has this route", getRouteDescriptor(prefix, domains), peerID) } } // check that peerGroupIDs are not in any route peerGroups list for _, groupID := range peerGroupIDs { - group := account.GetGroup(groupID) // we validated the group existent before entering this function, o need to check again. + group := account.GetGroup(groupID) // we validated the group existence before entering this function, no need to check again. if _, ok := seenPeerGroups[groupID]; ok { return status.Errorf( - status.AlreadyExists, "failed to add route with prefix %s - peer group %s already has this route", - prefix.String(), group.Name) + status.AlreadyExists, "failed to add route with %s - peer group %s already has this route", + getRouteDescriptor(prefix, domains), group.Name) } // check that the peers from peerGroupIDs groups are not the same peers we saw in routesWithPrefix @@ -105,8 +107,8 @@ func (am *DefaultAccountManager) checkRoutePrefixOrDomainsExistForPeers(account return status.Errorf(status.InvalidArgument, "peer with ID %s not found", peerID) } return status.Errorf(status.AlreadyExists, - "failed to add route with prefix %s - peer %s from the group %s already has this route", - prefix.String(), peer.Name, group.Name) + "failed to add route with %s - peer %s from the group %s already has this route", + getRouteDescriptor(prefix, domains), peer.Name, group.Name) } } } @@ -114,6 +116,13 @@ func (am *DefaultAccountManager) checkRoutePrefixOrDomainsExistForPeers(account return nil } +func getRouteDescriptor(prefix netip.Prefix, domains domain.List) string { + if len(domains) > 0 { + return fmt.Sprintf("domains [%s]", domains.SafeString()) + } + return fmt.Sprintf("prefix %s", prefix.String()) +} + // CreateRoute creates and saves a new route func (am *DefaultAccountManager) CreateRoute(accountID string, prefix netip.Prefix, networkType route.NetworkType, domains domain.List, peerID string, peerGroupIDs []string, description string, netID route.NetID, masquerade bool, metric int, groups []string, enabled bool, userID string, keepRoute bool) (*route.Route, error) { unlock := am.Store.AcquireAccountWriteLock(accountID)