fix(plugins): properly handle version preference in Sanitize, add CRD plugin validations (#2162)

* fix(plugins): pick the latest plugin version in sanitize

* feat(plugins): add update method

* feat(plugins): implement PluginMap

* chore(plugins): switch Sanitize to PluginMap

* chore(controllers): switch plugin reconciler to PluginMap

* chore(plugins): deprecate unused methods for PluginList

* chore(plugins): do not return errors for Update method

Versions will be validated at a CRD level

* chore(plugins): add CRD validation for plugins

* chore: regenerate CRDs

* chore(plugins): update comments

* chore(plugins): add another test case for plugin update

* chore(plugins): update comments

* chore(plugins): deprecate `Update` method

* chore(plugins): improve variable naming in Reconcile

* fix(plugins): logging line

* fix(plugins): return sorted list in GetPluginList

Author: weisdd

api/v1beta1/plugin_list.go

@@ -2,14 +2,19 @@ package v1beta1
 
 import (
 	"fmt"
+	"maps"
+	"slices"
 	"sort"
 	"strings"
 
 	"github.com/blang/semver/v4"
 )
 
 type GrafanaPlugin struct {
-	Name    string `json:"name"`
+	// +kubebuilder:validation:MinLength=1
+	Name string `json:"name"`
+	// TODO: kubernetes 1.34+ supports isSemver function, we should migrate to it after 1.33 reaches EOL. For now, using the official pattern https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string
+	// +kubebuilder:validation:Pattern=`^((0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?|latest)$`
 	Version string `json:"version"`
 }
 
@@ -35,100 +40,106 @@ func (p GrafanaPlugin) String() string {
 	return fmt.Sprintf("%s %s", p.Name, p.Version)
 }
 
-type PluginList []GrafanaPlugin
+// Update updates the plugin to the requested version if it's valid and newer
+func (p *GrafanaPlugin) Update(version string) {
+	if p.Version == version {
+		return
+	}
 
-type PluginMap map[string]PluginList
+	if version == "" {
+		return
+	}
 
-func (l PluginList) String() string {
-	plugins := make(sort.StringSlice, 0, len(l))
+	if p.Version == PluginVersionLatest {
+		return
+	}
 
-	for _, plugin := range l {
-		plugins = append(plugins, plugin.String())
+	if version == PluginVersionLatest {
+		p.Version = version
+
+		return
 	}
 
-	sort.Sort(plugins)
+	// NOTE: We should not see that happening due to CRD validations
+	// Version is not valid, so don't do anything
+	requestedVersion, err := semver.Parse(version)
+	if err != nil {
+		return
+	}
 
-	return strings.Join(plugins, ",")
-}
+	// NOTE: We should not see that happening due to CRD validations
+	// Helps to recover in case we have previously stored invalid version
+	listedVersion, err := semver.Parse(p.Version)
+	if err != nil {
+		p.Version = version
+		return
+	}
 
-// Update update plugin version
-func (l PluginList) Update(plugin *GrafanaPlugin) {
-	for i, installedPlugin := range l {
-		if installedPlugin.Name == plugin.Name {
-			l[i].Version = plugin.Version
-			break
-		}
+	if listedVersion.Compare(requestedVersion) == -1 {
+		p.Version = version
+
+		return
 	}
 }
 
-// Sanitize remove duplicates and enforce semver
-func (l PluginList) Sanitize() PluginList {
-	var sanitized PluginList
+// Helps to simplify version consolidation
+type PluginMap map[string]GrafanaPlugin
 
-	for _, plugin := range l {
-		if plugin.HasInvalidVersion() {
+func (m PluginMap) Merge(plugins PluginList) {
+	for _, p := range plugins {
+		if p.HasInvalidVersion() {
 			continue
 		}
 
-		if !sanitized.HasSomeVersionOf(&plugin) {
-			sanitized = append(sanitized, plugin)
+		if plugin, ok := m[p.Name]; ok {
+			plugin.Update(p.Version)
+			m[p.Name] = plugin
+		} else {
+			m[p.Name] = p
 		}
 	}
-
-	return sanitized
 }
 
-// HasSomeVersionOf returns true if the list contains the same plugin in the exact or a different version
-func (l PluginList) HasSomeVersionOf(plugin *GrafanaPlugin) bool {
-	for _, listedPlugin := range l {
-		if listedPlugin.Name == plugin.Name {
-			return true
-		}
-	}
+func (m PluginMap) GetPluginList() PluginList {
+	plugins := slices.Collect(maps.Values(m))
+	slices.SortFunc(plugins, func(a, b GrafanaPlugin) int {
+		return strings.Compare(a.Name, b.Name)
+	})
 
-	return false
+	return plugins
 }
 
-// HasExactVersionOf returns true if the list contains the same plugin in the same version
-func (l PluginList) HasExactVersionOf(plugin *GrafanaPlugin) bool {
-	for _, listedPlugin := range l {
-		if listedPlugin.Name == plugin.Name && listedPlugin.Version == plugin.Version {
-			return true
-		}
-	}
+func NewPluginMap() PluginMap {
+	pm := PluginMap{}
 
-	return false
+	return pm
 }
 
-// HasNewerVersionOf returns true if the list contains the same plugin but in a newer version
-func (l PluginList) HasNewerVersionOf(plugin *GrafanaPlugin) (bool, error) {
-	for _, listedPlugin := range l {
-		if listedPlugin.Name != plugin.Name {
-			continue
-		}
+func NewPluginMapFromList(plugins PluginList) PluginMap {
+	pm := PluginMap{}
 
-		if listedPlugin.Version == PluginVersionLatest && plugin.Version != PluginVersionLatest {
-			return true, nil
-		}
+	pm.Merge(plugins)
 
-		if plugin.Version == PluginVersionLatest {
-			return false, nil
-		}
+	return pm
+}
 
-		listedVersion, err := semver.Parse(listedPlugin.Version)
-		if err != nil {
-			return false, err
-		}
+type PluginList []GrafanaPlugin
 
-		requestedVersion, err := semver.Parse(plugin.Version)
-		if err != nil {
-			return false, err
-		}
+func (l PluginList) String() string {
+	plugins := make(sort.StringSlice, 0, len(l))
 
-		if listedVersion.Compare(requestedVersion) == 1 {
-			return true, nil
-		}
+	for _, plugin := range l {
+		plugins = append(plugins, plugin.String())
 	}
 
-	return false, nil
+	sort.Sort(plugins)
+
+	return strings.Join(plugins, ",")
+}
+
+// Sanitize remove duplicates and enforce semver
+func (l PluginList) Sanitize() PluginList {
+	plugins := NewPluginMapFromList(l)
+
+	return plugins.GetPluginList()
 }