Support for version number validation against mixins (#3215)

* Added parsing step and tests Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com> * Added linting step and tests Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com> * Added a test with no version number Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com> --------- Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com> Co-authored-by: Kim Christensen <2461567+kichristensen@users.noreply.github.com>
getporter · Sep 23, 2024 · 7538008 · 7538008
1 parent f61648f
commit 7538008
Show file tree

Hide file tree

Showing 8 changed files with 251 additions and 7 deletions.
diff --git a/pkg/linter/linter.go b/pkg/linter/linter.go
@@ -13,6 +13,7 @@ import (
 	"get.porter.sh/porter/pkg/portercontext"
 	"get.porter.sh/porter/pkg/tracing"
 	"get.porter.sh/porter/pkg/yaml"
+	"github.com/Masterminds/semver/v3"
 	"github.com/dustin/go-humanize"
 )
 
@@ -264,9 +265,46 @@ func (l *Linter) Lint(ctx context.Context, m *manifest.Manifest, config *config.
 		results = append(results, r...)
 	}
 
+	span.Debug("Getting versions for each mixin used in the manifest...")
+	err = l.validateVersionNumberConstraints(ctx, m)
+	if err != nil {
+		return nil, span.Error(err)
+	}
+
 	return results, nil
 }
 
+func (l *Linter) validateVersionNumberConstraints(ctx context.Context, m *manifest.Manifest) error {
+	for _, mixin := range m.Mixins {
+		if mixin.Version != nil {
+			installedMeta, err := l.Mixins.GetMetadata(ctx, mixin.Name)
+			if err != nil {
+				return fmt.Errorf("unable to get metadata from mixin %s: %w", mixin.Name, err)
+			}
+			installedVersion := installedMeta.GetVersionInfo().Version
+
+			err = validateSemverConstraint(mixin.Name, installedVersion, mixin.Version)
+			if err != nil {
+				return err
+			}
+		}
+	}
+
+	return nil
+}
+
+func validateSemverConstraint(name string, installedVersion string, versionConstraint *semver.Constraints) error {
+	v, err := semver.NewVersion(installedVersion)
+	if err != nil {
+		return fmt.Errorf("invalid version number from mixin %s: %s. %w", name, installedVersion, err)
+	}
+
+	if !versionConstraint.Check(v) {
+		return fmt.Errorf("mixin %s is installed at version %s but your bundle requires version %s", name, installedVersion, versionConstraint)
+	}
+	return nil
+}
+
 func validateParamsAppliesToAction(m *manifest.Manifest, steps manifest.Steps, tmplParams manifest.ParameterDefinitions, actionName string, config *config.Config) (Results, error) {
 	var results Results
 	for stepNumber, step := range steps {

diff --git a/pkg/linter/linter_test.go b/pkg/linter/linter_test.go
@@ -8,7 +8,10 @@ import (
 	"get.porter.sh/porter/pkg/config"
 	"get.porter.sh/porter/pkg/manifest"
 	"get.porter.sh/porter/pkg/mixin"
+	"get.porter.sh/porter/pkg/pkgmgmt"
 	"get.porter.sh/porter/pkg/portercontext"
+	"get.porter.sh/porter/tests"
+	"github.com/Masterminds/semver/v3"
 	"github.com/stretchr/testify/require"
 )
 
@@ -363,3 +366,105 @@ func TestLinter_DependencyMultipleTimes(t *testing.T) {
 		require.Len(t, results, 0, "linter should have returned 0 result")
 	})
 }
