Repository: kubernetes-sigs/security-profiles-operator
Pull request #344 (2021-03-04 15:37:14)

Add multi-container recording support
#### What type of PR is this?

/kind feature

#### What this PR does / why we need it:
This adds support for multi container recording via the OCI seccomp BPF
hook. To not confuse users, we now require the (unreleased) version of
CRI-O v1.21.0. We always suffix the recording by the container name,
which allows the user to distinguish the profiles in a sane way.

#### Which issue(s) this PR fixes:

<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.

Fixes #

or

None
-->
None
#### Does this PR have test?
Yes
<!--
If tests aren't applicable just write N/A.
-->

#### Special notes for your reviewer:
None
#### Does this PR introduce a user-facing change?

<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".

For more information on release notes see: https://git.k8s.io/community/contributors/guide/release-notes.md
-->

```release-note
None
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/security-profiles-operator/pull/344#" title="Author self-approved">saschagrunert</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fsecurity-profiles-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/security-profiles-operator/blob/master/OWNERS)~~ [saschagrunert]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/deploy/base/config.yaml b/deploy/base/config.yaml
index 647311b6b..e52390928 100644
--- a/deploy/base/config.yaml
+++ b/deploy/base/config.yaml
@@ -7,3 +7,13 @@ metadata:
 spec:
   enableSelinux: false
   enableLogEnricher: false
+  tolerations:
+    - key: "node-role.kubernetes.io/master"
+      operator: "Exists"
+      effect: "NoSchedule"
+    - key: "node-role.kubernetes.io/control-plane"
+      operator: "Exists"
+      effect: "NoSchedule"
+    - key: "node.kubernetes.io/not-ready"
+      operator: "Exists"
+      effect: "NoExecute"
diff --git a/deploy/namespace-operator.yaml b/deploy/namespace-operator.yaml
index c668cd827..9b4b2abfd 100644
--- a/deploy/namespace-operator.yaml
+++ b/deploy/namespace-operator.yaml
@@ -768,6 +768,16 @@ metadata:
 spec:
   enableLogEnricher: false
   enableSelinux: false
+  tolerations:
+  - effect: NoSchedule
+    key: node-role.kubernetes.io/master
+    operator: Exists
+  - effect: NoSchedule
+    key: node-role.kubernetes.io/control-plane
+    operator: Exists
+  - effect: NoExecute
+    key: node.kubernetes.io/not-ready
+    operator: Exists
 ---
 apiVersion: v1
 data:
diff --git a/deploy/operator.yaml b/deploy/operator.yaml
index f32564257..6ba563f28 100644
--- a/deploy/operator.yaml
+++ b/deploy/operator.yaml
@@ -766,6 +766,16 @@ metadata:
 spec:
   enableLogEnricher: false
   enableSelinux: false
+  tolerations:
+  - effect: NoSchedule
+    key: node-role.kubernetes.io/master
+    operator: Exists
+  - effect: NoSchedule
+    key: node-role.kubernetes.io/control-plane
+    operator: Exists
+  - effect: NoExecute
+    key: node.kubernetes.io/not-ready
+    operator: Exists
 ---
 apiVersion: v1
 data:
diff --git a/installation-usage.md b/installation-usage.md
index 788d6ceae..74c0b97da 100644
--- a/installation-usage.md
+++ b/installation-usage.md
@@ -208,7 +208,7 @@ hooks_dir = [
 ```
 
 The hook references a [path][path] to the actual binary which gets executed on
-`prestart`. Please note that at least CRI-O v1.20.0 is required to let the hook
+`prestart`. Please note that at least CRI-O v1.21.0 is required to let the hook
 and CRI-O work nicely together.
 
 [bpf-hook]: https://github.com/containers/oci-seccomp-bpf-hook
