From 1ec3ecfce87b833875158948bfcdcf3cde96f3c3 Mon Sep 17 00:00:00 2001 From: aattuluri Date: Fri, 1 Jul 2022 15:14:54 -0700 Subject: [PATCH] Code review comments and tests --- admiral/pkg/clusters/handler.go | 2 +- admiral/pkg/clusters/handler_test.go | 7 +++---- admiral/pkg/clusters/registry.go | 18 ++++++++-------- admiral/pkg/clusters/types.go | 21 ++++++++++++------- admiral/pkg/controller/admiral/deployment.go | 3 +++ admiral/pkg/controller/admiral/rollouts.go | 6 +++++- .../pkg/controller/admiral/service_test.go | 4 ++-- admiral/pkg/controller/common/common.go | 20 +++++++++--------- 8 files changed, 46 insertions(+), 35 deletions(-) diff --git a/admiral/pkg/clusters/handler.go b/admiral/pkg/clusters/handler.go index dbad07ee..6a90d5ac 100644 --- a/admiral/pkg/clusters/handler.go +++ b/admiral/pkg/clusters/handler.go @@ -884,7 +884,7 @@ func getServiceForRollout(rc *RemoteController, rollout *argo.Rollout) map[strin 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.ROLLOUT_POD_HASH_LABEL { + if lkey == common.RolloutPodHashLabel { continue } value, ok := rollout.Spec.Selector.MatchLabels[lkey] diff --git a/admiral/pkg/clusters/handler_test.go b/admiral/pkg/clusters/handler_test.go index f6202dee..b84dd78c 100644 --- a/admiral/pkg/clusters/handler_test.go +++ b/admiral/pkg/clusters/handler_test.go @@ -693,12 +693,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, } @@ -711,7 +710,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, @@ -719,7 +718,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, diff --git a/admiral/pkg/clusters/registry.go b/admiral/pkg/clusters/registry.go index d6eb0e7f..393a86f8 100644 --- a/admiral/pkg/clusters/registry.go +++ b/admiral/pkg/clusters/registry.go @@ -124,7 +124,7 @@ func (r *RemoteRegistry) createCacheController(clientConfig *rest.Config, cluste rc.ServiceController, err = admiral.NewServiceController(clusterID, stop, &ServiceHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, 0) if err != nil { - return fmt.Errorf(" Error with ServiceController controller init: %v", err) + return fmt.Errorf("error with ServiceController controller init: %v", err) } log.Infof("starting global traffic policy controller custerID: %v", clusterID) @@ -132,7 +132,7 @@ func (r *RemoteRegistry) createCacheController(clientConfig *rest.Config, cluste rc.GlobalTraffic, err = admiral.NewGlobalTrafficController(clusterID, stop, &GlobalTrafficHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, 0) if err != nil { - return fmt.Errorf(" Error with GlobalTrafficController controller init: %v", err) + return fmt.Errorf("error with GlobalTrafficController controller init: %v", err) } @@ -140,41 +140,41 @@ func (r *RemoteRegistry) createCacheController(clientConfig *rest.Config, cluste rc.NodeController, err = admiral.NewNodeController(clusterID, stop, &NodeHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig) if err != nil { - return fmt.Errorf(" Error with NodeController controller init: %v", err) + return fmt.Errorf("error with NodeController controller init: %v", err) } log.Infof("starting service entry controller for custerID: %v", clusterID) rc.ServiceEntryController, err = istio.NewServiceEntryController(clusterID, stop, &ServiceEntryHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, 0) if err != nil { - return fmt.Errorf(" Error with ServiceEntryController init: %v", err) + return fmt.Errorf("error with ServiceEntryController init: %v", err) } log.Infof("starting destination rule controller for custerID: %v", clusterID) rc.DestinationRuleController, err = istio.NewDestinationRuleController(clusterID, stop, &DestinationRuleHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, 0) if err != nil { - return fmt.Errorf(" Error with DestinationRuleController init: %v", err) + return fmt.Errorf("error with DestinationRuleController init: %v", err) } log.Infof("starting virtual service controller for custerID: %v", clusterID) rc.VirtualServiceController, err = istio.NewVirtualServiceController(clusterID, stop, &VirtualServiceHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, 0) if err != nil { - return fmt.Errorf(" Error with VirtualServiceController init: %v", err) + return fmt.Errorf("error with VirtualServiceController init: %v", err) } rc.SidecarController, err = istio.NewSidecarController(clusterID, stop, &SidecarHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, 0) if err != nil { - return fmt.Errorf(" Error with DestinationRuleController init: %v", err) + return fmt.Errorf("error with DestinationRuleController init: %v", err) } log.Infof("starting deployment controller clusterID: %v", clusterID) rc.DeploymentController, err = admiral.NewDeploymentController(clusterID, stop, &DeploymentHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, resyncPeriod) if err != nil { - return fmt.Errorf(" Error with DeploymentController controller init: %v", err) + return fmt.Errorf("error with DeploymentController controller init: %v", err) } if r.AdmiralCache == nil { @@ -184,7 +184,7 @@ func (r *RemoteRegistry) createCacheController(clientConfig *rest.Config, cluste rc.RolloutController, err = admiral.NewRolloutsController(clusterID, stop, &RolloutHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, resyncPeriod) if err != nil { - return fmt.Errorf(" Error with Rollout controller init: %v", err) + return fmt.Errorf("error with Rollout controller init: %v", err) } } diff --git a/admiral/pkg/clusters/types.go b/admiral/pkg/clusters/types.go index 16fb5658..e39d4675 100644 --- a/admiral/pkg/clusters/types.go +++ b/admiral/pkg/clusters/types.go @@ -150,7 +150,7 @@ func (sh *ServiceHandler) Added(obj *k8sV1.Service) { log.Infof(LogFormat, "Added", "service", obj.Name, sh.ClusterID, "received") err := HandleEventForService(obj, sh.RemoteRegistry, sh.ClusterID) if err != nil { - log.Infof(err.Error()) + log.Errorf(LogErrFormat, "Error", "service", obj.Name, sh.ClusterID, err) } } @@ -158,7 +158,7 @@ func (sh *ServiceHandler) Updated(obj *k8sV1.Service) { log.Infof(LogFormat, "Updated", "service", obj.Name, sh.ClusterID, "received") err := HandleEventForService(obj, sh.RemoteRegistry, sh.ClusterID) if err != nil { - log.Infof(err.Error()) + log.Errorf(LogErrFormat, "Error", "service", obj.Name, sh.ClusterID, err) } } @@ -166,20 +166,25 @@ func (sh *ServiceHandler) Deleted(obj *k8sV1.Service) { log.Infof(LogFormat, "Deleted", "service", obj.Name, sh.ClusterID, "received") err := HandleEventForService(obj, sh.RemoteRegistry, sh.ClusterID) if err != nil { - log.Infof(err.Error()) + log.Errorf(LogErrFormat, "Error", "service", obj.Name, sh.ClusterID, err) } } func HandleEventForService(svc *k8sV1.Service, remoteRegistry *RemoteRegistry, clusterName string) error { + if svc.Spec.Selector == nil { + return errors.New("selector missing on service"); + } matchingDeployements := remoteRegistry.RemoteControllers[clusterName].DeploymentController.GetDeploymentBySelectorInNamespace(svc.Spec.Selector, svc.Namespace) - for _, deployment := range matchingDeployements { - HandleEventForDeployment(admiral.Update, &deployment, remoteRegistry, clusterName) + if len(matchingDeployements) > 0 { + for _, deployment := range matchingDeployements { + HandleEventForDeployment(admiral.Update, &deployment, remoteRegistry, clusterName) + } } if common.GetAdmiralParams().ArgoRolloutsEnabled { - rollouts := remoteRegistry.RemoteControllers[clusterName].RolloutController.GetRolloutBySelectorInNamespace(svc.Spec.Selector, svc.Namespace) + matchingRollouts := remoteRegistry.RemoteControllers[clusterName].RolloutController.GetRolloutBySelectorInNamespace(svc.Spec.Selector, svc.Namespace) - if len(rollouts) > 0 { - for _, rollout := range rollouts { + if len(matchingRollouts) > 0 { + for _, rollout := range matchingRollouts { HandleEventForRollout(admiral.Update, &rollout, remoteRegistry, clusterName) } } diff --git a/admiral/pkg/controller/admiral/deployment.go b/admiral/pkg/controller/admiral/deployment.go index 95ffc946..84f3ab35 100644 --- a/admiral/pkg/controller/admiral/deployment.go +++ b/admiral/pkg/controller/admiral/deployment.go @@ -191,6 +191,9 @@ func (d *DeploymentController) GetDeploymentBySelectorInNamespace(serviceSelecto var filteredDeployments = make([]k8sAppsV1.Deployment, 0) for _, deployment := range matchedDeployments.Items { + if deployment.Spec.Selector == nil || deployment.Spec.Selector.MatchLabels == nil { + continue + } if reflect.DeepEqual(deployment.Spec.Selector.MatchLabels, serviceSelector) { filteredDeployments = append(filteredDeployments, deployment) } diff --git a/admiral/pkg/controller/admiral/rollouts.go b/admiral/pkg/controller/admiral/rollouts.go index 9924533a..11327408 100644 --- a/admiral/pkg/controller/admiral/rollouts.go +++ b/admiral/pkg/controller/admiral/rollouts.go @@ -210,11 +210,15 @@ func (d *RolloutController) GetRolloutBySelectorInNamespace(serviceSelector map[ } for _, rollout := range matchedRollouts.Items { + + if rollout.Spec.Selector == nil || rollout.Spec.Selector.MatchLabels == nil { + continue + } 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 == common.ROLLOUT_POD_HASH_LABEL { + if lkey == common.RolloutPodHashLabel { continue } value, ok := rollout.Spec.Selector.MatchLabels[lkey] diff --git a/admiral/pkg/controller/admiral/service_test.go b/admiral/pkg/controller/admiral/service_test.go index 55bc340f..fb2cd30f 100644 --- a/admiral/pkg/controller/admiral/service_test.go +++ b/admiral/pkg/controller/admiral/service_test.go @@ -76,7 +76,7 @@ func TestServiceCache_Put(t *testing.T) { t.Errorf("Incorrect key. Got %v, expected ns", serviceCache.getKey(service)) } if !cmp.Equal(serviceCache.Get("ns")[0], service) { - t.Errorf("Incorrect service fount. Diff: %v", cmp.Diff(serviceCache.Get("ns")[0], service)) + t.Errorf("Incorrect service found. Diff: %v", cmp.Diff(serviceCache.Get("ns")[0], service)) } length := len(serviceCache.Get("ns")) @@ -87,7 +87,7 @@ func TestServiceCache_Put(t *testing.T) { t.Errorf("Incorrect key. Got %v, expected ns", serviceCache.getKey(service)) } if !cmp.Equal(serviceCache.Get("ns")[0], service) { - t.Errorf("Incorrect service fount. Diff: %v", cmp.Diff(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")) { t.Errorf("Re-added the same service. Cache length expected %v, got %v", length, len(serviceCache.Get("ns"))) diff --git a/admiral/pkg/controller/common/common.go b/admiral/pkg/controller/common/common.go index 49b8cc87..6a649dbe 100644 --- a/admiral/pkg/controller/common/common.go +++ b/admiral/pkg/controller/common/common.go @@ -26,16 +26,16 @@ const ( Slash = "/" DotLocalDomainSuffix = ".svc.cluster.local" Mesh = "mesh" - MulticlusterIngressGateway = "istio-multicluster-ingressgateway" - LocalAddressPrefix = "240.0" - NodeRegionLabel = "failure-domain.beta.kubernetes.io/region" - SpiffePrefix = "spiffe://" - SidecarEnabledPorts = "traffic.sidecar.istio.io/includeInboundPorts" - Default = "default" - AdmiralIgnoreAnnotation = "admiral.io/ignore" - AdmiralCnameCaseSensitive = "admiral.io/cname-case-sensitive" - BlueGreenRolloutPreviewPrefix = "preview" - ROLLOUT_POD_HASH_LABEL string = "rollouts-pod-template-hash" + MulticlusterIngressGateway = "istio-multicluster-ingressgateway" + LocalAddressPrefix = "240.0" + NodeRegionLabel = "failure-domain.beta.kubernetes.io/region" + SpiffePrefix = "spiffe://" + SidecarEnabledPorts = "traffic.sidecar.istio.io/includeInboundPorts" + Default = "default" + AdmiralIgnoreAnnotation = "admiral.io/ignore" + AdmiralCnameCaseSensitive = "admiral.io/cname-case-sensitive" + BlueGreenRolloutPreviewPrefix = "preview" + RolloutPodHashLabel string = "rollouts-pod-template-hash" ) type Event int