From 5bb5e5bc3cde5073c2ec9f69e3521fcb58bb1bbf Mon Sep 17 00:00:00 2001 From: aattuluri Date: Mon, 4 Jul 2022 23:21:08 -0700 Subject: [PATCH] Fix and refactor service to deploy/rollout matching (#239) (#137) Co-authored-by: aattuluri <44482891+aattuluri@users.noreply.github.com> --- admiral/pkg/clusters/handler.go | 23 ++----------------- admiral/pkg/controller/admiral/deployment.go | 17 ++++++++------ .../pkg/controller/admiral/deployment_test.go | 4 ---- admiral/pkg/controller/admiral/rollouts.go | 19 +++++++-------- .../pkg/controller/admiral/rollouts_test.go | 4 ---- admiral/pkg/controller/common/common.go | 20 ++++++++++++++++ 6 files changed, 42 insertions(+), 45 deletions(-) diff --git a/admiral/pkg/clusters/handler.go b/admiral/pkg/clusters/handler.go index 6e707c953..ca9bb6947 100644 --- a/admiral/pkg/clusters/handler.go +++ b/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) diff --git a/admiral/pkg/controller/admiral/deployment.go b/admiral/pkg/controller/admiral/deployment.go index 024939668..97306da55 100644 --- a/admiral/pkg/controller/admiral/deployment.go +++ b/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 } diff --git a/admiral/pkg/controller/admiral/deployment_test.go b/admiral/pkg/controller/admiral/deployment_test.go index c3c5cb88d..b11dc3b38 100644 --- a/admiral/pkg/controller/admiral/deployment_test.go +++ b/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) diff --git a/admiral/pkg/controller/admiral/rollouts.go b/admiral/pkg/controller/admiral/rollouts.go index 2be58bd7a..56bfdcedd 100644 --- a/admiral/pkg/controller/admiral/rollouts.go +++ b/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 } diff --git a/admiral/pkg/controller/admiral/rollouts_test.go b/admiral/pkg/controller/admiral/rollouts_test.go index ffd024cc1..2640dad02 100644 --- a/admiral/pkg/controller/admiral/rollouts_test.go +++ b/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() diff --git a/admiral/pkg/controller/common/common.go b/admiral/pkg/controller/common/common.go index 442a23b17..b7fafbf38 100644 --- a/admiral/pkg/controller/common/common.go +++ b/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 +}