feat!: user strict semver parsing for image and commit selection, but…

… allow opt-out (#2441)

Signed-off-by: Kent Rancourt <[email protected]>

api/v1alpha1/warehouse_types.go

@@ -124,6 +124,16 @@ type GitSubscription struct {
 	// +kubebuilder:validation:MinLength=1
 	// +kubebuilder:validation:Pattern=`^\w+([-/]\w+)*$`
 	Branch string `json:"branch,omitempty" protobuf:"bytes,3,opt,name=branch"`
+	// StrictSemvers specifies whether only "strict" semver tags should be
+	// considered. A "strict" semver tag is one containing ALL of major, minor,
+	// and patch version components. This is enabled by default, but only has any
+	// effect when the CommitSelectionStrategy is SemVer. This should be disabled
+	// cautiously, as it creates the potential for any tag containing numeric
+	// characters only to be mistaken for a semver string containing the major
+	// version number only.
+	//
+	// +kubebuilder:default=true
+	StrictSemvers bool `json:"strictSemvers" protobuf:"varint,11,opt,name=strictSemvers"`
 	// SemverConstraint specifies constraints on what new tagged commits are
 	// considered in determining the newest commit of interest. The value in this
 	// field only has any effect when the CommitSelectionStrategy is SemVer. This
@@ -217,6 +227,17 @@ type ImageSubscription struct {
 	//
 	// +kubebuilder:default=SemVer
 	ImageSelectionStrategy ImageSelectionStrategy `json:"imageSelectionStrategy,omitempty" protobuf:"bytes,3,opt,name=imageSelectionStrategy"`
+	// StrictSemvers specifies whether only "strict" semver tags should be
+	// considered. A "strict" semver tag is one containing ALL of major, minor,
+	// and patch version components. This is enabled by default, but only has any
+	// effect when the ImageSelectionStrategy is SemVer. This should be disabled
+	// cautiously, as it is not uncommon to tag container images with short Git
+	// commit hashes, which have the potential to contain numeric characters only
+	// and could be mistaken for a semver string containing the major version
+	// number only.
+	//
+	// +kubebuilder:default=true
+	StrictSemvers bool `json:"strictSemvers" protobuf:"varint,10,opt,name=strictSemvers"`
 	// SemverConstraint specifies constraints on what new image versions are
 	// permissible. The value in this field only has any effect when the
 	// ImageSelectionStrategy is SemVer or left unspecified (which is implicitly

charts/kargo/resources/crds/kargo.akuity.io_warehouses.yaml

@@ -246,8 +246,20 @@ spec:
                             should be taken with leaving this field unspecified, as it can lead to the
                             unanticipated rollout of breaking changes.
                           type: string
+                        strictSemvers:
+                          default: true
+                          description: |-
+                            StrictSemvers specifies whether only "strict" semver tags should be
+                            considered. A "strict" semver tag is one containing ALL of major, minor,
+                            and patch version components. This is enabled by default, but only has any
+                            effect when the CommitSelectionStrategy is SemVer. This should be disabled
+                            cautiously, as it creates the potential for any tag containing numeric
+                            characters only to be mistaken for a semver string containing the major
+                            version number only.
+                          type: boolean
                       required:
                       - repoURL
+                      - strictSemvers
                       type: object
                     image:
                       description: Image describes a subscription to container image
@@ -337,8 +349,21 @@ spec:
                             changes. Refer to Image Updater documentation for more details.
                             More info: https://github.com/masterminds/semver#checking-version-constraints
                           type: string
+                        strictSemvers:
+                          default: true
+                          description: |-
+                            StrictSemvers specifies whether only "strict" semver tags should be
+                            considered. A "strict" semver tag is one containing ALL of major, minor,
+                            and patch version components. This is enabled by default, but only has any
+                            effect when the ImageSelectionStrategy is SemVer. This should be disabled
+                            cautiously, as it is not uncommon to tag container images with short Git
+                            commit hashes, which have the potential to contain numeric characters only
+                            and could be mistaken for a semver string containing the major version
+                            number only.
+                          type: boolean
                       required:
                       - repoURL
+                      - strictSemvers
                       type: object
                   type: object
                 minItems: 1

internal/controller/semver/parser.go

@@ -0,0 +1,23 @@
+package semver
+
+import (
+	"strings"
+
+	"github.com/Masterminds/semver/v3"
+)
+
+func Parse(s string, strict bool) *semver.Version {
+	// We do the non-strict parsing first, because it will ensure that
+	// sv.Original() returns the original string from which we have not
+	// potentially stripped a leading "v".
+	sv, err := semver.NewVersion(s)
+	if err != nil {
+		return nil // tag wasn't a semantic version
+	}
+	if strict {
+		if _, err = semver.StrictNewVersion(strings.TrimPrefix(s, "v")); err != nil {
+			return nil // tag wasn't a strict semantic version
+		}
+	}
+	return sv
+}

internal/controller/semver/parser_test.go

@@ -0,0 +1,97 @@
+package semver
+
+import (
+	"testing"
+
+	"github.com/Masterminds/semver/v3"
+	"github.com/stretchr/testify/require"
+)
+
+func TestParse(t *testing.T) {
+	testCases := []struct {
+		name       string
+		tag        string
+		strict     bool
+		assertions func(t *testing.T, sv *semver.Version)
+	}{
+		{
+			name:   "invalid with strict parsing",
+			strict: true,
+			tag:    "invalid",
+			assertions: func(t *testing.T, sv *semver.Version) {
+				require.Nil(t, sv)
+			},
+		},
+		{
+			name:   "invalid without strict parsing",
+			strict: false,
+			tag:    "invalid",
+			assertions: func(t *testing.T, sv *semver.Version) {
+				require.Nil(t, sv)
+			},
+		},
+		{
+			name:   "valid, but not strictly so, with strict parsing",
+			strict: true,
+			tag:    "1",
+			assertions: func(t *testing.T, sv *semver.Version) {
+				require.Nil(t, sv)
+			},
+		},
+		{
+			name:   "valid, but not strictly so, without strict parsing",
+			strict: false,
+			tag:    "1",
+			assertions: func(t *testing.T, sv *semver.Version) {
+				require.NotNil(t, sv)
+				require.Equal(t, "1.0.0", sv.String())
+				require.Equal(t, "1", sv.Original())
+			},
+		},
+		{
+			name:   "strictly valid, with strict parsing",
+			strict: true,
+			tag:    "1.0.0",
+			assertions: func(t *testing.T, sv *semver.Version) {
+				require.NotNil(t, sv)
+				require.Equal(t, "1.0.0", sv.String())
+				require.Equal(t, "1.0.0", sv.Original())
+			},
+		},
+		{
+			name:   "strictly valid, without strict parsing",
+			strict: false,
+			tag:    "1.0.0",
+			assertions: func(t *testing.T, sv *semver.Version) {
+				require.NotNil(t, sv)
+				require.Equal(t, "1.0.0", sv.String())
+				require.Equal(t, "1.0.0", sv.Original())
+			},
+		},
+		{
+			name:   "strictly valid, with leading v and strict parsing",
+			strict: true,
+			tag:    "v1.0.0",
+			assertions: func(t *testing.T, sv *semver.Version) {
+				require.NotNil(t, sv)
+				require.Equal(t, "1.0.0", sv.String())
+				require.Equal(t, "v1.0.0", sv.Original())
+			},
+		},
+		{
+			name:   "strictly valid, with leading v, but without strict parsing",
+			strict: false,
+			tag:    "v1.0.0",
+			assertions: func(t *testing.T, sv *semver.Version) {
+				require.NotNil(t, sv)
+				require.Equal(t, "1.0.0", sv.String())
+				require.Equal(t, "v1.0.0", sv.Original())
+			},
+		},
+	}
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			testCase.assertions(t, Parse(testCase.tag, testCase.strict))
+		})
+	}
+}

