feat: Add Perses Flag OU-571. Update logic in monitoring.go to have a…

…ll validations first before building the pluginInfo object. this makes the code easier to read and more maintainable.
rhobs · Feb 4, 2025 · 67b0edb · 67b0edb
1 parent 15ffcc9
commit 67b0edb
Show file tree

Hide file tree

Showing 2 changed files with 202 additions and 54 deletions.
diff --git a/pkg/controllers/uiplugin/monitoring.go b/pkg/controllers/uiplugin/monitoring.go
@@ -14,23 +14,60 @@ import (
 	uiv1alpha1 "github.com/rhobs/observability-operator/pkg/apis/uiplugin/v1alpha1"
 )
 
-func createMonitoringPluginInfo(plugin *uiv1alpha1.UIPlugin, namespace, name, image string, features []string) (*UIPluginInfo, error) {
-	config := plugin.Spec.Monitoring
-	if config == nil {
-		return nil, fmt.Errorf("monitoring configuration can not be empty for plugin type %s", plugin.Spec.Type)
+var AcmErrorMsg = `if you intend to enable "acm-alerting" thanos querier url and alertmanager url needs to be configured in the custom resource UIPlugin. `
+var PersesErrorMsg = `if you intend to enable "perses-dashboards" a perses service name and namespace needs to be configured in the custom resource UIPlugin.`
+var AlertmanagerEmptyMsg = "alertmanager location is empty for plugin type monitoring."
+var ThanosEmptyMsg = "thanosQuerier location is empty for plugin type monitoring."
+var PersesNameEmptyMsg = "persesName location is empty for plugin type monitoring."
+var PersesNamespaceEmptyMsg = "persesNamespace location is empty for plugin type monitoring."
+var IncompatibleFeaturesAndConfigsErrorMsg = "UIPlugin configurations are incompatible with feature flags"
+
+func getConfigError(config *uiv1alpha1.MonitoringConfig) (bool, bool, bool, string) {
+	errorSlice := []string{}
+	errorMessage := ""
+
+	// alertManager and thanosQuerier url configurations are required to enable 'acm-alerting'
+	validAlertManagerUrl := config.Alertmanager.Url != ""
+	if !validAlertManagerUrl {
+		errorSlice = append(errorSlice, AlertmanagerEmptyMsg)
 	}
 
-	// Get feature flag status determined from compatbility matrix
-	persesDashboardsFeatureEnabled := slices.Contains(features, "perses-dashboards")
-	acmAlertingFeatureEnabled := slices.Contains(features, "acm-alerting")
-	if !acmAlertingFeatureEnabled && !persesDashboardsFeatureEnabled {
-		return nil, fmt.Errorf("monitoring feature flags were not set, check cluster compatibility")
+	validThanosQuerierUrl := config.ThanosQuerier.Url != ""
+	if !validThanosQuerierUrl {
+		errorSlice = append(errorSlice, ThanosEmptyMsg)
+	}
+	isValidAcmAlertingConfig := validAlertManagerUrl && validThanosQuerierUrl
+
+	// perses name and namespace are required to enable 'perses-dashboards'
+	validPersesName := config.Perses.Name != ""
+	if !validPersesName {
+		errorSlice = append(errorSlice, PersesNameEmptyMsg)
+	}
+	validPersesNamespace := config.Perses.Namespace != ""
+	if !validPersesNamespace {
+		errorSlice = append(errorSlice, PersesNamespaceEmptyMsg)
+	}
+	isValidPersesConfig := validPersesName && validPersesNamespace
+
+	// build error message by converting slice into one string
+	if len(errorSlice) > 0 {
+		// Add extra information in the error message based on type of incorrect configurations
+		if !isValidPersesConfig {
+			errorSlice = append([]string{PersesErrorMsg}, errorSlice...)
+		}
+		if !isValidAcmAlertingConfig {
+			errorSlice = append([]string{AcmErrorMsg}, errorSlice...)
+		}
+		errorMessage = strings.Join(errorSlice, "")
 	}
 
-	invalidACMConfig := config.Alertmanager.Url == "" || config.ThanosQuerier.Url == ""
-	invalidPersesConfig := config.Perses.Name == "" || config.Perses.Namespace == ""
+	allConfigsInvalid := !isValidAcmAlertingConfig && !isValidPersesConfig
+
+	return allConfigsInvalid, isValidAcmAlertingConfig, isValidPersesConfig, errorMessage
+}
 
-	pluginInfo := &UIPluginInfo{
+func getBasePluginInfo(namespace, name, image string, features []string) *UIPluginInfo {
+	return &UIPluginInfo{
 		Image:       image,
 		Name:        name,
 		ConsoleName: "monitoring-console-plugin",
@@ -68,45 +105,34 @@ func createMonitoringPluginInfo(plugin *uiv1alpha1.UIPlugin, namespace, name, im
 			},
 		},
 	}
+}
 
-	if persesDashboardsFeatureEnabled && !invalidPersesConfig {
-		pluginInfo.Proxies = append(pluginInfo.Proxies, osv1.ConsolePluginProxy{
-			Alias:         "perses",
-			Authorization: "UserToken",
-			Endpoint: osv1.ConsolePluginProxyEndpoint{
-				Type: osv1.ProxyTypeService,
-				Service: &osv1.ConsolePluginProxyServiceConfig{
-					Name:      config.Perses.Name,
-					Namespace: config.Perses.Namespace,
-					Port:      8080,
-				},
-			},
-		})
-		pluginInfo.LegacyProxies = append(pluginInfo.LegacyProxies, osv1alpha1.ConsolePluginProxy{
-			Type:      "Service",
-			Alias:     "perses",
-			Authorize: true,
-			Service: osv1alpha1.ConsolePluginProxyServiceConfig{
-				Name:      config.Perses.Name,
-				Namespace: config.Perses.Namespace,
+func addPersesProxy(pluginInfo *UIPluginInfo, persesName string, persesNamespace string) {
+	pluginInfo.Proxies = append(pluginInfo.Proxies, osv1.ConsolePluginProxy{
+		Alias:         "perses",
+		Authorization: "UserToken",
+		Endpoint: osv1.ConsolePluginProxyEndpoint{
+			Type: osv1.ProxyTypeService,
+			Service: &osv1.ConsolePluginProxyServiceConfig{
+				Name:      persesName,
+				Namespace: persesNamespace,
 				Port:      8080,
 			},
-		})
-
-		if !acmAlertingFeatureEnabled || invalidACMConfig {
-			return pluginInfo, nil
-		}
-	}
-
-	if invalidACMConfig {
-		if config.Alertmanager.Url == "" {
-			return nil, fmt.Errorf("alertmanager location can not be empty for plugin type %s", plugin.Spec.Type)
-		}
-		if config.ThanosQuerier.Url == "" {
-			return nil, fmt.Errorf("thanosQuerier location can not be empty for plugin type %s", plugin.Spec.Type)
-		}
-	}
+		},
+	})
+	pluginInfo.LegacyProxies = append(pluginInfo.LegacyProxies, osv1alpha1.ConsolePluginProxy{
+		Type:      "Service",
+		Alias:     "perses",
+		Authorize: true,
+		Service: osv1alpha1.ConsolePluginProxyServiceConfig{
+			Name:      persesName,
+			Namespace: persesNamespace,
+			Port:      8080,
+		},
+	})
+}
 
+func addAcmAlertingProxy(pluginInfo *UIPluginInfo, name string, namespace string, config *uiv1alpha1.MonitoringConfig) {
 	pluginInfo.ExtraArgs = append(pluginInfo.ExtraArgs,
 		fmt.Sprintf("-alertmanager=%s", config.Alertmanager.Url),
 		fmt.Sprintf("-thanos-querier=%s", config.ThanosQuerier.Url),
@@ -159,6 +185,42 @@ func createMonitoringPluginInfo(plugin *uiv1alpha1.UIPlugin, namespace, name, im
 			},
 		},
 	)
+}
+
+func createMonitoringPluginInfo(plugin *uiv1alpha1.UIPlugin, namespace, name, image string, features []string) (*UIPluginInfo, error) {
+	config := plugin.Spec.Monitoring
+	if config == nil {
+		return nil, fmt.Errorf("monitoring configuration can not be empty for plugin type %s", plugin.Spec.Type)
+	}
+
+	// Validate at least one feature flag is present
+	persesDashboardsFeatureEnabled := slices.Contains(features, "perses-dashboards")
+	acmAlertingFeatureEnabled := slices.Contains(features, "acm-alerting")
+	if !acmAlertingFeatureEnabled && !persesDashboardsFeatureEnabled {
+		return nil, fmt.Errorf("monitoring feature flags were not set, check cluster compatibility")
+	}
+
+	// Validate UIPlugin configuration
+	allConfigsInvalid, validAcmAlertingConfig, validPersesConfig, errorMessage := getConfigError(config)
+	if allConfigsInvalid {
+		return nil, fmt.Errorf("%s", errorMessage)
+	}
+
+	// Validate at least one proxy can be added to monitoring plugin info
+	validPersesProxyConditions := persesDashboardsFeatureEnabled && validPersesConfig
+	validAcmAlertingProxyConditions := acmAlertingFeatureEnabled && validAcmAlertingConfig
+	invalidProxyConditions := !validPersesProxyConditions && !validAcmAlertingProxyConditions
+	if invalidProxyConditions {
+		return nil, fmt.Errorf("%s", IncompatibleFeaturesAndConfigsErrorMsg)
+	}
+
+	pluginInfo := getBasePluginInfo(namespace, name, image, features)
+	if validPersesProxyConditions {
+		addPersesProxy(pluginInfo, config.Perses.Name, config.Perses.Namespace)
+	}
+	if validAcmAlertingProxyConditions {
+		addAcmAlertingProxy(pluginInfo, name, namespace, config)
+	}
 
 	return pluginInfo, nil
 }

diff --git a/pkg/controllers/uiplugin/monitoring_test.go b/pkg/controllers/uiplugin/monitoring_test.go
@@ -57,6 +57,42 @@ var pluginConfigPerses = &uiv1alpha1.UIPlugin{
 	},
 }
 
+var pluginConfigPersesName = &uiv1alpha1.UIPlugin{
+	TypeMeta: metav1.TypeMeta{
+		APIVersion: "observability.openshift.io/v1alpha1",
+		Kind:       "UIPlugin",
+	},
+	ObjectMeta: metav1.ObjectMeta{
+		Name: "monitoring-plugin",
+	},
+	Spec: uiv1alpha1.UIPluginSpec{
+		Type: "monitoring",
+		Monitoring: &uiv1alpha1.MonitoringConfig{
+			Perses: uiv1alpha1.PersesReference{
+				Namespace: "perses-operator",
+			},
+		},
+	},
+}
+
+var pluginConfigPersesNameSpace = &uiv1alpha1.UIPlugin{
+	TypeMeta: metav1.TypeMeta{
+		APIVersion: "observability.openshift.io/v1alpha1",
+		Kind:       "UIPlugin",
+	},
+	ObjectMeta: metav1.ObjectMeta{
+		Name: "monitoring-plugin",
+	},
+	Spec: uiv1alpha1.UIPluginSpec{
+		Type: "monitoring",
+		Monitoring: &uiv1alpha1.MonitoringConfig{
+			Perses: uiv1alpha1.PersesReference{
+				Name: "perses-api-http",
+			},
+		},
+	},
+}
+
 var pluginConfigACM = &uiv1alpha1.UIPlugin{
 	TypeMeta: metav1.TypeMeta{
 		APIVersion: "observability.openshift.io/v1alpha1",
@@ -187,7 +223,7 @@ func TestCreateMonitoringPluginInfo(t *testing.T) {
 	t.Run("Test createMonitoringPluginInfo with missing URLs from thanos and alertmanager", func(t *testing.T) {
 		var features = []string{"acm-alerting", "perses-dashboards"}
 
-		// this should throw and error because thanosQuerier.URL and alertManager.URL are not set
+		// should not error because "perses-dashboards" feature enabled and persesName and persesNamespace are included in UIPlugin
 		pluginInfo, error := getPluginInfo(pluginConfigPerses, features)
 		alertmanagerFound, thanosFound, persesFound := containsProxy(pluginInfo)
 
@@ -197,33 +233,83 @@ func TestCreateMonitoringPluginInfo(t *testing.T) {
 		assert.Assert(t, persesFound == true)
 	})
 
+	t.Run("Test createMonitoringPluginInfo with acm-alerting flag and perses configuration", func(t *testing.T) {
+		var features = []string{"acm-alerting"}
+
+		pluginInfo, error := getPluginInfo(pluginConfigPerses, features)
+
+		// should throw an error because acm-alerting configurations are not given
+		assert.Assert(t, pluginInfo == nil)
+		assert.Equal(t, error.Error(), IncompatibleFeaturesAndConfigsErrorMsg)
+	})
+
+	t.Run("Test createMonitoringPluginInfo with missing acm flag", func(t *testing.T) {
+		var features = []string{"perses-dashboards"}
+
+		pluginInfo, error := getPluginInfo(pluginConfigACM, features)
+
+		// should throw an error because acm-alerting configurations are given but ACM flag is not set
+		assert.Assert(t, pluginInfo == nil)
+		assert.Equal(t, error.Error(), IncompatibleFeaturesAndConfigsErrorMsg)
+	})
+
 	t.Run("Test createMonitoringPluginInfo with missing URL from thanos", func(t *testing.T) {
 		var features = []string{"acm-alerting", "perses-dashboards"}
 
-		// this should throw and error because thanosQuerier.URL is not set
+		errorMessage := AcmErrorMsg + PersesErrorMsg + ThanosEmptyMsg + PersesNameEmptyMsg + PersesNamespaceEmptyMsg
+
+		// this should throw an error because thanosQuerier.URL is not set
 		pluginInfo, error := getPluginInfo(pluginConfigAlertmanager, features)
 		assert.Assert(t, pluginInfo == nil)
 		assert.Assert(t, error != nil)
-		assert.Equal(t, error.Error(), "thanosQuerier location can not be empty for plugin type monitoring")
+		assert.Equal(t, error.Error(), errorMessage)
 	})
 
 	t.Run("Test createMonitoringPluginInfo with missing URL from alertmanager ", func(t *testing.T) {
 		var features = []string{"acm-alerting", "perses-dashboards"}
 
-		// this should throw and error because alertManager.URL is not set
+		errorMessage := AcmErrorMsg + PersesErrorMsg + AlertmanagerEmptyMsg + PersesNameEmptyMsg + PersesNamespaceEmptyMsg
+
+		// this should throw an error because alertManager.URL is not set
 		pluginInfo, error := getPluginInfo(pluginConfigThanos, features)
 		assert.Assert(t, pluginInfo == nil)
 		assert.Assert(t, error != nil)
-		assert.Equal(t, error.Error(), "alertmanager location can not be empty for plugin type monitoring")
+		assert.Equal(t, error.Error(), errorMessage)
+	})
+
+	t.Run("Test createMonitoringPluginInfo with missing persesName ", func(t *testing.T) {
+		var features = []string{"acm-alerting", "perses-dashboards"}
+
+		errorMessage := AcmErrorMsg + PersesErrorMsg + AlertmanagerEmptyMsg + ThanosEmptyMsg + PersesNamespaceEmptyMsg
+
+		// this should throw an error because persesName is not set
+		pluginInfo, error := getPluginInfo(pluginConfigPersesNameSpace, features)
+		assert.Assert(t, pluginInfo == nil)
+		assert.Assert(t, error != nil)
+		assert.Equal(t, error.Error(), errorMessage)
+	})
+
+	t.Run("Test createMonitoringPluginInfo with missing persesNamespace ", func(t *testing.T) {
+		var features = []string{"acm-alerting", "perses-dashboards"}
+
+		errorMessage := AcmErrorMsg + PersesErrorMsg + AlertmanagerEmptyMsg + ThanosEmptyMsg + PersesNameEmptyMsg
+
+		// this should throw an error because persesNamespace is not set
+		pluginInfo, error := getPluginInfo(pluginConfigPersesName, features)
+		assert.Assert(t, pluginInfo == nil)
+		assert.Assert(t, error != nil)
+		assert.Equal(t, error.Error(), errorMessage)
 	})
 
 	t.Run("Test createMonitoringPluginInfo with malform UIPlugin custom resource", func(t *testing.T) {
 		var features = []string{"acm-alerting", "perses-dashboards"}
 
-		// this should throw and error because UIPlugin doesn't include alertmanager, thanos, or perses
+		errorMessage := AcmErrorMsg + PersesErrorMsg + AlertmanagerEmptyMsg + ThanosEmptyMsg + PersesNameEmptyMsg + PersesNamespaceEmptyMsg
+
+		// this should throw an error because UIPlugin doesn't include alertmanager, thanos, or perses
 		pluginInfo, error := getPluginInfo(pluginMalformed, features)
 		assert.Assert(t, pluginInfo == nil)
 		assert.Assert(t, error != nil)
-		assert.Equal(t, error.Error(), "alertmanager location can not be empty for plugin type monitoring")
+		assert.Equal(t, error.Error(), errorMessage)
 	})
 }