Repository: zegl/kube-score
Pull request #396 (2021-09-28 18:16:13)

score: add check for PodDisruptionBudget with no policy
This adds check for PodDisruptionBudget with no policy. PDBs should specify a policy with `minAvailable` or `maxUnavailable`. This test flags PDBs that don't specify either.

This is based on the assumption that the PDB is invalid without `minAvailable` or `maxUnavailable` defined -- do you know of any cases where it's valid to not specify either? I don't. When I've tested a PDB like this in k8s (by accident 😄), the controller sets `.status.expectedPods: 0` regardless of the deployment replicas field.

<!--
    Optional: Add this change to the release notes by adding a RELNOTE comment
    If this shouldn't appear in the notes, simply remove this.
-->

```
RELNOTE: Add check for PodDisruptionBudget with no policy
```



Patch:
diff --git a/domain/kube-score.go b/domain/kube-score.go
index 6bdc013..17dab41 100644
--- a/domain/kube-score.go
+++ b/domain/kube-score.go
@@ -2,10 +2,12 @@ package domain
 
 import (
 	"io"
+
 	appsv1 "k8s.io/api/apps/v1"
 	autoscalingv1 "k8s.io/api/autoscaling/v1"
 	corev1 "k8s.io/api/core/v1"
 	networkingv1 "k8s.io/api/networking/v1"
+	policyv1 "k8s.io/api/policy/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -127,7 +129,10 @@ type CronJobs interface {
 }
 
 type PodDisruptionBudget interface {
+	GetTypeMeta() metav1.TypeMeta
+	GetObjectMeta() metav1.ObjectMeta
 	Namespace() string
+	Spec() policyv1.PodDisruptionBudgetSpec
 	PodDisruptionBudgetSelector() *metav1.LabelSelector
 	FileLocationer
 }
diff --git a/parser/internal/pdb/pod.go b/parser/internal/pdb/pod.go
index 913b5d9..df0246d 100644
--- a/parser/internal/pdb/pod.go
+++ b/parser/internal/pdb/pod.go
@@ -13,6 +13,14 @@ type PodDisruptionBudgetV1beta1 struct {
 	Location ks.FileLocation
 }
 
+func (p PodDisruptionBudgetV1beta1) GetObjectMeta() metav1.ObjectMeta {
+	return p.Obj.ObjectMeta
+}
+
+func (p PodDisruptionBudgetV1beta1) GetTypeMeta() metav1.TypeMeta {
+	return p.Obj.TypeMeta
+}
+
 func (p PodDisruptionBudgetV1beta1) PodDisruptionBudgetSelector() *metav1.LabelSelector {
 	return p.Obj.Spec.Selector
 }
@@ -25,11 +33,23 @@ func (p PodDisruptionBudgetV1beta1) FileLocation() ks.FileLocation {
 	return p.Location
 }
 
+func (p PodDisruptionBudgetV1beta1) Spec() policyv1.PodDisruptionBudgetSpec {
+	return policyv1.PodDisruptionBudgetSpec(p.Obj.Spec)
+}
+
 type PodDisruptionBudgetV1 struct {
 	Obj      policyv1.PodDisruptionBudget
 	Location ks.FileLocation
 }
 
+func (p PodDisruptionBudgetV1) GetObjectMeta() metav1.ObjectMeta {
+	return p.Obj.ObjectMeta
+}
+
+func (p PodDisruptionBudgetV1) GetTypeMeta() metav1.TypeMeta {
+	return p.Obj.TypeMeta
+}
+
 func (p PodDisruptionBudgetV1) PodDisruptionBudgetSelector() *metav1.LabelSelector {
 	return p.Obj.Spec.Selector
 }
