Code review comments and tests

istio-ecosystem · Jul 1, 2022 · 1ec3ecf · 1ec3ecf
1 parent 7d2098a
commit 1ec3ecf
Show file tree

Hide file tree

Showing 8 changed files with 46 additions and 35 deletions.
diff --git 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
@@ -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,15 +710,15 @@ 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,
 		},
 	}
 
 	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
@@ -124,57 +124,57 @@ 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)
 
 	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)
 	}
 
 
 	log.Infof("starting node controller clusterID: %v", clusterID)
 	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
@@ -150,36 +150,41 @@ 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)
 	}
 }
 
 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)
 	}
 }
 
 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
@@ -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
@@ -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
@@ -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
@@ -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