Fix and refactor service to deploy/rollout matching (istio-ecosystem#239

) (istio-ecosystem#137)

Co-authored-by: aattuluri <44482891+aattuluri@users.noreply.github.com>

admiral/pkg/clusters/handler.go

@@ -779,14 +779,7 @@ func getServiceForDeployment(rc *RemoteController, deployment *k8sAppsV1.Deploym
 	}
 	var matchedService *k8sV1.Service
 	for _, service := range cachedServices {
-		var match = true
-		for lkey, lvalue := range service.Spec.Selector {
-			value, ok := deployment.Spec.Selector.MatchLabels[lkey]
-			if !ok || value != lvalue {
-				match = false
-				break
-			}
-		}
+		var match = common.IsServiceMatch(service.Spec.Selector, deployment.Spec.Selector)
 		//make sure the service matches the deployment Selector and also has a mesh port in the port spec
 		if match {
 			ports := GetMeshPorts(rc.ClusterID, service, deployment)
@@ -925,25 +918,13 @@ func getServiceForRollout(rc *RemoteController, rollout *argo.Rollout) map[strin
 	var matchedServices = make(map[string]*WeightedService)
 
 	for _, service := range cachedServices {
-		var match = true
 		//skip services that are not referenced in the rollout
 		if len(blueGreenActiveService) > 0 && service.ObjectMeta.Name != blueGreenActiveService && service.ObjectMeta.Name != blueGreenPreviewService {
 			log.Infof("Skipping service=%s for rollout=%s in namespace=%s and cluster=%s", service.Name, rollout.Name, rollout.Namespace, rc.ClusterID)
 			continue
 		}
 
-		for lkey, lvalue := range service.Spec.Selector {
-			// Rollouts controller adds a dynamic label with name rollouts-pod-template-hash to both active and passive replicasets.
-			// This dynamic label is not available on the rollout template. Hence ignoring the label with name rollouts-pod-template-hash
-			if lkey == common.RolloutPodHashLabel {
-				continue
-			}
-			value, ok := rollout.Spec.Selector.MatchLabels[lkey]
-			if !ok || value != lvalue {
-				match = false
-				break
-			}
-		}
+		match := common.IsServiceMatch(service.Spec.Selector, rollout.Spec.Selector)
 		//make sure the service matches the rollout Selector and also has a mesh port in the port spec
 		if match {
 			ports := GetMeshPortsForRollout(rc.ClusterID, service, rollout)

admiral/pkg/controller/admiral/deployment.go

@@ -6,7 +6,6 @@ import (
 	"github.com/istio-ecosystem/admiral/admiral/pkg/controller/util"
 	"github.com/sirupsen/logrus"
 	k8sAppsV1 "k8s.io/api/apps/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	k8sAppsinformers "k8s.io/client-go/informers/apps/v1"
 	"k8s.io/client-go/rest"
 	"time"
@@ -176,11 +175,7 @@ func (d *DeploymentController) shouldIgnoreBasedOnLabels(deployment *k8sAppsV1.D
 
 func (d *DeploymentController) GetDeploymentBySelectorInNamespace(serviceSelector map[string]string, namespace string) []k8sAppsV1.Deployment {
 
-	labelOptions := meta_v1.ListOptions{
-		LabelSelector: labels.SelectorFromSet(serviceSelector).String(),
-	}
-
-	matchedDeployments, err := d.K8sClient.AppsV1().Deployments(namespace).List(labelOptions)
+	matchedDeployments, err := d.K8sClient.AppsV1().Deployments(namespace).List(meta_v1.ListOptions{})
 
 	if err != nil {
 		logrus.Errorf("Failed to list deployments in cluster, error: %v", err)
@@ -191,5 +186,13 @@ func (d *DeploymentController) GetDeploymentBySelectorInNamespace(serviceSelecto
 		return []k8sAppsV1.Deployment{}
 	}
 
-	return matchedDeployments.Items
+	filteredDeployments := make ([]k8sAppsV1.Deployment, 0)
+
+	for _, deployment := range matchedDeployments.Items {
+		if common.IsServiceMatch(serviceSelector, deployment.Spec.Selector) {
+			filteredDeployments = append(filteredDeployments, deployment)
+		}
+	}
+
+	return filteredDeployments
 }

admiral/pkg/controller/admiral/deployment_test.go

@@ -209,7 +209,6 @@ func TestDeploymentController_GetDeploymentBySelectorInNamespace(t *testing.T) {
 			},
 		},
 	}
-	deployment.Labels = map[string]string{"identity": "app1"}
 
 	deployment2 := k8sAppsV1.Deployment{}
 	deployment2.Namespace = "namespace"
@@ -222,7 +221,6 @@ func TestDeploymentController_GetDeploymentBySelectorInNamespace(t *testing.T) {
 			},
 		},
 	}
-	deployment2.Labels = map[string]string{"identity": "app1"}
 
 	deployment3 := k8sAppsV1.Deployment{}
 	deployment3.Namespace = "namespace"
@@ -236,7 +234,6 @@ func TestDeploymentController_GetDeploymentBySelectorInNamespace(t *testing.T) {
 			},
 		},
 	}
-	deployment3.Labels = map[string]string{"identity": "app1"}
 
 	deployment4 := k8sAppsV1.Deployment{}
 	deployment4.Namespace = "namespace"
@@ -250,7 +247,6 @@ func TestDeploymentController_GetDeploymentBySelectorInNamespace(t *testing.T) {
 			},
 		},
 	}
-	deployment4.Labels = map[string]string{"identity": "app2"}
 
 	oneDeploymentClient := fake.NewSimpleClientset(&deployment)
 

admiral/pkg/controller/admiral/rollouts.go

@@ -2,7 +2,6 @@ package admiral
 
 import (
 	"fmt"
-	"k8s.io/apimachinery/pkg/labels"
 	"sync"
 	"time"
 
@@ -204,13 +203,7 @@ func (roc *RolloutController) Deleted(ojb interface{}) {
 
 func (d *RolloutController) GetRolloutBySelectorInNamespace(serviceSelector map[string]string, namespace string) []argo.Rollout {
 
-	//Remove pod template hash from the service selector as that's not on the deployment
-	delete(serviceSelector, common.RolloutPodHashLabel)
-
-	labelOptions := meta_v1.ListOptions{
-		LabelSelector: labels.SelectorFromSet(serviceSelector).String(),
-	}
-	matchedRollouts, err := d.RolloutClient.Rollouts(namespace).List(labelOptions)
+	matchedRollouts, err := d.RolloutClient.Rollouts(namespace).List(meta_v1.ListOptions{})
 
 	if err != nil {
 		logrus.Errorf("Failed to list rollouts in cluster, error: %v", err)
@@ -221,5 +214,13 @@ func (d *RolloutController) GetRolloutBySelectorInNamespace(serviceSelector map[
 		return make([]argo.Rollout,0)
 	}
 
-	return matchedRollouts.Items
+	filteredRollouts := make ([]argo.Rollout, 0)
+
+	for _, rollout := range matchedRollouts.Items {
+		if common.IsServiceMatch(serviceSelector, rollout.Spec.Selector) {
+			filteredRollouts = append(filteredRollouts, rollout)
+		}
+	}
+
+	return filteredRollouts
 }

admiral/pkg/controller/admiral/rollouts_test.go

@@ -207,7 +207,6 @@ func TestRolloutController_GetRolloutBySelectorInNamespace(t *testing.T) {
 			},
 		},
 	}
-	rollout.Labels = map[string]string{"identity": "app1"}
 
 	rollout2 := argo.Rollout{}
 	rollout2.Namespace = "namespace"
@@ -220,7 +219,6 @@ func TestRolloutController_GetRolloutBySelectorInNamespace(t *testing.T) {
 			},
 		},
 	}
-	rollout2.Labels = map[string]string{"identity": "app1"}
 
 	rollout3 := argo.Rollout{}
 	rollout3.Namespace = "namespace"
@@ -234,7 +232,6 @@ func TestRolloutController_GetRolloutBySelectorInNamespace(t *testing.T) {
 			},
 		},
 	}