@@ -249,17 +249,17 @@ multiple times.
 Now we can start the recording by running a workload which contains the label:
 
 ```
-$ kubectl run --rm -it alpine --image=alpine --labels app=alpine -- sh
+$ kubectl run --rm -it my-pod --image=alpine --labels app=alpine -- sh
 / # mkdir test
 ```
 
 If we exit the workload, then it automatically will be removed because of the
 `--rm` CLI flag. Once the workload is removed, the operator will create the CRD
-for us:
+for us. The name of the CRD is suffixed with the pod name:
 
 ```
-$ kubectl describe seccompprofile test-recording
-Name:         test-recording
+$ kubectl describe seccompprofile test-recording-my-pod
+Name:         test-recording-my-pod
 Namespace:    security-profiles-operator
 …
 Spec:
@@ -272,10 +272,10 @@ Spec:
       …[other syscalls]…
       mkdir
       …[other syscalls]…
-  Target Workload:
+  Target Workload: my-pod
 Status:
-  Localhost Profile:  operator/security-profiles-operator/test-recording.json
-  Path:               /var/lib/kubelet/seccomp/operator/security-profiles-operator/test-recording.json
+  Localhost Profile:  operator/security-profiles-operator/my-pod/test-recording-my-pod.json
+  Path:               /var/lib/kubelet/seccomp/operator/security-profiles-operator/my-pod/test-recording-my-pod.json
   Status:             Active
 Events:
   Type    Reason                 Age                From             Message
@@ -297,6 +297,46 @@ if anything goes wrong:
 11s         Normal    SavedSeccompProfile            seccompprofile/test-recording                             Successfully saved profile to disk
 ```
 
+It is also possible to record profiles from multiple containers, for example by
+using this recording and Pod manifest:
+
+```
+---
+apiVersion: security-profiles-operator.x-k8s.io/v1alpha1
+kind: ProfileRecording
+metadata:
+  name: recording
+spec:
+  kind: SeccompProfile
+  podSelector:
+    matchLabels:
+      app: my-app
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: my-pod
+  labels:
+    app: my-app
+spec:
+  containers:
+  - name: nginx
+    image: quay.io/security-profiles-operator/test-nginx:1.19.1
+  - name: redis
+    image: quay.io/security-profiles-operator/redis:6.2.1
+  restartPolicy: Never
+```
+
+If the workload gets created and removed again, then the recorder will produce
+two seccomp profiles for each container:
+
+```
+> kubectl get sp -o wide
+NAME              STATUS   AGE   LOCALHOSTPROFILE
+recording-nginx   Active   32s   operator/default/my-pod/recording-nginx.json
+recording-redis   Active   32s   operator/default/my-pod/recording-redis.json
+```
+
 Please note that we encourage you to only use this recording approach for
 development purposes. It is not recommended to use the OCI hook in production
 clusters because it runs as highly privileged process to trace the container
diff --git a/internal/pkg/daemon/profilerecorder/profilerecorder.go b/internal/pkg/daemon/profilerecorder/profilerecorder.go
index cf60f28e7..029c14b31 100644
--- a/internal/pkg/daemon/profilerecorder/profilerecorder.go
+++ b/internal/pkg/daemon/profilerecorder/profilerecorder.go
@@ -19,6 +19,7 @@ package profilerecorder
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -64,7 +65,7 @@ type RecorderReconciler struct {
 	log           logr.Logger
 	record        event.Recorder
 	nodeAddresses []string
-	podsToWatch   map[string]string
+	podsToWatch   map[string][]string
 }
 
 // +kubebuilder:rbac:groups=core,resources=nodes,verbs=get;list;watch
@@ -103,7 +104,7 @@ func Setup(ctx context.Context, mgr ctrl.Manager, l logr.Logger) error {
 		log:           l,
 		nodeAddresses: nodeAddresses,
 		record:        event.NewAPIRecorder(mgr.GetEventRecorderFor(name)),
-		podsToWatch:   map[string]string{},
+		podsToWatch:   map[string][]string{},
 	}
 
 	return ctrl.NewControllerManagedBy(mgr).
