Skip to content

Commit

Permalink
recording webhook: don't use the replica name to track pods being rec…
Browse files Browse the repository at this point in the history
…orded, 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.
  • Loading branch information
jhrozek committed Aug 23, 2022
1 parent 7b182e1 commit b3928c9
Show file tree
Hide file tree
Showing 13 changed files with 518 additions and 72 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@ rules:
- events
verbs:
- create
- apiGroups:
- ""
resources:
- pods
verbs:
- get
- list
- watch
- apiGroups:
- security-profiles-operator.x-k8s.io
resources:
Expand Down
8 changes: 8 additions & 0 deletions deploy/base/role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,14 @@ rules:
- events
verbs:
- create
- apiGroups:
- ""
resources:
- pods
verbs:
- get
- list
- watch
- apiGroups:
- security-profiles-operator.x-k8s.io
resources:
Expand Down
8 changes: 8 additions & 0 deletions deploy/helm/templates/static-resources.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,14 @@ rules:
- events
verbs:
- create
- apiGroups:
- ""
resources:
- pods
verbs:
- get
- list
- watch
- apiGroups:
- security-profiles-operator.x-k8s.io
resources:
Expand Down
8 changes: 8 additions & 0 deletions deploy/namespace-operator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1569,6 +1569,14 @@ rules:
- events
verbs:
- create
- apiGroups:
- ""
resources:
- pods
verbs:
- get
- list
- watch
- apiGroups:
- security-profiles-operator.x-k8s.io
resources:
Expand Down
8 changes: 8 additions & 0 deletions deploy/openshift-dev.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1569,6 +1569,14 @@ rules:
- events
verbs:
- create
- apiGroups:
- ""
resources:
- pods
verbs:
- get
- list
- watch
- apiGroups:
- security-profiles-operator.x-k8s.io
resources:
Expand Down
8 changes: 8 additions & 0 deletions deploy/operator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1569,6 +1569,14 @@ rules:
- events
verbs:
- create
- apiGroups:
- ""
resources:
- pods
verbs:
- get
- list
- watch
- apiGroups:
- security-profiles-operator.x-k8s.io
resources:
Expand Down
8 changes: 8 additions & 0 deletions deploy/webhook-operator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1423,6 +1423,14 @@ rules:
- events
verbs:
- create
- apiGroups:
- ""
resources:
- pods
verbs:
- get
- list
- watch
- apiGroups:
- security-profiles-operator.x-k8s.io
resources:
Expand Down
27 changes: 26 additions & 1 deletion internal/pkg/webhooks/recording/impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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)
Expand Down Expand Up @@ -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,
Expand Down
Loading

0 comments on commit b3928c9

Please sign in to comment.