Skip to content

Commit 54dbda3

Browse files
hiscochiliec
hisco
and
chiliec
committed
Honor shared Pod resource budgets in audits
Model Kubernetes resource defaulting for workload templates, preserve missing and zero-budget findings, and include restartable sidecars. Co-authored-by: Vladimir Babin <vovababin@gmail.com>
1 parent bc17499 commit 54dbda3

8 files changed

Lines changed: 798 additions & 75 deletions

File tree

‎internal/audit/resource_defaulting_test.go‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
package audit
2+
3+
import (
4+
"net/http"
5+
"net/http/httptest"
6+
"sync/atomic"
7+
"testing"
8+
9+
"github.com/skyhook-io/radar/internal/k8s"
10+
"github.com/skyhook-io/radar/pkg/k8score"
11+
appsv1 "k8s.io/api/apps/v1"
12+
corev1 "k8s.io/api/core/v1"
13+
"k8s.io/apimachinery/pkg/api/resource"
14+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
15+
"k8s.io/client-go/kubernetes"
16+
"k8s.io/client-go/kubernetes/fake"
17+
"k8s.io/client-go/rest"
18+
)
19+
20+
func TestRunFromCacheUsesServerVersionForResourceDefaults(t *testing.T) {
21+
d := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: "app", Namespace: "team"}, Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{
22+
Resources: &corev1.ResourceRequirements{Requests: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("500m")}},
23+
Containers: []corev1.Container{{Name: "app", Resources: corev1.ResourceRequirements{Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("128Mi")}}}, {Name: "helper"}},
24+
}}}}
25+
if err := k8s.InitScopedTestResourceCache(fake.NewClientset(d), map[string]k8score.ResourceScope{k8score.Deployments: {Enabled: true}, k8score.LimitRanges: {Enabled: true}}); err != nil {
26+
t.Fatal(err)
27+
}
28+
t.Cleanup(k8s.ResetTestState)
29+
var versionCalls atomic.Int64
30+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
31+
if r.URL.Path != "/version" {
32+
http.NotFound(w, r)
33+
return
34+
}
35+
versionCalls.Add(1)
36+
w.Header().Set("Content-Type", "application/json")
37+
_, _ = w.Write([]byte(`{"major":"1","minor":"35","gitVersion":"v1.35.0"}`))
38+
}))
39+
defer server.Close()
40+
client, err := kubernetes.NewForConfig(&rest.Config{Host: server.URL})
41+
if err != nil {
42+
t.Fatal(err)
43+
}
44+
previous := k8s.SetTestClient(client)
45+
k8s.InvalidateServerVersionCache()
46+
t.Cleanup(func() { k8s.SetTestClient(previous); k8s.InvalidateServerVersionCache() })
47+
for _, opts := range []*RunOptions{nil, {}, {ClusterVersion: "1.37"}, {ClusterVersion: "1.35"}} {
48+
result := RunFromCache(k8s.GetResourceCache(), []string{"team"}, opts)
49+
count := result.CheckCounts["memoryRequestMissing"]
50+
if opts != nil && opts.ClusterVersion == "1.37" {
51+
if count.Evaluated != 0 {
52+
t.Fatalf("explicit version ignored: %+v", count)
53+
}
54+
} else if count.Evaluated != 1 || count.Passed != 0 {
55+
t.Fatalf("discovered legacy version ignored: %+v", count)
56+
}
57+
}
58+
if versionCalls.Load() != 1 {
59+
t.Fatalf("version discovery calls = %d, want 1", versionCalls.Load())
60+
}
61+
}