@@ -139,11 +140,17 @@ func (r *RecorderReconciler) isPodWithTraceAnnotation(obj runtime.Object) bool {
 		return false
 	}
 
-	return p.Annotations[config.SeccompProfileRecordAnnotationKey] != ""
+	for key := range p.Annotations {
+		if strings.HasPrefix(key, config.SeccompProfileRecordAnnotationKey+"/") {
+			return true
+		}
+	}
+
+	return false
 }
 
 func (r *RecorderReconciler) Reconcile(_ context.Context, req reconcile.Request) (reconcile.Result, error) {
-	logger := r.log.WithValues("recorder", req.Name, "namespace", req.Namespace)
+	logger := r.log.WithValues("pod", req.Name, "namespace", req.Namespace)
 
 	ctx, cancel := context.WithTimeout(context.Background(), reconcileTimeout)
 	defer cancel()
@@ -162,11 +169,12 @@ func (r *RecorderReconciler) Reconcile(_ context.Context, req reconcile.Request)
 	}
 
 	if pod.Status.Phase == corev1.PodPending {
-		if r.podsToWatch[req.NamespacedName.String()] != "" {
+		if len(r.podsToWatch[req.NamespacedName.String()]) > 0 {
 			// We're tracking this pod already
 			return reconcile.Result{}, nil
 		}
-		outputFile, err := parseAnnotation(pod.Annotations[config.SeccompProfileRecordAnnotationKey])
+
+		outputFiles, err := parseAnnotations(pod.Annotations)
 		if err != nil {
 			// Malformed annotations could be set by users directly, which is
 			// why we are ignoring them.
@@ -174,14 +182,12 @@ func (r *RecorderReconciler) Reconcile(_ context.Context, req reconcile.Request)
 			r.record.Event(pod, event.Warning(reasonAnnotationParsing, err))
 			return reconcile.Result{}, nil
 		}
-		if !strings.HasPrefix(outputFile, config.ProfileRecordingOutputPath) {
-			logger.Info("Ignoring profile outside standard output path")
-			return reconcile.Result{}, nil
-		}
 
-		logger.Info("Recording seccomp profile", "path", outputFile, "pod", pod.Name)
-		r.podsToWatch[req.NamespacedName.String()] = outputFile
-		r.record.Event(pod, event.Normal(reasonProfileRecording, "Recording seccomp profile"))
+		logger.Info(fmt.Sprintf(
+			"Recording seccomp profiles: %s", strings.Join(outputFiles, ", "),
+		))
+		r.podsToWatch[req.NamespacedName.String()] = outputFiles
+		r.record.Event(pod, event.Normal(reasonProfileRecording, "Recording seccomp profiles"))
 	}
 
 	if pod.Status.Phase == corev1.PodSucceeded {
@@ -194,68 +200,90 @@ func (r *RecorderReconciler) Reconcile(_ context.Context, req reconcile.Request)
 }
 
 func (r *RecorderReconciler) collectProfile(ctx context.Context, name types.NamespacedName) error {
-	profilePath := r.podsToWatch[name.String()]
-	if profilePath == "" {
-		// ignoring this error for now
-		return nil
-	}
-
-	r.log.Info("Collecting profile", "path", r.podsToWatch[name.String()])
 	defer delete(r.podsToWatch, name.String())
-	data, err := ioutil.ReadFile(profilePath)
-	if err != nil {
-		r.log.Error(err, "Failed to read profile")
-		return errors.Wrap(err, "read profile")
-	}
 
-	// Remove the file extension and timestamp
-	base := filepath.Base(profilePath)
-	lastIndex := strings.LastIndex(base, "-")
-	if lastIndex == -1 {
-		return errors.Errorf("malformed profile path: %s", profilePath)
-	}
-	profileName := base[:lastIndex]
+	for _, profilePath := range r.podsToWatch[name.String()] {
+		r.log.Info("Collecting profile", "path", profilePath)
 
-	profile := &v1alpha1.SeccompProfile{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      profileName,
-			Namespace: name.Namespace,
-		},
-	}
-	res, err := controllerutil.CreateOrUpdate(ctx, r.client, profile, func() error {
-		err := json.Unmarshal(data, &profile.Spec)
-		return errors.Wrap(err, "unmarshal profile spec JSON")
-	})
-	if err != nil {
-		r.log.Error(err, "cannot create seccompprofile resource")
-		r.record.Event(profile, event.Warning(reasonProfileCreationFailed, err))
-		return errors.Wrap(err, "create seccompProfile resource")
+		data, err := ioutil.ReadFile(profilePath)
+		if err != nil {
+			r.log.Error(err, "Failed to read profile")
+			return errors.Wrap(err, "read profile")
+		}
+
+		// Remove the file extension and timestamp
+		base := filepath.Base(profilePath)
+		lastIndex := strings.LastIndex(base, "-")
+		if lastIndex == -1 {
+			return errors.Errorf("Malformed profile path: %s", profilePath)
+		}
+		profileName := base[:lastIndex]
+
+		profile := &v1alpha1.SeccompProfile{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      profileName,
+				Namespace: name.Namespace,
+			},
+			Spec: v1alpha1.SeccompProfileSpec{
+				TargetWorkload: name.Name,
+			},
+		}
+		res, err := controllerutil.CreateOrUpdate(ctx, r.client, profile,
+			func() error {
+				return errors.Wrap(
+					json.Unmarshal(data, &profile.Spec),
+					"unmarshal profile spec JSON",
+				)
+			},
+		)
+		if err != nil {
+			r.log.Error(err, "Cannot create seccompprofile resource")
+			r.record.Event(profile, event.Warning(reasonProfileCreationFailed, err))
+			return errors.Wrap(err, "create seccompProfile resource")
+		}
+		r.log.Info("Created/updated profile", "action", res, "name", profileName)
+		r.record.Event(
+			profile,
+			event.Normal(reasonProfileCreated, "seccomp profile created"),
+		)
 	}
-	r.log.Info("Created/updated profile", "action", res, "name", profileName)
-	r.record.Event(profile, event.Normal(reasonProfileCreated, "seccomp profile created"))
+
 	return nil
 }
 
