Repository: zegl/kube-score
Pull request #118 (2019-04-27 15:33:45)

score: propagate errors instead of panicing


Patch:
diff --git a/cmd/kube-score/main.go b/cmd/kube-score/main.go
index 0a8cc74..95a5646 100644
--- a/cmd/kube-score/main.go
+++ b/cmd/kube-score/main.go
@@ -137,12 +137,12 @@ Use "-" as filename to read from STDIN.`)
 
 	parsedFiles, err := parser.ParseFiles(cnf)
 	if err != nil {
-		panic(err)
+		return err
 	}
 
 	scoreCard, err := score.Score(parsedFiles, cnf)
 	if err != nil {
-		panic(err)
+		return err
 	}
 
 	ignoredTests := make(map[string]struct{})
@@ -156,7 +156,7 @@ Use "-" as filename to read from STDIN.`)
 	// Detect which output format we should use
 	humanOutput := *outputFormat == "human"
 
-	for _, scoredObject := range scoreCard {
+	for _, scoredObject := range *scoreCard {
 		// Headers for each object
 		if humanOutput {
 			color.New(color.FgMagenta).Printf("%s/%s %s", scoredObject.TypeMeta.APIVersion, scoredObject.TypeMeta.Kind, scoredObject.ObjectMeta.Name)
diff --git a/score/apps/apps.go b/score/apps/apps.go
index 11078fa..c47ea85 100644
--- a/score/apps/apps.go
+++ b/score/apps/apps.go
@@ -14,7 +14,7 @@ func Register(allChecks *checks.Checks) {
 	allChecks.RegisterStatefulSetCheck("StatefulSet has host PodAntiAffinity", "Makes sure that a podAntiAffinity has been set that prevents multiple pods from being scheduled on the same node. https://kubernetes.io/docs/concepts/configuration/assign-pod-node/", statefulsetHasAntiAffinity)
 }
 
-func deploymentHasAntiAffinity(deployment appsv1.Deployment) (score scorecard.TestScore) {
+func deploymentHasAntiAffinity(deployment appsv1.Deployment) (score scorecard.TestScore, err error) {
 	// Ignore if the deployment only has a single replica
 	// If replicas is not explicitly set, we'll still warn if the anti affinity is missing
 	// as that might indicate use of a Horizontal Pod Autoscaler
@@ -46,7 +46,7 @@ func deploymentHasAntiAffinity(deployment appsv1.Deployment) (score scorecard.Te
 	return
 }
 
-func statefulsetHasAntiAffinity(statefulset appsv1.StatefulSet) (score scorecard.TestScore) {
+func statefulsetHasAntiAffinity(statefulset appsv1.StatefulSet) (score scorecard.TestScore, err error) {
 	// Ignore if the statefulset only has a single replica
 	// If replicas is not explicitly set, we'll still warn if the anti affinity is missing
 	// as that might indicate use of a Horizontal Pod Autoscaler
diff --git a/score/checks/checks.go b/score/checks/checks.go
index 61bd5b4..3defe5e 100644
--- a/score/checks/checks.go
+++ b/score/checks/checks.go
@@ -58,12 +58,12 @@ type ServiceCheck struct {
 
 type StatefulSetCheck struct {
 	ks.Check
-	Fn func(appsv1.StatefulSet) scorecard.TestScore
+	Fn func(appsv1.StatefulSet) (scorecard.TestScore, error)
 }
 
 type DeploymentCheck struct {
 	ks.Check
-	Fn func(appsv1.Deployment) scorecard.TestScore
+	Fn func(appsv1.Deployment) (scorecard.TestScore, error)
 }
 
 type NetworkPolicyCheck struct {
@@ -123,7 +123,7 @@ func (c *Checks) CronJobs() map[string]CronJobCheck {
 	return c.cronjobs
 }
 
-func (c *Checks) RegisterStatefulSetCheck(name, comment string, fn func(appsv1.StatefulSet) scorecard.TestScore) {
+func (c *Checks) RegisterStatefulSetCheck(name, comment string, fn func(appsv1.StatefulSet) (scorecard.TestScore, error)) {
 	ch := NewCheck(name, "StatefulSet", comment)
 	c.all = append(c.all, ch)
 	c.statefulsets[machineFriendlyName(name)] = StatefulSetCheck{ch, fn}
@@ -133,7 +133,7 @@ func (c *Checks) StatefulSets() map[string]StatefulSetCheck {
 	return c.statefulsets
 }
 
-func (c *Checks) RegisterDeploymentCheck(name, comment string, fn func(appsv1.Deployment) scorecard.TestScore) {
+func (c *Checks) RegisterDeploymentCheck(name, comment string, fn func(appsv1.Deployment) (scorecard.TestScore, error)) {
 	ch := NewCheck(name, "Deployment", comment)
 	c.all = append(c.all, ch)
 	c.deployments[machineFriendlyName(name)] = DeploymentCheck{ch, fn}
diff --git a/score/disruptionbudget/disruptionbudget.go b/score/disruptionbudget/disruptionbudget.go
index 4ed68a9..9ec1fd6 100644
--- a/score/disruptionbudget/disruptionbudget.go
+++ b/score/disruptionbudget/disruptionbudget.go
@@ -1,6 +1,8 @@
 package disruptionbudget
 
 import (
+	"fmt"
+
 	ks "github.com/zegl/kube-score/domain"
 	"github.com/zegl/kube-score/score/checks"
 	"github.com/zegl/kube-score/score/internal"
@@ -16,7 +18,7 @@ func Register(allChecks *checks.Checks, budgets ks.PodDisruptionBudgets) {
 	allChecks.RegisterDeploymentCheck("Deployment has PodDisruptionBudget", `Makes sure that all Deployments are targeted by a PDB`, deploymentHas(budgets.PodDisruptionBudgets()))
 }
 
-func hasMatching(budgets []policyv1beta1.PodDisruptionBudget, namespace string, lables map[string]string) bool {
+func hasMatching(budgets []policyv1beta1.PodDisruptionBudget, namespace string, lables map[string]string) (bool, error) {
 	for _, budget := range budgets {
 		if budget.Namespace != namespace {
 			continue
@@ -24,26 +26,32 @@ func hasMatching(budgets []policyv1beta1.PodDisruptionBudget, namespace string,
 
 		selector, err := metav1.LabelSelectorAsSelector(budget.Spec.Selector)
 		if err != nil {
-			panic(err)
+			return false, fmt.Errorf("failed to create selector: %v", err)
 		}
 
 		if selector.Matches(internal.MapLables(lables)) {
-			return true
+			return true, nil
 		}
 	}
 
-	return false
+	return false, nil
 }
 
-func statefulSetHas(budgets []policyv1beta1.PodDisruptionBudget) func(appsv1.StatefulSet) scorecard.TestScore {
-	return func(statefulset appsv1.StatefulSet) (score scorecard.TestScore) {
+func statefulSetHas(budgets []policyv1beta1.PodDisruptionBudget) func(appsv1.StatefulSet) (scorecard.TestScore, error) {
+	return func(statefulset appsv1.StatefulSet) (score scorecard.TestScore, err error) {
 		if statefulset.Spec.Replicas != nil && *statefulset.Spec.Replicas < 2 {
 			score.Grade = scorecard.GradeAllOK
 			score.AddComment("", "Skipped", "Skipped because the statefulset has less than 2 replicas")
 			return
 		}
 
-		if hasMatching(budgets, statefulset.Namespace, statefulset.Spec.Template.Labels) {
+		match, matchErr := hasMatching(budgets, statefulset.Namespace, statefulset.Spec.Template.Labels)
+		if matchErr != nil {
+			err = matchErr
+			return
+		}
+
+		if match {
 			score.Grade = scorecard.GradeAllOK
 		} else {
 			score.Grade = scorecard.GradeCritical
@@ -54,15 +62,21 @@ func statefulSetHas(budgets []policyv1beta1.PodDisruptionBudget) func(appsv1.Sta
 	}
 }
 
-func deploymentHas(budgets []policyv1beta1.PodDisruptionBudget) func(appsv1.Deployment) scorecard.TestScore {
-	return func(deployment appsv1.Deployment) (score scorecard.TestScore) {
+func deploymentHas(budgets []policyv1beta1.PodDisruptionBudget) func(appsv1.Deployment) (scorecard.TestScore, error) {
+	return func(deployment appsv1.Deployment) (score scorecard.TestScore, err error) {
 		if deployment.Spec.Replicas != nil && *deployment.Spec.Replicas < 2 {
 			score.Grade = scorecard.GradeAllOK
 			score.AddComment("", "Skipped", "Skipped because the deployment has less than 2 replicas")
 			return
 		}
 
-		if hasMatching(budgets, deployment.Namespace, deployment.Spec.Template.Labels) {
+		match, matchErr := hasMatching(budgets, deployment.Namespace, deployment.Spec.Template.Labels)
+		if matchErr != nil {
+			err = matchErr
+			return
+		}
+
+		if match {
 			score.Grade = scorecard.GradeAllOK
 		} else {
 			score.Grade = scorecard.GradeCritical
diff --git a/score/score.go b/score/score.go
index d22bad8..263821a 100644
--- a/score/score.go
+++ b/score/score.go
@@ -38,7 +38,7 @@ func RegisterAllChecks(allObjects ks.AllTypes, cnf config.Configuration) *checks
 
 // Score runs a pre-configured list of tests against the files defined in the configuration, and returns a scorecard.
 // Additional configuration and tuning parameters can be provided via the config.
-func Score(allObjects ks.AllTypes, cnf config.Configuration) (scorecard.Scorecard, error) {
+func Score(allObjects ks.AllTypes, cnf config.Configuration) (*scorecard.Scorecard, error) {
 	allChecks := RegisterAllChecks(allObjects, cnf)
 	scoreCard := scorecard.New()
 
@@ -85,14 +85,22 @@ func Score(allObjects ks.AllTypes, cnf config.Configuration) (scorecard.Scorecar
 	for _, statefulset := range allObjects.StatefulSets() {
 		o := scoreCard.NewObject(statefulset.TypeMeta, statefulset.ObjectMeta)
 		for _, test := range allChecks.StatefulSets() {
-			o.Add(test.Fn(statefulset), test.Check)
+			res, err := test.Fn(statefulset)
+			if err != nil {
+				return nil, err
+			}
+			o.Add(res, test.Check)
 		}
 	}
 
 	for _, deployment := range allObjects.Deployments() {
 		o := scoreCard.NewObject(deployment.TypeMeta, deployment.ObjectMeta)
 		for _, test := range allChecks.Deployments() {
-			o.Add(test.Fn(deployment), test.Check)
+			res, err := test.Fn(deployment)
+			if err != nil {
+				return nil, err
+			}
+			o.Add(res, test.Check)
 		}
 	}
 
@@ -110,5 +118,5 @@ func Score(allObjects ks.AllTypes, cnf config.Configuration) (scorecard.Scorecar
 		}
 	}
 
-	return scoreCard, nil
+	return &scoreCard, nil
 }


Test patch:
diff --git a/score/apps/apps_test.go b/score/apps/apps_test.go
index f870164..2d5cede 100644
--- a/score/apps/apps_test.go
+++ b/score/apps/apps_test.go
@@ -113,7 +113,8 @@ func TestStatefulsetHasAntiAffinity(t *testing.T) {
 			},
 		}
 
-		score := statefulsetHasAntiAffinity(s)
+		score, err := statefulsetHasAntiAffinity(s)
+		assert.Nil(t, err)
 		assert.Equal(t, tc.expectedGrade, score.Grade, "caseID=%d", caseID)
 	}
 }
@@ -136,7 +137,8 @@ func TestDeploymentHasAntiAffinity(t *testing.T) {
 			},
 		}
 
-		score := deploymentHasAntiAffinity(s)
+		score, err := deploymentHasAntiAffinity(s)
+		assert.Nil(t, err)
 		assert.Equal(t, tc.expectedGrade, score.Grade, "caseID=%d", caseID)
 	}
 }
diff --git a/score/disruptionbudget/disruptionbudget_test.go b/score/disruptionbudget/disruptionbudget_test.go
index 7fe7bb0..e033d43 100644
--- a/score/disruptionbudget/disruptionbudget_test.go
+++ b/score/disruptionbudget/disruptionbudget_test.go
@@ -17,7 +17,8 @@ func TestStatefulSetReplicas(t *testing.T) {
 	fn := statefulSetHas(nil)
 
 	for replicas, expected := range cases {
-		res := fn(appsv1.StatefulSet{Spec: appsv1.StatefulSetSpec{Replicas: replicas}})
+		res, err := fn(appsv1.StatefulSet{Spec: appsv1.StatefulSetSpec{Replicas: replicas}})
+		assert.Nil(t, err)
 
 		if replicas == nil {
 			assert.Equal(t, expected, res.Grade, "replicas=nil")
@@ -37,7 +38,8 @@ func TestDeploymentReplicas(t *testing.T) {
 	fn := deploymentHas(nil)
 
 	for replicas, expected := range cases {
-		res := fn(appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: replicas}})
+		res, err := fn(appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: replicas}})
+		assert.Nil(t, err)
 
 		if replicas == nil {
 			assert.Equal(t, expected, res.Grade, "replicas=nil")
diff --git a/score/score_test.go b/score/score_test.go
index ca0c2da..a566c6f 100644
--- a/score/score_test.go
+++ b/score/score_test.go
@@ -45,7 +45,12 @@ func testScore(config config.Configuration) (scorecard.Scorecard, error) {
 		return nil, err
 	}
 
-	return Score(parsed, config)
+	card, err := Score(parsed, config)
+	if err != nil {
+		return nil, err
+	}
+
+	return *card, err
 }
 
 func testExpectedScore(t *testing.T, filename string, testcase string, expectedScore scorecard.Grade) []scorecard.TestScoreComment {