Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added validation if someone specifies a version on a bundle #3148

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions pkg/linter/linter.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,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"
)

Expand Down Expand Up @@ -263,9 +264,55 @@ func (l *Linter) Lint(ctx context.Context, m *manifest.Manifest) (Results, error
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 mapConfig, ok := mixin.Config.(map[string]interface{}); ok {
if v, exists := mapConfig["version"]; exists {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there currently anything ensuring that mixins cannot use a config field called version?
I'm thinking of the situation where mixins already provide a field called version, which might result in unintended behaviour

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it was simply the suggested field name on the issue. We could always use mixinVersion or something more verbose to avoid clashing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally like version the best, but it could break some existing plugins.
@schristoff do you know of any way we can easily handle a "reservation" of a mixin configuration property?

Copy link

@KurtSchenk KurtSchenk Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kichristensen, @schristoff could a similar approach such as porter-state, porter-debug be taken?
image


if versionConstraint, ok := v.(string); ok {
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, versionConstraint)
if err != nil {
return err
}
}
}
}
}
return nil
}

func validateSemverConstraint(name string, installedVersion string, versionConstraint string) error {
c, err := semver.NewConstraint(versionConstraint)
if err != nil {
return fmt.Errorf("invalid constraint for mixin%s: %s. %w", name, versionConstraint, err)
}

v, err := semver.NewVersion(installedVersion)
if err != nil {
return fmt.Errorf("invalid version number from mixin %s: %s. %w", name, installedVersion, err)
}

if !c.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) (Results, error) {
var results Results
for stepNumber, step := range steps {
Expand Down
112 changes: 112 additions & 0 deletions pkg/linter/linter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ import (

"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/stretchr/testify/require"
)

Expand Down Expand Up @@ -356,3 +358,113 @@ 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)

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)
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)

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())

testCases := []struct {
name string
errExpected bool
mixins []manifest.MixinDeclaration
}{
{"exact-semver", false, []manifest.MixinDeclaration{
{
Name: mixin.ExampleMixinName,
Config: map[string]interface{}{
"version": exampleMixinVersion,
},
},
}},
{"different-patch", true, []manifest.MixinDeclaration{
{
Name: mixin.ExampleMixinName,
Config: map[string]interface{}{
"version": patchDifferenceSemver,
},
},
}},
{"accept-different-patch", false, []manifest.MixinDeclaration{
{
Name: mixin.ExampleMixinName,
Config: map[string]interface{}{
"version": anyPatchAccepted,
},
},
}},
{"accept-less-than-versions", false, []manifest.MixinDeclaration{
{
Name: mixin.ExampleMixinName,
Config: map[string]interface{}{
"version": lessThanNextMajor,
},
},
}},
}

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)
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,
getVersionFromMixinConfig(testCase.mixins[0].Config),
))
} else {
require.NoError(t, err, "Linting should not return an error")
}
require.Len(t, results, 0, "linter should have returned 0 result")
})
}

}

func getVersionFromMixinConfig(config interface{}) string {
if mapConfig, ok := config.(map[string]interface{}); ok {
if v, exists := mapConfig["version"]; exists {

if versionConstraint, ok := v.(string); ok {
return versionConstraint
}
}
}
return ""
}
11 changes: 8 additions & 3 deletions pkg/mixin/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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{
Expand All @@ -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",
},
Expand Down Expand Up @@ -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
Expand Down
Loading