validation: improve error message when failing to load a TracingPolicy #2031

christian-2 · 2024-01-27T19:50:53Z

Callers of tracingpolicy.FromFile() no longer receive highly specific errors, but a generic error, which can also be presented to end users. Callers of tracingpolicy.FromJSON() still receive the specific errors, and any generic error wraps a specific one.

The implementation is along the lines suggested by @mtardy in #2022. (Thanks for that input.) Please indicate if my use of Panicf() in pkg/tracingpolicy/generictracingpolicy_test.go is unwelcome.

BTW, it seems to me that drop-privileges should be added to contrib/tester-progs/.gitignore. I could create another PR for that, if applicable (plus an issue, if necessary for the trivial change).

Fixes #2022

Signed-off-by: Christian Hörtnagl christian2@univie.ac.at

tixxdz · 2024-01-29T09:36:21Z

pkg/tracingpolicy/generictracingpolicy_test.go

+func createTempFile(data string) string {
+	file, err := ioutil.TempFile("", "*")
+	if err != nil {
+		logger.GetLogger().Panic("cannot create temporary file")


Here just return an error, then in the caller do t.Fatalf() in case

yeah don't use the logger and panic in tests, if you have t *testing.T available (see my other remark) just call t.Fatal if you want to stop this test here because of the error.

tixxdz

So lgtm, just a minor fix in the test so we fatal in the test getting the context.

Also for that .gitignore yes another PR is welcome ;-)

Thank you!

tixxdz · 2024-01-29T09:38:56Z

Some CI code format are failing too. Thank you

mtardy · 2024-01-29T10:52:46Z

BTW, it seems to me that drop-privileges should be added to contrib/tester-progs/.gitignore. I could create another PR for that, if applicable (plus an issue, if necessary for the trivial change).

Also for that .gitignore yes another PR is welcome ;-)

Thanks a lot that's nice, an issue is not needed for a trivial change like that!

mtardy

Welcome to contributing to Tetragon! 🤗

That's very nice of you to take the time to add tests for invalid tracing policies. I have a few minor comments, hope it's helpful with Djalal's review as well.

mtardy · 2024-01-29T10:55:40Z

pkg/tracingpolicy/generictracingpolicy_test.go

+func deleteTempFile(path string) {
+	err := os.Remove(path)
+	if err != nil {
+		logger.GetLogger().Panicf("cannot remove temporary file %q", path)
+	}
+}


you can remove this function by passing t *testing.T to your createTempFile function and register your cleanup function t.Cleanup(f func()). It's a bit like a defer statement but that executes when the test or subtest is done.

mtardy · 2024-01-29T10:58:11Z

pkg/tracingpolicy/generictracingpolicy_test.go

