From a21e0e41aaada7ed3bea611915261516416e2e89 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Wed, 31 Aug 2022 16:23:50 -0400
Subject: [PATCH] kola: Add `isolation=readonly|dynamicuser`

Part of my war against duplicative comments in our kola tests.
We have a few tests that write a comment like this:

```
 # - exclusive: false
 #   - This test doesn't make meaningful changes to the system and
 #     should be able to be combined with other tests.
```
Of course, this comment is already redundant because the
meaning of the `exclusive` tag is defined canonically in coreos-assembler
(here) and copy-pasting that into every test that uses it would
be pointlessly verbose.

But - we can do one better.  Instead of having a test flag which
is mainly an "I promise not to mutate the system in a way which could
interfere with other tests", let's add a field that *enforces* this.
Then it doesn't need to be commented; we have a variety of
tests which are just "system inspection" (e.g. query rpmdb) and
run just fine with `DynamicUser=yes` and hence *cannot* affect
the system, and hence are inherently isolated from other concurrent
tests.
---
 docs/kola/external-tests.md                   |  7 +++++
 mantle/kola/harness.go                        | 31 ++++++++++++++++++-
 .../tests/kola/exclusive-readonly             |  9 ++++++
 3 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100755 tests/kola-ci-self/tests/kola/exclusive-readonly

diff --git a/docs/kola/external-tests.md b/docs/kola/external-tests.md
index 0d94f3d0a3..9696ba82ef 100644
--- a/docs/kola/external-tests.md
+++ b/docs/kola/external-tests.md
@@ -264,6 +264,13 @@ is simple and is not expected to conflict with other tests, it should be marked
 `exclusive: false`. When the `exclusive` key is not provided, tests are marked
 `exclusive: true` by default.
 
+The `isolation` key can take two values: `readonly` and `dynamicuser`.  These
+are thin wrappers for the equivalent systemd options; `readonly` equals `ProtectSystem=strict`,
+and `dynamicuser` means `DynamicUser=yes`.  Setting either of these options
+also implies `exclusive: false`.  Use these for tests that are mostly about
+read-only system inspection.  If specified, the test *cannot* provide
+an Ignition (or butane) config either.
+
 The `conflicts` key takes a list of test names that conflict with this test.
 This key can only be specified if `exclusive` is marked `false` since
 `exclusive: true` tests are run exclusively in their own VM.  At runtime,
diff --git a/mantle/kola/harness.go b/mantle/kola/harness.go
index 2894a4da07..1b5f795fc6 100644
--- a/mantle/kola/harness.go
+++ b/mantle/kola/harness.go
@@ -826,6 +826,7 @@ type externalTestMeta struct {
 	AppendKernelArgs          string   `json:"appendKernelArgs,omitempty"`
 	AppendFirstbootKernelArgs string   `json:"appendFirstbootKernelArgs,omitempty"`
 	Exclusive                 bool     `json:"exclusive"`
+	Isolation                 string   `json:"isolation"`
 	TimeoutMin                int      `json:"timeoutMin"`
 	Conflicts                 []string `json:"conflicts"`
 	AllowConfigWarnings       bool     `json:"allowConfigWarnings"`
@@ -943,6 +944,24 @@ func registerExternalTest(testname, executable, dependencydir string, userdata *
 		targetMeta = &metaCopy
 	}
 
+	switch targetMeta.Isolation {
+	case "readonly":
+	case "dynamicuser":
+		// These tests cannot provide their own Ignition
+		if userdata != nil {
+			return fmt.Errorf("test %v specifies isolation=%v but includes an Ignition config", testname, targetMeta.Isolation)
+		}
+		targetMeta.Exclusive = false
+	case "":
+		break
+	default:
+		return fmt.Errorf("test %v specifies unknown isolation=%v", testname, targetMeta.Isolation)
+	}
+
+	if userdata == nil {
+		userdata = conf.EmptyIgnition()
+	}
+
 	warningsAction := conf.FailWarnings
 	if targetMeta.AllowConfigWarnings {
 		warningsAction = conf.IgnoreWarnings
@@ -980,6 +999,16 @@ Environment=KOLA_TEST_EXE=%s
 Environment=%s=%s
 ExecStart=%s
 `, unitName, testname, base, kolaExtBinDataEnv, destDataDir, remotepath)
+	switch targetMeta.Isolation {
+	case "readonly":
+		unit += "ProtectSystem=strict\nPrivateTmp=yes\n"
+	case "dynamicuser":
+		unit += "DynamicUser=yes\n"
+	case "":
+		break
+	default:
+		return fmt.Errorf("test %v specifies unknown isolation=%v", testname, targetMeta.Isolation)
+	}
 	if targetMeta.InjectContainer {
 		if CosaBuild == nil {
 			return fmt.Errorf("test %v uses injectContainer, but no cosa build found", testname)
@@ -1091,7 +1120,7 @@ func registerTestDir(dir, testprefix string, children []os.FileInfo) error {
 	var dependencydir string
 	var meta externalTestMeta
 	var err error
-	userdata := conf.EmptyIgnition()
+	var userdata *conf.UserData
 	executables := []string{}
 	for _, c := range children {
 		fpath := filepath.Join(dir, c.Name())
diff --git a/tests/kola-ci-self/tests/kola/exclusive-readonly b/tests/kola-ci-self/tests/kola/exclusive-readonly
new file mode 100755
index 0000000000..76dae88100
--- /dev/null
+++ b/tests/kola-ci-self/tests/kola/exclusive-readonly
@@ -0,0 +1,9 @@
+#!/bin/bash
+# This verifies that isolation=dynamicuser maps to systemd DynamicUser=yes and
+# (unlike the default kola model) runs as non-root.
+# kola: { "isolation": "dynamicuser" }
+set -xeuo pipefail
+
+id=$(id -u)
+test "$id" '!=' 0
+echo "ok dynamicuser"