+
+func TestLinter_Lint_MissingMixin(t *testing.T) {
+	cxt := portercontext.NewTestContext(t)
+	mixins := mixin.NewTestMixinProvider()
+	l := New(cxt.Context, mixins)
+	testConfig := config.NewTestConfig(t).Config
+
+	mixinName := "made-up-mixin-that-is-not-installed"
+
+	m := &manifest.Manifest{
+		Mixins: []manifest.MixinDeclaration{
+			{
+				Name: mixinName,
+			},
+		},
+	}
+
+	mixins.RunAssertions = append(mixins.RunAssertions, func(mixinCxt *portercontext.Context, mixinName string, commandOpts pkgmgmt.CommandOptions) error {
+		return fmt.Errorf("%s not installed", mixinName)
+	})
+
+	_, err := l.Lint(context.Background(), m, testConfig)
+	require.Error(t, err, "Linting should return an error")
+	tests.RequireOutputContains(t, err.Error(), fmt.Sprintf("%s is not currently installed", mixinName))
+}
+
+func TestLinter_Lint_MixinVersions(t *testing.T) {
+	cxt := portercontext.NewTestContext(t)
+	mixinProvider := mixin.NewTestMixinProvider()
+	l := New(cxt.Context, mixinProvider)
+	testConfig := config.NewTestConfig(t).Config
+
+	exampleMixinVersion := mixin.ExampleMixinSemver.String()
+
+	// build up some test semvers
+	patchDifferenceSemver := fmt.Sprintf("%d.%d.%d", mixin.ExampleMixinSemver.Major(), mixin.ExampleMixinSemver.Minor(), mixin.ExampleMixinSemver.Patch()+1)
+	anyPatchAccepted := fmt.Sprintf("%d.%d.x", mixin.ExampleMixinSemver.Major(), mixin.ExampleMixinSemver.Minor())
+	lessThanNextMajor := fmt.Sprintf("<%d.%d", mixin.ExampleMixinSemver.Major()+1, mixin.ExampleMixinSemver.Minor())
+
+	exampleMixinVersionConstraint, _ := semver.NewConstraint(exampleMixinVersion)
+	patchDifferenceSemverConstraint, _ := semver.NewConstraint(patchDifferenceSemver)
+	anyPatchAcceptedConstraint, _ := semver.NewConstraint(anyPatchAccepted)
+	lessThanNextMajorConstraint, _ := semver.NewConstraint(lessThanNextMajor)
+
+	testCases := []struct {
+		name        string
+		errExpected bool
+		mixins      []manifest.MixinDeclaration
+	}{
+		{"exact-semver", false, []manifest.MixinDeclaration{
+			{
+				Name:    mixin.ExampleMixinName,
+				Version: exampleMixinVersionConstraint,
+			},
+		}},
+		{"different-patch", true, []manifest.MixinDeclaration{
+			{
+				Name:    mixin.ExampleMixinName,
+				Version: patchDifferenceSemverConstraint,
+			},
+		}},
+		{"accept-different-patch", false, []manifest.MixinDeclaration{
+			{
+				Name:    mixin.ExampleMixinName,
+				Version: anyPatchAcceptedConstraint,
+			},
+		}},
+		{"accept-less-than-versions", false, []manifest.MixinDeclaration{
+			{
+				Name:    mixin.ExampleMixinName,
+				Version: lessThanNextMajorConstraint,
+			},
+		}},
+		{"no-version-provided", false, []manifest.MixinDeclaration{
+			{
+				Name: mixin.ExampleMixinName,
+			},
+		}},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			m := &manifest.Manifest{
+				Mixins: testCase.mixins,
+			}
+			results, err := l.Lint(context.Background(), m, testConfig)
+			if testCase.errExpected {
+				require.Error(t, err, "Linting should return an error")
+				tests.RequireOutputContains(t, err.Error(), fmt.Sprintf(
+					"mixin %s is installed at version v%s but your bundle requires version %s",
+					mixin.ExampleMixinName,
+					exampleMixinVersion,
+					testCase.mixins[0].Version.String(),
+				))
+			} else {
+				require.NoError(t, err, "Linting should not return an error")
+			}
+			require.Len(t, results, 0, "linter should have returned 0 result")
+		})
+	}
+
+}
diff --git a/pkg/manifest/manifest.go b/pkg/manifest/manifest.go
@@ -640,8 +640,30 @@ func (l Location) IsEmpty() bool {
 }
 
 type MixinDeclaration struct {
-	Name   string
-	Config interface{}
+	Name    string
+	Version *semver.Constraints
+	Config  interface{}
+}
+
+func extractVersionFromName(name string) (string, *semver.Constraints, error) {
+	parts := strings.Split(name, "@")
+
+	// if there isn't a version in the name, just stop!
+	if len(parts) == 1 {
+		return name, nil, nil
+	}
+
+	// if we somehow got more parts than expected!
+	if len(parts) != 2 {
+		return "", nil, fmt.Errorf("expected name@version, got: %s", name)
+	}
+
+	version, err := semver.NewConstraint(parts[1])
+	if err != nil {
+		return "", nil, err
+	}
+
+	return parts[0], version, nil
 }
 
 // UnmarshalYAML allows mixin declarations to either be a normal list of strings
