diff --git a/pkg/cnab/provider/action.go b/pkg/cnab/provider/action.go index 80ceca006..24501e2a5 100644 --- a/pkg/cnab/provider/action.go +++ b/pkg/cnab/provider/action.go @@ -270,9 +270,22 @@ func (r *Runtime) SaveOperationResult(ctx context.Context, opResult driver.Opera bigerr = multierror.Append(bigerr, fmt.Errorf("error updating installation record for %s\n%#v: %w", installation, installation, err)) } + extBun := cnab.ExtendedBundle{Bundle: run.Bundle} + actionModifies := true + if action, err := run.Bundle.GetAction(run.Action); err == nil { + actionModifies = action.Modifies + } + for outputName, outputValue := range opResult.Outputs { + // porter-state tracks bundle-managed resource state. Skip persisting it + // for modifies:false actions so that a read-only invoke cannot overwrite + // the installation's state record. + if !actionModifies && extBun.IsInternalOutput(outputName) { + continue + } + output := result.NewOutput(outputName, []byte(outputValue)) - output, err = r.sanitizer.CleanOutput(ctx, output, cnab.ExtendedBundle{Bundle: run.Bundle}) + output, err = r.sanitizer.CleanOutput(ctx, output, extBun) if err != nil { bigerr = multierror.Append(bigerr, fmt.Errorf("error sanitizing sensitive %s output for %s run of installation %s\n%#v: %w", output.Name, run.Action, installation, output, err)) } diff --git a/pkg/cnab/provider/action_test.go b/pkg/cnab/provider/action_test.go index 22c17ef06..fffaec2c7 100644 --- a/pkg/cnab/provider/action_test.go +++ b/pkg/cnab/provider/action_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/cnabio/cnab-go/bundle" + "github.com/cnabio/cnab-go/bundle/definition" "github.com/cnabio/cnab-go/driver" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -73,3 +74,93 @@ func TestAddFiles(t *testing.T) { require.Contains(t, op.Files, config.ClaimFilepath, "The claim should have been injected into the bundle") test.CompareGoldenFile(t, "testdata/want-claim.json", op.Files[config.ClaimFilepath]) } + +func TestSaveOperationResult_ModifiesFalse_SkipsPorterState(t *testing.T) { + t.Parallel() + + ctx := context.Background() + d := NewTestRuntime(t) + defer d.Close() + + instName := "mybuns" + bun := bundle.Bundle{ + Actions: map[string]bundle.Action{ + "dry-run": {Modifies: false}, + }, + Outputs: map[string]bundle.Output{ + "porter-state": {Definition: "porter-state", Path: "/cnab/app/outputs/porter-state.tgz"}, + "user-output": {Definition: "user-output", Path: "/cnab/app/outputs/user-output"}, + }, + Definitions: map[string]*definition.Schema{ + "porter-state": {Comment: cnab.PorterInternal}, + "user-output": {Type: "string"}, + }, + } + i := d.TestInstallations.CreateInstallation(storage.NewInstallation("", instName), d.TestInstallations.SetMutableInstallationValues) + run := storage.NewRun("", instName) + run.Bundle = bun + run.Action = "dry-run" + run = d.TestInstallations.CreateRun(run, d.TestInstallations.SetMutableRunValues) + result := run.NewResult(cnab.StatusSucceeded) + + opResult := driver.OperationResult{ + Outputs: map[string]string{ + "porter-state": "state-data", + "user-output": "hello", + }, + } + + err := d.SaveOperationResult(ctx, opResult, i, run, result) + require.NoError(t, err) + + outputs, err := d.TestInstallations.GetOutputs(ctx, run.ID) + require.NoError(t, err) + + _, hasPorterState := outputs.GetByName("porter-state") + assert.False(t, hasPorterState, "porter-state should not be saved for modifies:false actions") + + _, hasUserOutput := outputs.GetByName("user-output") + assert.True(t, hasUserOutput, "user-defined outputs should still be saved") +} + +func TestSaveOperationResult_ModifiesTrue_SavesPorterState(t *testing.T) { + t.Parallel() + + ctx := context.Background() + d := NewTestRuntime(t) + defer d.Close() + + instName := "mybuns" + bun := bundle.Bundle{ + Actions: map[string]bundle.Action{ + "rotate-creds": {Modifies: true}, + }, + Outputs: map[string]bundle.Output{ + "porter-state": {Definition: "porter-state", Path: "/cnab/app/outputs/porter-state.tgz"}, + }, + Definitions: map[string]*definition.Schema{ + "porter-state": {Comment: cnab.PorterInternal}, + }, + } + i := d.TestInstallations.CreateInstallation(storage.NewInstallation("", instName), d.TestInstallations.SetMutableInstallationValues) + run := storage.NewRun("", instName) + run.Bundle = bun + run.Action = "rotate-creds" + run = d.TestInstallations.CreateRun(run, d.TestInstallations.SetMutableRunValues) + result := run.NewResult(cnab.StatusSucceeded) + + opResult := driver.OperationResult{ + Outputs: map[string]string{ + "porter-state": "state-data", + }, + } + + err := d.SaveOperationResult(ctx, opResult, i, run, result) + require.NoError(t, err) + + outputs, err := d.TestInstallations.GetOutputs(ctx, run.ID) + require.NoError(t, err) + + _, hasPorterState := outputs.GetByName("porter-state") + assert.True(t, hasPorterState, "porter-state should be saved for modifies:true actions") +} diff --git a/pkg/runtime/runtime_manifest_test.go b/pkg/runtime/runtime_manifest_test.go index c556e6ed5..2c566dc8d 100644 --- a/pkg/runtime/runtime_manifest_test.go +++ b/pkg/runtime/runtime_manifest_test.go @@ -1404,6 +1404,60 @@ func createTestStateArchive(t *testing.T, stateName, content string) []byte { return buf.Bytes() } +func TestFinalize_PackStateBag(t *testing.T) { + mContent := `schemaVersion: 1.0.0-alpha.2 +state: +- name: mystate + path: /path/to/mystate + +install: +- mymixin: + Parameters: + Thing: test +` + setup := func(t *testing.T, action string, modifies bool, isBuiltin bool) (*RuntimeManifest, *config.TestConfig) { + testConfig := config.NewTestConfig(t) + rm := runtimeManifestFromStepYaml(t, testConfig, mContent) + rm.Action = action + + actions := map[string]bundle.Action{} + if !isBuiltin { + actions[action] = bundle.Action{Modifies: modifies} + } + rm.bundle = cnab.NewBundle(bundle.Bundle{Actions: actions}) + + require.NoError(t, testConfig.FileSystem.MkdirAll(config.BundleOutputsDir, pkg.FileModeDirectory)) + return rm, testConfig + } + + t.Run("modifies false still writes porter-state", func(t *testing.T) { + // packStateBag always runs so the CNAB driver can collect the output. + // Skipping the save on the host side is what prevents state from being + // persisted for modifies:false actions. + rm, testConfig := setup(t, "dry-run", false, false) + require.NoError(t, rm.Finalize(context.Background())) + exists, err := testConfig.FileSystem.Exists("/cnab/app/outputs/porter-state") + require.NoError(t, err) + assert.True(t, exists, "porter-state must be written so the CNAB driver can collect it") + }) + + t.Run("modifies true writes porter-state", func(t *testing.T) { + rm, testConfig := setup(t, "rotate-creds", true, false) + require.NoError(t, rm.Finalize(context.Background())) + exists, err := testConfig.FileSystem.Exists("/cnab/app/outputs/porter-state") + require.NoError(t, err) + assert.True(t, exists, "porter-state should be written for modifies: true actions") + }) + + t.Run("built-in action writes porter-state", func(t *testing.T) { + rm, testConfig := setup(t, cnab.ActionInstall, false, true) + require.NoError(t, rm.Finalize(context.Background())) + exists, err := testConfig.FileSystem.Exists("/cnab/app/outputs/porter-state") + require.NoError(t, err) + assert.True(t, exists, "porter-state should be written for built-in actions") + }) +} + func TestResolveInvocationImage(t *testing.T) { testcases := []struct { name string diff --git a/pkg/storage/run_test.go b/pkg/storage/run_test.go index 902414cd2..9597021b1 100644 --- a/pkg/storage/run_test.go +++ b/pkg/storage/run_test.go @@ -107,6 +107,27 @@ func TestRun_ShouldRecord(t *testing.T) { assert.True(t, r.ShouldRecord()) }) + t.Run("modifies false, stateful, with applyTo output", func(t *testing.T) { + // Stateful + modifies:false + user output: run and outputs are recorded. + // porter-state isolation is enforced separately in Finalize(), not here. + b := bundle.Bundle{ + Actions: map[string]bundle.Action{ + "dry-run": { + Modifies: false, + Stateless: false, + }, + }, + Outputs: map[string]bundle.Output{ + "diff": { + ApplyTo: []string{"dry-run"}, + }, + }, + } + + r := Run{Bundle: b, Action: "dry-run"} + assert.True(t, r.ShouldRecord()) + }) + t.Run("has only internal bundle level output", func(t *testing.T) { b := bundle.Bundle{ Definitions: definition.Definitions{ diff --git a/tests/integration/invoke_test.go b/tests/integration/invoke_test.go index df8c2b5e0..72475373b 100644 --- a/tests/integration/invoke_test.go +++ b/tests/integration/invoke_test.go @@ -62,3 +62,51 @@ func TestInvokeCustomAction(t *testing.T) { require.NoError(t, err, "GetLastClaim failed") assert.Equal(t, "zombies", c.Action, "the custom action wasn't recorded in the installation") } + +func TestInvoke_ModifiesFalse_DoesNotPersistState(t *testing.T) { + t.Parallel() + + p := porter.NewTestPorter(t) + defer p.Close() + ctx := p.SetupIntegrationTest() + + err := p.Create() + require.NoError(t, err) + + bundleName := p.AddTestBundleDir("testdata/bundles/bundle-with-modifies-false-action", true) + + // Install to establish initial porter-state + installOpts := porter.NewInstallOptions() + err = installOpts.Validate(ctx, []string{}, p.Porter) + require.NoError(t, err) + err = p.InstallBundle(ctx, installOpts) + require.NoError(t, err, "install should have succeeded") + + // Capture the porter-state output written during install + stateAfterInstall, err := p.Installations.GetLastOutput(ctx, "", bundleName, "porter-state") + require.NoError(t, err, "porter-state output should exist after install") + + // Invoke the modifies:false dry-run action + invokeOpts := porter.NewInvokeOptions() + invokeOpts.Action = "dry-run" + err = invokeOpts.Validate(ctx, []string{}, p.Porter) + require.NoError(t, err) + err = p.InvokeBundle(ctx, invokeOpts) + require.NoError(t, err, "dry-run invoke should have succeeded") + + // porter-state must not have been updated by the dry-run + stateAfterDryRun, err := p.Installations.GetLastOutput(ctx, "", bundleName, "porter-state") + require.NoError(t, err, "porter-state output should still exist after dry-run") + assert.Equal(t, stateAfterInstall.ResultID, stateAfterDryRun.ResultID, + "porter-state should not have been updated by a modifies:false action") + + // The dry-run run record must exist + lastRun, err := p.Installations.GetLastRun(ctx, "", bundleName) + require.NoError(t, err, "GetLastRun failed") + assert.Equal(t, "dry-run", lastRun.Action, "dry-run should have been recorded") + + // The user-defined output from dry-run must have been saved + dryRunOutput, err := p.Installations.GetOutput(ctx, lastRun.ID, "dry-run-result") + require.NoError(t, err, "dry-run-result output should have been saved") + assert.Contains(t, string(dryRunOutput.Value), "dry-run-ok") +} diff --git a/tests/integration/testdata/bundles/bundle-with-modifies-false-action/porter.yaml b/tests/integration/testdata/bundles/bundle-with-modifies-false-action/porter.yaml new file mode 100644 index 000000000..f18d70376 --- /dev/null +++ b/tests/integration/testdata/bundles/bundle-with-modifies-false-action/porter.yaml @@ -0,0 +1,59 @@ +schemaVersion: 1.0.0-alpha.1 +name: bundle-with-modifies-false-action +version: 0.1.0 +description: "Bundle for testing that modifies:false actions do not persist porter-state" +registry: "localhost:5000" + +mixins: + - exec + +state: + - name: some_state + path: /path/to/some_state + +customActions: + dry-run: + description: "Dry run - does not modify resources" + stateless: false + modifies: false + +install: + - exec: + description: "Install" + command: sh + arguments: + - -c + - | + mkdir -p /path/to + echo "installed" > /path/to/some_state + +dry-run: + - exec: + description: "Dry run" + command: sh + arguments: + - -c + - echo "dry-run-ok" > /cnab/app/outputs/dry-run-result + +upgrade: + - exec: + description: "Upgrade" + command: sh + arguments: + - -c + - echo "upgraded" + +uninstall: + - exec: + description: "Uninstall" + command: sh + arguments: + - -c + - echo "uninstalled" + +outputs: + - name: dry-run-result + type: string + path: /cnab/app/outputs/dry-run-result + applyTo: + - dry-run