Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[PodSecurity] Aggregate identical warnings for multiple pods in a namespace #105889

Merged
merged 2 commits into from
Oct 26, 2021
Merged

[PodSecurity] Aggregate identical warnings for multiple pods in a namespace #105889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3102131
[PodSecurity] Aggregate identical warnings for multiple pods in a nam…
njuptlzf Aug 5, 2021
6dabf5f
Make warning order deterministic, limit accumulated pod name data
liggitt Oct 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 51 additions & 5 deletions staging/src/k8s.io/pod-security-admission/admission/admission.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"
"net/http"
"reflect"
"sort"
"time"

"k8s.io/klog/v2"
Expand Down Expand Up @@ -446,6 +447,15 @@ func (a *Admission) EvaluatePod(ctx context.Context, nsPolicy api.Policy, nsPoli
return response
}

// podCount is used to track the number of pods sharing identical warnings when validating a namespace
type podCount struct {
// podName is the lexically first pod name for the given warning
podName string

// podCount is the total number of pods with the same warnings
podCount int
}

func (a *Admission) EvaluatePodsInNamespace(ctx context.Context, namespace string, enforce api.LevelVersion) []string {
timeout := namespacePodCheckTimeout
if deadline, ok := ctx.Deadline(); ok {
Expand All @@ -464,7 +474,12 @@ func (a *Admission) EvaluatePodsInNamespace(ctx context.Context, namespace strin
return []string{"Failed to list pods"}
}

var warnings []string
var (
warnings []string

podWarnings []string
podWarningsToCount = make(map[string]podCount)
)
if len(pods) > namespaceMaxPodsToCheck {
warnings = append(warnings, fmt.Sprintf("Large namespace: only checking the first %d of %d pods", namespaceMaxPodsToCheck, len(pods)))
pods = pods[0:namespaceMaxPodsToCheck]
Expand All @@ -477,15 +492,46 @@ func (a *Admission) EvaluatePodsInNamespace(ctx context.Context, namespace strin
}
r := policy.AggregateCheckResults(a.Evaluator.EvaluatePod(enforce, &pod.ObjectMeta, &pod.Spec))
if !r.Allowed {
// TODO: consider aggregating results (e.g. multiple pods failed for the same reasons)
warnings = append(warnings, fmt.Sprintf("%s: %s", pod.Name, r.ForbiddenReason()))
warning := r.ForbiddenReason()
c, seen := podWarningsToCount[warning]
if !seen {
c.podName = pod.Name
liggitt marked this conversation as resolved.
Show resolved Hide resolved
podWarnings = append(podWarnings, warning)
} else if pod.Name < c.podName {
c.podName = pod.Name
}
c.podCount++
podWarningsToCount[warning] = c
}
if time.Now().After(deadline) {
return append(warnings, fmt.Sprintf("Timeout reached after checking %d pods", i+1))
warnings = append(warnings, fmt.Sprintf("Timeout reached after checking %d pods", i+1))
break
}
}

return warnings
// prepend pod names to warnings
decoratePodWarnings(podWarningsToCount, podWarnings)
// put warnings in a deterministic order
sort.Strings(podWarnings)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a slight preference for sorting before prepending the pod names. It means warning messages would always be in approximately the same order, irrespective of how workloads are named / across namespaces, etc.
Also means that warnings with a common failure (as long as it's first) will be grouped together

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... I think sorting by pod name makes more sense:

  1. that's the same order kubectl get pods returns
  2. the warning lines will start with the pod name
  3. the lexical ordering of the warnings isn't particularly meaningful

ordering by pod looks like this:

Warning: backend-j23h42: non-default capabilities, unrestricted capabilities
Warning: frontend-h23gf2: allowPrivilegeEscalation != false
Warning: myjob-g342hj (and 1 other pod): host namespaces, allowPrivilegeEscalation != false

ordering by warning looks like this:

Warning: frontend-h23gf2: allowPrivilegeEscalation != false
Warning: myjob-g342hj (and 1 other pod): host namespaces, allowPrivilegeEscalation != false
Warning: backend-j23h42: non-default capabilities, unrestricted capabilities

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm fine with this 👍


return append(warnings, podWarnings...)
}

// prefixes warnings with the pod names related to that warning
func decoratePodWarnings(podWarningsToCount map[string]podCount, warnings []string) {
for i, warning := range warnings {
tallclair marked this conversation as resolved.
Show resolved Hide resolved
c := podWarningsToCount[warning]
switch c.podCount {
case 0:
// unexpected, just leave the warning alone
case 1:
warnings[i] = fmt.Sprintf("%s: %s", c.podName, warning)
case 2:
warnings[i] = fmt.Sprintf("%s (and 1 other pod): %s", c.podName, warning)
default:
warnings[i] = fmt.Sprintf("%s (and %d other pods): %s", c.podName, c.podCount-1, warning)
}
}
}

func (a *Admission) PolicyToEvaluate(labels map[string]string) (api.Policy, error) {
Expand Down
8 changes: 5 additions & 3 deletions staging/src/k8s.io/pod-security-admission/admission/admission_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ func TestValidateNamespace(t *testing.T) {
expectAllowed: true,
expectListPods: true,
expectEvaluate: api.LevelVersion{Level: api.LevelRestricted, Version: api.LatestVersion()},
expectWarnings: []string{"noruntimeclasspod: message", "runtimeclass1pod: message", "runtimeclass2pod: message"},
expectWarnings: []string{"noruntimeclasspod (and 2 other pods): message", "runtimeclass3pod: message, message2"},
},
{
name: "update with runtimeclass exempt pods",
Expand All @@ -362,10 +362,9 @@ func TestValidateNamespace(t *testing.T) {
expectAllowed: true,
expectListPods: true,
expectEvaluate: api.LevelVersion{Level: api.LevelRestricted, Version: api.LatestVersion()},
expectWarnings: []string{"noruntimeclasspod: message", "runtimeclass2pod: message"},
expectWarnings: []string{"noruntimeclasspod (and 1 other pod): message", "runtimeclass3pod: message, message2"},
},

// TODO: test for aggregating pods with identical warnings
// TODO: test for bounding evalution time with a warning
// TODO: test for bounding pod count with a warning
// TODO: test for prioritizing evaluating pods from unique controllers
Expand Down Expand Up @@ -424,6 +423,9 @@ func TestValidateNamespace(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{Name: "runtimeclass2pod", Annotations: map[string]string{"error": "message"}},
Spec: corev1.PodSpec{RuntimeClassName: pointer.String("runtimeclass2")},
},
{
ObjectMeta: metav1.ObjectMeta{Name: "runtimeclass3pod", Annotations: map[string]string{"error": "message, message2"}},
},
}
}
podLister := &testPodLister{pods: pods}
Expand Down