From 93e7b4b12d9ffc0ae1e7e37aa447b20cabb590b9 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 23 Aug 2022 10:53:59 +0200 Subject: [PATCH 1/7] e2e: use readinessProbe to make sure we record containers when they're actually ready If no readiness probe is used, then we might end up recording the container before it is truly ready and the policy might not contain all the calls we need. --- test/tc_bpf_recorder_test.go | 7 +++++++ test/tc_profilerecordings_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/test/tc_bpf_recorder_test.go b/test/tc_bpf_recorder_test.go index a11f912495..c5120d7daa 100644 --- a/test/tc_bpf_recorder_test.go +++ b/test/tc_bpf_recorder_test.go @@ -149,6 +149,13 @@ spec: containers: - name: nginx image: quay.io/security-profiles-operator/test-nginx-unprivileged:1.21 + ports: + - containerPort: 8080 + readinessProbe: + tcpSocket: + port: 8080 + initialDelaySeconds: 5 + periodSeconds: 5 ` testFile, err := os.CreateTemp("", "recording-deployment*.yaml") e.Nil(err) diff --git a/test/tc_profilerecordings_test.go b/test/tc_profilerecordings_test.go index c49756c584..94cc4057db 100644 --- a/test/tc_profilerecordings_test.go +++ b/test/tc_profilerecordings_test.go @@ -352,6 +352,13 @@ spec: containers: - name: nginx image: quay.io/security-profiles-operator/test-nginx-unprivileged:1.21 + ports: + - containerPort: 8080 + readinessProbe: + tcpSocket: + port: 8080 + initialDelaySeconds: 5 + periodSeconds: 5 ` testFile, err := os.CreateTemp("", "recording-deployment*.yaml") @@ -404,6 +411,13 @@ spec: containers: - image: quay.io/security-profiles-operator/test-nginx-unprivileged:1.21 name: nginx + ports: + - containerPort: 8080 + readinessProbe: + tcpSocket: + port: 8080 + initialDelaySeconds: 5 + periodSeconds: 5 restartPolicy: Never ` testPodFile, err := os.CreateTemp("", "recording-pod*.yaml") @@ -439,8 +453,18 @@ spec: containers: - name: nginx image: quay.io/security-profiles-operator/test-nginx-unprivileged:1.21 + readinessProbe: + tcpSocket: + port: 8080 + initialDelaySeconds: 5 + periodSeconds: 5 - name: redis image: quay.io/security-profiles-operator/redis:6.2.1 + readinessProbe: + tcpSocket: + port: 6379 + initialDelaySeconds: 5 + periodSeconds: 5 restartPolicy: Never ` testPodFile, err := os.CreateTemp("", "recording-pod*.yaml") From 80669e6d8f62c4cdb64ada3ccd9704e727ebaba2 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 9 Aug 2022 14:21:07 +0200 Subject: [PATCH 2/7] webhooks: Fix setting finalizers Only one call to set the status was used to set both finalizers and the status, which doesn't work. Let's use a separate call for setting the status and a separate call for setting the finalizer. --- internal/pkg/webhooks/binding/impl.go | 2 +- internal/pkg/webhooks/recording/impl.go | 12 ++- internal/pkg/webhooks/recording/recording.go | 4 +- .../recording/recordingfakes/fake_impl.go | 80 +++++++++++++++++++ internal/pkg/webhooks/utils/utils.go | 20 ++++- internal/pkg/webhooks/utils/utils_test.go | 33 ++++++++ 6 files changed, 146 insertions(+), 5 deletions(-) diff --git a/internal/pkg/webhooks/binding/impl.go b/internal/pkg/webhooks/binding/impl.go index fabd48c9a6..dcb52bbcf1 100644 --- a/internal/pkg/webhooks/binding/impl.go +++ b/internal/pkg/webhooks/binding/impl.go @@ -74,7 +74,7 @@ func (d *defaultImpl) UpdateResourceStatus( object client.Object, name string, ) error { - return utils.UpdateResource(ctx, logger, d.client.Status(), object, name) + return utils.UpdateResourceStatus(ctx, logger, d.client.Status(), object, name) } func (d *defaultImpl) SetDecoder(decoder *admission.Decoder) { diff --git a/internal/pkg/webhooks/recording/impl.go b/internal/pkg/webhooks/recording/impl.go index bec30de220..9d0afd6145 100644 --- a/internal/pkg/webhooks/recording/impl.go +++ b/internal/pkg/webhooks/recording/impl.go @@ -42,6 +42,7 @@ type defaultImpl struct { type impl interface { ListProfileRecordings(context.Context, ...client.ListOption) (*v1alpha1.ProfileRecordingList, error) UpdateResource(context.Context, logr.Logger, client.Object, string) error + UpdateResourceStatus(context.Context, logr.Logger, client.Object, string) error SetDecoder(*admission.Decoder) DecodePod(admission.Request) (*corev1.Pod, error) LabelSelectorAsSelector(*metav1.LabelSelector) (labels.Selector, error) @@ -64,7 +65,16 @@ func (d *defaultImpl) UpdateResource( object client.Object, name string, ) error { - return utils.UpdateResource(ctx, logger, d.client.Status(), object, name) + return utils.UpdateResource(ctx, logger, d.client, object, name) +} + +func (d *defaultImpl) UpdateResourceStatus( + ctx context.Context, + logger logr.Logger, + object client.Object, + name string, +) error { + return utils.UpdateResourceStatus(ctx, logger, d.client.Status(), object, name) } func (d *defaultImpl) SetDecoder(decoder *admission.Decoder) { diff --git a/internal/pkg/webhooks/recording/recording.go b/internal/pkg/webhooks/recording/recording.go index 2755678b5b..8f17155206 100644 --- a/internal/pkg/webhooks/recording/recording.go +++ b/internal/pkg/webhooks/recording/recording.go @@ -309,7 +309,7 @@ func (p *podSeccompRecorder) addPod( profileRecording.Status.ActiveWorkloads, podName, ) - if err := p.impl.UpdateResource( + if err := p.impl.UpdateResourceStatus( ctx, p.log, profileRecording, "profilerecording status", ); err != nil { return fmt.Errorf("update resource on adding pod: %w", err) @@ -335,7 +335,7 @@ func (p *podSeccompRecorder) removePod( } profileRecording.Status.ActiveWorkloads = newActiveWorkloads - if err := p.impl.UpdateResource( + if err := p.impl.UpdateResourceStatus( ctx, p.log, profileRecording, "profilerecording status", ); err != nil { return fmt.Errorf("update resource on removing pod: %w", err) diff --git a/internal/pkg/webhooks/recording/recordingfakes/fake_impl.go b/internal/pkg/webhooks/recording/recordingfakes/fake_impl.go index 93d145f253..65d8fa4bb2 100644 --- a/internal/pkg/webhooks/recording/recordingfakes/fake_impl.go +++ b/internal/pkg/webhooks/recording/recordingfakes/fake_impl.go @@ -100,6 +100,20 @@ type FakeImpl struct { updateResourceReturnsOnCall map[int]struct { result1 error } + UpdateResourceStatusStub func(context.Context, logr.Logger, client.Object, string) error + updateResourceStatusMutex sync.RWMutex + updateResourceStatusArgsForCall []struct { + arg1 context.Context + arg2 logr.Logger + arg3 client.Object + arg4 string + } + updateResourceStatusReturns struct { + result1 error + } + updateResourceStatusReturnsOnCall map[int]struct { + result1 error + } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } @@ -446,6 +460,70 @@ func (fake *FakeImpl) UpdateResourceReturnsOnCall(i int, result1 error) { }{result1} } +func (fake *FakeImpl) UpdateResourceStatus(arg1 context.Context, arg2 logr.Logger, arg3 client.Object, arg4 string) error { + fake.updateResourceStatusMutex.Lock() + ret, specificReturn := fake.updateResourceStatusReturnsOnCall[len(fake.updateResourceStatusArgsForCall)] + fake.updateResourceStatusArgsForCall = append(fake.updateResourceStatusArgsForCall, struct { + arg1 context.Context + arg2 logr.Logger + arg3 client.Object + arg4 string + }{arg1, arg2, arg3, arg4}) + stub := fake.UpdateResourceStatusStub + fakeReturns := fake.updateResourceStatusReturns + fake.recordInvocation("UpdateResourceStatus", []interface{}{arg1, arg2, arg3, arg4}) + fake.updateResourceStatusMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3, arg4) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeImpl) UpdateResourceStatusCallCount() int { + fake.updateResourceStatusMutex.RLock() + defer fake.updateResourceStatusMutex.RUnlock() + return len(fake.updateResourceStatusArgsForCall) +} + +func (fake *FakeImpl) UpdateResourceStatusCalls(stub func(context.Context, logr.Logger, client.Object, string) error) { + fake.updateResourceStatusMutex.Lock() + defer fake.updateResourceStatusMutex.Unlock() + fake.UpdateResourceStatusStub = stub +} + +func (fake *FakeImpl) UpdateResourceStatusArgsForCall(i int) (context.Context, logr.Logger, client.Object, string) { + fake.updateResourceStatusMutex.RLock() + defer fake.updateResourceStatusMutex.RUnlock() + argsForCall := fake.updateResourceStatusArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4 +} + +func (fake *FakeImpl) UpdateResourceStatusReturns(result1 error) { + fake.updateResourceStatusMutex.Lock() + defer fake.updateResourceStatusMutex.Unlock() + fake.UpdateResourceStatusStub = nil + fake.updateResourceStatusReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeImpl) UpdateResourceStatusReturnsOnCall(i int, result1 error) { + fake.updateResourceStatusMutex.Lock() + defer fake.updateResourceStatusMutex.Unlock() + fake.UpdateResourceStatusStub = nil + if fake.updateResourceStatusReturnsOnCall == nil { + fake.updateResourceStatusReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.updateResourceStatusReturnsOnCall[i] = struct { + result1 error + }{result1} +} + func (fake *FakeImpl) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() @@ -461,6 +539,8 @@ func (fake *FakeImpl) Invocations() map[string][][]interface{} { defer fake.setDecoderMutex.RUnlock() fake.updateResourceMutex.RLock() defer fake.updateResourceMutex.RUnlock() + fake.updateResourceStatusMutex.RLock() + defer fake.updateResourceStatusMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} for key, value := range fake.invocations { copiedInvocations[key] = value diff --git a/internal/pkg/webhooks/utils/utils.go b/internal/pkg/webhooks/utils/utils.go index 7e31adf876..0d531d3f9c 100644 --- a/internal/pkg/webhooks/utils/utils.go +++ b/internal/pkg/webhooks/utils/utils.go @@ -45,9 +45,27 @@ func RemoveIfExists(list []string, item string) []string { } // UpdateResource tries to update the provided object by using the -// client.StatusWriter. If the update fails, it automatically logs to the +// client.Writer. If the update fails, it automatically logs to the // provided logger. func UpdateResource( + ctx context.Context, + logger logr.Logger, + c client.Writer, + object client.Object, + name string, +) error { + if err := c.Update(ctx, object); err != nil { + msg := fmt.Sprintf("failed to update resource %s", name) + logger.Error(err, msg) + return fmt.Errorf("%s: %w", msg, err) + } + return nil +} + +// UpdateResourceStatus tries to update the provided object by using the +// client.StatusWriter. If the update fails, it automatically logs to the +// provided logger. +func UpdateResourceStatus( ctx context.Context, logger logr.Logger, c client.StatusWriter, diff --git a/internal/pkg/webhooks/utils/utils_test.go b/internal/pkg/webhooks/utils/utils_test.go index 6d304f1443..414562bd9e 100644 --- a/internal/pkg/webhooks/utils/utils_test.go +++ b/internal/pkg/webhooks/utils/utils_test.go @@ -91,6 +91,39 @@ type fakeClient struct { updateFails bool } +func (f *fakeClient) Create( + _ context.Context, + _ client.Object, + _ ...client.CreateOption, +) error { + if f.updateFails { + return errors.New("test") + } + return nil +} + +func (f *fakeClient) Delete( + _ context.Context, + _ client.Object, + _ ...client.DeleteOption, +) error { + if f.updateFails { + return errors.New("test") + } + return nil +} + +func (f *fakeClient) DeleteAllOf( + _ context.Context, + _ client.Object, + _ ...client.DeleteAllOfOption, +) error { + if f.updateFails { + return errors.New("test") + } + return nil +} + func (f *fakeClient) Update( ctx context.Context, _ client.Object, From 2e07e440e97b0bf85c3aad6221f5743f6350de7c Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 15 Aug 2022 16:10:26 +0200 Subject: [PATCH 3/7] webhooks: Make setting finalizers and status more resilient with retries Especially with multiple replicas, we've seen the webhooks error out due to a conflict. This is problematic because the webhooks have a hard-fail policy. Let's retry multiple times instead. --- internal/pkg/webhooks/recording/impl.go | 13 +++ internal/pkg/webhooks/recording/recording.go | 41 +++++++-- .../recording/recordingfakes/fake_impl.go | 83 +++++++++++++++++++ 3 files changed, 131 insertions(+), 6 deletions(-) diff --git a/internal/pkg/webhooks/recording/impl.go b/internal/pkg/webhooks/recording/impl.go index 9d0afd6145..fc8863babf 100644 --- a/internal/pkg/webhooks/recording/impl.go +++ b/internal/pkg/webhooks/recording/impl.go @@ -19,6 +19,7 @@ package recording import ( "context" "fmt" + "k8s.io/apimachinery/pkg/types" "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" @@ -40,6 +41,7 @@ type defaultImpl struct { //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate //counterfeiter:generate . impl type impl interface { + GetProfileRecording(ctx context.Context, name, namespace string) (*v1alpha1.ProfileRecording, error) ListProfileRecordings(context.Context, ...client.ListOption) (*v1alpha1.ProfileRecordingList, error) UpdateResource(context.Context, logr.Logger, client.Object, string) error UpdateResourceStatus(context.Context, logr.Logger, client.Object, string) error @@ -49,6 +51,17 @@ type impl interface { GetOperatorNamespace() string } +func (d *defaultImpl) GetProfileRecording( + ctx context.Context, name, namespace string, +) (*v1alpha1.ProfileRecording, error) { + profileRecording := &v1alpha1.ProfileRecording{} + prName := types.NamespacedName{Name: name, Namespace: namespace} + if err := d.client.Get(ctx, prName, profileRecording); err != nil { + return nil, fmt.Errorf("get profile recording: %w", err) + } + return profileRecording, nil +} + func (d *defaultImpl) ListProfileRecordings( ctx context.Context, opts ...client.ListOption, ) (*v1alpha1.ProfileRecordingList, error) { diff --git a/internal/pkg/webhooks/recording/recording.go b/internal/pkg/webhooks/recording/recording.go index 8f17155206..c06838145d 100644 --- a/internal/pkg/webhooks/recording/recording.go +++ b/internal/pkg/webhooks/recording/recording.go @@ -28,6 +28,7 @@ import ( "github.com/go-logr/logr" admissionv1 "k8s.io/api/admission/v1" corev1 "k8s.io/api/core/v1" + kerrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/labels" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -119,21 +120,32 @@ func (p *podSeccompRecorder) Handle( if req.Operation == admissionv1.Delete { p.cleanupReplicas(podName) - if err := p.removePod(ctx, podName, &item); err != nil { + if err := util.Retry(func() error { + if err := p.removePod(ctx, podName, item.Name, item.Namespace); err != nil { + return fmt.Errorf("removing pod tracking: %w", err) + } + return nil + }, kerrors.IsConflict); err != nil { return admission.Errored(http.StatusInternalServerError, err) } continue } if selector.Matches(podLabels) { - podChanged, err = p.updatePod(pod, &item) + podChanged, err = p.updatePod(pod, podName, &item) if err != nil { return admission.Errored(http.StatusInternalServerError, err) } } if podChanged { - if err := p.addPod(ctx, podName, &item); err != nil { + if err := util.Retry(func() error { + if err := p.addPod(ctx, podName, item.Name, item.Namespace); err != nil { + return fmt.Errorf("adding pod tracking: %w", err) + } + + return nil + }, kerrors.IsConflict); err != nil { return admission.Errored(http.StatusInternalServerError, err) } } @@ -164,6 +176,7 @@ func (p *podSeccompRecorder) shouldRecordContainer(containerName string, func (p *podSeccompRecorder) updatePod( pod *corev1.Pod, + podName string, profileRecording *profilerecordingv1alpha1.ProfileRecording, ) (podChanged bool, err error) { // Collect containers as references to not copy them during modification @@ -219,7 +232,7 @@ func (p *podSeccompRecorder) updatePod( errors.New("existing annotation"), fmt.Sprintf( "workload %s already has annotation %s (not mutating to %s).", - pod.Name, + podName, existingValue, value, ), @@ -303,8 +316,16 @@ func (p *podSeccompRecorder) cleanupReplicas(podName string) { func (p *podSeccompRecorder) addPod( ctx context.Context, podName string, - profileRecording *profilerecordingv1alpha1.ProfileRecording, + recordingName string, + namespace string, ) error { + // we Get the recording again because remove is used in a retry loop + // to handle conflicts, we want to get the most recent one + profileRecording, err := p.impl.GetProfileRecording(ctx, recordingName, namespace) + if err != nil { + return fmt.Errorf("cannot retrieve profilerecording: %w", err) + } + profileRecording.Status.ActiveWorkloads = utils.AppendIfNotExists( profileRecording.Status.ActiveWorkloads, podName, ) @@ -325,8 +346,16 @@ func (p *podSeccompRecorder) addPod( func (p *podSeccompRecorder) removePod( ctx context.Context, podName string, - profileRecording *profilerecordingv1alpha1.ProfileRecording, + recordingName string, + namespace string, ) error { + // we Get the recording again because remove is used in a retry loop + // to handle conflicts, we want to get the most recent one + profileRecording, err := p.impl.GetProfileRecording(ctx, recordingName, namespace) + if err != nil { + return fmt.Errorf("cannot retrieve profilerecording: %w", err) + } + newActiveWorkloads := []string{} for _, activeWorkload := range profileRecording.Status.ActiveWorkloads { if !strings.HasPrefix(podName, activeWorkload) { diff --git a/internal/pkg/webhooks/recording/recordingfakes/fake_impl.go b/internal/pkg/webhooks/recording/recordingfakes/fake_impl.go index 65d8fa4bb2..26b8482c7d 100644 --- a/internal/pkg/webhooks/recording/recordingfakes/fake_impl.go +++ b/internal/pkg/webhooks/recording/recordingfakes/fake_impl.go @@ -54,6 +54,21 @@ type FakeImpl struct { getOperatorNamespaceReturnsOnCall map[int]struct { result1 string } + GetProfileRecordingStub func(context.Context, string, string) (*v1alpha1.ProfileRecording, error) + getProfileRecordingMutex sync.RWMutex + getProfileRecordingArgsForCall []struct { + arg1 context.Context + arg2 string + arg3 string + } + getProfileRecordingReturns struct { + result1 *v1alpha1.ProfileRecording + result2 error + } + getProfileRecordingReturnsOnCall map[int]struct { + result1 *v1alpha1.ProfileRecording + result2 error + } LabelSelectorAsSelectorStub func(*v1a.LabelSelector) (labels.Selector, error) labelSelectorAsSelectorMutex sync.RWMutex labelSelectorAsSelectorArgsForCall []struct { @@ -235,6 +250,72 @@ func (fake *FakeImpl) GetOperatorNamespaceReturnsOnCall(i int, result1 string) { }{result1} } +func (fake *FakeImpl) GetProfileRecording(arg1 context.Context, arg2 string, arg3 string) (*v1alpha1.ProfileRecording, error) { + fake.getProfileRecordingMutex.Lock() + ret, specificReturn := fake.getProfileRecordingReturnsOnCall[len(fake.getProfileRecordingArgsForCall)] + fake.getProfileRecordingArgsForCall = append(fake.getProfileRecordingArgsForCall, struct { + arg1 context.Context + arg2 string + arg3 string + }{arg1, arg2, arg3}) + stub := fake.GetProfileRecordingStub + fakeReturns := fake.getProfileRecordingReturns + fake.recordInvocation("GetProfileRecording", []interface{}{arg1, arg2, arg3}) + fake.getProfileRecordingMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *FakeImpl) GetProfileRecordingCallCount() int { + fake.getProfileRecordingMutex.RLock() + defer fake.getProfileRecordingMutex.RUnlock() + return len(fake.getProfileRecordingArgsForCall) +} + +func (fake *FakeImpl) GetProfileRecordingCalls(stub func(context.Context, string, string) (*v1alpha1.ProfileRecording, error)) { + fake.getProfileRecordingMutex.Lock() + defer fake.getProfileRecordingMutex.Unlock() + fake.GetProfileRecordingStub = stub +} + +func (fake *FakeImpl) GetProfileRecordingArgsForCall(i int) (context.Context, string, string) { + fake.getProfileRecordingMutex.RLock() + defer fake.getProfileRecordingMutex.RUnlock() + argsForCall := fake.getProfileRecordingArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 +} + +func (fake *FakeImpl) GetProfileRecordingReturns(result1 *v1alpha1.ProfileRecording, result2 error) { + fake.getProfileRecordingMutex.Lock() + defer fake.getProfileRecordingMutex.Unlock() + fake.GetProfileRecordingStub = nil + fake.getProfileRecordingReturns = struct { + result1 *v1alpha1.ProfileRecording + result2 error + }{result1, result2} +} + +func (fake *FakeImpl) GetProfileRecordingReturnsOnCall(i int, result1 *v1alpha1.ProfileRecording, result2 error) { + fake.getProfileRecordingMutex.Lock() + defer fake.getProfileRecordingMutex.Unlock() + fake.GetProfileRecordingStub = nil + if fake.getProfileRecordingReturnsOnCall == nil { + fake.getProfileRecordingReturnsOnCall = make(map[int]struct { + result1 *v1alpha1.ProfileRecording + result2 error + }) + } + fake.getProfileRecordingReturnsOnCall[i] = struct { + result1 *v1alpha1.ProfileRecording + result2 error + }{result1, result2} +} + func (fake *FakeImpl) LabelSelectorAsSelector(arg1 *v1a.LabelSelector) (labels.Selector, error) { fake.labelSelectorAsSelectorMutex.Lock() ret, specificReturn := fake.labelSelectorAsSelectorReturnsOnCall[len(fake.labelSelectorAsSelectorArgsForCall)] @@ -531,6 +612,8 @@ func (fake *FakeImpl) Invocations() map[string][][]interface{} { defer fake.decodePodMutex.RUnlock() fake.getOperatorNamespaceMutex.RLock() defer fake.getOperatorNamespaceMutex.RUnlock() + fake.getProfileRecordingMutex.RLock() + defer fake.getProfileRecordingMutex.RUnlock() fake.labelSelectorAsSelectorMutex.RLock() defer fake.labelSelectorAsSelectorMutex.RUnlock() fake.listProfileRecordingsMutex.RLock() From 5012e6e3c8cbdb5a4cff354ec084c61cc209b342 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Sun, 21 Aug 2022 16:37:36 +0200 Subject: [PATCH 4/7] e2e: test that recording finalizer prevents deleting the resource Adds a test for the finalizer. --- test/e2e_flaky_test.go | 1 + test/tc_profilerecordings_test.go | 57 +++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/test/e2e_flaky_test.go b/test/e2e_flaky_test.go index 2ab1b764a6..6931718c99 100644 --- a/test/e2e_flaky_test.go +++ b/test/e2e_flaky_test.go @@ -66,6 +66,7 @@ func (e *e2e) TestSecurityProfilesOperator_Flaky() { e.testCaseProfileRecordingMultiContainerLogs() e.testCaseProfileRecordingSpecificContainerLogs() e.testCaseProfileRecordingDeploymentLogs() + e.testCaseRecordingFinalizers() }) e.Run("cluster-wide: Seccomp: Verify profile recording bpf", func() { diff --git a/test/tc_profilerecordings_test.go b/test/tc_profilerecordings_test.go index 94cc4057db..0f2a96c275 100644 --- a/test/tc_profilerecordings_test.go +++ b/test/tc_profilerecordings_test.go @@ -20,6 +20,7 @@ import ( "fmt" "os" "regexp" + spoutil "sigs.k8s.io/security-profiles-operator/internal/pkg/util" "time" ) @@ -276,6 +277,62 @@ func (e *e2e) testCaseProfileRecordingSelinuxDeploymentLogs() { ) } +func (e *e2e) testCaseRecordingFinalizers() { + e.logEnricherOnlyTestCase() + + const recordingName = "test-recording" + + e.logf("Creating recording for static pod test") + e.kubectl("create", "-f", exampleRecordingSeccompLogsPath) + + since, podName := e.createRecordingTestPod() + e.waitForEnricherLogs(since, regexp.MustCompile(`(?m)"syscallName"="listen"`)) + + // Check that the recording's status contains the resource. Retry to avoid + // test races. + e.logf("Testing that profile binding has pod reference") + if err := spoutil.Retry(func() error { + output := e.kubectl("get", "profilerecording", recordingName, "--output", "jsonpath={.status.activeWorkloads[0]}") + fmt.Println(output) + if output != podName { + return fmt.Errorf("pod name %s not found in status", podName) + } + return nil + }, func(err error) bool { + return true + }); err != nil { + e.Fail("failed to find pod name in status") + } + + // Check that the recording's finalizer is present. Don't retry anymore, the finalizer + // must be added at this point + output := e.kubectl("get", "profilerecording", recordingName, "--output", "jsonpath={.metadata.finalizers[0]}") + e.Equal("active-seccomp-profile-recording-lock", output) + + // Attempt to delete the resource, should not be possible + e.kubectl("delete", "--wait=false", "profilerecording", recordingName) + output = e.kubectl("get", "profilerecording", recordingName, "--output", "jsonpath={.metadata.deletionTimestamp}") + e.NotEmpty(output) + + isDeleted := make(chan bool) + go func() { + e.waitFor("delete", "profilerecording", recordingName) + isDeleted <- true + }() + + // Delete the pod and check that the resource is removed + e.kubectl("delete", "pod", podName) + + // Wait a bit for the recording to be actually deleted + <-isDeleted + + resourceName := recordingName + "-nginx" + profile := e.retryGetSeccompProfile(resourceName) + e.Contains(profile, "listen") + + e.kubectl("delete", "sp", resourceName) +} + func (e *e2e) profileRecordingDeployment( recording string, waitConditions ...*regexp.Regexp, ) { From 2b4b0450073ec4519ed80982d1e8b622d00ad5fb Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 22 Aug 2022 13:52:06 +0200 Subject: [PATCH 5/7] recording webhook: don't use the replica name to track pods being recorded, use labels instead The recording webhook used to track container replicas in a sync.Map, but its usage was problematic because: - all replicas were deleted in case any of replicas with the same generatedName were used. This meant replica numbers used for recorded policies were being reused and not all containers would end up having a recorded policy - if a replica was removed, the profilerecording would lose the status and finalizers - using the replica tracking in the sync.Map means that the webhook is maintaining its state, which it shouldn't. While this patch doesn't remove the state completely it reduces its usage. Instead of the above, this patch maintains the finalizer and the status as a combination of examining the pod being admitted as well as the currently existing pods which are listed during admission. The replica number is then always increasing by one e.g. across scaleups or scaledowns of replicated pods. Finally, because the webhook is watching all namespaces, but the sync.Map was only storing plain names, let's also add namespace as a prefix for the syncMap. This allows to empty the sync.Map once no pods for a single recording exist, keeping the memory usage low over time. --- ...c.authorization.k8s.io_v1_clusterrole.yaml | 8 + deploy/base/role.yaml | 8 + deploy/helm/templates/static-resources.yaml | 8 + deploy/namespace-operator.yaml | 8 + deploy/openshift-dev.yaml | 8 + deploy/operator.yaml | 8 + deploy/webhook-operator.yaml | 8 + internal/pkg/webhooks/recording/impl.go | 27 ++- internal/pkg/webhooks/recording/recording.go | 199 ++++++++++++------ .../pkg/webhooks/recording/recording_test.go | 177 ++++++++++++++++ .../recording/recordingfakes/fake_impl.go | 83 ++++++++ test/e2e_flaky_test.go | 1 + test/tc_profilerecordings_test.go | 45 +++- 13 files changed, 516 insertions(+), 72 deletions(-) diff --git a/bundle/manifests/spo-webhook_rbac.authorization.k8s.io_v1_clusterrole.yaml b/bundle/manifests/spo-webhook_rbac.authorization.k8s.io_v1_clusterrole.yaml index 887c9aa133..93037b45ad 100644 --- a/bundle/manifests/spo-webhook_rbac.authorization.k8s.io_v1_clusterrole.yaml +++ b/bundle/manifests/spo-webhook_rbac.authorization.k8s.io_v1_clusterrole.yaml @@ -12,6 +12,14 @@ rules: - events verbs: - create +- apiGroups: + - "" + resources: + - pods + verbs: + - get + - list + - watch - apiGroups: - security-profiles-operator.x-k8s.io resources: diff --git a/deploy/base/role.yaml b/deploy/base/role.yaml index 3d635af30c..2ad73ae44c 100644 --- a/deploy/base/role.yaml +++ b/deploy/base/role.yaml @@ -448,6 +448,14 @@ rules: - events verbs: - create +- apiGroups: + - "" + resources: + - pods + verbs: + - get + - list + - watch - apiGroups: - security-profiles-operator.x-k8s.io resources: diff --git a/deploy/helm/templates/static-resources.yaml b/deploy/helm/templates/static-resources.yaml index 6853851851..15cdec8267 100644 --- a/deploy/helm/templates/static-resources.yaml +++ b/deploy/helm/templates/static-resources.yaml @@ -492,6 +492,14 @@ rules: - events verbs: - create +- apiGroups: + - "" + resources: + - pods + verbs: + - get + - list + - watch - apiGroups: - security-profiles-operator.x-k8s.io resources: diff --git a/deploy/namespace-operator.yaml b/deploy/namespace-operator.yaml index c983135b6a..d3c2a2ec16 100644 --- a/deploy/namespace-operator.yaml +++ b/deploy/namespace-operator.yaml @@ -1570,6 +1570,14 @@ rules: - events verbs: - create +- apiGroups: + - "" + resources: + - pods + verbs: + - get + - list + - watch - apiGroups: - security-profiles-operator.x-k8s.io resources: diff --git a/deploy/openshift-dev.yaml b/deploy/openshift-dev.yaml index 0e746cd631..712b725d33 100644 --- a/deploy/openshift-dev.yaml +++ b/deploy/openshift-dev.yaml @@ -1570,6 +1570,14 @@ rules: - events verbs: - create +- apiGroups: + - "" + resources: + - pods + verbs: + - get + - list + - watch - apiGroups: - security-profiles-operator.x-k8s.io resources: diff --git a/deploy/operator.yaml b/deploy/operator.yaml index 0c9a8b1172..aa3043ae8d 100644 --- a/deploy/operator.yaml +++ b/deploy/operator.yaml @@ -1570,6 +1570,14 @@ rules: - events verbs: - create +- apiGroups: + - "" + resources: + - pods + verbs: + - get + - list + - watch - apiGroups: - security-profiles-operator.x-k8s.io resources: diff --git a/deploy/webhook-operator.yaml b/deploy/webhook-operator.yaml index 3e2626db09..fadeeaa33c 100644 --- a/deploy/webhook-operator.yaml +++ b/deploy/webhook-operator.yaml @@ -1424,6 +1424,14 @@ rules: - events verbs: - create +- apiGroups: + - "" + resources: + - pods + verbs: + - get + - list + - watch - apiGroups: - security-profiles-operator.x-k8s.io resources: diff --git a/internal/pkg/webhooks/recording/impl.go b/internal/pkg/webhooks/recording/impl.go index fc8863babf..99744d85e9 100644 --- a/internal/pkg/webhooks/recording/impl.go +++ b/internal/pkg/webhooks/recording/impl.go @@ -19,12 +19,12 @@ package recording import ( "context" "fmt" - "k8s.io/apimachinery/pkg/types" "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -43,6 +43,7 @@ type defaultImpl struct { type impl interface { GetProfileRecording(ctx context.Context, name, namespace string) (*v1alpha1.ProfileRecording, error) ListProfileRecordings(context.Context, ...client.ListOption) (*v1alpha1.ProfileRecordingList, error) + ListRecordedPods(ctx context.Context, inNs string, selector *metav1.LabelSelector) (*corev1.PodList, error) UpdateResource(context.Context, logr.Logger, client.Object, string) error UpdateResourceStatus(context.Context, logr.Logger, client.Object, string) error SetDecoder(*admission.Decoder) @@ -72,6 +73,30 @@ func (d *defaultImpl) ListProfileRecordings( return profileRecordings, nil } +func (d *defaultImpl) ListRecordedPods( + ctx context.Context, + inNs string, + selector *metav1.LabelSelector, +) (*corev1.PodList, error) { + podList := &corev1.PodList{} + + labelSelector, err := metav1.LabelSelectorAsSelector(selector) + if err != nil { + return nil, fmt.Errorf("get profile recording: %w", err) + } + + opts := client.ListOptions{ + LabelSelector: labelSelector, + Namespace: inNs, + } + + if err := d.client.List(ctx, podList, &opts); err != nil { + return nil, fmt.Errorf("list recorded pods: %w", err) + } + + return podList, nil +} + func (d *defaultImpl) UpdateResource( ctx context.Context, logger logr.Logger, diff --git a/internal/pkg/webhooks/recording/recording.go b/internal/pkg/webhooks/recording/recording.go index c06838145d..6317c46b85 100644 --- a/internal/pkg/webhooks/recording/recording.go +++ b/internal/pkg/webhooks/recording/recording.go @@ -67,6 +67,7 @@ func RegisterWebhook(server *webhook.Server, c client.Client) { // +kubebuilder:rbac:groups=security-profiles-operator.x-k8s.io,resources=profilerecordings,verbs=get;list;watch;create;update;patch // +kubebuilder:rbac:groups=security-profiles-operator.x-k8s.io,resources=profilerecordings/status,verbs=get;update;patch // +kubebuilder:rbac:groups=security-profiles-operator.x-k8s.io,resources=profilerecordings/finalizers,verbs=delete;get;update;patch +// +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch //nolint:gocritic func (p *podSeccompRecorder) Handle( @@ -118,17 +119,25 @@ func (p *podSeccompRecorder) Handle( return admission.Errored(http.StatusInternalServerError, err) } - if req.Operation == admissionv1.Delete { - p.cleanupReplicas(podName) - if err := util.Retry(func() error { - if err := p.removePod(ctx, podName, item.Name, item.Namespace); err != nil { - return fmt.Errorf("removing pod tracking: %w", err) - } - return nil - }, kerrors.IsConflict); err != nil { - return admission.Errored(http.StatusInternalServerError, err) + recordedPods, err := p.impl.ListRecordedPods(ctx, req.Namespace, &item.Spec.PodSelector) + if err != nil { + p.log.Error( + err, "Could not list pods for this recording", + ) + return admission.Errored(http.StatusInternalServerError, err) + } + + if err := util.Retry(func() error { + if err := p.setRecordingReferences(ctx, req.Operation, + &item, selector, + podName, podLabels, + recordedPods); err != nil { + return fmt.Errorf("adding pod tracking: %w", err) } - continue + + return nil + }, kerrors.IsConflict); err != nil { + return admission.Errored(http.StatusInternalServerError, err) } if selector.Matches(podLabels) { @@ -137,18 +146,6 @@ func (p *podSeccompRecorder) Handle( return admission.Errored(http.StatusInternalServerError, err) } } - - if podChanged { - if err := util.Retry(func() error { - if err := p.addPod(ctx, podName, item.Name, item.Namespace); err != nil { - return fmt.Errorf("adding pod tracking: %w", err) - } - - return nil - }, kerrors.IsConflict); err != nil { - return admission.Errored(http.StatusInternalServerError, err) - } - } } if !podChanged { @@ -193,15 +190,9 @@ func (p *podSeccompRecorder) updatePod( } // Handle replicas by tracking them - replica := "" - if pod.Name == "" && pod.GenerateName != "" { - v, _ := p.replicas.LoadOrStore(pod.GenerateName, uint(0)) - replica = fmt.Sprintf("-%d", v) - vUint, ok := v.(uint) - if !ok { - return false, errors.New("replicas value is not an uint") - } - p.replicas.Store(pod.GenerateName, vUint+1) + replica, err := p.getReplica(pod, profileRecording) + if err != nil { + return false, err } for i := range ctrs { @@ -299,13 +290,19 @@ func (p *podSeccompRecorder) updateSelinuxSecurityContext( ctr.SecurityContext.SELinuxOptions.Type = config.SelinuxPermissiveProfile } -func (p *podSeccompRecorder) cleanupReplicas(podName string) { +func replicaKey(recordingName, podName string) string { + return fmt.Sprintf("%s/%s", recordingName, podName) +} + +func (p *podSeccompRecorder) cleanupReplicas(recordingName, podName string) { + rKey := replicaKey(recordingName, podName) + p.replicas.Range(func(key, _ interface{}) bool { keyString, ok := key.(string) if !ok { return false } - if strings.HasPrefix(podName, keyString) { + if strings.HasPrefix(rKey, keyString) { p.replicas.Delete(key) return false } @@ -313,69 +310,131 @@ func (p *podSeccompRecorder) cleanupReplicas(podName string) { }) } -func (p *podSeccompRecorder) addPod( +func (p *podSeccompRecorder) getReplica( + pod *corev1.Pod, + profileRecording *profilerecordingv1alpha1.ProfileRecording, +) (string, error) { + replica := "" + if pod.Name == "" && pod.GenerateName != "" { + rKey := replicaKey(profileRecording.Name, pod.GenerateName) + + v, _ := p.replicas.LoadOrStore(rKey, uint(0)) + replica = fmt.Sprintf("-%d", v) + vUint, ok := v.(uint) + if !ok { + return "", errors.New("replicas value is not an uint") + } + p.replicas.Store(rKey, vUint+1) + } + + return replica, nil +} + +func (p *podSeccompRecorder) setRecordingReferences( ctx context.Context, + op admissionv1.Operation, + profileRecording *profilerecordingv1alpha1.ProfileRecording, + selector labels.Selector, podName string, - recordingName string, - namespace string, + podLabels labels.Set, + recordedPods *corev1.PodList, ) error { // we Get the recording again because remove is used in a retry loop // to handle conflicts, we want to get the most recent one - profileRecording, err := p.impl.GetProfileRecording(ctx, recordingName, namespace) - if err != nil { + profileRecording, err := p.impl.GetProfileRecording(ctx, profileRecording.Name, profileRecording.Namespace) + if kerrors.IsNotFound(err) { + // this can happen if the profile recording is deleted while we're reconciling + // just return without doing anything + return nil + } else if err != nil { return fmt.Errorf("cannot retrieve profilerecording: %w", err) } - profileRecording.Status.ActiveWorkloads = utils.AppendIfNotExists( - profileRecording.Status.ActiveWorkloads, podName, - ) + if err := p.setActiveWorkloads(ctx, op, profileRecording, selector, podName, podLabels, recordedPods); err != nil { + return fmt.Errorf("cannot set active workloads: %w", err) + } - if err := p.impl.UpdateResourceStatus( - ctx, p.log, profileRecording, "profilerecording status", - ); err != nil { - return fmt.Errorf("update resource on adding pod: %w", err) + return p.setFinalizers(ctx, op, profileRecording, selector, podName, podLabels, recordedPods) +} + +func (p *podSeccompRecorder) setActiveWorkloads( + ctx context.Context, + op admissionv1.Operation, + profileRecording *profilerecordingv1alpha1.ProfileRecording, + selector labels.Selector, + podName string, + podLabels labels.Set, + recordedPods *corev1.PodList, +) error { + newActiveWorkloads := []string{} + + for _, pod := range recordedPods.Items { + newActiveWorkloads = append(newActiveWorkloads, pod.Name) } - if !controllerutil.ContainsFinalizer(profileRecording, finalizer) { - controllerutil.AddFinalizer(profileRecording, finalizer) + if op == admissionv1.Delete { + newActiveWorkloads = utils.RemoveIfExists(newActiveWorkloads, podName) + } else { + if selector.Matches(podLabels) { + newActiveWorkloads = utils.AppendIfNotExists(newActiveWorkloads, podName) + } } - return p.impl.UpdateResource(ctx, p.log, profileRecording, "profilerecording") + profileRecording.Status.ActiveWorkloads = newActiveWorkloads + + return p.impl.UpdateResourceStatus(ctx, p.log, profileRecording, "profilerecording status") } -func (p *podSeccompRecorder) removePod( +func (p *podSeccompRecorder) setFinalizers( ctx context.Context, + op admissionv1.Operation, + profileRecording *profilerecordingv1alpha1.ProfileRecording, + selector labels.Selector, podName string, - recordingName string, - namespace string, + podLabels labels.Set, + recordedPods *corev1.PodList, ) error { - // we Get the recording again because remove is used in a retry loop - // to handle conflicts, we want to get the most recent one - profileRecording, err := p.impl.GetProfileRecording(ctx, recordingName, namespace) - if err != nil { - return fmt.Errorf("cannot retrieve profilerecording: %w", err) + if anyPodMatch(op, selector, podLabels, podName, recordedPods) { + if !controllerutil.ContainsFinalizer(profileRecording, finalizer) { + controllerutil.AddFinalizer(profileRecording, finalizer) + } + } else { + if controllerutil.ContainsFinalizer(profileRecording, finalizer) { + controllerutil.RemoveFinalizer(profileRecording, finalizer) + p.cleanupReplicas(profileRecording.Name, podName) + } } - newActiveWorkloads := []string{} - for _, activeWorkload := range profileRecording.Status.ActiveWorkloads { - if !strings.HasPrefix(podName, activeWorkload) { - newActiveWorkloads = append(newActiveWorkloads, activeWorkload) + return p.impl.UpdateResource(ctx, p.log, profileRecording, "profilerecording") +} + +func anyPodMatch( + op admissionv1.Operation, + selector labels.Selector, + podLabels labels.Set, + podName string, + foundPods *corev1.PodList, +) bool { + if len(foundPods.Items) > 0 { + if op != admissionv1.Delete { + return true } - } - profileRecording.Status.ActiveWorkloads = newActiveWorkloads - if err := p.impl.UpdateResourceStatus( - ctx, p.log, profileRecording, "profilerecording status", - ); err != nil { - return fmt.Errorf("update resource on removing pod: %w", err) + for _, pod := range foundPods.Items { + if podName == pod.Name { + continue + } + // if we ever get here, then we are deleting but we encountered + // a pod different than the one we're deleting, so there are still pods + return true + } } - if len(profileRecording.Status.ActiveWorkloads) == 0 && - controllerutil.ContainsFinalizer(profileRecording, finalizer) { - controllerutil.RemoveFinalizer(profileRecording, finalizer) + if op != admissionv1.Delete && selector.Matches(podLabels) { + return true } - return p.impl.UpdateResource(ctx, p.log, profileRecording, "profilerecording") + return false } func (p *podSeccompRecorder) InjectDecoder(decoder *admission.Decoder) error { diff --git a/internal/pkg/webhooks/recording/recording_test.go b/internal/pkg/webhooks/recording/recording_test.go index ede968a0b1..c2e97847cf 100644 --- a/internal/pkg/webhooks/recording/recording_test.go +++ b/internal/pkg/webhooks/recording/recording_test.go @@ -27,9 +27,11 @@ import ( "github.com/stretchr/testify/require" admissionv1 "k8s.io/api/admission/v1" corev1 "k8s.io/api/core/v1" + kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "sigs.k8s.io/security-profiles-operator/api/profilerecording/v1alpha1" @@ -102,6 +104,19 @@ func TestHandle(t *testing.T) { }, }, }, nil) + mock.GetProfileRecordingReturns(&v1alpha1.ProfileRecording{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-little-profile-recording", + Namespace: "test-ns", + }, + Spec: v1alpha1.ProfileRecordingSpec{ + Kind: v1alpha1.ProfileRecordingKindSelinuxProfile, + Recorder: v1alpha1.ProfileRecorderLogs, + }, + }, nil) + mock.ListRecordedPodsReturns(&corev1.PodList{ + Items: []corev1.Pod{}, + }, nil) mock.GetOperatorNamespaceReturns("test-ns") mock.DecodePodReturns(testPod.DeepCopy(), nil) mock.LabelSelectorAsSelectorReturns(labels.Everything(), nil) @@ -134,6 +149,19 @@ func TestHandle(t *testing.T) { }, }, }, nil) + mock.GetProfileRecordingReturns(&v1alpha1.ProfileRecording{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-little-profile-recording", + Namespace: "test-ns", + }, + Spec: v1alpha1.ProfileRecordingSpec{ + Kind: v1alpha1.ProfileRecordingKindSelinuxProfile, + Recorder: v1alpha1.ProfileRecorderLogs, + }, + }, nil) + mock.ListRecordedPodsReturns(&corev1.PodList{ + Items: []corev1.Pod{}, + }, nil) mock.DecodePodReturns(testPod.DeepCopy(), nil) mock.LabelSelectorAsSelectorReturns(labels.Everything(), nil) }, @@ -164,6 +192,19 @@ func TestHandle(t *testing.T) { }, }, }, nil) + mock.GetProfileRecordingReturns(&v1alpha1.ProfileRecording{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-little-profile-recording", + Namespace: "test-ns", + }, + Spec: v1alpha1.ProfileRecordingSpec{ + Kind: v1alpha1.ProfileRecordingKindSelinuxProfile, + Recorder: v1alpha1.ProfileRecorderLogs, + }, + }, nil) + mock.ListRecordedPodsReturns(&corev1.PodList{ + Items: []corev1.Pod{}, + }, nil) mock.DecodePodReturns(testPod.DeepCopy(), nil) }, assert: func(resp admission.Response) { @@ -171,6 +212,45 @@ func TestHandle(t *testing.T) { require.Empty(t, resp.Patches) }, }, + { // success although GetProfile returns IsNotFound + prepare: func(mock *recordingfakes.FakeImpl) { + mock.ListProfileRecordingsReturns(&v1alpha1.ProfileRecordingList{ + Items: []v1alpha1.ProfileRecording{ + { + Spec: v1alpha1.ProfileRecordingSpec{ + Kind: v1alpha1.ProfileRecordingKindSeccompProfile, + Recorder: v1alpha1.ProfileRecorderHook, + }, + }, + }, + }, nil) + mock.GetProfileRecordingReturns(nil, + kerrors.NewNotFound( + schema.GroupResource{}, + "my-little-profile-recording"), + ) + mock.ListRecordedPodsReturns(&corev1.PodList{ + Items: []corev1.Pod{}, + }, nil) + mock.DecodePodReturns(testPod.DeepCopy(), nil) + mock.LabelSelectorAsSelectorReturns(labels.Everything(), nil) + }, + request: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Object: runtime.RawExtension{ + Raw: func() []byte { + b, err := json.Marshal(testPod.DeepCopy()) + require.Nil(t, err) + return b + }(), + }, + }, + }, + assert: func(resp admission.Response) { + require.True(t, resp.AdmissionResponse.Allowed) + require.Len(t, resp.Patches, 1) + }, + }, { // failure LabelSelectorAsSelector prepare: func(mock *recordingfakes.FakeImpl) { mock.ListProfileRecordingsReturns(&v1alpha1.ProfileRecordingList{ @@ -182,6 +262,9 @@ func TestHandle(t *testing.T) { }, }, }, nil) + mock.ListRecordedPodsReturns(&corev1.PodList{ + Items: []corev1.Pod{}, + }, nil) mock.DecodePodReturns(testPod.DeepCopy(), nil) mock.LabelSelectorAsSelectorReturns(nil, errTest) }, @@ -200,6 +283,19 @@ func TestHandle(t *testing.T) { }, }, }, nil) + mock.GetProfileRecordingReturns(&v1alpha1.ProfileRecording{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-little-profile-recording", + Namespace: "test-ns", + }, + Spec: v1alpha1.ProfileRecordingSpec{ + Kind: v1alpha1.ProfileRecordingKindSelinuxProfile, + Recorder: v1alpha1.ProfileRecorderLogs, + }, + }, nil) + mock.ListRecordedPodsReturns(&corev1.PodList{ + Items: []corev1.Pod{}, + }, nil) mock.DecodePodReturns(testPod.DeepCopy(), nil) mock.LabelSelectorAsSelectorReturns(labels.Everything(), nil) mock.UpdateResourceReturns(errTest) @@ -231,6 +327,19 @@ func TestHandle(t *testing.T) { }, }, }, nil) + mock.GetProfileRecordingReturns(&v1alpha1.ProfileRecording{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-little-profile-recording", + Namespace: "test-ns", + }, + Spec: v1alpha1.ProfileRecordingSpec{ + Kind: v1alpha1.ProfileRecordingKindSelinuxProfile, + Recorder: v1alpha1.ProfileRecorderLogs, + }, + }, nil) + mock.ListRecordedPodsReturns(&corev1.PodList{ + Items: []corev1.Pod{}, + }, nil) pod := testPod.DeepCopy() pod.Annotations = map[string]string{ "io.containers.trace-syscall/container": "of:/some/path.json", @@ -268,6 +377,19 @@ func TestHandle(t *testing.T) { }, }, }, nil) + mock.GetProfileRecordingReturns(&v1alpha1.ProfileRecording{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-little-profile-recording", + Namespace: "test-ns", + }, + Spec: v1alpha1.ProfileRecordingSpec{ + Kind: v1alpha1.ProfileRecordingKindSelinuxProfile, + Recorder: v1alpha1.ProfileRecorderLogs, + }, + }, nil) + mock.ListRecordedPodsReturns(&corev1.PodList{ + Items: []corev1.Pod{}, + }, nil) mock.DecodePodReturns(testPod.DeepCopy(), nil) mock.LabelSelectorAsSelectorReturns(labels.Everything(), nil) }, @@ -280,6 +402,7 @@ func TestHandle(t *testing.T) { require.True(t, resp.AdmissionResponse.Allowed) }, }, + //nolint: dupl // golint flags this as a dup of the below, but here we're testing failure of UpdateResource { // failure pod deleted on UpdateResource prepare: func(mock *recordingfakes.FakeImpl) { mock.ListProfileRecordingsReturns(&v1alpha1.ProfileRecordingList{ @@ -294,6 +417,19 @@ func TestHandle(t *testing.T) { }, }, }, nil) + mock.GetProfileRecordingReturns(&v1alpha1.ProfileRecording{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-little-profile-recording", + Namespace: "test-ns", + }, + Spec: v1alpha1.ProfileRecordingSpec{ + Kind: v1alpha1.ProfileRecordingKindSelinuxProfile, + Recorder: v1alpha1.ProfileRecorderLogs, + }, + }, nil) + mock.ListRecordedPodsReturns(&corev1.PodList{ + Items: []corev1.Pod{}, + }, nil) mock.DecodePodReturns(testPod.DeepCopy(), nil) mock.LabelSelectorAsSelectorReturns(labels.Everything(), nil) mock.UpdateResourceReturns(errTest) @@ -307,6 +443,47 @@ func TestHandle(t *testing.T) { require.Equal(t, http.StatusInternalServerError, int(resp.Result.Code)) }, }, + //nolint: dupl // golint flags this as a dup of above, but here we're testing failure of UpdateResourceStatus + { // failure on UpdateResourceStatus + prepare: func(mock *recordingfakes.FakeImpl) { + mock.ListProfileRecordingsReturns(&v1alpha1.ProfileRecordingList{ + Items: []v1alpha1.ProfileRecording{ + { + Spec: v1alpha1.ProfileRecordingSpec{ + Kind: v1alpha1.ProfileRecordingKindSeccompProfile, + }, + Status: v1alpha1.ProfileRecordingStatus{ + ActiveWorkloads: []string{"1", "2", "3"}, + }, + }, + }, + }, nil) + mock.GetProfileRecordingReturns(&v1alpha1.ProfileRecording{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-little-profile-recording", + Namespace: "test-ns", + }, + Spec: v1alpha1.ProfileRecordingSpec{ + Kind: v1alpha1.ProfileRecordingKindSelinuxProfile, + Recorder: v1alpha1.ProfileRecorderLogs, + }, + }, nil) + mock.ListRecordedPodsReturns(&corev1.PodList{ + Items: []corev1.Pod{}, + }, nil) + mock.DecodePodReturns(testPod.DeepCopy(), nil) + mock.LabelSelectorAsSelectorReturns(labels.Everything(), nil) + mock.UpdateResourceStatusReturns(errTest) + }, + request: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Delete, + }, + }, + assert: func(resp admission.Response) { + require.Equal(t, http.StatusInternalServerError, int(resp.Result.Code)) + }, + }, } { mock := &recordingfakes.FakeImpl{} tc.prepare(mock) diff --git a/internal/pkg/webhooks/recording/recordingfakes/fake_impl.go b/internal/pkg/webhooks/recording/recordingfakes/fake_impl.go index 26b8482c7d..06f4c6ea21 100644 --- a/internal/pkg/webhooks/recording/recordingfakes/fake_impl.go +++ b/internal/pkg/webhooks/recording/recordingfakes/fake_impl.go @@ -96,6 +96,21 @@ type FakeImpl struct { result1 *v1alpha1.ProfileRecordingList result2 error } + ListRecordedPodsStub func(context.Context, string, *v1a.LabelSelector) (*v1.PodList, error) + listRecordedPodsMutex sync.RWMutex + listRecordedPodsArgsForCall []struct { + arg1 context.Context + arg2 string + arg3 *v1a.LabelSelector + } + listRecordedPodsReturns struct { + result1 *v1.PodList + result2 error + } + listRecordedPodsReturnsOnCall map[int]struct { + result1 *v1.PodList + result2 error + } SetDecoderStub func(*admission.Decoder) setDecoderMutex sync.RWMutex setDecoderArgsForCall []struct { @@ -445,6 +460,72 @@ func (fake *FakeImpl) ListProfileRecordingsReturnsOnCall(i int, result1 *v1alpha }{result1, result2} } +func (fake *FakeImpl) ListRecordedPods(arg1 context.Context, arg2 string, arg3 *v1a.LabelSelector) (*v1.PodList, error) { + fake.listRecordedPodsMutex.Lock() + ret, specificReturn := fake.listRecordedPodsReturnsOnCall[len(fake.listRecordedPodsArgsForCall)] + fake.listRecordedPodsArgsForCall = append(fake.listRecordedPodsArgsForCall, struct { + arg1 context.Context + arg2 string + arg3 *v1a.LabelSelector + }{arg1, arg2, arg3}) + stub := fake.ListRecordedPodsStub + fakeReturns := fake.listRecordedPodsReturns + fake.recordInvocation("ListRecordedPods", []interface{}{arg1, arg2, arg3}) + fake.listRecordedPodsMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *FakeImpl) ListRecordedPodsCallCount() int { + fake.listRecordedPodsMutex.RLock() + defer fake.listRecordedPodsMutex.RUnlock() + return len(fake.listRecordedPodsArgsForCall) +} + +func (fake *FakeImpl) ListRecordedPodsCalls(stub func(context.Context, string, *v1a.LabelSelector) (*v1.PodList, error)) { + fake.listRecordedPodsMutex.Lock() + defer fake.listRecordedPodsMutex.Unlock() + fake.ListRecordedPodsStub = stub +} + +func (fake *FakeImpl) ListRecordedPodsArgsForCall(i int) (context.Context, string, *v1a.LabelSelector) { + fake.listRecordedPodsMutex.RLock() + defer fake.listRecordedPodsMutex.RUnlock() + argsForCall := fake.listRecordedPodsArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 +} + +func (fake *FakeImpl) ListRecordedPodsReturns(result1 *v1.PodList, result2 error) { + fake.listRecordedPodsMutex.Lock() + defer fake.listRecordedPodsMutex.Unlock() + fake.ListRecordedPodsStub = nil + fake.listRecordedPodsReturns = struct { + result1 *v1.PodList + result2 error + }{result1, result2} +} + +func (fake *FakeImpl) ListRecordedPodsReturnsOnCall(i int, result1 *v1.PodList, result2 error) { + fake.listRecordedPodsMutex.Lock() + defer fake.listRecordedPodsMutex.Unlock() + fake.ListRecordedPodsStub = nil + if fake.listRecordedPodsReturnsOnCall == nil { + fake.listRecordedPodsReturnsOnCall = make(map[int]struct { + result1 *v1.PodList + result2 error + }) + } + fake.listRecordedPodsReturnsOnCall[i] = struct { + result1 *v1.PodList + result2 error + }{result1, result2} +} + func (fake *FakeImpl) SetDecoder(arg1 *admission.Decoder) { fake.setDecoderMutex.Lock() fake.setDecoderArgsForCall = append(fake.setDecoderArgsForCall, struct { @@ -618,6 +699,8 @@ func (fake *FakeImpl) Invocations() map[string][][]interface{} { defer fake.labelSelectorAsSelectorMutex.RUnlock() fake.listProfileRecordingsMutex.RLock() defer fake.listProfileRecordingsMutex.RUnlock() + fake.listRecordedPodsMutex.RLock() + defer fake.listRecordedPodsMutex.RUnlock() fake.setDecoderMutex.RLock() defer fake.setDecoderMutex.RUnlock() fake.updateResourceMutex.RLock() diff --git a/test/e2e_flaky_test.go b/test/e2e_flaky_test.go index 6931718c99..cbb1de56a4 100644 --- a/test/e2e_flaky_test.go +++ b/test/e2e_flaky_test.go @@ -67,6 +67,7 @@ func (e *e2e) TestSecurityProfilesOperator_Flaky() { e.testCaseProfileRecordingSpecificContainerLogs() e.testCaseProfileRecordingDeploymentLogs() e.testCaseRecordingFinalizers() + e.testCaseProfileRecordingDeploymentScaleUpDownLogs() }) e.Run("cluster-wide: Seccomp: Verify profile recording bpf", func() { diff --git a/test/tc_profilerecordings_test.go b/test/tc_profilerecordings_test.go index 0f2a96c275..1407f69665 100644 --- a/test/tc_profilerecordings_test.go +++ b/test/tc_profilerecordings_test.go @@ -20,8 +20,9 @@ import ( "fmt" "os" "regexp" - spoutil "sigs.k8s.io/security-profiles-operator/internal/pkg/util" "time" + + spoutil "sigs.k8s.io/security-profiles-operator/internal/pkg/util" ) const ( @@ -266,6 +267,16 @@ func (e *e2e) testCaseProfileRecordingDeploymentLogs() { ) } +func (e *e2e) testCaseProfileRecordingDeploymentScaleUpDownLogs() { + e.logEnricherOnlyTestCase() + e.profileRecordingScaleDeployment( + exampleRecordingSeccompLogsPath, + regexp.MustCompile( + `(?s)"container"="nginx".*"syscallName"="listen"`+ + `.*"container"="nginx".*"syscallName"="listen"`), + ) +} + func (e *e2e) testCaseProfileRecordingSelinuxDeploymentLogs() { e.logEnricherOnlyTestCase() e.selinuxOnlyTestCase() @@ -541,3 +552,35 @@ spec: return since, podName } + +// tests that scaling the deployment allows to record all replicas +// independent of what happens with the deployment. +func (e *e2e) profileRecordingScaleDeployment( + recording string, waitConditions ...*regexp.Regexp, +) { + e.logf("Creating recording for deployment test") + e.kubectl("create", "-f", recording) + + since, deployName := e.createRecordingTestDeployment() + + if waitConditions != nil { + e.waitForEnricherLogs(since, waitConditions...) + } + + e.kubectl("scale", "deploy", "--replicas=5", deployName) + e.waitFor("condition=available", "deploy", deployName) + // wait for the pods to be ready as per the readinessProbe + e.kubectl("rollout", "status", "deploy", deployName) + + e.kubectl("delete", "deploy", deployName) + + // check the expected number of policies was created + for i := 0; i < 5; i++ { + recordedProfileName := fmt.Sprintf("%s-nginx-%d", recordingName, i) + profile := e.retryGetSeccompProfile(recordedProfileName) + e.Contains(profile, "listen") + e.kubectl("delete", "sp", recordedProfileName) + } + + e.kubectl("delete", "-f", recording) +} From d5d59583fba3d19b45298d3b24c05aedb7d55ac1 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Sun, 28 Aug 2022 15:56:31 +0200 Subject: [PATCH 6/7] recording: Remove "hook" from the list of the supported recording types Removing the "hook" option enables us to remove the state from the webhook as well as simplifies the list of the supported matrix of recordings. --- .../v1alpha1/profilerecording_types.go | 14 +- ...s-operator.x-k8s.io_profilerecordings.yaml | 1 - deploy/base-crds/crds/profilerecording.yaml | 1 - deploy/helm/crds/crds.yaml | 1 - deploy/namespace-operator.yaml | 1 - deploy/openshift-dev.yaml | 1 - deploy/operator.yaml | 1 - deploy/webhook-operator.yaml | 1 - examples/profilerecording-hook.yaml | 13 - hack/ci/Vagrantfile-fedora | 2 +- hack/ci/e2e-fedora.sh | 1 - hack/ci/e2e-flatcar.sh | 1 - hack/ci/e2e-static-webhook.sh | 1 - hacking.md | 3 - installation-usage.md | 195 +------------ internal/pkg/config/config.go | 5 - internal/pkg/daemon/profilerecorder/impl.go | 7 - .../daemon/profilerecorder/profilerecorder.go | 122 +------- .../profilerecorder/profilerecorder_test.go | 260 ------------------ .../profilerecorderfakes/fake_impl.go | 79 ------ .../pkg/webhooks/recording/recording_test.go | 21 +- test/e2e_test.go | 7 - test/suite_test.go | 131 ++++----- test/tc_profilerecordings_test.go | 34 --- 24 files changed, 80 insertions(+), 823 deletions(-) delete mode 100644 examples/profilerecording-hook.yaml diff --git a/api/profilerecording/v1alpha1/profilerecording_types.go b/api/profilerecording/v1alpha1/profilerecording_types.go index b1a51749c2..e8d1219380 100644 --- a/api/profilerecording/v1alpha1/profilerecording_types.go +++ b/api/profilerecording/v1alpha1/profilerecording_types.go @@ -35,7 +35,6 @@ const ( type ProfileRecorder string const ( - ProfileRecorderHook ProfileRecorder = "hook" ProfileRecorderLogs ProfileRecorder = "logs" ProfileRecorderBpf ProfileRecorder = "bpf" ) @@ -47,7 +46,7 @@ type ProfileRecordingSpec struct { Kind ProfileRecordingKind `json:"kind"` // Recorder to be used. - // +kubebuilder:validation:Enum=bpf;hook;logs + // +kubebuilder:validation:Enum=bpf;logs Recorder ProfileRecorder `json:"recorder"` // PodSelector selects the pods to record. This field follows standard @@ -110,16 +109,6 @@ func (pr *ProfileRecording) ctrAnnotationSeccomp(ctrReplicaName, ctrName string) var annotationPrefix string switch pr.Spec.Recorder { - case ProfileRecorderHook: - annotationPrefix = config.SeccompProfileRecordHookAnnotationKey - value = fmt.Sprintf( - "of:%s/%s-%s-%d.json", - config.ProfileRecordingOutputPath, - pr.GetName(), - ctrReplicaName, - time.Now().Unix(), - ) - case ProfileRecorderLogs: annotationPrefix = config.SeccompProfileRecordLogsAnnotationKey value = fmt.Sprintf( @@ -161,7 +150,6 @@ func (pr *ProfileRecording) ctrAnnotationSelinux(ctrReplicaName, ctrName string) time.Now().Unix(), ) - case ProfileRecorderHook: case ProfileRecorderBpf: default: return "", "", fmt.Errorf( diff --git a/bundle/manifests/security-profiles-operator.x-k8s.io_profilerecordings.yaml b/bundle/manifests/security-profiles-operator.x-k8s.io_profilerecordings.yaml index d600ac5f3a..367991ef76 100644 --- a/bundle/manifests/security-profiles-operator.x-k8s.io_profilerecordings.yaml +++ b/bundle/manifests/security-profiles-operator.x-k8s.io_profilerecordings.yaml @@ -105,7 +105,6 @@ spec: description: Recorder to be used. enum: - bpf - - hook - logs type: string required: diff --git a/deploy/base-crds/crds/profilerecording.yaml b/deploy/base-crds/crds/profilerecording.yaml index bb81c5b76f..07aae08847 100644 --- a/deploy/base-crds/crds/profilerecording.yaml +++ b/deploy/base-crds/crds/profilerecording.yaml @@ -103,7 +103,6 @@ spec: description: Recorder to be used. enum: - bpf - - hook - logs type: string required: diff --git a/deploy/helm/crds/crds.yaml b/deploy/helm/crds/crds.yaml index c2fba8dcd8..7f51cd6f8e 100644 --- a/deploy/helm/crds/crds.yaml +++ b/deploy/helm/crds/crds.yaml @@ -182,7 +182,6 @@ spec: description: Recorder to be used. enum: - bpf - - hook - logs type: string required: diff --git a/deploy/namespace-operator.yaml b/deploy/namespace-operator.yaml index d3c2a2ec16..67ef724088 100644 --- a/deploy/namespace-operator.yaml +++ b/deploy/namespace-operator.yaml @@ -182,7 +182,6 @@ spec: description: Recorder to be used. enum: - bpf - - hook - logs type: string required: diff --git a/deploy/openshift-dev.yaml b/deploy/openshift-dev.yaml index 712b725d33..c0712ef9ff 100644 --- a/deploy/openshift-dev.yaml +++ b/deploy/openshift-dev.yaml @@ -182,7 +182,6 @@ spec: description: Recorder to be used. enum: - bpf - - hook - logs type: string required: diff --git a/deploy/operator.yaml b/deploy/operator.yaml index aa3043ae8d..c3c789730a 100644 --- a/deploy/operator.yaml +++ b/deploy/operator.yaml @@ -182,7 +182,6 @@ spec: description: Recorder to be used. enum: - bpf - - hook - logs type: string required: diff --git a/deploy/webhook-operator.yaml b/deploy/webhook-operator.yaml index fadeeaa33c..0c36499314 100644 --- a/deploy/webhook-operator.yaml +++ b/deploy/webhook-operator.yaml @@ -253,7 +253,6 @@ spec: description: Recorder to be used. enum: - bpf - - hook - logs type: string required: diff --git a/examples/profilerecording-hook.yaml b/examples/profilerecording-hook.yaml deleted file mode 100644 index 5bf04f4acb..0000000000 --- a/examples/profilerecording-hook.yaml +++ /dev/null @@ -1,13 +0,0 @@ ---- -apiVersion: security-profiles-operator.x-k8s.io/v1alpha1 -kind: ProfileRecording -metadata: - # The name of the Recording is the same as the resulting `SeccompProfile` CRD - # after reconciliation. - name: test-recording -spec: - kind: SeccompProfile - recorder: hook - podSelector: - matchLabels: - app: alpine diff --git a/hack/ci/Vagrantfile-fedora b/hack/ci/Vagrantfile-fedora index eb7640ff5f..1c2e50e683 100644 --- a/hack/ci/Vagrantfile-fedora +++ b/hack/ci/Vagrantfile-fedora @@ -40,7 +40,7 @@ Vagrant.configure("2") do |config| iptables \ jq \ make \ - oci-seccomp-bpf-hook \ + gcc \ openssl \ podman diff --git a/hack/ci/e2e-fedora.sh b/hack/ci/e2e-fedora.sh index e16cf49718..a16f95abe7 100755 --- a/hack/ci/e2e-fedora.sh +++ b/hack/ci/e2e-fedora.sh @@ -19,7 +19,6 @@ export E2E_CLUSTER_TYPE=vanilla export E2E_TEST_SELINUX=true export E2E_TEST_LOG_ENRICHER=true export E2E_TEST_BPF_RECORDER=true -export E2E_TEST_PROFILE_RECORDING=true export E2E_TEST_FLAKY_TESTS_ONLY=${E2E_TEST_FLAKY_TESTS_ONLY:-false} # These are already tested in the standard e2e test. diff --git a/hack/ci/e2e-flatcar.sh b/hack/ci/e2e-flatcar.sh index 2468883135..d73760ccda 100755 --- a/hack/ci/e2e-flatcar.sh +++ b/hack/ci/e2e-flatcar.sh @@ -21,7 +21,6 @@ export E2E_TEST_SECCOMP=true export E2E_TEST_SELINUX=false export E2E_TEST_LOG_ENRICHER=false export E2E_TEST_BPF_RECORDER=true -export E2E_TEST_PROFILE_RECORDING=false export E2E_TEST_FLAKY_TESTS_ONLY=${E2E_TEST_FLAKY_TESTS_ONLY:-false} export HOSTFS_DEV_MOUNT_PATH="/hostfs" diff --git a/hack/ci/e2e-static-webhook.sh b/hack/ci/e2e-static-webhook.sh index ada3186ae0..d6bc0fb791 100755 --- a/hack/ci/e2e-static-webhook.sh +++ b/hack/ci/e2e-static-webhook.sh @@ -20,7 +20,6 @@ export OPERATOR_MANIFEST="deploy/webhook-operator.yaml" export E2E_TEST_SELINUX=true export E2E_TEST_LOG_ENRICHER=true export E2E_TEST_BPF_RECORDER=true -export E2E_TEST_PROFILE_RECORDING=true export E2E_SKIP_NAMESPACED_TESTS=true export E2E_TEST_SECCOMP=true export E2E_TEST_WEBHOOK_CONFIG=false diff --git a/hacking.md b/hacking.md index 9d4d76fb38..c3aae1f6c3 100644 --- a/hacking.md +++ b/hacking.md @@ -224,8 +224,6 @@ source file: record seccomp or SELinux profiles by tailing the `audit.log`. - `E2E_TEST_SECCOMP` - Whether to run seccomp related e2e tests. Our CI tests the seccomp tests in the kind-based prow target only. -- `E2E_TEST_PROFILE_RECORDING` - Whether to run profile recording tests that - use the `oci-seccomp-bpf-hook`. - `E2E_TEST_BPF_RECORDER` - Whether to test recording of seccomp profiles using our eBPF recorder. Currently, enabled for Fedora only. @@ -286,7 +284,6 @@ E2E_SPO_IMAGE=image-registry.openshift-image-registry.svc:5000/openshift/securit E2E_CLUSTER_TYPE=openshift \ E2E_SKIP_BUILD_IMAGES=true \ E2E_TEST_SECCOMP=false \ -E2E_TEST_PROFILE_RECORDING=false \ E2E_TEST_BPF_RECORDER=false \ E2E_TEST_LOG_ENRICHER=false \ E2E_TEST_SELINUX=true \ diff --git a/installation-usage.md b/installation-usage.md index ffc28cb8bb..34aad82e5e 100644 --- a/installation-usage.md +++ b/installation-usage.md @@ -17,7 +17,6 @@ - [Base syscalls for a container runtime](#base-syscalls-for-a-container-runtime) - [Bind workloads to profiles with ProfileBindings](#bind-workloads-to-profiles-with-profilebindings) - [Record profiles from workloads with ProfileRecordings](#record-profiles-from-workloads-with-profilerecordings) - - [Hook based recording](#hook-based-recording) - [Log enricher based recording](#log-enricher-based-recording) - [eBPF based recording](#ebpf-based-recording) - [Create a SELinux Profile](#create-a-selinux-profile) @@ -372,204 +371,12 @@ Binding a SELinux profile works in the same way, except you'd use the `SelinuxPr ### Record profiles from workloads with `ProfileRecordings` The operator is capable of recording seccomp or SELinux profiles by the usage of the -built-in [eBPF](https://ebpf.io) recorder, [oci-seccomp-bpf-hook][bpf-hook] or +built-in [eBPF](https://ebpf.io) recorder or by evaluating the [audit][auditd] or [syslog][syslog] files. Each method has its pros and cons as well as separate technical requirements. Note that SELinux profiles can only be recorded using the log enricher. -#### Hook based recording - -[OCI hooks][hooks] are part of the OCI runtime-spec, which allow to hook into -the container creation process. The Kubernetes container runtime [CRI-O][cri-o] -supports those hooks out of the box, but has to be configured to listen on the -hooks directory where the [oci-seccomp-bpf-hook.json][hook-json] is located. -This can be done via a drop-in configuration file, for example: - -``` -$ cat /etc/crio/crio.conf.d/03-hooks.conf -[crio.runtime] -hooks_dir = [ - "/path/to/seccomp/hook", -] -``` - -The `seccomp-hook` has to be installed as well. The package name differs on -each Linux distributions, for example of Fedora/RHEL, the package is named -`oci-seccomp-bpf-hook` - -The hook references a [path][path] to the actual binary which gets executed on -`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 -[hooks]: https://github.com/opencontainers/runtime-spec/blob/fd895fb/config.md#posix-platform-hooks -[cri-o]: https://cri-o.io -[hook-json]: https://github.com/containers/oci-seccomp-bpf-hook/blob/50e711/oci-seccomp-bpf-hook.json -[path]: https://github.com/containers/oci-seccomp-bpf-hook/blob/50e711/oci-seccomp-bpf-hook.json#L4 - -Unfortunately, [containerd][containerd] is not [capable of running OCI -hooks][containerd-hooks], yet. - -[containerd]: https://containerd.io -[containerd-hooks]: https://github.com/containerd/cri/issues/405 - -We can create a new `ProfileRecording` to indicate to the operator that a -specific workload should be recorded: - -```yaml -apiVersion: security-profiles-operator.x-k8s.io/v1alpha1 -kind: ProfileRecording -metadata: - name: test-recording -spec: - kind: SeccompProfile - recorder: hook - podSelector: - matchLabels: - app: alpine -``` - -Please note the `recorder: hook`, which has to be used to use the hook based -recording feature. Recordings are linked to workloads by using a `podSelector`, -which means that every workload, which contains the label `app=alpine`, will -from now on be recorded into the `SeccompProfile` CRD with the name -`test-recording`. Please be aware that the resource will be overwritten if -recordings are executed multiple times. - -Now we can start the recording by running a workload which contains the label: - -``` -$ 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. The name of the CRD is suffixed with the pod name: - -``` -$ kubectl describe seccompprofile test-recording-my-pod -Name: test-recording-my-pod -Namespace: security-profiles-operator -… -Spec: - Architectures: - SCMP_ARCH_X86_64 - Default Action: SCMP_ACT_ERRNO - Syscalls: - Action: SCMP_ACT_ALLOW - Names: - …[other syscalls]… - mkdir - …[other syscalls]… -Status: - Localhost Profile: operator/security-profiles-operator/test-recording-my-pod.json - Path: /var/lib/kubelet/seccomp/operator/security-profiles-operator/test-recording-my-pod.json - Status: Active -Events: - Type Reason Age From Message - ---- ------ ---- ---- ------- - Normal SeccompProfileCreated 32s profilerecorder seccomp profile created - Normal SavedSeccompProfile 30s (x3 over 32s) profile Successfully saved profile to disk -``` - -We can see that the created profile also contains the executed `mkdir` command -as well as got reconciled to every node. - -The events of the operator will give more insights about the overall process or -if anything goes wrong: - -``` -> kubectl get events | grep -i record -3m45s Normal SeccompProfileRecording pod/alpine Recording seccomp profile -3m29s Normal SeccompProfileCreated seccompprofile/test-recording seccomp profile created -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 - recorder: hook - 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/recording-nginx.json -recording-redis Active 32s operator/default/recording-redis.json -``` - -On top of that, we're able to record distinguishable replicas, for example when -working with Deployments like these: - -``` -apiVersion: apps/v1 -kind: Deployment -metadata: - name: my-deployment -spec: - selector: - matchLabels: - app: my-app - replicas: 3 - template: - metadata: - labels: - app: my-app - spec: - containers: - - name: nginx - image: quay.io/security-profiles-operator/test-nginx:1.19.1 -``` - -If the deployment gets deleted, then the operator writes three seccomp profiles -instead of just one: - -``` -> kubectl get sp -o wide -NAME STATUS AGE LOCALHOSTPROFILE -recording-nginx-0 Active 51s operator/default/recording-nginx-0.json -recording-nginx-1 Active 51s operator/default/recording-nginx-1.json -recording-nginx-2 Active 55s operator/default/recording-nginx-2.json -``` - -This may be helpful when testing in load balanced scenarios where the profiles -have to be compared in an additional step. - -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 -workload via a BPF module. - #### Log enricher based recording When using the log enricher for recording seccomp or SELinux profiles, please ensure that diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go index 3fbd257215..8d76ffefd9 100644 --- a/internal/pkg/config/config.go +++ b/internal/pkg/config/config.go @@ -81,11 +81,6 @@ const ( // DefaultProfilingPort is the start port where the profiling endpoint runs. DefaultProfilingPort = 6060 - // SeccompProfileRecordHookAnnotationKey is the annotation on a Pod that - // triggers the oci-seccomp-bpf-hook to trace the syscalls of a Pod and - // created a seccomp profile. - SeccompProfileRecordHookAnnotationKey = "io.containers.trace-syscall/" - // SeccompProfileRecordLogsAnnotationKey is the annotation on a Pod that // triggers the internal log enricher to trace the syscalls of a Pod and // created a seccomp profile. diff --git a/internal/pkg/daemon/profilerecorder/impl.go b/internal/pkg/daemon/profilerecorder/impl.go index 8597c53e58..44e44ec56a 100644 --- a/internal/pkg/daemon/profilerecorder/impl.go +++ b/internal/pkg/daemon/profilerecorder/impl.go @@ -18,8 +18,6 @@ package profilerecorder import ( "context" - "os" - "github.com/containers/common/pkg/seccomp" "github.com/crossplane/crossplane-runtime/pkg/resource" "google.golang.org/grpc" @@ -67,7 +65,6 @@ type impl interface { context.Context, client.Client, client.Object, controllerutil.MutateFn, ) (controllerutil.OperationResult, error) GoArchToSeccompArch(string) (seccomp.Arch, error) - ReadFile(string) ([]byte, error) Syscalls( context.Context, enricherapi.EnricherClient, *enricherapi.SyscallsRequest, ) (*enricherapi.SyscallsResponse, error) @@ -173,10 +170,6 @@ func (*defaultImpl) GoArchToSeccompArch(goArch string) (seccomp.Arch, error) { return seccomp.GoArchToSeccompArch(goArch) } -func (*defaultImpl) ReadFile(filename string) ([]byte, error) { - return os.ReadFile(filename) -} - func (*defaultImpl) Syscalls( ctx context.Context, c enricherapi.EnricherClient, in *enricherapi.SyscallsRequest, ) (*enricherapi.SyscallsResponse, error) { diff --git a/internal/pkg/daemon/profilerecorder/profilerecorder.go b/internal/pkg/daemon/profilerecorder/profilerecorder.go index 50480f36bb..e798a70e0c 100644 --- a/internal/pkg/daemon/profilerecorder/profilerecorder.go +++ b/internal/pkg/daemon/profilerecorder/profilerecorder.go @@ -18,12 +18,10 @@ package profilerecorder import ( "context" - "encoding/json" "errors" "fmt" "net/http" "os" - "path/filepath" "sort" "strings" "sync" @@ -183,8 +181,7 @@ func (r *RecorderReconciler) isPodWithTraceAnnotation(obj runtime.Object) bool { } for key := range p.Annotations { - if strings.HasPrefix(key, config.SeccompProfileRecordHookAnnotationKey) || - strings.HasPrefix(key, config.SelinuxProfileRecordLogsAnnotationKey) || + if strings.HasPrefix(key, config.SelinuxProfileRecordLogsAnnotationKey) || strings.HasPrefix(key, config.SeccompProfileRecordLogsAnnotationKey) || strings.HasPrefix(key, config.SeccompProfileRecordBpfAnnotationKey) { return true @@ -221,15 +218,6 @@ func (r *RecorderReconciler) Reconcile(_ context.Context, req reconcile.Request) return reconcile.Result{}, nil } - hookProfiles, err := parseHookAnnotations(pod.Annotations) - if err != nil { - // Malformed annotations could be set by users directly, which is - // why we are ignoring them. - logger.Info("Ignoring because unable to parse hook annotation", "error", err) - r.record.Event(pod, event.Warning(reasonAnnotationParsing, err)) - return reconcile.Result{}, nil - } - logProfiles, err := parseLogAnnotations(pod.Annotations) if err != nil { // Malformed annotations could be set by users directly, which is @@ -250,10 +238,7 @@ func (r *RecorderReconciler) Reconcile(_ context.Context, req reconcile.Request) var profiles []profileToCollect var recorder profilerecording1alpha1.ProfileRecorder - if len(hookProfiles) > 0 { //nolint:gocritic - profiles = hookProfiles - recorder = profilerecording1alpha1.ProfileRecorderHook - } else if len(logProfiles) > 0 { + if len(logProfiles) > 0 { profiles = logProfiles recorder = profilerecording1alpha1.ProfileRecorderLogs } else if len(bpfProfiles) > 0 { @@ -264,7 +249,7 @@ func (r *RecorderReconciler) Reconcile(_ context.Context, req reconcile.Request) profiles = bpfProfiles recorder = profilerecording1alpha1.ProfileRecorderBpf } else { - logger.Info("No hook, log or bpf annotations found on pod") + logger.Info("No log or bpf annotations found on pod") return reconcile.Result{}, nil } @@ -351,14 +336,6 @@ func (r *RecorderReconciler) collectProfile( return errors.New("type assert pod to watch") } - if podToWatch.recorder == profilerecording1alpha1.ProfileRecorderHook { - if err := r.collectLocalProfiles( - ctx, name, podToWatch.profiles, - ); err != nil { - return fmt.Errorf("collect local profile: %w", err) - } - } - if podToWatch.recorder == profilerecording1alpha1.ProfileRecorderLogs { if err := r.collectLogProfiles( ctx, name, podToWatch.profiles, @@ -379,60 +356,6 @@ func (r *RecorderReconciler) collectProfile( return nil } -func (r *RecorderReconciler) collectLocalProfiles( - ctx context.Context, - name types.NamespacedName, - profiles []profileToCollect, -) error { - for _, prf := range profiles { - profilePath := prf.name - if prf.kind != profilerecording1alpha1.ProfileRecordingKindSeccompProfile { - return errors.New("only seccomp profiles supported via a hook") - } - - r.log.Info("Collecting profile", "path", profilePath) - - data, err := r.ReadFile(profilePath) - if err != nil { - r.log.Error(err, "Failed to read profile") - return fmt.Errorf("read profile: %w", err) - } - - // Remove the file extension and timestamp - profileName, err := extractProfileName(filepath.Base(profilePath)) - if err != nil { - return fmt.Errorf("extract profile name: %w", err) - } - - profile := &seccompprofileapi.SeccompProfile{ - ObjectMeta: metav1.ObjectMeta{ - Name: profileName, - Namespace: name.Namespace, - }, - } - res, err := r.CreateOrUpdate(ctx, r.client, profile, - func() error { - if err := json.Unmarshal(data, &profile.Spec); err != nil { - return fmt.Errorf("unmarshal profile spec JSON: %w", err) - } - return nil - }, - ) - if err != nil { - r.log.Error(err, "Cannot create seccompprofile resource") - r.record.Event(profile, event.Warning(reasonProfileCreationFailed, err)) - return fmt.Errorf("create seccompProfile resource: %w", err) - } - r.log.Info("Created/updated profile", "action", res, "name", profileName) - r.record.Event( - profile, - event.Normal(reasonProfileCreated, "seccomp profile created"), - ) - } - - return nil -} - func (r *RecorderReconciler) collectLogProfiles( ctx context.Context, name types.NamespacedName, @@ -717,45 +640,6 @@ func extractProfileName(s string) (string, error) { return s[:lastIndex], nil } -// parseHookAnnotations parses the provided annotations and extracts the -// mandatory output files for the hook recorder. -func parseHookAnnotations(annotations map[string]string) (res []profileToCollect, err error) { - const prefix = "of:" - - for key, value := range annotations { - if !strings.HasPrefix(key, config.SeccompProfileRecordHookAnnotationKey) { - continue - } - - if !strings.HasPrefix(value, prefix) { - return nil, fmt.Errorf( - "%s: must start with %q prefix", errInvalidAnnotation, prefix, - ) - } - - outputFile := strings.TrimSpace(strings.TrimPrefix(value, prefix)) - if !filepath.IsAbs(outputFile) { - return nil, fmt.Errorf( - "%s: output file path must be absolute: %q", - errInvalidAnnotation, outputFile, - ) - } - - if !strings.HasPrefix(outputFile, config.ProfileRecordingOutputPath) { - // Ignoring profile outside standard output path, it may be - // user-defined - continue - } - - res = append(res, profileToCollect{ - kind: profilerecording1alpha1.ProfileRecordingKindSeccompProfile, - name: outputFile, - }) - } - - return res, nil -} - // parseLogAnnotations parses the provided annotations and extracts the // mandatory output profiles for the log recorder. func parseLogAnnotations(annotations map[string]string) (res []profileToCollect, err error) { diff --git a/internal/pkg/daemon/profilerecorder/profilerecorder_test.go b/internal/pkg/daemon/profilerecorder/profilerecorder_test.go index 44a49982f9..17c12e4a4f 100644 --- a/internal/pkg/daemon/profilerecorder/profilerecorder_test.go +++ b/internal/pkg/daemon/profilerecorder/profilerecorder_test.go @@ -21,7 +21,6 @@ import ( "errors" "fmt" "runtime" - "sync" "testing" "time" @@ -141,15 +140,6 @@ func TestReconcile(t *testing.T) { }, } - emptySyncMap := func(m *sync.Map) { - cnt := 0 - m.Range(func(_, _ interface{}) bool { - cnt++ - return true - }) - assert.Zero(t, cnt) - } - for _, tc := range []struct { prepare func(*RecorderReconciler, *profilerecorderfakes.FakeImpl) assert func(*RecorderReconciler, error) @@ -604,21 +594,6 @@ func TestReconcile(t *testing.T) { assert.Nil(t, err) }, }, - { // parseHookAnnotations failed - prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - mock.GetPodReturns(&corev1.Pod{ - Status: corev1.PodStatus{Phase: corev1.PodPending}, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - config.SeccompProfileRecordHookAnnotationKey: "", - }, - }, - }, nil) - }, - assert: func(sut *RecorderReconciler, err error) { - assert.Nil(t, err) - }, - }, { // failure GetPod (error reading pod) prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { mock.GetPodReturns(nil, errTest) @@ -627,81 +602,6 @@ func TestReconcile(t *testing.T) { assert.NotNil(t, err) }, }, - { // hook success record - prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - mock.GetPodReturns(&corev1.Pod{ - Status: corev1.PodStatus{Phase: corev1.PodPending}, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - config.SeccompProfileRecordHookAnnotationKey: "of:" + config.ProfileRecordingOutputPath + "/file.json", - }, - }, - }, nil) - }, - assert: func(sut *RecorderReconciler, err error) { - assert.Nil(t, err) - v, ok := sut.podsToWatch.Load(testRequest.NamespacedName.String()) - assert.True(t, ok) - pod, ok := v.(podToWatch) - assert.True(t, ok) - assert.Equal(t, recordingapi.ProfileRecorderHook, pod.recorder) - assert.Len(t, pod.profiles, 1) - assert.Equal(t, recordingapi.ProfileRecordingKindSeccompProfile, pod.profiles[0].kind) - assert.Contains(t, pod.profiles[0].name, "file.json") - assert.Contains(t, pod.profiles[0].name, config.ProfileRecordingOutputPath) - // already tracking - _, retryErr := sut.Reconcile(context.Background(), testRequest) - assert.Nil(t, retryErr) - }, - }, - { // hook record failed no output file - prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - mock.GetPodReturns(&corev1.Pod{ - Status: corev1.PodStatus{Phase: corev1.PodPending}, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - config.SeccompProfileRecordHookAnnotationKey: "of:", - }, - }, - }, nil) - }, - assert: func(sut *RecorderReconciler, err error) { - assert.Nil(t, err) - emptySyncMap(&sut.podsToWatch) - }, - }, - { // hook record failed no absolute path - prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - mock.GetPodReturns(&corev1.Pod{ - Status: corev1.PodStatus{Phase: corev1.PodPending}, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - config.SeccompProfileRecordHookAnnotationKey: "of:relative/path", - }, - }, - }, nil) - }, - assert: func(sut *RecorderReconciler, err error) { - assert.Nil(t, err) - emptySyncMap(&sut.podsToWatch) - }, - }, - { // hook record failed wrong output path - prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - mock.GetPodReturns(&corev1.Pod{ - Status: corev1.PodStatus{Phase: corev1.PodPending}, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - config.SeccompProfileRecordHookAnnotationKey: "of:/wrong/output/path", - }, - }, - }, nil) - }, - assert: func(sut *RecorderReconciler, err error) { - assert.Nil(t, err) - emptySyncMap(&sut.podsToWatch) - }, - }, { //nolint:dupl // test duplicates are fine // log seccomp success record prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { @@ -756,154 +656,6 @@ func TestReconcile(t *testing.T) { assert.Nil(t, retryErr) }, }, - { // hook success collect - prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) - value := podToWatch{ - recorder: recordingapi.ProfileRecorderHook, - profiles: []profileToCollect{ - { - kind: recordingapi.ProfileRecordingKindSeccompProfile, - name: profileName, - }, - }, - } - sut.podsToWatch.Store(testRequest.NamespacedName.String(), value) - - mock.GetPodReturns(&corev1.Pod{ - Status: corev1.PodStatus{Phase: corev1.PodSucceeded}, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - config.SeccompProfileRecordHookAnnotationKey: profileName, - }, - }, - }, nil) - mock.ReadFileReturns([]byte("{}"), nil) - mock.CreateOrUpdateCalls(func( - ctx context.Context, - c client.Client, - obj client.Object, - f controllerutil.MutateFn, - ) (controllerutil.OperationResult, error) { - err := f() - assert.Nil(t, err) - return "", nil - }) - }, - assert: func(sut *RecorderReconciler, err error) { - assert.Nil(t, err) - }, - }, - { // hook collect failed CreateOrUpdate - prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) - value := podToWatch{ - recorder: recordingapi.ProfileRecorderHook, - profiles: []profileToCollect{ - { - kind: recordingapi.ProfileRecordingKindSeccompProfile, - name: profileName, - }, - }, - } - sut.podsToWatch.Store(testRequest.NamespacedName.String(), value) - - mock.GetPodReturns(&corev1.Pod{ - Status: corev1.PodStatus{Phase: corev1.PodSucceeded}, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - config.SeccompProfileRecordHookAnnotationKey: profileName, - }, - }, - }, nil) - mock.CreateOrUpdateReturns("", errTest) - }, - assert: func(sut *RecorderReconciler, err error) { - assert.NotNil(t, err) - }, - }, - { // hook collect failed extractProfileName - prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - const profileName = "profile" - value := podToWatch{ - recorder: recordingapi.ProfileRecorderHook, - profiles: []profileToCollect{ - { - kind: recordingapi.ProfileRecordingKindSeccompProfile, - name: profileName, - }, - }, - } - sut.podsToWatch.Store(testRequest.NamespacedName.String(), value) - - mock.GetPodReturns(&corev1.Pod{ - Status: corev1.PodStatus{Phase: corev1.PodSucceeded}, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - config.SeccompProfileRecordHookAnnotationKey: profileName, - }, - }, - }, nil) - }, - assert: func(sut *RecorderReconciler, err error) { - assert.NotNil(t, err) - }, - }, - { // hook collect failed ReadFile - prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - const profileName = "profile" - value := podToWatch{ - recorder: recordingapi.ProfileRecorderHook, - profiles: []profileToCollect{ - { - kind: recordingapi.ProfileRecordingKindSeccompProfile, - name: profileName, - }, - }, - } - sut.podsToWatch.Store(testRequest.NamespacedName.String(), value) - - mock.GetPodReturns(&corev1.Pod{ - Status: corev1.PodStatus{Phase: corev1.PodSucceeded}, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - config.SeccompProfileRecordHookAnnotationKey: profileName, - }, - }, - }, nil) - mock.ReadFileReturns(nil, errTest) - }, - assert: func(sut *RecorderReconciler, err error) { - assert.NotNil(t, err) - }, - }, - { // hook collect failed wrong kind - prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - const profileName = "profile" - value := podToWatch{ - recorder: recordingapi.ProfileRecorderHook, - profiles: []profileToCollect{ - { - kind: "wrong", - name: profileName, - }, - }, - } - sut.podsToWatch.Store(testRequest.NamespacedName.String(), value) - - mock.GetPodReturns(&corev1.Pod{ - Status: corev1.PodStatus{Phase: corev1.PodSucceeded}, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - config.SeccompProfileRecordHookAnnotationKey: profileName, - }, - }, - }, nil) - }, - assert: func(sut *RecorderReconciler, err error) { - assert.NotNil(t, err) - }, - }, { // logs seccomp success collect prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) @@ -1486,18 +1238,6 @@ func TestIsPodWithTraceAnnotation(t *testing.T) { prepare func(*RecorderReconciler) apiruntime.Object assert func(bool) }{ - { // success seccomp hook - prepare: func(sut *RecorderReconciler) apiruntime.Object { - return &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - config.SeccompProfileRecordHookAnnotationKey: "", - }, - }} - }, - assert: func(res bool) { - assert.True(t, res) - }, - }, { // success selinux logs prepare: func(sut *RecorderReconciler) apiruntime.Object { return &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/pkg/daemon/profilerecorder/profilerecorderfakes/fake_impl.go b/internal/pkg/daemon/profilerecorder/profilerecorderfakes/fake_impl.go index 67131f3f49..25c14f073d 100644 --- a/internal/pkg/daemon/profilerecorder/profilerecorderfakes/fake_impl.go +++ b/internal/pkg/daemon/profilerecorder/profilerecorderfakes/fake_impl.go @@ -203,19 +203,6 @@ type FakeImpl struct { newControllerManagedByReturnsOnCall map[int]struct { result1 error } - ReadFileStub func(string) ([]byte, error) - readFileMutex sync.RWMutex - readFileArgsForCall []struct { - arg1 string - } - readFileReturns struct { - result1 []byte - result2 error - } - readFileReturnsOnCall map[int]struct { - result1 []byte - result2 error - } ResetAvcsStub func(context.Context, api_enricher.EnricherClient, *api_enricher.AvcRequest) error resetAvcsMutex sync.RWMutex resetAvcsArgsForCall []struct { @@ -1062,70 +1049,6 @@ func (fake *FakeImpl) NewControllerManagedByReturnsOnCall(i int, result1 error) }{result1} } -func (fake *FakeImpl) ReadFile(arg1 string) ([]byte, error) { - fake.readFileMutex.Lock() - ret, specificReturn := fake.readFileReturnsOnCall[len(fake.readFileArgsForCall)] - fake.readFileArgsForCall = append(fake.readFileArgsForCall, struct { - arg1 string - }{arg1}) - stub := fake.ReadFileStub - fakeReturns := fake.readFileReturns - fake.recordInvocation("ReadFile", []interface{}{arg1}) - fake.readFileMutex.Unlock() - if stub != nil { - return stub(arg1) - } - if specificReturn { - return ret.result1, ret.result2 - } - return fakeReturns.result1, fakeReturns.result2 -} - -func (fake *FakeImpl) ReadFileCallCount() int { - fake.readFileMutex.RLock() - defer fake.readFileMutex.RUnlock() - return len(fake.readFileArgsForCall) -} - -func (fake *FakeImpl) ReadFileCalls(stub func(string) ([]byte, error)) { - fake.readFileMutex.Lock() - defer fake.readFileMutex.Unlock() - fake.ReadFileStub = stub -} - -func (fake *FakeImpl) ReadFileArgsForCall(i int) string { - fake.readFileMutex.RLock() - defer fake.readFileMutex.RUnlock() - argsForCall := fake.readFileArgsForCall[i] - return argsForCall.arg1 -} - -func (fake *FakeImpl) ReadFileReturns(result1 []byte, result2 error) { - fake.readFileMutex.Lock() - defer fake.readFileMutex.Unlock() - fake.ReadFileStub = nil - fake.readFileReturns = struct { - result1 []byte - result2 error - }{result1, result2} -} - -func (fake *FakeImpl) ReadFileReturnsOnCall(i int, result1 []byte, result2 error) { - fake.readFileMutex.Lock() - defer fake.readFileMutex.Unlock() - fake.ReadFileStub = nil - if fake.readFileReturnsOnCall == nil { - fake.readFileReturnsOnCall = make(map[int]struct { - result1 []byte - result2 error - }) - } - fake.readFileReturnsOnCall[i] = struct { - result1 []byte - result2 error - }{result1, result2} -} - func (fake *FakeImpl) ResetAvcs(arg1 context.Context, arg2 api_enricher.EnricherClient, arg3 *api_enricher.AvcRequest) error { fake.resetAvcsMutex.Lock() ret, specificReturn := fake.resetAvcsReturnsOnCall[len(fake.resetAvcsArgsForCall)] @@ -1535,8 +1458,6 @@ func (fake *FakeImpl) Invocations() map[string][][]interface{} { defer fake.newClientMutex.RUnlock() fake.newControllerManagedByMutex.RLock() defer fake.newControllerManagedByMutex.RUnlock() - fake.readFileMutex.RLock() - defer fake.readFileMutex.RUnlock() fake.resetAvcsMutex.RLock() defer fake.resetAvcsMutex.RUnlock() fake.resetSyscallsMutex.RLock() diff --git a/internal/pkg/webhooks/recording/recording_test.go b/internal/pkg/webhooks/recording/recording_test.go index c2e97847cf..19dcb4f2ca 100644 --- a/internal/pkg/webhooks/recording/recording_test.go +++ b/internal/pkg/webhooks/recording/recording_test.go @@ -144,7 +144,7 @@ func TestHandle(t *testing.T) { { Spec: v1alpha1.ProfileRecordingSpec{ Kind: v1alpha1.ProfileRecordingKindSeccompProfile, - Recorder: v1alpha1.ProfileRecorderHook, + Recorder: v1alpha1.ProfileRecorderBpf, }, }, }, @@ -156,7 +156,7 @@ func TestHandle(t *testing.T) { }, Spec: v1alpha1.ProfileRecordingSpec{ Kind: v1alpha1.ProfileRecordingKindSelinuxProfile, - Recorder: v1alpha1.ProfileRecorderLogs, + Recorder: v1alpha1.ProfileRecorderBpf, }, }, nil) mock.ListRecordedPodsReturns(&corev1.PodList{ @@ -219,7 +219,7 @@ func TestHandle(t *testing.T) { { Spec: v1alpha1.ProfileRecordingSpec{ Kind: v1alpha1.ProfileRecordingKindSeccompProfile, - Recorder: v1alpha1.ProfileRecorderHook, + Recorder: v1alpha1.ProfileRecorderBpf, }, }, }, @@ -320,9 +320,13 @@ func TestHandle(t *testing.T) { mock.ListProfileRecordingsReturns(&v1alpha1.ProfileRecordingList{ Items: []v1alpha1.ProfileRecording{ { + ObjectMeta: metav1.ObjectMeta{ + Name: "my-little-profile-recording", + Namespace: "test-ns", + }, Spec: v1alpha1.ProfileRecordingSpec{ Kind: v1alpha1.ProfileRecordingKindSeccompProfile, - Recorder: v1alpha1.ProfileRecorderHook, + Recorder: v1alpha1.ProfileRecorderLogs, }, }, }, @@ -342,7 +346,14 @@ func TestHandle(t *testing.T) { }, nil) pod := testPod.DeepCopy() pod.Annotations = map[string]string{ - "io.containers.trace-syscall/container": "of:/some/path.json", + "io.containers.trace-logs/container": "my-little-profile-recording-container-0-1661693966", + } + localhostProfile := "operator//log-enricher-trace.json" + pod.Spec.SecurityContext = &corev1.PodSecurityContext{ + SeccompProfile: &corev1.SeccompProfile{ + Type: corev1.SeccompProfileTypeLocalhost, + LocalhostProfile: &localhostProfile, + }, } mock.DecodePodReturns(pod, nil) mock.LabelSelectorAsSelectorReturns(labels.Everything(), nil) diff --git a/test/e2e_test.go b/test/e2e_test.go index d27dcd679d..2c7cba4b4e 100644 --- a/test/e2e_test.go +++ b/test/e2e_test.go @@ -132,13 +132,6 @@ func (e *e2e) TestSecurityProfilesOperator() { e.testCaseSelinuxProfileBinding(nodes) }) - e.Run("cluster-wide: Seccomp: Verify profile recording hook", func() { - e.testCaseProfileRecordingStaticPodHook() - e.testCaseProfileRecordingKubectlRunHook() - e.testCaseProfileRecordingMultiContainerHook() - e.testCaseProfileRecordingDeploymentHook() - }) - e.Run("cluster-wide: Selinux: Verify SELinux profile recording logs", func() { e.testCaseProfileRecordingStaticPodSELinuxLogs() e.testCaseProfileRecordingMultiContainerSELinuxLogs() diff --git a/test/suite_test.go b/test/suite_test.go index 04274aefc0..253eeac600 100644 --- a/test/suite_test.go +++ b/test/suite_test.go @@ -46,21 +46,20 @@ const ( ) var ( - clusterType = os.Getenv("E2E_CLUSTER_TYPE") - envSkipBuildImages = os.Getenv("E2E_SKIP_BUILD_IMAGES") - envTestImage = os.Getenv("E2E_SPO_IMAGE") - spodConfig = os.Getenv("E2E_SPOD_CONFIG") - envSkipFlakyTests = os.Getenv("E2E_SKIP_FLAKY_TESTS") - envSkipNamespacedTests = os.Getenv("E2E_SKIP_NAMESPACED_TESTS") - envSelinuxTestsEnabled = os.Getenv("E2E_TEST_SELINUX") - envLogEnricherTestsEnabled = os.Getenv("E2E_TEST_LOG_ENRICHER") - envSeccompTestsEnabled = os.Getenv("E2E_TEST_SECCOMP") - envProfileRecordingTestsEnabled = os.Getenv("E2E_TEST_PROFILE_RECORDING") - envBpfRecorderTestsEnabled = os.Getenv("E2E_TEST_BPF_RECORDER") - envWebhookConfigTestsEnabled = os.Getenv("E2E_TEST_WEBHOOK_CONFIG") - containerRuntime = os.Getenv("CONTAINER_RUNTIME") - nodeRootfsPrefix = os.Getenv("NODE_ROOTFS_PREFIX") - operatorManifest = os.Getenv("OPERATOR_MANIFEST") + clusterType = os.Getenv("E2E_CLUSTER_TYPE") + envSkipBuildImages = os.Getenv("E2E_SKIP_BUILD_IMAGES") + envTestImage = os.Getenv("E2E_SPO_IMAGE") + spodConfig = os.Getenv("E2E_SPOD_CONFIG") + envSkipFlakyTests = os.Getenv("E2E_SKIP_FLAKY_TESTS") + envSkipNamespacedTests = os.Getenv("E2E_SKIP_NAMESPACED_TESTS") + envSelinuxTestsEnabled = os.Getenv("E2E_TEST_SELINUX") + envLogEnricherTestsEnabled = os.Getenv("E2E_TEST_LOG_ENRICHER") + envSeccompTestsEnabled = os.Getenv("E2E_TEST_SECCOMP") + envBpfRecorderTestsEnabled = os.Getenv("E2E_TEST_BPF_RECORDER") + envWebhookConfigTestsEnabled = os.Getenv("E2E_TEST_WEBHOOK_CONFIG") + containerRuntime = os.Getenv("CONTAINER_RUNTIME") + nodeRootfsPrefix = os.Getenv("NODE_ROOTFS_PREFIX") + operatorManifest = os.Getenv("OPERATOR_MANIFEST") ) const ( @@ -90,7 +89,6 @@ type e2e struct { selinuxEnabled bool logEnricherEnabled bool testSeccomp bool - testProfileRecording bool bpfRecorderEnabled bool skipNamespacedTests bool skipFlakyTests bool @@ -145,10 +143,6 @@ func TestSuite(t *testing.T) { if err != nil { testSeccomp = true } - testProfileRecording, err := strconv.ParseBool(envProfileRecordingTestsEnabled) - if err != nil { - testProfileRecording = false - } bpfRecorderEnabled, err := strconv.ParseBool(envBpfRecorderTestsEnabled) if err != nil { bpfRecorderEnabled = false @@ -181,22 +175,21 @@ func TestSuite(t *testing.T) { } suite.Run(t, &kinde2e{ e2e{ - logger: klogr.New(), - pullPolicy: "Never", - testImage: testImage, - spodConfig: spodConfig, - containerRuntime: containerRuntime, - nodeRootfsPrefix: nodeRootfsPrefix, - selinuxEnabled: selinuxEnabled, - logEnricherEnabled: logEnricherEnabled, - testSeccomp: testSeccomp, - testProfileRecording: testProfileRecording, - selinuxdImage: selinuxdImage, - bpfRecorderEnabled: bpfRecorderEnabled, - skipNamespacedTests: skipNamespacedTests, - operatorManifest: operatorManifest, - testWebhookConfig: testWebhookConfig, - skipFlakyTests: skipFlakyTests, + logger: klogr.New(), + pullPolicy: "Never", + testImage: testImage, + spodConfig: spodConfig, + containerRuntime: containerRuntime, + nodeRootfsPrefix: nodeRootfsPrefix, + selinuxEnabled: selinuxEnabled, + logEnricherEnabled: logEnricherEnabled, + testSeccomp: testSeccomp, + selinuxdImage: selinuxdImage, + bpfRecorderEnabled: bpfRecorderEnabled, + skipNamespacedTests: skipNamespacedTests, + operatorManifest: operatorManifest, + testWebhookConfig: testWebhookConfig, + skipFlakyTests: skipFlakyTests, }, "", "", }) @@ -214,21 +207,20 @@ func TestSuite(t *testing.T) { logger: klogr.New(), // Need to pull the image as it'll be uploaded to the cluster OCP // image registry and not on the nodes. - pullPolicy: "Always", - testImage: testImage, - spodConfig: spodConfig, - containerRuntime: containerRuntime, - nodeRootfsPrefix: nodeRootfsPrefix, - selinuxEnabled: selinuxEnabled, - logEnricherEnabled: logEnricherEnabled, - testSeccomp: testSeccomp, - testProfileRecording: testProfileRecording, - selinuxdImage: selinuxdImage, - bpfRecorderEnabled: bpfRecorderEnabled, - skipNamespacedTests: skipNamespacedTests, - operatorManifest: operatorManifest, - testWebhookConfig: testWebhookConfig, - skipFlakyTests: skipFlakyTests, + pullPolicy: "Always", + testImage: testImage, + spodConfig: spodConfig, + containerRuntime: containerRuntime, + nodeRootfsPrefix: nodeRootfsPrefix, + selinuxEnabled: selinuxEnabled, + logEnricherEnabled: logEnricherEnabled, + testSeccomp: testSeccomp, + selinuxdImage: selinuxdImage, + bpfRecorderEnabled: bpfRecorderEnabled, + skipNamespacedTests: skipNamespacedTests, + operatorManifest: operatorManifest, + testWebhookConfig: testWebhookConfig, + skipFlakyTests: skipFlakyTests, }, skipBuildImages, skipPushImages, @@ -240,22 +232,21 @@ func TestSuite(t *testing.T) { selinuxdImage = "quay.io/security-profiles-operator/selinuxd-fedora" suite.Run(t, &vanilla{ e2e{ - logger: klogr.New(), - pullPolicy: "Never", - testImage: testImage, - spodConfig: spodConfig, - containerRuntime: containerRuntime, - nodeRootfsPrefix: nodeRootfsPrefix, - selinuxEnabled: selinuxEnabled, - logEnricherEnabled: logEnricherEnabled, - testSeccomp: testSeccomp, - testProfileRecording: testProfileRecording, - selinuxdImage: selinuxdImage, - bpfRecorderEnabled: bpfRecorderEnabled, - skipNamespacedTests: skipNamespacedTests, - operatorManifest: operatorManifest, - testWebhookConfig: testWebhookConfig, - skipFlakyTests: skipFlakyTests, + logger: klogr.New(), + pullPolicy: "Never", + testImage: testImage, + spodConfig: spodConfig, + containerRuntime: containerRuntime, + nodeRootfsPrefix: nodeRootfsPrefix, + selinuxEnabled: selinuxEnabled, + logEnricherEnabled: logEnricherEnabled, + testSeccomp: testSeccomp, + selinuxdImage: selinuxdImage, + bpfRecorderEnabled: bpfRecorderEnabled, + skipNamespacedTests: skipNamespacedTests, + operatorManifest: operatorManifest, + testWebhookConfig: testWebhookConfig, + skipFlakyTests: skipFlakyTests, // NOTE(jaosorior): Our current vanilla jobs are // single-node only. singleNodeEnvironment: true, @@ -710,12 +701,6 @@ func (e *e2e) seccompOnlyTestCase() { } } -func (e *e2e) profileRecordingTestCase() { - if !e.testProfileRecording { - e.T().Skip("Skipping Profile-Recording-related test") - } -} - func (e *e2e) bpfRecorderOnlyTestCase() { if !e.bpfRecorderEnabled { e.T().Skip("Skipping bpf recorder related test") diff --git a/test/tc_profilerecordings_test.go b/test/tc_profilerecordings_test.go index 1407f69665..f7e50d35d4 100644 --- a/test/tc_profilerecordings_test.go +++ b/test/tc_profilerecordings_test.go @@ -26,7 +26,6 @@ import ( ) const ( - exampleRecordingHookPath = "examples/profilerecording-hook.yaml" exampleRecordingSeccompLogsPath = "examples/profilerecording-seccomp-logs.yaml" exampleRecordingSeccompSpecificContainerLogsPath = "examples/profilerecording-seccomp-specific-container-logs.yaml" exampleRecordingSelinuxLogsPath = "examples/profilerecording-selinux-logs.yaml" @@ -58,11 +57,6 @@ func (e *e2e) waitForEnricherLogs(since time.Time, conditions ...*regexp.Regexp) } } -func (e *e2e) testCaseProfileRecordingStaticPodHook() { - e.profileRecordingTestCase() - e.profileRecordingStaticPod(exampleRecordingHookPath) -} - func (e *e2e) testCaseProfileRecordingStaticPodLogs() { e.logEnricherOnlyTestCase() e.profileRecordingStaticPod( @@ -123,29 +117,6 @@ func (e *e2e) profileRecordingStaticPod(recording string, waitConditions ...*reg e.kubectl("delete", "sp", resourceName) } -func (e *e2e) testCaseProfileRecordingKubectlRunHook() { - e.profileRecordingTestCase() - - e.logf("Creating recording for kubectl run test") - e.kubectl("create", "-f", exampleRecordingHookPath) - - e.logf("Creating test pod") - e.kubectlRun("--labels=app=alpine", "fedora", "--", "echo", "test") - - resourceName := recordingName + "-fedora" - profile := e.retryGetSeccompProfile(resourceName) - e.Contains(profile, "prctl") - e.NotContains(profile, "mkdir") - - e.kubectl("delete", "-f", exampleRecordingHookPath) - e.kubectl("delete", "sp", resourceName) -} - -func (e *e2e) testCaseProfileRecordingMultiContainerHook() { - e.profileRecordingTestCase() - e.profileRecordingMultiContainer(exampleRecordingHookPath) -} - func (e *e2e) testCaseProfileRecordingMultiContainerLogs() { e.logEnricherOnlyTestCase() e.profileRecordingMultiContainer( @@ -252,11 +223,6 @@ func (e *e2e) profileRecordingSpecificContainer( e.kubectl("delete", "sp", profileNameNginx) } -func (e *e2e) testCaseProfileRecordingDeploymentHook() { - e.profileRecordingTestCase() - e.profileRecordingDeployment(exampleRecordingHookPath) -} - func (e *e2e) testCaseProfileRecordingDeploymentLogs() { e.logEnricherOnlyTestCase() e.profileRecordingDeployment( From 81a5617c05f409a3a94e6186aa2fb549ce587b85 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 5 Sep 2022 20:26:06 +0200 Subject: [PATCH 7/7] recording: Remove state from the webhook, use a random nonce instead Instead of keeping track of the replicas in the webhook, let's use a random nonce to distinguish between different container replicas. When the profile is recorded for a replicated container, we already know the pod name, so let's use the hash of the pod to append to the container. This would also make it easier to cross-reference the pods and the containers. In order to parse the recording annotations easily, we also switch from using dashes in the recording annotations to underscores. Because underscores can't be used in k8s object names, they make it easy to split the annotations along them and are allowed in annotations. --- .../v1alpha1/profilerecording_types.go | 51 +++----- internal/pkg/daemon/profilerecorder/impl.go | 1 + .../daemon/profilerecorder/profilerecorder.go | 118 +++++++++++++----- .../profilerecorder/profilerecorder_test.go | 42 +++---- internal/pkg/webhooks/recording/recording.go | 54 +------- test/tc_bpf_recorder_test.go | 14 ++- test/tc_profilerecordings_test.go | 55 ++++---- 7 files changed, 172 insertions(+), 163 deletions(-) diff --git a/api/profilerecording/v1alpha1/profilerecording_types.go b/api/profilerecording/v1alpha1/profilerecording_types.go index e8d1219380..85fe2b419a 100644 --- a/api/profilerecording/v1alpha1/profilerecording_types.go +++ b/api/profilerecording/v1alpha1/profilerecording_types.go @@ -21,6 +21,7 @@ import ( "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilrand "k8s.io/apimachinery/pkg/util/rand" "sigs.k8s.io/security-profiles-operator/internal/pkg/config" ) @@ -79,17 +80,12 @@ type ProfileRecording struct { Status ProfileRecordingStatus `json:"status,omitempty"` } -func (pr *ProfileRecording) CtrAnnotation(replica, ctrName string) (key, value string, err error) { - ctrReplicaName := ctrName - if replica != "" { - ctrReplicaName += replica - } - +func (pr *ProfileRecording) CtrAnnotation(ctrName string) (key, value string, err error) { switch pr.Spec.Kind { case ProfileRecordingKindSeccompProfile: - return pr.ctrAnnotationSeccomp(ctrReplicaName, ctrName) + return pr.ctrAnnotationSeccomp(ctrName) case ProfileRecordingKindSelinuxProfile: - return pr.ctrAnnotationSelinux(ctrReplicaName, ctrName) + return pr.ctrAnnotationSelinux(ctrName) } return "", "", fmt.Errorf( @@ -105,28 +101,26 @@ func (pr *ProfileRecording) IsKindSupported() bool { return false } -func (pr *ProfileRecording) ctrAnnotationSeccomp(ctrReplicaName, ctrName string) (key, value string, err error) { +func (pr *ProfileRecording) ctrAnnotationValue(ctrName string) string { + const nonceSize = 5 + + return fmt.Sprintf( + "%s_%s_%s_%d", + pr.GetName(), + ctrName, + utilrand.String(nonceSize), + time.Now().Unix(), + ) +} + +func (pr *ProfileRecording) ctrAnnotationSeccomp(ctrName string) (key, value string, err error) { var annotationPrefix string switch pr.Spec.Recorder { case ProfileRecorderLogs: annotationPrefix = config.SeccompProfileRecordLogsAnnotationKey - value = fmt.Sprintf( - "%s-%s-%d", - pr.GetName(), - ctrReplicaName, - time.Now().Unix(), - ) - case ProfileRecorderBpf: annotationPrefix = config.SeccompProfileRecordBpfAnnotationKey - value = fmt.Sprintf( - "%s-%s-%d", - pr.GetName(), - ctrReplicaName, - time.Now().Unix(), - ) - default: return "", "", fmt.Errorf( "invalid recorder: %s", pr.Spec.Recorder, @@ -134,22 +128,16 @@ func (pr *ProfileRecording) ctrAnnotationSeccomp(ctrReplicaName, ctrName string) } key = annotationPrefix + ctrName + value = pr.ctrAnnotationValue(ctrName) return key, value, err } -func (pr *ProfileRecording) ctrAnnotationSelinux(ctrReplicaName, ctrName string) (key, value string, err error) { +func (pr *ProfileRecording) ctrAnnotationSelinux(ctrName string) (key, value string, err error) { var annotationPrefix string switch pr.Spec.Recorder { case ProfileRecorderLogs: annotationPrefix = config.SelinuxProfileRecordLogsAnnotationKey - value = fmt.Sprintf( - "%s-%s-%d", - pr.GetName(), - ctrReplicaName, - time.Now().Unix(), - ) - case ProfileRecorderBpf: default: return "", "", fmt.Errorf( @@ -157,6 +145,7 @@ func (pr *ProfileRecording) ctrAnnotationSelinux(ctrReplicaName, ctrName string) ) } + value = pr.ctrAnnotationValue(ctrName) key = annotationPrefix + ctrName return } diff --git a/internal/pkg/daemon/profilerecorder/impl.go b/internal/pkg/daemon/profilerecorder/impl.go index 44e44ec56a..090c08e963 100644 --- a/internal/pkg/daemon/profilerecorder/impl.go +++ b/internal/pkg/daemon/profilerecorder/impl.go @@ -18,6 +18,7 @@ package profilerecorder import ( "context" + "github.com/containers/common/pkg/seccomp" "github.com/crossplane/crossplane-runtime/pkg/resource" "google.golang.org/grpc" diff --git a/internal/pkg/daemon/profilerecorder/profilerecorder.go b/internal/pkg/daemon/profilerecorder/profilerecorder.go index e798a70e0c..88b4d3cfdf 100644 --- a/internal/pkg/daemon/profilerecorder/profilerecorder.go +++ b/internal/pkg/daemon/profilerecorder/profilerecorder.go @@ -90,6 +90,7 @@ type profileToCollect struct { } type podToWatch struct { + baseName types.NamespacedName recorder profilerecording1alpha1.ProfileRecorder profiles []profileToCollect } @@ -257,9 +258,16 @@ func (r *RecorderReconciler) Reconcile(_ context.Context, req reconcile.Request) logger.Info("Recording profile", "kind", prf.kind, "name", prf.name, "pod", req.NamespacedName.String()) } + // for pods managed by a replicated controller, let's store the replicated + // name so that we can later strip the suffix from the fully-generated pod name + baseName := req.NamespacedName + if pod.GenerateName != "" { + baseName.Name = pod.GenerateName + } + r.podsToWatch.Store( req.NamespacedName.String(), - podToWatch{recorder, profiles}, + podToWatch{baseName, recorder, profiles}, ) r.record.Event(pod, event.Normal(reasonProfileRecording, "Recording profiles")) } @@ -322,9 +330,9 @@ func (r *RecorderReconciler) stopBpfRecorder() error { } func (r *RecorderReconciler) collectProfile( - ctx context.Context, name types.NamespacedName, + ctx context.Context, podName types.NamespacedName, ) error { - n := name.String() + n := podName.String() value, ok := r.podsToWatch.Load(n) if !ok { @@ -336,9 +344,15 @@ func (r *RecorderReconciler) collectProfile( return errors.New("type assert pod to watch") } + replicaSuffix := "" + if podToWatch.baseName.Name != podName.Name && strings.HasPrefix(podName.Name, podToWatch.baseName.Name) { + // this is a replica, we need to strip the suffix from the pod name + replicaSuffix = strings.TrimPrefix(podName.Name, podToWatch.baseName.Name) + } + if podToWatch.recorder == profilerecording1alpha1.ProfileRecorderLogs { if err := r.collectLogProfiles( - ctx, name, podToWatch.profiles, + ctx, replicaSuffix, podName, podToWatch.profiles, ); err != nil { return fmt.Errorf("collect log profile: %w", err) } @@ -346,7 +360,7 @@ func (r *RecorderReconciler) collectProfile( if podToWatch.recorder == profilerecording1alpha1.ProfileRecorderBpf { if err := r.collectBpfProfiles( - ctx, name, podToWatch.profiles, + ctx, replicaSuffix, podName, podToWatch.profiles, ); err != nil { return fmt.Errorf("collect bpf profile: %w", err) } @@ -358,7 +372,8 @@ func (r *RecorderReconciler) collectProfile( func (r *RecorderReconciler) collectLogProfiles( ctx context.Context, - name types.NamespacedName, + replicaSuffix string, + podName types.NamespacedName, profiles []profileToCollect, ) error { r.log.Info("Checking checking if enricher is enabled") @@ -381,19 +396,18 @@ func (r *RecorderReconciler) collectLogProfiles( enricherClient := enricherapi.NewEnricherClient(conn) for _, prf := range profiles { - // Remove the timestamp - profileName, err := extractProfileName(prf.name) + profileNamespacedName, err := profileIDToName(replicaSuffix, podName.Namespace, prf.name) if err != nil { - return fmt.Errorf("extract profile name: %w", err) + return fmt.Errorf("converting profile id to name: %w", err) } - r.log.Info("Collecting profile", "name", profileName, "kind", prf.kind) + r.log.Info("Collecting profile", "name", profileNamespacedName, "kind", prf.kind) switch prf.kind { case profilerecording1alpha1.ProfileRecordingKindSeccompProfile: - err = r.collectLogSeccompProfile(ctx, enricherClient, profileName, name.Namespace, prf.name) + err = r.collectLogSeccompProfile(ctx, enricherClient, profileNamespacedName, prf.name) case profilerecording1alpha1.ProfileRecordingKindSelinuxProfile: - err = r.collectLogSelinuxProfile(ctx, enricherClient, profileName, name.Namespace, prf.name) + err = r.collectLogSelinuxProfile(ctx, enricherClient, profileNamespacedName, prf.name) default: err = fmt.Errorf("unrecognized kind %s", prf.kind) } @@ -409,8 +423,7 @@ func (r *RecorderReconciler) collectLogProfiles( func (r *RecorderReconciler) collectLogSeccompProfile( ctx context.Context, enricherClient enricherapi.EnricherClient, - profileName string, - namespace string, + profileNamespacedName types.NamespacedName, profileID string, ) error { // Retrieve the syscalls for the recording @@ -436,8 +449,8 @@ func (r *RecorderReconciler) collectLogSeccompProfile( profile := &seccompprofileapi.SeccompProfile{ ObjectMeta: metav1.ObjectMeta{ - Name: profileName, - Namespace: namespace, + Name: profileNamespacedName.Name, + Namespace: profileNamespacedName.Namespace, }, Spec: profileSpec, } @@ -454,7 +467,7 @@ func (r *RecorderReconciler) collectLogSeccompProfile( return fmt.Errorf("create seccompProfile resource: %w", err) } - r.log.Info("Created/updated profile", "action", res, "name", profileName) + r.log.Info("Created/updated profile", "action", res, "name", profileNamespacedName.Name) r.record.Event( profile, event.Normal(reasonProfileCreated, "seccomp profile created"), @@ -471,8 +484,7 @@ func (r *RecorderReconciler) collectLogSeccompProfile( func (r *RecorderReconciler) collectLogSelinuxProfile( ctx context.Context, enricherClient enricherapi.EnricherClient, - profileName string, - namespace string, + profileNamespacedName types.NamespacedName, profileID string, ) error { // Retrieve the syscalls for the recording @@ -493,8 +505,8 @@ func (r *RecorderReconciler) collectLogSelinuxProfile( profile := &selxv1alpha2.SelinuxProfile{ ObjectMeta: metav1.ObjectMeta{ - Name: profileName, - Namespace: namespace, + Name: profileNamespacedName.Name, + Namespace: profileNamespacedName.Namespace, }, Spec: selinuxProfileSpec, } @@ -518,7 +530,7 @@ func (r *RecorderReconciler) collectLogSelinuxProfile( r.record.Event(profile, event.Warning(reasonProfileCreationFailed, err)) return fmt.Errorf("create selinuxprofile resource: %w", err) } - r.log.Info("Created/updated selinux profile", "action", res, "name", profileName) + r.log.Info("Created/updated selinux profile", "action", res, "name", profileNamespacedName) r.record.Event( profile, event.Normal(reasonProfileCreated, "selinuxprofile profile created"), @@ -526,7 +538,7 @@ func (r *RecorderReconciler) collectLogSelinuxProfile( // Reset the selinuxprofile for further recordings if err := r.ResetAvcs(ctx, enricherClient, request); err != nil { - return fmt.Errorf("reset selinuxprofile for profile %s: %w", profileName, err) + return fmt.Errorf("reset selinuxprofile for profile %s: %w", profileNamespacedName, err) } return nil @@ -552,7 +564,8 @@ func (r *RecorderReconciler) formatSelinuxProfile( func (r *RecorderReconciler) collectBpfProfiles( ctx context.Context, - name types.NamespacedName, + replicaSuffix string, + podName types.NamespacedName, profiles []profileToCollect, ) error { recorderClient, cancel, err := r.getBpfRecorderClient() @@ -593,15 +606,15 @@ func (r *RecorderReconciler) collectBpfProfiles( } // Remove the timestamp - profileName, err := extractProfileName(profile.name) + profileNamespacedName, err := profileIDToName(replicaSuffix, podName.Namespace, profile.name) if err != nil { - return fmt.Errorf("extract profile name: %w", err) + return fmt.Errorf("converting profile id to name: %w", err) } profile := &seccompprofileapi.SeccompProfile{ ObjectMeta: metav1.ObjectMeta{ - Name: profileName, - Namespace: name.Namespace, + Name: profileNamespacedName.Name, + Namespace: profileNamespacedName.Namespace, }, Spec: profileSpec, } @@ -618,7 +631,7 @@ func (r *RecorderReconciler) collectBpfProfiles( return fmt.Errorf("create seccompProfile resource: %w", err) } - r.log.Info("Created/updated profile", "action", res, "name", profileName) + r.log.Info("Created/updated profile", "action", res, "name", profileNamespacedName) r.record.Event( profile, event.Normal(reasonProfileCreated, "seccomp profile created"), @@ -632,12 +645,49 @@ func (r *RecorderReconciler) collectBpfProfiles( return nil } -func extractProfileName(s string) (string, error) { - lastIndex := strings.LastIndex(s, "-") - if lastIndex == -1 { - return "", fmt.Errorf("malformed profile path: %s", s) +type parsedAnnotation struct { + profileName string + cntName string + nonce string + timestamp string +} + +func parseProfileAnnotation(annotation string) (*parsedAnnotation, error) { + const expectedParts = 4 + + parts := strings.Split(annotation, "_") + if len(parts) != expectedParts { + return nil, + fmt.Errorf("invalid annotation: %s, expected %d parts got %d", annotation, expectedParts, len(parts)) } - return s[:lastIndex], nil + + return &parsedAnnotation{ + profileName: parts[0], + cntName: parts[1], + nonce: parts[2], // unused for now, but we might need it in the future + timestamp: parts[3], // unused for now, but let's keep it for future use + }, nil +} + +func createProfileName(cntName, replicaSuffix, namespace, profileName string) types.NamespacedName { + name := fmt.Sprintf("%s-%s", profileName, cntName) + if replicaSuffix != "" { + name = fmt.Sprintf("%s-%s", name, replicaSuffix) + } + + return types.NamespacedName{ + Name: name, + Namespace: namespace, + } +} + +func profileIDToName(replicaSuffix, namespace, profileID string) (types.NamespacedName, error) { + parsedProfileName, err := parseProfileAnnotation(profileID) + if err != nil { + return types.NamespacedName{}, fmt.Errorf("parse profile raw annotation: %w", err) + } + + return createProfileName(parsedProfileName.cntName, replicaSuffix, namespace, parsedProfileName.profileName), nil } // parseLogAnnotations parses the provided annotations and extracts the diff --git a/internal/pkg/daemon/profilerecorder/profilerecorder_test.go b/internal/pkg/daemon/profilerecorder/profilerecorder_test.go index 17c12e4a4f..362a5dd9e8 100644 --- a/internal/pkg/daemon/profilerecorder/profilerecorder_test.go +++ b/internal/pkg/daemon/profilerecorder/profilerecorder_test.go @@ -194,7 +194,7 @@ func TestReconcile(t *testing.T) { }, { // BPF success collect prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_4bbwm_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderBpf, profiles: []profileToCollect{ @@ -242,7 +242,7 @@ func TestReconcile(t *testing.T) { { //nolint:dupl // test duplicates are fine // BPF GoArchToSeccompArch fails prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderBpf, profiles: []profileToCollect{ @@ -281,7 +281,7 @@ func TestReconcile(t *testing.T) { { //nolint:dupl // test duplicates are fine // BPF CreateOrUpdate fails prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderBpf, profiles: []profileToCollect{ @@ -320,7 +320,7 @@ func TestReconcile(t *testing.T) { { //nolint:dupl // test duplicates are fine // BPF DialBpfRecorder fails prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderBpf, profiles: []profileToCollect{ @@ -351,7 +351,7 @@ func TestReconcile(t *testing.T) { }, { // BPF DialBpfRecorder fails on StopBpfRecorder prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderBpf, profiles: []profileToCollect{ @@ -427,7 +427,7 @@ func TestReconcile(t *testing.T) { }, { // BPF SyscallsForProfile returns not found prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderBpf, profiles: []profileToCollect{ @@ -461,7 +461,7 @@ func TestReconcile(t *testing.T) { { //nolint:dupl // test duplicates are fine // BPF SyscallsForProfile fails prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderBpf, profiles: []profileToCollect{ @@ -658,7 +658,7 @@ func TestReconcile(t *testing.T) { }, { // logs seccomp success collect prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_4bbwm_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderLogs, profiles: []profileToCollect{ @@ -702,7 +702,7 @@ func TestReconcile(t *testing.T) { }, { // logs seccomp failed ResetSyscalls prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderLogs, profiles: []profileToCollect{ @@ -738,7 +738,7 @@ func TestReconcile(t *testing.T) { { //nolint:dupl // test duplicates are fine // logs seccomp failed CreateOrUpdate prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderLogs, profiles: []profileToCollect{ @@ -774,7 +774,7 @@ func TestReconcile(t *testing.T) { { //nolint:dupl // test duplicates are fine // logs seccomp failed GoArchToSeccompArch prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderLogs, profiles: []profileToCollect{ @@ -810,7 +810,7 @@ func TestReconcile(t *testing.T) { { //nolint:dupl // test duplicates are fine // logs seccomp failed Syscalls prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderLogs, profiles: []profileToCollect{ @@ -842,7 +842,7 @@ func TestReconcile(t *testing.T) { }, { // logs seccomp failed unknown kind prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderLogs, profiles: []profileToCollect{ @@ -906,7 +906,7 @@ func TestReconcile(t *testing.T) { { //nolint:dupl // test duplicates are fine // logs seccomp DialEnricher fails prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderLogs, profiles: []profileToCollect{ @@ -937,7 +937,7 @@ func TestReconcile(t *testing.T) { }, { // logs seccomp EnableLogEnricher false prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderLogs, profiles: []profileToCollect{ @@ -967,7 +967,7 @@ func TestReconcile(t *testing.T) { }, { // logs seccomp failed GetSPOD prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderLogs, profiles: []profileToCollect{ @@ -995,7 +995,7 @@ func TestReconcile(t *testing.T) { }, { // logs selinux success collect prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_4bbwm_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderLogs, profiles: []profileToCollect{ @@ -1044,7 +1044,7 @@ func TestReconcile(t *testing.T) { }, { // logs selinux failed ResetAvcs prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderLogs, profiles: []profileToCollect{ @@ -1076,7 +1076,7 @@ func TestReconcile(t *testing.T) { }, { // logs selinux failed CreateOrUpdate prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderLogs, profiles: []profileToCollect{ @@ -1108,7 +1108,7 @@ func TestReconcile(t *testing.T) { }, { // logs selinux failed formatSelinuxProfile prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderLogs, profiles: []profileToCollect{ @@ -1145,7 +1145,7 @@ func TestReconcile(t *testing.T) { { //nolint:dupl // test duplicates are fine // logs selinux failed Avcs prepare: func(sut *RecorderReconciler, mock *profilerecorderfakes.FakeImpl) { - profileName := fmt.Sprintf("profile-%d", time.Now().Unix()) + profileName := fmt.Sprintf("profile_replica-123_%d", time.Now().Unix()) value := podToWatch{ recorder: recordingapi.ProfileRecorderLogs, profiles: []profileToCollect{ diff --git a/internal/pkg/webhooks/recording/recording.go b/internal/pkg/webhooks/recording/recording.go index 6317c46b85..679eba46aa 100644 --- a/internal/pkg/webhooks/recording/recording.go +++ b/internal/pkg/webhooks/recording/recording.go @@ -22,8 +22,6 @@ import ( "errors" "fmt" "net/http" - "strings" - "sync" "github.com/go-logr/logr" admissionv1 "k8s.io/api/admission/v1" @@ -46,8 +44,7 @@ const finalizer = "active-seccomp-profile-recording-lock" type podSeccompRecorder struct { impl - log logr.Logger - replicas sync.Map + log logr.Logger } func RegisterWebhook(server *webhook.Server, c client.Client) { @@ -189,16 +186,10 @@ func (p *podSeccompRecorder) updatePod( } } - // Handle replicas by tracking them - replica, err := p.getReplica(pod, profileRecording) - if err != nil { - return false, err - } - for i := range ctrs { ctr := ctrs[i] - key, value, err := profileRecording.CtrAnnotation(replica, ctr.Name) + key, value, err := profileRecording.CtrAnnotation(ctr.Name) if err != nil { return false, err } @@ -290,46 +281,6 @@ func (p *podSeccompRecorder) updateSelinuxSecurityContext( ctr.SecurityContext.SELinuxOptions.Type = config.SelinuxPermissiveProfile } -func replicaKey(recordingName, podName string) string { - return fmt.Sprintf("%s/%s", recordingName, podName) -} - -func (p *podSeccompRecorder) cleanupReplicas(recordingName, podName string) { - rKey := replicaKey(recordingName, podName) - - p.replicas.Range(func(key, _ interface{}) bool { - keyString, ok := key.(string) - if !ok { - return false - } - if strings.HasPrefix(rKey, keyString) { - p.replicas.Delete(key) - return false - } - return true - }) -} - -func (p *podSeccompRecorder) getReplica( - pod *corev1.Pod, - profileRecording *profilerecordingv1alpha1.ProfileRecording, -) (string, error) { - replica := "" - if pod.Name == "" && pod.GenerateName != "" { - rKey := replicaKey(profileRecording.Name, pod.GenerateName) - - v, _ := p.replicas.LoadOrStore(rKey, uint(0)) - replica = fmt.Sprintf("-%d", v) - vUint, ok := v.(uint) - if !ok { - return "", errors.New("replicas value is not an uint") - } - p.replicas.Store(rKey, vUint+1) - } - - return replica, nil -} - func (p *podSeccompRecorder) setRecordingReferences( ctx context.Context, op admissionv1.Operation, @@ -401,7 +352,6 @@ func (p *podSeccompRecorder) setFinalizers( } else { if controllerutil.ContainsFinalizer(profileRecording, finalizer) { controllerutil.RemoveFinalizer(profileRecording, finalizer) - p.cleanupReplicas(profileRecording.Name, podName) } } diff --git a/test/tc_bpf_recorder_test.go b/test/tc_bpf_recorder_test.go index c5120d7daa..35faf5eb5c 100644 --- a/test/tc_bpf_recorder_test.go +++ b/test/tc_bpf_recorder_test.go @@ -92,11 +92,14 @@ func (e *e2e) testCaseBpfRecorderStaticPod() { e.kubectl("delete", "sp", resourceName) metrics := e.getSpodMetrics() + // we don't use resource name here, because the metrics are tracked by the annotation name which contains + // underscores instead of dashes + metricName := recordingName + "_nginx" e.Regexp(fmt.Sprintf(`(?m)security_profiles_operator_seccomp_profile_bpf_total{`+ `mount_namespace=".*",`+ `node=".*",`+ - `profile="%s-.*"} \d+`, - resourceName, + `profile="%s_.*"} \d+`, + metricName, ), metrics) } @@ -170,9 +173,12 @@ spec: e.retryGet("deploy", deployName) e.waitFor("condition=available", "deploy", deployName) + suffixes := e.getPodSuffixesByLabel("app=alpine") + e.Len(suffixes, 2) + since := time.Now() - const profileName0 = recordingName + "-nginx-0" - const profileName1 = recordingName + "-nginx-1" + profileName0 := recordingName + "-nginx-" + suffixes[0] + profileName1 := recordingName + "-nginx-" + suffixes[1] e.waitForBpfRecorderLogs(since, profileName0, profileName1) e.kubectl("delete", "deploy", deployName) diff --git a/test/tc_profilerecordings_test.go b/test/tc_profilerecordings_test.go index f7e50d35d4..a04372db23 100644 --- a/test/tc_profilerecordings_test.go +++ b/test/tc_profilerecordings_test.go @@ -20,6 +20,7 @@ import ( "fmt" "os" "regexp" + "strings" "time" spoutil "sigs.k8s.io/security-profiles-operator/internal/pkg/util" @@ -322,17 +323,17 @@ func (e *e2e) profileRecordingDeployment( e.waitForEnricherLogs(since, waitConditions...) } + suffixes := e.getPodSuffixesByLabel("app=alpine") e.kubectl("delete", "deploy", deployName) - const profileName0 = recordingName + "-nginx-0" - const profileName1 = recordingName + "-nginx-1" - profile0 := e.retryGetSeccompProfile(profileName0) - profile1 := e.retryGetSeccompProfile(profileName1) - e.Contains(profile0, "listen") - e.Contains(profile1, "listen") + for _, sfx := range suffixes { + recordedProfileName := recordingName + "-nginx-" + sfx + profile := e.retryGetSeccompProfile(recordedProfileName) + e.Contains(profile, "listen") + e.kubectl("delete", "sp", recordedProfileName) + } e.kubectl("delete", "-f", recording) - e.kubectl("delete", "sp", profileName0, profileName1) } func (e *e2e) profileRecordingSelinuxDeployment( @@ -346,24 +347,24 @@ func (e *e2e) profileRecordingSelinuxDeployment( e.waitForEnricherLogs(since, waitConditions...) } + suffixes := e.getPodSuffixesByLabel("app=alpine") e.kubectl("delete", "deploy", deployName) - const profileName0 = selinuxRecordingName + "-nginx-0" - const profileName1 = selinuxRecordingName + "-nginx-1" - - profile0result := e.retryGetSelinuxJsonpath("{.spec.allow.http_cache_port_t.tcp_socket}", profileName0) - e.Contains(profile0result, "name_bind") + fmt.Println(e.kubectl("get", "sp")) - profile1result := e.retryGetSelinuxJsonpath("{.spec.allow.http_cache_port_t.tcp_socket}", profileName1) - e.Contains(profile1result, "name_bind") + for _, sfx := range suffixes { + recordedProfileName := selinuxRecordingName + "-nginx-" + sfx + profileResult := e.retryGetSelinuxJsonpath("{.spec.allow.http_cache_port_t.tcp_socket}", recordedProfileName) + e.Contains(profileResult, "name_bind") + e.kubectl("delete", "selinuxprofile", recordedProfileName) + } e.kubectl("delete", "-f", recording) - e.kubectl("delete", "selinuxprofile", profileName0, profileName1) } -func (e *e2e) createRecordingTestDeployment() (since time.Time, podName string) { +func (e *e2e) createRecordingTestDeployment() (since time.Time, deployName string) { e.logf("Creating test deployment") - podName = "my-deployment" + deployName = "my-deployment" e.setupRecordingSa(e.getCurrentContextNamespace(defaultNamespace)) @@ -405,12 +406,11 @@ spec: since = time.Now() e.kubectl("create", "-f", testFile.Name()) - const deployName = "my-deployment" e.retryGet("deploy", deployName) e.waitFor("condition=available", "deploy", deployName) e.Nil(os.Remove(testFile.Name())) - return since, podName + return since, deployName } func (e *e2e) retryGetProfile(kind string, args ...string) string { @@ -538,11 +538,12 @@ func (e *e2e) profileRecordingScaleDeployment( // wait for the pods to be ready as per the readinessProbe e.kubectl("rollout", "status", "deploy", deployName) + suffixes := e.getPodSuffixesByLabel("app=alpine") e.kubectl("delete", "deploy", deployName) // check the expected number of policies was created - for i := 0; i < 5; i++ { - recordedProfileName := fmt.Sprintf("%s-nginx-%d", recordingName, i) + for _, sfx := range suffixes { + recordedProfileName := recordingName + "-nginx-" + sfx profile := e.retryGetSeccompProfile(recordedProfileName) e.Contains(profile, "listen") e.kubectl("delete", "sp", recordedProfileName) @@ -550,3 +551,15 @@ func (e *e2e) profileRecordingScaleDeployment( e.kubectl("delete", "-f", recording) } + +func (e *e2e) getPodSuffixesByLabel(label string) []string { //nolint:unparam // it's better to keep the param around + suffixes := make([]string, 0) + podNamesString := e.kubectl("get", "pods", "-l", label, "-o", "jsonpath={.items[*].metadata.name}") + podNames := strings.Fields(podNamesString) + for _, podName := range podNames { + suffixIdx := strings.LastIndex(podName, "-") + suffixes = append(suffixes, podName[suffixIdx+1:]) + } + + return suffixes +}