From 97ca665f991c98f5910159474f7755b3e25ef775 Mon Sep 17 00:00:00 2001 From: Kim Christensen Date: Mon, 25 Mar 2024 01:01:00 +0100 Subject: [PATCH 1/3] Lint for parameters being used in actions to which they don't apply Signed-off-by: Kim Christensen --- docs/content/docs/references/linter.md | 28 ++++++ pkg/linter/linter.go | 78 ++++++++++++++++ pkg/linter/linter_test.go | 123 +++++++++++++++++++++++++ pkg/manifest/manifest.go | 34 ++++++- 4 files changed, 261 insertions(+), 2 deletions(-) diff --git a/docs/content/docs/references/linter.md b/docs/content/docs/references/linter.md index 955c7412d..7451d7c85 100644 --- a/docs/content/docs/references/linter.md +++ b/docs/content/docs/references/linter.md @@ -6,6 +6,7 @@ weight: 9 - [exec-100](#exec-100) - [porter-100](#porter-100) +- [porter-101](#porter-101) ## exec-100 @@ -30,3 +31,30 @@ Using a reserved prefix can be problematic as it can overwrite a predefined para To fix the problem indicated by the porter-100 error, you should replace the prefix of any newly defined parameters to not start with "porter". You can find more information about parameters in following URL: https://porter.sh/quickstart/parameters/. + +## porter-101 + +The porter-101 error suggests that an action uses a parameter that is not available to it. + +This is an of a manifest triggering the error (shorten for brevity): + +```yaml +parameters: + - name: uninstallParam + type: string + applyTo: + - uninstall # Notice the parameter only applies to the uninstall action + +install: + - exec: + description: "Install Hello World" + command: ./helpers.sh + arguments: + - install + - "${ bundle.parameters.uninstallParam }" +``` + +To fix the problem indicated by the porter-101 error, you should ensure that all parameters used in the action applies to the actions where +it is referenced. + +You can find more information about applyTo in the following URL: https://porter.sh/docs/bundle/manifest/#parameter-types. diff --git a/pkg/linter/linter.go b/pkg/linter/linter.go index 20c8958b2..612718107 100644 --- a/pkg/linter/linter.go +++ b/pkg/linter/linter.go @@ -11,6 +11,7 @@ import ( "get.porter.sh/porter/pkg/pkgmgmt" "get.porter.sh/porter/pkg/portercontext" "get.porter.sh/porter/pkg/tracing" + "get.porter.sh/porter/pkg/yaml" "github.com/dustin/go-humanize" ) @@ -154,6 +155,11 @@ func New(cxt *portercontext.Context, mixins pkgmgmt.PackageManager) *Linter { } } +type action struct { + name string + steps manifest.Steps +} + func (l *Linter) Lint(ctx context.Context, m *manifest.Manifest) (Results, error) { // Check for reserved porter prefix on parameter names reservedPrefixes := []string{"porter-", "porter_"} @@ -183,7 +189,29 @@ func (l *Linter) Lint(ctx context.Context, m *manifest.Manifest) (Results, error } } } + + // Check if parameters apply to the steps ctx, span := tracing.StartSpan(ctx) + span.Debug("Validating that parameters applies to the actions...") + tmplParams := m.GetTemplatedParameters() + actions := []action{ + {"install", m.Install}, + {"upgrade", m.Upgrade}, + {"uninstall", m.Uninstall}, + } + for actionName, steps := range m.CustomActions { + actions = append(actions, action{actionName, steps}) + } + for _, action := range actions { + res, err := validateParamsAppliesToAction(m, action.steps, tmplParams, action.name) + if err != nil { + return nil, span.Error(fmt.Errorf("error validating action: %s", action.name)) + } + results = append(results, res...) + } + span.EndSpan() + + ctx, span = tracing.StartSpan(ctx) defer span.EndSpan() span.Debug("Running linters for each mixin used in the manifest...") @@ -213,3 +241,53 @@ func (l *Linter) Lint(ctx context.Context, m *manifest.Manifest) (Results, error return results, nil } + +func validateParamsAppliesToAction(m *manifest.Manifest, steps manifest.Steps, tmplParams manifest.ParameterDefinitions, actionName string) (Results, error) { + var results Results + for stepNumber, step := range steps { + data, err := yaml.Marshal(step.Data) + if err != nil { + return nil, fmt.Errorf("error during marshalling: %w", err) + } + + tmplResult, err := m.ScanManifestTemplating(data) + if err != nil { + return nil, fmt.Errorf("error parsing templating: %w", err) + } + + for _, variable := range tmplResult.Variables { + paramName, ok := m.GetTemplateParameterName(variable) + if !ok { + continue + } + + for _, tmplParam := range tmplParams { + if tmplParam.Name != paramName { + continue + } + if !tmplParam.AppliesTo(actionName) { + description, err := step.GetDescription() + if err != nil { + return nil, fmt.Errorf("error getting step description: %w", err) + } + res := Result{ + Level: LevelError, + Location: Location{ + Action: actionName, + Mixin: step.GetMixinName(), + StepNumber: stepNumber + 1, + StepDescription: description, + }, + Code: "porter-101", + Title: "Parameter does not apply to action", + Message: fmt.Sprintf("Parameter %s does not apply to %s action", paramName, actionName), + URL: "https://porter.sh/docs/references/linter/#porter-101", + } + results = append(results, res) + } + } + } + } + + return results, nil +} diff --git a/pkg/linter/linter_test.go b/pkg/linter/linter_test.go index d29f55013..5bea86be8 100644 --- a/pkg/linter/linter_test.go +++ b/pkg/linter/linter_test.go @@ -2,6 +2,7 @@ package linter import ( "context" + "fmt" "testing" "get.porter.sh/porter/pkg/manifest" @@ -173,3 +174,125 @@ func TestLinter_Lint(t *testing.T) { require.NotContains(t, results[0].String(), ": 0th step in the mixin ()") }) } + +func TestLinter_Lint_ParameterDeosNotApplyTo(t *testing.T) { + ctx := context.Background() + testCases := []struct { + action string + setSteps func(*manifest.Manifest, manifest.Steps) + }{ + {"install", func(m *manifest.Manifest, steps manifest.Steps) { m.Install = steps }}, + {"upgrade", func(m *manifest.Manifest, steps manifest.Steps) { m.Upgrade = steps }}, + {"uninstall", func(m *manifest.Manifest, steps manifest.Steps) { m.Uninstall = steps }}, + {"customAction", func(m *manifest.Manifest, steps manifest.Steps) { + m.CustomActions = make(map[string]manifest.Steps) + m.CustomActions["customAction"] = steps + }}, + } + + for _, tc := range testCases { + t.Run(tc.action, func(t *testing.T) { + cxt := portercontext.NewTestContext(t) + mixins := mixin.NewTestMixinProvider() + l := New(cxt.Context, mixins) + + param := map[string]manifest.ParameterDefinition{ + "doesNotApply": { + Name: "doesNotApply", + ApplyTo: []string{"dummy"}, + }, + } + steps := manifest.Steps{ + &manifest.Step{ + Data: map[string]interface{}{ + "exec": map[string]interface{}{ + "description": "exec step", + "parameters": []string{ + "\"${ bundle.parameters.doesNotApply }\"", + }, + }, + }, + }, + } + m := &manifest.Manifest{ + SchemaVersion: "1.0.1", + TemplateVariables: []string{"bundle.parameters.doesNotApply"}, + Parameters: param, + } + tc.setSteps(m, steps) + + lintResults := Results{ + { + Level: LevelError, + Location: Location{ + Action: tc.action, + Mixin: "exec", + StepNumber: 1, + StepDescription: "exec step", + }, + Code: "porter-101", + Title: "Parameter does not apply to action", + Message: fmt.Sprintf("Parameter doesNotApply does not apply to %s action", tc.action), + URL: "https://porter.sh/docs/references/linter/#porter-101", + }, + } + results, err := l.Lint(ctx, m) + require.NoError(t, err, "Lint failed") + require.Len(t, results, 1, "linter should have returned 1 result") + require.Equal(t, lintResults, results, "unexpected lint results") + }) + } +} + +func TestLinter_Lint_ParameterAppliesTo(t *testing.T) { + ctx := context.Background() + testCases := []struct { + action string + setSteps func(*manifest.Manifest, manifest.Steps) + }{ + {"install", func(m *manifest.Manifest, steps manifest.Steps) { m.Install = steps }}, + {"upgrade", func(m *manifest.Manifest, steps manifest.Steps) { m.Upgrade = steps }}, + {"uninstall", func(m *manifest.Manifest, steps manifest.Steps) { m.Uninstall = steps }}, + {"customAction", func(m *manifest.Manifest, steps manifest.Steps) { + m.CustomActions = make(map[string]manifest.Steps) + m.CustomActions["customAction"] = steps + }}, + } + + for _, tc := range testCases { + t.Run(tc.action, func(t *testing.T) { + cxt := portercontext.NewTestContext(t) + mixins := mixin.NewTestMixinProvider() + l := New(cxt.Context, mixins) + + param := map[string]manifest.ParameterDefinition{ + "appliesTo": { + Name: "appliesTo", + ApplyTo: []string{tc.action}, + }, + } + steps := manifest.Steps{ + &manifest.Step{ + Data: map[string]interface{}{ + "exec": map[string]interface{}{ + "description": "exec step", + "parameters": []string{ + "\"${ bundle.parameters.appliesTo }\"", + }, + }, + }, + }, + } + m := &manifest.Manifest{ + SchemaVersion: "1.0.1", + TemplateVariables: []string{"bundle.parameters.appliesTo"}, + Parameters: param, + } + tc.setSteps(m, steps) + + results, err := l.Lint(ctx, m) + require.NoError(t, err, "Lint failed") + require.Len(t, results, 0, "linter should have returned 1 result") + }) + } +} diff --git a/pkg/manifest/manifest.go b/pkg/manifest/manifest.go index cdb487c09..9a43de2e4 100644 --- a/pkg/manifest/manifest.go +++ b/pkg/manifest/manifest.go @@ -281,6 +281,19 @@ func (m *Manifest) getTemplateDependencyOutputName(value string) (string, string return dependencyName, outputName, true } +var templatedParameterRegex = regexp.MustCompile(`^bundle\.parameters\.(.+)$`) + +// GetTemplateParameterName returns the parameter name from the template variable. +func (m *Manifest) GetTemplateParameterName(value string) (string, bool) { + matches := templatedParameterRegex.FindStringSubmatch(value) + if len(matches) < 2 { + return "", false + } + + parameterName := matches[1] + return parameterName, true +} + // GetTemplatedOutputs returns the output definitions for any bundle level outputs // that have been templated, keyed by the output name. func (m *Manifest) GetTemplatedOutputs() OutputDefinitions { @@ -314,6 +327,23 @@ func (m *Manifest) GetTemplatedDependencyOutputs() DependencyOutputReferences { return outputs } +// GetTemplatedParameters returns the output definitions for any bundle level outputs +// that have been templated, keyed by the output name. +func (m *Manifest) GetTemplatedParameters() ParameterDefinitions { + parameters := make(ParameterDefinitions, len(m.TemplateVariables)) + for _, tmplVar := range m.TemplateVariables { + if name, ok := m.GetTemplateParameterName(tmplVar); ok { + parameterDef, ok := m.Parameters[name] + if !ok { + // Only return bundle level definitions + continue + } + parameters[name] = parameterDef + } + } + return parameters +} + // DetermineDependenciesExtensionUsed looks for how dependencies are used // by the bundle and which version of the dependency extension can be used. func (m *Manifest) DetermineDependenciesExtensionUsed() string { @@ -1257,7 +1287,7 @@ func ReadManifest(cxt *portercontext.Context, path string) (*Manifest, error) { return nil, fmt.Errorf("unsupported property set or a custom action is defined incorrectly: %w", err) } - tmplResult, err := m.scanManifestTemplating(data) + tmplResult, err := m.ScanManifestTemplating(data) if err != nil { return nil, err } @@ -1293,7 +1323,7 @@ func (m *Manifest) GetTemplatePrefix() string { return "" } -func (m *Manifest) scanManifestTemplating(data []byte) (templateScanResult, error) { +func (m *Manifest) ScanManifestTemplating(data []byte) (templateScanResult, error) { const disableHtmlEscaping = true templateSrc := m.GetTemplatePrefix() + string(data) tmpl, err := mustache.ParseStringRaw(templateSrc, disableHtmlEscaping) From a37d7c4f00bbaf9543855c2995364111c2557273 Mon Sep 17 00:00:00 2001 From: Kim Christensen Date: Wed, 27 Mar 2024 23:56:47 +0100 Subject: [PATCH 2/3] Revert back to a single span Signed-off-by: Kim Christensen --- pkg/linter/linter.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/linter/linter.go b/pkg/linter/linter.go index 612718107..b3008b4e0 100644 --- a/pkg/linter/linter.go +++ b/pkg/linter/linter.go @@ -192,6 +192,8 @@ func (l *Linter) Lint(ctx context.Context, m *manifest.Manifest) (Results, error // Check if parameters apply to the steps ctx, span := tracing.StartSpan(ctx) + defer span.EndSpan() + span.Debug("Validating that parameters applies to the actions...") tmplParams := m.GetTemplatedParameters() actions := []action{ @@ -209,10 +211,6 @@ func (l *Linter) Lint(ctx context.Context, m *manifest.Manifest) (Results, error } results = append(results, res...) } - span.EndSpan() - - ctx, span = tracing.StartSpan(ctx) - defer span.EndSpan() span.Debug("Running linters for each mixin used in the manifest...") q := query.New(l.Context, l.Mixins) From 3cafb67b6a09868a493279934dd43f51a135bf06 Mon Sep 17 00:00:00 2001 From: Kim Christensen Date: Thu, 28 Mar 2024 00:19:01 +0100 Subject: [PATCH 3/3] Add integration test Signed-off-by: Kim Christensen --- tests/integration/lint_test.go | 11 ++++++ .../porter.yaml | 39 +++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 tests/integration/testdata/bundles/bundle-with-param-apply-lint-error/porter.yaml diff --git a/tests/integration/lint_test.go b/tests/integration/lint_test.go index 2f9cd2920..78e17bf6f 100644 --- a/tests/integration/lint_test.go +++ b/tests/integration/lint_test.go @@ -34,5 +34,16 @@ func TestLint(t *testing.T) { cmd.Env("PORTER_VERBOSITY=info") }) require.NotContains(t, output, "unknown command", "an unsupported mixin command should not be printed to the console in info") +} + +func TestLint_ApplyToParam(t *testing.T) { + test, err := tester.NewTest(t) + defer test.Close() + require.NoError(t, err, "test setup failed") + _, output, _ := test.RunPorterWith(func(cmd *shx.PreparedCommand) { + cmd.Args("lint") + cmd.In(filepath.Join(test.RepoRoot, "tests/integration/testdata/bundles/bundle-with-param-apply-lint-error")) + }) + require.Contains(t, output, "error(porter-101) - Parameter does not apply to action", "parameters being used in actions to which they don't apply should be an error") } diff --git a/tests/integration/testdata/bundles/bundle-with-param-apply-lint-error/porter.yaml b/tests/integration/testdata/bundles/bundle-with-param-apply-lint-error/porter.yaml new file mode 100644 index 000000000..b8ed6a64f --- /dev/null +++ b/tests/integration/testdata/bundles/bundle-with-param-apply-lint-error/porter.yaml @@ -0,0 +1,39 @@ +# This bundle is designed to cause the porter lint/build commands to fail +schemaType: Bundle +schemaVersion: 1.0.1 +name: exec-mixin-lint-error +version: 0.1.0 +description: "This bundle is designed to cause the porter lint/build commands to fail, use --no-lint to use it anyway" +registry: "localhost:5000" + +mixins: + - exec + +parameters: + - name: onlyApplyToUninstall + description: Only applies to uninstall + type: string + default: appliesToUninstall + applyTo: + - uninstall + +install: + - exec: + description: trigger a lint error + command: echo + arguments: + - ${ bundle.parameters.onlyApplyToUninstall } + +upgrade: + - exec: + description: "World 2.0" + command: echo + arguments: + - upgrade + +uninstall: + - exec: + description: "Uninstall Hello World" + command: echo + arguments: + - uninstall