Skip to content

Commit

Permalink
feat: remove acm-version check for acm-alerting ui feature (#677)
Browse files Browse the repository at this point in the history
  • Loading branch information
PeterYurkovich authored Feb 21, 2025
1 parent c5b39c1 commit 365e30d
Show file tree
Hide file tree
Showing 10 changed files with 22 additions and 88 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -498,14 +498,6 @@ spec:
verbs:
- get
- update
- apiGroups:
- operator.open-cluster-management.io
resources:
- multiclusterhubs
verbs:
- get
- list
- watch
- apiGroups:
- operator.openshift.io
resources:
Expand Down
8 changes: 0 additions & 8 deletions deploy/operator/observability-operator-cluster-role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -223,14 +223,6 @@ rules:
verbs:
- get
- update
- apiGroups:
- operator.open-cluster-management.io
resources:
- multiclusterhubs
verbs:
- get
- list
- watch
- apiGroups:
- operator.openshift.io
resources:
Expand Down
1 change: 0 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ require (
github.com/prometheus/common v0.60.1
github.com/rhobs/obo-prometheus-operator v0.77.1-rhobs1
github.com/rhobs/obo-prometheus-operator/pkg/apis/monitoring v0.77.1-rhobs1
github.com/stolostron/multiclusterhub-operator v0.0.0-20240626140553-4f1ed6be3b84
go.uber.org/zap v1.27.0
golang.org/x/exp v0.0.0-20240909161429-701f63a606c0
golang.org/x/mod v0.22.0
Expand Down
7 changes: 2 additions & 5 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ github.com/efficientgo/core v1.0.0-rc.2 h1:7j62qHLnrZqO3V3UA0AqOGd5d5aXV3AX6m/NZ
github.com/efficientgo/core v1.0.0-rc.2/go.mod h1:FfGdkzWarkuzOlY04VY+bGfb1lWrjaL6x/GLcQ4vJps=
github.com/emicklei/go-restful/v3 v3.12.1 h1:PJMDIM/ak7btuL8Ex0iYET9hxM3CI2sjZtzpL63nKAU=
github.com/emicklei/go-restful/v3 v3.12.1/go.mod h1:6n3XBCmQQb25CM2LCACGz8ukIrRry+4bhvbpWn3mrbc=
github.com/evanphx/json-patch v5.7.0+incompatible h1:vgGkfT/9f8zE6tvSCe74nfpAVDQ2tG6yudJd8LBksgI=
github.com/evanphx/json-patch v5.7.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk=
github.com/evanphx/json-patch v5.6.0+incompatible h1:jBYDEEiFBPxA0v50tFdvOzQQTCvpL6mnFh5mB2/l16U=
github.com/evanphx/json-patch v5.6.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk=
github.com/evanphx/json-patch/v5 v5.9.0 h1:kcBlZQbplgElYIlo/n1hJbls2z/1awpXxpRi0/FOJfg=
github.com/evanphx/json-patch/v5 v5.9.0/go.mod h1:VNkHZ/282BpEyt/tObQO8s5CMPmYYq14uClGH4abBuQ=
github.com/facette/natsort v0.0.0-20181210072756-2cd4dd1e2dcb h1:IT4JYU7k4ikYg1SCxNI1/Tieq/NFvh6dzLdgi7eu0tM=
Expand Down Expand Up @@ -91,7 +91,6 @@ github.com/go-openapi/swag v0.23.0/go.mod h1:esZ8ITTYEsH1V2trKHjAN8Ai7xHb8RV+YSZ
github.com/go-openapi/validate v0.24.0 h1:LdfDKwNbpB6Vn40xhTdNZAnfLECL81w+VX3BumrGD58=
github.com/go-openapi/validate v0.24.0/go.mod h1:iyeX1sEufmv3nPbBdX3ieNviWnOZaJ1+zquzJEf2BAQ=
github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY=
github.com/go-task/slim-sprig v0.0.0-20230315185526-52ccab3ef572 h1:tfuBGBXKqDEevZMzYi5KSi8KkcZtzBcTgAUUtapy0OI=
github.com/go-task/slim-sprig/v3 v3.0.0 h1:sUs3vkvUymDpBKi3qH1YSqBQk9+9D/8M2mN1vB6EwHI=
github.com/go-task/slim-sprig/v3 v3.0.0/go.mod h1:W848ghGpv3Qj3dhTPRyJypKRiqCdHZiAzKg9hl15HA8=
github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ=
Expand Down Expand Up @@ -244,8 +243,6 @@ github.com/spf13/cobra v1.8.1 h1:e5/vxKd/rZsfSJMUX1agtjeTDf+qv1/JdBF8gg5k9ZM=
github.com/spf13/cobra v1.8.1/go.mod h1:wHxEcudfqmLYa8iTfL+OuZPbBZkmvliBWKIezN3kD9Y=
github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA=
github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
github.com/stolostron/multiclusterhub-operator v0.0.0-20240626140553-4f1ed6be3b84 h1:kjoi1qzaohRJSzdtZVmLhxJsi9nQmpitjwwp83QcTF8=
github.com/stolostron/multiclusterhub-operator v0.0.0-20240626140553-4f1ed6be3b84/go.mod h1:fVXNVgAb4lcyAurs9qi3UG5bkpRCO2hYmEkj9s9++MY=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
Expand Down
2 changes: 0 additions & 2 deletions pkg/controllers/uiplugin/compatibility_matrix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ func TestLookupImageAndFeatures(t *testing.T) {
for _, tc := range []struct {
pluginType uiv1alpha1.UIPluginType
clusterVersion string
acmVersion string
expectedKey string
expectedErr error
expectedFeatures []string
Expand Down Expand Up @@ -201,7 +200,6 @@ func TestLookupImageAndFeatures(t *testing.T) {
{
pluginType: uiv1alpha1.TypeMonitoring,
clusterVersion: "v4.14.0-0.nightly-2024-06-06-064349",
acmVersion: "v2.11.3",
expectedKey: "ui-monitoring",
expectedFeatures: []string{},
expectedErr: nil,
Expand Down
20 changes: 1 addition & 19 deletions pkg/controllers/uiplugin/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,13 @@ package uiplugin
import (
"context"
"slices"
"strings"
"time"

"github.com/go-logr/logr"
configv1 "github.com/openshift/api/config/v1"
osv1 "github.com/openshift/api/console/v1"
osv1alpha1 "github.com/openshift/api/console/v1alpha1"
operatorv1 "github.com/openshift/api/operator/v1"
mchv1 "github.com/stolostron/multiclusterhub-operator/api/v1"
appsv1 "k8s.io/api/apps/v1"
v1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
Expand Down Expand Up @@ -75,9 +73,6 @@ const (
// RBAC for distributed tracing
// +kubebuilder:rbac:groups=tempo.grafana.com,resources=tempostacks;tempomonolithics,verbs=list

// RBAC for monitoring
// +kubebuilder:rbac:groups=operator.open-cluster-management.io,resources=multiclusterhubs,verbs=get;list;watch

// RBAC for logging view plugin
// +kubebuilder:rbac:groups=loki.grafana.com,resources=application;infrastructure;audit,verbs=get

Expand Down Expand Up @@ -214,19 +209,6 @@ func (rm resourceManager) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
}
}

multiClusterHubList := &mchv1.MultiClusterHubList{}
acmVersion := "acm version not found"
err = rm.k8sClient.List(ctx, multiClusterHubList, &client.ListOptions{})

// Multiple MultiClusterHub's are undefined behavior
if err == nil && len(multiClusterHubList.Items) == 1 {
multiClusterHub := multiClusterHubList.Items[0]
acmVersion = multiClusterHub.Status.CurrentVersion
if !strings.HasPrefix(acmVersion, "v") {
acmVersion = "v" + acmVersion
}
}

compatibilityInfo, err := lookupImageAndFeatures(plugin.Spec.Type, rm.clusterVersion)
if err != nil {
return ctrl.Result{}, err
Expand All @@ -251,7 +233,7 @@ func (rm resourceManager) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
return ctrl.Result{}, err
}

pluginInfo, err := PluginInfoBuilder(ctx, rm.k8sClient, plugin, rm.pluginConf, compatibilityInfo, acmVersion, rm.clusterVersion)
pluginInfo, err := PluginInfoBuilder(ctx, rm.k8sClient, plugin, rm.pluginConf, compatibilityInfo, rm.clusterVersion)

if err != nil {
logger.Error(err, "failed to reconcile plugin")
Expand Down
15 changes: 4 additions & 11 deletions pkg/controllers/uiplugin/monitoring.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,16 @@ import (
Requirements for ACM enablement
1. UIPlugin configuration requires acm.enabled, acm.thanosQuerier.Url, and acm.alertmanager.Url
2. OpenShift Container Platform requirement: v4.14+
3. Advanced Cluster Management: v2.11+
*/
func validateACMConfig(config *uiv1alpha1.MonitoringConfig, acmVersion string) bool {
func validateACMConfig(config *uiv1alpha1.MonitoringConfig) bool {
enabled := config.ACM.Enabled

// alertManager and thanosQuerier url configurations are required to enable 'acm-alerting'
validAlertManagerUrl := config.ACM.Alertmanager.Url != ""
validThanosQuerierUrl := config.ACM.ThanosQuerier.Url != ""
isValidAcmAlertingConfig := validAlertManagerUrl && validThanosQuerierUrl

// "acm-alerting" feature is supported in ACM v2.11+
if !strings.HasPrefix(acmVersion, "v") {
acmVersion = "v" + acmVersion
}
minACMVersionMet := semver.Compare(acmVersion, "v2.11") >= 0

return isValidAcmAlertingConfig && enabled && minACMVersionMet
return isValidAcmAlertingConfig && enabled
}

func validatePersesConfig(config *uiv1alpha1.MonitoringConfig) bool {
Expand Down Expand Up @@ -188,7 +181,7 @@ func addAcmAlertingProxy(pluginInfo *UIPluginInfo, name string, namespace string
)
}

func createMonitoringPluginInfo(plugin *uiv1alpha1.UIPlugin, namespace, name, image string, features []string, acmVersion string, clusterVersion string) (*UIPluginInfo, error) {
func createMonitoringPluginInfo(plugin *uiv1alpha1.UIPlugin, namespace, name, image string, features []string, clusterVersion 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)
Expand All @@ -198,7 +191,7 @@ func createMonitoringPluginInfo(plugin *uiv1alpha1.UIPlugin, namespace, name, im
}

// Validate feature configuration and cluster conditions support enablement
isValidAcmConfig := validateACMConfig(config, acmVersion)
isValidAcmConfig := validateACMConfig(config)
isValidPersesConfig := validatePersesConfig(config)
isValidIncidentsConfig := validateIncidentsConfig(config, clusterVersion)

Expand Down
43 changes: 13 additions & 30 deletions pkg/controllers/uiplugin/monitoring_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -290,17 +290,16 @@ func containsProxy(pluginInfo *UIPluginInfo) (bool, bool, bool) {
}

var features = []string{}
var acmVersion = "v2.11"
var clusterVersion = "v4.18"

func getPluginInfo(plugin *uiv1alpha1.UIPlugin, features []string, clusterVersion string, acmVersion string) (*UIPluginInfo, error) {
return createMonitoringPluginInfo(plugin, namespace, name, image, features, acmVersion, clusterVersion)
func getPluginInfo(plugin *uiv1alpha1.UIPlugin, features []string, clusterVersion string) (*UIPluginInfo, error) {
return createMonitoringPluginInfo(plugin, namespace, name, image, features, clusterVersion)
}

func TestCreateMonitoringPluginInfo(t *testing.T) {
/** Postive Test - ALL **/
t.Run("Test createMonitoringPluginInfo with all monitoring configurations", func(t *testing.T) {
pluginInfo, error := getPluginInfo(pluginConfigAll, features, clusterVersion, acmVersion)
pluginInfo, error := getPluginInfo(pluginConfigAll, features, clusterVersion)
assert.Assert(t, error == nil)

alertmanagerProxyFound, thanosProxyFound, persesProxyFound := containsProxy(pluginInfo)
Expand All @@ -316,7 +315,7 @@ func TestCreateMonitoringPluginInfo(t *testing.T) {

/** Postive Test - ACM **/
t.Run("Test createMonitoringPluginInfo with AMC configuration only", func(t *testing.T) {
pluginInfo, error := getPluginInfo(pluginConfigACM, features, clusterVersion, acmVersion)
pluginInfo, error := getPluginInfo(pluginConfigACM, features, clusterVersion)
assert.Assert(t, error == nil)

alertmanagerProxyFound, thanosProxyFound, persesProxyFound := containsProxy(pluginInfo)
Expand All @@ -330,25 +329,9 @@ func TestCreateMonitoringPluginInfo(t *testing.T) {
assert.Assert(t, incidentsFlagFound == false)
})

t.Run("Test validACMConfig() with valid and invalid acmVersions", func(t *testing.T) {

// UIPlugin monitoring ACM feature is only supported in v2.11+
assert.Assert(t, validateACMConfig(pluginConfigACM.Spec.Monitoring, "v2.11.3") == true)
assert.Assert(t, validateACMConfig(pluginConfigACM.Spec.Monitoring, "v2.11") == true)
assert.Assert(t, validateACMConfig(pluginConfigACM.Spec.Monitoring, "2.11") == true)
assert.Assert(t, validateACMConfig(pluginConfigACM.Spec.Monitoring, "v2.11") == true)

assert.Assert(t, validateACMConfig(pluginConfigACM.Spec.Monitoring, "2.10") == false)
assert.Assert(t, validateACMConfig(pluginConfigACM.Spec.Monitoring, "v2.10") == false)
assert.Assert(t, validateACMConfig(pluginConfigACM.Spec.Monitoring, "1.0.0") == false)
assert.Assert(t, validateACMConfig(pluginConfigACM.Spec.Monitoring, "v1.0.0") == false)

assert.Assert(t, validateACMConfig(pluginConfigACM.Spec.Monitoring, "acm version not found") == false)
})

/** Postive Test - Perses **/
t.Run("Test createMonitoringPluginInfo with Perses configuration only", func(t *testing.T) {
pluginInfo, error := getPluginInfo(pluginConfigPerses, features, clusterVersion, acmVersion)
pluginInfo, error := getPluginInfo(pluginConfigPerses, features, clusterVersion)
assert.Assert(t, error == nil)

alertmanagerProxyFound, thanosProxyFound, persesProxyFound := containsProxy(pluginInfo)
Expand All @@ -364,7 +347,7 @@ func TestCreateMonitoringPluginInfo(t *testing.T) {
})

t.Run("Test createMonitoringPluginInfo with Perses default namespace and namespace", func(t *testing.T) {
pluginInfo, error := getPluginInfo(pluginConfigPersesDefault, features, clusterVersion, acmVersion)
pluginInfo, error := getPluginInfo(pluginConfigPersesDefault, features, clusterVersion)
assert.Assert(t, error == nil)

alertmanagerProxyFound, thanosProxyFound, persesProxyFound := containsProxy(pluginInfo)
Expand All @@ -382,7 +365,7 @@ func TestCreateMonitoringPluginInfo(t *testing.T) {
t.Run("Test createMonitoringPluginInfo with Perses default serviceName", func(t *testing.T) {
// should not throw an error because serviceName is allowed to be empty
// a default serviceName will be assigned
pluginInfo, error := getPluginInfo(pluginConfigPersesDefaultServiceName, features, clusterVersion, acmVersion)
pluginInfo, error := getPluginInfo(pluginConfigPersesDefaultServiceName, features, clusterVersion)
assert.Assert(t, error == nil)

alertmanagerProxyFound, thanosProxyFound, persesProxyFound := containsProxy(pluginInfo)
Expand All @@ -399,7 +382,7 @@ func TestCreateMonitoringPluginInfo(t *testing.T) {
t.Run("Test createMonitoringPluginInfo with Perses default namespace", func(t *testing.T) {
// should not throw an error because namespace is allowed to be empty
// a default namespace will be assigned
pluginInfo, error := getPluginInfo(pluginConfigPersesDefaultNamespace, features, clusterVersion, acmVersion)
pluginInfo, error := getPluginInfo(pluginConfigPersesDefaultNamespace, features, clusterVersion)
assert.Assert(t, error == nil)

alertmanagerProxyFound, thanosProxyFound, persesProxyFound := containsProxy(pluginInfo)
Expand All @@ -415,7 +398,7 @@ func TestCreateMonitoringPluginInfo(t *testing.T) {

/** Postive Test - Incidients **/
t.Run("Test createMonitoringPluginInfo with Incidents configuration only", func(t *testing.T) {
pluginInfo, error := getPluginInfo(pluginConfigIncidents, features, clusterVersion, acmVersion)
pluginInfo, error := getPluginInfo(pluginConfigIncidents, features, clusterVersion)
assert.Assert(t, error == nil)

alertmanagerProxyFound, thanosProxyFound, persesProxyFound := containsProxy(pluginInfo)
Expand Down Expand Up @@ -456,30 +439,30 @@ func TestCreateMonitoringPluginInfo(t *testing.T) {
/** Negative Tests - ACM **/
t.Run("Test createMonitoringPluginInfo with missing URL from thanos", func(t *testing.T) {
// this should throw an error because thanosQuerier.URL is not set
pluginInfo, error := getPluginInfo(pluginConfigAlertmanager, features, clusterVersion, acmVersion)
pluginInfo, error := getPluginInfo(pluginConfigAlertmanager, features, clusterVersion)
assert.Assert(t, pluginInfo == nil)
assert.Assert(t, error != nil)
})

t.Run("Test createMonitoringPluginInfo with missing URL from alertmanager ", func(t *testing.T) {
// this should throw an error because alertManager.URL is not set
pluginInfo, error := getPluginInfo(pluginConfigThanos, features, clusterVersion, acmVersion)
pluginInfo, error := getPluginInfo(pluginConfigThanos, features, clusterVersion)
assert.Assert(t, pluginInfo == nil)
assert.Assert(t, error != nil)
})

/** Negative Tests - Perses **/
t.Run("Test createMonitoringPluginInfo with missing Perses enabled field ", func(t *testing.T) {
// this should throw an error because 'enabled: true' is not set
pluginInfo, error := getPluginInfo(pluginConfigPersesEmpty, features, clusterVersion, acmVersion)
pluginInfo, error := getPluginInfo(pluginConfigPersesEmpty, features, clusterVersion)
assert.Assert(t, pluginInfo == nil)
assert.Assert(t, error != nil)
})

/** Negative Tests - ALL **/
t.Run("Test createMonitoringPluginInfo with malform UIPlugin custom resource", func(t *testing.T) {
// this should throw an error because UIPlugin doesn't include alertmanager, thanos, perses, or incidents
pluginInfo, error := getPluginInfo(pluginMalformed, features, clusterVersion, acmVersion)
pluginInfo, error := getPluginInfo(pluginMalformed, features, clusterVersion)
assert.Assert(t, pluginInfo == nil)
assert.Assert(t, error != nil)
})
Expand Down
4 changes: 2 additions & 2 deletions pkg/controllers/uiplugin/plugin_info_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ var pluginTypeToConsoleName = map[uiv1alpha1.UIPluginType]string{
uiv1alpha1.TypeLogging: "logging-view-plugin",
}

func PluginInfoBuilder(ctx context.Context, k client.Client, plugin *uiv1alpha1.UIPlugin, pluginConf UIPluginsConfiguration, compatibilityInfo CompatibilityEntry, acmVersion string, clusterVersion string) (*UIPluginInfo, error) {
func PluginInfoBuilder(ctx context.Context, k client.Client, plugin *uiv1alpha1.UIPlugin, pluginConf UIPluginsConfiguration, compatibilityInfo CompatibilityEntry, clusterVersion string) (*UIPluginInfo, error) {
image := pluginConf.Images[compatibilityInfo.ImageKey]
if image == "" {
return nil, fmt.Errorf("no image provided for plugin type %s with key %s", plugin.Spec.Type, compatibilityInfo.ImageKey)
Expand Down Expand Up @@ -160,7 +160,7 @@ func PluginInfoBuilder(ctx context.Context, k client.Client, plugin *uiv1alpha1.
return createLoggingPluginInfo(plugin, namespace, plugin.Name, image, compatibilityInfo.Features)

case uiv1alpha1.TypeMonitoring:
return createMonitoringPluginInfo(plugin, namespace, plugin.Name, image, compatibilityInfo.Features, acmVersion, clusterVersion)
return createMonitoringPluginInfo(plugin, namespace, plugin.Name, image, compatibilityInfo.Features, clusterVersion)
}

return nil, fmt.Errorf("plugin type not supported: %s", plugin.Spec.Type)
Expand Down
2 changes: 0 additions & 2 deletions pkg/operator/scheme.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
operatorv1 "github.com/openshift/api/operator/v1"
monv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
monitoringv1 "github.com/rhobs/obo-prometheus-operator/pkg/apis/monitoring/v1"
multiclusterhubv1 "github.com/stolostron/multiclusterhub-operator/api/v1"
corev1 "k8s.io/api/core/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand All @@ -30,7 +29,6 @@ func NewScheme(cfg *OperatorConfiguration) *runtime.Scheme {
utilruntime.Must(osv1.Install(scheme))
utilruntime.Must(osv1alpha1.Install(scheme))
utilruntime.Must(operatorv1.Install(scheme))
utilruntime.Must(multiclusterhubv1.AddToScheme(scheme))
utilruntime.Must(corev1.AddToScheme(scheme))
utilruntime.Must(monv1.AddToScheme(scheme))
}
Expand Down

0 comments on commit 365e30d

Please sign in to comment.