Repository: zegl/kube-score
Pull request #37 (2018-10-11 19:36:03)

Probes improvements


Patch:
diff --git a/score/container.go b/score/container.go
index eec23f8..6552fe8 100644
--- a/score/container.go
+++ b/score/container.go
@@ -109,103 +109,6 @@ func scoreContainerImagePullPolicy(podTemplate corev1.PodTemplateSpec) (score sc
 	return
 }
 
-func scoreContainerProbes(allServices []corev1.Service) func(corev1.PodTemplateSpec) scorecard.TestScore {
-	return func(podTemplate corev1.PodTemplateSpec) (score scorecard.TestScore) {
-		score.Name = "Pod Probes"
-
-		allContainers := podTemplate.Spec.InitContainers
-		allContainers = append(allContainers, podTemplate.Spec.Containers...)
-
-		hasReadinessProbe := false
-		hasLivenessProbe := false
-		probesAreIdentical := false
-		isTargetedByService := false
-
-		for _, service := range allServices {
-			if podTemplate.Namespace == service.Namespace {
-				for selectorKey, selectorVal := range service.Spec.Selector {
-					if podLabelVal, ok := podTemplate.Labels[selectorKey]; ok && podLabelVal == selectorVal {
-						isTargetedByService = true
-					}
-				}
-			}
-		}
-
-		for _, container := range allContainers {
-			if container.ReadinessProbe != nil {
-				hasReadinessProbe = true
-			} else {
-				if isTargetedByService {
-					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 = true
-			} else {
-				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 {
-
-				r := container.ReadinessProbe
-				l := container.LivenessProbe
-
-				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.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.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.Exec != nil && l.Exec != nil {
-					if len(r.Exec.Command) == len(l.Exec.Command) {
-						hasDifferent := false
-						for i, v := range r.Exec.Command {
-							if l.Exec.Command[i] != v {
-								hasDifferent = true
-								break
-							}
-						}
-
-						if !hasDifferent {
-							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 hasLivenessProbe && (hasReadinessProbe || !isTargetedByService) {
-			if !probesAreIdentical {
-				score.Grade = 10
-			} else {
-				score.Grade = 7
-			}
-		} else if !hasReadinessProbe && !hasLivenessProbe {
-			score.Grade = 0
-		} else if isTargetedByService && !hasReadinessProbe {
-			score.Grade = 0
-		} else if !hasLivenessProbe {
-			score.Grade = 5
-		} else {
-			score.Grade = 0
-		}
-
-		return score
-	}
-}
-
 func scoreContainerSecurityContext(podTemplate corev1.PodTemplateSpec) (score scorecard.TestScore) {
 	score.Name = "Container Security Context"
 
diff --git a/score/probes.go b/score/probes.go
new file mode 100644
index 0000000..4afc30c
--- /dev/null
+++ b/score/probes.go
@@ -0,0 +1,97 @@
+package score
+
+import (
+	"github.com/zegl/kube-score/scorecard"
+	corev1 "k8s.io/api/core/v1"
+)
+
+func scoreContainerProbes(allServices []corev1.Service) func(corev1.PodTemplateSpec) scorecard.TestScore {
+	return func(podTemplate corev1.PodTemplateSpec) (score scorecard.TestScore) {
+		score.Name = "Pod Probes"
+
+		allContainers := podTemplate.Spec.InitContainers
+		allContainers = append(allContainers, podTemplate.Spec.Containers...)
+
+		hasReadinessProbe := false
+		hasLivenessProbe := false
+		probesAreIdentical := false
+		isTargetedByService := false
+
+		for _, service := range allServices {
+			if podTemplate.Namespace == service.Namespace {
+				for selectorKey, selectorVal := range service.Spec.Selector {
+					if podLabelVal, ok := podTemplate.Labels[selectorKey]; ok && podLabelVal == selectorVal {
+						isTargetedByService = true
+					}
+				}
+			}
+		}
+
+		for _, container := range allContainers {
+			if container.ReadinessProbe != nil {
+				hasReadinessProbe = true
+			}
+
+			if container.LivenessProbe != nil {
+				hasLivenessProbe = true
+			}
+
+			if container.ReadinessProbe != nil && container.LivenessProbe != nil {
+
+				r := container.ReadinessProbe
+				l := container.LivenessProbe
+
+				if r.HTTPGet != nil && l.HTTPGet != nil {
+					if r.HTTPGet.Path == l.HTTPGet.Path &&
+						r.HTTPGet.Port.IntValue() == l.HTTPGet.Port.IntValue() {
+						probesAreIdentical = true
+					}
+				}
+
+				if r.TCPSocket != nil && l.TCPSocket != nil {
+					if r.TCPSocket.Port == l.TCPSocket.Port {
+						probesAreIdentical = true
+					}
+				}
+
+				if r.Exec != nil && l.Exec != nil {
+					if len(r.Exec.Command) == len(l.Exec.Command) {
+						hasDifferent := false
+						for i, v := range r.Exec.Command {
+							if l.Exec.Command[i] != v {
+								hasDifferent = true
+								break
+							}
+						}
+
+						if !hasDifferent {
+							probesAreIdentical = true
+						}
+					}
+				}
+
+			}
+		}
+
+		if hasLivenessProbe && (hasReadinessProbe || !isTargetedByService) {
+			if !probesAreIdentical {
+				score.Grade = 10
+			} else {
+				score.Grade = 7
+				score.AddComment("", "Pod has the same readiness and liveness probe", "It's recommended to have different probes for the two different purposes.")
+			}
+		} else if !hasReadinessProbe && !hasLivenessProbe {
+			score.Grade = 0
+			score.AddComment("", "Container is missing a readinessProbe", "Without a readinessProbe Services will start sending traffic to this pod before it's ready")
+			score.AddComment("", "Container is missing a livenessProbe", "Without a livenessProbe kubelet can not restart the Pod if it has crashed")
+		} else if isTargetedByService && !hasReadinessProbe {
+			score.Grade = 0
+			score.AddComment("", "Container is missing a readinessProbe", "Without a readinessProbe Services will start sending traffic to this pod before it's ready")
+		} else if !hasLivenessProbe {
+			score.Grade = 5
+			score.AddComment("", "Pod is missing a livenessProbe", "Without a livenessProbe kubelet can not restart the Pod if it has crashed")
+		}
+
+		return score
+	}
+}


Test patch:
diff --git a/score/probe_test.go b/score/probe_test.go
new file mode 100644
index 0000000..bbfeca2
--- /dev/null
+++ b/score/probe_test.go
@@ -0,0 +1,69 @@
+package score
+
+import (
+	"github.com/stretchr/testify/assert"
+	"testing"
+)
+
+func TestProbesPodAllMissing(t *testing.T) {
+	comments := testExpectedScore(t, "pod-probes-all-missing.yaml", "Pod Probes", 0)
+	assert.Len(t, comments, 2)
+	assert.Equal(t, "Container is missing a readinessProbe", comments[0].Summary)
+	assert.Equal(t, "Container is missing a livenessProbe", comments[1].Summary)
+}
+
+func TestProbesPodMissingReady(t *testing.T) {
+	// not targeted by service, expected full score
+	comments := testExpectedScore(t, "pod-probes-missing-ready.yaml", "Pod Probes", 10)
+	assert.Len(t, comments, 0)
+}
+
+func TestProbesPodIdenticalHTTP(t *testing.T) {
+	comments := testExpectedScore(t, "pod-probes-identical-http.yaml", "Pod Probes", 7)
+	assert.Len(t, comments, 1)
+	assert.Equal(t, "Pod has the same readiness and liveness probe", comments[0].Summary)
+}
+
+func TestProbesPodIdenticalTCP(t *testing.T) {
+	comments := testExpectedScore(t, "pod-probes-identical-tcp.yaml", "Pod Probes", 7)
+	assert.Len(t, comments, 1)
+	assert.Equal(t, "Pod has the same readiness and liveness probe", comments[0].Summary)
+}
+
+func TestProbesPodIdenticalExec(t *testing.T) {
+	comments := testExpectedScore(t, "pod-probes-identical-exec.yaml", "Pod Probes", 7)
+	assert.Len(t, comments, 1)
+	assert.Equal(t, "Pod has the same readiness and liveness probe", comments[0].Summary)
+}
+
+func TestProbesTargetedByService(t *testing.T) {
+	comments := testExpectedScore(t, "pod-probes-targeted-by-service.yaml", "Pod Probes", 0)
+	assert.Len(t, comments, 1)
+	assert.Equal(t, "Container is missing a readinessProbe", comments[0].Summary)
+}
+
+func TestProbesTargetedByServiceSameNamespace(t *testing.T) {
+	comments := testExpectedScore(t, "pod-probes-targeted-by-service-same-namespace.yaml", "Pod Probes", 0)
+	assert.Len(t, comments, 1)
+	assert.Equal(t, "Container is missing a readinessProbe", comments[0].Summary)
+}
+
+func TestProbesTargetedByServiceDifferentNamespace(t *testing.T) {
+	comments := testExpectedScore(t, "pod-probes-targeted-by-service-different-namespace.yaml", "Pod Probes", 10)
+	assert.Len(t, comments, 0)
+}
+
+func TestProbesTargetedByServiceNotTargeted(t *testing.T) {
+	comments := testExpectedScore(t, "pod-probes-not-targeted-by-service.yaml", "Pod Probes", 10)
+	assert.Len(t, comments, 0)
+}
+
+func TestProbesMultipleContainers(t *testing.T) {
+	comments := testExpectedScore(t, "pod-probes-on-different-containers.yaml", "Pod Probes", 10)
+	assert.Len(t, comments, 0)
+}
+
+func TestProbesMultipleContainersInit(t *testing.T) {
+	comments := testExpectedScore(t, "pod-probes-on-different-containers-init.yaml", "Pod Probes", 10)
+	assert.Len(t, comments, 0)
+}
diff --git a/score/score_test.go b/score/score_test.go
index e4f851e..bed5978 100644
--- a/score/score_test.go
+++ b/score/score_test.go
@@ -17,39 +17,28 @@ func testFile(name string) *os.File {
 	return fp
 }
 
-func testExpectedScoreWithConfig(t *testing.T, config Configuration, filename string, testcase string, expectedScore int) {
+// testExpectedScoreWithConfig runs all tests, but makes sure that the test for "testcase" was executed, and that
+// the grade is set to expectedScore. The function returns the comments of "testcase".
+func testExpectedScoreWithConfig(t *testing.T, config Configuration, filename string, testcase string, expectedScore int) []scorecard.TestScoreComment {
 	config.AllFiles = []io.Reader{testFile(filename)}
 	sc, err := Score(config)
 	assert.NoError(t, err)
-	tested := false
+
 	for _, objectScore := range sc.Scores {
 		for _, s := range objectScore {
 			if s.Name == testcase {
 				assert.Equal(t, expectedScore, s.Grade)
-				tested = true
+				return s.Comments
 			}
 		}
 	}
-	assert.True(t, tested, "Was not tested")
-}
 
-func testExpectedScore(t *testing.T, filename string, testcase string, expectedScore int) {
-	testExpectedScoreWithConfig(t, Configuration{}, filename, testcase, expectedScore)
+	t.Error("Was not tested")
+	return nil
 }
 
-func testGetComments(t *testing.T, filename, testcase string) []scorecard.TestScoreComment {
-	sc, err := Score(Configuration{
-		AllFiles: []io.Reader{testFile(filename)}})
-	assert.NoError(t, err)
-	for _, objectScore := range sc.Scores {
-		for _, s := range objectScore {
-			if s.Name == testcase {
-				return s.Comments
-			}
-		}
-	}
-	t.Fatalf("Testcase %s was not run", testcase)
-	return nil
+func testExpectedScore(t *testing.T, filename string, testcase string, expectedScore int) []scorecard.TestScoreComment {
+	return testExpectedScoreWithConfig(t, Configuration{}, filename, testcase, expectedScore)
 }
 
 func TestPodContainerNoResources(t *testing.T) {
@@ -116,52 +105,6 @@ func TestPodHasMatchingEgressNetworkPolicy(t *testing.T) {
 	testExpectedScore(t, "networkpolicy-matching-only-egress.yaml", "Pod NetworkPolicy", 5)
 }
 
-func TestPodProbesAllMissing(t *testing.T) {
-	testExpectedScore(t, "pod-probes-all-missing.yaml", "Pod Probes", 0)
-}
-
-func TestPodProbesMissingReady(t *testing.T) {
-	testExpectedScore(t, "pod-probes-missing-ready.yaml", "Pod Probes", 10)
-}
-
-func TestPodProbesIdenticalHTTP(t *testing.T) {
-	testExpectedScore(t, "pod-probes-identical-http.yaml", "Pod Probes", 7)
-}
-
-func TestPodProbesIdenticalTCP(t *testing.T) {
-	testExpectedScore(t, "pod-probes-identical-tcp.yaml", "Pod Probes", 7)
-}
-
-func TestPodProbesIdenticalExec(t *testing.T) {
-	testExpectedScore(t, "pod-probes-identical-exec.yaml", "Pod Probes", 7)
-}
-
-func TestProbesTargetedByService(t *testing.T) {
-	comments := testGetComments(t, "pod-probes-targeted-by-service.yaml", "Pod Probes")
-	assert.Len(t, comments, 1)
-	assert.Equal(t, "Container is missing a readinessProbe", comments[0].Summary)
-
-	testExpectedScore(t, "pod-probes-targeted-by-service.yaml", "Pod Probes", 0)
-}
-
-func TestProbesTargetedByServiceSameNamespace(t *testing.T) {
-	comments := testGetComments(t, "pod-probes-targeted-by-service-same-namespace.yaml", "Pod Probes")
-	assert.Len(t, comments, 1)
-	assert.Equal(t, "Container is missing a readinessProbe", comments[0].Summary)
-
-	testExpectedScore(t, "pod-probes-targeted-by-service-same-namespace.yaml", "Pod Probes", 0)
-}
-
-func TestProbesTargetedByServiceDifferentNamespace(t *testing.T) {
-	comments := testGetComments(t, "pod-probes-targeted-by-service-different-namespace.yaml", "Pod Probes")
-	assert.Len(t, comments, 0)
-	testExpectedScore(t, "pod-probes-targeted-by-service-different-namespace.yaml", "Pod Probes", 10)
-}
-
-func TestProbesTargetedByServiceNotTargeted(t *testing.T) {
-	testExpectedScore(t, "pod-probes-not-targeted-by-service.yaml", "Pod Probes", 10)
-}
-
 func TestContainerSecurityContextPrivilegied(t *testing.T) {
 	testExpectedScore(t, "pod-security-context-privilegied.yaml", "Container Security Context", 0)
 }
diff --git a/score/testdata/pod-probes-on-different-containers-init.yaml b/score/testdata/pod-probes-on-different-containers-init.yaml
new file mode 100644
index 0000000..2988cab
--- /dev/null
+++ b/score/testdata/pod-probes-on-different-containers-init.yaml
@@ -0,0 +1,19 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod-test-1
+spec:
+  initContainers:
+  - name: foobar2
+    image: foo/bar:latest
+  containers:
+  - name: foobar
+    image: foo/bar:latest
+    readinessProbe:
+      httpGet:
+        path: /ready
+        port: 8080
+    livenessProbe:
+      httpGet:
+        path: /live
+        port: 8080
\ No newline at end of file
diff --git a/score/testdata/pod-probes-on-different-containers.yaml b/score/testdata/pod-probes-on-different-containers.yaml
new file mode 100644
index 0000000..f11a078
--- /dev/null
+++ b/score/testdata/pod-probes-on-different-containers.yaml
@@ -0,0 +1,18 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod-test-1
+spec:
+  containers:
+  - name: foobar
+    image: foo/bar:latest
+    readinessProbe:
+      httpGet:
+        path: /ready
+        port: 8080
+    livenessProbe:
+      httpGet:
+        path: /live
+        port: 8080
+  - name: foobar2
+    image: foo/bar:latest
\ No newline at end of file