Repository: zegl/kube-score (go)

Issue:
readinessProbe false positive when a pod matches partially service selector
**Which version of kube-score are you using?**

> kube-score version: 1.9.0

**What did you do?**

> I have the following manifests describing 2 deployments and 1 service targeting pod's of first deployment. The second deployment labels matches partially service selector but not fully, so its pods are not targeted by service. However kube-score warns for missing readinessProbe for the second deployment.

```yaml
---
apiVersion: v1
kind: Service
metadata:
  labels:
    env: staging
  name: test-service
spec:
  ports:
  - name: http
    port: 8080
    targetPort: 8080
  selector:
    component: frontend
    env: staging
---
apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    component: frontend
    env: staging
  name: frontend
spec:
  replicas: 1
  selector:
    component: frontend
  template:
    metadata:
      labels:
        component: frontend
        env: staging
    spec:
      containers:
      - image: frontend-image:tag
        imagePullPolicy: Always
        name: fontend
---
apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    component: backend
    env: staging
  name: backend
spec:
  replicas: 1
  selector:
    component: backend
  template:
    metadata:
      labels:
        component: backend
        env: staging
    spec:
      containers:
      - image: backend-image:tag
        imagePullPolicy: Always
        name: backend
```

**What did you expect to see?**

> I think the second deployement (backend here) should not raise missing readinessProbe alert

**What did you see instead?**

> Actually I get:
```
[....]
apps/v1/Deployment backend                                                    💥
    [CRITICAL] Pod Probes
        · Container is missing a readinessProbe
            A readinessProbe should be used to indicate when the service is ready to receive traffic. Without it, the Pod is risking to receive traffic before it has booted. It's also used during rollouts, and can prevent
            downtime if a new version of the application is failing.
            More information: https://github.com/zegl/kube-score/blob/master/README_PROBES.md
[...]
```
> If I remove label `env` everywhere, it's OK. 
> I think kube-score wrongly considers backend to be targeted by service because there are some label matching but not all.
> Note: This bug is quite annoying for me as I cannot just remove unnecessary labels from service selector as there are globally added by kustomize overlay

Pull request:
probes: correctly check if a pod is targeted by a service
Previously only one label needed to match and not all, causing false positives

```
RELNOTE: Fix a bug in the "Pod Probes" test where a Pod could incorrectly be considered to be targeted by a Service
```

This fixes #316

Patch:
diff --git a/go.sum b/go.sum
index be89520..b5d4d2f 100644
--- a/go.sum
+++ b/go.sum
@@ -309,6 +309,7 @@ k8s.io/apimachinery v0.18.8/go.mod h1:6sQd+iHEqmOtALqOFjSWp2KZ9F0wlU/nWm0ZgsYWMi
 k8s.io/apimachinery v0.19.0/go.mod h1:DnPGDnARWFvYa3pMHgSxtbZb7gpzzAZ1pTfaUNDVlmA=
 k8s.io/apimachinery v0.19.1 h1:cwsxZazM/LA9aUsBaL4bRS5ygoM6bYp8dFk22DSYQa4=
 k8s.io/apimachinery v0.19.1/go.mod h1:DnPGDnARWFvYa3pMHgSxtbZb7gpzzAZ1pTfaUNDVlmA=
+k8s.io/apimachinery v0.19.2 h1:5Gy9vQpAGTKHPVOh5c4plE274X8D/6cuEiTO2zve7tc=
 k8s.io/apimachinery v0.19.2/go.mod h1:DnPGDnARWFvYa3pMHgSxtbZb7gpzzAZ1pTfaUNDVlmA=
 k8s.io/gengo v0.0.0-20190128074634-0689ccc1d7d6/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0=
 k8s.io/gengo v0.0.0-20200413195148-3a45101e95ac/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0=
diff --git a/score/probes/probes.go b/score/probes/probes.go
index eb19313..c67f037 100644
--- a/score/probes/probes.go
+++ b/score/probes/probes.go
@@ -6,6 +6,7 @@ import (
 
 	ks "github.com/zegl/kube-score/domain"
 	"github.com/zegl/kube-score/score/checks"
+	"github.com/zegl/kube-score/score/internal"
 	"github.com/zegl/kube-score/scorecard"
 )
 