-	rollout3.Labels = map[string]string{"identity": "app1"}
 
 	rollout4 := argo.Rollout{}
 	rollout4.Namespace = "namespace"
@@ -248,7 +245,6 @@ func TestRolloutController_GetRolloutBySelectorInNamespace(t *testing.T) {
 			},
 		},
 	}
-	rollout4.Labels = map[string]string{"identity": "app2"}
 
 	oneRolloutClient := argofake.NewSimpleClientset(&rollout).ArgoprojV1alpha1()
 

admiral/pkg/controller/common/common.go

@@ -185,3 +185,23 @@ func ConstructGtpKey(env, identity string) string {
 func ShouldIgnoreResource(metadata v12.ObjectMeta) bool {
 	return  metadata.Annotations[AdmiralIgnoreAnnotation] == "true" || metadata.Labels[AdmiralIgnoreAnnotation] == "true"
 }
+
+func IsServiceMatch(serviceSelector map[string]string, selector *v12.LabelSelector) bool {
+	if selector == nil || len(selector.MatchLabels) == 0 || len(serviceSelector) == 0 {
+		return false
+	}
+	var match = true
+	for lkey, lvalue := range serviceSelector {
+		// Rollouts controller adds a dynamic label with name rollouts-pod-template-hash to both active and passive replicasets.
+		// This dynamic label is not available on the rollout template. Hence ignoring the label with name rollouts-pod-template-hash
+		if lkey == RolloutPodHashLabel {
+			continue
+		}
+		value, ok := selector.MatchLabels[lkey]
+		if !ok || value != lvalue {
+			match = false
+			break
+		}
+	}
+	return match
+}