@@ -41,3 +61,7 @@ func (p PodDisruptionBudgetV1) FileLocation() ks.FileLocation {
 func (p PodDisruptionBudgetV1) Namespace() string {
 	return p.Obj.Namespace
 }
+
+func (p PodDisruptionBudgetV1) Spec() policyv1.PodDisruptionBudgetSpec {
+	return p.Obj.Spec
+}
diff --git a/parser/parse.go b/parser/parse.go
index 272cd93..49263bb 100644
--- a/parser/parse.go
+++ b/parser/parse.go
@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"fmt"
 	"io/ioutil"
-	policyv1 "k8s.io/api/policy/v1"
 	"log"
 	"strings"
 
@@ -21,6 +20,7 @@ import (
 	extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
 	networkingv1 "k8s.io/api/networking/v1"
 	networkingv1beta1 "k8s.io/api/networking/v1beta1"
+	policyv1 "k8s.io/api/policy/v1"
 	policyv1beta1 "k8s.io/api/policy/v1beta1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -53,6 +53,7 @@ func addToScheme(scheme *runtime.Scheme) {
 	batchv1.AddToScheme(scheme)
 	batchv1beta1.AddToScheme(scheme)
 	policyv1beta1.AddToScheme(scheme)
+	policyv1.AddToScheme(scheme)
 }
 
 type detectKind struct {
diff --git a/score/checks/checks.go b/score/checks/checks.go
index 24f35cb..1a6a178 100644
--- a/score/checks/checks.go
+++ b/score/checks/checks.go
@@ -27,6 +27,7 @@ func New(cnf config.Configuration) *Checks {
 		ingresses:                make(map[string]IngressCheck),
 		cronjobs:                 make(map[string]CronJobCheck),
 		horizontalPodAutoscalers: make(map[string]HorizontalPodAutoscalerCheck),
+		poddisruptionbudgets:     make(map[string]PodDisruptionBudgetCheck),
 	}
 }
 
@@ -100,6 +101,12 @@ type HorizontalPodAutoscalerCheck struct {
 	Fn HorizontalPodAutoscalerCheckFn
 }
 
+type PodDisruptionBudgetCheckFn = func(ks.PodDisruptionBudget) scorecard.TestScore
+type PodDisruptionBudgetCheck struct {
+	ks.Check
+	Fn PodDisruptionBudgetCheckFn
+}
+
 type Checks struct {
 	all                      []ks.Check
 	metas                    map[string]MetaCheck
@@ -111,6 +118,7 @@ type Checks struct {
 	ingresses                map[string]IngressCheck
 	cronjobs                 map[string]CronJobCheck
 	horizontalPodAutoscalers map[string]HorizontalPodAutoscalerCheck
+	poddisruptionbudgets     map[string]PodDisruptionBudgetCheck
 
 	cnf config.Configuration
 }
@@ -317,6 +325,24 @@ func (c *Checks) NetworkPolicies() map[string]NetworkPolicyCheck {
 	return c.networkpolicies
 }
 
+func (c *Checks) RegisterPodDisruptionBudgetCheck(name, comment string, fn PodDisruptionBudgetCheckFn) {
+	ch := NewCheck(name, "PodDisruptionBudget", comment, false)
+	c.registerPodDisruptionBudgetCheck(PodDisruptionBudgetCheck{ch, fn})
+}
+
+func (c *Checks) registerPodDisruptionBudgetCheck(ch PodDisruptionBudgetCheck) {
+	c.all = append(c.all, ch.Check)
+
+	if !c.isEnabled(ch.Check) {
+		return
+	}
+	c.poddisruptionbudgets[machineFriendlyName(ch.Name)] = ch
+}
+
+func (c *Checks) PodDisruptionBudgets() map[string]PodDisruptionBudgetCheck {
+	return c.poddisruptionbudgets
+}
+
 func (c *Checks) RegisterServiceCheck(name, comment string, fn ServiceCheckFn) {
 	ch := NewCheck(name, "Service", comment, false)
 	c.registerServiceCheck(ServiceCheck{ch, fn})
diff --git a/score/disruptionbudget/disruptionbudget.go b/score/disruptionbudget/disruptionbudget.go
index c6b1b68..358be6f 100644
--- a/score/disruptionbudget/disruptionbudget.go
+++ b/score/disruptionbudget/disruptionbudget.go
@@ -15,9 +15,10 @@ import (
 func Register(allChecks *checks.Checks, budgets ks.PodDisruptionBudgets) {
 	allChecks.RegisterStatefulSetCheck("StatefulSet has PodDisruptionBudget", `Makes sure that all StatefulSets are targeted by a PDB`, statefulSetHas(budgets.PodDisruptionBudgets()))
 	allChecks.RegisterDeploymentCheck("Deployment has PodDisruptionBudget", `Makes sure that all Deployments are targeted by a PDB`, deploymentHas(budgets.PodDisruptionBudgets()))
+	allChecks.RegisterPodDisruptionBudgetCheck("PodDisruptionBudget has policy", `Makes sure that PodDisruptionBudgets specify minAvailable or maxUnavailable`, hasPolicy)
 }
 
-func hasMatching(budgets []ks.PodDisruptionBudget, namespace string, lables map[string]string) (bool, error) {
+func hasMatching(budgets []ks.PodDisruptionBudget, namespace string, labels map[string]string) (bool, error) {
 	for _, budget := range budgets {
 		if budget.Namespace() != namespace {
 			continue
@@ -28,7 +29,7 @@ func hasMatching(budgets []ks.PodDisruptionBudget, namespace string, lables map[
 			return false, fmt.Errorf("failed to create selector: %v", err)
 		}
 
-		if selector.Matches(internal.MapLables(lables)) {
+		if selector.Matches(internal.MapLables(labels)) {
 			return true, nil
 		}
 	}
@@ -85,3 +86,15 @@ func deploymentHas(budgets []ks.PodDisruptionBudget) func(appsv1.Deployment) (sc
 		return
 	}
 }
+
+func hasPolicy(pdb ks.PodDisruptionBudget) (score scorecard.TestScore) {
+	spec := pdb.Spec()
+	if spec.MinAvailable == nil && spec.MaxUnavailable == nil {
+		score.AddComment("", "PodDisruptionBudget missing policy", "PodDisruptionBudget should specify minAvailable or maxUnavailable.")
+		score.Grade = scorecard.GradeCritical
+	} else {
+		score.Grade = scorecard.GradeAllOK
+	}
+
+	return
+}
diff --git a/score/score.go b/score/score.go
index 133b550..0ce5b83 100644
--- a/score/score.go
+++ b/score/score.go
@@ -134,5 +134,12 @@ func Score(allObjects ks.AllTypes, cnf config.Configuration) (*scorecard.Scoreca
 		}
 	}
 
+	for _, pdb := range allObjects.PodDisruptionBudgets() {
+		o := newObject(pdb.GetTypeMeta(), pdb.GetObjectMeta())
+		for _, test := range allChecks.PodDisruptionBudgets() {
+			o.Add(test.Fn(pdb), test.Check, pdb)
+		}
+	}
+
 	return &scoreCard, nil
 }


Test patch:
diff --git a/score/poddisruptionbudget_test.go b/score/poddisruptionbudget_test.go
index 27c7e41..5d4992b 100644
--- a/score/poddisruptionbudget_test.go
+++ b/score/poddisruptionbudget_test.go
@@ -46,6 +46,16 @@ func TestDeploymentPodDisruptionBudgetNoMatch(t *testing.T) {
 	testExpectedScore(t, "deployment-poddisruptionbudget-v1beta1-no-match.yaml", "Deployment has PodDisruptionBudget", scorecard.GradeCritical)
 }
 
+func TestDeploymentPodDisruptionBudgetNoPolicy(t *testing.T) {
+	t.Parallel()
+	testExpectedScore(t, "deployment-poddisruptionbudget-v1beta1-no-policy.yaml", "PodDisruptionBudget has policy", scorecard.GradeCritical)
+}
+
+func TestDeploymentPodDisruptionBudgetV1NoPolicy(t *testing.T) {
+	t.Parallel()
+	testExpectedScore(t, "deployment-poddisruptionbudget-v1-no-policy.yaml", "PodDisruptionBudget has policy", scorecard.GradeCritical)
+}
+
 func TestDeploymentPodDisruptionBudgetV1Matches(t *testing.T) {
 	t.Parallel()
 	testExpectedScore(t, "deployment-poddisruptionbudget-v1-matches.yaml", "Deployment has PodDisruptionBudget", scorecard.GradeAllOK)
diff --git a/score/testdata/deployment-poddisruptionbudget-v1-no-policy.yaml b/score/testdata/deployment-poddisruptionbudget-v1-no-policy.yaml
new file mode 100644
index 0000000..7a18bd7
--- /dev/null
+++ b/score/testdata/deployment-poddisruptionbudget-v1-no-policy.yaml
@@ -0,0 +1,22 @@
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+  name: app-budget
+spec:
+  selector:
+    matchLabels:
+      app: foo
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: statefulset-test-1
+spec:
+  template:
+    metadata:
+      labels:
+        app: foo
+    spec:
+      containers:
+      - name: foobar
+        image: foo:bar
\ No newline at end of file
diff --git a/score/testdata/deployment-poddisruptionbudget-v1beta1-no-policy.yaml b/score/testdata/deployment-poddisruptionbudget-v1beta1-no-policy.yaml
new file mode 100644
index 0000000..d63bb89
--- /dev/null
+++ b/score/testdata/deployment-poddisruptionbudget-v1beta1-no-policy.yaml
@@ -0,0 +1,22 @@
+apiVersion: policy/v1beta1
+kind: PodDisruptionBudget
+metadata:
+  name: app-budget
+spec:
+  selector:
+    matchLabels:
+      app: foo
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: statefulset-test-1
+spec:
+  template:
+    metadata:
+      labels:
+        app: foo
+    spec:
+      containers:
+      - name: foobar
+        image: foo:bar
\ No newline at end of file