Don't abort UI debug bundle when up/down fails

client/cmd/service_test.go

@@ -4,9 +4,7 @@ import (
 	"context"
 	"fmt"
 	"os"
-	"os/signal"
 	"runtime"
-	"syscall"
 	"testing"
 	"time"
 
@@ -15,22 +13,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// TestMain intercepts when this test binary is run as a daemon subprocess.
-// On FreeBSD, the rc.d service script runs the binary via daemon(8) -r with
-// "service run ..." arguments. Since the test binary can't handle cobra CLI
-// args, it exits immediately, causing daemon -r to respawn rapidly until
-// hitting the rate limit and exiting. This makes service restart unreliable.
-// Blocking here keeps the subprocess alive until the init system sends SIGTERM.
-func TestMain(m *testing.M) {
-	if len(os.Args) > 2 && os.Args[1] == "service" && os.Args[2] == "run" {
-		sig := make(chan os.Signal, 1)
-		signal.Notify(sig, syscall.SIGTERM, os.Interrupt)
-		<-sig
-		return
-	}
-	os.Exit(m.Run())
-}
-
 const (
 	serviceStartTimeout = 10 * time.Second
 	serviceStopTimeout  = 5 * time.Second
@@ -97,34 +79,6 @@ func TestServiceLifecycle(t *testing.T) {
 	logLevel = "info"
 	daemonAddr = fmt.Sprintf("unix://%s/netbird-test.sock", tempDir)
 
-	// Ensure cleanup even if a subtest fails and Stop/Uninstall subtests don't run.
-	t.Cleanup(func() {
-		cfg, err := newSVCConfig()
-		if err != nil {
-			t.Errorf("cleanup: create service config: %v", err)
-			return
-		}
-		ctxSvc, cancel := context.WithCancel(context.Background())
-		defer cancel()
-		s, err := newSVC(newProgram(ctxSvc, cancel), cfg)
-		if err != nil {
-			t.Errorf("cleanup: create service: %v", err)
-			return
-		}
-
-		// If the subtests already cleaned up, there's nothing to do.
-		if _, err := s.Status(); err != nil {
-			return
-		}
-
-		if err := s.Stop(); err != nil {
-			t.Errorf("cleanup: stop service: %v", err)
-		}
-		if err := s.Uninstall(); err != nil {
-			t.Errorf("cleanup: uninstall service: %v", err)
-		}
-	})
-
 	ctx := context.Background()
 
 	t.Run("Install", func(t *testing.T) {

client/internal/auth/auth.go

@@ -221,7 +221,6 @@ func (a *Auth) getPKCEFlow(client *mgm.GrpcClient) (*PKCEAuthorizationFlow, erro
 	config := &PKCEAuthProviderConfig{
 		Audience:              protoConfig.GetAudience(),
 		ClientID:              protoConfig.GetClientID(),
-		ClientSecret:          protoConfig.GetClientSecret(), //nolint:staticcheck
 		TokenEndpoint:         protoConfig.GetTokenEndpoint(),
 		AuthorizationEndpoint: protoConfig.GetAuthorizationEndpoint(),
 		Scope:                 protoConfig.GetScope(),
@@ -266,7 +265,6 @@ func (a *Auth) getDeviceFlow(client *mgm.GrpcClient) (*DeviceAuthorizationFlow,
 	config := &DeviceAuthProviderConfig{
 		Audience:           protoConfig.GetAudience(),
 		ClientID:           protoConfig.GetClientID(),
-		ClientSecret:       protoConfig.GetClientSecret(), //nolint:staticcheck
 		Domain:             protoConfig.Domain,
 		TokenEndpoint:      protoConfig.GetTokenEndpoint(),
 		DeviceAuthEndpoint: protoConfig.GetDeviceAuthEndpoint(),

client/internal/auth/device_flow.go

@@ -29,8 +29,6 @@ var _ OAuthFlow = &DeviceAuthorizationFlow{}
 type DeviceAuthProviderConfig struct {
 	// ClientID An IDP application client id
 	ClientID string
-	// ClientSecret An IDP application client secret
-	ClientSecret string
 	// Domain An IDP API domain
 	// Deprecated. Use OIDCConfigEndpoint instead
 	Domain string

client/internal/auth/pkce_flow.go

@@ -38,8 +38,6 @@ const (
 type PKCEAuthProviderConfig struct {
 	// ClientID An IDP application client id
 	ClientID string
-	// ClientSecret An IDP application client secret
-	ClientSecret string
 	// Audience An Audience for to authorization validation
 	Audience string
 	// TokenEndpoint is the endpoint of an IDP manager where clients can obtain access token
@@ -111,8 +109,7 @@ func NewPKCEAuthorizationFlow(config PKCEAuthProviderConfig) (*PKCEAuthorization
 	}
 
 	cfg := &oauth2.Config{
-		ClientID:     config.ClientID,
-		ClientSecret: config.ClientSecret,
+		ClientID: config.ClientID,
 		Endpoint: oauth2.Endpoint{
 			AuthURL:  config.AuthorizationEndpoint,
 			TokenURL: config.TokenEndpoint,

client/internal/engine.go

@@ -500,7 +500,7 @@ func (e *Engine) Start(netbirdConfig *mgmProto.NetbirdConfig, mgmtURL *url.URL)
 	e.routeManager.SetRouteChangeListener(e.mobileDep.NetworkChangeListener)
 
 	e.dnsServer.SetRouteChecker(func(ip netip.Addr) bool {
-		for _, routes := range e.routeManager.GetSelectedClientRoutes() {
+		for _, routes := range e.routeManager.GetClientRoutes() {
 			for _, r := range routes {
 				if r.Network.Contains(ip) {
 					return true

client/internal/routemanager/manager.go

@@ -52,7 +52,6 @@ type Manager interface {
 	TriggerSelection(route.HAMap)
 	GetRouteSelector() *routeselector.RouteSelector
 	GetClientRoutes() route.HAMap
-	GetSelectedClientRoutes() route.HAMap
 	GetClientRoutesWithNetID() map[route.NetID][]*route.Route
 	SetRouteChangeListener(listener listener.NetworkChangeListener)
 	InitialRouteRange() []string
@@ -466,16 +465,6 @@ func (m *DefaultManager) GetClientRoutes() route.HAMap {
 	return maps.Clone(m.clientRoutes)
 }
 
-// GetSelectedClientRoutes returns only the currently selected/active client routes,
-// filtering out deselected exit nodes. Use this instead of GetClientRoutes when checking
-// if traffic should be routed through the tunnel.
-func (m *DefaultManager) GetSelectedClientRoutes() route.HAMap {
-	m.mux.Lock()
-	defer m.mux.Unlock()
-
-	return m.routeSelector.FilterSelectedExitNodes(maps.Clone(m.clientRoutes))
-}
-
 // GetClientRoutesWithNetID returns the current routes from the route map, but the keys consist of the network ID only
 func (m *DefaultManager) GetClientRoutesWithNetID() map[route.NetID][]*route.Route {
 	m.mux.Lock()

client/internal/routemanager/mock.go

@@ -18,7 +18,6 @@ type MockManager struct {
 	TriggerSelectionFunc         func(haMap route.HAMap)
 	GetRouteSelectorFunc         func() *routeselector.RouteSelector
 	GetClientRoutesFunc          func() route.HAMap
-	GetSelectedClientRoutesFunc  func() route.HAMap
 	GetClientRoutesWithNetIDFunc func() map[route.NetID][]*route.Route
 	StopFunc                     func(manager *statemanager.Manager)
 }
@@ -62,7 +61,7 @@ func (m *MockManager) GetRouteSelector() *routeselector.RouteSelector {
 	return nil
 }
 
-// GetClientRoutes mock implementation of GetClientRoutes from the Manager interface
+// GetClientRoutes mock implementation of GetClientRoutes from Manager interface
 func (m *MockManager) GetClientRoutes() route.HAMap {
 	if m.GetClientRoutesFunc != nil {
 		return m.GetClientRoutesFunc()
@@ -70,14 +69,6 @@ func (m *MockManager) GetClientRoutes() route.HAMap {
 	return nil
 }
 
-// GetSelectedClientRoutes mock implementation of GetSelectedClientRoutes from the Manager interface
-func (m *MockManager) GetSelectedClientRoutes() route.HAMap {
-	if m.GetSelectedClientRoutesFunc != nil {
-		return m.GetSelectedClientRoutesFunc()
-	}
-	return nil
-}
-
 // GetClientRoutesWithNetID mock implementation of GetClientRoutesWithNetID from Manager interface
 func (m *MockManager) GetClientRoutesWithNetID() map[route.NetID][]*route.Route {
 	if m.GetClientRoutesWithNetIDFunc != nil {

client/ui/debug.go

@@ -371,46 +371,49 @@ func (s *serviceClient) configureServiceForDebug(
 	conn proto.DaemonServiceClient,
 	state *debugInitialState,
 	enablePersistence bool,
-) error {
+) {
 	if state.wasDown {
 		if _, err := conn.Up(s.ctx, &proto.UpRequest{}); err != nil {
-			return fmt.Errorf("bring service up: %v", err)
+			log.Warnf("failed to bring service up: %v", err)
+		} else {
+			log.Info("Service brought up for debug")
+			time.Sleep(time.Second * 10)
 		}
-		log.Info("Service brought up for debug")
-		time.Sleep(time.Second * 10)
 	}
 
 	if !state.isLevelTrace {
 		if _, err := conn.SetLogLevel(s.ctx, &proto.SetLogLevelRequest{Level: proto.LogLevel_TRACE}); err != nil {
-			return fmt.Errorf("set log level to TRACE: %v", err)
+			log.Warnf("failed to set log level to TRACE: %v", err)
+		} else {
+			log.Info("Log level set to TRACE for debug")
 		}
-		log.Info("Log level set to TRACE for debug")
 	}
 
 	if _, err := conn.Down(s.ctx, &proto.DownRequest{}); err != nil {
-		return fmt.Errorf("bring service down: %v", err)
+		log.Warnf("failed to bring service down: %v", err)
+	} else {
+		time.Sleep(time.Second)
 	}
-	time.Sleep(time.Second)
 
 	if enablePersistence {
 		if _, err := conn.SetSyncResponsePersistence(s.ctx, &proto.SetSyncResponsePersistenceRequest{
 			Enabled: true,
 		}); err != nil {
-			return fmt.Errorf("enable sync response persistence: %v", err)
+			log.Warnf("failed to enable sync response persistence: %v", err)
+		} else {
+			log.Info("Sync response persistence enabled for debug")
 		}
-		log.Info("Sync response persistence enabled for debug")
 	}
 
 	if _, err := conn.Up(s.ctx, &proto.UpRequest{}); err != nil {
-		return fmt.Errorf("bring service back up: %v", err)
+		log.Warnf("failed to bring service back up: %v", err)
+	} else {
+		time.Sleep(time.Second * 3)
 	}
-	time.Sleep(time.Second * 3)
 
 	if _, err := conn.StartCPUProfile(s.ctx, &proto.StartCPUProfileRequest{}); err != nil {
 		log.Warnf("failed to start CPU profiling: %v", err)
 	}
-
-	return nil
 }
 
 func (s *serviceClient) collectDebugData(
@@ -424,9 +427,7 @@ func (s *serviceClient) collectDebugData(
 	var wg sync.WaitGroup
 	startProgressTracker(ctx, &wg, params.duration, progress)
 
-	if err := s.configureServiceForDebug(conn, state, params.enablePersistence); err != nil {
-		return err
-	}
+	s.configureServiceForDebug(conn, state, params.enablePersistence)
 
 	wg.Wait()
 	progress.progressBar.Hide()
@@ -484,7 +485,7 @@ func (s *serviceClient) createDebugBundleFromCollection(
 func (s *serviceClient) restoreServiceState(conn proto.DaemonServiceClient, state *debugInitialState) {
 	if state.wasDown {
 		if _, err := conn.Down(s.ctx, &proto.DownRequest{}); err != nil {
-			log.Errorf("Failed to restore down state: %v", err)
+			log.Warnf("failed to restore down state: %v", err)
 		} else {
 			log.Info("Service state restored to down")
 		}
@@ -492,7 +493,7 @@ func (s *serviceClient) restoreServiceState(conn proto.DaemonServiceClient, stat
 
 	if !state.isLevelTrace {
 		if _, err := conn.SetLogLevel(s.ctx, &proto.SetLogLevelRequest{Level: state.logLevel}); err != nil {
-			log.Errorf("Failed to restore log level: %v", err)
+			log.Warnf("failed to restore log level: %v", err)
 		} else {
 			log.Info("Log level restored to original setting")
 		}

management/internals/modules/reverseproxy/service/manager/manager.go

@@ -288,8 +288,6 @@ func (m *Manager) validateSubdomainRequirement(ctx context.Context, domain, clus
 }
 
 func (m *Manager) persistNewService(ctx context.Context, accountID string, svc *service.Service) error {
-	customPorts := m.clusterCustomPorts(ctx, svc)
-
 	return m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error {
 		if svc.Domain != "" {
 			if err := m.checkDomainAvailable(ctx, transaction, svc.Domain, ""); err != nil {
@@ -297,7 +295,7 @@ func (m *Manager) persistNewService(ctx context.Context, accountID string, svc *
 			}
 		}
 
-		if err := m.ensureL4Port(ctx, transaction, svc, customPorts); err != nil {
+		if err := m.ensureL4Port(ctx, transaction, svc); err != nil {
 			return err
 		}
 
@@ -317,23 +315,12 @@ func (m *Manager) persistNewService(ctx context.Context, accountID string, svc *
 	})
 }
 
-// clusterCustomPorts queries whether the cluster supports custom ports.
-// Must be called before entering a transaction: the underlying query uses
-// the main DB handle, which deadlocks when called inside a transaction
-// that already holds the connection.
-func (m *Manager) clusterCustomPorts(ctx context.Context, svc *service.Service) *bool {
-	if !service.IsL4Protocol(svc.Mode) {
-		return nil
-	}
-	return m.capabilities.ClusterSupportsCustomPorts(ctx, svc.ProxyCluster)
-}
-
 // ensureL4Port auto-assigns a listen port when needed and validates cluster support.
-// customPorts must be pre-computed via clusterCustomPorts before entering a transaction.
-func (m *Manager) ensureL4Port(ctx context.Context, tx store.Store, svc *service.Service, customPorts *bool) error {
+func (m *Manager) ensureL4Port(ctx context.Context, tx store.Store, svc *service.Service) error {
 	if !service.IsL4Protocol(svc.Mode) {
 		return nil
 	}
+	customPorts := m.capabilities.ClusterSupportsCustomPorts(ctx, svc.ProxyCluster)
 	if service.IsPortBasedProtocol(svc.Mode) && svc.ListenPort > 0 && (customPorts == nil || !*customPorts) {
 		if svc.Source != service.SourceEphemeral {
 			return status.Errorf(status.InvalidArgument, "custom ports not supported on cluster %s", svc.ProxyCluster)
@@ -417,14 +404,12 @@ func (m *Manager) assignPort(ctx context.Context, tx store.Store, cluster string
 // The count and exists queries use FOR UPDATE locking to serialize concurrent creates
 // for the same peer, preventing the per-peer limit from being bypassed.
 func (m *Manager) persistNewEphemeralService(ctx context.Context, accountID, peerID string, svc *service.Service) error {
-	customPorts := m.clusterCustomPorts(ctx, svc)
-
 	return m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error {
 		if err := m.validateEphemeralPreconditions(ctx, transaction, accountID, peerID, svc); err != nil {
 			return err
 		}
 
-		if err := m.ensureL4Port(ctx, transaction, svc, customPorts); err != nil {
+		if err := m.ensureL4Port(ctx, transaction, svc); err != nil {
 			return err
 		}
 
@@ -527,49 +512,16 @@ type serviceUpdateInfo struct {
 }
 
 func (m *Manager) persistServiceUpdate(ctx context.Context, accountID string, service *service.Service) (*serviceUpdateInfo, error) {
-	effectiveCluster, err := m.resolveEffectiveCluster(ctx, accountID, service)
-	if err != nil {
-		return nil, err
-	}
-
-	svcForCaps := *service
-	svcForCaps.ProxyCluster = effectiveCluster
-	customPorts := m.clusterCustomPorts(ctx, &svcForCaps)
-
 	var updateInfo serviceUpdateInfo
 
-	err = m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error {
-		return m.executeServiceUpdate(ctx, transaction, accountID, service, &updateInfo, customPorts)
+	err := m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error {
+		return m.executeServiceUpdate(ctx, transaction, accountID, service, &updateInfo)
 	})
 
 	return &updateInfo, err
 }
 
-// resolveEffectiveCluster determines the cluster that will be used after the update.
-// It reads the existing service without locking and derives the new cluster if the domain changed.
-func (m *Manager) resolveEffectiveCluster(ctx context.Context, accountID string, svc *service.Service) (string, error) {
-	existing, err := m.store.GetServiceByID(ctx, store.LockingStrengthNone, accountID, svc.ID)
-	if err != nil {
-		return "", err
-	}
-
-	if existing.Domain == svc.Domain {
-		return existing.ProxyCluster, nil
-	}
-
-	if m.clusterDeriver != nil {
-		derived, err := m.clusterDeriver.DeriveClusterFromDomain(ctx, accountID, svc.Domain)
-		if err != nil {
-			log.WithError(err).Warnf("could not derive cluster from domain %s", svc.Domain)
-		} else {
-			return derived, nil
-		}
-	}
-
-	return existing.ProxyCluster, nil
-}
-
-func (m *Manager) executeServiceUpdate(ctx context.Context, transaction store.Store, accountID string, service *service.Service, updateInfo *serviceUpdateInfo, customPorts *bool) error {
+func (m *Manager) executeServiceUpdate(ctx context.Context, transaction store.Store, accountID string, service *service.Service, updateInfo *serviceUpdateInfo) error {
 	existingService, err := transaction.GetServiceByID(ctx, store.LockingStrengthUpdate, accountID, service.ID)
 	if err != nil {
 		return err
@@ -606,7 +558,7 @@ func (m *Manager) executeServiceUpdate(ctx context.Context, transaction store.St
 	m.preserveListenPort(service, existingService)
 	updateInfo.serviceEnabledChanged = existingService.Enabled != service.Enabled
 
-	if err := m.ensureL4Port(ctx, transaction, service, customPorts); err != nil {
+	if err := m.ensureL4Port(ctx, transaction, service); err != nil {
 		return err
 	}
 	if err := m.checkPortConflict(ctx, transaction, service); err != nil {

management/internals/modules/reverseproxy/service/service.go

@@ -787,11 +787,6 @@ func (s *Service) validateHTTPTargets() error {
 }
 
 func (s *Service) validateL4Target(target *Target) error {
-	// L4 services have a single target; per-target disable is meaningless
-	// (use the service-level Enabled flag instead). Force it on so that
-	// buildPathMappings always includes the target in the proto.
-	target.Enabled = true
-
 	if target.Port == 0 {
 		return errors.New("target port is required for L4 services")
 	}

management/internals/shared/grpc/server.go

@@ -966,7 +966,6 @@ func (s *Server) GetDeviceAuthorizationFlow(ctx context.Context, req *proto.Encr
 			Provider: proto.DeviceAuthorizationFlowProvider(provider),
 			ProviderConfig: &proto.ProviderConfig{
 				ClientID:           s.config.DeviceAuthorizationFlow.ProviderConfig.ClientID,
-				ClientSecret:       s.config.DeviceAuthorizationFlow.ProviderConfig.ClientSecret,
 				Domain:             s.config.DeviceAuthorizationFlow.ProviderConfig.Domain,
 				Audience:           s.config.DeviceAuthorizationFlow.ProviderConfig.Audience,
 				DeviceAuthEndpoint: s.config.DeviceAuthorizationFlow.ProviderConfig.DeviceAuthEndpoint,
@@ -1037,7 +1036,6 @@ func (s *Server) GetPKCEAuthorizationFlow(ctx context.Context, req *proto.Encryp
 			ProviderConfig: &proto.ProviderConfig{
 				Audience:              s.config.PKCEAuthorizationFlow.ProviderConfig.Audience,
 				ClientID:              s.config.PKCEAuthorizationFlow.ProviderConfig.ClientID,
-				ClientSecret:          s.config.PKCEAuthorizationFlow.ProviderConfig.ClientSecret,
 				TokenEndpoint:         s.config.PKCEAuthorizationFlow.ProviderConfig.TokenEndpoint,
 				AuthorizationEndpoint: s.config.PKCEAuthorizationFlow.ProviderConfig.AuthorizationEndpoint,
 				Scope:                 s.config.PKCEAuthorizationFlow.ProviderConfig.Scope,

shared/management/client/client_test.go

@@ -545,8 +545,7 @@ func Test_GetPKCEAuthorizationFlow(t *testing.T) {
 
 	expectedFlowInfo := &mgmtProto.PKCEAuthorizationFlow{
 		ProviderConfig: &mgmtProto.ProviderConfig{
-			ClientID:     "client",
-			ClientSecret: "secret",
+			ClientID: "client",
 		},
 	}
 
@@ -569,5 +568,4 @@ func Test_GetPKCEAuthorizationFlow(t *testing.T) {
 	}
 
 	assert.Equal(t, expectedFlowInfo.ProviderConfig.ClientID, flowInfo.ProviderConfig.ClientID, "provider configured client ID should match")
-	assert.Equal(t, expectedFlowInfo.ProviderConfig.ClientSecret, flowInfo.ProviderConfig.ClientSecret, "provider configured client secret should match") //nolint:staticcheck
 }