Repository: zegl/kube-score
Pull request #11 (2018-09-27 18:57:57)

Smarter comments


Patch:
diff --git a/cmd/kube-score/main.go b/cmd/kube-score/main.go
index 48f29e3..c97eb64 100644
--- a/cmd/kube-score/main.go
+++ b/cmd/kube-score/main.go
@@ -85,7 +85,19 @@ func main() {
 			color.New(col).Printf("    [%s] %s\n", status, card.Name)
 
 			for _, comment := range card.Comments {
-				fmt.Printf("        * %s\n", comment)
+				fmt.Printf("        * ")
+
+				if len(comment.Path) > 0 {
+					fmt.Printf("%s -> ", comment.Path)
+				}
+
+				fmt.Print(comment.Summary)
+
+				if len(comment.Description) > 0 {
+					fmt.Printf("\n             %s", comment.Description)
+				}
+
+				fmt.Println()
 			}
 
 			sumGrade += card.Grade
diff --git a/score/container.go b/score/container.go
index 48e3c6c..6511b5d 100644
--- a/score/container.go
+++ b/score/container.go
@@ -21,26 +21,26 @@ func scoreContainerLimits(podTemplate corev1.PodTemplateSpec) (score scorecard.T
 
 	for _, container := range allContainers {
 		if container.Resources.Limits.Cpu().IsZero() {
-			score.Comments = append(score.Comments, "CPU limit is not set")
+			score.AddComment(container.Name, "CPU limit is not set", "Resource limits are recommended to avoid resource DDOS")
 			hasMissingLimit = true
 		}
 		if container.Resources.Limits.Memory().IsZero() {
-			score.Comments = append(score.Comments, "Memory limit is not set")
+			score.AddComment(container.Name, "Memory limit is not set", "Resource limits are recommended to avoid resource DDOS")
 			hasMissingLimit = true
 		}
 		if container.Resources.Requests.Cpu().IsZero() {
-			score.Comments = append(score.Comments, "CPU request is not set")
+			score.AddComment(container.Name, "CPU request is not set", "Resource requests are recommended to make sure that the application can start and run without crashing")
 			hasMissingRequest = true
 		}
 		if container.Resources.Requests.Memory().IsZero() {
-			score.Comments = append(score.Comments, "Memory request is not set")
+			score.AddComment(container.Name, "Memory request is not set", "Resource requests are recommended to make sure that the application can start and run without crashing")
 			hasMissingRequest = true
 		}
 	}
 
 	if len(allContainers) == 0 {
 		score.Grade = 0
-		score.Comments = append(score.Comments, "No containers defined")
+		score.AddComment("", "No containers defined", "")
 	} else if hasMissingLimit {
 		score.Grade = 0
 	} else if hasMissingRequest {
@@ -67,7 +67,7 @@ func scoreContainerImageTag(podTemplate corev1.PodTemplateSpec) (score scorecard
 		imageVersion := imageParts[len(imageParts)-1]
 
 		if imageVersion == "latest" {
-			score.Comments = append(score.Comments, "Image with latest tag")
+			score.AddComment(container.Name, "Image with latest tag", "Using a fixed tag is recommended to avoid accidental upgrades")
 			hasTagLatest = true
 		}
 	}
@@ -93,7 +93,7 @@ func scoreContainerImagePullPolicy(podTemplate corev1.PodTemplateSpec) (score sc
 
 	for _, container := range allContainers{
 		if container.ImagePullPolicy != corev1.PullAlways {
-			score.Comments = append(score.Comments, "ImagePullPolicy is not set to PullAlways")
+			score.AddComment(container.Name, "ImagePullPolicy is not set to PullAlways", "It's recommended to always set the ImagePullPolicy to PullAlways, to make sure that the imagePullSecrets are always correct, and to always get the image you want.")
 			hasNonAlways = true
 		}
 	}
@@ -121,12 +121,12 @@ func scoreContainerProbes(podTemplate corev1.PodTemplateSpec) (score scorecard.T
 	for _, container := range allContainers {
 		if container.ReadinessProbe == nil  {
 			hasReadinessProbe = false
-			score.Comments = append(score.Comments, "Container is missing readinessProbe")
+			score.AddComment(container.Name, "Container is missing a readinessProbe", "Without a readinessProbe Services will start sending traffic to this pod before it's ready")
 		}
 
 		if container.LivenessProbe == nil {
 			hasLivenessProbe = false
-			score.Comments = append(score.Comments, "Container is missing livenessProbe")
+			score.AddComment(container.Name, "Container is missing a livenessProbe", "Without a livenessProbe kubelet can not restart the Pod if it has crashed")
 		}
 
 		if container.ReadinessProbe != nil && container.LivenessProbe != nil {
@@ -137,15 +137,15 @@ func scoreContainerProbes(podTemplate corev1.PodTemplateSpec) (score scorecard.T
 			if r.HTTPGet != nil && l.HTTPGet != nil {
 				if r.HTTPGet.Path == l.HTTPGet.Path &&
 					r.HTTPGet.Port.IntValue() == l.HTTPGet.Port.IntValue() {
-						probesAreIdentical = true
-					score.Comments = append(score.Comments, "Container has the same readiness and liveness probe")
+					probesAreIdentical = true
+					score.AddComment(container.Name, "Container has the same readiness and liveness probe", "It's recommended to have different probes for the two different purposes.")
 				}
 			}
 
 			if r.TCPSocket != nil && l.TCPSocket != nil {
 				if r.TCPSocket.Port == l.TCPSocket.Port {
 					probesAreIdentical = true
-					score.Comments = append(score.Comments, "Container has the same readiness and liveness probe")
+					score.AddComment(container.Name, "Container has the same readiness and liveness probe", "It's recommended to have different probes for the two different purposes.")
 				}
 			}
 
@@ -161,7 +161,7 @@ func scoreContainerProbes(podTemplate corev1.PodTemplateSpec) (score scorecard.T
 
 					if !hasDifferent {
 						probesAreIdentical = true
-						score.Comments = append(score.Comments, "Container has the same readiness and liveness probe")
+						score.AddComment(container.Name, "Container has the same readiness and liveness probe", "It's recommended to have different probes for the two different purposes.")
 					}
 				}
 			}
@@ -205,22 +205,22 @@ func scoreContainerSecurityContext(podTemplate corev1.PodTemplateSpec) (score sc
 
 		if sec.Privileged != nil && *sec.Privileged {
 			hasPrivileged = true
-			score.Comments = append(score.Comments, "The pod has a privileged container")
+			score.AddComment(container.Name, "The container is privileged", "")
 		}
 
 		if sec.ReadOnlyRootFilesystem != nil && *sec.ReadOnlyRootFilesystem == false {
 			hasWritableRootFS = true
-			score.Comments = append(score.Comments, "The pod has a container with a writable root filesystem")
+			score.AddComment(container.Name, "The pod has a container with a writable root filesystem", "")
 		}
 
 		if sec.RunAsUser != nil && *sec.RunAsUser < 10000 {
 			hasLowUserID = true
-			score.Comments = append(score.Comments, "The pod has a container running with a low user ID")
+			score.AddComment(container.Name, "The container is running with a low user ID", "A userid above 10 000 is recommended to avoid conflicts with the host")
 		}
 
 		if sec.RunAsGroup != nil && *sec.RunAsGroup < 10000 {
 			hasLowGroupID = true
-			score.Comments = append(score.Comments, "The pod has a container running with a low group ID")
+			score.AddComment(container.Name, "The container running with a low group ID", "A groupid above 10 000 is recommended to avoid conflicts with the host")
 		}
 	}
 
diff --git a/score/networkpolicy.go b/score/networkpolicy.go
index 9d33d61..76eec10 100644
--- a/score/networkpolicy.go
+++ b/score/networkpolicy.go
@@ -37,13 +37,13 @@ func scorePodHasNetworkPolicy(allNetpols []networkingv1.NetworkPolicy) func(spec
 			score.Grade = 10
 		} else if hasMatchingEgressNetpol && !hasMatchingIngressNetpol {
 			score.Grade = 5
-			score.Comments = append(score.Comments, "The pod does not have a matching ingress network policy")
+			score.AddComment("", "The pod does not have a matching ingress network policy", "")
 		} else if hasMatchingIngressNetpol && !hasMatchingEgressNetpol {
 			score.Grade = 5
-			score.Comments = append(score.Comments, "The pod does not have a matching egress network policy")
+			score.AddComment("", "The pod does not have a matching egress network policy", "")
 		} else {
 			score.Grade = 0
-			score.Comments = append(score.Comments, "The pod does not have a matching network policy")
+			score.AddComment("", "The pod does not have a matching network policy", "")
 		}
 
 		return
diff --git a/score/stableVersion.go b/score/stable_version.go
similarity index 69%
rename from score/stableVersion.go
rename to score/stable_version.go
index 96bc1fe..45272db 100644
--- a/score/stableVersion.go
+++ b/score/stable_version.go
@@ -1,10 +1,13 @@
 package score
 
-import "github.com/zegl/kube-score/scorecard"
+import (
+	"fmt"
+	"github.com/zegl/kube-score/scorecard"
+)
 import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 func scoreMetaStableAvailable(meta metav1.TypeMeta) (score scorecard.TestScore) {
-	score.Name = "Unstable version"
+	score.Name = "Stable version"
 
 	withStable := map[string]map[string]string {
 		"extensions/v1beta1": {
@@ -25,7 +28,10 @@ func scoreMetaStableAvailable(meta metav1.TypeMeta) (score scorecard.TestScore)
 	if inVersion, ok := withStable[meta.APIVersion]; ok  {
 		if recommendedVersion, ok := inVersion[meta.Kind]; ok {
 			score.Grade = 5
-			score.Comments = []string{"Recommended replacement: " + recommendedVersion}
+			score.AddComment("",
+				fmt.Sprintf("The apiVersion and kind %s/%s is deprecated", meta.APIVersion, meta.Kind),
+				fmt.Sprintf("It's recommended to use %s instead", recommendedVersion),
+			)
 			return
 		}
 	}
diff --git a/scorecard/scorecard.go b/scorecard/scorecard.go
index 4e1dd42..83ae876 100644
--- a/scorecard/scorecard.go
+++ b/scorecard/scorecard.go
@@ -36,7 +36,21 @@ type TestScore struct {
 	}
 
 	Grade       int
-	Comments    []string
+	Comments    []TestScoreComment
+}
+
+type TestScoreComment struct {
+	Path string
+	Summary string
+	Description string
+}
+
+func (ts *TestScore) AddComment(path, summary, description string) {
+	ts.Comments = append(ts.Comments, TestScoreComment{
+		Path: path,
+		Summary: summary,
+		Description: description,
+	})
 }
 
 func (ts *TestScore) AddMeta(typeMeta metav1.TypeMeta, objectMeta metav1.ObjectMeta) {


Test patch:
diff --git a/score/score_test.go b/score/score_test.go
index a8974b6..6c62126 100644
--- a/score/score_test.go
+++ b/score/score_test.go
@@ -51,14 +51,6 @@ func TestStatefulSetResources(t *testing.T) {
 	testExpectedScore(t, "statefulset-test-resources.yaml", "Container Resources", 5)
 }
 
-func TestStatefulSetAppsv1beta1(t *testing.T) {
-	testExpectedScore(t, "statefulset-appsv1beta1.yaml", "Unstable version", 5)
-}
-
-func TestStatefulSetAppsv1beta2(t *testing.T) {
-	testExpectedScore(t, "statefulset-appsv1beta2.yaml", "Unstable version", 5)
-}
-
 func TestPodContainerTagLatest(t *testing.T) {
 	testExpectedScore(t, "pod-image-tag-latest.yaml", "Container Image Tag", 0)
 }
@@ -115,38 +107,6 @@ func TestPodProbesIdenticalExec(t *testing.T) {
 	testExpectedScore(t, "pod-probes-identical-exec.yaml", "Pod Probes", 7)
 }
 
-func TestDeploymentExtensionsv1beta1(t *testing.T) {
-	testExpectedScore(t, "deployment-extensions-v1beta1.yaml", "Unstable version", 5)
-}
-
-func TestDeploymentAppsv1beta1(t *testing.T) {
-	testExpectedScore(t, "deployment-appsv1beta1.yaml", "Unstable version", 5)
-}
-
-func TestDeploymentAppsv1beta2(t *testing.T) {
-	testExpectedScore(t, "deployment-appsv1beta2.yaml", "Unstable version", 5)
-}
-
-func TestDaemonSetAppsv1(t *testing.T) {
-	testExpectedScore(t, "daemonset-appsv1.yaml", "Unstable version", 10)
-}
-
-func TestDaemonSetAppsv1beta2(t *testing.T) {
-	testExpectedScore(t, "daemonset-appsv1beta2.yaml", "Unstable version", 5)
-}
-
-func TestDaemonSetExtensionsv1beta1(t *testing.T) {
-	testExpectedScore(t, "daemonset-extensionsv1beta1.yaml", "Unstable version", 5)
-}
-
-func TestCronJobBatchv1beta1(t *testing.T) {
-	testExpectedScore(t, "cronjob-batchv1beta1.yaml", "Unstable version", 10)
-}
-
-func TestJobBatchv1(t *testing.T) {
-	testExpectedScore(t, "job-batchv1.yaml", "Unstable version", 10)
-}
-
 func TestContainerSecurityContextPrivilegied(t *testing.T) {
 	testExpectedScore(t, "pod-security-context-privilegied.yaml", "Container Security Context", 0)
 }
diff --git a/score/stable_version_test.go b/score/stable_version_test.go
new file mode 100644
index 0000000..8324804
--- /dev/null
+++ b/score/stable_version_test.go
@@ -0,0 +1,44 @@
+package score
+
+import "testing"
+
+
+func TestStatefulSetAppsv1beta1(t *testing.T) {
+	testExpectedScore(t, "statefulset-appsv1beta1.yaml", "Stable version", 5)
+}
+
+func TestStatefulSetAppsv1beta2(t *testing.T) {
+	testExpectedScore(t, "statefulset-appsv1beta2.yaml", "Stable version", 5)
+}
+
+func TestDeploymentExtensionsv1beta1(t *testing.T) {
+	testExpectedScore(t, "deployment-extensions-v1beta1.yaml", "Stable version", 5)
+}
+
+func TestDeploymentAppsv1beta1(t *testing.T) {
+	testExpectedScore(t, "deployment-appsv1beta1.yaml", "Stable version", 5)
+}
+
+func TestDeploymentAppsv1beta2(t *testing.T) {
+	testExpectedScore(t, "deployment-appsv1beta2.yaml", "Stable version", 5)
+}
+
+func TestDaemonSetAppsv1(t *testing.T) {
+	testExpectedScore(t, "daemonset-appsv1.yaml", "Stable version", 10)
+}
+
+func TestDaemonSetAppsv1beta2(t *testing.T) {
+	testExpectedScore(t, "daemonset-appsv1beta2.yaml", "Stable version", 5)
+}
+
+func TestDaemonSetExtensionsv1beta1(t *testing.T) {
+	testExpectedScore(t, "daemonset-extensionsv1beta1.yaml", "Stable version", 5)
+}
+
+func TestCronJobBatchv1beta1(t *testing.T) {
+	testExpectedScore(t, "cronjob-batchv1beta1.yaml", "Stable version", 10)
+}
+
+func TestJobBatchv1(t *testing.T) {
+	testExpectedScore(t, "job-batchv1.yaml", "Stable version", 10)
+}