internal/controller/warehouses/git.go

@@ -14,6 +14,7 @@ import (
 
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
 	"github.com/akuity/kargo/internal/controller/git"
+	libSemver "github.com/akuity/kargo/internal/controller/semver"
 	"github.com/akuity/kargo/internal/credentials"
 	"github.com/akuity/kargo/internal/logging"
 )
@@ -254,7 +255,7 @@ func (r *reconciler) discoverTags(repo git.Repo, sub kargoapi.GitSubscription) (
 
 	switch sub.CommitSelectionStrategy {
 	case kargoapi.CommitSelectionStrategySemVer:
-		if tags, err = selectSemVerTags(tags, sub.SemverConstraint); err != nil {
+		if tags, err = selectSemVerTags(tags, sub.StrictSemvers, sub.SemverConstraint); err != nil {
 			return nil, fmt.Errorf("failed to select semver tags: %w", err)
 		}
 	case kargoapi.CommitSelectionStrategyLexical:
@@ -431,7 +432,7 @@ pathLoop:
 	return false, nil
 }
 
-func selectSemVerTags(tags []git.TagMetadata, constraint string) ([]git.TagMetadata, error) {
+func selectSemVerTags(tags []git.TagMetadata, strict bool, constraint string) ([]git.TagMetadata, error) {
 	var svConstraint *semver.Constraints
 	if constraint != "" {
 		var err error
@@ -447,8 +448,8 @@ func selectSemVerTags(tags []git.TagMetadata, constraint string) ([]git.TagMetad
 
 	var svs []semVerTag
 	for _, meta := range tags {
-		sv, err := semver.NewVersion(meta.Tag)
-		if err != nil {
+		sv := libSemver.Parse(meta.Tag, strict)
+		if sv == nil {
 			continue
 		}
 		if svConstraint == nil || svConstraint.Check(sv) {

internal/controller/warehouses/git_test.go

@@ -784,7 +784,7 @@ func TestSelectSemVerTags(t *testing.T) {
 	}
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			tags, err := selectSemVerTags(testCase.tags, testCase.constraint)
+			tags, err := selectSemVerTags(testCase.tags, false, testCase.constraint)
 			testCase.assertions(t, tags, err)
 		})
 	}

internal/controller/warehouses/images.go

@@ -166,6 +166,7 @@ func imageSelectorForSubscription(
 		sub.RepoURL,
 		image.SelectionStrategy(sub.ImageSelectionStrategy),
 		&image.SelectorOptions{
+			StrictSemvers:         sub.StrictSemvers,
 			Constraint:            sub.SemverConstraint,
 			AllowRegex:            sub.AllowTags,
 			Ignore:                sub.IgnoreTags,

internal/image/digest_selector.go

@@ -14,42 +14,36 @@ import (
 // digestSelector implements the Selector interface for SelectionStrategyDigest.
 type digestSelector struct {
 	repoClient *repositoryClient
-	constraint string
-	platform   *platformConstraint
+	opts       SelectorOptions
 }
 
 // newDigestSelector returns an implementation of the Selector interface for
 // SelectionStrategyDigest.
-func newDigestSelector(
-	repoClient *repositoryClient,
-	constraint string,
-	platform *platformConstraint,
-) (Selector, error) {
-	if constraint == "" {
+func newDigestSelector(repoClient *repositoryClient, opts SelectorOptions) (Selector, error) {
+	if opts.Constraint == "" {
 		return nil, errors.New("digest selection strategy requires a constraint")
 	}
 	return &digestSelector{
 		repoClient: repoClient,
-		constraint: constraint,
-		platform:   platform,
+		opts:       opts,
 	}, nil
 }
 
 // Select implements the Selector interface.
 func (d *digestSelector) Select(ctx context.Context) ([]Image, error) {
-	tag := d.constraint
+	tag := d.opts.Constraint
 	logger := logging.LoggerFromContext(ctx).WithValues(
 		"registry", d.repoClient.registry.name,
 		"image", d.repoClient.repoURL,
 		"selectionStrategy", SelectionStrategyDigest,
 		"tag", tag,
-		"platformConstrained", d.platform != nil,
+		"platformConstrained", d.opts.platform != nil,
 	)
 	logger.Trace("selecting image")
 
 	ctx = logging.ContextWithLogger(ctx, logger)
 
-	image, err := d.repoClient.getImageByTag(ctx, tag, d.platform)
+	image, err := d.repoClient.getImageByTag(ctx, tag, d.opts.platform)
 	if err != nil {
 		var te *transport.Error
 		if errors.As(err, &te) && te.StatusCode == http.StatusNotFound {

internal/image/digest_selector_test.go

@@ -7,15 +7,16 @@ import (
 )
 
 func TestNewDigestSelector(t *testing.T) {
-	const testConstraint = "fake-constraint"
-	testPlatform := &platformConstraint{
-		os:   "linux",
-		arch: "amd64",
+	testOpts := SelectorOptions{
+		Constraint: "fake-constraint",
+		platform: &platformConstraint{
+			os:   "linux",
+			arch: "amd64",
+		},
 	}
-	s, err := newDigestSelector(nil, testConstraint, testPlatform)
+	s, err := newDigestSelector(nil, testOpts)
 	require.NoError(t, err)
 	selector, ok := s.(*digestSelector)
 	require.True(t, ok)
-	require.Equal(t, testConstraint, selector.constraint)
-	require.Equal(t, testPlatform, selector.platform)
+	require.Equal(t, testOpts, selector.opts)
 }