‎internal/audit/runner.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,9 +62,14 @@ func RunFromCache(cache *k8s.ResourceCache, namespaces []string, opts *RunOption
6262
input.GitOpsToolsPresent, input.ArgoAppNames = gitOpsRoots()
6363

6464
if opts != nil {
65-
input.ClusterVersion = opts.ClusterVersion
65+
if opts.ClusterVersion != "" {
66+
input.ClusterVersion = opts.ClusterVersion
67+
}
6668
input.ServedAPIs = opts.ServedAPIs
6769
}
70+
if input.ClusterVersion == "" {
71+
input.ClusterVersion = k8s.GetServerVersion()
72+
}
6873

6974
// Crossplane Managed Resources / Composites / Claims live in the dynamic
7075
// cache (unbounded kind set — one CRD per provider service). Listing here

‎pkg/audit/checks.go‎

Lines changed: 44 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -301,7 +301,7 @@ func checkWorkloadPodSpecs(tr *evalTracker, input *CheckInput) []Finding {
301301
for _, w := range specs {
302302
findings = append(findings, checkPodSpecSecurity(tr, w.kind, w.namespace, w.name, w.spec, saByKey, saCovers(w.namespace))...)
303303
findings = append(findings, checkPodSpecReliability(tr, w.kind, w.namespace, w.name, w.spec)...)
304-
findings = append(findings, checkPodSpecEfficiency(tr, w.kind, w.namespace, w.name, w.spec, limitsByNs[w.namespace], lrAuthoritative)...)
304+
findings = append(findings, checkPodSpecEfficiency(tr, w.kind, w.namespace, w.name, w.spec, limitsByNs[w.namespace], lrAuthoritative, input.ClusterVersion)...)
305305
findings = append(findings, checkPodSpecVolumes(tr, w.kind, w.namespace, w.name, w.spec)...)
306306
}
307307
return findings
@@ -359,40 +359,6 @@ func indexLimitRangesByNamespace(lrs []*corev1.LimitRange) map[string][]*corev1.
359359
return m
360360
}
361361

362-
// containerDefaultsFromLimitRanges reports which container resource types
363-
// (cpu/memory requests/limits) would be filled in by admission based on the
364-
// namespace's LimitRange defaults. Only LimitRange items with Type=Container
365-
// contribute to container defaults.
366-
type containerDefaults struct {
367-
cpuRequest, memoryRequest bool
368-
cpuLimit, memoryLimit bool
369-
}
370-
371-
func containerDefaultsFromLimitRanges(lrs []*corev1.LimitRange) containerDefaults {
372-
var d containerDefaults
373-
for _, lr := range lrs {
374-
for _, item := range lr.Spec.Limits {
375-
if item.Type != corev1.LimitTypeContainer {
376-
continue
377-
}
378-
// DefaultRequest covers requests; Default covers limits.
379-
if _, ok := item.DefaultRequest[corev1.ResourceCPU]; ok {
380-
d.cpuRequest = true
381-
}
382-
if _, ok := item.DefaultRequest[corev1.ResourceMemory]; ok {
383-
d.memoryRequest = true
384-
}
385-
if _, ok := item.Default[corev1.ResourceCPU]; ok {
386-
d.cpuLimit = true
387-
}
388-
if _, ok := item.Default[corev1.ResourceMemory]; ok {
389-
d.memoryLimit = true
390-
}
391-
}
392-
}
393-
return d
394-
}
395-
396362
// ============================================================================
397363
// Security checks
398364
// ============================================================================
@@ -851,42 +817,55 @@ func checkMissingPDB(tr *evalTracker, deployments []*appsv1.Deployment, stateful
851817
// Efficiency checks
852818
// ============================================================================
853819

854-
func checkPodSpecEfficiency(tr *evalTracker, kind, namespace, name string, spec corev1.PodSpec, lrs []*corev1.LimitRange, lrAuthoritative bool) []Finding {
855-
if !lrAuthoritative {
856-
// LimitRange defaults suppress these findings; without the inventory
857-
// every emission would be a potential false positive. Skip the whole
858-
// family rather than report on data Radar couldn't see.
820+
func checkPodSpecEfficiency(tr *evalTracker, kind, namespace, name string, spec corev1.PodSpec, lrs []*corev1.LimitRange, lrAuthoritative bool, version string) []Finding {
821+
if kind != "Pod" && !lrAuthoritative {
859822
return nil
860823
}
861-
tr.recordAll(podSpecEfficiencyCheckIDs, namespace)
862-
var findings []Finding
863-
f := func(checkID, severity, msg string) {
864-
findings = append(findings, Finding{
865-
Kind: kind, Namespace: namespace, Name: name,
866-
CheckID: checkID, Category: CategoryEfficiency, Severity: severity, Message: msg,
867-
})
824+
candidates := podResourceCandidates(spec, kind == "Pod", version, lrs)
825+
containerSets := make([][]corev1.Container, len(candidates))
826+
for i, candidate := range candidates {
827+
containerSets[i] = efficiencyContainers(candidate)
868828
}
869-
870-
// LimitRange defaults in the namespace are applied by admission — skip
871-
// flagging missing values that would be filled in automatically.
872-
defaults := containerDefaultsFromLimitRanges(lrs)
873-
874-
for _, c := range spec.Containers {
875-
res := c.Resources
876-
if (res.Requests.Cpu() == nil || res.Requests.Cpu().IsZero()) && !defaults.cpuRequest {
877-
f("cpuRequestMissing", SeverityWarning, fmt.Sprintf("Container %q has no CPU request", c.Name))
878-
}
879-
if (res.Requests.Memory() == nil || res.Requests.Memory().IsZero()) && !defaults.memoryRequest {
880-
f("memoryRequestMissing", SeverityWarning, fmt.Sprintf("Container %q has no memory request", c.Name))
881-
}
882-
if (res.Limits.Cpu() == nil || res.Limits.Cpu().IsZero()) && !defaults.cpuLimit {
883-
f("cpuLimitMissing", SeverityWarning, fmt.Sprintf("Container %q has no CPU limit", c.Name))
829+
var findings []Finding
830+
for _, check := range []struct {
831+
id, label string
832+
resource corev1.ResourceName
833+
request bool
834+
}{
835+
{"cpuRequestMissing", "CPU request", corev1.ResourceCPU, true},
836+
{"memoryRequestMissing", "memory request", corev1.ResourceMemory, true},
837+
{"cpuLimitMissing", "CPU limit", corev1.ResourceCPU, false},
838+
{"memoryLimitMissing", "memory limit", corev1.ResourceMemory, false},
839+
} {
840+
allCovered, failed := true, false
841+
for i, c := range efficiencyContainers(spec) {
842+
alwaysMissing := true
843+
for j, candidate := range candidates {
844+
if resourceConfigured(candidate, containerSets[j][i], check.resource, check.request) {
845+
alwaysMissing = false
846+
} else {
847+
allCovered = false
848+
}
849+
}
850+
if alwaysMissing {
851+
label := "Container"
852+
if i >= len(spec.Containers) {
853+
label = "Sidecar"
854+
}
855+
findings = append(findings, Finding{
856+
Kind: kind, Namespace: namespace, Name: name,
857+
CheckID: check.id, Category: CategoryEfficiency, Severity: SeverityWarning,
858+
Message: fmt.Sprintf("%s %q has no %s", label, c.Name, check.label),
859+
})
860+
failed = true
861+
}
884862
}
885-
if (res.Limits.Memory() == nil || res.Limits.Memory().IsZero()) && !defaults.memoryLimit {
886-
f("memoryLimitMissing", SeverityWarning, fmt.Sprintf("Container %q has no memory limit", c.Name))
863+
if allCovered || failed {
864+
tr.record(check.id, namespace)
865+
} else {
866+
tr.skip("pod-resource-defaulting")
887867
}
888868
}
889-
890869
return findings
891870
}
892871

0 commit comments

Comments
 (0)