Introduce Masterminds/semver (#374)

* Update Version regex to support ranges

Fixes #345

Add positive and negative test cases.

Signed-off-by: Todd Short <tshort@redhat.com>

* Introduce Masterminds/semver

Fixes #346

Add support for Masterminds/semver for .spec.Version
This is a bit more entangled into the code than I expected,
most instances of bsemver were replaced.

Signed-off-by: Todd Short <tshort@redhat.com>

* fixup! Introduce Masterminds/semver

Signed-off-by: Todd Short <tshort@redhat.com>

---------

Signed-off-by: Todd Short <tshort@redhat.com>

api/v1alpha1/operator_types.go

@@ -29,7 +29,7 @@ type OperatorSpec struct {
 	PackageName string `json:"packageName"`
 
 	//+kubebuilder:validation:MaxLength:=64
-	//+kubebuilder:validation:Pattern=^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(-(0|[1-9]\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*)(\.(0|[1-9]\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*))*)?(\+([0-9a-zA-Z-]+(\.[0-9a-zA-Z-]+)*))?$
+	//+kubebuilder:validation:Pattern=`^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$`
 	//+kubebuilder:Optional
 	// Version is an optional semver constraint on the package version. If not specified, the latest version available of the package will be installed.
 	// If specified, the specific version of the package will be installed so long as it is available in any of the content sources available.

config/crd/bases/operators.operatorframework.io_operators.yaml

@@ -51,7 +51,7 @@ spec:
                   sources available. Examples: 1.2.3, 1.0.0-alpha, 1.0.0-rc.1 \n For
                   more information on semver, please see https://semver.org/"
                 maxLength: 64
-                pattern: ^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(-(0|[1-9]\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*)(\.(0|[1-9]\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*))*)?(\+([0-9a-zA-Z-]+(\.[0-9a-zA-Z-]+)*))?$
+                pattern: ^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$
                 type: string
             required:
             - packageName

go.mod

@@ -3,6 +3,7 @@ module github.com/operator-framework/operator-controller
 go 1.20
 
 require (
+	github.com/Masterminds/semver/v3 v3.2.0
 	github.com/blang/semver/v4 v4.0.0
 	github.com/go-logr/logr v1.2.4
 	github.com/onsi/ginkgo/v2 v2.11.0

go.sum

@@ -48,6 +48,8 @@ github.com/Azure/go-autorest/logger v0.2.0/go.mod h1:T9E3cAhj2VqvPOtCYAvby9aBXkZ
 github.com/Azure/go-autorest/tracing v0.6.0/go.mod h1:+vhtPC754Xsa23ID7GlGsrdKBpUA79WCAKPPZVC2DeU=
 github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
 github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo=
+github.com/Masterminds/semver/v3 v3.2.0 h1:3MEsd0SM6jqZojhjLWWeBY+Kcjy9i6MQAeY7YgDP83g=
+github.com/Masterminds/semver/v3 v3.2.0/go.mod h1:qvl/7zhW3nngYb5+80sSMF+FG2BjYrf8m9wsX0PNOMQ=
 github.com/Microsoft/go-winio v0.4.11/go.mod h1:VhR8bwka0BXejwEJY73c50VrPtXAaKcyvVC4A4RozmA=
 github.com/Microsoft/go-winio v0.4.14/go.mod h1:qXqCSQ3Xa7+6tgxaGTIe4Kpcdsi+P8jBhyzoq1bpyYA=
 github.com/Microsoft/go-winio v0.4.15-0.20190919025122-fc70bd9a86b5/go.mod h1:tTuCMEN+UleMWgg9dVx4Hu52b1bJo+59jBh3ajtinzw=

internal/controllers/admission_test.go

@@ -61,6 +61,16 @@ var _ = Describe("Operator Spec Validations", func() {
 			"1.2.3-pre+bad_metadata",
 			"1.2.-3",
 			".1.2.3",
+			"<<1.2.3",
+			">>1.2.3",
+			">~1.2.3",
+			"==1.2.3",
+			"=!1.2.3",
+			"!1.2.3",
+			"1.Y",
+			">1.2.3 && <2.3.4",
+			">1.2.3;<2.3.4",
+			"1.2.3 - 2.3.4",
 		}
 		for _, invalidSemver := range invalidSemvers {
 			err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{
@@ -69,7 +79,54 @@ var _ = Describe("Operator Spec Validations", func() {
 			}))
 
 			Expect(err).To(HaveOccurred(), "expected error for invalid semver %q", invalidSemver)
-			Expect(err.Error()).To(ContainSubstring("spec.version in body should match '^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)(-(0|[1-9]\\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*)(\\.(0|[1-9]\\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*))*)?(\\+([0-9a-zA-Z-]+(\\.[0-9a-zA-Z-]+)*))?$'"))
+			// Don't need to include the whole regex, this should be enough to match the MasterMinds regex
+			Expect(err.Error()).To(ContainSubstring("spec.version in body should match '^(\\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\\^)"))
+		}
+	})
+	It("should pass if a valid semver range given", func() {
+		validSemvers := []string{
+			">=1.2.3",
+			"=>1.2.3",
+			">= 1.2.3",
+			">=v1.2.3",
+			">= v1.2.3",
+			"<=1.2.3",
+			"=<1.2.3",
+			"=1.2.3",
+			"!=1.2.3",
+			"<1.2.3",
+			">1.2.3",
+			"~1.2.2",
+			"~>1.2.3",
+			"^1.2.3",
+			"1.2.3",
+			"v1.2.3",
+			"1.x",
+			"1.X",
+			"1.*",
+			"1.2.x",
+			"1.2.X",
+			"1.2.*",
+			">=1.2.3 <2.3.4",
+			">=1.2.3,<2.3.4",
+			">=1.2.3, <2.3.4",
+			"<1.2.3||>2.3.4",
+			"<1.2.3|| >2.3.4",
+			"<1.2.3 ||>2.3.4",
+			"<1.2.3 || >2.3.4",
+			">1.0.0,<1.2.3 || >2.1.0",
+			"<1.2.3-abc >2.3.4-def",
+			"<1.2.3-abc+def >2.3.4-ghi+jkl",
+		}
+		for _, validSemver := range validSemvers {
+			op := operator(operatorsv1alpha1.OperatorSpec{
+				PackageName: "package",
+				Version:     validSemver,
+			})
+			err := cl.Create(ctx, op)
+			Expect(err).NotTo(HaveOccurred(), "expected success for semver range '%q': %w", validSemver, err)
+			err = cl.Delete(ctx, op)
+			Expect(err).NotTo(HaveOccurred(), "unexpected error deleting valid semver '%q': %w", validSemver, err)
 		}
 	})
 	It("should fail if an invalid channel name is given", func() {

internal/controllers/validators/validators.go

@@ -3,7 +3,7 @@ package validators
 import (
 	"fmt"
 
-	bsemver "github.com/blang/semver/v4"
+	mmsemver "github.com/Masterminds/semver/v3"
 
 	operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
 )
@@ -18,7 +18,7 @@ func validateSemver(operator *operatorsv1alpha1.Operator) error {
 	if operator.Spec.Version == "" {
 		return nil
 	}
-	if _, err := bsemver.Parse(operator.Spec.Version); err != nil {
+	if _, err := mmsemver.NewConstraint(operator.Spec.Version); err != nil {
 		return fmt.Errorf("invalid .spec.version: %w", err)
 	}
 	return nil

internal/resolution/entities/bundle_entity_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"testing"
 
+	mmsemver "github.com/Masterminds/semver/v3"
 	bsemver "github.com/blang/semver/v4"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -48,7 +49,7 @@ var _ = Describe("BundleEntity", func() {
 	})
 
 	Describe("Version", func() {
-		It("should return the bundle version if present", func() {
+		It("should return the bundle blang version if present", func() {
 			entity := input.NewEntity("operatorhub/prometheus/0.14.0", map[string]string{
 				"olm.package": "{\"packageName\":\"prometheus\",\"version\":\"0.14.0\"}",
 			})
@@ -57,6 +58,17 @@ var _ = Describe("BundleEntity", func() {
 			Expect(err).ToNot(HaveOccurred())
 			Expect(*version).To(Equal(bsemver.MustParse("0.14.0")))
 		})
+		It("should return the bundle Masterminds version if present", func() {
+			entity := input.NewEntity("operatorhub/prometheus/0.14.0", map[string]string{
+				"olm.package": "{\"packageName\":\"prometheus\",\"version\":\"0.14.0\"}",
+			})
+			bundleEntity := olmentity.NewBundleEntity(entity)
+			bVersion, err := bundleEntity.Version()
+			Expect(err).ToNot(HaveOccurred())
+			mVersion, err := mmsemver.NewVersion(bVersion.String())
+			Expect(err).ToNot(HaveOccurred())
+			Expect(*mVersion).To(Equal(*mmsemver.MustParse("0.14.0")))
+		})
 		It("should return an error if the property is not found", func() {
 			entity := input.NewEntity("operatorhub/prometheus/0.14.0", map[string]string{})
 			bundleEntity := olmentity.NewBundleEntity(entity)

internal/resolution/util/predicates/predicates.go

@@ -1,6 +1,7 @@
 package predicates
 
 import (
+	mmsemver "github.com/Masterminds/semver/v3"
 	bsemver "github.com/blang/semver/v4"
 	"github.com/operator-framework/deppy/pkg/deppy/input"
 
@@ -18,7 +19,26 @@ func WithPackageName(packageName string) input.Predicate {
 	}
 }
 
-func InSemverRange(semverRange bsemver.Range) input.Predicate {
+func InMastermindsSemverRange(semverRange *mmsemver.Constraints) input.Predicate {
+	return func(entity *input.Entity) bool {
+		bundleEntity := olmentity.NewBundleEntity(entity)
+		bVersion, err := bundleEntity.Version()
+		if err != nil {
+			return false
+		}
+		// No error should occur here because the simple version was successfully parsed by blang
+		// We are unaware of any tests cases that would cause one to fail but not the other
+		// This will cause code coverage to drop for this line. We don't ignore the error because
+		// there might be that one extreme edge case that might cause one to fail but not the other
+		mVersion, err := mmsemver.NewVersion(bVersion.String())
+		if err != nil {
+			return false
+		}
+		return semverRange.Check(mVersion)
+	}
+}
+
+func InBlangSemverRange(semverRange bsemver.Range) input.Predicate {
 	return func(entity *input.Entity) bool {
 		bundleEntity := olmentity.NewBundleEntity(entity)
 		bundleVersion, err := bundleEntity.Version()

internal/resolution/util/predicates/predicates_test.go

@@ -3,6 +3,7 @@ package predicates_test
 import (
 	"testing"
 
+	mmsemver "github.com/Masterminds/semver/v3"
 	bsemver "github.com/blang/semver/v4"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -33,20 +34,40 @@ var _ = Describe("Predicates", func() {
 		})
 	})
 
-	Describe("InSemverRange", func() {
+	Describe("InMastermindsSemverRange", func() {
+		It("should return true when the entity has the has version in the right range", func() {
+			entity := input.NewEntity("test", map[string]string{
+				property.TypePackage: `{"packageName": "mypackage", "version": "1.0.0"}`,
+			})
+			inRange, err := mmsemver.NewConstraint(">=1.0.0")
+			Expect(err).NotTo(HaveOccurred())
+			notInRange, err := mmsemver.NewConstraint(">=2.0.0")
+			Expect(err).NotTo(HaveOccurred())
+			Expect(predicates.InMastermindsSemverRange(inRange)(entity)).To(BeTrue())
+			Expect(predicates.InMastermindsSemverRange(notInRange)(entity)).To(BeFalse())
+		})
+		It("should return false when the entity does not have a version", func() {
+			entity := input.NewEntity("test", map[string]string{})
+			inRange, err := mmsemver.NewConstraint(">=1.0.0")
+			Expect(err).NotTo(HaveOccurred())
+			Expect(predicates.InMastermindsSemverRange(inRange)(entity)).To(BeFalse())
+		})
+	})
+
+	Describe("InBlangSemverRange", func() {
 		It("should return true when the entity has the has version in the right range", func() {
 			entity := input.NewEntity("test", map[string]string{
 				property.TypePackage: `{"packageName": "mypackage", "version": "1.0.0"}`,
 			})
 			inRange := bsemver.MustParseRange(">=1.0.0")
 			notInRange := bsemver.MustParseRange(">=2.0.0")
-			Expect(predicates.InSemverRange(inRange)(entity)).To(BeTrue())
-			Expect(predicates.InSemverRange(notInRange)(entity)).To(BeFalse())
+			Expect(predicates.InBlangSemverRange(inRange)(entity)).To(BeTrue())
+			Expect(predicates.InBlangSemverRange(notInRange)(entity)).To(BeFalse())
 		})
 		It("should return false when the entity does not have a version", func() {
 			entity := input.NewEntity("test", map[string]string{})
 			inRange := bsemver.MustParseRange(">=1.0.0")
-			Expect(predicates.InSemverRange(inRange)(entity)).To(BeFalse())
+			Expect(predicates.InBlangSemverRange(inRange)(entity)).To(BeFalse())
 		})
 	})
 

internal/resolution/variablesources/bundles_and_dependencies.go

@@ -91,7 +91,7 @@ func (b *BundlesAndDepsVariableSource) getEntityDependencies(ctx context.Context
 		if err != nil {
 			return nil, err
 		}
-		packageDependencyBundles, err := entitySource.Filter(ctx, input.And(predicates.WithPackageName(requiredPackage.PackageName), predicates.InSemverRange(semverRange)))
+		packageDependencyBundles, err := entitySource.Filter(ctx, input.And(predicates.WithPackageName(requiredPackage.PackageName), predicates.InBlangSemverRange(semverRange)))
 		if err != nil {
 			return nil, err
 		}

internal/resolution/variablesources/required_package.go

@@ -4,7 +4,7 @@ import (
 	"context"
 	"fmt"
 
-	bsemver "github.com/blang/semver/v4"
+	mmsemver "github.com/Masterminds/semver/v3"
 	"github.com/operator-framework/deppy/pkg/deppy"
 	"github.com/operator-framework/deppy/pkg/deppy/input"
 
@@ -21,14 +21,14 @@ type RequiredPackageVariableSourceOption func(*RequiredPackageVariableSource) er
 func InVersionRange(versionRange string) RequiredPackageVariableSourceOption {
 	return func(r *RequiredPackageVariableSource) error {
 		if versionRange != "" {
-			vr, err := bsemver.ParseRange(versionRange)
+			vr, err := mmsemver.NewConstraint(versionRange)
 			if err == nil {
 				r.versionRange = versionRange
-				r.predicates = append(r.predicates, predicates.InSemverRange(vr))
+				r.predicates = append(r.predicates, predicates.InMastermindsSemverRange(vr))
 				return nil
 			}
 
-			return fmt.Errorf("invalid version range '%s': %v", versionRange, err)
+			return fmt.Errorf("invalid version range '%s': %w", versionRange, err)
 		}
 		return nil
 	}

internal/resolution/variablesources/required_package_test.go

@@ -80,7 +80,7 @@ var _ = Describe("RequiredPackageVariableSource", func() {
 	It("should filter by version range", func() {
 		// recreate source with version range option
 		var err error
-		rpvs, err = variablesources.NewRequiredPackageVariableSource(packageName, variablesources.InVersionRange(">=1.0.0 !2.0.0 <3.0.0"))
+		rpvs, err = variablesources.NewRequiredPackageVariableSource(packageName, variablesources.InVersionRange(">=1.0.0 !=2.0.0 <3.0.0"))
 		Expect(err).NotTo(HaveOccurred())
 
 		variables, err := rpvs.GetVariables(context.TODO(), mockEntitySource)