diff --git a/admiral/pkg/clusters/handler_test.go b/admiral/pkg/clusters/handler_test.go index ff2f4f13..199dd870 100644 --- a/admiral/pkg/clusters/handler_test.go +++ b/admiral/pkg/clusters/handler_test.go @@ -656,10 +656,10 @@ func TestHandleVirtualServiceEvent(t *testing.T) { func TestGetServiceForRolloutCanary(t *testing.T) { //Struct of test case info. Name is required. - const NAMESPACE = "namespace" - const SERVICENAME = "serviceName" - const STABLESERVICENAME = "stableserviceName" - const CANARYSERVICENAME = "canaryserviceName" + const Namespace = "namespace" + const ServiceName = "serviceName" + const StableServiceName = "stableserviceName" + const CanaryServiceName = "canaryserviceName" const VS_NAME_1 = "virtualservice1" const VS_NAME_2 = "virtualservice2" const VS_NAME_3 = "virtualservice3" @@ -692,12 +692,11 @@ func TestGetServiceForRolloutCanary(t *testing.T) { selectorMap["app"] = "test" service := &coreV1.Service{ + ObjectMeta: v12.ObjectMeta{Name: ServiceName, Namespace: Namespace, CreationTimestamp: v12.NewTime(time.Now())}, Spec: coreV1.ServiceSpec{ Selector: selectorMap, }, } - service.Name = SERVICENAME - service.Namespace = NAMESPACE port1 := coreV1.ServicePort{ Port: 8080, } @@ -710,7 +709,7 @@ func TestGetServiceForRolloutCanary(t *testing.T) { service.Spec.Ports = ports stableService := &coreV1.Service{ - ObjectMeta: v12.ObjectMeta{Name: STABLESERVICENAME, Namespace: NAMESPACE}, + ObjectMeta: v12.ObjectMeta{Name: StableServiceName, Namespace: Namespace, CreationTimestamp: v12.NewTime(time.Now().Add(time.Duration(-15)))}, Spec: coreV1.ServiceSpec{ Selector: selectorMap, Ports: ports, @@ -718,7 +717,7 @@ func TestGetServiceForRolloutCanary(t *testing.T) { } canaryService := &coreV1.Service{ - ObjectMeta: v12.ObjectMeta{Name: CANARYSERVICENAME, Namespace: NAMESPACE}, + ObjectMeta: v12.ObjectMeta{Name: CanaryServiceName, Namespace: Namespace, CreationTimestamp: v12.NewTime(time.Now().Add(time.Duration(-15)))}, Spec: coreV1.ServiceSpec{ Selector: selectorMap, Ports: ports, @@ -776,38 +775,38 @@ func TestGetServiceForRolloutCanary(t *testing.T) { rcTemp.ServiceController.Cache.Put(canaryService) virtualService := &v1alpha32.VirtualService{ - ObjectMeta: v12.ObjectMeta{Name: VS_NAME_1, Namespace: NAMESPACE}, + ObjectMeta: v12.ObjectMeta{Name: VS_NAME_1, Namespace: Namespace}, Spec: v1alpha3.VirtualService{ Http: []*v1alpha3.HTTPRoute{{Route: []*v1alpha3.HTTPRouteDestination{ - {Destination: &v1alpha3.Destination{Host: STABLESERVICENAME}, Weight: 80}, - {Destination: &v1alpha3.Destination{Host: CANARYSERVICENAME}, Weight: 20}, + {Destination: &v1alpha3.Destination{Host: StableServiceName}, Weight: 80}, + {Destination: &v1alpha3.Destination{Host: CanaryServiceName}, Weight: 20}, }}}, }, } vsMutipleRoutesWithMatch := &v1alpha32.VirtualService{ - ObjectMeta: v12.ObjectMeta{Name: VS_NAME_2, Namespace: NAMESPACE}, + ObjectMeta: v12.ObjectMeta{Name: VS_NAME_2, Namespace: Namespace}, Spec: v1alpha3.VirtualService{ Http: []*v1alpha3.HTTPRoute{{Name: VS_ROUTE_PRIMARY, Route: []*v1alpha3.HTTPRouteDestination{ - {Destination: &v1alpha3.Destination{Host: STABLESERVICENAME}, Weight: 80}, - {Destination: &v1alpha3.Destination{Host: CANARYSERVICENAME}, Weight: 20}, + {Destination: &v1alpha3.Destination{Host: StableServiceName}, Weight: 80}, + {Destination: &v1alpha3.Destination{Host: CanaryServiceName}, Weight: 20}, }}}, }, } vsMutipleRoutesWithZeroWeight := &v1alpha32.VirtualService{ - ObjectMeta: v12.ObjectMeta{Name: VS_NAME_4, Namespace: NAMESPACE}, + ObjectMeta: v12.ObjectMeta{Name: VS_NAME_4, Namespace: Namespace}, Spec: v1alpha3.VirtualService{ Http: []*v1alpha3.HTTPRoute{{Name: "random", Route: []*v1alpha3.HTTPRouteDestination{ - {Destination: &v1alpha3.Destination{Host: STABLESERVICENAME}, Weight: 100}, - {Destination: &v1alpha3.Destination{Host: CANARYSERVICENAME}, Weight: 0}, + {Destination: &v1alpha3.Destination{Host: StableServiceName}, Weight: 100}, + {Destination: &v1alpha3.Destination{Host: CanaryServiceName}, Weight: 0}, }}}, }, } - rcTemp.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(NAMESPACE).Create(virtualService) - rcTemp.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(NAMESPACE).Create(vsMutipleRoutesWithMatch) - rcTemp.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(NAMESPACE).Create(vsMutipleRoutesWithZeroWeight) + rcTemp.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(Namespace).Create(virtualService) + rcTemp.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(Namespace).Create(vsMutipleRoutesWithMatch) + rcTemp.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(Namespace).Create(vsMutipleRoutesWithZeroWeight) canaryRollout := argo.Rollout{ Spec: argo.RolloutSpec{Template: coreV1.PodTemplateSpec{ @@ -821,7 +820,7 @@ func TestGetServiceForRolloutCanary(t *testing.T) { } canaryRollout.Spec.Selector = &labelSelector - canaryRollout.Namespace = NAMESPACE + canaryRollout.Namespace = Namespace canaryRollout.Spec.Strategy = argo.RolloutStrategy{ Canary: &argo.CanaryStrategy{}, } @@ -871,11 +870,11 @@ func TestGetServiceForRolloutCanary(t *testing.T) { }}} canaryRolloutIstioVs.Spec.Selector = &labelSelector - canaryRolloutIstioVs.Namespace = NAMESPACE + canaryRolloutIstioVs.Namespace = Namespace canaryRolloutIstioVs.Spec.Strategy = argo.RolloutStrategy{ Canary: &argo.CanaryStrategy{ - StableService: STABLESERVICENAME, - CanaryService: CANARYSERVICENAME, + StableService: StableServiceName, + CanaryService: CanaryServiceName, TrafficRouting: &argo.RolloutTrafficRouting{ Istio: &argo.IstioTrafficRouting{ VirtualService: argo.IstioVirtualService{Name: VS_NAME_1}, @@ -890,11 +889,11 @@ func TestGetServiceForRolloutCanary(t *testing.T) { }}} canaryRolloutIstioVsRouteMatch.Spec.Selector = &labelSelector - canaryRolloutIstioVsRouteMatch.Namespace = NAMESPACE + canaryRolloutIstioVsRouteMatch.Namespace = Namespace canaryRolloutIstioVsRouteMatch.Spec.Strategy = argo.RolloutStrategy{ Canary: &argo.CanaryStrategy{ - StableService: STABLESERVICENAME, - CanaryService: CANARYSERVICENAME, + StableService: StableServiceName, + CanaryService: CanaryServiceName, TrafficRouting: &argo.RolloutTrafficRouting{ Istio: &argo.IstioTrafficRouting{ VirtualService: argo.IstioVirtualService{Name: VS_NAME_2, Routes: []string{VS_ROUTE_PRIMARY}}, @@ -909,11 +908,11 @@ func TestGetServiceForRolloutCanary(t *testing.T) { }}} canaryRolloutIstioVsRouteMisMatch.Spec.Selector = &labelSelector - canaryRolloutIstioVsRouteMisMatch.Namespace = NAMESPACE + canaryRolloutIstioVsRouteMisMatch.Namespace = Namespace canaryRolloutIstioVsRouteMisMatch.Spec.Strategy = argo.RolloutStrategy{ Canary: &argo.CanaryStrategy{ - StableService: STABLESERVICENAME, - CanaryService: CANARYSERVICENAME, + StableService: StableServiceName, + CanaryService: CanaryServiceName, TrafficRouting: &argo.RolloutTrafficRouting{ Istio: &argo.IstioTrafficRouting{ VirtualService: argo.IstioVirtualService{Name: VS_NAME_3, Routes: []string{"random"}}, @@ -928,11 +927,11 @@ func TestGetServiceForRolloutCanary(t *testing.T) { }}} canaryRolloutIstioVsZeroWeight.Spec.Selector = &labelSelector - canaryRolloutIstioVsZeroWeight.Namespace = NAMESPACE + canaryRolloutIstioVsZeroWeight.Namespace = Namespace canaryRolloutIstioVsZeroWeight.Spec.Strategy = argo.RolloutStrategy{ Canary: &argo.CanaryStrategy{ - StableService: STABLESERVICENAME, - CanaryService: CANARYSERVICENAME, + StableService: StableServiceName, + CanaryService: CanaryServiceName, TrafficRouting: &argo.RolloutTrafficRouting{ Istio: &argo.IstioTrafficRouting{ VirtualService: argo.IstioVirtualService{Name: VS_NAME_4}, @@ -947,11 +946,11 @@ func TestGetServiceForRolloutCanary(t *testing.T) { }}} canaryRolloutIstioVsMimatch.Spec.Selector = &labelSelector - canaryRolloutIstioVsMimatch.Namespace = NAMESPACE + canaryRolloutIstioVsMimatch.Namespace = Namespace canaryRolloutIstioVsMimatch.Spec.Strategy = argo.RolloutStrategy{ Canary: &argo.CanaryStrategy{ - StableService: STABLESERVICENAME, - CanaryService: CANARYSERVICENAME, + StableService: StableServiceName, + CanaryService: CanaryServiceName, TrafficRouting: &argo.RolloutTrafficRouting{ Istio: &argo.IstioTrafficRouting{ VirtualService: argo.IstioVirtualService{Name: "random"}, @@ -962,14 +961,14 @@ func TestGetServiceForRolloutCanary(t *testing.T) { resultForDummy := map[string]*WeightedService{"dummy": {Weight: 1, Service: service1}} - resultForRandomMatch := map[string]*WeightedService{CANARYSERVICENAME: {Weight: 1, Service: canaryService}} + resultForRandomMatch := map[string]*WeightedService{CanaryServiceName: {Weight: 1, Service: canaryService}} - resultForStableServiceOnly := map[string]*WeightedService{STABLESERVICENAME: {Weight: 1, Service: stableService}} + resultForStableServiceOnly := map[string]*WeightedService{StableServiceName: {Weight: 1, Service: stableService}} - resultForCanaryWithIstio := map[string]*WeightedService{STABLESERVICENAME: {Weight: 80, Service: stableService}, - CANARYSERVICENAME: {Weight: 20, Service: canaryService}} + resultForCanaryWithIstio := map[string]*WeightedService{StableServiceName: {Weight: 80, Service: stableService}, + CanaryServiceName: {Weight: 20, Service: canaryService}} - resultForCanaryWithStableService := map[string]*WeightedService{STABLESERVICENAME: {Weight: 100, Service: stableService}} + resultForCanaryWithStableService := map[string]*WeightedService{StableServiceName: {Weight: 100, Service: stableService}} testCases := []struct { name string diff --git a/admiral/pkg/controller/admiral/controller.go b/admiral/pkg/controller/admiral/controller.go index 56d2db3c..c0c43f9d 100644 --- a/admiral/pkg/controller/admiral/controller.go +++ b/admiral/pkg/controller/admiral/controller.go @@ -102,7 +102,8 @@ func (c *Controller) Run(stopCh <-chan struct{}) { } log.Infof("Informer caches synced for controller=%v, current keys=%v", c.name, c.informer.GetStore().ListKeys()) - wait.Until(c.runWorker, 5*time.Second, stopCh) + //wait for 30 seconds for the first time (for all caches to sync) + wait.Until(c.runWorker, 30 * time.Second, stopCh) } func (c *Controller) runWorker() { diff --git a/admiral/pkg/controller/admiral/deployment.go b/admiral/pkg/controller/admiral/deployment.go index e1b5ec7c..b4c7277e 100644 --- a/admiral/pkg/controller/admiral/deployment.go +++ b/admiral/pkg/controller/admiral/deployment.go @@ -5,6 +5,7 @@ import ( "github.com/istio-ecosystem/admiral/admiral/pkg/controller/common" "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" @@ -78,45 +79,6 @@ func (p *deploymentCache) DeleteFromDeploymentClusterCache(key string, deploymen } } -func (d *DeploymentController) GetDeployments() ([]*k8sAppsV1.Deployment, error) { - - ns := d.K8sClient.CoreV1().Namespaces() - - namespaceSidecarInjectionLabelFilter := d.labelSet.NamespaceSidecarInjectionLabel + "=" + d.labelSet.NamespaceSidecarInjectionLabelValue - istioEnabledNs, err := ns.List(meta_v1.ListOptions{LabelSelector: namespaceSidecarInjectionLabelFilter}) - - if err != nil { - return nil, fmt.Errorf("error getting istio labled namespaces: %v", err) - } - - var res []*k8sAppsV1.Deployment - - for _, v := range istioEnabledNs.Items { - - deployments := d.K8sClient.AppsV1().Deployments(v.Name) - deploymentsList, err := deployments.List(meta_v1.ListOptions{}) - if err != nil { - return nil, fmt.Errorf("error listing deployments: %v", err) - } - var admiralDeployments []k8sAppsV1.Deployment - for _, deployment := range deploymentsList.Items { - if !d.shouldIgnoreBasedOnLabels(&deployment) { - admiralDeployments = append(admiralDeployments, deployment) - } - } - - if err != nil { - return nil, fmt.Errorf("error getting istio labled namespaces: %v", err) - } - - for _, pi := range admiralDeployments { - res = append(res, &pi) - } - } - - return res, nil -} - func NewDeploymentController(clusterID string, stopCh <-chan struct{}, handler DeploymentHandler, config *rest.Config, resyncPeriod time.Duration) (*DeploymentController, error) { deploymentController := DeploymentController{} @@ -211,10 +173,12 @@ func (d *DeploymentController) shouldIgnoreBasedOnLabels(deployment *k8sAppsV1.D return false //labels are fine, we should not ignore } -func (d *DeploymentController) GetDeploymentByLabel(labelValue string, namespace string) []k8sAppsV1.Deployment { - matchLabel := common.GetGlobalTrafficDeploymentLabel() - labelOptions := meta_v1.ListOptions{} - labelOptions.LabelSelector = fmt.Sprintf("%s=%s", matchLabel, labelValue) +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) if err != nil { diff --git a/admiral/pkg/controller/admiral/deployment_test.go b/admiral/pkg/controller/admiral/deployment_test.go index 2046dc07..cd8bc472 100644 --- a/admiral/pkg/controller/admiral/deployment_test.go +++ b/admiral/pkg/controller/admiral/deployment_test.go @@ -4,7 +4,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/istio-ecosystem/admiral/admiral/pkg/controller/common" "github.com/istio-ecosystem/admiral/admiral/pkg/test" - log "github.com/sirupsen/logrus" k8sAppsV1 "k8s.io/api/apps/v1" coreV1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -183,63 +182,6 @@ func TestDeploymentController_Deleted(t *testing.T) { } } -func TestDeploymentController_GetDeployments(t *testing.T) { - - depController := DeploymentController{ - labelSet: &common.LabelSet{ - DeploymentAnnotation: "sidecar.istio.io/inject", - NamespaceSidecarInjectionLabel: "istio-injection", - NamespaceSidecarInjectionLabelValue: "enabled", - AdmiralIgnoreLabel: "admiral-ignore", - }, - } - - client := fake.NewSimpleClientset() - - ns := coreV1.Namespace{} - ns.Labels = map[string]string{"istio-injection": "enabled"} - ns.Name = "test-ns" - - _, err := client.CoreV1().Namespaces().Create(&ns) - if err != nil { - t.Errorf("%v", err) - } - - deployment := k8sAppsV1.Deployment{} - deployment.Namespace = "test-ns" - deployment.Name = "deployment" - deployment.Spec.Template.Labels = map[string]string{"identity": "id", "istio-injected": "true"} - deployment.Spec.Template.Annotations = map[string]string{"sidecar.istio.io/inject": "true"} - deploymentWithBadLabels := k8sAppsV1.Deployment{} - deploymentWithBadLabels.Namespace = "test-ns" - deploymentWithBadLabels.Name = "deploymentWithBadLabels" - deploymentWithBadLabels.Spec.Template.Labels = map[string]string{"identity": "id", "random-label": "true"} - deploymentWithBadLabels.Spec.Template.Annotations = map[string]string{"woo": "yay"} - deploymentWithIgnoreLabels := k8sAppsV1.Deployment{} - deploymentWithIgnoreLabels.Namespace = "test-ns" - deploymentWithIgnoreLabels.Name = "deploymentWithIgnoreLabels" - deploymentWithIgnoreLabels.Spec.Template.Labels = map[string]string{"identity": "id", "istio-injected": "true", "admiral-ignore": "true"} - deploymentWithIgnoreLabels.Spec.Template.Annotations = map[string]string{"sidecar.istio.io/inject": "true"} - _, err = client.AppsV1().Deployments("test-ns").Create(&deployment) - _, err = client.AppsV1().Deployments("test-ns").Create(&deploymentWithBadLabels) - _, err = client.AppsV1().Deployments("test-ns").Create(&deploymentWithIgnoreLabels) - - if err != nil { - t.Errorf("%v", err) - } - - depController.K8sClient = client - resultingDeps, _ := depController.GetDeployments() - - if len(resultingDeps) != 1 { - t.Errorf("Get Deployments returned too many values. Expected 1, got %v", len(resultingDeps)) - } - if !cmp.Equal(resultingDeps[0], &deployment) { - log.Info("Object Diff: " + cmp.Diff(resultingDeps[0], &deployment)) - t.Errorf("Get Deployments returned the incorrect value. Got %v, expected %v", resultingDeps[0], deployment) - } -} - func TestNewDeploymentController(t *testing.T) { config, err := clientcmd.BuildConfigFromFlags("", "../../test/resources/admins@fake-cluster.k8s.local") if err != nil { @@ -255,11 +197,12 @@ func TestNewDeploymentController(t *testing.T) { } } -func TestDeploymentController_GetDeploymentByLabel(t *testing.T) { +func TestDeploymentController_GetDeploymentBySelectorInNamespace(t *testing.T) { deployment := k8sAppsV1.Deployment{} deployment.Namespace = "namespace" deployment.Name = "fake-app-deployment-qal" deployment.Spec = k8sAppsV1.DeploymentSpec{ + Selector: &v1.LabelSelector{MatchLabels: map[string]string{"identity": "app1"},}, Template: coreV1.PodTemplateSpec{ ObjectMeta: v1.ObjectMeta{ Labels: map[string]string{"identity": "app1", "env": "qal"}, @@ -272,6 +215,7 @@ func TestDeploymentController_GetDeploymentByLabel(t *testing.T) { deployment2.Namespace = "namespace" deployment2.Name = "fake-app-deployment-e2e" deployment2.Spec = k8sAppsV1.DeploymentSpec{ + Selector: &v1.LabelSelector{MatchLabels: map[string]string{"identity": "app1"},}, Template: coreV1.PodTemplateSpec{ ObjectMeta: v1.ObjectMeta{ Labels: map[string]string{"identity": "app1", "env": "e2e"}, @@ -285,6 +229,7 @@ func TestDeploymentController_GetDeploymentByLabel(t *testing.T) { deployment3.Name = "fake-app-deployment-prf-1" deployment3.CreationTimestamp = v1.Now() deployment3.Spec = k8sAppsV1.DeploymentSpec{ + Selector: &v1.LabelSelector{MatchLabels: map[string]string{"identity": "app1"},}, Template: coreV1.PodTemplateSpec{ ObjectMeta: v1.ObjectMeta{ Labels: map[string]string{"identity": "app1", "env": "prf"}, @@ -298,6 +243,7 @@ func TestDeploymentController_GetDeploymentByLabel(t *testing.T) { deployment4.Name = "fake-app-deployment-prf-2" deployment4.CreationTimestamp = v1.Date(2020, 1, 1, 1, 1, 1, 1, time.UTC) deployment4.Spec = k8sAppsV1.DeploymentSpec{ + Selector: &v1.LabelSelector{MatchLabels: map[string]string{"identity": "app2"},}, Template: coreV1.PodTemplateSpec{ ObjectMeta: v1.ObjectMeta{ Labels: map[string]string{"identity": "app2", "env": "prf"}, @@ -319,37 +265,37 @@ func TestDeploymentController_GetDeploymentByLabel(t *testing.T) { name string expectedDeployments []k8sAppsV1.Deployment fakeClient *fake.Clientset - labelValue string + selector map[string]string }{ { name: "Get one", expectedDeployments: []k8sAppsV1.Deployment{deployment}, fakeClient: oneDeploymentClient, - labelValue: "app1", + selector: map[string]string{"identity": "app1"}, }, { name: "Get one from long list", expectedDeployments: []k8sAppsV1.Deployment{deployment4}, fakeClient: allDeploymentsClient, - labelValue: "app2", + selector: map[string]string{"identity": "app2"}, }, { name: "Get many from long list", expectedDeployments: []k8sAppsV1.Deployment{deployment, deployment3, deployment2}, fakeClient: allDeploymentsClient, - labelValue: "app1", + selector: map[string]string{"identity": "app1"}, }, { name: "Get none from long list", expectedDeployments: []k8sAppsV1.Deployment{}, fakeClient: allDeploymentsClient, - labelValue: "app3", + selector: map[string]string{"identity": "app3"}, }, { name: "Get none from empty list", expectedDeployments: []k8sAppsV1.Deployment{}, fakeClient: noDeploymentsClient, - labelValue: "app1", + selector: map[string]string{"identity": "app1"}, }, } @@ -357,7 +303,7 @@ func TestDeploymentController_GetDeploymentByLabel(t *testing.T) { for _, c := range testCases { t.Run(c.name, func(t *testing.T) { deploymentController.K8sClient = c.fakeClient - returnedDeployments := deploymentController.GetDeploymentByLabel(c.labelValue, "namespace") + returnedDeployments := deploymentController.GetDeploymentBySelectorInNamespace(c.selector, "namespace") sort.Slice(returnedDeployments, func(i, j int) bool { return returnedDeployments[i].Name > returnedDeployments[j].Name diff --git a/admiral/pkg/controller/admiral/rollouts.go b/admiral/pkg/controller/admiral/rollouts.go index ca927270..50e93f02 100644 --- a/admiral/pkg/controller/admiral/rollouts.go +++ b/admiral/pkg/controller/admiral/rollouts.go @@ -2,6 +2,7 @@ package admiral import ( "fmt" + "k8s.io/apimachinery/pkg/labels" "sync" "time" @@ -194,10 +195,14 @@ func (roc *RolloutController) Deleted(ojb interface{}) { roc.RolloutHandler.Deleted(rollout) } -func (d *RolloutController) GetRolloutByLabel(labelValue string, namespace string) []argo.Rollout { - matchLabel := common.GetGlobalTrafficDeploymentLabel() - labelOptions := meta_v1.ListOptions{} - labelOptions.LabelSelector = fmt.Sprintf("%s=%s", matchLabel, labelValue) +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) if err != nil { @@ -206,7 +211,7 @@ func (d *RolloutController) GetRolloutByLabel(labelValue string, namespace strin } if matchedRollouts.Items == nil { - return []argo.Rollout{} + return make([]argo.Rollout,0) } return matchedRollouts.Items diff --git a/admiral/pkg/controller/admiral/rollouts_test.go b/admiral/pkg/controller/admiral/rollouts_test.go index b0c8e14d..08b44fd9 100644 --- a/admiral/pkg/controller/admiral/rollouts_test.go +++ b/admiral/pkg/controller/admiral/rollouts_test.go @@ -195,11 +195,12 @@ func TestRolloutController_Deleted(t *testing.T) { } -func TestRolloutController_GetRolloutByLabel(t *testing.T) { +func TestRolloutController_GetRolloutBySelectorInNamespace(t *testing.T) { rollout := argo.Rollout{} rollout.Namespace = "namespace" rollout.Name = "fake-app-rollout-qal" rollout.Spec = argo.RolloutSpec{ + Selector: &v1.LabelSelector{MatchLabels: map[string]string{"identity": "app1"},}, Template: coreV1.PodTemplateSpec{ ObjectMeta: v1.ObjectMeta{ Labels: map[string]string{"identity": "app1", "env": "qal"}, @@ -212,6 +213,7 @@ func TestRolloutController_GetRolloutByLabel(t *testing.T) { rollout2.Namespace = "namespace" rollout2.Name = "fake-app-rollout-e2e" rollout2.Spec = argo.RolloutSpec{ + Selector: &v1.LabelSelector{MatchLabels: map[string]string{"identity": "app1"},}, Template: coreV1.PodTemplateSpec{ ObjectMeta: v1.ObjectMeta{ Labels: map[string]string{"identity": "app1", "env": "e2e"}, @@ -225,6 +227,7 @@ func TestRolloutController_GetRolloutByLabel(t *testing.T) { rollout3.Name = "fake-app-rollout-prf-1" rollout3.CreationTimestamp = v1.Now() rollout3.Spec = argo.RolloutSpec{ + Selector: &v1.LabelSelector{MatchLabels: map[string]string{"identity": "app1"},}, Template: coreV1.PodTemplateSpec{ ObjectMeta: v1.ObjectMeta{ Labels: map[string]string{"identity": "app1", "env": "prf"}, @@ -238,6 +241,7 @@ func TestRolloutController_GetRolloutByLabel(t *testing.T) { rollout4.Name = "fake-app-rollout-prf-2" rollout4.CreationTimestamp = v1.Date(2020, 1, 1, 1, 1, 1, 1, time.UTC) rollout4.Spec = argo.RolloutSpec{ + Selector: &v1.LabelSelector{MatchLabels: map[string]string{"identity": "app2"},}, Template: coreV1.PodTemplateSpec{ ObjectMeta: v1.ObjectMeta{ Labels: map[string]string{"identity": "app2", "env": "prf"}, @@ -259,37 +263,37 @@ func TestRolloutController_GetRolloutByLabel(t *testing.T) { name string expectedRollouts []argo.Rollout fakeClient argoprojv1alpha1.ArgoprojV1alpha1Interface - labelValue string + selector map[string]string }{ { name: "Get one", expectedRollouts: []argo.Rollout{rollout}, fakeClient: oneRolloutClient, - labelValue: "app1", + selector: map[string]string {"identity": "app1", common.RolloutPodHashLabel: "random-hash"}, }, { name: "Get one from long list", expectedRollouts: []argo.Rollout{rollout4}, fakeClient: allRolloutsClient, - labelValue: "app2", + selector: map[string]string {"identity": "app2", common.RolloutPodHashLabel: "random-hash"}, }, { name: "Get many from long list", expectedRollouts: []argo.Rollout{rollout, rollout3, rollout2}, fakeClient: allRolloutsClient, - labelValue: "app1", + selector: map[string]string {"identity": "app1", common.RolloutPodHashLabel: "random-hash"}, }, { name: "Get none from long list", expectedRollouts: []argo.Rollout{}, fakeClient: allRolloutsClient, - labelValue: "app3", + selector: map[string]string {"identity": "app3", common.RolloutPodHashLabel: "random-hash"}, }, { name: "Get none from empty list", expectedRollouts: []argo.Rollout{}, fakeClient: noRolloutsClient, - labelValue: "app1", + selector: map[string]string {"identity": "app1"}, }, } @@ -297,7 +301,7 @@ func TestRolloutController_GetRolloutByLabel(t *testing.T) { for _, c := range testCases { t.Run(c.name, func(t *testing.T) { rolloutController.RolloutClient = c.fakeClient - returnedRollouts := rolloutController.GetRolloutByLabel(c.labelValue, "namespace") + returnedRollouts := rolloutController.GetRolloutBySelectorInNamespace(c.selector, "namespace") sort.Slice(returnedRollouts, func(i, j int) bool { return returnedRollouts[i].Name > returnedRollouts[j].Name diff --git a/admiral/pkg/controller/admiral/service.go b/admiral/pkg/controller/admiral/service.go index 8d741e16..5d39b732 100644 --- a/admiral/pkg/controller/admiral/service.go +++ b/admiral/pkg/controller/admiral/service.go @@ -2,6 +2,8 @@ package admiral import ( "fmt" + "github.com/prometheus/common/log" + "sort" "time" "github.com/istio-ecosystem/admiral/admiral/pkg/controller/common" @@ -19,6 +21,9 @@ import ( // Handler interface contains the methods that are required type ServiceHandler interface { + Added(obj *k8sV1.Service) + Updated(obj *k8sV1.Service) + Deleted(obj *k8sV1.Service) } type ServiceClusterEntry struct { @@ -70,10 +75,31 @@ func (s *serviceCache) getKey(service *k8sV1.Service) string { return service.Namespace } -func (s *serviceCache) Get(key string) *ServiceClusterEntry { +func (s *serviceCache) Get(key string) []*k8sV1.Service { s.mutex.Lock() defer s.mutex.Unlock() - return s.cache[key] + serviceClusterEntry := s.cache[key] + if serviceClusterEntry != nil { + return getOrderedServices (serviceClusterEntry.Service[key]) + } else { + return nil + } +} + +func getOrderedServices(serviceMap map[string]*k8sV1.Service) []*k8sV1.Service { + orderedServices := make([]*k8sV1.Service, 0, len(serviceMap)) + for _, value := range serviceMap { + orderedServices = append(orderedServices, value) + } + if len(orderedServices) > 1 { + sort.Slice(orderedServices, func(i, j int) bool { + iTime := orderedServices[i].CreationTimestamp + jTime := orderedServices[j].CreationTimestamp + log.Debugf("Service sorting name1=%s creationTime1=%v name2=%s creationTime2=%v", orderedServices[i].Name, iTime, orderedServices[j].Name, jTime) + return iTime.After(jTime.Time) + }) + } + return orderedServices } func (s *serviceCache) Delete(service *k8sV1.Service) { @@ -94,11 +120,11 @@ func (s *serviceCache) GetLoadBalancer(key string, namespace string) (string, in lb = "dummy.admiral.global" lbPort = common.DefaultMtlsPort ) - service := s.Get(namespace) - if service == nil || service.Service[namespace] == nil { + services := s.Get(namespace) + if len(services) == 0 { return lb, 0 } - for _, service := range service.Service[namespace] { + for _, service := range services { if service.Labels["app"] == key { loadBalancerStatus := service.Status.LoadBalancer.Ingress if len(loadBalancerStatus) > 0 { @@ -157,21 +183,21 @@ func NewServiceController(clusterID string, stopCh <-chan struct{}, handler Serv } func (s *ServiceController) Added(obj interface{}) { - HandleAddUpdateService(obj, s) + service := obj.(*k8sV1.Service) + s.Cache.Put(service) + s.ServiceHandler.Added(service) } func (s *ServiceController) Updated(obj interface{}, oldObj interface{}) { - HandleAddUpdateService(obj, s) -} - -func HandleAddUpdateService(obj interface{}, s *ServiceController) { service := obj.(*k8sV1.Service) s.Cache.Put(service) + s.ServiceHandler.Updated(service) } func (s *ServiceController) Deleted(obj interface{}) { service := obj.(*k8sV1.Service) s.Cache.Delete(service) + s.ServiceHandler.Deleted(service) } func (s *serviceCache) shouldIgnoreBasedOnLabels(service *k8sV1.Service) bool { diff --git a/admiral/pkg/controller/admiral/service_test.go b/admiral/pkg/controller/admiral/service_test.go index b2b439b3..fb2cd30f 100644 --- a/admiral/pkg/controller/admiral/service_test.go +++ b/admiral/pkg/controller/admiral/service_test.go @@ -75,22 +75,22 @@ func TestServiceCache_Put(t *testing.T) { if serviceCache.getKey(service) != "ns" { t.Errorf("Incorrect key. Got %v, expected ns", serviceCache.getKey(service)) } - if !cmp.Equal(serviceCache.Get("ns").Service["ns"][service.Name], service) { - t.Errorf("Incorrect service fount. Diff: %v", cmp.Diff(serviceCache.Get("ns").Service["ns"], service)) + if !cmp.Equal(serviceCache.Get("ns")[0], service) { + t.Errorf("Incorrect service found. Diff: %v", cmp.Diff(serviceCache.Get("ns")[0], service)) } - length := len(serviceCache.Get("ns").Service["ns"]) + length := len(serviceCache.Get("ns")) serviceCache.Put(service) if serviceCache.getKey(service) != "ns" { t.Errorf("Incorrect key. Got %v, expected ns", serviceCache.getKey(service)) } - if !cmp.Equal(serviceCache.Get("ns").Service["ns"][service.Name], service) { - t.Errorf("Incorrect service fount. Diff: %v", cmp.Diff(serviceCache.Get("ns").Service["ns"], service)) + if !cmp.Equal(serviceCache.Get("ns")[0], service) { + t.Errorf("Incorrect service found. Diff: %v", cmp.Diff(serviceCache.Get("ns")[0], service)) } - if (length) != len(serviceCache.Get("ns").Service["ns"]) { - t.Errorf("Re-added the same service. Cache length expected %v, got %v", length, len(serviceCache.Get("ns").Service["ns"])) + if (length) != len(serviceCache.Get("ns")) { + t.Errorf("Re-added the same service. Cache length expected %v, got %v", length, len(serviceCache.Get("ns"))) } serviceCache.Delete(service) @@ -299,3 +299,50 @@ func TestConcurrentGetAndPut(t *testing.T) { wg.Wait() } + +func TestGetOrderedServices(t *testing.T) { + + //Struct of test case info. Name is required. + testCases := []struct { + name string + services map[string]*v1.Service + expectedResult string + }{ + { + name: "Should return nil for nil input", + services: nil, + expectedResult: "", + }, + { + name: "Should return the only service", + services: map[string]*v1.Service { + "s1": {ObjectMeta: metaV1.ObjectMeta{Name: "s1", Namespace: "ns1", CreationTimestamp: metaV1.NewTime(time.Now())}}, + }, + expectedResult: "s1", + }, + { + name: "Should return the latest service by creationTime", + services: map[string]*v1.Service { + "s1": {ObjectMeta: metaV1.ObjectMeta{Name: "s1", Namespace: "ns1", CreationTimestamp: metaV1.NewTime(time.Now().Add(time.Duration(-15)))}}, + "s2": {ObjectMeta: metaV1.ObjectMeta{Name: "s2", Namespace: "ns1", CreationTimestamp: metaV1.NewTime(time.Now())}}, + }, + expectedResult: "s2", + }, + } + + //Run the test for every provided case + for _, c := range testCases { + t.Run(c.name, func(t *testing.T) { + result := getOrderedServices(c.services) + if c.expectedResult == "" && len(result) > 0 { + t.Errorf("Failed. Got %v, expected no service", result[0].Name) + } else if c.expectedResult != "" { + if len(result) > 0 && result[0].Name == c.expectedResult{ + //perfect + } else { + t.Errorf("Failed. Got %v, expected %v", result[0].Name, c.expectedResult) + } + } + }) + } +} diff --git a/admiral/pkg/controller/common/common.go b/admiral/pkg/controller/common/common.go index 94a6ae10..442a23b1 100644 --- a/admiral/pkg/controller/common/common.go +++ b/admiral/pkg/controller/common/common.go @@ -35,6 +35,7 @@ const ( AdmiralIgnoreAnnotation = "admiral.io/ignore" AdmiralCnameCaseSensitive = "admiral.io/cname-case-sensitive" BlueGreenRolloutPreviewPrefix = "preview" + RolloutPodHashLabel = "rollouts-pod-template-hash" ) type Event int diff --git a/admiral/pkg/test/mock.go b/admiral/pkg/test/mock.go index 3c231601..14a777d9 100644 --- a/admiral/pkg/test/mock.go +++ b/admiral/pkg/test/mock.go @@ -58,6 +58,10 @@ func (m *MockServiceHandler) Added(obj *k8sCoreV1.Service) { } +func (m *MockServiceHandler) Updated(obj *k8sCoreV1.Service) { + +} + func (m *MockServiceHandler) Deleted(obj *k8sCoreV1.Service) { }