Improve applying installation parameters without reparsing as flags

carolynvs · carolynvs · commit dc71d166a522 · 2022-11-14T17:13:30.000-06:00
Do not re-parse parameters set on an installation resource as --param flags and instead apply any --param flag overrides on top of parameters defined on the installation. This changes the current behavior which would remove any parameter overrides set on an installation resource when the overrides were not re-specified with --param.

Example:

```
porter install mybuns --param logLevel=10

porter installation show mybuns -o yaml &gt; mybuns.yaml

porter installation apply mybuns.yaml
```

Signed-off-by: Carolyn Van Slyck &lt;me@carolynvanslyck.com&gt;
diff --git a/pkg/porter/install.go b/pkg/porter/install.go
@@ -101,7 +101,7 @@ func (p *Porter) InstallBundle(ctx context.Context, opts InstallOptions) error {
 // Users are expected to edit the installation record if they don't want that behavior.
 func (p *Porter) applyActionOptionsToInstallation(ctx context.Context, i *storage.Installation, opts *BundleExecutionOptions) error {
 	// Record the parameters specified by the user, with flags taking precedence over parameter set values
-	err := opts.LoadParameters(ctx, p, opts.bundleRef.Definition)
+	err := opts.LoadParameters(ctx, p, opts.bundleRef.Definition, i.Parameters)
 	if err != nil {
 		return err
 	}
diff --git a/pkg/porter/lifecycle.go b/pkg/porter/lifecycle.go
@@ -102,7 +102,7 @@ func (o *BundleExecutionOptions) Validate(ctx context.Context, args []string, p
 
 // LoadParameters validates and resolves the parameters and sets. It must be
 // called after porter has loaded the bundle definition.
-func (o *BundleExecutionOptions) LoadParameters(ctx context.Context, p *Porter, bun cnab.ExtendedBundle) error {
+func (o *BundleExecutionOptions) LoadParameters(ctx context.Context, p *Porter, bun cnab.ExtendedBundle, internalPs storage.ParameterSet) error {
 	// This is called in multiple code paths, so exit early if
 	// we have already loaded the parameters into combinedParameters
 	if o.combinedParameters != nil {
@@ -114,7 +114,7 @@ func (o *BundleExecutionOptions) LoadParameters(ctx context.Context, p *Porter,
 		return err
 	}
 
-	err = o.parseParamSets(ctx, p, bun)
+	err = o.parseParamSets(ctx, p, bun, internalPs)
 	if err != nil {
 		return err
 	}
@@ -135,33 +135,39 @@ func (o *BundleExecutionOptions) parseParams() error {
 	return nil
 }
 
-func (o *BundleExecutionOptions) populateInternalParameterSet(ctx context.Context, p *Porter, bun cnab.ExtendedBundle, i *storage.Installation) error {
+func (o *BundleExecutionOptions) populateInternalParameterSet(ctx context.Context, p *Porter, bun cnab.ExtendedBundle, inst *storage.Installation) error {
 	strategies := make([]secrets.Strategy, 0, len(o.parsedParams))
 	for name, value := range o.parsedParams {
 		strategies = append(strategies, storage.ValueStrategy(name, value))
 	}
 
-	strategies, err := p.Sanitizer.CleanParameters(ctx, strategies, bun, i.ID)
+	strategies, err := p.Sanitizer.CleanParameters(ctx, strategies, bun, inst.ID)
 	if err != nil {
 		return err
 	}
 
-	if len(strategies) == 0 {
-		// if no override is specified, clear out the old parameters on the
-		// installation record
-		i.Parameters.Parameters = nil
-		return nil
-	}
+	for _, paramOverride := range strategies {
+		replaced := false
+		for i, existingParam := range inst.Parameters.Parameters {
+			if paramOverride.Name == existingParam.Name {
+				inst.Parameters.Parameters[i] = paramOverride
+				replaced = true
+				break
+			}
+		}
 
-	i.Parameters = i.NewInternalParameterSet(strategies...)
+		if !replaced {
+			inst.Parameters.Parameters = append(inst.Parameters.Parameters, paramOverride)
+		}
+	}
 
 	return nil
 }
 
 // parseParamSets parses the variable assignments in ParameterSets.
-func (o *BundleExecutionOptions) parseParamSets(ctx context.Context, p *Porter, bun cnab.ExtendedBundle) error {
+func (o *BundleExecutionOptions) parseParamSets(ctx context.Context, p *Porter, bun cnab.ExtendedBundle, internalPs storage.ParameterSet) error {
 	if len(o.ParameterSets) > 0 {
-		parsed, err := p.loadParameterSets(ctx, bun, o.Namespace, o.ParameterSets)
+		parsed, err := p.loadParameterSets(ctx, bun, o.Namespace, o.ParameterSets, internalPs)
 		if err != nil {
 			return fmt.Errorf("unable to process provided parameter sets: %w", err)
 		}
@@ -358,7 +364,7 @@ func (p *Porter) BuildActionArgs(ctx context.Context, installation storage.Insta
 
 	// Resolve the final set of typed parameters, taking into account the user overrides, parameter sources
 	// and defaults
-	err = opts.LoadParameters(ctx, p, opts.bundleRef.Definition)
+	err = opts.LoadParameters(ctx, p, opts.bundleRef.Definition, installation.Parameters)
 	if err != nil {
 		return bundleruntime.ActionArguments{}, err
 	}
diff --git a/pkg/porter/lifecycle_test.go b/pkg/porter/lifecycle_test.go
@@ -252,7 +252,7 @@ func TestBundleExecutionOptions_ParseParamSets(t *testing.T) {
 	err := opts.Validate(ctx, []string{}, p.Porter)
 	assert.NoError(t, err)
 
-	err = opts.parseParamSets(ctx, p.Porter, cnab.ExtendedBundle{})
+	err = opts.parseParamSets(ctx, p.Porter, cnab.ExtendedBundle{}, storage.ParameterSet{})
 	assert.NoError(t, err)
 
 	wantParams := map[string]string{
@@ -282,7 +282,7 @@ func TestBundleExecutionOptions_ParseParamSets_Failed(t *testing.T) {
 	err = opts.Validate(ctx, []string{}, p.Porter)
 	assert.NoError(t, err)
 
-	err = opts.parseParamSets(ctx, p.Porter, bun)
+	err = opts.parseParamSets(ctx, p.Porter, bun, storage.ParameterSet{})
 	assert.Error(t, err)
 
 }
@@ -302,7 +302,7 @@ func TestBundleExecutionOptions_LoadParameters(t *testing.T) {
 	opts := NewBundleExecutionOptions()
 	opts.Params = []string{"my-first-param=1", "my-second-param=2"}
 
-	err = opts.LoadParameters(context.Background(), p.Porter, bun)
+	err = opts.LoadParameters(context.Background(), p.Porter, bun, storage.ParameterSet{})
 	require.NoError(t, err)
 
 	assert.Len(t, opts.Params, 2)
@@ -383,11 +383,11 @@ func TestBundleExecutionOptions_populateInternalParameterSet(t *testing.T) {
 	opts := NewBundleExecutionOptions()
 	opts.Params = []string{nonsensitiveParamName + "=" + nonsensitiveParamValue, sensitiveParamName + "=" + sensitiveParamValue}
 
-	err = opts.LoadParameters(ctx, p.Porter, bun)
-	require.NoError(t, err)
-
 	i := storage.NewInstallation("", bun.Name)
 
+	err = opts.LoadParameters(ctx, p.Porter, bun, i.Parameters)
+	require.NoError(t, err)
+
 	err = opts.populateInternalParameterSet(ctx, p.Porter, bun, &i)
 	require.NoError(t, err)
 
@@ -408,10 +408,54 @@ func TestBundleExecutionOptions_populateInternalParameterSet(t *testing.T) {
 	// as well
 	opts.combinedParameters = nil
 	opts.Params = make([]string, 0)
-	err = opts.LoadParameters(ctx, p.Porter, bun)
+	err = opts.LoadParameters(ctx, p.Porter, bun, i.Parameters)
+	require.NoError(t, err)
+	err = opts.populateInternalParameterSet(ctx, p.Porter, bun, &i)
+	require.NoError(t, err)
+
+	// Check that when no parameter overrides are specified, we use the originally specified parameters from the previous run
+	require.Len(t, i.Parameters.Parameters, 2)
+	require.Equal(t, "my-first-param", i.Parameters.Parameters[0].Name)
+	require.Equal(t, "1", i.Parameters.Parameters[0].Source.Value)
+	require.Equal(t, "my-second-param", i.Parameters.Parameters[1].Name)
+	require.Equal(t, "secret", i.Parameters.Parameters[1].Source.Key)
+}
+
+func TestBundleExecutionOptions_populateInternalParameterSet_ExistingParams(t *testing.T) {
+	p := NewTestPorter(t)
+	defer p.Close()
+
+	ctx := context.Background()
+
+	p.TestConfig.TestContext.AddTestFile("testdata/porter.yaml", config.Name)
+	m, err := manifest.LoadManifestFrom(context.Background(), p.Config, config.Name)
+	require.NoError(t, err)
+	bun, err := configadapter.ConvertToTestBundle(ctx, p.Config, m)
 	require.NoError(t, err)
+
+	nonsensitiveParamName := "my-first-param"
+	nonsensitiveParamValue := "3"
+	opts := NewBundleExecutionOptions()
+	opts.Params = []string{nonsensitiveParamName + "=" + nonsensitiveParamValue}
+
+	i := storage.NewInstallation("", bun.Name)
+	i.Parameters = storage.NewParameterSet("", "internal-ps",
+		storage.ValueStrategy("my-first-param", "1"),
+		storage.ValueStrategy("my-second-param", "2"),
+	)
+	err = opts.LoadParameters(ctx, p.Porter, bun, i.Parameters)
+	require.NoError(t, err)
+
 	err = opts.populateInternalParameterSet(ctx, p.Porter, bun, &i)
 	require.NoError(t, err)
 
-	require.Len(t, i.Parameters.Parameters, 0)
+	require.Len(t, i.Parameters.Parameters, 2)
+
+	// Check that overrides are applied on top of existing parameters
+	require.Len(t, i.Parameters.Parameters, 2)
+	require.Equal(t, "my-first-param", i.Parameters.Parameters[0].Name)
+	require.Equal(t, "3", i.Parameters.Parameters[0].Source.Value)
+	require.Equal(t, "my-second-param", i.Parameters.Parameters[1].Name)
+	require.Equal(t, "value", i.Parameters.Parameters[1].Source.Key)
+	require.Equal(t, "2", i.Parameters.Parameters[1].Source.Value)
 }
diff --git a/pkg/porter/list.go b/pkg/porter/list.go
@@ -168,7 +168,7 @@ func (d DisplayInstallation) ConvertToInstallation() (storage.Installation, erro
 	}
 
 	var err error
-	i.Parameters, err = d.ConvertParamToSet(i)
+	i.Parameters, err = d.ConvertParamToSet()
 	if err != nil {
 		return storage.Installation{}, err
 	}
@@ -182,18 +182,15 @@ func (d DisplayInstallation) ConvertToInstallation() (storage.Installation, erro
 }
 
 // ConvertParamToSet converts a Parameters into a internal ParameterSet.
-func (d DisplayInstallation) ConvertParamToSet(i storage.Installation) (storage.ParameterSet, error) {
+func (d DisplayInstallation) ConvertParamToSet() (storage.ParameterSet, error) {
 	strategies := make([]secrets.Strategy, 0, len(d.Parameters))
 	for name, value := range d.Parameters {
 		stringVal, err := cnab.WriteParameterToString(name, value)
 		if err != nil {
 			return storage.ParameterSet{}, err
 		}
-		strategy := secrets.Strategy{
-			Name:  name,
-			Value: stringVal,
-		}
-		strategies = append(strategies, strategy)
+
+		strategies = append(strategies, storage.ValueStrategy(name, stringVal))
 	}
 
 	return storage.NewInternalParameterSet(d.Namespace, d.Name, strategies...), nil
diff --git a/pkg/porter/parameters.go b/pkg/porter/parameters.go
@@ -343,10 +343,11 @@ func validateParameterName(args []string) error {
 }
 
 // loadParameterSets loads parameter values per their parameter set strategies
-func (p *Porter) loadParameterSets(ctx context.Context, bun cnab.ExtendedBundle, namespace string, params []string) (secrets.Set, error) {
+func (p *Porter) loadParameterSets(ctx context.Context, bun cnab.ExtendedBundle, namespace string, params []string, internalPs storage.ParameterSet) (secrets.Set, error) {
 	resolvedParameters := secrets.Set{}
-	for _, name := range params {
 
+	// First apply all named parameter sets
+	for _, name := range params {
 		// Try to get the params in the local namespace first, fallback to the global creds
 		query := storage.FindOptions{
 			Sort: []string{"-namespace"},
@@ -398,6 +399,15 @@ func (p *Porter) loadParameterSets(ctx context.Context, bun cnab.ExtendedBundle,
 		}
 	}
 
+	// Then resolve the installation's internal parameter set and apply it on top
+	rc, err := p.Parameters.ResolveAll(ctx, internalPs)
+	if err != nil {
+		return nil, err
+	}
+	for k, v := range rc {
+		resolvedParameters[k] = v
+	}
+
 	return resolvedParameters, nil
 }
 
diff --git a/pkg/porter/reconcile.go b/pkg/porter/reconcile.go
@@ -73,24 +73,14 @@ func (p *Porter) ReconcileInstallation(ctx context.Context, opts ReconcileOption
 	lifecycleOpts.Name = opts.Name
 	lifecycleOpts.Namespace = opts.Namespace
 	lifecycleOpts.CredentialIdentifiers = opts.Installation.CredentialSets
-
 	lifecycleOpts.ParameterSets = opts.Installation.ParameterSets
-	lifecycleOpts.Params = make([]string, 0, len(opts.Installation.Parameters.Parameters))
 
 	// Write out the parameters as string values. Not efficient but reusing ExecuteAction would need more refactoring otherwise
 	_, err = p.resolveBundleReference(ctx, lifecycleOpts.BundleReferenceOptions)
 	if err != nil {
 		return err
 	}
 
-	for _, param := range opts.Installation.Parameters.Parameters {
-		lifecycleOpts.Params = append(lifecycleOpts.Params, fmt.Sprintf("%s=%s", param.Name, param.Value))
-	}
-
-	if err := p.applyActionOptionsToInstallation(ctx, &opts.Installation, lifecycleOpts); err != nil {
-		return err
-	}
-
 	if !opts.DryRun {
 		if err = p.Installations.UpsertInstallation(ctx, opts.Installation); err != nil {
 			return err

Original file line number	Diff line number	Diff line change
`@@ -101,7 +101,7 @@ func (p *Porter) InstallBundle(ctx context.Context, opts InstallOptions) error {`
`101`	`101`	`// Users are expected to edit the installation record if they don't want that behavior.`
`102`	`102`	`func (p Porter) applyActionOptionsToInstallation(ctx context.Context, i storage.Installation, opts *BundleExecutionOptions) error {`
`103`	`103`	`// Record the parameters specified by the user, with flags taking precedence over parameter set values`
`104`		`- err := opts.LoadParameters(ctx, p, opts.bundleRef.Definition)`
	`104`	`+ err := opts.LoadParameters(ctx, p, opts.bundleRef.Definition, i.Parameters)`
`105`	`105`	`if err != nil {`
`106`	`106`	`return err`
`107`	`107`	`}`
Original file line number	Diff line number	Diff line change
`@@ -168,7 +168,7 @@ func (d DisplayInstallation) ConvertToInstallation() (storage.Installation, erro`
`168`	`168`	`}`
`169`	`169`
`170`	`170`	`var err error`
`171`		`- i.Parameters, err = d.ConvertParamToSet(i)`
	`171`	`+ i.Parameters, err = d.ConvertParamToSet()`
`172`	`172`	`if err != nil {`
`173`	`173`	`return storage.Installation{}, err`
`174`	`174`	`}`
`@@ -182,18 +182,15 @@ func (d DisplayInstallation) ConvertToInstallation() (storage.Installation, erro`
`182`	`182`	`}`
`183`	`183`
`184`	`184`	`// ConvertParamToSet converts a Parameters into a internal ParameterSet.`
`185`		`-func (d DisplayInstallation) ConvertParamToSet(i storage.Installation) (storage.ParameterSet, error) {`
	`185`	`+func (d DisplayInstallation) ConvertParamToSet() (storage.ParameterSet, error) {`
`186`	`186`	`strategies := make([]secrets.Strategy, 0, len(d.Parameters))`
`187`	`187`	`for name, value := range d.Parameters {`
`188`	`188`	`stringVal, err := cnab.WriteParameterToString(name, value)`
`189`	`189`	`if err != nil {`
`190`	`190`	`return storage.ParameterSet{}, err`
`191`	`191`	`}`
`192`		`- strategy := secrets.Strategy{`
`193`		`- Name: name,`
`194`		`- Value: stringVal,`
`195`		`- }`
`196`		`- strategies = append(strategies, strategy)`
	`192`	`+`
	`193`	`+ strategies = append(strategies, storage.ValueStrategy(name, stringVal))`
`197`	`194`	`}`
`198`	`195`
`199`	`196`	`return storage.NewInternalParameterSet(d.Namespace, d.Name, strategies...), nil`