@@ -34,13 +35,9 @@ func containerProbes(allServices []ks.Service) func(corev1.PodTemplateSpec, meta
 		isTargetedByService := false
 
 		for _, s := range allServices {
-			service := s.Service()
-			if podTemplate.Namespace == service.Namespace {
-				for selectorKey, selectorVal := range service.Spec.Selector {
-					if podLabelVal, ok := podTemplate.Labels[selectorKey]; ok && podLabelVal == selectorVal {
-						isTargetedByService = true
-					}
-				}
+			if podIsTargetedByService(podTemplate, s.Service()) {
+				isTargetedByService = true
+				break
 			}
 		}
 
@@ -132,3 +129,21 @@ func containerProbes(allServices []ks.Service) func(corev1.PodTemplateSpec, meta
 		return score
 	}
 }
+
+func podIsTargetedByService(pod corev1.PodTemplateSpec, service corev1.Service) bool {
+	if pod.Namespace != service.Namespace {
+		return false
+	}
+
+	labelSelector := &metav1.LabelSelector{
+		MatchLabels: service.Spec.Selector,
+	}
+
+	selector, err := metav1.LabelSelectorAsSelector(labelSelector)
+	if err != nil {
+		return false
+	}
+
+	lables := internal.MapLables(pod.GetObjectMeta().GetLabels())
+	return selector.Matches(internal.MapLables(lables))
+}


Test patch:
diff --git a/score/probe_test.go b/score/probe_test.go
index 0c6afdd..9d835ef 100644
--- a/score/probe_test.go
+++ b/score/probe_test.go
@@ -1,9 +1,10 @@
 package score
 
 import (
-	"github.com/stretchr/testify/assert"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/zegl/kube-score/scorecard"
 )
 
@@ -56,6 +57,13 @@ func TestProbesTargetedByServiceSameNamespace(t *testing.T) {
 	assert.Equal(t, "Container is missing a readinessProbe", comments[0].Summary)
 }
 
+func TestProbesTargetedByServiceSameNamespaceMultiLabels(t *testing.T) {
+	t.Parallel()
+	comments := testExpectedScore(t, "pod-probes-targeted-by-service-same-namespace-multi-labels.yaml", "Pod Probes", scorecard.GradeCritical)
+	assert.Len(t, comments, 1)
+	assert.Equal(t, "Container is missing a readinessProbe", comments[0].Summary)
+}
+
 func TestProbesTargetedByServiceDifferentNamespace(t *testing.T) {
 	t.Parallel()
 	comments := testExpectedScore(t, "pod-probes-targeted-by-service-different-namespace.yaml", "Pod Probes", scorecard.GradeAllOK)
@@ -70,6 +78,13 @@ func TestProbesTargetedByServiceNotTargeted(t *testing.T) {
 	assert.Equal(t, "The pod is not targeted by a service, skipping probe checks.", comments[0].Summary)
 }
 
+func TestProbesTargetedByServiceNotTargetedMultiLabels(t *testing.T) {
+	t.Parallel()
+	comments := testExpectedScore(t, "pod-probes-not-targeted-by-service-multi-labels.yaml", "Pod Probes", scorecard.GradeAllOK)
+	assert.Len(t, comments, 1)
+	assert.Equal(t, "The pod is not targeted by a service, skipping probe checks.", comments[0].Summary)
+}
+
 func TestProbesMultipleContainers(t *testing.T) {
 	t.Parallel()
 	comments := testExpectedScore(t, "pod-probes-on-different-containers.yaml", "Pod Probes", scorecard.GradeAllOK)
diff --git a/score/probes/probes_test.go b/score/probes/probes_test.go
new file mode 100644
index 0000000..8d90436
--- /dev/null
+++ b/score/probes/probes_test.go
@@ -0,0 +1,135 @@
+package probes
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestPodIsTargetedByService(t *testing.T) {
+	t.Run("single label match", func(t *testing.T) {
+		res := podIsTargetedByService(v1.PodTemplateSpec{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{"foo": "bar"},
+			},
+		},
+			v1.Service{
+				Spec: v1.ServiceSpec{
+					Selector: map[string]string{"foo": "bar"},
+				},
+			},
+		)
+
+		assert.True(t, res)
+	})
+
+	t.Run("single label mismatch", func(t *testing.T) {
+		res := podIsTargetedByService(v1.PodTemplateSpec{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{"foo": "bar"},
+			},
+		},
+			v1.Service{
+				Spec: v1.ServiceSpec{
+					Selector: map[string]string{"foo": "baz"},
+				},
+			},
+		)
+
+		assert.False(t, res)
+	})
+
+	t.Run("multi label match", func(t *testing.T) {
+		res := podIsTargetedByService(v1.PodTemplateSpec{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					"foo1": "bar1",
+					"foo2": "bar2",
+				},
+			},
+		},
+			v1.Service{
+				Spec: v1.ServiceSpec{
+					Selector: map[string]string{
+						"foo1": "bar1",
+						"foo2": "bar2",
+					},
+				},
+			},
+		)
+
+		assert.True(t, res)
+	})
+
+	t.Run("multi non full match", func(t *testing.T) {
+		res := podIsTargetedByService(v1.PodTemplateSpec{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					"foo1": "bar1",
+					"foo2": "bar2",
+				},
+			},
+		},
+			v1.Service{
+				Spec: v1.ServiceSpec{
+					Selector: map[string]string{
+						"foo1": "bar1",
+						"foo2": "bar-whatever",
+					},
+				},
+			},
+		)
+
+		assert.False(t, res)
+	})
+
+	t.Run("multi label match same namespace", func(t *testing.T) {
+		res := podIsTargetedByService(v1.PodTemplateSpec{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: "foospace",
+				Labels: map[string]string{
+					"foo1": "bar1",
+					"foo2": "bar2",
+				},
+			},
+		},
+			v1.Service{
+				ObjectMeta: metav1.ObjectMeta{Namespace: "foospace"},
+				Spec: v1.ServiceSpec{
+					Selector: map[string]string{
+						"foo1": "bar1",
+						"foo2": "bar2",
+					},
+				},
+			},
+		)
+
+		assert.True(t, res)
+	})
+
+	t.Run("multi label match different namespace", func(t *testing.T) {
+		res := podIsTargetedByService(v1.PodTemplateSpec{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: "foospace",
+				Labels: map[string]string{
+					"foo1": "bar1",
+					"foo2": "bar2",
+				},
+			},
+		},
+			v1.Service{
+				ObjectMeta: metav1.ObjectMeta{Namespace: "someOtherNamespace"},
+				Spec: v1.ServiceSpec{
+					Selector: map[string]string{
+						"foo1": "bar1",
+						"foo2": "bar2",
+					},
+				},
+			},
+		)
+
+		assert.False(t, res)
+	})
+}
diff --git a/score/testdata/pod-probes-not-targeted-by-service-multi-labels.yaml b/score/testdata/pod-probes-not-targeted-by-service-multi-labels.yaml
new file mode 100644
index 0000000..e118bae
--- /dev/null
+++ b/score/testdata/pod-probes-not-targeted-by-service-multi-labels.yaml
@@ -0,0 +1,28 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod-test-1
+  labels:
+    app: my-app
+    otherLabel: foo
+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
+    otherLabel: foo
+  ports:
+  - protocol: TCP
+    port: 80
+    targetPort: 8080
diff --git a/score/testdata/pod-probes-targeted-by-service-same-namespace-multi-labels.yaml b/score/testdata/pod-probes-targeted-by-service-same-namespace-multi-labels.yaml
new file mode 100644
index 0000000..e7a879b
--- /dev/null
+++ b/score/testdata/pod-probes-targeted-by-service-same-namespace-multi-labels.yaml
@@ -0,0 +1,30 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod-test-1
+  namespace: foospace
+  labels:
+    app: my-app
+    foo: bar
+spec:
+  containers:
+  - name: foobar
+    image: foo/bar:latest
+    livenessProbe:
+      httpGet:
+        path: /ready
+        port: 8080
+---
+kind: Service
+apiVersion: v1
+metadata:
+  name: my-service
+  namespace: foospace
+spec:
+  selector:
+    app: my-app
+    foo: bar
+  ports:
+  - protocol: TCP
+    port: 80
+    targetPort: 8080