fix(pkg/*): Fix service selector match logic

This PR fixes a bug in how service selectors are matched against pod
labels. If there were no selectors for a service it was considered as a
match. With the changes in this PR services without selectors are not
considered for a match.

Fixes #2578

Signed-off-by: Sneha Chhabria <snchh@microsoft.com>

pkg/catalog/xds_certificates.go

@@ -169,6 +169,10 @@ func listServicesForPod(pod *v1.Pod, kubeController k8s.Controller) ([]v1.Servic
 			continue
 		}
 		svcRawSelector := svc.Spec.Selector
+		// service has no selectors, we do not need to match against the pod label
+		if len(svcRawSelector) == 0 {
+			continue
+		}
 		selector := labels.Set(svcRawSelector).AsSelector()
 		if selector.Matches(labels.Set(pod.Labels)) {
 			serviceList = append(serviceList, *svc)

pkg/endpoint/providers/kube/client.go

@@ -190,6 +190,10 @@ func (c *Client) getServicesByLabels(podLabels map[string]string, namespace stri
 		}
 
 		svcRawSelector := svc.Spec.Selector
+		// service has no selectors, we do not need to match against the pod label
+		if len(svcRawSelector) == 0 {
+			continue
+		}
 		selector := labels.Set(svcRawSelector).AsSelector()
 		if selector.Matches(labels.Set(podLabels)) {
 			finalList = append(finalList, *svc)

pkg/endpoint/providers/kube/client_test.go

@@ -278,7 +278,7 @@ var _ = Describe("Test Kube Client Provider (/w kubecontroller)", func() {
 		stop <- struct{}{}
 	})
 
-	It("should return not return a service when a pod matching the selector doesn't exist", func() {
+	It("should return a synthetic service when a pod matching the selector doesn't exist", func() {
 		svc := &corev1.Service{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      "test-1",
@@ -390,7 +390,7 @@ var _ = Describe("Test Kube Client Provider (/w kubecontroller)", func() {
 		<-podsAndServiceChannel
 	})
 
-	It("should return an error when the Service selector doesn't match the pod", func() {
+	It("should return a synthetic service when the Service selector doesn't match the pod", func() {
 		podsChannel := events.GetPubSubInstance().Subscribe(announcements.PodAdded,
 			announcements.PodDeleted,
 			announcements.PodUpdated)
@@ -464,6 +464,76 @@ var _ = Describe("Test Kube Client Provider (/w kubecontroller)", func() {
 		<-podsChannel
 	})
 
+	It("should return a synthetic service when the service doesn't have a selector", func() {
+		podsChannel := events.GetPubSubInstance().Subscribe(announcements.PodAdded,
+			announcements.PodDeleted,
+			announcements.PodUpdated)
+		defer events.GetPubSubInstance().Unsub(podsChannel)
+
+		// Create a Service
+		svc := &corev1.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-1",
+				Namespace: testNamespace,
+			},
+			Spec: corev1.ServiceSpec{
+				Ports: []corev1.ServicePort{{
+					Name:     "servicePort",
+					Protocol: corev1.ProtocolTCP,
+					Port:     tests.ServicePort,
+				}},
+			},
+		}
+
+		_, err := fakeClientSet.CoreV1().Services(testNamespace).Create(context.TODO(), svc, metav1.CreateOptions{})
+		Expect(err).ToNot(HaveOccurred())
+
+		// Create a Pod with labels that match the service selector
+		pod := &corev1.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					"app": "test",
+				},
+			},
+			Spec: corev1.PodSpec{
+				ServiceAccountName: "test-service-account",
+				Containers: []corev1.Container{
+					{
+						Name:  "BookbuyerContainerA",
+						Image: "random",
+						Ports: []corev1.ContainerPort{
+							{
+								Name:          "http",
+								Protocol:      corev1.ProtocolTCP,
+								ContainerPort: 80,
+							},
+						},
+					},
+				},
+			},
+		}
+
+		_, err = fakeClientSet.CoreV1().Pods(testNamespace).Create(context.Background(), pod, metav1.CreateOptions{})
+		Expect(err).ToNot(HaveOccurred())
+		<-podsChannel
+
+		givenSvcAccount := service.K8sServiceAccount{
+			Namespace: testNamespace,
+			Name:      "test-service-account", // Should match the service account in the Deployment spec above
+		}
+
+		// Expect a MeshService that corresponds to a Service that matches the Deployment spec labels
+		svcs, err := provider.GetServicesForServiceAccount(givenSvcAccount)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(len(svcs)).To(Equal(1))
+		expectedServiceName := fmt.Sprintf("test-service-account.testNamespace.osm.synthetic-%s", service.SyntheticServiceSuffix)
+		Expect(svcs[0].Name).To(Equal(expectedServiceName))
+
+		err = fakeClientSet.CoreV1().Pods(testNamespace).Delete(context.Background(), pod.Name, metav1.DeleteOptions{})
+		Expect(err).ToNot(HaveOccurred())
+		<-podsChannel
+	})
+
 	It("should return all services when multiple services match the same Pod", func() {
 		// This test is meant to ensure the
 		// service selector logic works as expected when multiple services

pkg/kubernetes/client.go

@@ -279,6 +279,10 @@ func (c Client) ListServiceAccountsForService(svc service.MeshService) ([]servic
 	for _, pod := range pods {
 		svcRawSelector := k8sSvc.Spec.Selector
 		selector := labels.Set(svcRawSelector).AsSelector()
+		// service has no selectors, we do not need to match against the pod label
+		if len(svcRawSelector) == 0 {
+			continue
+		}
 		if selector.Matches(labels.Set(pod.Labels)) {
 			podSvcAccount := service.K8sServiceAccount{
 				Name:      pod.Spec.ServiceAccountName,

pkg/kubernetes/client_test.go

@@ -456,6 +456,99 @@ var _ = Describe("Test Namespace KubeController Methods", func() {
 			Expect(svcAccounts).Should(ConsistOf(expectedSvcAccounts))
 		})
 
+		It("should not return any ServiceAccounts associated with a service without selectors", func() {
+			testNamespaceName := "test-ns"
+			testSvcAccountName1 := "test-service-account-1"
+			testSvcAccountName2 := "test-service-account-2"
+
+			serviceChannel := events.GetPubSubInstance().Subscribe(announcements.ServiceAdded,
+				announcements.ServiceDeleted,
+				announcements.ServiceUpdated)
+			defer events.GetPubSubInstance().Unsub(serviceChannel)
+			podsChannel := events.GetPubSubInstance().Subscribe(announcements.PodAdded,
+				announcements.PodDeleted,
+				announcements.PodUpdated)
+			defer events.GetPubSubInstance().Unsub(podsChannel)
+
+			// Create a namespace
+			testNamespace := &corev1.Namespace{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:   testNamespaceName,
+					Labels: map[string]string{constants.OSMKubeResourceMonitorAnnotation: testMeshName},
+				},
+			}
+			_, err = kubeClient.CoreV1().Namespaces().Create(context.TODO(), testNamespace, metav1.CreateOptions{})
+			Expect(err).To(BeNil())
+			// Wait on namespace to be ready so that resources in this namespace are marked as monitored as soon as possible
+			Eventually(func() bool {
+				return kubeController.IsMonitoredNamespace(testNamespace.Name)
+			}, nsInformerSyncTimeout).Should(BeTrue())
+
+			// Create pods with labels that match the service
+			pod1 := &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "pod1",
+					Namespace: testNamespaceName,
+					Labels: map[string]string{
+						"some-label": "test",
+						"version":    "v1",
+					},
+				},
+				Spec: corev1.PodSpec{
+					ServiceAccountName: testSvcAccountName1,
+				},
+			}
+			_, err = kubeClient.CoreV1().Pods(testNamespaceName).Create(context.TODO(), pod1, metav1.CreateOptions{})
+			Expect(err).To(BeNil())
+			<-podsChannel
+
+			pod2 := &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "pod2",
+					Namespace: testNamespaceName,
+					Labels: map[string]string{
+						"some-label": "test",
+						"version":    "v2",
+					},
+				},
+				Spec: corev1.PodSpec{
+					ServiceAccountName: testSvcAccountName2,
+				},
+			}
+			_, err = kubeClient.CoreV1().Pods(testNamespaceName).Create(context.TODO(), pod2, metav1.CreateOptions{})
+			Expect(err).To(BeNil())
+			<-podsChannel
+
+			// Create a service with selector that matches the pods above
+			svc := &corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-1",
+					Namespace: testNamespaceName,
+				},
+				Spec: corev1.ServiceSpec{
+					Ports: []corev1.ServicePort{{
+						Name:     "servicePort",
+						Protocol: corev1.ProtocolTCP,
+						Port:     tests.ServicePort,
+					}},
+				},
+			}
+
+			_, err := kubeClient.CoreV1().Services(testNamespaceName).Create(context.TODO(), svc, metav1.CreateOptions{})
+			Expect(err).ToNot(HaveOccurred())
+			<-serviceChannel
+
+			meshSvc := service.MeshService{Name: svc.Name, Namespace: svc.Namespace}
+
+			svcAccounts, err := kubeController.ListServiceAccountsForService(meshSvc)
+
+			Expect(err).ToNot(HaveOccurred())
+
+			expectedSvcAccounts := []service.K8sServiceAccount{}
+			Expect(svcAccounts).Should(HaveLen(0))
+			Expect(svcAccounts).Should(ConsistOf(expectedSvcAccounts))
+		})
+
 	})
 
 })