@@ -652,12 +674,25 @@ type MixinDeclaration struct {
 //   - az:
 //     extensions:
 //   - iot
+//
+// for each type, we can optionally support a version number in the name field
+// mixins:
+// - exec@2.1.1
+// or
+//   - az@2.1.1
+//     extensions:
+//   - iot
 func (m *MixinDeclaration) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	// First try to just read the mixin name
 	var mixinNameOnly string
 	err := unmarshal(&mixinNameOnly)
 	if err == nil {
-		m.Name = mixinNameOnly
+		name, version, err := extractVersionFromName(mixinNameOnly)
+		if err != nil {
+			return fmt.Errorf("invalid mixin name/version: %w", err)
+		}
+		m.Name = name
+		m.Version = version
 		m.Config = nil
 		return nil
 	}
@@ -676,7 +711,12 @@ func (m *MixinDeclaration) UnmarshalYAML(unmarshal func(interface{}) error) erro
 	}
 
 	for mixinName, config := range mixinWithConfig {
-		m.Name = mixinName
+		name, version, err := extractVersionFromName(mixinName)
+		if err != nil {
+			return fmt.Errorf("invalid mixin name/version: %w", err)
+		}
+		m.Name = name
+		m.Version = version
 		m.Config = config
 		break // There is only one mixin anyway but break for clarity
 	}

