From 50c6a72815c81f11bf4e7da68b0e6a0f78325cd6 Mon Sep 17 00:00:00 2001 From: Kevin Sheldrake Date: Wed, 17 Jul 2024 16:49:55 +0100 Subject: [PATCH 1/5] Test: Change panic to t.Fatal Panicking in tests is bad as it fails to tell the test handler that the test failed. Instead we should use t.Fatal(). This commit changes instances of panic in tests to t.Fatal(). Signed-off-by: Kevin Sheldrake --- pkg/sensors/manager_test.go | 16 ++++++++-------- pkg/sensors/tracing/generickprobe_test.go | 2 +- pkg/sensors/tracing/kprobe_test.go | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/sensors/manager_test.go b/pkg/sensors/manager_test.go index ba1a63fde27..646716ba798 100644 --- a/pkg/sensors/manager_test.go +++ b/pkg/sensors/manager_test.go @@ -46,7 +46,7 @@ func TestAddPolicy(t *testing.T) { assert.NoError(t, err) t.Cleanup(func() { if err := mgr.StopSensorManager(ctx); err != nil { - panic("failed to stop sensor manager") + t.Fatal("failed to stop sensor manager") } }) policy.ObjectMeta.Name = "test-policy" @@ -74,7 +74,7 @@ func TestAddPolicies(t *testing.T) { assert.NoError(t, err) t.Cleanup(func() { if err := mgr.StopSensorManager(ctx); err != nil { - panic("failed to stop sensor manager") + t.Fatal("failed to stop sensor manager") } }) policy.ObjectMeta.Name = "test-policy" @@ -105,7 +105,7 @@ func TestAddPolicySpecError(t *testing.T) { assert.NoError(t, err) t.Cleanup(func() { if err := mgr.StopSensorManager(ctx); err != nil { - panic("failed to stop sensor manager") + t.Fatal("failed to stop sensor manager") } }) policy.ObjectMeta.Name = "test-policy" @@ -137,7 +137,7 @@ func TestAddPolicyLoadError(t *testing.T) { assert.NoError(t, err) t.Cleanup(func() { if err := mgr.StopSensorManager(ctx); err != nil { - panic("failed to stop sensor manager") + t.Fatal("failed to stop sensor manager") } }) policy.ObjectMeta.Name = "test-policy" @@ -214,7 +214,7 @@ func TestPolicyStates(t *testing.T) { require.NoError(t, err) t.Cleanup(func() { if err := mgr.StopSensorManager(ctx); err != nil { - panic("failed to stop sensor manager") + t.Fatal("failed to stop sensor manager") } }) policy.ObjectMeta.Name = "test-policy" @@ -239,7 +239,7 @@ func TestPolicyStates(t *testing.T) { require.NoError(t, err) t.Cleanup(func() { if err := mgr.StopSensorManager(ctx); err != nil { - panic("failed to stop sensor manager") + t.Fatal("failed to stop sensor manager") } }) policy.ObjectMeta.Name = "test-policy" @@ -279,7 +279,7 @@ func TestPolicyLoadErrorOverride(t *testing.T) { require.NoError(t, err) t.Cleanup(func() { if err := mgr.StopSensorManager(ctx); err != nil { - panic("failed to stop sensor manager") + t.Fatal("failed to stop sensor manager") } }) policy.ObjectMeta.Name = "test-policy" @@ -318,7 +318,7 @@ func TestPolicyListingWhileLoadUnload(t *testing.T) { require.NoError(t, err) t.Cleanup(func() { if err := mgr.StopSensorManager(ctx); err != nil { - panic("failed to stop sensor manager") + t.Fatal("failed to stop sensor manager") } }) diff --git a/pkg/sensors/tracing/generickprobe_test.go b/pkg/sensors/tracing/generickprobe_test.go index 2da2b610f1f..f65a302186b 100644 --- a/pkg/sensors/tracing/generickprobe_test.go +++ b/pkg/sensors/tracing/generickprobe_test.go @@ -127,7 +127,7 @@ func Test_Kprobe_DisableEnablePolicy(t *testing.T) { assert.NoError(t, err) t.Cleanup(func() { if err := mgr.StopSensorManager(ctx); err != nil { - panic("failed to stop sensor manager") + t.Fatal("failed to stop sensor manager") } }) diff --git a/pkg/sensors/tracing/kprobe_test.go b/pkg/sensors/tracing/kprobe_test.go index 3a0c007cbf3..587134133c1 100644 --- a/pkg/sensors/tracing/kprobe_test.go +++ b/pkg/sensors/tracing/kprobe_test.go @@ -5845,14 +5845,14 @@ spec: socket, err := net.Dial("udp", "127.0.0.1:9468") if err != nil { fmt.Printf("ERROR dialing socket\n") - panic(err) + t.Fatal(err) } for i := 0; i < 5; i++ { _, err := socket.Write([]byte("data")) if err != nil { fmt.Printf("ERROR writing to socket\n") - panic(err) + t.Fatal(err) } } From e26bdea631d0b55ffa9df95504b58fbdb6f94a1a Mon Sep 17 00:00:00 2001 From: Mahe Tardy Date: Wed, 17 Jul 2024 19:49:59 +0200 Subject: [PATCH 2/5] vmtests: bump base image used to root-images:20240717.161638 For the base image to have nc.openbsd, see cilium/little-vm-helper-images@2af406c7fe7a50cb8b00b2dbbc2344708365ceac. Signed-off-by: Mahe Tardy --- tests/vmtests/fetch-data.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/vmtests/fetch-data.sh b/tests/vmtests/fetch-data.sh index 6a9ae147d4b..0232784a25a 100755 --- a/tests/vmtests/fetch-data.sh +++ b/tests/vmtests/fetch-data.sh @@ -4,7 +4,7 @@ set -eu -o pipefail OCIORG=quay.io/lvh-images -ROOTIMG=$OCIORG/root-images:20240415.162748@sha256:2637beacabbb48e2ee89a8f296a123142257ae10616308f81e7210ac85b92789 +ROOTIMG=$OCIORG/root-images:20240717.161638@sha256:62a9890111ab39749792fda4f59c8f736fa350ecaedb0667e3eecbbe790d82ed KERNIMG=$OCIORG/kernel-images CONTAINER_ENGINE=${CONTAINER_ENGINE:-docker} KERNEL_VERS="$@" From 625bc6930f6daf2a58db44993209b9d88d1e4de3 Mon Sep 17 00:00:00 2001 From: Mahe Tardy Date: Thu, 18 Jul 2024 12:49:24 +0200 Subject: [PATCH 3/5] pkg/sensors/tracing: fix enforcer security tests Because of root-images update in previous commit, the enforcer security tests started failing, after investigation we realized that the difference was that the old image mounted /tmp on the disk fs and the new one with tmpfs. Then the direct-write-tester.c program was failing using O_DIRECT (because apparently it fails on tmpfs). It failed silently on recent linux versions however because O_DIRECT on tmpfs might just be a noop. /var/tmp should be backed by disk so it should be a fix for our issue. Signed-off-by: Mahe Tardy --- pkg/sensors/tracing/enforcer_test.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/pkg/sensors/tracing/enforcer_test.go b/pkg/sensors/tracing/enforcer_test.go index 58002e3466d..bc16ad8bdd2 100644 --- a/pkg/sensors/tracing/enforcer_test.go +++ b/pkg/sensors/tracing/enforcer_test.go @@ -296,6 +296,22 @@ func testSecurity(t *testing.T, tracingPolicy, tempFile string) { } } +func enforcerSecurityTempFile(t *testing.T) string { + // We can't use t.TempDir as it writes into /tmp by default. + // The direct-write-tester.c program opens and writes using the O_DIRECT + // flag that is unsupported and return EINVAL on tmpfs, while it works on a + // disk based fs. Recently, the base image used by vmtests started to switch + // /tmp from the disk to tmpfs which made that test fail. + tempFile, err := os.CreateTemp("/var/tmp", "tetragon-testfile-*") + if err != nil { + t.Fatalf("failed to create temporary file for tester prog: %s", err) + } + t.Cleanup(func() { + os.Remove(tempFile.Name()) + }) + return tempFile.Name() +} + // Testing the ability to kill the process before it executes the syscall, // in this case direct pwrite syscall. // Standard Sigkill action kills executed from sys_pwrite probe kills the @@ -325,7 +341,7 @@ func TestEnforcerSecuritySigKill(t *testing.T) { t.Skip("Older kernels do not support matchArgs for more than one arguments") } - tempFile := t.TempDir() + "/test" + tempFile := enforcerSecurityTempFile(t) tracingPolicy := ` apiVersion: cilium.io/v1alpha1 @@ -412,7 +428,7 @@ func TestEnforcerSecurityNotifyEnforcer(t *testing.T) { t.Skip("Older kernels do not support matchArgs for more than one arguments") } - tempFile := t.TempDir() + "/test" + tempFile := enforcerSecurityTempFile(t) tracingPolicy := ` apiVersion: cilium.io/v1alpha1 From f7f227e7dbd9ffdf4d2253ab238cb8ecce3ba6f7 Mon Sep 17 00:00:00 2001 From: Mahe Tardy Date: Thu, 18 Jul 2024 13:32:34 +0200 Subject: [PATCH 4/5] tester-progs: do not define O_DIRECT manually O_DIRECT is not posix so we need to add _GNU_SOURCE Co-authored-by: Jiri Olsa Signed-off-by: Mahe Tardy --- contrib/tester-progs/direct-write-tester.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/tester-progs/direct-write-tester.c b/contrib/tester-progs/direct-write-tester.c index 5a2cf50d034..7b8c5388e97 100644 --- a/contrib/tester-progs/direct-write-tester.c +++ b/contrib/tester-progs/direct-write-tester.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) // Copyright Authors of Tetragon +#define _GNU_SOURCE #include #include #include @@ -10,7 +11,6 @@ #include #define BLOCKSIZE 4096 -#define O_DIRECT 00040000 int main(int argc, char **argv) { From fe232eec240a72c622d0f104231b86b4f32bd6a5 Mon Sep 17 00:00:00 2001 From: Mahe Tardy Date: Thu, 18 Jul 2024 17:33:05 +0200 Subject: [PATCH 5/5] pkg/sensors: rate limit test, avoid calling printf Use the output returned by Fatal instead of passing stuff to printf Signed-off-by: Mahe Tardy --- pkg/sensors/tracing/kprobe_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/sensors/tracing/kprobe_test.go b/pkg/sensors/tracing/kprobe_test.go index 587134133c1..5ecc0d2902b 100644 --- a/pkg/sensors/tracing/kprobe_test.go +++ b/pkg/sensors/tracing/kprobe_test.go @@ -5844,15 +5844,13 @@ spec: // Generate 5 datagrams socket, err := net.Dial("udp", "127.0.0.1:9468") if err != nil { - fmt.Printf("ERROR dialing socket\n") - t.Fatal(err) + t.Fatalf("failed dialing socket: %s", err) } for i := 0; i < 5; i++ { _, err := socket.Write([]byte("data")) if err != nil { - fmt.Printf("ERROR writing to socket\n") - t.Fatal(err) + t.Fatalf("failed writing to socket: %s", err) } }