Skip to content

Commit

Permalink
score: don't warn for missing readinessProbes if the pod is not targe…
Browse files Browse the repository at this point in the history 
…ted by a service

Fixes #12
  • Loading branch information
@zegl
zegl committed Oct 7, 2018
1 parent 55591d9 commit fecb240
Show file tree
Hide file tree
Showing 5 changed files with 178 additions and 71 deletions.
130 changes: 74 additions & 56 deletions score/container.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func scoreContainerImageTag(podTemplate corev1.PodTemplateSpec) (score scorecard

hasTagLatest := false

for _, container := range allContainers{
for _, container := range allContainers {
imageParts := strings.Split(container.Image, ":")
imageVersion := imageParts[len(imageParts)-1]

Expand Down Expand Up @@ -91,7 +91,7 @@ func scoreContainerImagePullPolicy(podTemplate corev1.PodTemplateSpec) (score sc

hasNonAlways := false

for _, container := range allContainers{
for _, container := range allContainers {
if container.ImagePullPolicy != corev1.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
Expand All @@ -107,81 +107,99 @@ func scoreContainerImagePullPolicy(podTemplate corev1.PodTemplateSpec) (score sc
return
}

func scoreContainerProbes(podTemplate corev1.PodTemplateSpec) (score scorecard.TestScore) {
score.Name = "Pod Probes"

allContainers := podTemplate.Spec.InitContainers
allContainers = append(allContainers, podTemplate.Spec.Containers...)
func scoreContainerProbes(allServices []corev1.Service) func(corev1.PodTemplateSpec) scorecard.TestScore {
return func(podTemplate corev1.PodTemplateSpec) (score scorecard.TestScore) {
score.Name = "Pod Probes"

hasReadinessProbe := true
hasLivenessProbe := true
allContainers := podTemplate.Spec.InitContainers
allContainers = append(allContainers, podTemplate.Spec.Containers...)

probesAreIdentical := false
hasReadinessProbe := false
hasLivenessProbe := false
probesAreIdentical := false
isTargetedByService := false

for _, container := range allContainers {
if container.ReadinessProbe == nil {
hasReadinessProbe = false
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.AddComment(container.Name, "Container is missing a livenessProbe", "Without a livenessProbe kubelet can not restart the Pod if it has crashed")
for _, service := range allServices {
for selectorKey, selectorVal := range service.Spec.Selector {
if podLabelVal, ok := podTemplate.Labels[selectorKey]; ok && podLabelVal == selectorVal {
isTargetedByService = 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
score.AddComment(container.Name, "Container has the same readiness and liveness probe", "It's recommended to have different probes for the two different purposes.")
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 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 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 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 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 !hasDifferent {
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 hasReadinessProbe && hasLivenessProbe {
if !probesAreIdentical {
score.Grade = 10
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 = 7
score.Grade = 0
}
} else if hasLivenessProbe || hasReadinessProbe {
score.Grade = 5
} else {
score.Grade = 0
}

return
return score
}
}

func scoreContainerSecurityContext(podTemplate corev1.PodTemplateSpec) (score scorecard.TestScore) {
Expand Down Expand Up @@ -231,4 +249,4 @@ func scoreContainerSecurityContext(podTemplate corev1.PodTemplateSpec) (score sc
}

return
}
}
36 changes: 23 additions & 13 deletions score/score.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,21 @@ package score
import (
"bytes"
"io"
"log"
"io/ioutil"
"log"

"github.com/zegl/kube-score/scorecard"

"gopkg.in/yaml.v2"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
appsv1beta1 "k8s.io/api/apps/v1beta1"
appsv1beta2 "k8s.io/api/apps/v1beta2"
networkingv1 "k8s.io/api/networking/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
batchv1 "k8s.io/api/batch/v1"
batchv1beta1 "k8s.io/api/batch/v1beta1"
corev1 "k8s.io/api/core/v1"
extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
networkingv1 "k8s.io/api/networking/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/serializer"
)
Expand Down Expand Up @@ -49,25 +49,29 @@ type PodSpecer interface {
func Score(files []io.Reader) (*scorecard.Scorecard, error) {
type detectKind struct {
ApiVersion string `yaml:"apiVersion"`
Kind string `yaml:"kind"`
Kind string `yaml:"kind"`
}

type bothMeta struct {
typeMeta metav1.TypeMeta
typeMeta metav1.TypeMeta
objectMeta metav1.ObjectMeta
}

var typeMetas []bothMeta
var pods []corev1.Pod
var podspecers []PodSpecer
var networkPolies []networkingv1.NetworkPolicy
var services []corev1.Service

for _, file := range files {
fullFile, err := ioutil.ReadAll(file)
if err != nil {
return nil, err
}

// Convert to unix style newlines
fullFile = bytes.Replace(fullFile, []byte("\r\n"), []byte("\n"), -1)

for _, fileContents := range bytes.Split(fullFile, []byte("---\n")) {
var detect detectKind
err = yaml.Unmarshal(fileContents, &detect)
Expand Down Expand Up @@ -162,7 +166,7 @@ func Score(files []io.Reader) (*scorecard.Scorecard, error) {
}

podspecers = append(podspecers, podspecer)
typeMetas = append(typeMetas, bothMeta{
typeMetas = append(typeMetas, bothMeta{
podspecer.GetTypeMeta(),
podspecer.GetObjectMeta(),
})
Expand All @@ -171,15 +175,21 @@ func Score(files []io.Reader) (*scorecard.Scorecard, error) {
var netpol networkingv1.NetworkPolicy
decode(fileContents, &netpol)
networkPolies = append(networkPolies, netpol)
typeMetas = append(typeMetas, bothMeta{netpol.TypeMeta, netpol.ObjectMeta})
typeMetas = append(typeMetas, bothMeta{netpol.TypeMeta, netpol.ObjectMeta})

case "Service":
var service corev1.Service
decode(fileContents, &service)
services = append(services, service)
typeMetas = append(typeMetas, bothMeta{service.TypeMeta, service.ObjectMeta})

default:
log.Printf("Unknown datatype: %s", detect.Kind)
}
}
}

metaTests := []func(metav1.TypeMeta) scorecard.TestScore {
metaTests := []func(metav1.TypeMeta) scorecard.TestScore{
scoreMetaStableAvailable,
}

Expand All @@ -188,7 +198,7 @@ func Score(files []io.Reader) (*scorecard.Scorecard, error) {
scoreContainerImageTag,
scoreContainerImagePullPolicy,
scorePodHasNetworkPolicy(networkPolies),
scoreContainerProbes,
scoreContainerProbes(services),
scoreContainerSecurityContext,
}

Expand All @@ -206,7 +216,7 @@ func Score(files []io.Reader) (*scorecard.Scorecard, error) {
for _, podTest := range podTests {
score := podTest(corev1.PodTemplateSpec{
ObjectMeta: pod.ObjectMeta,
Spec: pod.Spec,
Spec: pod.Spec,
})
score.AddMeta(pod.TypeMeta, pod.ObjectMeta)
scoreCard.Add(score)
Expand All @@ -222,4 +232,4 @@ func Score(files []io.Reader) (*scorecard.Scorecard, error) {
}

return scoreCard, nil
}
}
31 changes: 29 additions & 2 deletions score/score_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package score

import (
"github.com/zegl/kube-score/scorecard"
"io"
"os"
"testing"
Expand Down Expand Up @@ -31,6 +32,20 @@ func testExpectedScore(t *testing.T, filename string, testcase string, expectedS
assert.True(t, tested, "Was not tested")
}

func testGetComments(t *testing.T, filename, testcase string) []scorecard.TestScoreComment {
sc, err := Score([]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 TestPodContainerNoResources(t *testing.T) {
testExpectedScore(t, "pod-test-resources-none.yaml", "Container Resources", 0)
}
Expand Down Expand Up @@ -92,7 +107,7 @@ func TestPodProbesAllMissing(t *testing.T) {
}

func TestPodProbesMissingReady(t *testing.T) {
testExpectedScore(t, "pod-probes-missing-ready.yaml", "Pod Probes", 5)
testExpectedScore(t, "pod-probes-missing-ready.yaml", "Pod Probes", 10)
}

func TestPodProbesIdenticalHTTP(t *testing.T) {
Expand All @@ -107,6 +122,18 @@ 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 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)
}
Expand All @@ -125,4 +152,4 @@ func TestContainerSecurityContextLowGroup(t *testing.T) {

func TestContainerSecurityContextHighIds(t *testing.T) {
testExpectedScore(t, "pod-security-context-high-ids.yaml", "Container Security Context", 10)
}
}
26 changes: 26 additions & 0 deletions score/testdata/pod-probes-not-targeted-by-service.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
apiVersion: v1
kind: Pod
metadata:
name: pod-test-1
labels:
app: my-app
spec:
containers:
- name: foobar
image: foo/bar:latest
livenessProbe:
httpGet:
path: /ready
port: 8080
---
kind: Service
apiVersion: v1
metadata:
name: my-service
spec:
selector:
app: my-app-not-matching
ports:
- protocol: TCP
port: 80
targetPort: 8080
Loading

0 comments on commit fecb240

Please sign in to comment.