diff --git a/pkg/manifest/manifest_test.go b/pkg/manifest/manifest_test.go
@@ -12,6 +12,7 @@ import (
 	"get.porter.sh/porter/pkg/portercontext"
 	"get.porter.sh/porter/pkg/schema"
 	"get.porter.sh/porter/pkg/yaml"
+	"github.com/Masterminds/semver/v3"
 	"github.com/cnabio/cnab-go/bundle/definition"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -589,6 +590,43 @@ func TestMixinDeclaration_UnmarshalYAML_Invalid(t *testing.T) {
 	assert.Contains(t, err.Error(), "mixin declaration contained more than one mixin")
 }
 
+func TestMixinDeclaration_UnmarshalYAML_Versions(t *testing.T) {
+	cxt := portercontext.NewTestContext(t)
+	cxt.AddTestFile("testdata/mixin-with-versions.yaml", config.Name)
+	m, err := ReadManifest(cxt.Context, config.Name, config.NewTestConfig(t).Config)
+
+	execVersion, _ := semver.NewConstraint("1")
+	axVersion, _ := semver.NewConstraint("1.1.X")
+	terraformVersoin, _ := semver.NewConstraint(">=2")
+
+	require.NoError(t, err)
+	assert.Len(t, m.Mixins, 3, "expected 3 mixins")
+	assert.Equal(t, "exec", m.Mixins[0].Name)
+	assert.Equal(t, "az", m.Mixins[1].Name)
+	assert.Equal(t, "terraform", m.Mixins[2].Name)
+	assert.Equal(t, execVersion, m.Mixins[0].Version)
+	assert.Equal(t, axVersion, m.Mixins[1].Version)
+	assert.Equal(t, terraformVersoin, m.Mixins[2].Version)
+}
+
+func TestMixinDeclaration_UnmarshalYAML_Versions_Empty(t *testing.T) {
+	cxt := portercontext.NewTestContext(t)
+	cxt.AddTestFile("testdata/mixin-with-empty-version.yaml", config.Name)
+	_, err := ReadManifest(cxt.Context, config.Name, config.NewTestConfig(t).Config)
+
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "invalid mixin name/version: improper constraint:")
+}
+
+func TestMixinDeclaration_UnmarshalYAML_Versions_Invalid(t *testing.T) {
+	cxt := portercontext.NewTestContext(t)
+	cxt.AddTestFile("testdata/mixin-with-invalid-version.yaml", config.Name)
+	_, err := ReadManifest(cxt.Context, config.Name, config.NewTestConfig(t).Config)
+
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "invalid mixin name/version: expected name@version, got: az@this@that")
+}
+
 func TestCredentialsDefinition_UnmarshalYAML(t *testing.T) {
 	assertAllCredentialsRequired := func(t *testing.T, creds CredentialDefinitions) {
 		for _, cred := range creds {

diff --git a/pkg/manifest/testdata/mixin-with-empty-version.yaml b/pkg/manifest/testdata/mixin-with-empty-version.yaml
@@ -0,0 +1,6 @@
+mixins:
+  - exec@
+  - az:
+      extensions:
+        - iot
+  - terraform
diff --git a/pkg/manifest/testdata/mixin-with-invalid-version.yaml b/pkg/manifest/testdata/mixin-with-invalid-version.yaml
@@ -0,0 +1,6 @@
+mixins:
+  - exec
+  - az@this@that:
+      extensions:
+        - iot
+  - terraform
diff --git a/pkg/manifest/testdata/mixin-with-versions.yaml b/pkg/manifest/testdata/mixin-with-versions.yaml
@@ -0,0 +1,6 @@
+mixins:
+  - exec@1
+  - az@1.1.X:
+      extensions:
+        - iot
+  - terraform@>=2
diff --git a/pkg/mixin/helpers.go b/pkg/mixin/helpers.go
@@ -10,6 +10,7 @@ import (
 	"get.porter.sh/porter/pkg/pkgmgmt"
 	"get.porter.sh/porter/pkg/pkgmgmt/client"
 	"get.porter.sh/porter/pkg/portercontext"
+	"github.com/Masterminds/semver/v3"
 )
 
 type TestMixinProvider struct {
@@ -24,6 +25,10 @@ type TestMixinProvider struct {
 	ReturnBuildError bool
 }
 
+// hoist these into variables so tests can reference them safely
+var ExampleMixinName = "testmixin"
+var ExampleMixinSemver = semver.New(0, 1, 0, "", "")
+
 // NewTestMixinProvider helps us test Porter.Mixins in our unit tests without actually hitting any real plugins on the file system.
 func NewTestMixinProvider() *TestMixinProvider {
 	packages := []pkgmgmt.PackageMetadata{
@@ -36,9 +41,9 @@ func NewTestMixinProvider() *TestMixinProvider {
 			},
 		},
 		&Metadata{
-			Name: "testmixin",
+			Name: ExampleMixinName,
 			VersionInfo: pkgmgmt.VersionInfo{
-				Version: "v0.1.0",
+				Version: fmt.Sprintf("v%s", ExampleMixinSemver.String()),
 				Commit:  "abc123",
 				Author:  "Porter Authors",
 			},
@@ -78,7 +83,7 @@ func (p *TestMixinProvider) GetSchema(ctx context.Context, name string) (string,
 	switch name {
 	case "exec":
 		schemaFile = "../exec/schema/exec.json"
-	case "testmixin":
+	case ExampleMixinName:
 		schemaFile = "../../cmd/testmixin/schema.json"
 	default:
 		return "", nil