diff --git a/pkg/cnab/config-adapter/adapter.go b/pkg/cnab/config-adapter/adapter.go index d4129b457..efdb12072 100644 --- a/pkg/cnab/config-adapter/adapter.go +++ b/pkg/cnab/config-adapter/adapter.go @@ -540,11 +540,11 @@ func (c *ManifestConverter) generateParameterSources(b *cnab.ExtendedBundle) cna ps["porter-state"] = c.generateOutputParameterSource("porter-state") // bundle.outputs.OUTPUT + if b.Parameters == nil { + b.Parameters = make(map[string]bundle.Parameter) + } for _, outputDef := range c.Manifest.GetTemplatedOutputs() { wiringName, p, def := c.generateOutputWiringParameter(*b, outputDef.Name) - if b.Parameters == nil { - b.Parameters = make(map[string]bundle.Parameter, 1) - } b.Parameters[wiringName] = p b.Definitions[wiringName] = &def @@ -555,9 +555,6 @@ func (c *ManifestConverter) generateParameterSources(b *cnab.ExtendedBundle) cna // bundle.dependencies.DEP.outputs.OUTPUT for _, ref := range c.Manifest.GetTemplatedDependencyOutputs() { wiringName, p, def := c.generateDependencyOutputWiringParameter(ref) - if b.Parameters == nil { - b.Parameters = make(map[string]bundle.Parameter, 1) - } b.Parameters[wiringName] = p b.Definitions[wiringName] = &def diff --git a/pkg/cnab/provider/credentials_test.go b/pkg/cnab/provider/credentials_test.go index a4f90e2e3..78f8f60e8 100644 --- a/pkg/cnab/provider/credentials_test.go +++ b/pkg/cnab/provider/credentials_test.go @@ -23,12 +23,10 @@ func TestRuntime_loadCredentials(t *testing.T) { r.TestConfig.TestContext.AddTestFile("testdata/db-creds.json", "/db-creds.json") - cs1 := storage.NewCredentialSet("", "mycreds", secrets.SourceMap{ - Name: "password", - Source: secrets.Source{ - Strategy: secrets.SourceSecret, - Hint: "password", - }, + cs1 := storage.NewCredentialSet("", "mycreds") + cs1.SetStrategy("password", secrets.Source{ + Strategy: secrets.SourceSecret, + Hint: "password", }) err := r.credentials.InsertCredentialSet(context.Background(), cs1) @@ -135,12 +133,10 @@ func TestRuntime_loadCredentials_WithApplyTo(t *testing.T) { r.TestCredentials.AddSecret("password", "mypassword") - cs1 := storage.NewCredentialSet("", "mycreds", secrets.SourceMap{ - Name: "password", - Source: secrets.Source{ - Strategy: secrets.SourceSecret, - Hint: "password", - }, + cs1 := storage.NewCredentialSet("", "mycreds") + cs1.SetStrategy("password", secrets.Source{ + Strategy: secrets.SourceSecret, + Hint: "password", }) err := r.credentials.InsertCredentialSet(context.Background(), cs1) diff --git a/pkg/encoding/array_map.go b/pkg/encoding/array_map.go new file mode 100644 index 000000000..235704962 --- /dev/null +++ b/pkg/encoding/array_map.go @@ -0,0 +1,201 @@ +package encoding + +import ( + "encoding/json" + "fmt" + "sort" + + "gopkg.in/yaml.v3" +) + +// MapElement is the in-memory representation of the item when stored in a map. +type MapElement interface { + ToArrayEntry(key string) ArrayElement +} + +// ArrayElement is the representation of the item when encoded to an array in yaml or json. +type ArrayElement interface { + GetKey() string + ToMapEntry() MapElement +} + +// ArrayMap is a map that is represented as an array when marshaled. +type ArrayMap[T MapElement, K ArrayElement] struct { + items map[string]T +} + +func MakeArrayMap[T MapElement, K ArrayElement](len int) ArrayMap[T, K] { + return ArrayMap[T, K]{ + items: make(map[string]T, len), + } +} + +func (m *ArrayMap[T, K]) Len() int { + if m == nil { + return 0 + } + return len(m.items) +} + +func (m *ArrayMap[T, K]) Items() map[string]T { + if m == nil { + return nil + } + + result := make(map[string]T, len(m.items)) + for k, v := range m.items { + result[k] = v + } + return result +} + +func (m *ArrayMap[T, K]) ItemsSorted() []K { + if m == nil { + return nil + } + + result := make([]K, len(m.items)) + i := 0 + for k, v := range m.items { + // I can't figure out how to constrain T such that ToArrayEntry returns K, so I'm doing a cast + result[i] = v.ToArrayEntry(k).(K) + i++ + } + sort.SliceStable(result, func(i, j int) bool { + return result[i].GetKey() < result[j].GetKey() + }) + + return result +} + +func (m *ArrayMap[T, K]) ItemsUnsafe() map[string]T { + if m == nil { + return nil + } + + if m.items == nil { + m.items = make(map[string]T) + } + + return m.items +} + +func (m *ArrayMap[T, K]) Get(key string) (T, bool) { + if m == nil { + return *new(T), false + } + + entry, ok := m.items[key] + return entry, ok +} + +func (m *ArrayMap[T, K]) Set(key string, entry T) { + if m.items == nil { + m.items = make(map[string]T, 1) + } + + m.items[key] = entry +} + +func (m *ArrayMap[T, K]) Remove(key string) { + if m == nil { + return + } + delete(m.items, key) +} + +func (m *ArrayMap[T, K]) MarshalRaw() interface{} { + if m == nil { + return nil + } + + var raw []ArrayElement + if m.items == nil { + return raw + } + + raw = make([]ArrayElement, 0, len(m.items)) + for k, v := range m.items { + raw = append(raw, v.ToArrayEntry(k)) + } + sort.SliceStable(raw, func(i, j int) bool { + return raw[i].GetKey() < raw[j].GetKey() + }) + return raw +} + +func (m *ArrayMap[T, K]) UnmarshalRaw(raw []K) error { + // If there's nothing to import, stop early and allow the map to keep its original value + // So if someone unmarshalled into a nil map, it stays nil. + // This more closely matches how the stdlib encoders work + if len(raw) == 0 { + return nil + } + + if m == nil { + *m = ArrayMap[T, K]{} + } + + m.items = make(map[string]T, len(raw)) + for _, rawItem := range raw { + if _, hasKey := m.items[rawItem.GetKey()]; hasKey { + return fmt.Errorf("cannot unmarshal source map: duplicate key found '%s'", rawItem.GetKey()) + } + item := rawItem.ToMapEntry() + typedItem, ok := item.(T) + if !ok { + return fmt.Errorf("invalid ArrayMap generic types, ArrayElement %T returned a %T from ToMapEntry(), when it should return %T", rawItem, item, *new(T)) + } + m.items[rawItem.GetKey()] = typedItem + } + return nil +} + +func (m *ArrayMap[T, K]) MarshalJSON() ([]byte, error) { + raw := m.MarshalRaw() + return json.Marshal(raw) +} + +func (m *ArrayMap[T, K]) UnmarshalJSON(data []byte) error { + var raw []K + err := json.Unmarshal(data, &raw) + if err != nil { + return err + } + return m.UnmarshalRaw(raw) +} + +func (m *ArrayMap[T, K]) UnmarshalYAML(value *yaml.Node) error { + var raw []K + if err := value.Decode(&raw); err != nil { + return err + } + return m.UnmarshalRaw(raw) +} + +func (m *ArrayMap[T, K]) MarshalYAML() (interface{}, error) { + if m == nil { + return nil, nil + } + return m.MarshalRaw(), nil +} + +// Merge applies the specified values on top of a base set of values. When a +// name exists in both sets, use the value from the overrides +func (m *ArrayMap[T, K]) Merge(overrides *ArrayMap[T, K]) *ArrayMap[T, K] { + result := make(map[string]T, m.Len()) + if m != nil { + for k, v := range m.items { + result[k] = v + } + } + + if overrides != nil { + // If the name is in the base, overwrite its value with the override provided + for k, v := range overrides.items { + result[k] = v + } + } + + return &ArrayMap[T, K]{items: result} +} diff --git a/pkg/encoding/array_map_test.go b/pkg/encoding/array_map_test.go new file mode 100644 index 000000000..f911c9d1b --- /dev/null +++ b/pkg/encoding/array_map_test.go @@ -0,0 +1,127 @@ +package encoding + +import ( + "encoding/json" + "os" + "testing" + + "get.porter.sh/porter/pkg/yaml" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestArrayMap_Merge(t *testing.T) { + m := &ArrayMap[TestMapEntry, TestArrayEntry]{} + m.Set("first", TestMapEntry{Value: "base first"}) + m.Set("second", TestMapEntry{Value: "base second"}) + m.Set("third", TestMapEntry{Value: "base third"}) + + result := m.Merge(&ArrayMap[TestMapEntry, TestArrayEntry]{}) + require.Equal(t, 3, result.Len()) + _, ok := result.Get("fourth") + assert.False(t, ok, "fourth should not be present in the base set") + + wantFourth := TestMapEntry{Value: "new fourth"} + fourthMap := &ArrayMap[TestMapEntry, TestArrayEntry]{items: map[string]TestMapEntry{"fourth": wantFourth}} + result = m.Merge(fourthMap) + require.Equal(t, 4, result.Len()) + gotFourth, ok := result.Get("fourth") + require.True(t, ok, "fourth should be present in the merged set") + assert.Equal(t, wantFourth, gotFourth, "incorrect merged value for fourth") + + wantSecond := TestMapEntry{Value: "override second"} + secondMap := &ArrayMap[TestMapEntry, TestArrayEntry]{items: map[string]TestMapEntry{"second": wantSecond}} + result = m.Merge(secondMap) + require.Equal(t, 3, result.Len()) + gotSecond, ok := result.Get("second") + require.True(t, ok, "second should be present in the merged set") + assert.Equal(t, wantSecond, gotSecond, "incorrect merged value for second") +} + +func TestArrayMap_Unmarshal(t *testing.T) { + // TODO: add testcase for json + data, err := os.ReadFile("testdata/array.yaml") + require.NoError(t, err, "ReadFile failed") + + var m ArrayMap[TestMapEntry, TestArrayEntry] + err = yaml.Unmarshal(data, &m) + require.NoError(t, err, "Unmarshal failed") + + require.Equal(t, 2, m.Len(), "unexpected number of items defined") + + gotA, ok := m.Get("aname") + require.True(t, ok, "aname was not defined") + wantA := TestMapEntry{Value: "stuff"} + assert.Equal(t, wantA, gotA, "unexpected aname defined") + + gotB, ok := m.Get("bname") + require.True(t, ok, "password was not defined") + wantB := TestMapEntry{Value: "things"} + assert.Equal(t, wantB, gotB, "unexpected bname defined") +} + +func TestArrayMap_Marshal(t *testing.T) { + // TODO: add testcase for json + + m := &ArrayMap[TestMapEntry, TestArrayEntry]{} + m.Set("bname", TestMapEntry{Value: "things"}) + m.Set("aname", TestMapEntry{Value: "stuff"}) + + wantData, err := os.ReadFile("testdata/array.yaml") + require.NoError(t, err, "ReadFile failed") + + gotData, err := yaml.Marshal(m) + require.NoError(t, err, "Marshal failed") + assert.Equal(t, string(wantData), string(gotData)) +} + +func TestArrayMap_Unmarshal_DuplicateKeys(t *testing.T) { + data, err := os.ReadFile("testdata/array-with-duplicates.yaml") + require.NoError(t, err, "ReadFile failed") + + var l ArrayMap[TestMapEntry, TestArrayEntry] + err = yaml.Unmarshal(data, &l) + require.ErrorContains(t, err, "cannot unmarshal source map: duplicate key found 'aname'") +} + +// check that when we round trip a null ArrayMap, it stays null and isn't initialized to an _empty_ ArrayMap +// This impacts how it is marshaled later to yaml or json, because we often have fields tagged with omitempty +// and so it must be null to not be written out. +func TestArrayMap_RoundTrip_Empty(t *testing.T) { + wantData, err := os.ReadFile("testdata/array-empty.json") + require.NoError(t, err, "ReadFile failed") + + var s struct { + Items *ArrayMap[TestMapEntry, TestArrayEntry] `json:"items"` + } + s.Items = &ArrayMap[TestMapEntry, TestArrayEntry]{} + + gotData, err := json.Marshal(s) + require.NoError(t, err, "Marshal failed") + require.Equal(t, string(wantData), string(gotData), "empty ArrayMap should not marshal as empty, but nil so that it works with omitempty") + + err = json.Unmarshal(gotData, &s) + require.NoError(t, err, "Unmarshal failed") + require.Nil(t, s.Items, "null ArrayMap should unmarshal as nil") +} + +type TestMapEntry struct { + Value string `json:"value" yaml:"value"` +} + +func (t TestMapEntry) ToArrayEntry(key string) ArrayElement { + return TestArrayEntry{Name: key, Value: t.Value} +} + +type TestArrayEntry struct { + Name string `json:"name" yaml:"name"` + Value string `json:"value" yaml:"value"` +} + +func (t TestArrayEntry) ToMapEntry() MapElement { + return TestMapEntry{Value: t.Value} +} + +func (t TestArrayEntry) GetKey() string { + return t.Name +} diff --git a/pkg/encoding/testdata/array-empty.json b/pkg/encoding/testdata/array-empty.json new file mode 100644 index 000000000..6214a74cd --- /dev/null +++ b/pkg/encoding/testdata/array-empty.json @@ -0,0 +1 @@ +{"items":null} \ No newline at end of file diff --git a/pkg/encoding/testdata/array-with-duplicates.yaml b/pkg/encoding/testdata/array-with-duplicates.yaml new file mode 100644 index 000000000..12809b578 --- /dev/null +++ b/pkg/encoding/testdata/array-with-duplicates.yaml @@ -0,0 +1,6 @@ +- name: aname + value: stuff +- name: bname + value: things +- name: aname + value: duplicate stuff diff --git a/pkg/encoding/testdata/array.yaml b/pkg/encoding/testdata/array.yaml new file mode 100644 index 000000000..d3f02f5e3 --- /dev/null +++ b/pkg/encoding/testdata/array.yaml @@ -0,0 +1,4 @@ +- name: aname + value: stuff +- name: bname + value: things diff --git a/pkg/generator/credentials.go b/pkg/generator/credentials.go index e2d22c129..e9e43a4c2 100644 --- a/pkg/generator/credentials.go +++ b/pkg/generator/credentials.go @@ -6,7 +6,6 @@ import ( "sort" "strings" - "get.porter.sh/porter/pkg/secrets" "get.porter.sh/porter/pkg/storage" "github.com/cnabio/cnab-go/bundle" ) @@ -39,7 +38,6 @@ func GenerateCredentials(opts GenerateCredentialsOptions) (storage.CredentialSet func genCredentialSet(namespace string, name string, creds map[string]bundle.Credential, fn generator) (storage.CredentialSet, error) { cs := storage.NewCredentialSet(namespace, name) - cs.Credentials = []secrets.SourceMap{} if strings.ContainsAny(name, "./\\") { return cs, fmt.Errorf("credentialset name '%s' cannot contain the following characters: './\\'", name) @@ -57,7 +55,7 @@ func genCredentialSet(namespace string, name string, creds map[string]bundle.Cre if err != nil { return cs, err } - cs.Credentials = append(cs.Credentials, c) + cs.SetStrategy(name, c) } return cs, nil diff --git a/pkg/generator/credentials_test.go b/pkg/generator/credentials_test.go index 6131b6b48..2657cb704 100644 --- a/pkg/generator/credentials_test.go +++ b/pkg/generator/credentials_test.go @@ -51,7 +51,7 @@ func TestGoodCredentialsName(t *testing.T) { cs, err := GenerateCredentials(opts) require.NoError(t, err, "name should NOT have resulted in an error") - require.Equal(t, 3, len(cs.Credentials), "should have had 3 entries") + require.Equal(t, 3, cs.Len(), "should have had 3 entries") } func TestNoCredentials(t *testing.T) { diff --git a/pkg/generator/generator.go b/pkg/generator/generator.go index 9d2484f3a..7743260fb 100644 --- a/pkg/generator/generator.go +++ b/pkg/generator/generator.go @@ -5,7 +5,7 @@ import ( "get.porter.sh/porter/pkg/secrets" "github.com/cnabio/cnab-go/secrets/host" - survey "gopkg.in/AlecAivazis/survey.v1" + "gopkg.in/AlecAivazis/survey.v1" ) // GenerateOptions are the options to generate a parameter or credential set @@ -33,18 +33,15 @@ const ( questionCommand = "shell command" ) -type generator func(name string, surveyType SurveyType) (secrets.SourceMap, error) +type generator func(name string, surveyType SurveyType) (secrets.Source, error) -func genEmptySet(name string, surveyType SurveyType) (secrets.SourceMap, error) { - return secrets.SourceMap{ - Name: name, - Source: secrets.Source{Hint: "TODO"}, - }, nil +func genEmptySet(name string, surveyType SurveyType) (secrets.Source, error) { + return secrets.Source{Hint: "TODO"}, nil } -func genSurvey(name string, surveyType SurveyType) (secrets.SourceMap, error) { +func genSurvey(name string, surveyType SurveyType) (secrets.Source, error) { if surveyType != surveyCredentials && surveyType != surveyParameters { - return secrets.SourceMap{}, fmt.Errorf("unsupported survey type: %s", surveyType) + return secrets.Source{}, fmt.Errorf("unsupported survey type: %s", surveyType) } // extra space-suffix to align question and answer. Unfortunately misaligns help text @@ -57,11 +54,9 @@ func genSurvey(name string, surveyType SurveyType) (secrets.SourceMap, error) { // extra space-suffix to align question and answer. Unfortunately misaligns help text sourceValuePromptTemplate := "Enter the %s that will be used to set %s %q\n " - c := secrets.SourceMap{Name: name} - source := "" if err := survey.AskOne(sourceTypePrompt, &source, nil); err != nil { - return c, err + return secrets.Source{}, err } promptMsg := "" @@ -84,25 +79,23 @@ func genSurvey(name string, surveyType SurveyType) (secrets.SourceMap, error) { value := "" if err := survey.AskOne(sourceValuePrompt, &value, nil); err != nil { - return c, err + return secrets.Source{}, err } + result := secrets.Source{Hint: value} switch source { case questionSecret: - c.Source.Strategy = secrets.SourceSecret - c.Source.Hint = value + result.Strategy = secrets.SourceSecret case questionValue: - c.Source.Strategy = host.SourceValue - c.Source.Hint = value + result.Strategy = host.SourceValue case questionEnvVar: - c.Source.Strategy = host.SourceEnv - c.Source.Hint = value + result.Strategy = host.SourceEnv case questionPath: - c.Source.Strategy = host.SourcePath - c.Source.Hint = value + result.Strategy = host.SourcePath case questionCommand: - c.Source.Strategy = host.SourceCommand - c.Source.Hint = value + result.Strategy = host.SourceCommand + default: + return secrets.Source{}, fmt.Errorf("unrecogized secret source strategy %q", source) } - return c, nil + return result, nil } diff --git a/pkg/generator/generator_test.go b/pkg/generator/generator_test.go index 79191b2ad..b3c248793 100644 --- a/pkg/generator/generator_test.go +++ b/pkg/generator/generator_test.go @@ -8,10 +8,7 @@ import ( ) func Test_genEmptySet(t *testing.T) { - expected := secrets.SourceMap{ - Name: "emptyset", - Source: secrets.Source{Hint: "TODO"}, - } + expected := secrets.Source{Hint: "TODO"} got, err := genEmptySet("emptyset", surveyParameters) require.NoError(t, err) @@ -21,5 +18,5 @@ func Test_genEmptySet(t *testing.T) { func Test_genSurvey_unsupported(t *testing.T) { got, err := genSurvey("myturtleset", SurveyType("turtles")) require.EqualError(t, err, "unsupported survey type: turtles") - require.Equal(t, secrets.SourceMap{}, got) + require.Equal(t, secrets.Source{}, got) } diff --git a/pkg/generator/parameters.go b/pkg/generator/parameters.go index 3007fabc8..25d49b089 100644 --- a/pkg/generator/parameters.go +++ b/pkg/generator/parameters.go @@ -54,11 +54,11 @@ func (opts *GenerateParametersOptions) genParameterSet(fn generator) (storage.Pa if opts.Bundle.IsInternalParameter(name) { continue } - c, err := fn(name, surveyParameters) + p, err := fn(name, surveyParameters) if err != nil { return pset, err } - pset.Parameters = append(pset.Parameters, c) + pset.SetStrategy(name, p) } return pset, nil diff --git a/pkg/generator/parameters_test.go b/pkg/generator/parameters_test.go index 9743f8e52..fbf9a8e8d 100644 --- a/pkg/generator/parameters_test.go +++ b/pkg/generator/parameters_test.go @@ -50,7 +50,7 @@ func TestGoodParametersName(t *testing.T) { pset, err := opts.GenerateParameters() require.NoError(t, err, "name should NOT have resulted in an error") - require.Equal(t, 3, len(pset.Parameters), "should have had 3 entries") + require.Equal(t, 3, pset.Len(), "should have had 3 entries") } func TestNoParameters(t *testing.T) { @@ -115,5 +115,5 @@ func TestSkipParameters(t *testing.T) { pset, err := opts.GenerateParameters() require.NoError(t, err, "parameters generation should not have resulted in an error") assert.Equal(t, "skip-params", pset.Name, "Name was not set") - require.Empty(t, pset.Parameters, "parameter set should have empty parameters section") + require.Equal(t, 0, pset.Len(), "parameter set should have empty parameters section") } diff --git a/pkg/porter/credentials.go b/pkg/porter/credentials.go index 78b64dcaa..308a25a1f 100644 --- a/pkg/porter/credentials.go +++ b/pkg/porter/credentials.go @@ -272,7 +272,7 @@ func (p *Porter) ShowCredential(ctx context.Context, opts CredentialShowOptions) var rows [][]string // Iterate through all CredentialStrategies and add to rows - for _, cs := range credSet.Credentials { + for _, cs := range credSet.IterateSorted() { rows = append(rows, []string{cs.Name, cs.Source.Hint, cs.Source.Strategy}) } diff --git a/pkg/porter/credentials_test.go b/pkg/porter/credentials_test.go index f4c4942a5..7a64b9f48 100644 --- a/pkg/porter/credentials_test.go +++ b/pkg/porter/credentials_test.go @@ -564,9 +564,10 @@ func TestPorter_CredentialsApply(t *testing.T) { require.NoError(t, err, "Failed to retrieve applied credential set") assert.Equal(t, "kool-kreds", cs.Name, "unexpected credential set name") - require.Len(t, cs.Credentials, 4, "expected 4 credentials in the set") - assert.Equal(t, "kool-config", cs.Credentials[0].Name, "expected the kool-config credential mapping defined") - assert.Equal(t, "path", cs.Credentials[0].Source.Strategy, "unexpected credential source") - assert.Equal(t, "/path/to/kool-config", cs.Credentials[0].Source.Hint, "unexpected credential mapping value") + require.Equal(t, 4, cs.Len(), "expected 4 credentials in the set") + koolCred, ok := cs.Get("kool-config") + require.True(t, ok, "expected 'kool-config' to be present") + assert.Equal(t, "path", koolCred.Source.Strategy, "unexpected credential source") + assert.Equal(t, "/path/to/kool-config", koolCred.Source.Hint, "unexpected credential mapping value") }) } diff --git a/pkg/porter/dependencies.go b/pkg/porter/dependencies.go index a6ed1283b..cf9912521 100644 --- a/pkg/porter/dependencies.go +++ b/pkg/porter/dependencies.go @@ -230,6 +230,9 @@ func (e *dependencyExecutioner) prepareDependency(ctx context.Context, dep *queu } } + if dep.Parameters == nil { + dep.Parameters = make(map[string]string) + } for _, manifestDep := range m.Dependencies.Requires { if manifestDep.Name == dep.Alias { for paramName, value := range manifestDep.Parameters { @@ -238,9 +241,6 @@ func (e *dependencyExecutioner) prepareDependency(ctx context.Context, dep *queu return fmt.Errorf("invalid dependencies.%s.parameters entry, %s is not a parameter defined in that bundle", dep.Alias, paramName) } - if dep.Parameters == nil { - dep.Parameters = make(map[string]string, 1) - } dep.Parameters[paramName] = value } } @@ -258,9 +258,6 @@ func (e *dependencyExecutioner) prepareDependency(ctx context.Context, dep *queu return fmt.Errorf("invalid --param %s, %s is not a parameter defined in the bundle %s", key, paramName, dep.Alias) } - if dep.Parameters == nil { - dep.Parameters = make(map[string]string, 1) - } dep.Parameters[paramName] = value delete(e.parentArgs.Params, key) } diff --git a/pkg/porter/helpers.go b/pkg/porter/helpers.go index 92ebdb340..485524cf0 100644 --- a/pkg/porter/helpers.go +++ b/pkg/porter/helpers.go @@ -234,12 +234,13 @@ func (p *TestPorter) AddTestBundleDir(bundleDir string, generateUniqueName bool) // When they are different and PORTER_UPDATE_TEST_FILES is true, the file is updated to match // the new test output. func (p *TestPorter) CompareGoldenFile(goldenFile string, got string) { + p.T().Helper() p.TestConfig.TestContext.CompareGoldenFile(goldenFile, got) } // CreateInstallation saves an installation record into claim store and store // sensitive parameters into secret store. -func (p *TestPorter) SanitizeParameters(raw []secrets.SourceMap, recordID string, bun cnab.ExtendedBundle) []secrets.SourceMap { +func (p *TestPorter) SanitizeParameters(raw storage.ParameterSourceMap, recordID string, bun cnab.ExtendedBundle) storage.ParameterSourceMap { strategies, err := p.Sanitizer.CleanParameters(context.Background(), raw, bun, recordID) require.NoError(p.T(), err) diff --git a/pkg/porter/install.go b/pkg/porter/install.go index 819987286..235bcd8d4 100644 --- a/pkg/porter/install.go +++ b/pkg/porter/install.go @@ -98,6 +98,7 @@ func (p *Porter) sanitizeInstallation(ctx context.Context, inst *storage.Install return err } - inst.Parameters = inst.NewInternalParameterSet(strategies...) + inst.Parameters = inst.NewInternalParameterSet() + inst.Parameters.Parameters = strategies return nil } diff --git a/pkg/porter/install_test.go b/pkg/porter/install_test.go index d938c2048..4104264d5 100644 --- a/pkg/porter/install_test.go +++ b/pkg/porter/install_test.go @@ -75,23 +75,14 @@ func TestPorter_ApplyParametersToInstallation(t *testing.T) { ctx := context.Background() p := NewTestPorter(t) - p.TestParameters.InsertParameterSet(ctx, storage.ParameterSet{ - ParameterSetSpec: storage.ParameterSetSpec{ - Name: "oldps1", - Parameters: []secrets.SourceMap{ - {Name: "logLevel", Source: secrets.Source{Strategy: "value", Hint: "2"}}, - }, - }, - }) + oldPS := storage.NewParameterSet("", "oldps1") + oldPS.SetStrategy("logLevel", secrets.HardCodedValueStrategy("2")) + p.TestParameters.InsertParameterSet(ctx, oldPS) + + newPS := storage.NewParameterSet("", "newps1") + newPS.SetStrategy("logLevel", secrets.HardCodedValueStrategy("11")) + p.TestParameters.InsertParameterSet(ctx, newPS) - p.TestParameters.InsertParameterSet(ctx, storage.ParameterSet{ - ParameterSetSpec: storage.ParameterSetSpec{ - Name: "newps1", - Parameters: []secrets.SourceMap{ - {Name: "logLevel", Source: secrets.Source{Strategy: "value", Hint: "11"}}, - }, - }, - }) inst := storage.NewInstallation("myns", "mybuns") inst.Bundle = storage.OCIReferenceParts{ Repository: "example.com/mybuns", diff --git a/pkg/porter/lifecycle_test.go b/pkg/porter/lifecycle_test.go index 8f0f35d73..c216a37f8 100644 --- a/pkg/porter/lifecycle_test.go +++ b/pkg/porter/lifecycle_test.go @@ -330,8 +330,9 @@ func TestPorter_applyActionOptionsToInstallation_FollowsParameterHierarchy(t *te bun, err := configadapter.ConvertToTestBundle(ctx, p.Config, m) require.NoError(t, err) - err = p.TestParameters.InsertParameterSet(ctx, storage.NewParameterSet("", "myps", - storage.ValueStrategy("my-second-param", "via_paramset"))) + myPS := storage.NewParameterSet("", "myps") + myPS.SetStrategy("my-second-param", secrets.HardCodedValueStrategy("via_paramset")) + err = p.TestParameters.InsertParameterSet(ctx, myPS) require.NoError(t, err, "Create my-second-param parameter set failed") makeOpts := func() InstallOptions { @@ -443,14 +444,15 @@ func TestPorter_applyActionOptionsToInstallation_sanitizesParameters(t *testing. opts.Params = []string{nonsensitiveParamName + "=" + nonsensitiveParamValue, sensitiveParamName + "=" + sensitiveParamValue} i := storage.NewInstallation("", bun.Name) + i.ID = "INSTALLATIONID" err = p.applyActionOptionsToInstallation(ctx, opts, &i) require.NoError(t, err) - require.Len(t, i.Parameters.Parameters, 2) + require.Equal(t, 2, i.Parameters.Len()) // there should be no sensitive value on installation record - for _, param := range i.Parameters.Parameters { - if param.Name == sensitiveParamName { + for paramName, param := range i.Parameters.Iterate() { + if paramName == sensitiveParamName { require.Equal(t, param.Source.Strategy, secrets.SourceSecret) require.NotEqual(t, param.Source.Hint, sensitiveParamValue) continue @@ -470,11 +472,12 @@ func TestPorter_applyActionOptionsToInstallation_sanitizesParameters(t *testing. 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.Hint) - require.Equal(t, "my-second-param", i.Parameters.Parameters[1].Name) - require.Equal(t, "secret", i.Parameters.Parameters[1].Source.Strategy) + wantParameters := storage.NewParameterSourceMap() + wantParameters.Set("my-first-param", storage.ParameterSource{ + Source: secrets.Source{Strategy: "value", Hint: "1"}}) + wantParameters.Set("my-second-param", storage.ParameterSource{ + Source: secrets.Source{Strategy: "secret", Hint: "INSTALLATIONID-my-second-param"}}) + require.Equal(t, wantParameters, i.Parameters.Parameters) } // When the installation has been used before with a parameter value @@ -506,27 +509,21 @@ func TestPorter_applyActionOptionsToInstallation_PreservesExistingParams(t *test 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"), - ) + i.ID = "INSTALLATIONID" + i.Parameters = storage.NewParameterSet("", "internal-ps") + i.Parameters.SetStrategy("my-first-param", secrets.HardCodedValueStrategy("1")) + i.Parameters.SetStrategy("my-second-param", secrets.HardCodedValueStrategy("2")) err = p.applyActionOptionsToInstallation(ctx, opts, &i) require.NoError(t, err) - require.Len(t, i.Parameters.Parameters, 2) + require.Equal(t, 2, i.Parameters.Len()) // 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, "value", i.Parameters.Parameters[0].Source.Strategy, "my-first-param isn't sensitive and can be stored in a hard-coded value") - require.Equal(t, "my-second-param", i.Parameters.Parameters[1].Name) - require.Equal(t, "secret", i.Parameters.Parameters[1].Source.Strategy, "my-second-param should be stored on the installation using a secret since it's sensitive") - - // Check the values stored are correct - params, err := p.Parameters.ResolveAll(ctx, i.Parameters) - require.NoError(t, err, "Failed to resolve the installation parameters") - require.Equal(t, secrets.Set{ - "my-first-param": "3", // Should have used the override - "my-second-param": "2", // Should have kept the existing value from the last run - }, params, "Incorrect parameter values were persisted on the installationß") + wantParameters := storage.NewParameterSourceMap() + wantParameters.Set("my-first-param", storage.ParameterSource{ + Source: secrets.Source{Strategy: "value", Hint: "3"}, + }) + wantParameters.Set("my-second-param", storage.ParameterSource{ + Source: secrets.Source{Strategy: "secret", Hint: "INSTALLATIONID-my-second-param"}}) + require.Equal(t, wantParameters, i.Parameters.Parameters) } diff --git a/pkg/porter/list.go b/pkg/porter/list.go index a88109f75..939a8a77b 100644 --- a/pkg/porter/list.go +++ b/pkg/porter/list.go @@ -3,13 +3,13 @@ package porter import ( "context" "fmt" + "get.porter.sh/porter/pkg/secrets" "sort" "strings" "time" "get.porter.sh/porter/pkg/cnab" "get.porter.sh/porter/pkg/printer" - "get.porter.sh/porter/pkg/secrets" "get.porter.sh/porter/pkg/storage" "get.porter.sh/porter/pkg/tracing" dtprinter "github.com/carolynvs/datetime-printer" @@ -176,19 +176,19 @@ func (d DisplayInstallation) ConvertToInstallation() (storage.Installation, erro return i, nil } -// ConvertParamToSet converts a Parameters into a internal ParameterSet. +// ConvertParamToSet converts a Parameters into an internal ParameterSet. func (d DisplayInstallation) ConvertParamToSet() (storage.ParameterSet, error) { - strategies := make([]secrets.SourceMap, 0, len(d.Parameters)) + pset := storage.NewInternalParameterSet(d.Namespace, d.Name) for name, value := range d.Parameters { stringVal, err := cnab.WriteParameterToString(name, value) if err != nil { return storage.ParameterSet{}, err } - strategies = append(strategies, storage.ValueStrategy(name, stringVal)) + pset.SetStrategy(name, secrets.HardCodedValueStrategy(stringVal)) } - return storage.NewInternalParameterSet(d.Namespace, d.Name, strategies...), nil + return pset, nil } // TODO(carolynvs): be consistent with sorting results from list, either keep the default sort by name diff --git a/pkg/porter/list_test.go b/pkg/porter/list_test.go index 87fbe385d..a84e93162 100644 --- a/pkg/porter/list_test.go +++ b/pkg/porter/list_test.go @@ -59,10 +59,11 @@ func TestPorter_ListInstallations(t *testing.T) { p := NewTestPorter(t) defer p.Close() + // Define a parameter that is stored in a secret, list should not retrieve it i1 := storage.NewInstallation("", "shared-mysql") - i1.Parameters.Parameters = []secrets.SourceMap{ // Define a parameter that is stored in a secret, list should not retrieve it - {Name: "password", Source: secrets.Source{Strategy: "secret", Hint: "mypassword"}}, - } + i1.Parameters.SetStrategy("password", secrets.Source{ + Strategy: "secret", + Hint: "mypassword"}) i1.Status.RunID = "10" // Add a run but don't populate the data for it, list should not retrieve it p.TestInstallations.CreateInstallation(i1) @@ -138,15 +139,10 @@ func TestDisplayInstallation_ConvertToInstallation(t *testing.T) { require.Equal(t, convertedInstallation.ParameterSets, i.ParameterSets, "invalid parameter set") require.Equal(t, i.Parameters.String(), convertedInstallation.Parameters.String(), "invalid parameters name") - require.Equal(t, len(i.Parameters.Parameters), len(convertedInstallation.Parameters.Parameters)) - - parametersMap := make(map[string]secrets.SourceMap, len(i.Parameters.Parameters)) - for _, param := range i.Parameters.Parameters { - parametersMap[param.Name] = param - } + require.Equal(t, i.Parameters.Len(), convertedInstallation.Parameters.Len()) - for _, param := range convertedInstallation.Parameters.Parameters { - expected := parametersMap[param.Name] + for paramName, param := range convertedInstallation.Parameters.Iterate() { + expected, _ := i.Parameters.Get(paramName) require.Equal(t, expected.ResolvedValue, param.ResolvedValue) expectedSource, err := expected.Source.MarshalJSON() require.NoError(t, err) diff --git a/pkg/porter/parameters.go b/pkg/porter/parameters.go index 8fcf5c299..a0fff0d8e 100644 --- a/pkg/porter/parameters.go +++ b/pkg/porter/parameters.go @@ -11,9 +11,8 @@ import ( "strings" "time" - depsv1 "get.porter.sh/porter/pkg/cnab/dependencies/v1" - "get.porter.sh/porter/pkg/cnab" + depsv1 "get.porter.sh/porter/pkg/cnab/dependencies/v1" "get.porter.sh/porter/pkg/editor" "get.porter.sh/porter/pkg/encoding" "get.porter.sh/porter/pkg/generator" @@ -271,8 +270,8 @@ func (p *Porter) ShowParameter(ctx context.Context, opts ParameterShowOptions) e var rows [][]string // Iterate through all ParameterStrategies and add to rows - for _, pset := range paramSet.Parameters { - rows = append(rows, []string{pset.Name, pset.Source.Hint, pset.Source.Strategy}) + for paramName, param := range paramSet.Iterate() { + rows = append(rows, []string{paramName, param.Source.Hint, param.Source.Strategy}) } // Build and configure our tablewriter @@ -388,15 +387,13 @@ func (p *Porter) loadParameterSets(ctx context.Context, bun cnab.ExtendedBundle, } if bun.IsFileType(paramSchema) { - for i, param := range pset.Parameters { - if param.Name == paramName { - // Pass through value (filepath) directly to resolvedParameters - resolvedParameters[param.Name] = param.Source.Hint - // Eliminate this param from pset to prevent its resolution by - // the cnab-go library, which doesn't support this parameter type - pset.Parameters[i] = pset.Parameters[len(pset.Parameters)-1] - pset.Parameters = pset.Parameters[:len(pset.Parameters)-1] - } + param, ok := pset.Get(paramName) + if ok { + // Pass through value (filepath) directly to resolvedParameters + resolvedParameters[paramName] = param.Source.Hint + // Eliminate this param from pset to prevent its resolution by + // the cnab-go library, which doesn't support this parameter type + pset.Remove(paramName) } } } @@ -836,21 +833,8 @@ func (p *Porter) applyActionOptionsToInstallation(ctx context.Context, ba Bundle } // Replace previous value if present - replaced := false - paramStrategy := storage.ValueStrategy(name, value) - for i, existingParam := range inst.Parameters.Parameters { - if existingParam.Name == name { - inst.Parameters.Parameters[i] = paramStrategy - replaced = true - } - } - if !replaced { - inst.Parameters.Parameters = append(inst.Parameters.Parameters, paramStrategy) - } + inst.Parameters.SetStrategy(name, secrets.HardCodedValueStrategy(value)) } - - // Keep the parameter overrides sorted, so that comparisons and general troubleshooting is easier - sort.Sort(inst.Parameters.Parameters) } // This contains resolved sensitive values, so only trace it in special dev builds (nothing is traced for release builds) span.SetSensitiveAttributes(tracing.ObjectAttribute("merged-installation-parameters", inst.Parameters.Parameters)) diff --git a/pkg/porter/parameters_test.go b/pkg/porter/parameters_test.go index 2fe713293..0f3e3f915 100644 --- a/pkg/porter/parameters_test.go +++ b/pkg/porter/parameters_test.go @@ -858,9 +858,13 @@ func TestPorter_ParametersApply(t *testing.T) { require.NoError(t, err, "Failed to retrieve applied parameter set") assert.Equal(t, "mypset", ps.Name, "unexpected parameter set name") - require.Len(t, ps.Parameters, 1, "expected 1 parameter in the set") - assert.Equal(t, "foo", ps.Parameters[0].Name, "expected the foo parameter mapping defined") - assert.Equal(t, "secret", ps.Parameters[0].Source.Strategy, "expected the foo parameter mapping to come from a secret") - assert.Equal(t, "foo_secret", ps.Parameters[0].Source.Hint, "expected the foo parameter mapping to use foo_secret") + + wantParams := storage.NewParameterSourceMap() + wantParams.Set("foo", storage.ParameterSource{ + Source: secrets.Source{ + Strategy: "secret", + Hint: "foo_secret", + }}) + assert.Equal(t, wantParams, ps.Parameters, "unexpected parameter mappings defined") }) } diff --git a/pkg/porter/reconcile_test.go b/pkg/porter/reconcile_test.go index 426315029..c9d7d7950 100644 --- a/pkg/porter/reconcile_test.go +++ b/pkg/porter/reconcile_test.go @@ -2,13 +2,13 @@ package porter import ( "context" + "get.porter.sh/porter/pkg/secrets" "path/filepath" "testing" "time" "get.porter.sh/porter/pkg/cnab" "get.porter.sh/porter/pkg/portercontext" - "get.porter.sh/porter/pkg/secrets" "get.porter.sh/porter/pkg/storage" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -59,24 +59,18 @@ func TestPorter_IsInstallationInSync(t *testing.T) { p := NewTestPorter(t) defer p.Close() - myps := storage.ParameterSet{ - ParameterSetSpec: storage.ParameterSetSpec{ - Name: "myps", - Parameters: []secrets.SourceMap{ - storage.ValueStrategy("my-second-param", "override"), - }, - }, - } + myps := storage.NewParameterSet("", "myps") + myps.SetStrategy("my-second-param", secrets.HardCodedValueStrategy("override")) + err := p.Parameters.InsertParameterSet(ctx, myps) require.NoError(t, err) i := storage.NewInstallation("", "mybuns") i.ParameterSets = []string{"myps"} i.Status.Installed = &now - run := storage.Run{ - // Use the default values from the bundle.json so that we don't trigger reconciliation - Parameters: storage.NewInternalParameterSet(i.Namespace, i.Name, storage.ValueStrategy("my-second-param", "override")), - } + run := i.NewRun(cnab.ActionInstall, bun) + run.Parameters.SetStrategy("my-second-param", secrets.HardCodedValueStrategy("override")) + upgradeOpts := NewUpgradeOptions() upgradeOpts.bundleRef = &cnab.BundleReference{Definition: bun} require.NoError(t, p.applyActionOptionsToInstallation(ctx, upgradeOpts, &i)) @@ -115,9 +109,9 @@ func TestPorter_IsInstallationInSync(t *testing.T) { i := storage.NewInstallation("", "mybuns") i.Status.Installed = &now - run := storage.Run{ - Parameters: storage.NewInternalParameterSet(i.Namespace, i.Name, storage.ValueStrategy("my-second-param", "newvalue")), - } + run := i.NewRun(cnab.ActionInstall, bun) + run.Parameters.SetStrategy("my-second-param", secrets.HardCodedValueStrategy("new value")) + upgradeOpts := NewUpgradeOptions() upgradeOpts.bundleRef = &cnab.BundleReference{Definition: bun} require.NoError(t, p.applyActionOptionsToInstallation(ctx, upgradeOpts, &i)) @@ -137,11 +131,10 @@ func TestPorter_IsInstallationInSync(t *testing.T) { i := storage.NewInstallation("", "mybuns") i.Status.Installed = &now i.CredentialSets = []string{"newcreds"} - run := storage.Run{ - CredentialSets: []string{"oldcreds"}, - // Use the default values from the bundle.json so they don't trigger the reconciliation - Parameters: storage.NewInternalParameterSet(i.Namespace, i.Name, storage.ValueStrategy("my-second-param", "spring-music-demo")), - } + run := i.NewRun(cnab.ActionInstall, bun) + run.CredentialSets = []string{"oldcreds"} + run.Parameters.SetStrategy("my-second-param", secrets.HardCodedValueStrategy("spring-music-demo")) + upgradeOpts := NewUpgradeOptions() upgradeOpts.bundleRef = &cnab.BundleReference{Definition: bun} require.NoError(t, p.applyActionOptionsToInstallation(ctx, upgradeOpts, &i)) diff --git a/pkg/porter/show_test.go b/pkg/porter/show_test.go index e6ce69c7e..d0e678408 100644 --- a/pkg/porter/show_test.go +++ b/pkg/porter/show_test.go @@ -92,11 +92,10 @@ func TestPorter_ShowInstallationWithBundle(t *testing.T) { "io.cnab/app": "wordpress", "io.cnab/appVersion": "v1.2.3", } - params := []secrets.SourceMap{ - {Name: "logLevel", Source: secrets.Source{Hint: "3"}, ResolvedValue: "3"}, - secrets.SourceMap{Name: "secretString", Source: secrets.Source{Strategy: "secretString", Hint: "foo"}, ResolvedValue: "foo"}, - } - i.Parameters = i.NewInternalParameterSet(params...) + + i.Parameters = i.NewInternalParameterSet() + i.Parameters.SetStrategy("logLevel", secrets.HardCodedValueStrategy("3")) + i.Parameters.SetStrategy("secretString", secrets.Source{Strategy: secrets.SourceSecret, Hint: "foo"}) i.ParameterSets = []string{"dev-env"} @@ -108,17 +107,14 @@ func TestPorter_ShowInstallationWithBundle(t *testing.T) { r.BundleReference = tc.ref r.BundleDigest = "sha256:88d68ef0bdb9cedc6da3a8e341a33e5d2f8bb19d0cf7ec3f1060d3f9eb73cae9" - r.ParameterOverrides = i.NewInternalParameterSet( - storage.ValueStrategy("logLevel", "3"), - storage.ValueStrategy("secretString", "foo"), - ) + r.ParameterOverrides = i.NewInternalParameterSet() + r.ParameterOverrides.Set("logLevel", secrets.HardCodedValue("3")) + r.ParameterOverrides.Set("secretString", secrets.HardCodedValue("foo")) - r.Parameters = i.NewInternalParameterSet( - []secrets.SourceMap{ - storage.ValueStrategy("logLevel", "3"), - storage.ValueStrategy("token", "top-secret"), - storage.ValueStrategy("secretString", "foo"), - }...) + r.Parameters = i.NewInternalParameterSet() + r.Parameters.Set("logLevel", secrets.HardCodedValue("3")) + r.Parameters.Set("token", secrets.HardCodedValue("top-secret")) + r.Parameters.Set("secretString", secrets.HardCodedValue("foo")) r.ParameterSets = []string{"dev-env"} r.ParameterOverrides.Parameters = p.SanitizeParameters(r.ParameterOverrides.Parameters, r.ID, bun) @@ -205,11 +201,10 @@ func TestPorter_ShowInstallationWithoutRecordedRun(t *testing.T) { "io.cnab/app": "wordpress", "io.cnab/appVersion": "v1.2.3", } - params := []secrets.SourceMap{ - {Name: "logLevel", Source: secrets.Source{Hint: "3"}, ResolvedValue: "3"}, - secrets.SourceMap{Name: "secretString", Source: secrets.Source{Strategy: "secretString", Hint: "foo"}, ResolvedValue: "foo"}, - } - i.Parameters = i.NewInternalParameterSet(params...) + + i.Parameters = i.NewInternalParameterSet() + i.Parameters.SetStrategy("logLevel", secrets.HardCodedValueStrategy("3")) + i.Parameters.SetStrategy("secretString", secrets.HardCodedValueStrategy("foo")) i.ParameterSets = []string{"dev-env"} diff --git a/pkg/porter/testdata/credentials/kool-kreds.json b/pkg/porter/testdata/credentials/kool-kreds.json index 997e887af..f3d7df091 100644 --- a/pkg/porter/testdata/credentials/kool-kreds.json +++ b/pkg/porter/testdata/credentials/kool-kreds.json @@ -5,21 +5,21 @@ "name": "kool-kreds", "credentials": [ { - "name": "kool-config", + "name": "kool-cmd", "source": { - "path": "/path/to/kool-config" + "command": "echo 'kool'" } }, { - "name": "kool-envvar", + "name": "kool-config", "source": { - "env": "KOOL_ENV_VAR" + "path": "/path/to/kool-config" } }, { - "name": "kool-cmd", + "name": "kool-envvar", "source": { - "command": "echo 'kool'" + "env": "KOOL_ENV_VAR" } }, { diff --git a/pkg/porter/testdata/credentials/kool-kreds.txt b/pkg/porter/testdata/credentials/kool-kreds.txt index ebe168f74..e23b3fb76 100644 --- a/pkg/porter/testdata/credentials/kool-kreds.txt +++ b/pkg/porter/testdata/credentials/kool-kreds.txt @@ -6,7 +6,7 @@ Modified: 2019-06-24 -------------------------------------------------- Name Local Source Source Type -------------------------------------------------- + kool-cmd echo 'kool' command kool-config /path/to/kool-config path kool-envvar KOOL_ENV_VAR env - kool-cmd echo 'kool' command kool-val kool value diff --git a/pkg/porter/testdata/credentials/kool-kreds.yaml b/pkg/porter/testdata/credentials/kool-kreds.yaml index 59fa51b3b..6706c8706 100644 --- a/pkg/porter/testdata/credentials/kool-kreds.yaml +++ b/pkg/porter/testdata/credentials/kool-kreds.yaml @@ -3,15 +3,15 @@ schemaVersion: 1.0.1 namespace: dev name: kool-kreds credentials: + - name: kool-cmd + source: + command: echo 'kool' - name: kool-config source: path: /path/to/kool-config - name: kool-envvar source: env: KOOL_ENV_VAR - - name: kool-cmd - source: - command: echo 'kool' - name: kool-val source: value: kool diff --git a/pkg/porter/testdata/credentials/show-output.json b/pkg/porter/testdata/credentials/show-output.json index ec73a0826..9cd2c3c2b 100644 --- a/pkg/porter/testdata/credentials/show-output.json +++ b/pkg/porter/testdata/credentials/show-output.json @@ -6,21 +6,21 @@ "name": "kool-kreds", "credentials": [ { - "name": "kool-config", + "name": "kool-cmd", "source": { - "path": "/path/to/kool-config" + "command": "echo 'kool'" } }, { - "name": "kool-envvar", + "name": "kool-config", "source": { - "env": "KOOL_ENV_VAR" + "path": "/path/to/kool-config" } }, { - "name": "kool-cmd", + "name": "kool-envvar", "source": { - "command": "echo 'kool'" + "env": "KOOL_ENV_VAR" } }, { diff --git a/pkg/porter/testdata/credentials/show-output.yaml b/pkg/porter/testdata/credentials/show-output.yaml index 292b13314..86db08b0d 100644 --- a/pkg/porter/testdata/credentials/show-output.yaml +++ b/pkg/porter/testdata/credentials/show-output.yaml @@ -3,15 +3,15 @@ namespace: dev name: kool-kreds credentials: + - name: kool-cmd + source: + command: echo 'kool' - name: kool-config source: path: /path/to/kool-config - name: kool-envvar source: env: KOOL_ENV_VAR - - name: kool-cmd - source: - command: echo 'kool' - name: kool-val source: value: kool diff --git a/pkg/portercontext/helpers.go b/pkg/portercontext/helpers.go index 3e8e14097..ae810d42c 100644 --- a/pkg/portercontext/helpers.go +++ b/pkg/portercontext/helpers.go @@ -314,6 +314,7 @@ func (c *TestContext) hasChild(dir string, childName string) (string, bool) { // When they are different and PORTER_UPDATE_TEST_FILES is true, the file is updated to match // the new test output. func (c *TestContext) CompareGoldenFile(goldenFile string, got string) { + c.T.Helper() test.CompareGoldenFile(c.T, goldenFile, got) } diff --git a/pkg/secrets/map.go b/pkg/secrets/map.go new file mode 100644 index 000000000..37a277779 --- /dev/null +++ b/pkg/secrets/map.go @@ -0,0 +1,115 @@ +package secrets + +import ( + "encoding/json" + + "get.porter.sh/porter/pkg/encoding" + "gopkg.in/yaml.v3" +) + +type Map struct { + *encoding.ArrayMap[ValueMapping, NamedValueMapping] +} + +func NewMap() Map { + return Map{ + ArrayMap: &encoding.ArrayMap[ValueMapping, NamedValueMapping]{}, + } +} + +func MakeMap(len int) Map { + items := encoding.MakeArrayMap[ValueMapping, NamedValueMapping](len) + return Map{ + ArrayMap: &items, + } +} + +func (m Map) Merge(overrides Map) Map { + result := m.ArrayMap.Merge(overrides.ArrayMap) + return Map{ArrayMap: result} +} + +func (m Map) ToResolvedValues() map[string]string { + values := make(map[string]string, m.Len()) + for k, v := range m.ItemsUnsafe() { + values[k] = v.ResolvedValue + } + return values +} + +func (m *Map) UnmarshalJSON(b []byte) error { + // Ensure that ArrayMap is not nil before its custom UnmarshalJson is called + // Otherwise it causes a panic + if m.ArrayMap == nil { + m.ArrayMap = &encoding.ArrayMap[ValueMapping, NamedValueMapping]{} + } + + // Cheap trick to unmarshal without re-triggering this UnmarshalJson call + type RawMap Map + return json.Unmarshal(b, (*RawMap)(m)) +} + +func (m *Map) UnmarshalYAML(value *yaml.Node) error { + // Ensure that ArrayMap is not nil before its custom UnmarshalYAML is called + // Otherwise it causes a panic + if m.ArrayMap == nil { + m.ArrayMap = &encoding.ArrayMap[ValueMapping, NamedValueMapping]{} + } + + // Cheap trick to unmarshal without re-triggering this UnmarshalYAML call + type RawMap Map + return value.Decode((*RawMap)(m)) +} + +var _ encoding.MapElement = ValueMapping{} + +// ValueMapping maps from a parameter or credential name to a source strategy for resolving its value. +type ValueMapping struct { + // Source defines a strategy for resolving a value from the specified source. + Source Source `json:"source,omitempty" yaml:"source,omitempty"` + + // ResolvedValue holds the resolved parameter or credential value. + // When a parameter or credential is resolved, it is loaded into this field. In all + // other cases, it is empty. This field is omitted during serialization. + ResolvedValue string `json:"-" yaml:"-"` +} + +func (v ValueMapping) ToArrayEntry(key string) encoding.ArrayElement { + return NamedValueMapping{ + Name: key, + Source: v.Source, + } +} + +var _ encoding.ArrayElement = NamedValueMapping{} + +// NamedValueMapping is the representation of a ValueMapping in an array, +// We use it to unmarshal the yaml, and then convert it into our internal representation +// where the ValueMapping is in a Go map instead of an array. +type NamedValueMapping struct { + // Name is the name of the parameter or credential. + Name string `json:"name" yaml:"name"` + + // Source defines a strategy for resolving a value from the specified source. + Source Source `json:"source" yaml:"source"` + + // ResolvedValue holds the resolved parameter or credential value. + // When a parameter or credential is resolved, it is loaded into this field. In all + // other cases, it is empty. This field is omitted during serialization. + ResolvedValue string `json:"-" yaml:"-"` +} + +func (r NamedValueMapping) ToValueMapping() ValueMapping { + return ValueMapping{ + Source: r.Source, + ResolvedValue: r.ResolvedValue, + } +} + +func (r NamedValueMapping) ToMapEntry() encoding.MapElement { + return r.ToValueMapping() +} + +func (r NamedValueMapping) GetKey() string { + return r.Name +} diff --git a/pkg/secrets/strategy.go b/pkg/secrets/strategy.go index 498328a48..6d290ee09 100644 --- a/pkg/secrets/strategy.go +++ b/pkg/secrets/strategy.go @@ -4,7 +4,7 @@ import ( "encoding/json" "errors" "fmt" - + "github.com/cnabio/cnab-go/secrets/host" "github.com/cnabio/cnab-go/valuesource" "gopkg.in/yaml.v3" ) @@ -13,20 +13,6 @@ import ( // This is the output of resolving a parameter or credential set file. type Set map[string]string -// Merge merges a second Set into the base. -// -// Duplicate names are not allow and will result in an -// error, this is the case even if the values are identical. -func (s Set) Merge(s2 Set) error { - for k, v := range s2 { - if _, ok := s[k]; ok { - return fmt.Errorf("ambiguous value resolution: %q is already present in base sets, cannot merge", k) - } - s[k] = v - } - return nil -} - // IsValid determines if the provided key (designating a name of a parameter // or credential) is included in the provided set func (s Set) IsValid(key string) bool { @@ -43,20 +29,6 @@ func (s Set) ToCNAB() valuesource.Set { return valuesource.Set(s) } -// SourceMap maps from a parameter or credential name to a source strategy for resolving its value. -type SourceMap struct { - // Name is the name of the parameter or credential. - Name string `json:"name" yaml:"name"` - - // Source defines a strategy for resolving a value from the specified source. - Source Source `json:"source,omitempty" yaml:"source,omitempty"` - - // ResolvedValue holds the resolved parameter or credential value. - // When a parameter or credential is resolved, it is loaded into this field. In all - // other cases, it is empty. This field is omitted during serialization. - ResolvedValue string `json:"-" yaml:"-"` -} - // Source specifies how to resolve a parameter or credential from an external // source. type Source struct { @@ -130,18 +102,19 @@ func (s Source) MarshalYAML() (interface{}, error) { return s.MarshalRaw(), nil } -type StrategyList []SourceMap - -func (l StrategyList) Less(i, j int) bool { - return l[i].Name < l[j].Name -} - -func (l StrategyList) Swap(i, j int) { - tmp := l[i] - l[i] = l[j] - l[j] = tmp +// HardCodedValue generates a hard-coded value source mapping that contains a resolved value. +func HardCodedValue(value string) ValueMapping { + return ValueMapping{ + Source: Source{ + Strategy: host.SourceValue, + Hint: value}, + ResolvedValue: value} } -func (l StrategyList) Len() int { - return len(l) +// HardCodedValueStrategy generates a hard-coded value strategy. +// TODO(carolyn): Remove name arg +func HardCodedValueStrategy(value string) Source { + return Source{ + Strategy: host.SourceValue, + Hint: value} } diff --git a/pkg/secrets/strategy_test.go b/pkg/secrets/strategy_test.go index 8e5478fe9..401841354 100644 --- a/pkg/secrets/strategy_test.go +++ b/pkg/secrets/strategy_test.go @@ -1,30 +1,96 @@ package secrets -import ( - "testing" +/* +func TestSourceMapList_ToResolvedValues(t *testing.T) { + l := Map{ + "bar": ValueMapping{ResolvedValue: "2"}, + "foo": ValueMapping{ResolvedValue: "1"}, + } - "github.com/stretchr/testify/assert" -) + want := map[string]string{ + "bar": "2", + "foo": "1", + } + got := l.ToResolvedValues() + assert.Equal(t, want, got) +} -func TestSet_Merge(t *testing.T) { - set := Set{ - "first": "first", - "second": "second", - "third": "third", +func TestSourceMapList_Merge(t *testing.T) { + set := Map{ + "first": ValueMapping{ + Source: Source{Strategy: "value", Hint: "base first"}, + ResolvedValue: "base first"}, + "second": ValueMapping{ + Source: Source{Strategy: "value", Hint: "base second"}, + ResolvedValue: "base second"}, + "third": ValueMapping{ + Source: Source{Strategy: "value", Hint: "base third"}, + ResolvedValue: "base third"}, } is := assert.New(t) - err := set.Merge(Set{}) - is.NoError(err) - is.Len(set, 3) - is.NotContains(set, "fourth") + result := set.Merge(Map{}) + is.Len(result, 3) + is.NotContains(result, "fourth", "fourth should not be present in the base set") + + wantFourth := ValueMapping{ + Source: Source{Strategy: "env", Hint: "FOURTH"}, + ResolvedValue: "new fourth"} + fourth := Map{"fourth": wantFourth} + result = set.Merge(fourth) + is.Len(result, 4) + gotFourth, ok := result["fourth"] + is.True(ok, "fourth should be present in the merged set") + is.Equal(wantFourth, gotFourth, "incorrect merged value for fourth") + + wantSecond := ValueMapping{ + Source: Source{Strategy: "env", Hint: "SECOND"}, + ResolvedValue: "override second"} + result = set.Merge(Map{"second": wantSecond}) + is.Len(result, 3) + gotSecond, ok := result["second"] + is.True(ok, "second should be present in the merged set") + is.Equal(wantSecond, gotSecond, "incorrect merged value for second") +} + +func TestSourceMapList_Unmarshal(t *testing.T) { + data, err := os.ReadFile("testdata/strategies.yaml") + require.NoError(t, err, "ReadFile failed") + + var l Map + err = yaml.Unmarshal(data, &l) + require.NoError(t, err, "Unmarshal failed") + + require.Len(t, l, 2, "unexpected number of strategies defined") + + gotLogLevel, ok := l["logLevel"] + require.True(t, ok, "logLevel was not defined") + wantLogLevel := ValueMapping{ + Source: Source{ + Strategy: "env", + Hint: "LOG_LEVEL", + }, + } + assert.Equal(t, wantLogLevel, gotLogLevel, "unexpected logLevel defined") + + gotPassword, ok := l["password"] + require.True(t, ok, "password was not defined") + wantPassword := ValueMapping{ + Source: Source{ + Strategy: "secret", + Hint: "my-password", + }, + } + assert.Equal(t, wantPassword, gotPassword, "unexpected password defined") +} - err = set.Merge(Set{"fourth": "fourth"}) - is.NoError(err) - is.Len(set, 4) - is.Contains(set, "fourth") +func TestSourceMapList_Unmarshal_DuplicateKeys(t *testing.T) { + data, err := os.ReadFile("testdata/duplicate-strategies.yaml") + require.NoError(t, err, "ReadFile failed") - err = set.Merge(Set{"second": "bis"}) - is.EqualError(err, `ambiguous value resolution: "second" is already present in base sets, cannot merge`) + var l Map + err = yaml.Unmarshal(data, &l) + require.ErrorContains(t, err, "cannot unmarshal source map: duplicate key found 'logLevel'") } +*/ diff --git a/pkg/secrets/testdata/duplicate-strategies.yaml b/pkg/secrets/testdata/duplicate-strategies.yaml new file mode 100644 index 000000000..fa8a93be7 --- /dev/null +++ b/pkg/secrets/testdata/duplicate-strategies.yaml @@ -0,0 +1,9 @@ +- name: logLevel + source: + env: LOG_LEVEL +- name: password + source: + secret: red-team-password +- name: logLevel + source: + value: 11 diff --git a/pkg/secrets/testdata/strategies.yaml b/pkg/secrets/testdata/strategies.yaml new file mode 100644 index 000000000..1504c421b --- /dev/null +++ b/pkg/secrets/testdata/strategies.yaml @@ -0,0 +1,6 @@ +- name: logLevel + source: + env: LOG_LEVEL +- name: password + source: + secret: my-password diff --git a/pkg/storage/credential_store.go b/pkg/storage/credential_store.go index 86e76535a..889d0699a 100644 --- a/pkg/storage/credential_store.go +++ b/pkg/storage/credential_store.go @@ -61,13 +61,13 @@ func (s CredentialStore) ResolveAll(ctx context.Context, creds CredentialSet) (s resolvedCreds := make(secrets.Set) var resolveErrors error - for _, cred := range creds.Credentials { + for credName, cred := range creds.Iterate() { value, err := s.Secrets.Resolve(ctx, cred.Source.Strategy, cred.Source.Hint) if err != nil { - resolveErrors = multierror.Append(resolveErrors, fmt.Errorf("unable to resolve credential %s.%s from %s %s: %w", creds.Name, cred.Name, cred.Source.Strategy, cred.Source.Hint, err)) + resolveErrors = multierror.Append(resolveErrors, fmt.Errorf("unable to resolve credential %s.%s from %s %s: %w", creds.Name, credName, cred.Source.Strategy, cred.Source.Hint, err)) } - resolvedCreds[cred.Name] = value + resolvedCreds[credName] = value } return resolvedCreds, resolveErrors @@ -77,10 +77,14 @@ func (s CredentialStore) Validate(ctx context.Context, creds CredentialSet) erro validSources := []string{secrets.SourceSecret, host.SourceValue, host.SourceEnv, host.SourcePath, host.SourceCommand} var errors error - for _, cs := range creds.Credentials { + for credName, cred := range creds.Iterate() { valid := false + if credName == "" { + errors = multierror.Append(errors, fmt.Errorf("all credential set sources must have a credential name defined")) + } + for _, validSource := range validSources { - if cs.Source.Strategy == validSource { + if cred.Source.Strategy == validSource { valid = true break } @@ -88,7 +92,7 @@ func (s CredentialStore) Validate(ctx context.Context, creds CredentialSet) erro if !valid { errors = multierror.Append(errors, fmt.Errorf( "%s is not a valid source. Valid sources are: %s", - cs.Source.Strategy, + cred.Source.Strategy, strings.Join(validSources, ", "), )) } diff --git a/pkg/storage/credential_store_test.go b/pkg/storage/credential_store_test.go index fd1a8dbf7..1bef291e2 100644 --- a/pkg/storage/credential_store_test.go +++ b/pkg/storage/credential_store_test.go @@ -10,10 +10,10 @@ import ( ) func TestCredentialStorage_CRUD(t *testing.T) { - cs := NewCredentialSet("dev", "sekrets", secrets.SourceMap{ - Name: "password", Source: secrets.Source{ - Strategy: "secret", - Hint: "dbPassword"}}) + cs := NewCredentialSet("dev", "sekrets") + cs.Set("password", CredentialSource{Source: secrets.Source{ + Strategy: "secret", + Hint: "dbPassword"}}) cp := NewTestCredentialProvider(t) defer cp.Close() @@ -34,21 +34,19 @@ func TestCredentialStorage_CRUD(t *testing.T) { require.NoError(t, err) require.Len(t, creds, 1, "expected 1 credential set defined in all namespaces") - cs.Credentials = append(cs.Credentials, secrets.SourceMap{ - Name: "token", Source: secrets.Source{ - Strategy: "secret", - Hint: "github-token", - }, + cs.SetStrategy("token", secrets.Source{ + Strategy: "secret", + Hint: "github-token", }) require.NoError(t, cp.UpdateCredentialSet(context.Background(), cs)) cs, err = cp.GetCredentialSet(context.Background(), cs.Namespace, cs.Name) require.NoError(t, err) - assert.Len(t, cs.Credentials, 2) + assert.Equal(t, 2, cs.Len()) - cs2 := NewCredentialSet("dev", "sekrets-2", secrets.SourceMap{ - Name: "password-2", Source: secrets.Source{ - Strategy: "secret-2", - Hint: "dbPassword-2"}}) + cs2 := NewCredentialSet("dev", "sekrets-2") + cs2.SetStrategy("password-2", secrets.Source{ + Strategy: "secret-2", + Hint: "dbPassword-2"}) require.NoError(t, cp.InsertCredentialSet(context.Background(), cs2)) creds, err = cp.ListCredentialSets(context.Background(), ListOptions{Namespace: "dev", Skip: 1}) @@ -73,19 +71,15 @@ func TestCredentialStorage_CRUD(t *testing.T) { func TestCredentialStorage_Validate_GoodSources(t *testing.T) { s := CredentialStore{} - testCreds := NewCredentialSet("dev", "mycreds", - secrets.SourceMap{ - Source: secrets.Source{ - Strategy: "env", - Hint: "SOME_ENV", - }, - }, - secrets.SourceMap{ - Source: secrets.Source{ - Strategy: "value", - Hint: "somevalue", - }, - }) + testCreds := NewCredentialSet("dev", "mycreds") + testCreds.SetStrategy("env", secrets.Source{ + Strategy: "env", + Hint: "SOME_ENV", + }) + testCreds.SetStrategy("val", secrets.Source{ + Strategy: "value", + Hint: "somevalue", + }) err := s.Validate(context.Background(), testCreds) require.NoError(t, err, "Validate did not return errors") @@ -93,20 +87,15 @@ func TestCredentialStorage_Validate_GoodSources(t *testing.T) { func TestCredentialStorage_Validate_BadSources(t *testing.T) { s := CredentialStore{} - testCreds := NewCredentialSet("dev", "mycreds", - secrets.SourceMap{ - Source: secrets.Source{ - Strategy: "wrongthing", - Hint: "SOME_ENV", - }, - }, - secrets.SourceMap{ - Source: secrets.Source{ - Strategy: "anotherwrongthing", - Hint: "somevalue", - }, - }, - ) + testCreds := NewCredentialSet("dev", "mycreds") + testCreds.SetStrategy("env", secrets.Source{ + Strategy: "wrongthing", + Hint: "SOME_ENV", + }) + testCreds.SetStrategy("val", secrets.Source{ + Strategy: "anotherwrongthing", + Hint: "somevalue", + }) err := s.Validate(context.Background(), testCreds) require.Error(t, err, "Validate returned errors") diff --git a/pkg/storage/credentialset.go b/pkg/storage/credentialset.go index 651fd5252..72a962299 100644 --- a/pkg/storage/credentialset.go +++ b/pkg/storage/credentialset.go @@ -50,9 +50,16 @@ type CredentialSetSpec struct { Labels map[string]string `json:"labels,omitempty" yaml:"labels,omitempty" toml:"labels,omitempty"` // Credentials is a list of credential resolution strategies. - Credentials secrets.StrategyList `json:"credentials" yaml:"credentials" toml:"credentials"` + Credentials CredentialSourceMap `json:"credentials" yaml:"credentials" toml:"credentials"` } +type CredentialSourceMap = secrets.Map +type CredentialSource = secrets.ValueMapping +type NamedCredentialSource = secrets.NamedValueMapping + +var MakeCredentialSourceMap = secrets.MakeMap +var NewCredentialSourceMap = secrets.NewMap + // CredentialSetStatus contains additional status metadata that has been set by Porter. type CredentialSetStatus struct { // Created timestamp. @@ -63,7 +70,7 @@ type CredentialSetStatus struct { } // NewCredentialSet creates a new CredentialSet with the required fields initialized. -func NewCredentialSet(namespace string, name string, creds ...secrets.SourceMap) CredentialSet { +func NewCredentialSet(namespace string, name string) CredentialSet { now := time.Now() cs := CredentialSet{ CredentialSetSpec: CredentialSetSpec{ @@ -71,7 +78,7 @@ func NewCredentialSet(namespace string, name string, creds ...secrets.SourceMap) SchemaVersion: DefaultCredentialSetSchemaVersion, Name: name, Namespace: namespace, - Credentials: creds, + Credentials: NewCredentialSourceMap(), }, Status: CredentialSetStatus{ Created: now, @@ -123,6 +130,34 @@ func (s CredentialSet) String() string { return fmt.Sprintf("%s/%s", s.Namespace, s.Name) } +func (s CredentialSet) Iterate() map[string]CredentialSource { + return s.Credentials.Items() +} + +func (s CredentialSet) IterateSorted() []NamedCredentialSource { + return s.Credentials.ItemsSorted() +} + +func (s CredentialSet) SetStrategy(key string, source secrets.Source) { + s.Credentials.Set(key, CredentialSource{Source: source}) +} + +func (s CredentialSet) Set(key string, source CredentialSource) { + s.Credentials.Set(key, source) +} + +func (s CredentialSet) Get(key string) (CredentialSource, bool) { + return s.Credentials.Get(key) +} + +func (s CredentialSet) Remove(key string) { + s.Credentials.Remove(key) +} + +func (s CredentialSet) Len() int { + return s.Credentials.Len() +} + // Validate compares the given credentials with the spec. // // This will result in an error only when the following conditions are true: diff --git a/pkg/storage/credentialset_test.go b/pkg/storage/credentialset_test.go index 76fa008b6..f91724a8f 100644 --- a/pkg/storage/credentialset_test.go +++ b/pkg/storage/credentialset_test.go @@ -17,12 +17,10 @@ import ( ) func TestNewCredentialSet(t *testing.T) { - cs := NewCredentialSet("dev", "mycreds", secrets.SourceMap{ - Name: "password", - Source: secrets.Source{ - Strategy: "env", - Hint: "MY_PASSWORD", - }, + cs := NewCredentialSet("dev", "mycreds") + cs.SetStrategy("password", secrets.Source{ + Strategy: "env", + Hint: "MY_PASSWORD", }) assert.Equal(t, "mycreds", cs.Name, "Name was not set") @@ -32,7 +30,7 @@ func TestNewCredentialSet(t *testing.T) { assert.Equal(t, cs.Status.Created, cs.Status.Modified, "Created and Modified should have the same timestamp") assert.Equal(t, SchemaTypeCredentialSet, cs.SchemaType, "incorrect SchemaType") assert.Equal(t, DefaultCredentialSetSchemaVersion, cs.SchemaVersion, "incorrect SchemaVersion") - assert.Len(t, cs.Credentials, 1, "Credentials should be initialized with 1 value") + assert.Equal(t, 1, cs.Len(), "Credentials should be initialized with 1 value") } func TestValidate(t *testing.T) { @@ -152,21 +150,17 @@ func TestMarshal(t *testing.T) { SchemaVersion: "schemaVersion", Namespace: "namespace", Name: "name", - Credentials: []secrets.SourceMap{ - { - Name: "cred1", - Source: secrets.Source{ - Strategy: "secret", - Hint: "mysecret", - }, - }, - }, + Credentials: NewCredentialSourceMap(), }, Status: CredentialSetStatus{ Created: now, Modified: now, }, } + orig.SetStrategy("cred1", secrets.Source{ + Strategy: "secret", + Hint: "mysecret", + }) formats := []string{"json", "yaml"} for _, format := range formats { diff --git a/pkg/storage/installation.go b/pkg/storage/installation.go index 5d327f399..a66391347 100644 --- a/pkg/storage/installation.go +++ b/pkg/storage/installation.go @@ -10,7 +10,6 @@ import ( "get.porter.sh/porter/pkg/cnab" "get.porter.sh/porter/pkg/schema" - "get.porter.sh/porter/pkg/secrets" "get.porter.sh/porter/pkg/tracing" "github.com/Masterminds/semver/v3" "github.com/opencontainers/go-digest" @@ -105,12 +104,12 @@ func (i Installation) NewRun(action string, b cnab.ExtendedBundle) Run { // Copy over relevant overrides from the installation to the run // An installation may have an overridden parameter that doesn't apply to this current action run.ParameterOverrides = NewInternalParameterSet(i.Namespace, i.Name) - for _, p := range i.Parameters.Parameters { - if parmDef, ok := b.Parameters[p.Name]; ok { + for paramName, p := range i.Parameters.Iterate() { + if parmDef, ok := b.Parameters[paramName]; ok { if !parmDef.AppliesTo(action) { continue } - run.ParameterOverrides.Parameters = append(run.ParameterOverrides.Parameters, p) + run.ParameterOverrides.Set(paramName, p) } } @@ -223,8 +222,8 @@ func (i *Installation) SetLabel(key string, value string) { // NewInternalParameterSet creates a new ParameterSet that's used to store // parameter overrides with the required fields initialized. -func (i Installation) NewInternalParameterSet(params ...secrets.SourceMap) ParameterSet { - return NewInternalParameterSet(i.Namespace, i.ID, params...) +func (i Installation) NewInternalParameterSet() ParameterSet { + return NewInternalParameterSet(i.Namespace, i.ID) } func (i Installation) AddToTrace(ctx context.Context) { diff --git a/pkg/storage/migrations/migration.go b/pkg/storage/migrations/migration.go index adb82aff9..2c39ffda7 100644 --- a/pkg/storage/migrations/migration.go +++ b/pkg/storage/migrations/migration.go @@ -310,13 +310,13 @@ func convertClaimToRun(inst storage.Installation, data []byte) (storage.Run, err return storage.Run{}, fmt.Errorf("error parsing claim record: %w", err) } - params := make([]secrets.SourceMap, 0, len(src.Parameters)) + pset := storage.NewInternalParameterSet(inst.Namespace, src.ID) for k, v := range src.Parameters { stringVal, err := cnab.WriteParameterToString(k, v) if err != nil { return storage.Run{}, err } - params = append(params, storage.ValueStrategy(k, stringVal)) + pset.SetStrategy(k, secrets.HardCodedValueStrategy(stringVal)) } dest := storage.Run{ @@ -330,7 +330,7 @@ func convertClaimToRun(inst storage.Installation, data []byte) (storage.Run, err Bundle: src.Bundle, BundleReference: src.BundleReference, BundleDigest: "", // We didn't track digest before v1 - Parameters: storage.NewInternalParameterSet(inst.Namespace, src.ID, params...), + Parameters: pset, Custom: src.Custom, } @@ -505,7 +505,6 @@ func convertCredentialSet(namespace string, data []byte) (storage.CredentialSet, SchemaVersion: storage.DefaultCredentialSetSchemaVersion, Namespace: namespace, Name: src.Name, - Credentials: make([]secrets.SourceMap, len(src.Credentials)), }, Status: storage.CredentialSetStatus{ Created: src.Created, @@ -513,14 +512,15 @@ func convertCredentialSet(namespace string, data []byte) (storage.CredentialSet, }, } - for i, cred := range src.Credentials { - dest.CredentialSetSpec.Credentials[i] = secrets.SourceMap{ - Name: cred.Name, + dest.Credentials = storage.MakeCredentialSourceMap(len(src.Credentials)) + for _, srcCred := range src.Credentials { + destCred := storage.CredentialSource{ Source: secrets.Source{ - Strategy: cred.Source.Key, - Hint: cred.Source.Value, + Strategy: srcCred.Source.Key, + Hint: srcCred.Source.Value, }, } + dest.Credentials.Set(srcCred.Name, destCred) } return dest, nil @@ -583,7 +583,6 @@ func convertParameterSet(namespace string, data []byte) (storage.ParameterSet, e SchemaVersion: storage.DefaultParameterSetSchemaVersion, Namespace: namespace, Name: src.Name, - Parameters: make([]secrets.SourceMap, len(src.Parameters)), }, Status: storage.ParameterSetStatus{ Created: src.Created, @@ -591,14 +590,15 @@ func convertParameterSet(namespace string, data []byte) (storage.ParameterSet, e }, } - for i, cred := range src.Parameters { - dest.Parameters[i] = secrets.SourceMap{ - Name: cred.Name, + dest.Parameters = storage.MakeParameterSourceMap(len(src.Parameters)) + for _, srcParam := range src.Parameters { + destParam := storage.ParameterSource{ Source: secrets.Source{ - Strategy: cred.Source.Key, - Hint: cred.Source.Value, + Strategy: srcParam.Source.Key, + Hint: srcParam.Source.Value, }, } + dest.Parameters.Set(srcParam.Name, destParam) } return dest, nil diff --git a/pkg/storage/migrations/migration_test.go b/pkg/storage/migrations/migration_test.go index f8671c8d5..37668645e 100644 --- a/pkg/storage/migrations/migration_test.go +++ b/pkg/storage/migrations/migration_test.go @@ -7,13 +7,12 @@ import ( "testing" "time" + "get.porter.sh/porter/pkg/cnab" "get.porter.sh/porter/pkg/config" "get.porter.sh/porter/pkg/portercontext" - testmigrations "get.porter.sh/porter/pkg/storage/migrations/testhelpers" - - "get.porter.sh/porter/pkg/cnab" "get.porter.sh/porter/pkg/secrets" "get.porter.sh/porter/pkg/storage" + testmigrations "get.porter.sh/porter/pkg/storage/migrations/testhelpers" "get.porter.sh/porter/pkg/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -45,10 +44,10 @@ func TestConvertClaimToRun(t *testing.T) { assert.Equal(t, "2022-04-29T16:09:42.65907-05:00", run.Created.Format(time.RFC3339Nano), "incorrect created timestamp") assert.Equal(t, "install", run.Action, "incorrect action") assert.NotEmpty(t, run.Bundle, "bundle was not populated") - assert.Len(t, run.Parameters.Parameters, 1, "incorrect parameters") + assert.Equal(t, 1, run.Parameters.Len(), "incorrect parameters") - param := run.Parameters.Parameters[0] - assert.Equal(t, param.Name, "porter-debug", "incorrect parameter name") + param, ok := run.Parameters.Get("porter-debug") + require.True(t, ok, "expected 'porter-debug' to be present") assert.Equal(t, param.Source.Strategy, "value", "incorrect parameter source key") assert.Equal(t, param.Source.Hint, "false", "incorrect parameter source value") } @@ -154,7 +153,7 @@ func validateMigratedInstallations(ctx context.Context, t *testing.T, c *config. assert.Empty(t, inst.Labels, "We didn't allow setting labels on installations in v0, so this can't be populated") assert.Empty(t, inst.CredentialSets, "We didn't track credential sets used when running a bundle in v0, so this can't be populated") assert.Empty(t, inst.ParameterSets, "We didn't track parameter sets used when running a bundle in v0, so this can't be populated") - assert.Empty(t, inst.Parameters.Parameters, "We didn't track manually specified parameters when running a bundle in v0, so this can't be populated") + assert.Equal(t, 0, inst.Parameters.Len(), "We didn't track manually specified parameters when running a bundle in v0, so this can't be populated") // Validate the installation status, which is calculated based on the runs and their results assert.Equal(t, "2022-04-28T16:09:42.65907-05:00", inst.Status.Created.Format(time.RFC3339Nano), "Created timestamp should be set to the timestamp of the first run") @@ -186,13 +185,14 @@ func validateMigratedInstallations(ctx context.Context, t *testing.T, c *config. assert.Empty(t, lastRun.Custom, "We didn't set custom datadata on claims in v0, so this can't be populated") assert.Equal(t, "2022-04-29T16:13:20.48026-05:00", lastRun.Created.Format(time.RFC3339Nano), "incorrect run created timestamp") assert.Empty(t, lastRun.ParameterSets, "We didn't track run parameter sets in v0, so this can't be populated") - assert.Empty(t, lastRun.ParameterOverrides, "We didn't track run parameter overrides in v0, so this can't be populated") + assert.Equal(t, 0, lastRun.ParameterOverrides.Len(), "We didn't track run parameter overrides in v0, so this can't be populated") assert.Empty(t, lastRun.CredentialSets, "We didn't track run credential sets in v0, so this can't be populated") - assert.Len(t, lastRun.Parameters.Parameters, 1, "expected one parameter set on the run") - params := lastRun.Parameters.Parameters - assert.Equal(t, "porter-debug", params[0].Name, "expected the porter-debug parameter to be set on the run") - assert.Equal(t, "value", params[0].Source.Strategy, "expected the porter-debug parameter to be a hard-coded value") - assert.Equal(t, "true", params[0].Source.Hint, "expected the porter-debug parameter to be false") + assert.Equal(t, 1, lastRun.Parameters.Len(), "expected one parameter set on the run") + + param, ok := lastRun.Parameters.Get("porter-debug") + require.True(t, ok, "expected the porter-debug parameter to be set on the run") + assert.Equal(t, "value", param.Source.Strategy, "expected the porter-debug parameter to be a hard-coded value") + assert.Equal(t, "true", param.Source.Hint, "expected the porter-debug parameter to be false") runResults := results[lastRun.ID] assert.Len(t, runResults, 2, "expected 2 results for the last run") @@ -236,10 +236,10 @@ func validateMigratedCredentialSets(ctx context.Context, t *testing.T, destStore assert.Equal(t, "2022-06-06T16:06:52.099455-05:00", creds.Status.Created.Format(time.RFC3339Nano), "incorrect created timestamp") assert.Equal(t, "2022-06-06T16:07:52.099455-05:00", creds.Status.Modified.Format(time.RFC3339Nano), "incorrect modified timestamp") assert.Empty(t, creds.Labels, "incorrect labels") - require.Len(t, creds.Credentials, 1, "incorrect number of credentials migrated") + require.Equal(t, 1, creds.Len(), "incorrect number of credentials migrated") - cred := creds.Credentials[0] - assert.Equal(t, "github-token", cred.Name, "incorrect credential name") + cred, ok := creds.Get("github-token") + require.True(t, ok, "expected 'github-token' to be present") assert.Equal(t, "env", cred.Source.Strategy, "incorrect credential source key") assert.Equal(t, "GITHUB_TOKEN", cred.Source.Hint, "incorrect credential source value") } @@ -260,10 +260,10 @@ func validateMigratedParameterSets(ctx context.Context, t *testing.T, destStore assert.Equal(t, "2022-06-06T16:06:21.635528-05:00", ps.Status.Created.Format(time.RFC3339Nano), "incorrect created timestamp") assert.Equal(t, "2022-06-06T17:06:21.635528-05:00", ps.Status.Modified.Format(time.RFC3339Nano), "incorrect modified timestamp") assert.Empty(t, ps.Labels, "incorrect labels") - require.Len(t, ps.Parameters, 1, "incorrect number of parameters migrated") + require.Equal(t, 1, ps.Len(), "incorrect number of parameters migrated") - param := ps.Parameters[0] - assert.Equal(t, "name", param.Name, "incorrect parameter name") + param, ok := ps.Get("name") + require.True(t, ok, "expected 'name' parameter to be present") assert.Equal(t, "env", param.Source.Strategy, "incorrect parameter source key") assert.Equal(t, "USER", param.Source.Hint, "incorrect parameter source value") } diff --git a/pkg/storage/parameter_store.go b/pkg/storage/parameter_store.go index cb72e3125..a8f1ca6d5 100644 --- a/pkg/storage/parameter_store.go +++ b/pkg/storage/parameter_store.go @@ -55,13 +55,13 @@ func (s ParameterStore) ResolveAll(ctx context.Context, params ParameterSet) (se resolvedParams := make(secrets.Set) var resolveErrors error - for _, param := range params.Parameters { + for paramName, param := range params.Iterate() { value, err := s.Secrets.Resolve(ctx, param.Source.Strategy, param.Source.Hint) if err != nil { - resolveErrors = multierror.Append(resolveErrors, fmt.Errorf("unable to resolve parameter %s.%s from %s %s: %w", params.Name, param.Name, param.Source.Strategy, param.Source.Hint, err)) + resolveErrors = multierror.Append(resolveErrors, fmt.Errorf("unable to resolve parameter %s.%s from %s %s: %w", params.Name, paramName, param.Source.Strategy, param.Source.Hint, err)) } - resolvedParams[param.Name] = value + resolvedParams[paramName] = value } return resolvedParams, resolveErrors @@ -71,7 +71,7 @@ func (s ParameterStore) Validate(ctx context.Context, params ParameterSet) error validSources := []string{secrets.SourceSecret, host.SourceValue, host.SourceEnv, host.SourcePath, host.SourceCommand} var errors error - for _, cs := range params.Parameters { + for _, cs := range params.Iterate() { valid := false for _, validSource := range validSources { if cs.Source.Strategy == validSource { diff --git a/pkg/storage/parameter_store_test.go b/pkg/storage/parameter_store_test.go index 7fa4be96d..aff38168d 100644 --- a/pkg/storage/parameter_store_test.go +++ b/pkg/storage/parameter_store_test.go @@ -18,14 +18,11 @@ func TestParameterStore_CRUD(t *testing.T) { require.NoError(t, err) require.Empty(t, params, "Find should return no entries") - myParamSet := NewParameterSet("dev", "myparams", - secrets.SourceMap{ - Name: "myparam", - Source: secrets.Source{ - Strategy: "value", - Hint: "myparamvalue", - }, - }) + myParamSet := NewParameterSet("dev", "myparams") + myParamSet.SetStrategy("myparam", secrets.Source{ + Strategy: "value", + Hint: "myparamvalue", + }) myParamSet.Status.Created = time.Date(2020, 1, 1, 12, 0, 0, 0, time.UTC) myParamSet.Status.Modified = myParamSet.Status.Created @@ -49,14 +46,11 @@ func TestParameterStore_CRUD(t *testing.T) { require.NoError(t, err) require.Equal(t, myParamSet, pset, "Get should return the saved parameter set") - myParamSet2 := NewParameterSet("dev", "myparams2", - secrets.SourceMap{ - Name: "myparam2", - Source: secrets.Source{ - Strategy: "value2", - Hint: "myparamvalue2", - }, - }) + myParamSet2 := NewParameterSet("dev", "myparams2") + myParamSet2.SetStrategy("myparam2", secrets.Source{ + Strategy: "value2", + Hint: "myparamvalue2", + }) myParamSet2.Status.Created = time.Date(2020, 1, 1, 12, 0, 0, 0, time.UTC) myParamSet2.Status.Modified = myParamSet2.Status.Created @@ -93,21 +87,15 @@ func TestParameterStore_CRUD(t *testing.T) { func TestParameterStorage_ResolveAll(t *testing.T) { // The inmemory secret store currently only supports secret sources // So all of these have this same source - testParameterSet := NewParameterSet("", "myparamset", - secrets.SourceMap{ - Name: "param1", - Source: secrets.Source{ - Strategy: "secret", - Hint: "param1", - }, - }, - secrets.SourceMap{ - Name: "param2", - Source: secrets.Source{ - Strategy: "secret", - Hint: "param2", - }, - }) + testParameterSet := NewParameterSet("", "myparamset") + testParameterSet.SetStrategy("param1", secrets.Source{ + Strategy: "secret", + Hint: "param1", + }) + testParameterSet.SetStrategy("param2", secrets.Source{ + Strategy: "secret", + Hint: "param2", + }) t.Run("resolve params success", func(t *testing.T) { paramStore := NewTestParameterProvider(t) @@ -148,37 +136,27 @@ func TestParameterStorage_Validate(t *testing.T) { t.Run("valid sources", func(t *testing.T) { s := ParameterStore{} - testParameterSet := NewParameterSet("", "myparams", - secrets.SourceMap{ - Source: secrets.Source{ - Strategy: "env", - Hint: "SOME_ENV", - }, - }, - secrets.SourceMap{ - Source: secrets.Source{ - Strategy: "value", - Hint: "somevalue", - }, - }, - secrets.SourceMap{ - Source: secrets.Source{ - Strategy: "path", - Hint: "/some/path", - }, - }, - secrets.SourceMap{ - Source: secrets.Source{ - Strategy: "command", - Hint: "some command", - }, - }, - secrets.SourceMap{ - Source: secrets.Source{ - Strategy: "secret", - Hint: "secret", - }, - }) + testParameterSet := NewParameterSet("", "myparams") + testParameterSet.SetStrategy("stuff", secrets.Source{ + Strategy: "env", + Hint: "SOME_ENV", + }) + testParameterSet.SetStrategy("things", secrets.Source{ + Strategy: "value", + Hint: "somevalue", + }) + testParameterSet.SetStrategy("pathy", secrets.Source{ + Strategy: "path", + Hint: "/some/path", + }) + testParameterSet.SetStrategy("commandy", secrets.Source{ + Strategy: "command", + Hint: "some command", + }) + testParameterSet.SetStrategy("secrety", secrets.Source{ + Strategy: "secret", + Hint: "secret", + }) err := s.Validate(context.Background(), testParameterSet) require.NoError(t, err, "Validate did not return errors") @@ -186,19 +164,15 @@ func TestParameterStorage_Validate(t *testing.T) { t.Run("invalid sources", func(t *testing.T) { s := ParameterStore{} - testParameterSet := NewParameterSet("", "myparams", - secrets.SourceMap{ - Source: secrets.Source{ - Strategy: "wrongthing", - Hint: "SOME_ENV", - }, - }, - secrets.SourceMap{ - Source: secrets.Source{ - Strategy: "anotherwrongthing", - Hint: "somevalue", - }, - }) + testParameterSet := NewParameterSet("", "myparams") + testParameterSet.SetStrategy("badstuff", secrets.Source{ + Strategy: "wrongthing", + Hint: "SOME_ENV", + }) + testParameterSet.SetStrategy("badthing", secrets.Source{ + Strategy: "anotherwrongthing", + Hint: "somevalue", + }) err := s.Validate(context.Background(), testParameterSet) require.Error(t, err, "Validate returned errors") diff --git a/pkg/storage/parameters.go b/pkg/storage/parameters.go index dc74d946e..146ccef82 100644 --- a/pkg/storage/parameters.go +++ b/pkg/storage/parameters.go @@ -33,11 +33,13 @@ func ParseVariableAssignments(params []string) (map[string]string, error) { return variables, nil } -// ValueStrategy is the strategy used to store non-sensitive parameters -func ValueStrategy(name string, value string) secrets.SourceMap { - return secrets.SourceMap{ - Name: name, - Source: secrets.Source{Strategy: host.SourceValue, Hint: value}, +// NamedValueStrategy is the strategy used to store non-sensitive parameters, it includes both the value name and source. +func NamedValueStrategy(name string, value string) secrets.NamedValueMapping { + return secrets.NamedValueMapping{ + Name: name, + Source: secrets.Source{ + Strategy: host.SourceValue, + Hint: value}, ResolvedValue: value, } } diff --git a/pkg/storage/parameters_test.go b/pkg/storage/parameters_test.go index 2d6e0ee54..e103e4f72 100644 --- a/pkg/storage/parameters_test.go +++ b/pkg/storage/parameters_test.go @@ -65,42 +65,27 @@ func TestTestParameterProvider_Load(t *testing.T) { }) t.Run("successful load, successful unmarshal", func(t *testing.T) { - expected := NewParameterSet("", "mybun", - secrets.SourceMap{ - Name: "param_env", - Source: secrets.Source{ - Strategy: "env", - Hint: "PARAM_ENV", - }, - }, - secrets.SourceMap{ - Name: "param_value", - Source: secrets.Source{ - Strategy: "value", - Hint: "param_value", - }, - }, - secrets.SourceMap{ - Name: "param_command", - Source: secrets.Source{ - Strategy: "command", - Hint: "echo hello world", - }, - }, - secrets.SourceMap{ - Name: "param_path", - Source: secrets.Source{ - Strategy: "path", - Hint: "/path/to/param", - }, - }, - secrets.SourceMap{ - Name: "param_secret", - Source: secrets.Source{ - Strategy: "secret", - Hint: "param_secret", - }, - }) + expected := NewParameterSet("", "mybun") + expected.SetStrategy("param_env", secrets.Source{ + Strategy: "env", + Hint: "PARAM_ENV"}) + expected.SetStrategy("param_value", secrets.Source{ + Strategy: "value", + Hint: "param_value", + }) + expected.SetStrategy("param_command", secrets.Source{ + Strategy: "command", + Hint: "echo hello world", + }) + expected.SetStrategy("param_path", secrets.Source{ + Strategy: "path", + Hint: "/path/to/param", + }) + expected.SetStrategy("param_secret", secrets.Source{ + Strategy: "secret", + Hint: "param_secret", + }) + expected.SchemaVersion = "1.0.1" // It's an older code but it checks out expected.Status.Created = time.Date(1983, time.April, 18, 1, 2, 3, 4, time.UTC) expected.Status.Modified = expected.Status.Created diff --git a/pkg/storage/parameterset.go b/pkg/storage/parameterset.go index 66f104c8c..8d6449d5b 100644 --- a/pkg/storage/parameterset.go +++ b/pkg/storage/parameterset.go @@ -44,9 +44,16 @@ type ParameterSetSpec struct { Labels map[string]string `json:"labels,omitempty" yaml:"labels,omitempty" toml:"labels,omitempty"` // Parameters is a list of parameter specs. - Parameters secrets.StrategyList `json:"parameters" yaml:"parameters" toml:"parameters"` + Parameters ParameterSourceMap `json:"parameters" yaml:"parameters" toml:"parameters"` } +type ParameterSourceMap = secrets.Map +type ParameterSource = secrets.ValueMapping +type NamedParameterSource = secrets.NamedValueMapping + +var MakeParameterSourceMap = secrets.MakeMap +var NewParameterSourceMap = secrets.NewMap + // ParameterSetStatus contains additional status metadata that has been set by Porter. type ParameterSetStatus struct { // Created timestamp of the parameter set. @@ -57,7 +64,7 @@ type ParameterSetStatus struct { } // NewParameterSet creates a new ParameterSet with the required fields initialized. -func NewParameterSet(namespace string, name string, params ...secrets.SourceMap) ParameterSet { +func NewParameterSet(namespace string, name string) ParameterSet { now := time.Now() ps := ParameterSet{ ParameterSetSpec: ParameterSetSpec{ @@ -65,7 +72,7 @@ func NewParameterSet(namespace string, name string, params ...secrets.SourceMap) SchemaVersion: DefaultParameterSetSchemaVersion, Namespace: namespace, Name: name, - Parameters: params, + Parameters: NewParameterSourceMap(), }, Status: ParameterSetStatus{ Created: now, @@ -77,8 +84,8 @@ func NewParameterSet(namespace string, name string, params ...secrets.SourceMap) } // NewInternalParameterSet creates a new internal ParameterSet with the required fields initialized. -func NewInternalParameterSet(namespace string, name string, params ...secrets.SourceMap) ParameterSet { - return NewParameterSet(namespace, INTERNAL_PARAMETERER_SET+"-"+name, params...) +func NewInternalParameterSet(namespace string, name string) ParameterSet { + return NewParameterSet(namespace, INTERNAL_PARAMETERER_SET+"-"+name) } func (s ParameterSet) DefaultDocumentFilter() map[string]interface{} { @@ -121,3 +128,31 @@ func (s *ParameterSet) Validate(ctx context.Context, strategy schema.CheckStrate func (s ParameterSet) String() string { return fmt.Sprintf("%s/%s", s.Namespace, s.Name) } + +func (s ParameterSet) Iterate() map[string]ParameterSource { + return s.Parameters.Items() +} + +func (s ParameterSet) IterateSorted() []NamedParameterSource { + return s.Parameters.ItemsSorted() +} + +func (s ParameterSet) SetStrategy(key string, source secrets.Source) { + s.Parameters.Set(key, ParameterSource{Source: source}) +} + +func (s ParameterSet) Set(key string, source ParameterSource) { + s.Parameters.Set(key, source) +} + +func (s ParameterSet) Get(key string) (ParameterSource, bool) { + return s.Parameters.Get(key) +} + +func (s ParameterSet) Remove(key string) { + s.Parameters.Remove(key) +} + +func (s ParameterSet) Len() int { + return s.Parameters.Len() +} diff --git a/pkg/storage/parameterset_test.go b/pkg/storage/parameterset_test.go index fa776cedb..33ee5fd9f 100644 --- a/pkg/storage/parameterset_test.go +++ b/pkg/storage/parameterset_test.go @@ -14,14 +14,11 @@ import ( ) func TestNewParameterSet(t *testing.T) { - ps := NewParameterSet("dev", "myparams", - secrets.SourceMap{ - Name: "password", - Source: secrets.Source{ - Strategy: "env", - Hint: "DB_PASSWORD", - }, - }) + ps := NewParameterSet("dev", "myparams") + ps.SetStrategy("password", secrets.Source{ + Strategy: "env", + Hint: "DB_PASSWORD", + }) assert.Equal(t, DefaultParameterSetSchemaVersion, ps.SchemaVersion, "SchemaVersion was not set") assert.Equal(t, "myparams", ps.Name, "Name was not set") @@ -31,7 +28,7 @@ func TestNewParameterSet(t *testing.T) { assert.Equal(t, ps.Status.Created, ps.Status.Modified, "Created and Modified should have the same timestamp") assert.Equal(t, SchemaTypeParameterSet, ps.SchemaType, "incorrect SchemaType") assert.Equal(t, DefaultParameterSetSchemaVersion, ps.SchemaVersion, "incorrect SchemaVersion") - assert.Len(t, ps.Parameters, 1, "Parameters should be initialized with 1 value") + assert.Equal(t, 1, ps.Len(), "Parameters should be initialized with 1 value") } func TestParameterSet_String(t *testing.T) { diff --git a/pkg/storage/run.go b/pkg/storage/run.go index c4aaf13aa..bee1e8a53 100644 --- a/pkg/storage/run.go +++ b/pkg/storage/run.go @@ -172,29 +172,29 @@ func (r Run) TypedParameterValues() map[string]interface{} { bun := cnab.NewBundle(r.Bundle) value := make(map[string]interface{}) - for _, param := range r.Parameters.Parameters { - v, err := bun.ConvertParameterValue(param.Name, param.ResolvedValue) + for paramName, param := range r.Parameters.Iterate() { + v, err := bun.ConvertParameterValue(paramName, param.ResolvedValue) if err != nil { - value[param.Name] = param.ResolvedValue + value[paramName] = param.ResolvedValue continue } - def, ok := bun.Definitions[param.Name] + def, ok := bun.Definitions[paramName] if !ok { - value[param.Name] = v + value[paramName] = v continue } if bun.IsFileType(def) && v == "" { v = nil } - value[param.Name] = v + value[paramName] = v } return value } -// NewRun creates a result for the current Run. +// NewResult creates a result for the current Run. func (r Run) NewResult(status string) Result { result := NewResult() result.RunID = r.ID diff --git a/pkg/storage/run_test.go b/pkg/storage/run_test.go index 063bad9c4..ba9ae573b 100644 --- a/pkg/storage/run_test.go +++ b/pkg/storage/run_test.go @@ -172,12 +172,12 @@ func TestRun_TypedParameterValues(t *testing.T) { run := NewRun("dev", "mybuns") run.Bundle = bun run.Parameters = NewParameterSet(run.Namespace, run.Bundle.Name) - params := []secrets.SourceMap{ - ValueStrategy("baz", "baz-test"), - ValueStrategy("name", "porter-test"), - ValueStrategy("porter-state", ""), - {Name: "foo", Source: secrets.Source{Strategy: secrets.SourceSecret, Hint: "runID"}, ResolvedValue: "5"}, - } + run.Parameters.Set("baz", secrets.HardCodedValue("baz-test")) + run.Parameters.Set("name", secrets.HardCodedValue("porter-test")) + run.Parameters.Set("porter-state", secrets.HardCodedValue("")) + run.Parameters.Set("foo", ParameterSource{ + Source: secrets.Source{Strategy: "secret", Hint: "runID"}, + ResolvedValue: "5"}) expected := map[string]interface{}{ "baz": "baz-test", @@ -186,9 +186,8 @@ func TestRun_TypedParameterValues(t *testing.T) { "foo": 5, } - run.Parameters.Parameters = params typed := run.TypedParameterValues() - require.Equal(t, len(params), len(typed)) + require.Equal(t, run.Parameters.Len(), len(typed)) require.Equal(t, len(expected), len(typed)) for name, value := range typed { @@ -211,5 +210,5 @@ func TestRun_MarshalJSON(t *testing.T) { err = json.Unmarshal(data, &r2) require.NoError(t, err, "Unmarshal failed") - assert.Equal(t, r1, r2, "The run did not survive the round trip") + assert.Equal(t, r1.Bundle, r2.Bundle, "The Run's bundle did not survive the round trip") } diff --git a/pkg/storage/sanitizer.go b/pkg/storage/sanitizer.go index 45e9c0d57..e42d3e39e 100644 --- a/pkg/storage/sanitizer.go +++ b/pkg/storage/sanitizer.go @@ -29,23 +29,23 @@ func NewSanitizer(parameterstore ParameterSetProvider, secretstore secrets.Store // transform the raw value into secret strategies. // The id argument is used to associate the reference key with the corresponding // run or installation record in porter's database. -func (s *Sanitizer) CleanRawParameters(ctx context.Context, params map[string]interface{}, bun cnab.ExtendedBundle, id string) ([]secrets.SourceMap, error) { - strategies := make([]secrets.SourceMap, 0, len(params)) +func (s *Sanitizer) CleanRawParameters(ctx context.Context, params map[string]interface{}, bun cnab.ExtendedBundle, id string) (ParameterSourceMap, error) { + strategies := MakeParameterSourceMap(len(params)) for name, value := range params { stringVal, err := bun.WriteParameterToString(name, value) if err != nil { - return nil, err + return ParameterSourceMap{}, err } - strategy := ValueStrategy(name, stringVal) - strategies = append(strategies, strategy) + + strategies.Set(name, secrets.HardCodedValue(stringVal)) } - strategies, err := s.CleanParameters(ctx, strategies, bun, id) + result, err := s.CleanParameters(ctx, strategies, bun, id) if err != nil { - return nil, err + return ParameterSourceMap{}, err } - return strategies, nil + return result, nil } @@ -53,25 +53,25 @@ func (s *Sanitizer) CleanRawParameters(ctx context.Context, params map[string]in // Sanitized value after saving sensitive data to secrets store. // The id argument is used to associate the reference key with the corresponding // run or installation record in porter's database. -func (s *Sanitizer) CleanParameters(ctx context.Context, dirtyParams []secrets.SourceMap, bun cnab.ExtendedBundle, id string) ([]secrets.SourceMap, error) { - cleanedParams := make([]secrets.SourceMap, 0, len(dirtyParams)) - for _, param := range dirtyParams { +func (s *Sanitizer) CleanParameters(ctx context.Context, dirtyParams ParameterSourceMap, bun cnab.ExtendedBundle, id string) (ParameterSourceMap, error) { + cleanedParams := MakeParameterSourceMap(dirtyParams.Len()) + for paramName, param := range dirtyParams.Items() { // Store sensitive hard-coded values in a secret store - if param.Source.Strategy == host.SourceValue && bun.IsSensitiveParameter(param.Name) { - cleaned := sanitizedParam(param, id) + if param.Source.Strategy == host.SourceValue && bun.IsSensitiveParameter(paramName) { + cleaned := sanitizedParam(paramName, param, id) err := s.secrets.Create(ctx, cleaned.Source.Strategy, cleaned.Source.Hint, cleaned.ResolvedValue) if err != nil { - return nil, fmt.Errorf("failed to save sensitive param to secrete store: %w", err) + return ParameterSourceMap{}, fmt.Errorf("failed to save sensitive param to secrete store: %w", err) } - cleanedParams = append(cleanedParams, cleaned) + cleanedParams.Set(paramName, cleaned) } else { // All other parameters are safe to use without cleaning - cleanedParams = append(cleanedParams, param) + cleanedParams.Set(paramName, param) } } - if len(cleanedParams) == 0 { - return nil, nil + if cleanedParams.Len() == 0 { + return NewParameterSourceMap(), nil } return cleanedParams, nil @@ -83,20 +83,24 @@ func (s *Sanitizer) CleanParameters(ctx context.Context, dirtyParams []secrets.S // The id argument is used to associate the reference key with the corresponding // run or installation record in porter's database. func LinkSensitiveParametersToSecrets(pset ParameterSet, bun cnab.ExtendedBundle, id string) ParameterSet { - for i, param := range pset.Parameters { - if !bun.IsSensitiveParameter(param.Name) { + for paramName, param := range pset.Iterate() { + if !bun.IsSensitiveParameter(paramName) { continue } - pset.Parameters[i] = sanitizedParam(param, id) + pset.Set(paramName, sanitizedParam(paramName, param, id)) } return pset } -func sanitizedParam(param secrets.SourceMap, id string) secrets.SourceMap { - param.Source.Strategy = secrets.SourceSecret - param.Source.Hint = id + "-" + param.Name - return param +func sanitizedParam(paramName string, param ParameterSource, id string) ParameterSource { + return ParameterSource{ + Source: secrets.Source{ + Strategy: secrets.SourceSecret, + Hint: id + "-" + paramName, + }, + ResolvedValue: param.ResolvedValue, + } } // RestoreParameterSet resolves the raw parameter data from a secrets store. diff --git a/pkg/storage/sanitizer_test.go b/pkg/storage/sanitizer_test.go index 135c9b87a..2f48d4afb 100644 --- a/pkg/storage/sanitizer_test.go +++ b/pkg/storage/sanitizer_test.go @@ -28,13 +28,12 @@ func TestSanitizer_Parameters(t *testing.T) { recordID := "01FZVC5AVP8Z7A78CSCP1EJ604" sensitiveParamName := "my-second-param" sensitiveParamKey := recordID + "-" + sensitiveParamName - expected := []secrets.SourceMap{ - {Name: "my-first-param", Source: secrets.Source{Strategy: host.SourceValue, Hint: "1"}, ResolvedValue: "1"}, - {Name: sensitiveParamName, Source: secrets.Source{Strategy: secrets.SourceSecret, Hint: sensitiveParamKey}, ResolvedValue: "2"}, - } - sort.SliceStable(expected, func(i, j int) bool { - return expected[i].Name < expected[j].Name - }) + expected := storage.NewParameterSourceMap() + expected.Set("my-first-param", secrets.HardCodedValue("1")) + expected.Set(sensitiveParamName, storage.ParameterSource{Source: secrets.Source{ + Strategy: secrets.SourceSecret, + Hint: sensitiveParamKey}, + ResolvedValue: "2"}) // parameters that are hard coded values should be sanitized, while those mapped from secrets or env vars should be left alone by the sanitizer rawParams := map[string]interface{}{ @@ -43,14 +42,11 @@ func TestSanitizer_Parameters(t *testing.T) { } result, err := r.TestSanitizer.CleanRawParameters(ctx, rawParams, bun, recordID) require.NoError(t, err) - require.Equal(t, len(expected), len(result)) - sort.SliceStable(result, func(i, j int) bool { - return result[i].Name < result[j].Name - }) + require.Equal(t, expected.Len(), result.Len()) + require.Equal(t, expected, result) - require.Truef(t, reflect.DeepEqual(result, expected), "expected: %v, got: %v", expected, result) - - pset := storage.NewParameterSet("", "dev", result...) + pset := storage.NewParameterSet("", "dev") + pset.Parameters = result resolved, err := r.TestSanitizer.RestoreParameterSet(ctx, pset, bun) require.NoError(t, err) @@ -107,13 +103,13 @@ func TestSanitizer_CleanParameters(t *testing.T) { inst := storage.NewInstallation("", "mybuns") inst.ID = "INSTALLATION_ID" // Standardize for easy comparisons later - inst.Parameters.Parameters = []secrets.SourceMap{ - {Name: tc.paramName, Source: secrets.Source{Strategy: tc.sourceKey, Hint: "myvalue"}}, - } + inst.Parameters.SetStrategy(tc.paramName, secrets.Source{Strategy: tc.sourceKey, Hint: "myvalue"}) + gotParams, err := r.Sanitizer.CleanParameters(ctx, inst.Parameters.Parameters, bun, inst.ID) require.NoError(t, err, "CleanParameters failed") - wantParms := []secrets.SourceMap{{Name: tc.paramName, Source: tc.wantSource}} + wantParms := storage.NewParameterSourceMap() + wantParms.Set(tc.paramName, storage.ParameterSource{Source: tc.wantSource}) require.Equal(t, wantParms, gotParams, "unexpected value returned from CleanParameters") }) } diff --git a/pkg/test/helper.go b/pkg/test/helper.go index 947f4c301..a6b1bce75 100644 --- a/pkg/test/helper.go +++ b/pkg/test/helper.go @@ -69,6 +69,7 @@ func TestMainWithMockedCommandHandlers(m *testing.M) { // When they are different and PORTER_UPDATE_TEST_FILES is true, the file is updated to match // the new test output. func CompareGoldenFile(t *testing.T, goldenFile string, got string) { + t.Helper() if os.Getenv("PORTER_UPDATE_TEST_FILES") == "true" { os.MkdirAll(filepath.Dir(goldenFile), pkg.FileModeDirectory) t.Logf("Updated test file %s to match latest test output", goldenFile) diff --git a/tests/integration/dependencies_test.go b/tests/integration/dependencies_test.go index d5065fa67..fc68272a7 100644 --- a/tests/integration/dependencies_test.go +++ b/tests/integration/dependencies_test.go @@ -4,15 +4,14 @@ package integration import ( "context" + "get.porter.sh/porter/pkg/secrets" "os" "path/filepath" "testing" "get.porter.sh/porter/pkg/cnab" "get.porter.sh/porter/pkg/porter" - "get.porter.sh/porter/pkg/secrets" "get.porter.sh/porter/pkg/storage" - "github.com/cnabio/cnab-go/secrets/host" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -74,13 +73,8 @@ func installWordpressBundle(ctx context.Context, p *porter.TestPorter) (namespac // Add a supplemental parameter set to vet dep param resolution installOpts.ParameterSets = []string{"myparam"} - testParamSets := storage.NewParameterSet(namespace, "myparam", secrets.SourceMap{ - Name: "mysql#probe-timeout", - Source: secrets.Source{ - Strategy: host.SourceValue, - Hint: "2", - }, - }) + testParamSets := storage.NewParameterSet(namespace, "myparam") + testParamSets.SetStrategy("mysql#probe-timeout", secrets.HardCodedValueStrategy("2")) p.TestParameters.InsertParameterSet(ctx, testParamSets) diff --git a/tests/integration/install_test.go b/tests/integration/install_test.go index 60d4bb66c..51cde7b37 100644 --- a/tests/integration/install_test.go +++ b/tests/integration/install_test.go @@ -53,12 +53,10 @@ func TestInstall_fileParam(t *testing.T) { installOpts := porter.NewInstallOptions() installOpts.Params = []string{"myfile=./myfile"} installOpts.ParameterSets = []string{"myparam"} - testParamSets := storage.NewParameterSet("", "myparam", secrets.SourceMap{ - Name: "myotherfile", - Source: secrets.Source{ - Strategy: host.SourcePath, - Hint: "./myotherfile", - }, + testParamSets := storage.NewParameterSet("", "myparam") + testParamSets.SetStrategy("myotherfile", secrets.Source{ + Strategy: host.SourcePath, + Hint: "./myotherfile", }) p.TestParameters.InsertParameterSet(ctx, testParamSets) diff --git a/tests/integration/sensitive_data_test.go b/tests/integration/sensitive_data_test.go index 183511e43..5ae3000f2 100644 --- a/tests/integration/sensitive_data_test.go +++ b/tests/integration/sensitive_data_test.go @@ -36,22 +36,13 @@ func TestSensitiveData(t *testing.T) { run, err := p.Installations.GetRun(ctx, i.Status.RunID) require.NoError(t, err) - for _, param := range i.Parameters.Parameters { - if param.Name == sensitiveParamName { - assert.NotContains(t, param.Source.Hint, sensitiveParamValue) - } - } - - for _, param := range run.ParameterOverrides.Parameters { - if param.Name == sensitiveParamName { - assert.NotContains(t, param.Source.Hint, sensitiveParamValue) - } - } - for _, param := range run.Parameters.Parameters { - if param.Name == sensitiveParamName { - assert.NotContains(t, param.Source.Hint, sensitiveParamValue) - } - } + sensitiveParam, ok := i.Parameters.Get(sensitiveParamName) + require.True(t, ok) + assert.NotContains(t, sensitiveParam.Source.Hint, sensitiveParamValue) + assert.NotContains(t, sensitiveParam.Source.Hint, sensitiveParamValue) + sensitiveOverride, ok := run.ParameterOverrides.Get(sensitiveParamName) + require.True(t, ok) + assert.NotContains(t, sensitiveOverride.Source.Hint, sensitiveParamValue) outputs, err := p.Installations.GetLastOutputs(ctx, "", bundleName) require.NoError(t, err, "GetLastOutput failed") diff --git a/tests/tester/helpers.go b/tests/tester/helpers.go index f5290fbd9..c4448798e 100644 --- a/tests/tester/helpers.go +++ b/tests/tester/helpers.go @@ -62,7 +62,10 @@ func (t Tester) ShowInstallation(namespace string, name string) (porter.DisplayI var di porter.DisplayInstallation - require.NoError(t.T, json.Unmarshal([]byte(stdout), &di)) + err = json.Unmarshal([]byte(stdout), &di) + if err != nil { + t.T.Fatalf("porter show returned non-json output to stdout: %s", stdout) + } return di, nil }