From e1b04fa39478fe4da2543f17c93f23affb75bd90 Mon Sep 17 00:00:00 2001 From: Kim Christensen Date: Sun, 29 Dec 2024 00:15:13 +0100 Subject: [PATCH 1/6] fix(credentials): optimize credential resolution by filtering unused credentials - Update ResolveAll to accept a list of required credential keys - Add Keys() method to CredentialSet to get list of credential names - Filter credentials during resolution to only process required ones - Update credential provider interface and implementations Signed-off-by: Kim Christensen --- pkg/cnab/provider/credentials.go | 2 +- pkg/storage/credential_store.go | 7 ++++++- pkg/storage/credentialset.go | 9 +++++++++ pkg/storage/credentialset_provider.go | 2 +- 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/pkg/cnab/provider/credentials.go b/pkg/cnab/provider/credentials.go index 44cec328f..6821ded40 100644 --- a/pkg/cnab/provider/credentials.go +++ b/pkg/cnab/provider/credentials.go @@ -12,7 +12,7 @@ func (r *Runtime) loadCredentials(ctx context.Context, b cnab.ExtendedBundle, ru ctx, span := tracing.StartSpan(ctx) defer span.EndSpan() - resolvedCredentials, err := r.credentials.ResolveAll(ctx, run.Credentials) + resolvedCredentials, err := r.credentials.ResolveAll(ctx, run.Credentials, run.Credentials.Keys()) if err != nil { return span.Error(err) } diff --git a/pkg/storage/credential_store.go b/pkg/storage/credential_store.go index d0a177402..cd23a813f 100644 --- a/pkg/storage/credential_store.go +++ b/pkg/storage/credential_store.go @@ -3,6 +3,7 @@ package storage import ( "context" "fmt" + "slices" "strings" "get.porter.sh/porter/pkg/secrets" @@ -60,11 +61,15 @@ func (s CredentialStore) GetDataStore() Store { Secrets */ -func (s CredentialStore) ResolveAll(ctx context.Context, creds CredentialSet) (secrets.Set, error) { +func (s CredentialStore) ResolveAll(ctx context.Context, creds CredentialSet, keys []string) (secrets.Set, error) { resolvedCreds := make(secrets.Set) var resolveErrors error for _, cred := range creds.Credentials { + if !slices.Contains(keys, cred.Name) { + continue + } + var value string var err error if isHandledByHostPlugin(cred.Source.Strategy) { diff --git a/pkg/storage/credentialset.go b/pkg/storage/credentialset.go index 932bdb689..54bc8d52b 100644 --- a/pkg/storage/credentialset.go +++ b/pkg/storage/credentialset.go @@ -185,3 +185,12 @@ func (s CredentialSet) ValidateBundle(spec map[string]bundle.Credential, action } return nil } + +// Keys returns the names of all the credentials in the set. +func (s CredentialSet) Keys() []string { + keys := make([]string, len(s.Credentials)) + for _, cred := range s.Credentials { + keys = append(keys, cred.Name) + } + return keys +} diff --git a/pkg/storage/credentialset_provider.go b/pkg/storage/credentialset_provider.go index cfa83eb37..2df8944eb 100644 --- a/pkg/storage/credentialset_provider.go +++ b/pkg/storage/credentialset_provider.go @@ -9,7 +9,7 @@ import ( // CredentialSetProvider is Porter's interface for managing and resolving credentials. type CredentialSetProvider interface { GetDataStore() Store - ResolveAll(ctx context.Context, creds CredentialSet) (secrets.Set, error) + ResolveAll(ctx context.Context, creds CredentialSet, keys []string) (secrets.Set, error) Validate(ctx context.Context, creds CredentialSet) error InsertCredentialSet(ctx context.Context, creds CredentialSet) error ListCredentialSets(ctx context.Context, listOptions ListOptions) ([]CredentialSet, error) From f83abffffb99fa1b3d7837f2a225abca07214391 Mon Sep 17 00:00:00 2001 From: Kim Christensen Date: Sun, 29 Dec 2024 00:16:56 +0100 Subject: [PATCH 2/6] perf(parameters): optimize parameter resolution by filtering unused keys - Add Keys() method to ParameterSet to get parameter names - Update ResolveAll signature to accept list of required parameter keys - Modify parameter resolution to skip parameters not in requested keys - Update all parameter resolution calls to pass required keys Signed-off-by: Kim Christensen --- pkg/cnab/provider/parameters.go | 2 +- pkg/porter/lifecycle_test.go | 2 +- pkg/porter/parameters.go | 4 ++-- pkg/storage/parameter_store.go | 7 ++++++- pkg/storage/parameter_store_test.go | 22 +++++++++++++++++++--- pkg/storage/parameterset.go | 9 +++++++++ pkg/storage/parameterset_provider.go | 2 +- pkg/storage/sanitizer.go | 2 +- 8 files changed, 40 insertions(+), 10 deletions(-) diff --git a/pkg/cnab/provider/parameters.go b/pkg/cnab/provider/parameters.go index 377bd84c5..0fec12f5c 100644 --- a/pkg/cnab/provider/parameters.go +++ b/pkg/cnab/provider/parameters.go @@ -14,7 +14,7 @@ func (r *Runtime) loadParameters(ctx context.Context, b cnab.ExtendedBundle, run ctx, span := tracing.StartSpan(ctx) defer span.EndSpan() - resolvedParameters, err := r.parameters.ResolveAll(ctx, run.Parameters) + resolvedParameters, err := r.parameters.ResolveAll(ctx, run.Parameters, run.Parameters.Keys()) if err != nil { return err } diff --git a/pkg/porter/lifecycle_test.go b/pkg/porter/lifecycle_test.go index 9d1d2878c..c1752a2e5 100644 --- a/pkg/porter/lifecycle_test.go +++ b/pkg/porter/lifecycle_test.go @@ -532,7 +532,7 @@ func TestPorter_applyActionOptionsToInstallation_PreservesExistingParams(t *test 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) + params, err := p.Parameters.ResolveAll(ctx, i.Parameters, i.Parameters.Keys()) require.NoError(t, err, "Failed to resolve the installation parameters") require.Equal(t, secrets.Set{ "my-first-param": "3", // Should have used the override diff --git a/pkg/porter/parameters.go b/pkg/porter/parameters.go index 630372d84..5027eb97c 100644 --- a/pkg/porter/parameters.go +++ b/pkg/porter/parameters.go @@ -402,7 +402,7 @@ func (p *Porter) loadParameterSets(ctx context.Context, bun cnab.ExtendedBundle, } } - rc, err := p.Parameters.ResolveAll(ctx, pset) + rc, err := p.Parameters.ResolveAll(ctx, pset, pset.Keys()) if err != nil { return nil, err } @@ -894,7 +894,7 @@ func (p *Porter) applyActionOptionsToInstallation(ctx context.Context, ba Bundle // // 4. Resolve the installation's internal parameter set - resolvedOverrides, err := p.Parameters.ResolveAll(ctx, inst.Parameters) + resolvedOverrides, err := p.Parameters.ResolveAll(ctx, inst.Parameters, inst.Parameters.Keys()) if err != nil { return err } diff --git a/pkg/storage/parameter_store.go b/pkg/storage/parameter_store.go index c0f885ee8..fe975e0da 100644 --- a/pkg/storage/parameter_store.go +++ b/pkg/storage/parameter_store.go @@ -3,6 +3,7 @@ package storage import ( "context" "fmt" + "slices" "strings" "get.porter.sh/porter/pkg/secrets" @@ -54,11 +55,15 @@ func (s ParameterStore) GetDataStore() Store { return s.Documents } -func (s ParameterStore) ResolveAll(ctx context.Context, params ParameterSet) (secrets.Set, error) { +func (s ParameterStore) ResolveAll(ctx context.Context, params ParameterSet, keys []string) (secrets.Set, error) { resolvedParams := make(secrets.Set) var resolveErrors error for _, param := range params.Parameters { + if !slices.Contains(keys, param.Name) { + continue + } + var value string var err error if isHandledByHostPlugin(param.Source.Strategy) { diff --git a/pkg/storage/parameter_store_test.go b/pkg/storage/parameter_store_test.go index b0644442e..fc4846cd2 100644 --- a/pkg/storage/parameter_store_test.go +++ b/pkg/storage/parameter_store_test.go @@ -117,7 +117,7 @@ func TestParameterStorage_ResolveNonSecret(t *testing.T) { "param2": "param2_value", } - resolved, err := paramStore.ResolveAll(context.Background(), testParameterSet) + resolved, err := paramStore.ResolveAll(context.Background(), testParameterSet, testParameterSet.Keys()) require.NoError(t, err) require.Equal(t, expected, resolved) } @@ -153,7 +153,23 @@ func TestParameterStorage_ResolveAll(t *testing.T) { "param2": "param2_value", } - resolved, err := paramStore.ResolveAll(context.Background(), testParameterSet) + resolved, err := paramStore.ResolveAll(context.Background(), testParameterSet, testParameterSet.Keys()) + require.NoError(t, err) + require.Equal(t, expected, resolved) + }) + + t.Run("resolve params only resolves the requested keys", func(t *testing.T) { + paramStore := NewTestParameterProvider(t) + defer paramStore.Close() + + paramStore.AddSecret("param1", "param1_value") + paramStore.AddSecret("param2", "param2_value") + + expected := secrets.Set{ + "param1": "param1_value", + } + + resolved, err := paramStore.ResolveAll(context.Background(), testParameterSet, []string{"param1"}) require.NoError(t, err) require.Equal(t, expected, resolved) }) @@ -170,7 +186,7 @@ func TestParameterStorage_ResolveAll(t *testing.T) { "param2": "", } - resolved, err := paramStore.ResolveAll(context.Background(), testParameterSet) + resolved, err := paramStore.ResolveAll(context.Background(), testParameterSet, testParameterSet.Keys()) require.EqualError(t, err, "1 error occurred:\n\t* unable to resolve parameter myparamset.param2 from secret param2: secret not found\n\n") require.Equal(t, expected, resolved) }) diff --git a/pkg/storage/parameterset.go b/pkg/storage/parameterset.go index 437f5ceed..80d7e0d45 100644 --- a/pkg/storage/parameterset.go +++ b/pkg/storage/parameterset.go @@ -154,3 +154,12 @@ func (s ParameterSet) ValidateBundle(spec map[string]bundle.Parameter, action st } return nil } + +// Keys returns the names of all the parameters in the set. +func (s ParameterSet) Keys() []string { + keys := make([]string, len(s.Parameters)) + for _, param := range s.Parameters { + keys = append(keys, param.Name) + } + return keys +} diff --git a/pkg/storage/parameterset_provider.go b/pkg/storage/parameterset_provider.go index 64b35c3d3..489012cf2 100644 --- a/pkg/storage/parameterset_provider.go +++ b/pkg/storage/parameterset_provider.go @@ -11,7 +11,7 @@ type ParameterSetProvider interface { GetDataStore() Store // ResolveAll parameter values in the parameter set. - ResolveAll(ctx context.Context, params ParameterSet) (secrets.Set, error) + ResolveAll(ctx context.Context, params ParameterSet, keys []string) (secrets.Set, error) // Validate the parameter set is defined properly. Validate(ctx context.Context, params ParameterSet) error diff --git a/pkg/storage/sanitizer.go b/pkg/storage/sanitizer.go index 45e9c0d57..645fe6808 100644 --- a/pkg/storage/sanitizer.go +++ b/pkg/storage/sanitizer.go @@ -101,7 +101,7 @@ func sanitizedParam(param secrets.SourceMap, id string) secrets.SourceMap { // RestoreParameterSet resolves the raw parameter data from a secrets store. func (s *Sanitizer) RestoreParameterSet(ctx context.Context, pset ParameterSet, bun cnab.ExtendedBundle) (map[string]interface{}, error) { - params, err := s.parameter.ResolveAll(ctx, pset) + params, err := s.parameter.ResolveAll(ctx, pset, pset.Keys()) if err != nil { return nil, err } From 3f78573e86d5f9047e6fcd2ce985159612edaf98 Mon Sep 17 00:00:00 2001 From: Kim Christensen Date: Tue, 31 Dec 2024 01:14:35 +0100 Subject: [PATCH 3/6] refactor(credentials): improve credential resolution error handling and lookup - Add GetCredential helper method to CredentialSet for safe credential lookup - Remove slices dependency in favor of direct credential lookup - Add tracing spans for better observability - Improve error messages with proper error wrapping - Change resolution logic to iterate through requested keys instead of all credentials Signed-off-by: Kim Christensen --- pkg/storage/credential_store.go | 14 +++++++++----- pkg/storage/credentialset.go | 10 ++++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/pkg/storage/credential_store.go b/pkg/storage/credential_store.go index cd23a813f..20c6f2c0d 100644 --- a/pkg/storage/credential_store.go +++ b/pkg/storage/credential_store.go @@ -3,7 +3,6 @@ package storage import ( "context" "fmt" - "slices" "strings" "get.porter.sh/porter/pkg/secrets" @@ -62,11 +61,16 @@ func (s CredentialStore) GetDataStore() Store { */ func (s CredentialStore) ResolveAll(ctx context.Context, creds CredentialSet, keys []string) (secrets.Set, error) { + ctx, span := tracing.StartSpan(ctx) + defer span.EndSpan() + resolvedCreds := make(secrets.Set) - var resolveErrors error + var resolveErrors *multierror.Error - for _, cred := range creds.Credentials { - if !slices.Contains(keys, cred.Name) { + for _, key := range keys { + cred, ok := creds.GetCredential(key) + if !ok { + resolveErrors = multierror.Append(resolveErrors, span.Errorf("credential %s not found", key)) continue } @@ -78,7 +82,7 @@ func (s CredentialStore) ResolveAll(ctx context.Context, creds CredentialSet, ke 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, span.Errorf("unable to resolve credential %s.%s from %s %s: %w", creds.Name, cred.Name, cred.Source.Strategy, cred.Source.Hint, err)) } resolvedCreds[cred.Name] = value diff --git a/pkg/storage/credentialset.go b/pkg/storage/credentialset.go index 54bc8d52b..dd4ac2a8e 100644 --- a/pkg/storage/credentialset.go +++ b/pkg/storage/credentialset.go @@ -194,3 +194,13 @@ func (s CredentialSet) Keys() []string { } return keys } + +// GetCredential returns the credential with the given name, and a boolean indicating if it was found. +func (s CredentialSet) GetCredential(name string) (secrets.SourceMap, bool) { + for _, cred := range s.Credentials { + if cred.Name == name { + return cred, true + } + } + return secrets.SourceMap{}, false +} From b61631f9f6c66fe1beb0ee24cba996a2a45a89a7 Mon Sep 17 00:00:00 2001 From: Kim Christensen Date: Tue, 31 Dec 2024 01:19:27 +0100 Subject: [PATCH 4/6] refactor(credentials): modify GetCredential to return pointer for in-place updates - Change GetCredential signature to return *secrets.SourceMap instead of value - Update loadCredentials to use Keys() iterator for credential lookup - Add explicit error handling for missing credentials - Fix credential value updates to modify original values via pointers Signed-off-by: Kim Christensen --- pkg/cnab/provider/credentials.go | 8 ++++++-- pkg/storage/credentialset.go | 6 +++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/pkg/cnab/provider/credentials.go b/pkg/cnab/provider/credentials.go index 6821ded40..12ba9b0f4 100644 --- a/pkg/cnab/provider/credentials.go +++ b/pkg/cnab/provider/credentials.go @@ -17,8 +17,12 @@ func (r *Runtime) loadCredentials(ctx context.Context, b cnab.ExtendedBundle, ru return span.Error(err) } - for i, cred := range run.Credentials.Credentials { - run.Credentials.Credentials[i].ResolvedValue = resolvedCredentials[cred.Name] + for _, key := range run.Credentials.Keys() { + cred, ok := run.Credentials.GetCredential(key) + if !ok { + return span.Errorf("credential %s not found", key) + } + cred.ResolvedValue = resolvedCredentials[key] } err = run.Credentials.ValidateBundle(b.Credentials, run.Action) diff --git a/pkg/storage/credentialset.go b/pkg/storage/credentialset.go index dd4ac2a8e..4e797cce4 100644 --- a/pkg/storage/credentialset.go +++ b/pkg/storage/credentialset.go @@ -196,11 +196,11 @@ func (s CredentialSet) Keys() []string { } // GetCredential returns the credential with the given name, and a boolean indicating if it was found. -func (s CredentialSet) GetCredential(name string) (secrets.SourceMap, bool) { +func (s CredentialSet) GetCredential(name string) (*secrets.SourceMap, bool) { for _, cred := range s.Credentials { if cred.Name == name { - return cred, true + return &cred, true } } - return secrets.SourceMap{}, false + return nil, false } From d7910292c560a92c7daeb9b146b93f85c7145377 Mon Sep 17 00:00:00 2001 From: Kim Christensen Date: Thu, 2 Jan 2025 09:18:58 +0100 Subject: [PATCH 5/6] fix(credentials): return nil instead of empty error group - Update ResolveAll to return ErrorOrNil() instead of raw error group - Ensures consistent error handling when no errors occurred during resolution Signed-off-by: Kim Christensen --- pkg/storage/credential_store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/storage/credential_store.go b/pkg/storage/credential_store.go index 20c6f2c0d..deb68e84c 100644 --- a/pkg/storage/credential_store.go +++ b/pkg/storage/credential_store.go @@ -88,7 +88,7 @@ func (s CredentialStore) ResolveAll(ctx context.Context, creds CredentialSet, ke resolvedCreds[cred.Name] = value } - return resolvedCreds, resolveErrors + return resolvedCreds, resolveErrors.ErrorOrNil() } func (s CredentialStore) Validate(ctx context.Context, creds CredentialSet) error { From be69c9dbae4ce2a7c92ae1a5fa663e522216769d Mon Sep 17 00:00:00 2001 From: Kim Christensen Date: Thu, 2 Jan 2025 10:04:36 +0100 Subject: [PATCH 6/6] fix: Fix unit tests Signed-off-by: Kim Christensen --- pkg/cnab/provider/credentials.go | 8 +++----- pkg/storage/credentialset.go | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/pkg/cnab/provider/credentials.go b/pkg/cnab/provider/credentials.go index 12ba9b0f4..3fff0eed7 100644 --- a/pkg/cnab/provider/credentials.go +++ b/pkg/cnab/provider/credentials.go @@ -17,12 +17,10 @@ func (r *Runtime) loadCredentials(ctx context.Context, b cnab.ExtendedBundle, ru return span.Error(err) } - for _, key := range run.Credentials.Keys() { - cred, ok := run.Credentials.GetCredential(key) - if !ok { - return span.Errorf("credential %s not found", key) + for i, cred := range run.Credentials.Credentials { + if resolvedValue, ok := resolvedCredentials[cred.Name]; ok { + run.Credentials.Credentials[i].ResolvedValue = resolvedValue } - cred.ResolvedValue = resolvedCredentials[key] } err = run.Credentials.ValidateBundle(b.Credentials, run.Action) diff --git a/pkg/storage/credentialset.go b/pkg/storage/credentialset.go index 4e797cce4..ed0779d98 100644 --- a/pkg/storage/credentialset.go +++ b/pkg/storage/credentialset.go @@ -188,7 +188,7 @@ func (s CredentialSet) ValidateBundle(spec map[string]bundle.Credential, action // Keys returns the names of all the credentials in the set. func (s CredentialSet) Keys() []string { - keys := make([]string, len(s.Credentials)) + keys := make([]string, 0, len(s.Credentials)) for _, cred := range s.Credentials { keys = append(keys, cred.Name) }