Auto-detect CRD versions to deprecate (#5050)

* Auto-detect CRD versions to deprecate

Fixes #5045.

* Fix tests and PR feedback

Author: matthchr

v2/cmd/asoctl/internal/crd/cleaner.go

@@ -97,9 +97,8 @@ func (c *Cleaner) Run(ctx context.Context) error {
 		asoCRDsSeen++
 		newStoredVersions, deprecatedVersions := crdmanagement.GetDeprecatedStorageVersions(crd)
 
-		// If there is no new version found other than the matched version, we short circuit here, as there is no updated version found in the CRDs
-		if len(newStoredVersions) <= 0 {
-			// TODO: test?
+		// If there is no new version found other than alpha/beta, it means there wasn't a newer version installed/reconciled yet and we short circuit here, as there is no updated version found in the CRDs
+		if len(newStoredVersions) == 1 && (strings.HasPrefix(newStoredVersions[0], "v1alpha") || strings.HasPrefix(newStoredVersions[0], "v1beta")) {
 			return eris.Errorf("it doesn't look like your version of ASO is one that supports deprecating CRD %q, versions %q. Have you upgraded ASO yet?", crd.Name, deprecatedVersions)
 		}
 

v2/internal/crdmanagement/deprecation.go

@@ -7,49 +7,33 @@ package crdmanagement
 
 import (
 	"regexp"
+	"slices"
 
 	apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 )
 
-var deprecatedVersionRegexp = regexp.MustCompile(`((v1alpha1api|v1beta)\d{8}(preview)?(storage)?|v1beta1)`) // handcrafted (non-ARM) resources have v1beta1 version
-
-// deprecatedVersionsMap is a map of crd name to a collection of deprecated versions
-var deprecatedVersionsMap = map[string][]string{
-	"trustedaccessrolebindings.containerservice.azure.com": {"v1api20230202previewstorage"},
-	"managedclusters.containerservice.azure.com":           {"v1api20210501storage", "v1api20231102previewstorage"},
-	"managedclustersagentpools.containerservice.azure.com": {"v1api20210501storage", "v1api20231102previewstorage"},
-}
-
-// IsVersionDeprecated checks if a given version of a CRD is deprecated.
-func IsVersionDeprecated(crd apiextensions.CustomResourceDefinition, version string) bool {
-	if deprecatedVersionRegexp.MatchString(version) {
-		return true
-	}
-
-	for _, deprecatedVersion := range deprecatedVersionsMap[crd.Name] {
-		if deprecatedVersion == version {
-			return true
-		}
-	}
-
-	return false
-}
+var versionRegexp = regexp.MustCompile(`^v(1api|1alpha1api|1beta)?(\d{8}|\d)(preview)?(storage)?$`)
 
 // GetDeprecatedStorageVersions returns a new list of storedVersions by removing the deprecated versions
 // The first return value is the list of versions that are NOT deprecated.
 // The second return value is the list of versions that ARE deprecated.
 func GetDeprecatedStorageVersions(crd apiextensions.CustomResourceDefinition) ([]string, []string) {
 	storedVersions := crd.Status.StoredVersions
 
+	if len(storedVersions) <= 1 {
+		// No versions to remove
+		return storedVersions, nil
+	}
+
+	maxVersion := slices.MaxFunc(storedVersions, CompareVersions)
 	newStoredVersions := make([]string, 0, len(storedVersions))
 	var removedVersions []string
 	for _, version := range storedVersions {
-		if IsVersionDeprecated(crd, version) {
+		if version == maxVersion {
+			newStoredVersions = append(newStoredVersions, version)
+		} else {
 			removedVersions = append(removedVersions, version)
-			continue
 		}
-
-		newStoredVersions = append(newStoredVersions, version)
 	}
 
 	return newStoredVersions, removedVersions
@@ -66,3 +50,74 @@ func GetAllDeprecatedStorageVersions(crds map[string]apiextensions.CustomResourc
 	}
 	return result
 }
+
+// CompareVersions compares two group version strings, for example: v1api20230101preview, v1api20220101, v20220101, v1api20220101storage, and v20220101storage
+// and returns -1 if a < b, 0 if a == b, and 1 if a > b. The storage suffix is ignored in the comparison.
+// Preview versions are considered less than non-preview versions.
+func CompareVersions(a string, b string) int {
+	matchA := versionRegexp.FindStringSubmatch(a)
+	matchB := versionRegexp.FindStringSubmatch(b)
+
+	if matchA == nil || matchB == nil {
+		// If either version doesn't match the expected pattern, fall back to string comparison
+		// TODO: Do we actually want to do this?
+		if a < b {
+			return -1
+		} else if a > b {
+			return 1
+		}
+		return 0
+	}
+
+	// Prefer non-preview over preview
+	isAPreview := matchA[3] == "preview"
+	isBPreview := matchB[3] == "preview"
+
+	if isAPreview && !isBPreview {
+		return -1
+	} else if !isAPreview && isBPreview {
+		return 1
+	}
+
+	// Prefer everything over v1alpha1apiYYYYMMDD
+	hasV1Alpha1A := matchA[1] == "1alpha1api"
+	hasV1Alpha1B := matchB[1] == "1alpha1api"
+
+	if hasV1Alpha1A && !hasV1Alpha1B {
+		return -1
+	} else if !hasV1Alpha1A && hasV1Alpha1B {
+		return 1
+	}
+
+	// Prefer everything over v1betaYYYYMMDD
+	hasV1BetaA := matchA[1] == "1beta"
+	hasV1BetaB := matchB[1] == "1beta"
+
+	if hasV1BetaA && !hasV1BetaB {
+		return -1
+	} else if !hasV1BetaA && hasV1BetaB {
+		return 1
+	}
+
+	// Prefer newer date over older date
+	dateA := matchA[2]
+	dateB := matchB[2]
+
+	if dateA < dateB {
+		return -1
+	} else if dateA > dateB {
+		return 1
+	}
+
+	// Prefer vYYYYMMDD over v1apiYYYYMMDD
+	hasV1ApiA := matchA[1] == "1api"
+	hasV1ApiB := matchB[1] == "1api"
+
+	if hasV1ApiA && !hasV1ApiB {
+		return -1
+	} else if !hasV1ApiA && hasV1ApiB {
+		return 1
+	}
+
+	return 0
+}