sensors: improve sensor manager testing

Currently, the observer uses a global variable for the sensor manager.
Tests may use the observer or create their own sensor manager. For the
latter case, a StartTestSensorManager was created. The idea behind the
test functions is that they make it easier to write tests by failing
when an error happens and registering cleanup functions when the test
finishes.

Some tests, however, require having an observer and others do not. This
patch:
 - Makes the sensor manager global variable in the observer a private
   one, and introduces access functions for it
   - This  allows us to check whether the global variable is set twice.
 - Changes the StartTestSensorManager to GetTestSensorManager that first
   checks if the global variable is set. If it is, it just returns the
   existing sensor manager.

Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>

cmd/tetragon/main.go

@@ -340,7 +340,7 @@ func tetragonExecute() error {
 	pm, err := tetragonGrpc.NewProcessManager(
 		ctx,
 		&cleanupWg,
-		observer.SensorManager,
+		observer.GetSensorManager(),
 		hookRunner)
 	if err != nil {
 		return err
@@ -358,7 +358,7 @@ func tetragonExecute() error {
 	obs.AddListener(pm)
 	saveInitInfo()
 	if option.Config.EnableK8s {
-		go crd.WatchTracePolicy(ctx, observer.SensorManager)
+		go crd.WatchTracePolicy(ctx, observer.GetSensorManager())
 	}
 
 	obs.LogPinnedBpf(observerDir)
@@ -375,7 +375,7 @@ func tetragonExecute() error {
 	// now that the base sensor was loaded, we can start the sensor manager
 	close(sensorMgWait)
 	sensorMgWait = nil
-	observer.SensorManager.LogSensorsAndProbes(ctx)
+	observer.GetSensorManager().LogSensorsAndProbes(ctx)
 
 	err = loadTpFromDir(ctx, option.Config.TracingPolicyDir)
 	if err != nil {
@@ -450,7 +450,7 @@ func addTracingPolicy(ctx context.Context, file string) error {
 		return err
 	}
 
-	err = observer.SensorManager.AddTracingPolicy(ctx, tp)
+	err = observer.GetSensorManager().AddTracingPolicy(ctx, tp)
 	if err != nil {
 		return err
 	}

pkg/bench/bench.go

@@ -227,7 +227,7 @@ func startBenchmarkExporter(ctx context.Context, obs *observer.Observer, summary
 	processManager, err := grpc.NewProcessManager(
 		ctx,
 		&wg,
-		observer.SensorManager,
+		observer.GetSensorManager(),
 		hookRunner)
 	if err != nil {
 		return err

pkg/observer/observer.go

@@ -40,9 +40,6 @@ var (
 	eventHandler = make(map[uint8]func(r *bytes.Reader) ([]Event, error))
 
 	observerList []*Observer
-
-	/* SensorManager handles dynamic sensors loading / unloading. */
-	SensorManager *sensors.Manager
 )
 
 type Event notify.Message
@@ -344,9 +341,11 @@ func (k *Observer) Start(ctx context.Context) error {
 
 // InitSensorManager starts the sensor controller
 func (k *Observer) InitSensorManager(waitChan chan struct{}) error {
-	var err error
-	SensorManager, err = sensors.StartSensorManager(option.Config.BpfDir, option.Config.MapDir, waitChan)
-	return err
+	mgr, err := sensors.StartSensorManager(option.Config.BpfDir, option.Config.MapDir, waitChan)
+	if err != nil {
+		return err
+	}
+	return SetSensorManager(mgr)
 }
 
 func NewObserver(configFile string) *Observer {
@@ -404,7 +403,9 @@ func (k *Observer) RemovePrograms() {
 }
 
 func RemoveSensors(ctx context.Context) {
-	SensorManager.RemoveAllSensors(ctx)
+	if mgr := GetSensorManager(); mgr != nil {
+		mgr.RemoveAllSensors(ctx)
+	}
 }
 
 // Log Active pinned BPF resources

pkg/observer/observertesthelper/observer_test_helper.go

@@ -369,9 +369,10 @@ func loadExporter(tb testing.TB, ctx context.Context, obs *observer.Observer, op
 
 	// NB(kkourt): we use the global that was set up by InitSensorManager(). We should clean
 	// this up and remove/hide the global variable.
-	sensorManager := observer.SensorManager
+	sensorManager := observer.GetSensorManager()
 	tb.Cleanup(func() {
 		sensorManager.StopSensorManager(ctx)
+		observer.ResetSensorManager()
 	})
 
 	if oo.crd {
@@ -436,7 +437,7 @@ func loadObserver(tb testing.TB, ctx context.Context, base *sensors.Sensor,
 	}
 
 	if tp != nil {
-		if err := observer.SensorManager.AddTracingPolicy(ctx, tp); err != nil {
+		if err := observer.GetSensorManager().AddTracingPolicy(ctx, tp); err != nil {
 			tb.Fatalf("SensorManager.AddTracingPolicy error: %s\n", err)
 		}
 	}

pkg/observer/sensor_manager.go

@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Authors of Tetragon
+
+package observer
+
+import (
+	"fmt"
+	"sync"
+
+	"github.com/cilium/tetragon/pkg/sensors"
+)
+
+var (
+	// SensorManager handles dynamic sensors loading / unloading, and is a global variable for
+	// now
+	sensorManager   *sensors.Manager
+	sensorManagerMu sync.Mutex
+)
+
+// ResetSensorManager resets the global sensorManager variable to nil. Intended only for testing.
+func ResetSensorManager() {
+	sensorManager = nil
+}
+
+func SetSensorManager(sm *sensors.Manager) error {
+	sensorManagerMu.Lock()
+	defer sensorManagerMu.Unlock()
+
+	if sensorManager != nil {
+		return fmt.Errorf("observer sensorManager already set")
+	}
+	sensorManager = sm
+	return nil
+}
+
+func GetSensorManager() *sensors.Manager {
+	sensorManagerMu.Lock()
+	defer sensorManagerMu.Unlock()
+	return sensorManager
+}

pkg/sensors/exec/cgroups_test.go

@@ -589,9 +589,7 @@ func setupTgRuntimeConf(t *testing.T, trackingCgrpLevel, logLevel, hierarchyId,
 }
 
 func setupObserver(ctx context.Context, t *testing.T) *tus.TestSensorManager {
-	testManager := tus.StartTestSensorManager(ctx, t)
-	observer.SensorManager = testManager.Manager
-
+	testManager := tus.GetTestSensorManager(ctx, t)
 	if err := observer.InitDataCache(1024); err != nil {
 		t.Fatalf("failed to call observer.InitDataCache %s", err)
 	}

pkg/sensors/test/lseek_test.go

@@ -11,7 +11,6 @@ import (
 
 	ec "github.com/cilium/tetragon/api/v1/tetragon/codegen/eventchecker"
 	"github.com/cilium/tetragon/pkg/jsonchecker"
-	"github.com/cilium/tetragon/pkg/observer"
 	"github.com/cilium/tetragon/pkg/observer/observertesthelper"
 	_ "github.com/cilium/tetragon/pkg/sensors/exec"
 	tus "github.com/cilium/tetragon/pkg/testutils/sensors"
@@ -77,8 +76,7 @@ func TestSensorLseekEnable(t *testing.T) {
 
 	sensor := GetTestSensor()
 
-	smanager := tus.StartTestSensorManager(ctx, t)
-	observer.SensorManager = smanager.Manager
+	smanager := tus.GetTestSensorManager(ctx, t)
 	smanager.AddAndEnableSensor(ctx, t, sensor, sensor.Name)
 
 	observertesthelper.LoopEvents(ctx, t, &doneWG, &readyWG, obs)

pkg/sensors/tracing/policyfilter_test.go

@@ -115,7 +115,7 @@ func TestNamespacedPolicies(t *testing.T) {
 
 	tus.LoadSensor(t, base.GetInitialSensor())
 	tus.LoadSensor(t, testsensor.GetTestSensor())
-	sm := tus.StartTestSensorManager(ctx, t)
+	sm := tus.GetTestSensorManager(ctx, t)
 
 	// First, we create two lseek-pipe commands and add them to a different cgroup. See
 	// contrib/tester-progs/go/lseek-pipe for details of how lseek-pipe wowkrs, but basically it

pkg/sensors/tracing/tracepoint_test.go

@@ -75,7 +75,7 @@ func TestGenericTracepointSimple(t *testing.T) {
 		t.Fatalf("GetDefaultObserver error: %s", err)
 	}
 
-	sm := tus.StartTestSensorManager(ctx, t)
+	sm := tus.GetTestSensorManager(ctx, t)
 	// create and add sensor
 	sensor, err := createGenericTracepointSensor("GtpLseekTest", []GenericTracepointConf{lseekConf}, policyfilter.NoFilterID,
 		"policyName", []v1alpha1.ListSpec{})
@@ -135,7 +135,7 @@ func doTestGenericTracepointPidFilter(t *testing.T, conf GenericTracepointConf,
 		t.Fatalf("GetDefaultObserver error: %s", err)
 	}
 
-	sm := tus.StartTestSensorManager(ctx, t)
+	sm := tus.GetTestSensorManager(ctx, t)
 	// create and add sensor
 	sensor, err := createGenericTracepointSensor("GtpLseekTest", []GenericTracepointConf{conf}, policyfilter.NoFilterID,
 		"policyName", []v1alpha1.ListSpec{})
@@ -527,7 +527,7 @@ func TestTracepointCloneThreads(t *testing.T) {
 		t.Fatalf("GetDefaultObserver error: %s", err)
 	}
 
-	sm := tus.StartTestSensorManager(ctx, t)
+	sm := tus.GetTestSensorManager(ctx, t)
 	// create and add sensor
 	sensor, err := createGenericTracepointSensor("GtpLseekTest", []GenericTracepointConf{lseekConf}, policyfilter.NoFilterID,
 		"policyName", []v1alpha1.ListSpec{})

pkg/testutils/sensors/sensors.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/cilium/tetragon/pkg/bpf"
+	"github.com/cilium/tetragon/pkg/observer"
 	"github.com/cilium/tetragon/pkg/sensors"
 )
 
@@ -32,10 +33,19 @@ type TestSensorManager struct {
 	Manager *sensors.Manager
 }
 
-// StartTestSensorManager starts a new sensor mananger.
-// It will use the test name to create a unqique directory for maps/etc, and
-// will also register the necessary cleanup functions using t.Cleanup()
-func StartTestSensorManager(ctx context.Context, t *testing.T) *TestSensorManager {
+// GetTestSensorManager returns a new test sensor manager.
+// Some tests require an observer running, some do not. To support both, the function checks if a
+// sensor manager has already been setup in the observer, and uses it if so.
+// Otherwise, it creates a new one. If it creates a new one it will use the test name to create a
+// unqique directory for maps/etc, and will also register the necessary cleanup functions using
+// t.Cleanup()
+func GetTestSensorManager(ctx context.Context, t *testing.T) *TestSensorManager {
+	if mgr := observer.GetSensorManager(); mgr != nil {
+		return &TestSensorManager{
+			Manager: mgr,
+		}
+	}
+
 	path := bpf.MapPrefixPath()
 	mgr, err := sensors.StartSensorManager(path, path, nil)
 	if err != nil {