Merge pull request #21 from jhrozek/ocp-merge_containers

OCPBUGS-4787: merging: Fix the mergeStrategy=containers option
openshift · Dec 16, 2022 · 2a82ee8 · 2a82ee8
2 parents 042bb06 + dee4f23
commit 2a82ee8
Show file tree

Hide file tree

Showing 4 changed files with 119 additions and 58 deletions.
diff --git a/internal/pkg/manager/recordingmerger/merge_utils.go b/internal/pkg/manager/recordingmerger/merge_utils.go
@@ -71,12 +71,14 @@ func mergeProfiles(profiles []mergeableProfile) (mergeableProfile, error) {
 	return base, nil
 }
 
+type perContainerMergeableProfiles map[string][]mergeableProfile
+
 func listPartialProfiles(
 	ctx context.Context,
 	cli client.Client,
 	list client.ObjectList,
 	recording *profilerecording1alpha1.ProfileRecording,
-) ([]mergeableProfile, error) {
+) (perContainerMergeableProfiles, error) {
 	if err := cli.List(
 		ctx,
 		list,
@@ -88,7 +90,7 @@ func listPartialProfiles(
 		return nil, fmt.Errorf("listing partial profiles for %s: %w", recording.Name, err)
 	}
 
-	partialProfiles := make([]mergeableProfile, 0)
+	partialProfiles := make(perContainerMergeableProfiles)
 	if err := meta.EachListItem(list, func(obj runtime.Object) error {
 		clientObj, ok := obj.(client.Object)
 		if !ok {
@@ -100,7 +102,12 @@ func listPartialProfiles(
 			return fmt.Errorf("failed to create mergeable profile for %s: %w", clientObj.GetName(), err)
 		}
 
-		partialProfiles = append(partialProfiles, partialPrf)
+		containerID := getContainerID(clientObj)
+		if containerID == "" {
+			// todo: log
+			return nil
+		}
+		partialProfiles[containerID] = append(partialProfiles[containerID], partialPrf)
 		return nil
 	}); err != nil {
 		return nil, fmt.Errorf("iterating over partial profiles: %w", err)
@@ -109,6 +116,14 @@ func listPartialProfiles(
 	return partialProfiles, nil
 }
 
+func getContainerID(prf client.Object) string {
+	labels := prf.GetLabels()
+	if labels == nil {
+		return ""
+	}
+	return labels[profilerecording1alpha1.ProfileToContainerLabel]
+}
+
 func deletePartialProfiles(
 	ctx context.Context,
 	cli client.Client,

diff --git a/internal/pkg/manager/recordingmerger/recordingmerger.go b/internal/pkg/manager/recordingmerger/recordingmerger.go
@@ -161,29 +161,33 @@ func (r *PolicyMergeReconciler) mergeTypedProfiles(
 		return nil
 	}
 
-	mergedProfile, err := mergeProfiles(partialProfiles)
-	if err != nil {
-		return fmt.Errorf("cannot merge partial profiles: %w", err)
-	}
+	for cntName, cntPartialProfiles := range partialProfiles {
+		r.log.Info("Merging profiles for container", "container", cntName)
 
-	if mergedProfile == nil {
-		r.record.Event(profileRecording,
-			event.Warning(
-				reasonMergedEmptyProfile,
-				errors.New(errEmptyMergedProfile),
-			),
-		)
-		r.log.Info(errEmptyMergedProfile)
-		return nil
-	}
+		mergedProfile, err := mergeProfiles(cntPartialProfiles)
+		if err != nil {
+			return fmt.Errorf("cannot merge partial profiles: %w", err)
+		}
 
-	mergedRecordingName := mergedProfileName(profileRecording.Name, partialProfiles[0])
-	res, err := createUpdateMergedProfile(ctx, r.client, profileRecording, mergedRecordingName, mergedProfile)
-	if err != nil {
-		r.record.Event(profileRecording, event.Warning(reasonCannotCreateUpdate, err))
-		return fmt.Errorf("cannot create or update merged profile: action:  %w", err)
+		if mergedProfile == nil {
+			r.record.Event(profileRecording,
+				event.Warning(
+					reasonMergedEmptyProfile,
+					errors.New(errEmptyMergedProfile),
+				),
+			)
+			r.log.Info(errEmptyMergedProfile)
+			return nil
+		}
+
+		mergedRecordingName := mergedProfileName(profileRecording.Name, cntPartialProfiles[0])
+		res, err := createUpdateMergedProfile(ctx, r.client, profileRecording, mergedRecordingName, mergedProfile)
+		if err != nil {
+			r.record.Event(profileRecording, event.Warning(reasonCannotCreateUpdate, err))
+			return fmt.Errorf("cannot create or update merged profile: action:  %w", err)
+		}
+		r.log.Info("Created/updated profile", "action", res, "name", mergedRecordingName)
 	}
-	r.log.Info("Created/updated profile", "action", res, "name", mergedRecordingName)
 
 	return deletePartialProfiles(ctx, r.client, profileItem, profileRecording)
 }

diff --git a/test/tc_profilemerging_test.go b/test/tc_profilemerging_test.go
@@ -26,7 +26,8 @@ import (
 )
 
 const (
-	containerName             = "nginx"
+	containerNameNginx        = "nginx"
+	containerNameRedis        = "redis"
 	mergeProfileRecordingName = "test-profile-merging"
 	profileRecordingTemplate  = `
     apiVersion: security-profiles-operator.x-k8s.io/v1alpha1
@@ -94,6 +95,40 @@ func (e *e2e) profileMergingTest(
 	recordedMethod, recorderKind, resource, trigger, commonAction, triggeredAction string,
 	conditions ...*regexp.Regexp,
 ) {
+	const testDeploymentMultiContainer = `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: my-deployment
+spec:
+  selector:
+    matchLabels:
+      app: alpine
+  replicas: 2
+  template:
+    metadata:
+      labels:
+        app: alpine
+    spec:
+      serviceAccountName: recording-sa
+      containers:
+      - name: redis
+        image: quay.io/security-profiles-operator/redis:6.2.1
+        readinessProbe:
+          tcpSocket:
+              port: 6379
+          initialDelaySeconds: 5
+          periodSeconds: 5
+      - name: nginx
+        image: quay.io/security-profiles-operator/test-nginx-unprivileged:1.21
+        ports:
+        - containerPort: 8080
+        readinessProbe:
+          tcpSocket:
+              port: 8080
+          initialDelaySeconds: 5
+          periodSeconds: 5
+`
 	e.logf("Creating a profile recording with merge strategy 'containers'")
 	deleteManifestFn := createTemplatedProfileRecording(e, profileRecTmplMetadata{
 		name:          mergeProfileRecordingName,
@@ -105,14 +140,14 @@ func (e *e2e) profileMergingTest(
 	})
 	defer deleteManifestFn()
 
-	since, deployName := e.createRecordingTestDeployment()
+	since, deployName := e.createRecordingTestDeploymentFromManifest(testDeploymentMultiContainer)
 	suffixes := e.getPodSuffixesByLabel("app=alpine")
 	if recordedMethod == "logs" {
 		e.waitForEnricherLogs(since, conditions...)
 	} else if recordedMethod == "bpf" {
 		profileNames := make([]string, 0)
 		for _, sfx := range suffixes {
-			profileNames = append(profileNames, mergeProfileRecordingName+"-"+containerName+"-"+sfx)
+			profileNames = append(profileNames, mergeProfileRecordingName+"-"+containerNameNginx+"-"+sfx)
 		}
 		e.waitForBpfRecorderLogs(since, profileNames...)
 	} else {
@@ -122,37 +157,40 @@ func (e *e2e) profileMergingTest(
 	podNamesString := e.kubectl("get", "pods", "-l", "app=alpine", "-o", "jsonpath={.items[*].metadata.name}")
 	onePodName := strings.Fields(podNamesString)[0]
 	e.kubectl(
-		"exec", onePodName, "--", "bash", "-c", trigger,
+		"exec", "-c", containerNameNginx, onePodName, "--", "bash", "-c", trigger,
 	)
 
 	e.kubectl("delete", "deploy", deployName)
 
 	// check that the policies are partial
 	for _, sfx := range suffixes {
-		recordedProfileName := mergeProfileRecordingName + "-" + containerName + "-" + sfx
-		e.logf("Checking that the recorded profile %s is partial", recordedProfileName)
-
-		profile := e.retryGetProfile(resource, recordedProfileName)
-		e.Contains(profile, v1alpha1.ProfilePartialLabel)
-		e.Contains(profile, commonAction)
-
-		if strings.HasSuffix(onePodName, sfx) {
-			// check the policy from the first container, it should contain the triggered action
-			e.Contains(profile, triggeredAction)
-		} else {
-			// the others should not
-			e.NotContains(profile, triggeredAction)
+		for _, containerName := range []string{containerNameNginx, containerNameRedis} {
+			recordedProfileName := mergeProfileRecordingName + "-" + containerName + "-" + sfx
+			e.logf("Checking that the recorded profile %s is partial", recordedProfileName)
+
+			profile := e.retryGetProfile(resource, recordedProfileName)
+			e.Contains(profile, v1alpha1.ProfilePartialLabel)
+			e.Contains(profile, commonAction)
+
+			if containerName == containerNameNginx {
+				if strings.HasSuffix(onePodName, sfx) {
+					// check the policy from the first container, it should contain the triggered action
+					e.Contains(profile, triggeredAction)
+				} else {
+					// the others should not
+					e.NotContains(profile, triggeredAction)
+				}
+			}
 		}
 	}
 
 	// delete the recording, this triggers the merge
 	e.kubectl("delete", "profilerecording", mergeProfileRecordingName)
 
-	// the partial policies should be gone, instead one policy should be created. Retry a couple of times
-	// because removing the partial policies is not atomic. In prod you'd probably list the profiles
-	// and check the absence of the partial label.
+	// the partial policies should be gone, instead one policy should be created for each container.
+	// Retry a couple of times because removing the partial policies is not atomic. In prod you'd probably list the
+	// profiles and check the absence of the partial label.
 	policiesRecorded := make([]string, 0)
-	mergedProfileName := fmt.Sprintf("%s-%s", mergeProfileRecordingName, containerName)
 	for i := 0; i < 3; i++ {
 		policiesRecordedString := e.kubectl("get", resource,
 			"-l", "spo.x-k8s.io/recording-id="+mergeProfileRecordingName,
@@ -163,14 +201,17 @@ func (e *e2e) profileMergingTest(
 			continue
 		}
 	}
-	e.Len(policiesRecorded, 1)
-	e.Equal(mergedProfileName, policiesRecorded[0])
-
-	// the result should contain the triggered action
-	mergedProfile := e.retryGetProfile(resource, mergedProfileName)
+	e.Len(policiesRecorded, 2)
+	mergedProfileNginx := fmt.Sprintf("%s-%s", mergeProfileRecordingName, containerNameNginx)
+	mergedProfileRedis := fmt.Sprintf("%s-%s", mergeProfileRecordingName, containerNameRedis)
+	e.Contains(policiesRecorded, mergedProfileNginx)
+	e.Contains(policiesRecorded, mergedProfileRedis)
+
+	// the result for the nginx container should contain the triggered action
+	mergedProfile := e.retryGetProfile(resource, mergedProfileNginx)
 	e.Contains(mergedProfile, triggeredAction)
 	e.Contains(mergedProfile, commonAction)
-	e.kubectl("delete", resource, mergedProfileName)
+	e.kubectl("delete", resource, mergedProfileNginx, mergedProfileRedis)
 }
 
 type profileRecTmplMetadata struct {

diff --git a/test/tc_profilerecordings_test.go b/test/tc_profilerecordings_test.go
@@ -396,13 +396,6 @@ func (e *e2e) profileRecordingSelinuxDeployment(
 }
 
 func (e *e2e) createRecordingTestDeployment() (since time.Time, deployName string) {
-	e.logf("Creating test deployment")
-	deployName = "my-deployment"
-
-	e.setupRecordingSa(e.getCurrentContextNamespace(defaultNamespace))
-
-	e.setupRecordingSa(e.getCurrentContextNamespace(defaultNamespace))
-
 	const testDeployment = `
 apiVersion: apps/v1
 kind: Deployment
@@ -430,10 +423,18 @@ spec:
           initialDelaySeconds: 5
           periodSeconds: 5
 `
+	return e.createRecordingTestDeploymentFromManifest(testDeployment)
+}
+
+func (e *e2e) createRecordingTestDeploymentFromManifest(manifest string) (since time.Time, deployName string) {
+	e.logf("Creating test deployment")
+	deployName = "my-deployment"
+
+	e.setupRecordingSa(e.getCurrentContextNamespace(defaultNamespace))
 
 	testFile, err := os.CreateTemp("", "recording-deployment*.yaml")
 	e.Nil(err)
-	_, err = testFile.WriteString(testDeployment)
+	_, err = testFile.WriteString(manifest)
 	e.Nil(err)
 	err = testFile.Close()
 	e.Nil(err)