From edc822f33e19ab74b99360d9083668c6337056ff Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 19 Apr 2023 11:31:58 -0500 Subject: [PATCH 1/5] Add functionality to SourceMapList Add methods to SourceMapList (the lists of parameters or credentials in a set) for working with the collection using item keys and converting to a map of resolved values Signed-off-by: Carolyn Van Slyck --- pkg/secrets/strategy.go | 134 ++++++++++++++++++++++++++++++----- pkg/secrets/strategy_test.go | 105 +++++++++++++++++++++++---- pkg/storage/credentialset.go | 2 +- pkg/storage/parameterset.go | 2 +- 4 files changed, 208 insertions(+), 35 deletions(-) diff --git a/pkg/secrets/strategy.go b/pkg/secrets/strategy.go index 498328a48..42203db0b 100644 --- a/pkg/secrets/strategy.go +++ b/pkg/secrets/strategy.go @@ -5,7 +5,9 @@ import ( "errors" "fmt" + "get.porter.sh/porter/pkg/cnab" "github.com/cnabio/cnab-go/valuesource" + "github.com/hashicorp/go-multierror" "gopkg.in/yaml.v3" ) @@ -13,20 +15,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 { @@ -130,18 +118,128 @@ func (s Source) MarshalYAML() (interface{}, error) { return s.MarshalRaw(), nil } -type StrategyList []SourceMap +// SourceMapList is a list of mappings that can be access via index or the item name. +type SourceMapList []SourceMap -func (l StrategyList) Less(i, j int) bool { +func (l SourceMapList) Less(i, j int) bool { return l[i].Name < l[j].Name } -func (l StrategyList) Swap(i, j int) { +func (l SourceMapList) Swap(i, j int) { tmp := l[i] l[i] = l[j] l[j] = tmp } -func (l StrategyList) Len() int { +func (l SourceMapList) Len() int { return len(l) } + +// HasName determines if the specified name is defined in the set. +func (l SourceMapList) HasName(name string) bool { + _, ok := l.GetByName(name) + return ok +} + +// GetByName returns the resolution strategy for the specified name and a bool indicating if it was found. +func (l SourceMapList) GetByName(name string) (SourceMap, bool) { + for _, item := range l { + if item.Name == name { + return item, true + } + } + + return SourceMap{}, false +} + +// GetResolvedValue returns the resolved value of the specified name and a bool indicating if it was found. +// You must resolve the value before calling, it does to do resolution for you. +func (l SourceMapList) GetResolvedValue(name string) (interface{}, bool) { + item, ok := l.GetByName(name) + if ok { + return item.ResolvedValue, true + } + + return nil, false +} + +// GetResolvedValues converts the items to a map of key/value pairs, with the values represented as CNAB-compatible strings +func (l SourceMapList) GetResolvedValues() map[string]string { + values := make(map[string]string, len(l)) + for _, item := range l { + values[item.Name] = item.ResolvedValue + } + return values +} + +// GetTypedResolvedValues converts the items to a map of key/value pairs, where the value is the type defined by the bundle. +// Validation errors are accumulated, and parameters are always passed back along with a validation error. +// If you care about valid parameters, check the error, but otherwise you can use this for display purposes and not have it blow up with no results. +func (l SourceMapList) GetTypedResolvedValues(bun cnab.ExtendedBundle) (map[string]interface{}, error) { + var bigErr *multierror.Error + + typedParams := make(map[string]interface{}, len(l)) + for key, unconverted := range l.GetResolvedValues() { + param, ok := bun.Parameters[key] + if !ok { + bigErr = multierror.Append(bigErr, fmt.Errorf("parameter %s not defined in bundle", key)) + typedParams[key] = unconverted + continue + } + + def, ok := bun.Definitions[param.Definition] + if !ok { + bigErr = multierror.Append(bigErr, fmt.Errorf("definition %s not defined in bundle", param.Definition)) + typedParams[key] = unconverted + continue + } + + if def.Type != nil { + value, err := def.ConvertValue(unconverted) + if err != nil { + bigErr = multierror.Append(bigErr, fmt.Errorf("unable to convert parameter's %s value %s to the destination parameter type %s: %w", key, unconverted, def.Type, err)) + typedParams[key] = unconverted + continue + } + + // Inject empty files as nil (no file), not an empty file + if bun.IsFileType(def) && value == "" { + value = nil + } + + typedParams[key] = value + } else { + // bundle dependency parameters can be any type, I'm not sure that we have a solid way to do a typed conversion + typedParams[key] = unconverted + } + } + return typedParams, bigErr.ErrorOrNil() +} + +// 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 (l SourceMapList) Merge(overrides SourceMapList) SourceMapList { + result := l + + // Make a lookup from the name to its index in result so that we can quickly find a + // named item while merging + lookup := make(map[string]int, len(result)) + for i, item := range result { + lookup[item.Name] = i + } + + for _, item := range overrides { + // If the name is in the base, overwrite its value with the override provided + if i, ok := lookup[item.Name]; ok { + result[i].Source = item.Source + + // Just in case the value was resolved, include in the merge results + result[i].ResolvedValue = item.ResolvedValue + } else { + // Append the override to the list of results if it's not in the base set of values + result = append(result, item) + } + } + + return result +} diff --git a/pkg/secrets/strategy_test.go b/pkg/secrets/strategy_test.go index 8e5478fe9..1fedf2a23 100644 --- a/pkg/secrets/strategy_test.go +++ b/pkg/secrets/strategy_test.go @@ -1,30 +1,105 @@ package secrets import ( + "sort" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func TestSet_Merge(t *testing.T) { - set := Set{ - "first": "first", - "second": "second", - "third": "third", +func TestStrategyList_GetResolvedValue(t *testing.T) { + l := SourceMapList{ + SourceMap{Name: "bar", ResolvedValue: "2"}, + SourceMap{Name: "foo", ResolvedValue: "1"}, + } + + fooVal, ok := l.GetResolvedValue("foo") + require.True(t, ok, "foo could not be found") + assert.Equal(t, "1", fooVal, "foo had the incorrect resolved value") + + _, ok = l.GetResolvedValue("missing") + require.False(t, ok, "GetResolvedValue should have returned that missing key was not found") +} + +func TestStrategyList_GetResolvedValues(t *testing.T) { + l := SourceMapList{ + SourceMap{Name: "bar", ResolvedValue: "2"}, + SourceMap{Name: "foo", ResolvedValue: "1"}, + } + + want := map[string]string{ + "bar": "2", + "foo": "1", + } + got := l.GetResolvedValues() + assert.Equal(t, want, got) +} + +func TestStrategyList_HasKey(t *testing.T) { + l := SourceMapList{ + SourceMap{Name: "bar", ResolvedValue: "2"}, + SourceMap{Name: "foo", ResolvedValue: "1"}, + } + + require.True(t, l.HasName("foo"), "HasName returned the wrong value for a key present in the list") + require.False(t, l.HasName("missing"), "HasName returned the wrong value for a missing key") +} + +func TestStrategyList_Sort(t *testing.T) { + l := SourceMapList{ + SourceMap{Name: "c"}, + SourceMap{Name: "a"}, + SourceMap{Name: "b"}, + } + + sort.Sort(l) + + wantResult := SourceMapList{ + SourceMap{Name: "a"}, + SourceMap{Name: "b"}, + SourceMap{Name: "c"}, + } + + require.Len(t, l, 3, "Len is not implemented correctly") + require.Equal(t, wantResult, l, "Sort is not implemented correctly") +} + +func TestStrategyList_GetStrategy(t *testing.T) { + wantToken := SourceMap{Name: "token", Source: Source{Strategy: "env", Hint: "GITHUB_TOKEN"}} + l := SourceMapList{ + SourceMap{Name: "logLevel", Source: Source{Strategy: "value", Hint: "11"}}, + wantToken, + } + + gotToken, ok := l.GetByName("token") + require.True(t, ok, "GetByName did not find 'token'") + assert.Equal(t, wantToken, gotToken, "GetByName returned the wrong 'token' strategy") +} + +func TestStrategyList_Merge(t *testing.T) { + set := SourceMapList{ + SourceMap{Name: "first", ResolvedValue: "base first"}, + SourceMap{Name: "second", ResolvedValue: "base second"}, + SourceMap{Name: "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(SourceMapList{}) + is.Len(result, 3) + _, ok := result.GetResolvedValue("base fourth") + is.False(ok, "forth should not be present in the base set") - err = set.Merge(Set{"fourth": "fourth"}) - is.NoError(err) - is.Len(set, 4) - is.Contains(set, "fourth") + result = set.Merge(SourceMapList{SourceMap{Name: "fourth", ResolvedValue: "new fourth"}}) + is.Len(result, 4) + val, ok := result.GetResolvedValue("fourth") + is.True(ok, "fourth should be present when merged as an override") + is.Equal("new fourth", val, "incorrect merged value") - err = set.Merge(Set{"second": "bis"}) - is.EqualError(err, `ambiguous value resolution: "second" is already present in base sets, cannot merge`) + result = set.Merge(SourceMapList{SourceMap{Name: "second", ResolvedValue: "override second"}}) + is.Len(result, 3) + val, ok = result.GetResolvedValue("second") + is.True(ok, "second should be overwritten when an override value is merged") + is.Equal("override second", val, "incorrect merged value") } diff --git a/pkg/storage/credentialset.go b/pkg/storage/credentialset.go index 651fd5252..07bc0577a 100644 --- a/pkg/storage/credentialset.go +++ b/pkg/storage/credentialset.go @@ -50,7 +50,7 @@ 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 secrets.SourceMapList `json:"credentials" yaml:"credentials" toml:"credentials"` } // CredentialSetStatus contains additional status metadata that has been set by Porter. diff --git a/pkg/storage/parameterset.go b/pkg/storage/parameterset.go index 66f104c8c..c31ffeecf 100644 --- a/pkg/storage/parameterset.go +++ b/pkg/storage/parameterset.go @@ -44,7 +44,7 @@ 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 secrets.SourceMapList `json:"parameters" yaml:"parameters" toml:"parameters"` } // ParameterSetStatus contains additional status metadata that has been set by Porter. From 8ab663eb8517a9d12c9e94dc6df2cb659e12a277 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 19 Apr 2023 15:35:29 -0500 Subject: [PATCH 2/5] Fix typo Signed-off-by: Carolyn Van Slyck --- pkg/secrets/strategy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/secrets/strategy.go b/pkg/secrets/strategy.go index 42203db0b..0494d98bd 100644 --- a/pkg/secrets/strategy.go +++ b/pkg/secrets/strategy.go @@ -153,7 +153,7 @@ func (l SourceMapList) GetByName(name string) (SourceMap, bool) { } // GetResolvedValue returns the resolved value of the specified name and a bool indicating if it was found. -// You must resolve the value before calling, it does to do resolution for you. +// You must resolve the value before calling, it does not do resolution for you. func (l SourceMapList) GetResolvedValue(name string) (interface{}, bool) { item, ok := l.GetByName(name) if ok { From 762fc59eba3558945a1dfcb22171183f4e6827e6 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 19 Apr 2023 15:39:13 -0500 Subject: [PATCH 3/5] Rename tests with new name Signed-off-by: Carolyn Van Slyck --- pkg/secrets/strategy_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/secrets/strategy_test.go b/pkg/secrets/strategy_test.go index 1fedf2a23..cddd99ca8 100644 --- a/pkg/secrets/strategy_test.go +++ b/pkg/secrets/strategy_test.go @@ -8,7 +8,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestStrategyList_GetResolvedValue(t *testing.T) { +func TestSourceMapList_GetResolvedValue(t *testing.T) { l := SourceMapList{ SourceMap{Name: "bar", ResolvedValue: "2"}, SourceMap{Name: "foo", ResolvedValue: "1"}, @@ -22,7 +22,7 @@ func TestStrategyList_GetResolvedValue(t *testing.T) { require.False(t, ok, "GetResolvedValue should have returned that missing key was not found") } -func TestStrategyList_GetResolvedValues(t *testing.T) { +func TestSourceMapList_GetResolvedValues(t *testing.T) { l := SourceMapList{ SourceMap{Name: "bar", ResolvedValue: "2"}, SourceMap{Name: "foo", ResolvedValue: "1"}, @@ -36,7 +36,7 @@ func TestStrategyList_GetResolvedValues(t *testing.T) { assert.Equal(t, want, got) } -func TestStrategyList_HasKey(t *testing.T) { +func TestSourceMapList_HasKey(t *testing.T) { l := SourceMapList{ SourceMap{Name: "bar", ResolvedValue: "2"}, SourceMap{Name: "foo", ResolvedValue: "1"}, @@ -46,7 +46,7 @@ func TestStrategyList_HasKey(t *testing.T) { require.False(t, l.HasName("missing"), "HasName returned the wrong value for a missing key") } -func TestStrategyList_Sort(t *testing.T) { +func TestSourceMapList_Sort(t *testing.T) { l := SourceMapList{ SourceMap{Name: "c"}, SourceMap{Name: "a"}, @@ -65,7 +65,7 @@ func TestStrategyList_Sort(t *testing.T) { require.Equal(t, wantResult, l, "Sort is not implemented correctly") } -func TestStrategyList_GetStrategy(t *testing.T) { +func TestSourceMapList_GetStrategy(t *testing.T) { wantToken := SourceMap{Name: "token", Source: Source{Strategy: "env", Hint: "GITHUB_TOKEN"}} l := SourceMapList{ SourceMap{Name: "logLevel", Source: Source{Strategy: "value", Hint: "11"}}, @@ -77,7 +77,7 @@ func TestStrategyList_GetStrategy(t *testing.T) { assert.Equal(t, wantToken, gotToken, "GetByName returned the wrong 'token' strategy") } -func TestStrategyList_Merge(t *testing.T) { +func TestSourceMapList_Merge(t *testing.T) { set := SourceMapList{ SourceMap{Name: "first", ResolvedValue: "base first"}, SourceMap{Name: "second", ResolvedValue: "base second"}, From 0ec032244e0328494c3cd3170b89e964e0af8c15 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 19 Apr 2023 15:43:33 -0500 Subject: [PATCH 4/5] Remove GetTypedResolvedValues GetTypedResolvedValues needs to lookup definitions by parameter name, which is too specific for where I've defined the function. I will move it to ParameterSet in another PR. Signed-off-by: Carolyn Van Slyck --- pkg/secrets/strategy.go | 46 ----------------------------------------- 1 file changed, 46 deletions(-) diff --git a/pkg/secrets/strategy.go b/pkg/secrets/strategy.go index 0494d98bd..e8fe6aca0 100644 --- a/pkg/secrets/strategy.go +++ b/pkg/secrets/strategy.go @@ -5,9 +5,7 @@ import ( "errors" "fmt" - "get.porter.sh/porter/pkg/cnab" "github.com/cnabio/cnab-go/valuesource" - "github.com/hashicorp/go-multierror" "gopkg.in/yaml.v3" ) @@ -172,50 +170,6 @@ func (l SourceMapList) GetResolvedValues() map[string]string { return values } -// GetTypedResolvedValues converts the items to a map of key/value pairs, where the value is the type defined by the bundle. -// Validation errors are accumulated, and parameters are always passed back along with a validation error. -// If you care about valid parameters, check the error, but otherwise you can use this for display purposes and not have it blow up with no results. -func (l SourceMapList) GetTypedResolvedValues(bun cnab.ExtendedBundle) (map[string]interface{}, error) { - var bigErr *multierror.Error - - typedParams := make(map[string]interface{}, len(l)) - for key, unconverted := range l.GetResolvedValues() { - param, ok := bun.Parameters[key] - if !ok { - bigErr = multierror.Append(bigErr, fmt.Errorf("parameter %s not defined in bundle", key)) - typedParams[key] = unconverted - continue - } - - def, ok := bun.Definitions[param.Definition] - if !ok { - bigErr = multierror.Append(bigErr, fmt.Errorf("definition %s not defined in bundle", param.Definition)) - typedParams[key] = unconverted - continue - } - - if def.Type != nil { - value, err := def.ConvertValue(unconverted) - if err != nil { - bigErr = multierror.Append(bigErr, fmt.Errorf("unable to convert parameter's %s value %s to the destination parameter type %s: %w", key, unconverted, def.Type, err)) - typedParams[key] = unconverted - continue - } - - // Inject empty files as nil (no file), not an empty file - if bun.IsFileType(def) && value == "" { - value = nil - } - - typedParams[key] = value - } else { - // bundle dependency parameters can be any type, I'm not sure that we have a solid way to do a typed conversion - typedParams[key] = unconverted - } - } - return typedParams, bigErr.ErrorOrNil() -} - // 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 (l SourceMapList) Merge(overrides SourceMapList) SourceMapList { From 23034f50f3df9df269d123456244d7b0056fc0a8 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Thu, 20 Apr 2023 15:43:09 -0500 Subject: [PATCH 5/5] Rename GetResolvedValues to ToResolvedValues Disambiguate GetResolvedValues and GetResolvedValue by renaming the converter function GetResolvedValues to ToResolvedValues, making it more clear that it converts the current struct into a new representation and doesn't just fetch data. Signed-off-by: Carolyn Van Slyck --- pkg/secrets/strategy.go | 4 ++-- pkg/secrets/strategy_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/secrets/strategy.go b/pkg/secrets/strategy.go index e8fe6aca0..c475a9d3e 100644 --- a/pkg/secrets/strategy.go +++ b/pkg/secrets/strategy.go @@ -161,8 +161,8 @@ func (l SourceMapList) GetResolvedValue(name string) (interface{}, bool) { return nil, false } -// GetResolvedValues converts the items to a map of key/value pairs, with the values represented as CNAB-compatible strings -func (l SourceMapList) GetResolvedValues() map[string]string { +// ToResolvedValues converts the items to a map of key/value pairs, with the resolved values represented as CNAB-compatible strings +func (l SourceMapList) ToResolvedValues() map[string]string { values := make(map[string]string, len(l)) for _, item := range l { values[item.Name] = item.ResolvedValue diff --git a/pkg/secrets/strategy_test.go b/pkg/secrets/strategy_test.go index cddd99ca8..da00d7f66 100644 --- a/pkg/secrets/strategy_test.go +++ b/pkg/secrets/strategy_test.go @@ -32,7 +32,7 @@ func TestSourceMapList_GetResolvedValues(t *testing.T) { "bar": "2", "foo": "1", } - got := l.GetResolvedValues() + got := l.ToResolvedValues() assert.Equal(t, want, got) }