Repository: zegl/kube-score
Pull request #552 (2023-08-07 11:38:19)

score/disruptionbudget: improved error message for PDB found but in wrong namespace


Patch:
diff --git a/score/disruptionbudget/disruptionbudget.go b/score/disruptionbudget/disruptionbudget.go
index 5264fd8..2953a2d 100644
--- a/score/disruptionbudget/disruptionbudget.go
+++ b/score/disruptionbudget/disruptionbudget.go
@@ -18,29 +18,33 @@ func Register(allChecks *checks.Checks, budgets ks.PodDisruptionBudgets) {
 	allChecks.RegisterPodDisruptionBudgetCheck("PodDisruptionBudget has policy", `Makes sure that PodDisruptionBudgets specify minAvailable or maxUnavailable`, hasPolicy)
 }
 
-func hasMatching(budgets []ks.PodDisruptionBudget, namespace string, labels map[string]string) (bool, error) {
-	var hasNamespaceMismatch bool
+func hasMatching(budgets []ks.PodDisruptionBudget, namespace string, labels map[string]string) (bool, string, error) {
+	var hasNamespaceMismatch []string
 	for _, budget := range budgets {
-		if budget.Namespace() != namespace {
-			hasNamespaceMismatch = true
-			continue
-		}
 
 		selector, err := metav1.LabelSelectorAsSelector(budget.PodDisruptionBudgetSelector())
 		if err != nil {
-			return false, fmt.Errorf("failed to create selector: %w", err)
+			return false, "", fmt.Errorf("failed to create selector: %w", err)
 		}
 
-		if selector.Matches(internal.MapLabels(labels)) {
-			return true, nil
+		if !selector.Matches(internal.MapLabels(labels)) {
+			continue
 		}
+
+		// matches, but in different namespace
+		if budget.Namespace() != namespace {
+			hasNamespaceMismatch = append(hasNamespaceMismatch, budget.Namespace())
+			continue
+		}
+
+		return true, "", nil
 	}
 
-	if hasNamespaceMismatch {
-		return false, fmt.Errorf("No matching PodDisruptionBudgets and Deployments would be found in the same namespace")
+	if len(hasNamespaceMismatch) > 0 {
+		return false, fmt.Sprintf("A matching budget was found, but in a different namespace. expected='%s' got='%+v'", namespace, hasNamespaceMismatch), nil
 	}
 
-	return false, nil
+	return false, "", nil
 }
 
 func statefulSetHas(budgets []ks.PodDisruptionBudget) func(appsv1.StatefulSet) (scorecard.TestScore, error) {
@@ -51,7 +55,7 @@ func statefulSetHas(budgets []ks.PodDisruptionBudget) func(appsv1.StatefulSet) (
 			return
 		}
 
-		match, matchErr := hasMatching(budgets, statefulset.Namespace, statefulset.Spec.Template.Labels)
+		match, comment, matchErr := hasMatching(budgets, statefulset.Namespace, statefulset.Spec.Template.Labels)
 		if matchErr != nil {
 			err = matchErr
 			return
@@ -61,7 +65,7 @@ func statefulSetHas(budgets []ks.PodDisruptionBudget) func(appsv1.StatefulSet) (
 			score.Grade = scorecard.GradeAllOK
 		} else {
 			score.Grade = scorecard.GradeCritical
-			score.AddComment("", "No matching PodDisruptionBudget was found", "It's recommended to define a PodDisruptionBudget to avoid unexpected downtime during Kubernetes maintenance operations, such as when draining a node.")
+			score.AddComment("", "No matching PodDisruptionBudget was found", "It's recommended to define a PodDisruptionBudget to avoid unexpected downtime during Kubernetes maintenance operations, such as when draining a node. "+comment)
 		}
 
 		return
@@ -76,7 +80,7 @@ func deploymentHas(budgets []ks.PodDisruptionBudget) func(appsv1.Deployment) (sc
 			return
 		}
 
-		match, matchErr := hasMatching(budgets, deployment.Namespace, deployment.Spec.Template.Labels)
+		match, comment, matchErr := hasMatching(budgets, deployment.Namespace, deployment.Spec.Template.Labels)
 		if matchErr != nil {
 			err = matchErr
 			return
@@ -86,7 +90,7 @@ func deploymentHas(budgets []ks.PodDisruptionBudget) func(appsv1.Deployment) (sc
 			score.Grade = scorecard.GradeAllOK
 		} else {
 			score.Grade = scorecard.GradeCritical
-			score.AddComment("", "No matching PodDisruptionBudget was found", "It's recommended to define a PodDisruptionBudget to avoid unexpected downtime during Kubernetes maintenance operations, such as when draining a node.")
+			score.AddComment("", "No matching PodDisruptionBudget was found", "It's recommended to define a PodDisruptionBudget to avoid unexpected downtime during Kubernetes maintenance operations, such as when draining a node. "+comment)
 		}
 
 		return


Test patch:
diff --git a/score/poddisruptionbudget_test.go b/score/poddisruptionbudget_test.go
index 5d4992b..6b68fea 100644
--- a/score/poddisruptionbudget_test.go
+++ b/score/poddisruptionbudget_test.go
@@ -3,6 +3,8 @@ package score
 import (
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/assert"
 	"github.com/zegl/kube-score/scorecard"
 )
 
@@ -63,5 +65,32 @@ func TestDeploymentPodDisruptionBudgetV1Matches(t *testing.T) {
 
 func TestDeploymentPodDisruptionBudgetV1NoMatch(t *testing.T) {
 	t.Parallel()
-	testExpectedScore(t, "deployment-poddisruptionbudget-v1-no-match.yaml", "Deployment has PodDisruptionBudget", scorecard.GradeCritical)
+	actual := testExpectedScore(t, "deployment-poddisruptionbudget-v1-no-match.yaml", "Deployment has PodDisruptionBudget", scorecard.GradeCritical)
+
+	expected := []scorecard.TestScoreComment{
+		{
+			Path:        "",
+			Summary:     "No matching PodDisruptionBudget was found",
+			Description: "It's recommended to define a PodDisruptionBudget to avoid unexpected downtime during Kubernetes maintenance operations, such as when draining a node. ",
+		},
+	}
+
+	diff := cmp.Diff(expected, actual)
+	assert.Empty(t, diff)
+}
+
+func TestDeploymentPodDisruptionBudgetV1NoMatchMatchInOtherNamespace(t *testing.T) {
+	t.Parallel()
+	actual := testExpectedScore(t, "deployment-poddisruptionbudget-v1-different-namespace.yaml", "Deployment has PodDisruptionBudget", scorecard.GradeCritical)
+
+	expected := []scorecard.TestScoreComment{
+		{
+			Path:        "",
+			Summary:     "No matching PodDisruptionBudget was found",
+			Description: "It's recommended to define a PodDisruptionBudget to avoid unexpected downtime during Kubernetes maintenance operations, such as when draining a node. A matching budget was found, but in a different namespace. expected='foo' got='[not-foo bar]'",
+		},
+	}
+
+	diff := cmp.Diff(expected, actual)
+	assert.Empty(t, diff)
 }
diff --git a/score/testdata/deployment-poddisruptionbudget-v1-different-namespace.yaml b/score/testdata/deployment-poddisruptionbudget-v1-different-namespace.yaml
new file mode 100644
index 0000000..35d5f8d
--- /dev/null
+++ b/score/testdata/deployment-poddisruptionbudget-v1-different-namespace.yaml
@@ -0,0 +1,47 @@
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+  name: app-budget
+  namespace: not-foo
+spec:
+  minAvailable: 2
+  selector:
+    matchLabels:
+      app: foo
+---
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+  name: app-budget
+  namespace: bar
+spec:
+  minAvailable: 2
+  selector:
+    matchLabels:
+      app: foo
+---
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+  name: app-budget-not-matching
+  namespace: baz
+spec:
+  minAvailable: 2
+  selector:
+    matchLabels:
+      app: not-foo
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: statefulset-test-1
+  namespace: foo
+spec:
+  template:
+    metadata:
+      labels:
+        app: foo
+    spec:
+      containers:
+      - name: foobar
+        image: foo:bar
\ No newline at end of file