+func createTempFile(data string) string {
+	file, err := ioutil.TempFile("", "*")
+	if err != nil {
+		logger.GetLogger().Panic("cannot create temporary file")


yeah don't use the logger and panic in tests, if you have t *testing.T available (see my other remark) just call t.Fatal if you want to stop this test here because of the error.

mtardy · 2024-01-29T11:00:16Z

pkg/tracingpolicy/generictracingpolicy_test.go

@@ -527,3 +529,82 @@ func TestYamlNamespaced(t *testing.T) {
 	_, ok := tp.(TracingPolicyNamespaced)
 	require.True(t, ok)
 }
+
+func createTempFile(data string) string {
+	file, err := ioutil.TempFile("", "*")


As the linter said, you can use https://pkg.go.dev/os#CreateTemp here, you can add a prefix or a suffix with tetragon in it btw it can be useful in case the temp file fails to cleanup, not necessary since it should be in tmp but still it's nice. Maybe you can find other example in the codebase, not sure!

mtardy · 2024-01-29T11:01:23Z

pkg/tracingpolicy/generictracingpolicy_test.go

+	}
+	err = file.Close()
+	if err != nil {
+		logger.GetLogger().Panicf("cannot close temporary file %q", path)


maybe you can just T.Fail here or just warn since it's not a big deal if we fail to close that file, and should not happen that often.

Incorporates reviewer's comments on PR cilium#2031 Signed-off-by: Christian Hörtnagl <christian2@univie.ac.at>

christian-2 · 2024-01-29T12:15:18Z

@tixxdz @mtardy thanks for your constructive comments; they should be on board now.

As agreed in PR cilium#2031 Signed-off-by: Christian Hörtnagl <christian2@univie.ac.at>

mtardy

Here are a few comments, could you also squash your commits? You can use:

git rebase -i <first commit of PR>^
# mark commit to squash as s
# if you are happy with the rebase, you can check with git log and git show
git push origin <your branch> --force-with-lease

mtardy · 2024-01-29T14:24:14Z

pkg/tracingpolicy/generictracingpolicy_test.go

+	t.Cleanup(func() {
+		err := os.Remove(path)
+		if err != nil {
+			t.Error(err)
+		}
+	})


since you are using file, err := os.CreateTemp(t.TempDir(), "tetragon-"), you can remove the cleanup part since the TempDir folder is already automatically removed:

TempDir returns a temporary directory for the test to use. The directory is automatically removed by Cleanup when the test and all its subtests complete.

mtardy · 2024-01-29T14:25:36Z

pkg/tracingpolicy/generictracingpolicy_test.go

+func createTempFile(t *testing.T, data string) string {
+	file, err := os.CreateTemp(t.TempDir(), "tetragon-")
+	if err != nil {
+		t.Error(err)


I think you should use Fatal here since you cannot recover from this and you should stop the test here.

mtardy · 2024-01-29T14:25:49Z

pkg/tracingpolicy/generictracingpolicy_test.go

+
+	_, err = file.WriteString(data)
+	if err != nil {
+		t.Error(err)


Same I think you should use Fatal here.

mtardy · 2024-01-29T14:27:30Z

pkg/tracingpolicy/generictracingpolicy_test.go

+		}
+	})
+
+	return path


Suggested change

return path

return file.Name()

mtardy · 2024-01-29T14:27:36Z

pkg/tracingpolicy/generictracingpolicy_test.go

+	if err != nil {
+		t.Error(err)
+	}
+	path := file.Name()


Suggested change

path := file.Name()

As agreed in PR cilium#2031 Signed-off-by: Christian Hörtnagl <christian2@univie.ac.at>

christian-2 · 2024-01-31T10:12:56Z

@mtardy I've taken your suggestions from the second round of reviewing on board now. The only difference is that I opted for testing.assert.NoError() instead of testing.Fatal(); both should stop the test's execution.

mtardy · 2024-01-31T12:24:02Z

@mtardy I've taken your suggestions from the second round of reviewing on board now. The only difference is that I opted for testing.assert.NoError() instead of testing.Fatal(); both should stop the test's execution.

Yeah it's fine.

One can argue that it should not be an assert.NoErrro() since it's not strictly speaking a test error, meaning the test found something that is wrong but that the test machinery failed. In other words the test did not uncover an issue but just failed in itself. But honestly, this is just a nit at this stage, let's merge this as-is, it's not a big deal!

As agreed in PR #2031 Signed-off-by: Christian Hörtnagl <christian2@univie.ac.at>

christian-2 · 2024-01-31T12:31:09Z

@mtardy Yep. I'd propose I'll consider this if we shall need another round of changes. All your input on this issue has been very constructive and helpful so far, so I'd have absolutely no problem with that. Thanks again.

mtardy · 2024-01-31T16:35:31Z

@mtardy Yep. I'd propose I'll consider this if we shall need another round of changes. All your input on this issue has been very constructive and helpful so far, so I'd have absolutely no problem with that. Thanks again.

it's up to you, we can merge as-is if you want.

christian-2 · 2024-01-31T16:39:46Z

Then I'd rather revert to testing.Fatal(), it's the (slightly) better choice also IMO. Just a sec ... ;)

Callers of tracingpolicy.FromFile() no longer receive highly specific errors, but a generic error, which can also be presented to end users. Callers of tracingpolicy.FromJSON() still receive the specific errors, and any generic error wraps a specific one. Signed-off-by: Christian Hörtnagl <christian2@univie.ac.at>

mtardy

Thanks again! 🎉

christian-2 requested a review from a team as a code owner January 27, 2024 19:50

christian-2 requested a review from tixxdz January 27, 2024 19:50

tixxdz reviewed Jan 29, 2024

View reviewed changes

tixxdz added the release-note/minor This PR introduces a minor user-visible change label Jan 29, 2024

mtardy reviewed Jan 29, 2024

View reviewed changes

christian-2 added a commit to christian-2/tetragon that referenced this pull request Jan 29, 2024

validation: improve error message when failing to load a TracingPolicy

aa4494e

Incorporates reviewer's comments on PR cilium#2031 Signed-off-by: Christian Hörtnagl <christian2@univie.ac.at>

christian-2 added a commit to christian-2/tetragon that referenced this pull request Jan 29, 2024

Git-ignore drop-privileges along with other binaries

5333200

As agreed in PR cilium#2031 Signed-off-by: Christian Hörtnagl <christian2@univie.ac.at>

christian-2 mentioned this pull request Jan 29, 2024

Git-ignore drop-privileges along with other binaries #2038

Merged

mtardy reviewed Jan 29, 2024

View reviewed changes

christian-2 force-pushed the fixes-2022 branch from aa4494e to cfcdf5c Compare January 31, 2024 09:58

christian-2 added a commit to christian-2/tetragon that referenced this pull request Jan 31, 2024

Git-ignore drop-privileges along with other binaries

30dd955

As agreed in PR cilium#2031 Signed-off-by: Christian Hörtnagl <christian2@univie.ac.at>

mtardy pushed a commit that referenced this pull request Jan 31, 2024

Git-ignore drop-privileges along with other binaries

21f3173

As agreed in PR #2031 Signed-off-by: Christian Hörtnagl <christian2@univie.ac.at>

mtardy approved these changes Jan 31, 2024

View reviewed changes

christian-2 force-pushed the fixes-2022 branch from cfcdf5c to 1c23baa Compare January 31, 2024 17:42

mtardy approved these changes Jan 31, 2024

View reviewed changes

mtardy merged commit 1da0a48 into cilium:main Jan 31, 2024
30 of 31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validation: improve error message when failing to load a TracingPolicy #2031

validation: improve error message when failing to load a TracingPolicy #2031

christian-2 commented Jan 27, 2024

tixxdz Jan 29, 2024

mtardy Jan 29, 2024

tixxdz left a comment

tixxdz commented Jan 29, 2024

mtardy commented Jan 29, 2024

mtardy left a comment

mtardy Jan 29, 2024

mtardy Jan 29, 2024

mtardy Jan 29, 2024

mtardy Jan 29, 2024

christian-2 commented Jan 29, 2024

mtardy left a comment

mtardy Jan 29, 2024

mtardy Jan 29, 2024

mtardy Jan 29, 2024

mtardy Jan 29, 2024

mtardy Jan 29, 2024

christian-2 commented Jan 31, 2024

mtardy commented Jan 31, 2024

christian-2 commented Jan 31, 2024

mtardy commented Jan 31, 2024

christian-2 commented Jan 31, 2024

mtardy left a comment

validation: improve error message when failing to load a TracingPolicy #2031

validation: improve error message when failing to load a TracingPolicy #2031

Conversation

christian-2 commented Jan 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tixxdz left a comment

Choose a reason for hiding this comment

tixxdz commented Jan 29, 2024

mtardy commented Jan 29, 2024

mtardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christian-2 commented Jan 29, 2024

mtardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christian-2 commented Jan 31, 2024

mtardy commented Jan 31, 2024

christian-2 commented Jan 31, 2024

mtardy commented Jan 31, 2024

christian-2 commented Jan 31, 2024

mtardy left a comment

Choose a reason for hiding this comment