PR Feedback: Nick

- improve the Validate semantics
- avoid config.Get()

Author: jshaughn

business/istio_validations.go

@@ -257,9 +257,11 @@ func (in *validationInfo) forceCheckers() bool {
 	return in.hasBaseChange
 }
 
-// Validate runs a full validation on all objects. It returns an IstioValidations object with all the checks found when running all
-// the enabled checkers.
-func (in *IstioValidationsService) Validate(ctx context.Context, cluster string, vInfo *validationInfo) (models.IstioValidations, error) {
+// Validate runs a full validation on all objects. The first return variable is the "validationPerformed" bool, indicating whether or not
+// the validation checkers were run. It will return false if a changeMap is provided in vInfo and no config changes were detected for the
+// cluster. Otherwise, it will return true. When true the new "validations" are returned in the second return variable. When false
+// the second argument is nil.
+func (in *IstioValidationsService) Validate(ctx context.Context, cluster string, vInfo *validationInfo) (bool, models.IstioValidations, error) {
 	var end observability.EndFunc
 	ctx, end = observability.StartSpan(ctx, "getValidations",
 		observability.Attribute("package", "business"),
@@ -299,15 +301,15 @@ func (in *IstioValidationsService) Validate(ctx context.Context, cluster string,
 	}
 	istioConfigList, err := in.istioConfig.GetIstioConfigListForCluster(ctx, cluster, meta_v1.NamespaceAll, criteria)
 	if err != nil {
-		return nil, err
+		return false, nil, err
 	}
 	vInfo.clusterInfo.istioConfig = istioConfigList
 
 	// if change detection is enabled then decide if we need to run the checkers
 	if vInfo.changeDetectionEnabled() {
 		changeDetected := detectClusterConfigChange(vInfo)
 		if !changeDetected && !vInfo.forceCheckers() {
-			return nil, nil
+			return false, nil, nil
 		}
 	}
 
@@ -318,19 +320,19 @@ func (in *IstioValidationsService) Validate(ctx context.Context, cluster string,
 
 		err := in.setNamespaceIstioConfig(vInfo)
 		if err != nil {
-			return nil, err
+			return false, nil, err
 		}
 
 		if err := in.setNonLocalMTLSConfig(vInfo); err != nil {
-			return nil, err
+			return false, nil, err
 		}
 
 		objectCheckers := in.getAllObjectCheckers(vInfo)
 
 		validations.MergeValidations(runObjectCheckers(objectCheckers))
 	}
 
-	return validations, nil
+	return true, validations, nil
 }
 
 // toWorkloadMap takes a list of workloads from different namespaces, and returns a map: namespace => models.Workloads

business/istio_validations_perf_test.go

@@ -70,9 +70,10 @@ func TestGetValidationsPerf(t *testing.T) {
 	now := time.Now()
 	vInfo, err := vs.NewValidationInfo(context.TODO(), []string{conf.KubernetesConfig.ClusterName}, nil)
 	require.NoError(err)
-	validations, err := vs.Validate(context.TODO(), conf.KubernetesConfig.ClusterName, vInfo)
+	validationPerformed, validations, err := vs.Validate(context.TODO(), conf.KubernetesConfig.ClusterName, vInfo)
 	require.NoError(err)
 	log.Debugf("Validation Performance test took %f seconds for %d namespaces", time.Since(now).Seconds(), numNs)
+	assert.True(validationPerformed)
 	assert.NotEmpty(validations)
 }
 
@@ -182,7 +183,7 @@ func BenchmarkValidate(b *testing.B) {
 
 	b.ResetTimer()
 	for n := 0; n < b.N; n++ {
-		_, err = vs.Validate(context.TODO(), conf.KubernetesConfig.ClusterName, vInfo)
+		_, _, err = vs.Validate(context.TODO(), conf.KubernetesConfig.ClusterName, vInfo)
 		if err != nil {
 			b.Fatal(err)
 		}

business/istio_validations_test.go

@@ -65,8 +65,9 @@ func TestGetNamespaceValidations(t *testing.T) {
 	var changeMap = map[string]string{}
 	vInfo, err := vs.NewValidationInfo(context.Background(), []string{conf.KubernetesConfig.ClusterName}, changeMap)
 	require.NoError(err)
-	validations, err := vs.Validate(context.Background(), conf.KubernetesConfig.ClusterName, vInfo)
+	validationPerformed, validations, err := vs.Validate(context.Background(), conf.KubernetesConfig.ClusterName, vInfo)
 	require.NoError(err)
+	assert.True(validationPerformed)
 	vs.kialiCache.Validations().Replace(validations)
 
 	validations, err = vs.GetValidations(context.TODO(), conf.KubernetesConfig.ClusterName)
@@ -77,17 +78,19 @@ func TestGetNamespaceValidations(t *testing.T) {
 	// simulate a reconcile w/o a config change should skip running the checkers (new vInfo but re-use the changemap)
 	vInfo, err = vs.NewValidationInfo(context.Background(), []string{conf.KubernetesConfig.ClusterName}, changeMap)
 	require.NoError(err)
-	validations, err = vs.Validate(context.Background(), conf.KubernetesConfig.ClusterName, vInfo)
+	validationPerformed, validations, err = vs.Validate(context.Background(), conf.KubernetesConfig.ClusterName, vInfo)
 	require.NoError(err)
+	assert.False(validationPerformed)
 	assert.Nil(validations)
 
 	// refresh the config but keep the changeMap, and we should see new validations. (note PeerAuthentication config updates its ResourceVersion)
 	vs = mockCombinedValidationService(t, conf, fakeIstioConfigList(),
 		[]string{"details.test.svc.cluster.local", "product.test.svc.cluster.local", "product2.test.svc.cluster.local", "customer.test.svc.cluster.local"})
 	vInfo, err = vs.NewValidationInfo(context.Background(), []string{conf.KubernetesConfig.ClusterName}, changeMap)
 	require.NoError(err)
-	validations, err = vs.Validate(context.Background(), conf.KubernetesConfig.ClusterName, vInfo)
+	validationPerformed, validations, err = vs.Validate(context.Background(), conf.KubernetesConfig.ClusterName, vInfo)
 	require.NoError(err)
+	assert.True(validationPerformed)
 	assert.NotNil(validations)
 }
 
@@ -723,8 +726,9 @@ func TestValidatingSingleObjectUpdatesList(t *testing.T) {
 
 	vInfo, err := vs.NewValidationInfo(context.Background(), []string{conf.KubernetesConfig.ClusterName}, nil)
 	require.NoError(err)
-	validations, err := vs.Validate(context.Background(), conf.KubernetesConfig.ClusterName, vInfo)
+	validationPerformed, validations, err := vs.Validate(context.Background(), conf.KubernetesConfig.ClusterName, vInfo)
 	require.NoError(err)
+	assert.True(validationPerformed)
 	vs.kialiCache.Validations().Replace(validations)
 
 	currentValidations, err := vs.GetValidations(context.Background(), conf.KubernetesConfig.ClusterName)
@@ -740,8 +744,9 @@ func TestValidatingSingleObjectUpdatesList(t *testing.T) {
 	// make sure validations are updated in a cache before retrieving them
 	vInfo, err = vs.NewValidationInfo(context.Background(), []string{conf.KubernetesConfig.ClusterName}, nil)
 	require.NoError(err)
-	validations, err = vs.Validate(context.Background(), conf.KubernetesConfig.ClusterName, vInfo)
+	validationPerformed, validations, err = vs.Validate(context.Background(), conf.KubernetesConfig.ClusterName, vInfo)
 	require.NoError(err)
+	assert.True(validationPerformed)
 	vs.kialiCache.Validations().Replace(validations)
 
 	updatedValidations, _, err := vs.ValidateIstioObject(context.Background(), conf.KubernetesConfig.ClusterName, "test", kubernetes.VirtualServices, "product-vs")

business/workloads.go

@@ -282,10 +282,9 @@ func (in *WorkloadService) GetWorkloadList(ctx context.Context, criteria Workloa
 		return *workloadList, err
 	}
 
-	config := config.Get()
 	for _, w := range ws {
 		wItem := &models.WorkloadListItem{Health: *models.EmptyWorkloadHealth()}
-		wItem.ParseWorkload(w, config)
+		wItem.ParseWorkload(w, in.config)
 		if istioConfigList, ok := istioConfigMap[cluster]; ok && criteria.IncludeIstioResources {
 			wItem.IstioReferences = FilterUniqueIstioReferences(FilterWorkloadReferences(in.config, wItem.Labels, istioConfigList, cluster))
 		}
@@ -1432,7 +1431,7 @@ func (in *WorkloadService) fetchWorkloadsFromCluster(ctx context.Context, cluste
 			// Add the Proxy Status to the workload
 			for _, pod := range w.Pods {
 				isWaypoint := w.IsWaypoint()
-				if config.Get().ExternalServices.Istio.IstioAPIEnabled && (pod.HasIstioSidecar() || isWaypoint) {
+				if in.config.ExternalServices.Istio.IstioAPIEnabled && (pod.HasIstioSidecar() || isWaypoint) {
 					pod.ProxyStatus = in.businessLayer.ProxyStatus.GetPodProxyStatus(cluster, namespace, pod.Name, !isWaypoint)
 				}
 				// Add the Proxy Status to the workload
@@ -2122,7 +2121,7 @@ func (in *WorkloadService) fetchWorkload(ctx context.Context, criteria WorkloadC
 
 		// Add the Proxy Status to the workload
 		for _, pod := range w.Pods {
-			if config.Get().ExternalServices.Istio.IstioAPIEnabled && (pod.HasIstioSidecar() || isWaypoint) {
+			if in.config.ExternalServices.Istio.IstioAPIEnabled && (pod.HasIstioSidecar() || isWaypoint) {
 				pod.ProxyStatus = in.businessLayer.ProxyStatus.GetPodProxyStatus(criteria.Cluster, criteria.Namespace, pod.Name, !isWaypoint)
 			}
 			// If Ambient is enabled for pod, check if has any Waypoint proxy

controller/validations.go

@@ -117,6 +117,10 @@ func (r *ValidationsReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	version := r.kialiCache.Validations().Version()
 	newValidations := make(models.IstioValidations)
 	prevValidations := r.kialiCache.Validations().Items()
+
+	// If enabled, the same changeMap is used on each Reconcile. It holds the "resource version" of
+	// each object used in the validation, and is then used to look for version changes (or a change in
+	// the number of objects).  We keep it in the cache for sane access.
 	var changeMap business.ValidationChangeMap
 	if config.Get().ExternalServices.Istio.ValidationChangeDetectionEnabled {
 		changeMap = r.kialiCache.ValidationConfig().Items()
@@ -130,14 +134,14 @@ func (r *ValidationsReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	}
 
 	for _, cluster := range r.clusters {
-		clusterValidations, err := r.validationsService.Validate(ctx, cluster, vInfo)
+		validationPerformed, clusterValidations, err := r.validationsService.Validate(ctx, cluster, vInfo)
 		if err != nil {
-			log.Errorf("[ValidationsReconciler] Error creating validations for cluster %s: %s", cluster, err)
+			log.Errorf("[ValidationsReconciler] Error performing validation for cluster %s: %s", cluster, err)
 			return ctrl.Result{}, err
 		}
 
 		// if there have been no config changes for the cluster, just re-use the prior validations
-		if clusterValidations == nil {
+		if !validationPerformed {
 			log.Tracef("validations: no changes for cluster [%s], re-using", cluster)
 			clusterValidations = models.IstioValidations(prevValidations).FilterByCluster(cluster)
 		} else {