Skip to content

Commit

Permalink
Fix dns route retrieval condition (#2165)
Browse files Browse the repository at this point in the history
* Fix route retrieval condition

* Make error messages take domains into account
  • Loading branch information
lixmal authored Jun 20, 2024
1 parent b075009 commit f9462ee
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 16 deletions.
15 changes: 8 additions & 7 deletions management/server/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 (
Expand Down Expand Up @@ -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)
}
}
Expand Down
27 changes: 18 additions & 9 deletions management/server/route.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package server

import (
"fmt"
"net/netip"
"unicode/utf8"

Expand Down Expand Up @@ -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
}
Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -105,15 +107,22 @@ 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)
}
}
}

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)
Expand Down

0 comments on commit f9462ee

Please sign in to comment.