From 9629bbbe7aa8bcd0638760cfeada8892f63f2901 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 26 Apr 2023 11:06:34 -0500 Subject: [PATCH 1/4] Add ArrayMap Signed-off-by: Carolyn Van Slyck --- pkg/encoding/array_map.go | 198 ++++++++++++++++++ pkg/encoding/array_map_test.go | 127 +++++++++++ pkg/encoding/testdata/array-empty.json | 1 + .../testdata/array-with-duplicates.yaml | 6 + pkg/encoding/testdata/array.yaml | 4 + 5 files changed, 336 insertions(+) create mode 100644 pkg/encoding/array_map.go create mode 100644 pkg/encoding/array_map_test.go create mode 100644 pkg/encoding/testdata/array-empty.json create mode 100644 pkg/encoding/testdata/array-with-duplicates.yaml create mode 100644 pkg/encoding/testdata/array.yaml diff --git a/pkg/encoding/array_map.go b/pkg/encoding/array_map.go new file mode 100644 index 000000000..abe8bd6b8 --- /dev/null +++ b/pkg/encoding/array_map.go @@ -0,0 +1,198 @@ +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 +} + +// TODO(carolyn): Can I make this work without K? +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 + } + + 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 From 05d173f2a0bb94c83118186071c9bc5437762222 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 26 Apr 2023 11:06:50 -0500 Subject: [PATCH 2/4] Use ArrayMap for Parameter and Credential Sets Signed-off-by: Carolyn Van Slyck --- pkg/cnab/config-adapter/adapter.go | 9 +- pkg/cnab/provider/credentials_test.go | 20 ++- pkg/generator/credentials.go | 4 +- pkg/generator/credentials_test.go | 2 +- pkg/generator/generator.go | 41 +++--- pkg/generator/generator_test.go | 7 +- pkg/generator/parameters.go | 4 +- pkg/generator/parameters_test.go | 2 +- pkg/porter/credentials.go | 2 +- pkg/porter/credentials_test.go | 9 +- pkg/porter/dependencies.go | 9 +- pkg/porter/helpers.go | 3 +- pkg/porter/install.go | 3 +- pkg/porter/install_test.go | 23 +--- pkg/porter/lifecycle_test.go | 53 ++++---- pkg/porter/list.go | 10 +- pkg/porter/list_test.go | 18 +-- pkg/porter/parameters.go | 38 ++---- pkg/porter/parameters_test.go | 12 +- pkg/porter/reconcile_test.go | 35 ++--- pkg/porter/show_test.go | 35 +++-- .../testdata/credentials/kool-kreds.json | 12 +- .../testdata/credentials/kool-kreds.txt | 2 +- .../testdata/credentials/kool-kreds.yaml | 6 +- .../testdata/credentials/show-output.json | 12 +- .../testdata/credentials/show-output.yaml | 6 +- pkg/portercontext/helpers.go | 1 + pkg/secrets/map.go | 71 ++++++++++ pkg/secrets/strategy.go | 55 ++------ pkg/secrets/strategy_test.go | 104 ++++++++++++--- .../testdata/duplicate-strategies.yaml | 9 ++ pkg/secrets/testdata/strategies.yaml | 6 + pkg/storage/credential_store.go | 16 ++- pkg/storage/credential_store_test.go | 71 +++++----- pkg/storage/credentialset.go | 42 +++++- pkg/storage/credentialset_test.go | 26 ++-- pkg/storage/installation.go | 11 +- pkg/storage/migrations/migration.go | 32 ++--- pkg/storage/migrations/migration_test.go | 34 ++--- pkg/storage/parameter_store.go | 8 +- pkg/storage/parameter_store_test.go | 124 +++++++----------- pkg/storage/parameters.go | 12 +- pkg/storage/parameters_test.go | 57 +++----- pkg/storage/parameterset.go | 49 ++++++- pkg/storage/parameterset_test.go | 15 +-- pkg/storage/run.go | 14 +- pkg/storage/run_test.go | 15 +-- pkg/storage/sanitizer.go | 48 +++---- pkg/storage/sanitizer_test.go | 32 ++--- pkg/test/helper.go | 1 + tests/integration/dependencies_test.go | 12 +- tests/integration/install_test.go | 10 +- tests/integration/sensitive_data_test.go | 23 +--- tests/tester/helpers.go | 5 +- 54 files changed, 677 insertions(+), 603 deletions(-) create mode 100644 pkg/secrets/map.go create mode 100644 pkg/secrets/testdata/duplicate-strategies.yaml create mode 100644 pkg/secrets/testdata/strategies.yaml 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/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..6b898d260 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) { 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..6a5917f91 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..717c083e3 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.ParameterSourceMap{} + 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.ParameterSourceMap{} + 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..b39db86fc 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.ParameterSourceMap{} + 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..b7b5b6f7a --- /dev/null +++ b/pkg/secrets/map.go @@ -0,0 +1,71 @@ +package secrets + +import "get.porter.sh/porter/pkg/encoding" + +type Map struct { + *encoding.ArrayMap[ValueMapping, NamedValueMapping] +} + +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 +} + +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, + } +} + +// 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..3cde4ebe4 100644 --- a/pkg/storage/credentialset.go +++ b/pkg/storage/credentialset.go @@ -7,6 +7,7 @@ import ( "time" "get.porter.sh/porter/pkg/cnab" + "get.porter.sh/porter/pkg/encoding" "get.porter.sh/porter/pkg/schema" "get.porter.sh/porter/pkg/secrets" "get.porter.sh/porter/pkg/tracing" @@ -50,9 +51,15 @@ 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 = encoding.ArrayMap[CredentialSource, NamedCredentialSource] + +// TODO(generics) +type CredentialSource = secrets.ValueMapping +type NamedCredentialSource = secrets.NamedValueMapping + // CredentialSetStatus contains additional status metadata that has been set by Porter. type CredentialSetStatus struct { // Created timestamp. @@ -62,8 +69,10 @@ type CredentialSetStatus struct { Modified time.Time `json:"modified" yaml:"modified" toml:"modified"` } +var MakeCredentialSourceMap = encoding.MakeArrayMap[CredentialSource, NamedCredentialSource] + // 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 +80,7 @@ func NewCredentialSet(namespace string, name string, creds ...secrets.SourceMap) SchemaVersion: DefaultCredentialSetSchemaVersion, Name: name, Namespace: namespace, - Credentials: creds, + Credentials: &CredentialSourceMap{}, }, Status: CredentialSetStatus{ Created: now, @@ -123,6 +132,33 @@ 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) { + if s.Credentials == nil { + s.Credentials = &CredentialSourceMap{} + } + s.Credentials.Set(key, source) +} + +func (s CredentialSet) Get(key string) (CredentialSource, bool) { + return s.Credentials.Get(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..caf655148 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: &CredentialSourceMap{}, }, 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..ec76ade6d 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,16 @@ func convertCredentialSet(namespace string, data []byte) (storage.CredentialSet, }, } - for i, cred := range src.Credentials { - dest.CredentialSetSpec.Credentials[i] = secrets.SourceMap{ - Name: cred.Name, + destCreds := storage.MakeCredentialSourceMap(len(src.Credentials)) + dest.Credentials = &destCreds + 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, }, } + destCreds.Set(srcCred.Name, destCred) } return dest, nil @@ -583,7 +584,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 +591,16 @@ func convertParameterSet(namespace string, data []byte) (storage.ParameterSet, e }, } - for i, cred := range src.Parameters { - dest.Parameters[i] = secrets.SourceMap{ - Name: cred.Name, + destParams := storage.MakeParameterSourceMap(len(src.Parameters)) + dest.Parameters = &destParams + 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, }, } + destParams.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..c0a7c3668 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") } @@ -188,11 +187,12 @@ func validateMigratedInstallations(ctx context.Context, t *testing.T, c *config. 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.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..699a81ca6 100644 --- a/pkg/storage/parameterset.go +++ b/pkg/storage/parameterset.go @@ -3,6 +3,7 @@ package storage import ( "context" "fmt" + "get.porter.sh/porter/pkg/encoding" "strings" "time" @@ -44,9 +45,17 @@ 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 = encoding.ArrayMap[ParameterSource, NamedParameterSource] + +var MakeParameterSourceMap = encoding.MakeArrayMap[ParameterSource, NamedParameterSource] + +// TODO(generics) +type ParameterSource = secrets.ValueMapping +type NamedParameterSource = secrets.NamedValueMapping + // ParameterSetStatus contains additional status metadata that has been set by Porter. type ParameterSetStatus struct { // Created timestamp of the parameter set. @@ -57,7 +66,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 +74,7 @@ func NewParameterSet(namespace string, name string, params ...secrets.SourceMap) SchemaVersion: DefaultParameterSetSchemaVersion, Namespace: namespace, Name: name, - Parameters: params, + Parameters: &ParameterSourceMap{}, }, Status: ParameterSetStatus{ Created: now, @@ -77,8 +86,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 +130,33 @@ 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 { + if s.Parameters == nil { + return nil + } + return s.Parameters.Items() +} + +func (s ParameterSet) SetStrategy(key string, source secrets.Source) { + s.Parameters.Set(key, ParameterSource{Source: source}) +} + +func (s ParameterSet) Set(key string, source ParameterSource) { + if s.Parameters == nil { // TODO(carolyn): do this in all the places + s.Parameters = &ParameterSourceMap{} + } + 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..096ceeed3 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 { diff --git a/pkg/storage/sanitizer.go b/pkg/storage/sanitizer.go index 45e9c0d57..023645d15 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 } - 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 strategies, nil + return result, nil } @@ -53,28 +53,28 @@ 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) } - 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 { + if cleanedParams.Len() == 0 { return nil, nil } - return cleanedParams, 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..a5d578235 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.ParameterSourceMap{} + 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.ParameterSourceMap{} + 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 } From 84b6fd16c31f024fc0832a0d1c10a271637cc9f7 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 26 Apr 2023 14:24:34 -0500 Subject: [PATCH 3/4] Use secrets.Map for ParameterSourceMap Signed-off-by: Carolyn Van Slyck --- pkg/encoding/array_map.go | 5 ++- pkg/generator/parameters_test.go | 2 +- pkg/porter/helpers.go | 2 +- pkg/porter/lifecycle_test.go | 4 +-- pkg/porter/parameters_test.go | 2 +- pkg/secrets/map.go | 44 +++++++++++++++++++++++- pkg/storage/migrations/migration.go | 5 ++- pkg/storage/migrations/migration_test.go | 4 +-- pkg/storage/parameterset.go | 16 +++------ pkg/storage/run_test.go | 2 +- pkg/storage/sanitizer.go | 16 ++++----- pkg/storage/sanitizer_test.go | 4 +-- 12 files changed, 72 insertions(+), 34 deletions(-) diff --git a/pkg/encoding/array_map.go b/pkg/encoding/array_map.go index abe8bd6b8..235704962 100644 --- a/pkg/encoding/array_map.go +++ b/pkg/encoding/array_map.go @@ -24,7 +24,6 @@ type ArrayMap[T MapElement, K ArrayElement] struct { items map[string]T } -// TODO(carolyn): Can I make this work without K? func MakeArrayMap[T MapElement, K ArrayElement](len int) ArrayMap[T, K] { return ArrayMap[T, K]{ items: make(map[string]T, len), @@ -133,6 +132,10 @@ func (m *ArrayMap[T, K]) UnmarshalRaw(raw []K) error { 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 { diff --git a/pkg/generator/parameters_test.go b/pkg/generator/parameters_test.go index 6b898d260..fbf9a8e8d 100644 --- a/pkg/generator/parameters_test.go +++ b/pkg/generator/parameters_test.go @@ -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/helpers.go b/pkg/porter/helpers.go index 6a5917f91..485524cf0 100644 --- a/pkg/porter/helpers.go +++ b/pkg/porter/helpers.go @@ -240,7 +240,7 @@ func (p *TestPorter) CompareGoldenFile(goldenFile string, got string) { // CreateInstallation saves an installation record into claim store and store // sensitive parameters into secret store. -func (p *TestPorter) SanitizeParameters(raw *storage.ParameterSourceMap, recordID string, bun cnab.ExtendedBundle) *storage.ParameterSourceMap { +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/lifecycle_test.go b/pkg/porter/lifecycle_test.go index 717c083e3..c216a37f8 100644 --- a/pkg/porter/lifecycle_test.go +++ b/pkg/porter/lifecycle_test.go @@ -472,7 +472,7 @@ 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 - wantParameters := &storage.ParameterSourceMap{} + wantParameters := storage.NewParameterSourceMap() wantParameters.Set("my-first-param", storage.ParameterSource{ Source: secrets.Source{Strategy: "value", Hint: "1"}}) wantParameters.Set("my-second-param", storage.ParameterSource{ @@ -519,7 +519,7 @@ func TestPorter_applyActionOptionsToInstallation_PreservesExistingParams(t *test require.Equal(t, 2, i.Parameters.Len()) // Check that overrides are applied on top of existing parameters - wantParameters := &storage.ParameterSourceMap{} + wantParameters := storage.NewParameterSourceMap() wantParameters.Set("my-first-param", storage.ParameterSource{ Source: secrets.Source{Strategy: "value", Hint: "3"}, }) diff --git a/pkg/porter/parameters_test.go b/pkg/porter/parameters_test.go index b39db86fc..0f3e3f915 100644 --- a/pkg/porter/parameters_test.go +++ b/pkg/porter/parameters_test.go @@ -859,7 +859,7 @@ func TestPorter_ParametersApply(t *testing.T) { assert.Equal(t, "mypset", ps.Name, "unexpected parameter set name") - wantParams := &storage.ParameterSourceMap{} + wantParams := storage.NewParameterSourceMap() wantParams.Set("foo", storage.ParameterSource{ Source: secrets.Source{ Strategy: "secret", diff --git a/pkg/secrets/map.go b/pkg/secrets/map.go index b7b5b6f7a..a3995a79a 100644 --- a/pkg/secrets/map.go +++ b/pkg/secrets/map.go @@ -1,11 +1,29 @@ package secrets -import "get.porter.sh/porter/pkg/encoding" +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} @@ -19,6 +37,30 @@ func (m Map) ToResolvedValues() map[string]string { 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. diff --git a/pkg/storage/migrations/migration.go b/pkg/storage/migrations/migration.go index ec76ade6d..3ede93942 100644 --- a/pkg/storage/migrations/migration.go +++ b/pkg/storage/migrations/migration.go @@ -591,8 +591,7 @@ func convertParameterSet(namespace string, data []byte) (storage.ParameterSet, e }, } - destParams := storage.MakeParameterSourceMap(len(src.Parameters)) - dest.Parameters = &destParams + dest.Parameters = storage.MakeParameterSourceMap(len(src.Parameters)) for _, srcParam := range src.Parameters { destParam := storage.ParameterSource{ Source: secrets.Source{ @@ -600,7 +599,7 @@ func convertParameterSet(namespace string, data []byte) (storage.ParameterSet, e Hint: srcParam.Source.Value, }, } - destParams.Set(srcParam.Name, destParam) + 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 c0a7c3668..37668645e 100644 --- a/pkg/storage/migrations/migration_test.go +++ b/pkg/storage/migrations/migration_test.go @@ -153,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") @@ -185,7 +185,7 @@ 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.Equal(t, 1, lastRun.Parameters.Len(), "expected one parameter set on the run") diff --git a/pkg/storage/parameterset.go b/pkg/storage/parameterset.go index 699a81ca6..2a4e5c144 100644 --- a/pkg/storage/parameterset.go +++ b/pkg/storage/parameterset.go @@ -3,7 +3,6 @@ package storage import ( "context" "fmt" - "get.porter.sh/porter/pkg/encoding" "strings" "time" @@ -45,12 +44,13 @@ type ParameterSetSpec struct { Labels map[string]string `json:"labels,omitempty" yaml:"labels,omitempty" toml:"labels,omitempty"` // Parameters is a list of parameter specs. - Parameters *ParameterSourceMap `json:"parameters" yaml:"parameters" toml:"parameters"` + Parameters ParameterSourceMap `json:"parameters" yaml:"parameters" toml:"parameters"` } -type ParameterSourceMap = encoding.ArrayMap[ParameterSource, NamedParameterSource] +type ParameterSourceMap = secrets.Map -var MakeParameterSourceMap = encoding.MakeArrayMap[ParameterSource, NamedParameterSource] +var MakeParameterSourceMap = secrets.MakeMap +var NewParameterSourceMap = secrets.NewMap // TODO(generics) type ParameterSource = secrets.ValueMapping @@ -74,7 +74,7 @@ func NewParameterSet(namespace string, name string) ParameterSet { SchemaVersion: DefaultParameterSetSchemaVersion, Namespace: namespace, Name: name, - Parameters: &ParameterSourceMap{}, + Parameters: NewParameterSourceMap(), }, Status: ParameterSetStatus{ Created: now, @@ -132,9 +132,6 @@ func (s ParameterSet) String() string { } func (s ParameterSet) Iterate() map[string]ParameterSource { - if s.Parameters == nil { - return nil - } return s.Parameters.Items() } @@ -143,9 +140,6 @@ func (s ParameterSet) SetStrategy(key string, source secrets.Source) { } func (s ParameterSet) Set(key string, source ParameterSource) { - if s.Parameters == nil { // TODO(carolyn): do this in all the places - s.Parameters = &ParameterSourceMap{} - } s.Parameters.Set(key, source) } diff --git a/pkg/storage/run_test.go b/pkg/storage/run_test.go index 096ceeed3..ba9ae573b 100644 --- a/pkg/storage/run_test.go +++ b/pkg/storage/run_test.go @@ -210,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 023645d15..e42d3e39e 100644 --- a/pkg/storage/sanitizer.go +++ b/pkg/storage/sanitizer.go @@ -29,20 +29,20 @@ 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) (*ParameterSourceMap, error) { +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 } strategies.Set(name, secrets.HardCodedValue(stringVal)) } - result, 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 result, nil @@ -53,7 +53,7 @@ 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 *ParameterSourceMap, bun cnab.ExtendedBundle, id string) (*ParameterSourceMap, error) { +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 @@ -61,7 +61,7 @@ func (s *Sanitizer) CleanParameters(ctx context.Context, dirtyParams *ParameterS 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.Set(paramName, cleaned) @@ -71,10 +71,10 @@ func (s *Sanitizer) CleanParameters(ctx context.Context, dirtyParams *ParameterS } if cleanedParams.Len() == 0 { - return nil, nil + return NewParameterSourceMap(), nil } - return &cleanedParams, nil + return cleanedParams, nil } diff --git a/pkg/storage/sanitizer_test.go b/pkg/storage/sanitizer_test.go index a5d578235..2f48d4afb 100644 --- a/pkg/storage/sanitizer_test.go +++ b/pkg/storage/sanitizer_test.go @@ -28,7 +28,7 @@ func TestSanitizer_Parameters(t *testing.T) { recordID := "01FZVC5AVP8Z7A78CSCP1EJ604" sensitiveParamName := "my-second-param" sensitiveParamKey := recordID + "-" + sensitiveParamName - expected := &storage.ParameterSourceMap{} + expected := storage.NewParameterSourceMap() expected.Set("my-first-param", secrets.HardCodedValue("1")) expected.Set(sensitiveParamName, storage.ParameterSource{Source: secrets.Source{ Strategy: secrets.SourceSecret, @@ -108,7 +108,7 @@ func TestSanitizer_CleanParameters(t *testing.T) { gotParams, err := r.Sanitizer.CleanParameters(ctx, inst.Parameters.Parameters, bun, inst.ID) require.NoError(t, err, "CleanParameters failed") - wantParms := &storage.ParameterSourceMap{} + wantParms := storage.NewParameterSourceMap() wantParms.Set(tc.paramName, storage.ParameterSource{Source: tc.wantSource}) require.Equal(t, wantParms, gotParams, "unexpected value returned from CleanParameters") }) From 0af33dbdfd6406602a4675c28aa1e4c8ff28f3d1 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 26 Apr 2023 14:34:55 -0500 Subject: [PATCH 4/4] wip: cs Signed-off-by: Carolyn Van Slyck --- pkg/secrets/map.go | 2 ++ pkg/storage/credentialset.go | 21 ++++++++++----------- pkg/storage/credentialset_test.go | 2 +- pkg/storage/migrations/migration.go | 5 ++--- pkg/storage/parameterset.go | 10 ++++++---- 5 files changed, 21 insertions(+), 19 deletions(-) diff --git a/pkg/secrets/map.go b/pkg/secrets/map.go index a3995a79a..37a277779 100644 --- a/pkg/secrets/map.go +++ b/pkg/secrets/map.go @@ -81,6 +81,8 @@ func (v ValueMapping) ToArrayEntry(key string) encoding.ArrayElement { } } +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. diff --git a/pkg/storage/credentialset.go b/pkg/storage/credentialset.go index 3cde4ebe4..72a962299 100644 --- a/pkg/storage/credentialset.go +++ b/pkg/storage/credentialset.go @@ -7,7 +7,6 @@ import ( "time" "get.porter.sh/porter/pkg/cnab" - "get.porter.sh/porter/pkg/encoding" "get.porter.sh/porter/pkg/schema" "get.porter.sh/porter/pkg/secrets" "get.porter.sh/porter/pkg/tracing" @@ -51,15 +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 *CredentialSourceMap `json:"credentials" yaml:"credentials" toml:"credentials"` + Credentials CredentialSourceMap `json:"credentials" yaml:"credentials" toml:"credentials"` } -type CredentialSourceMap = encoding.ArrayMap[CredentialSource, NamedCredentialSource] - -// TODO(generics) +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. @@ -69,8 +69,6 @@ type CredentialSetStatus struct { Modified time.Time `json:"modified" yaml:"modified" toml:"modified"` } -var MakeCredentialSourceMap = encoding.MakeArrayMap[CredentialSource, NamedCredentialSource] - // NewCredentialSet creates a new CredentialSet with the required fields initialized. func NewCredentialSet(namespace string, name string) CredentialSet { now := time.Now() @@ -80,7 +78,7 @@ func NewCredentialSet(namespace string, name string) CredentialSet { SchemaVersion: DefaultCredentialSetSchemaVersion, Name: name, Namespace: namespace, - Credentials: &CredentialSourceMap{}, + Credentials: NewCredentialSourceMap(), }, Status: CredentialSetStatus{ Created: now, @@ -145,9 +143,6 @@ func (s CredentialSet) SetStrategy(key string, source secrets.Source) { } func (s CredentialSet) Set(key string, source CredentialSource) { - if s.Credentials == nil { - s.Credentials = &CredentialSourceMap{} - } s.Credentials.Set(key, source) } @@ -155,6 +150,10 @@ 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() } diff --git a/pkg/storage/credentialset_test.go b/pkg/storage/credentialset_test.go index caf655148..f91724a8f 100644 --- a/pkg/storage/credentialset_test.go +++ b/pkg/storage/credentialset_test.go @@ -150,7 +150,7 @@ func TestMarshal(t *testing.T) { SchemaVersion: "schemaVersion", Namespace: "namespace", Name: "name", - Credentials: &CredentialSourceMap{}, + Credentials: NewCredentialSourceMap(), }, Status: CredentialSetStatus{ Created: now, diff --git a/pkg/storage/migrations/migration.go b/pkg/storage/migrations/migration.go index 3ede93942..2c39ffda7 100644 --- a/pkg/storage/migrations/migration.go +++ b/pkg/storage/migrations/migration.go @@ -512,8 +512,7 @@ func convertCredentialSet(namespace string, data []byte) (storage.CredentialSet, }, } - destCreds := storage.MakeCredentialSourceMap(len(src.Credentials)) - dest.Credentials = &destCreds + dest.Credentials = storage.MakeCredentialSourceMap(len(src.Credentials)) for _, srcCred := range src.Credentials { destCred := storage.CredentialSource{ Source: secrets.Source{ @@ -521,7 +520,7 @@ func convertCredentialSet(namespace string, data []byte) (storage.CredentialSet, Hint: srcCred.Source.Value, }, } - destCreds.Set(srcCred.Name, destCred) + dest.Credentials.Set(srcCred.Name, destCred) } return dest, nil diff --git a/pkg/storage/parameterset.go b/pkg/storage/parameterset.go index 2a4e5c144..8d6449d5b 100644 --- a/pkg/storage/parameterset.go +++ b/pkg/storage/parameterset.go @@ -48,14 +48,12 @@ type ParameterSetSpec struct { } type ParameterSourceMap = secrets.Map +type ParameterSource = secrets.ValueMapping +type NamedParameterSource = secrets.NamedValueMapping var MakeParameterSourceMap = secrets.MakeMap var NewParameterSourceMap = secrets.NewMap -// TODO(generics) -type ParameterSource = secrets.ValueMapping -type NamedParameterSource = secrets.NamedValueMapping - // ParameterSetStatus contains additional status metadata that has been set by Porter. type ParameterSetStatus struct { // Created timestamp of the parameter set. @@ -135,6 +133,10 @@ 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}) }