-// parseAnnotation parses the provided annotation and extracts the mandatory
-// output file.
-func parseAnnotation(annotation string) (string, error) {
+// parseAnnotations parses the provided annotations and extracts the mandatory
+// output files.
+func parseAnnotations(annotations map[string]string) (res []string, err error) {
 	const prefix = "of:"
 
-	if !strings.HasPrefix(annotation, prefix) {
-		return "", errors.Wrapf(errors.New(errInvalidAnnotation),
-			"must start with %q prefix", prefix)
-	}
+	for key, value := range annotations {
+		if !strings.HasPrefix(key, config.SeccompProfileRecordAnnotationKey+"/") {
+			continue
+		}
 
-	outputFile := strings.TrimSpace(strings.TrimPrefix(annotation, prefix))
-	if !filepath.IsAbs(outputFile) {
-		return "", errors.Wrapf(errors.New(errInvalidAnnotation),
-			"output file path must be absolute: %q", outputFile)
-	}
+		if !strings.HasPrefix(value, prefix) {
+			return nil, errors.Wrapf(errors.New(errInvalidAnnotation),
+				"must start with %q prefix", prefix)
+		}
+
+		outputFile := strings.TrimSpace(strings.TrimPrefix(value, prefix))
+		if !filepath.IsAbs(outputFile) {
+			return nil, errors.Wrapf(errors.New(errInvalidAnnotation),
+				"output file path must be absolute: %q", outputFile)
+		}
+
+		if outputFile == "" {
+			return nil, errors.Wrap(errors.New(errInvalidAnnotation),
+				"providing output file is mandatory")
+		}
+		if !strings.HasPrefix(outputFile, config.ProfileRecordingOutputPath) {
+			// Ignoring profile outside standard output path, it may be
+			// user-defined
+			continue
+		}
 
-	if outputFile == "" {
-		return "", errors.Wrap(errors.New(errInvalidAnnotation),
-			"providing output file is mandatory")
+		res = append(res, outputFile)
 	}
 
-	return outputFile, nil
+	return res, nil
 }
diff --git a/internal/pkg/webhooks/recording/recording.go b/internal/pkg/webhooks/recording/recording.go
index 7f64545ab..401ccc590 100644
--- a/internal/pkg/webhooks/recording/recording.go
+++ b/internal/pkg/webhooks/recording/recording.go
@@ -122,7 +122,7 @@ func (p *podSeccompRecorder) Handle(
 		}
 
 		if selector.Matches(podLabels) {
-			podChanged = p.addAnnotation(pod, &items[i])
+			podChanged = p.addAnnotations(pod, &items[i])
 		}
 
 		if podChanged {
@@ -145,41 +145,54 @@ func (p *podSeccompRecorder) Handle(
 	return admission.PatchResponseFromRaw(req.Object.Raw, marshaledPod)
 }
 
-func (p *podSeccompRecorder) addAnnotation(
+func (p *podSeccompRecorder) addAnnotations(
 	pod *corev1.Pod,
 	profileRecording *profilerecordingv1alpha1.ProfileRecording,
 ) (podChanged bool) {
-	value, ok := pod.GetAnnotations()[config.SeccompProfileRecordAnnotationKey]
-
-	targetValue := fmt.Sprintf(
-		"of:%s/%s-%d.json",
-		config.ProfileRecordingOutputPath,
-		profileRecording.GetName(),
-		time.Now().Unix(),
-	)
+	ctrs := []corev1.Container{}
+	ctrs = append(ctrs, pod.Spec.InitContainers...)
+	ctrs = append(ctrs, pod.Spec.Containers...)
+
+	for i := range ctrs {
+		ctr := &ctrs[i]
+		key := fmt.Sprintf("%s/%s", config.SeccompProfileRecordAnnotationKey, ctr.Name)
+
+		value := fmt.Sprintf(
+			"of:%s/%s-%s-%d.json",
+			config.ProfileRecordingOutputPath,
+			profileRecording.GetName(),
+			ctr.Name,
+			time.Now().Unix(),
+		)
 
-	if !ok {
-		if pod.Annotations == nil {
-			pod.Annotations = make(map[string]string)
+		existingValue, ok := pod.GetAnnotations()[key]
+		if !ok {
+			if pod.Annotations == nil {
+				pod.Annotations = make(map[string]string)
+			}
+			pod.Annotations[key] = value
+			log.Info(fmt.Sprintf(
+				"adding seccomp recording annotation %s=%s to pod %s",
+				key, value, pod.Name,
+			))
+			podChanged = true
+			continue
 		}
-		pod.Annotations[config.SeccompProfileRecordAnnotationKey] = targetValue
-		log.Info("adding seccomp recording annotation to pod", "Pod", pod.Name)
-		return true
-	}
 
-	if value != targetValue {
-		log.Error(
-			errors.New("existing annotation"),
-			fmt.Sprintf(
-				"Workload %s already has annotation %s (not mutating to %s).",
-				pod.Name,
-				value,
-				targetValue,
-			),
-		)
+		if existingValue != value {
+			log.Error(
+				errors.New("existing annotation"),
+				fmt.Sprintf(
+					"workload %s already has annotation %s (not mutating to %s).",
+					pod.Name,
+					existingValue,
+					value,
+				),
+			)
+		}
 	}
 
-	return false
+	return podChanged
 }
 
 func (p *podSeccompRecorder) addPod(


Test patch:
diff --git a/test/e2e_test.go b/test/e2e_test.go
index 0848f802b..e7c00f3f3 100644
--- a/test/e2e_test.go
+++ b/test/e2e_test.go
@@ -107,6 +107,7 @@ func (e *e2e) TestSecurityProfilesOperator() {
 	e.Run("cluster-wide: Seccomp: Verify profile recording", func() {
 		e.testCaseProfileRecordingStaticPod()
 		e.testCaseProfileRecordingKubectlRun()
+		e.testCaseProfileRecordingMultiContainer()
 	})
 
 	e.cleanupWebhook(webhookManifest)
diff --git a/test/tc_profilerecordings_test.go b/test/tc_profilerecordings_test.go
index 23b4929a7..23050da2c 100644
--- a/test/tc_profilerecordings_test.go
+++ b/test/tc_profilerecordings_test.go
@@ -44,8 +44,7 @@ metadata:
 spec:
   containers:
   - image: quay.io/security-profiles-operator/test-nginx:1.19.1
-    name: recording
-    resources: {}
+    name: nginx
   restartPolicy: Never
 `
 	testPodFile, err := ioutil.TempFile(os.TempDir(), "recording-pod*.yaml")
@@ -61,8 +60,9 @@ spec:
 	e.waitFor("condition=ready", "pod", "recording")
 	e.kubectl("delete", "pod", "recording")
 
-	profile := e.kubectl("get", "sp", recordingName, "-o", "yaml")
-	defer e.kubectl("delete", "sp", recordingName)
+	resourceName := recordingName + "-nginx"
+	profile := e.kubectl("get", "sp", resourceName, "-o", "yaml")
+	defer e.kubectl("delete", "sp", resourceName)
 	e.Contains(profile, "mkdir")
 }
 
@@ -77,10 +77,55 @@ func (e *e2e) testCaseProfileRecordingKubectlRun() {
 	e.kubectl(
 		"run", "--rm", "-it", "--restart=Never", "--labels=app=alpine",
 		"--image=registry.fedoraproject.org/fedora-minimal:latest",
-		"test", "--", "echo", "test",
+		"fedora", "--", "echo", "test",
 	)
 
-	profile := e.kubectl("get", "sp", recordingName, "-o", "yaml")
+	resourceName := recordingName + "-fedora"
+	profile := e.kubectl("get", "sp", resourceName, "-o", "yaml")
+	defer e.kubectl("delete", "sp", resourceName)
 	e.Contains(profile, "prctl")
 	e.NotContains(profile, "mkdir")
 }
+
+func (e *e2e) testCaseProfileRecordingMultiContainer() {
+	e.profileRecordingTestCase()
+
+	e.logf("Creating recording")
+	e.kubectl("create", "-f", exampleRecordingPath)
+	defer e.kubectl("delete", "-f", exampleRecordingPath)
+
+	e.logf("Creating test pod")
+	const testPod = `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: my-pod
+  labels:
+    app: alpine
+spec:
+  containers:
+  - name: nginx
+    image: quay.io/security-profiles-operator/test-nginx:1.19.1
+  - name: redis
+    image: quay.io/security-profiles-operator/redis:6.2.1
+  restartPolicy: Never
+`
+	testPodFile, err := ioutil.TempFile(os.TempDir(), "recording-pod*.yaml")
+	e.Nil(err)
+	defer os.Remove(testPodFile.Name())
+	_, err = testPodFile.Write([]byte(testPod))
+	e.Nil(err)
+	err = testPodFile.Close()
+	e.Nil(err)
+	e.kubectl("create", "-f", testPodFile.Name())
+
+	e.logf("Waiting for test pod to be initialized")
+	e.waitFor("condition=ready", "pod", "my-pod")
+	e.kubectl("delete", "pod", "my-pod")
+
+	profileRedis := e.kubectl("get", "sp", recordingName+"-redis", "-o", "yaml")
+	profileNginx := e.kubectl("get", "sp", recordingName+"-nginx", "-o", "yaml")
+	defer e.kubectl("delete", "sp", "--all")
+	e.Contains(profileNginx, "unlink")
+	e.Contains(profileRedis, "nanosleep")
+}