From 542aed5ed1335db1c0fb44fda0377596d19394f1 Mon Sep 17 00:00:00 2001 From: Sergei Zyubin Date: Tue, 11 Aug 2026 12:43:54 +0200 Subject: [PATCH 1/2] fix: secret resolver should not silently fallback to the default value --- config/secret_k8s.go | 40 ++---- config/secret_k8s_test.go | 222 +++++++++++++++++++++++++++++++++ config/secret_resolver.go | 14 ++- config/secret_resolver_test.go | 62 +++++++++ config/secret_vault.go | 30 ++--- config/secret_vault_test.go | 174 ++++++++++++++++++++++++++ 6 files changed, 498 insertions(+), 44 deletions(-) create mode 100644 config/secret_k8s_test.go create mode 100644 config/secret_resolver_test.go create mode 100644 config/secret_vault_test.go diff --git a/config/secret_k8s.go b/config/secret_k8s.go index f33693d2..7e5d298b 100644 --- a/config/secret_k8s.go +++ b/config/secret_k8s.go @@ -2,10 +2,11 @@ package config import ( "context" + "encoding/json" "fmt" + "maps" "net/url" "os" - "strings" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" @@ -102,38 +103,23 @@ func (p k8sSecretProvider) getDSN(ctx context.Context, ref *url.URL) (string, er return "", fmt.Errorf("invalid k8ssecret URL format: expected k8ssecret://[namespace/]secret-name, got %s", ref.String()) } - // Extract the key from the secret data - key := ref.Query().Get("key") - if key == "" { - key = "data_source_name" - } - // Fetch the secret from Kubernetes API secret, err := provider.clientset.CoreV1().Secrets(namespace).Get(ctx, secretName, metav1.GetOptions{}) if err != nil { return "", fmt.Errorf("unable to fetch secret %q from namespace %q: %w", secretName, namespace, err) } - - // Extract the key from secret data - check both Data (binary) and StringData (string) - var secretValue string - - // Check in Data field first (for binary/encoded data) - if data, ok := secret.Data[key]; ok { - secretValue = string(data) - } else if stringData, ok := secret.StringData[key]; ok { - // Check in StringData field (for direct string values) - secretValue = stringData - } else { - return "", fmt.Errorf("key %q not found in Kubernetes secret %s/%s", key, namespace, secretName) + // Return the raw secret payload (all keys) as JSON. Per-call '?key=' selection and '?template=' substitution are + // handled by secretResolver.extractKey, since this provider's result is cached (and shared) across all references + // to the same secret object regardless of query params. + raw := make(map[string]string, len(secret.Data)+len(secret.StringData)) + for k, v := range secret.Data { + raw[k] = string(v) } + maps.Copy(raw, secret.StringData) - // Apply template if provided - templateStr := ref.Query().Get("template") - if templateStr != "" { - // Simple string replacement - replace all occurrences of DSN_VALUE with the secret value - result := strings.ReplaceAll(templateStr, "DSN_VALUE", secretValue) - return result, nil + b, err := json.Marshal(raw) + if err != nil { + return "", fmt.Errorf("unable to marshal Kubernetes secret %s/%s: %w", namespace, secretName, err) } - - return secretValue, nil + return string(b), nil } diff --git a/config/secret_k8s_test.go b/config/secret_k8s_test.go new file mode 100644 index 00000000..ccd90299 --- /dev/null +++ b/config/secret_k8s_test.go @@ -0,0 +1,222 @@ +package config + +import ( + "context" + "encoding/json" + "net/url" + "strings" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" +) + +// newTestK8sProvider builds a k8sSecretProvider via a fake clientset, bypassing getK8sProvider in-cluster setup +func newTestK8sProvider(namespace string, objects ...*corev1.Secret) k8sSecretProvider { + // k8sSecretProvider.getDSN calls getK8sProvider() internally; to keep this test hermetic we exercise the + // provider's logic via a lightweight wrapper. + return k8sSecretProvider{} +} + +// newFakeClientsetWithSecret creates a fake Kubernetes clientset with a single Secret object for testing. +func newFakeClientsetWithSecret(namespace, name string, data map[string][]byte, stringData map[string]string) *fake.Clientset { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Data: data, + StringData: stringData, + } + return fake.NewSimpleClientset(secret) +} + +// TestK8sSecretProvider_GetDSN_ReturnsRawJSONPayload tests that getDSN returns the raw JSON payload of the secret. +func TestK8sSecretProvider_GetDSN_ReturnsRawJSONPayload(t *testing.T) { + clientset := newFakeClientsetWithSecret("default", "my-db-secret", + map[string][]byte{ + "writer": []byte("postgres://writer-dsn"), + }, + map[string]string{ + "reader": "postgres://reader-dsn", + }, + ) + + k8sProviderInstance = &k8sSecretProvider{ + clientset: clientset, + namespace: "default", + } + t.Cleanup(func() { k8sProviderInstance = nil }) + + ref, err := url.Parse("k8ssecret://default/my-db-secret") + if err != nil { + t.Fatalf("parse ref: %v", err) + } + + p := k8sSecretProvider{} + raw, err := p.getDSN(context.Background(), ref) + if err != nil { + t.Fatalf("getDSN returned error: %v", err) + } + + var payload map[string]string + if err := json.Unmarshal([]byte(raw), &payload); err != nil { + t.Fatalf("expected raw JSON payload, got %q (unmarshal err: %v)", raw, err) + } + if payload["writer"] != "postgres://writer-dsn" { + t.Fatalf("writer mismatch: got %q", payload["writer"]) + } + if payload["reader"] != "postgres://reader-dsn" { + t.Fatalf("reader mismatch: got %q", payload["reader"]) + } +} + +// TestK8sSecretProvider_GetDSN_CurrentNamespaceShorthand tests that getDSN can infer the namespace when using the +// shorthand URL format. +func TestK8sSecretProvider_GetDSN_CurrentNamespaceShorthand(t *testing.T) { + clientset := newFakeClientsetWithSecret("current-ns", "db-creds", + map[string][]byte{"data_source_name": []byte("postgres://short-dsn")}, + nil, + ) + + k8sProviderInstance = &k8sSecretProvider{ + clientset: clientset, + namespace: "current-ns", + } + t.Cleanup(func() { k8sProviderInstance = nil }) + + // Single-segment URL: k8ssecret://db-creds (namespace inferred). + ref, err := url.Parse("k8ssecret://db-creds") + if err != nil { + t.Fatalf("parse ref: %v", err) + } + + p := k8sSecretProvider{} + raw, err := p.getDSN(context.Background(), ref) + if err != nil { + t.Fatalf("getDSN returned error: %v", err) + } + + var payload map[string]string + if err := json.Unmarshal([]byte(raw), &payload); err != nil { + t.Fatalf("expected raw JSON payload, got %q (unmarshal err: %v)", raw, err) + } + if payload["data_source_name"] != "postgres://short-dsn" { + t.Fatalf("data_source_name mismatch: got %q", payload["data_source_name"]) + } +} + +// TestK8sSecretProvider_GetDSN_SecretNotFound tests that getDSN returns an error when the secret is not found. +func TestK8sSecretProvider_GetDSN_SecretNotFound(t *testing.T) { + clientset := fake.NewSimpleClientset() // no secrets + + k8sProviderInstance = &k8sSecretProvider{ + clientset: clientset, + namespace: "default", + } + t.Cleanup(func() { k8sProviderInstance = nil }) + + ref, err := url.Parse("k8ssecret://default/missing-secret") + if err != nil { + t.Fatalf("parse ref: %v", err) + } + + p := k8sSecretProvider{} + _, err = p.getDSN(context.Background(), ref) + if err == nil { + t.Fatal("expected error for missing secret, got nil") + } + if !strings.Contains(err.Error(), "unable to fetch secret") { + t.Fatalf("unexpected error message: %v", err) + } +} + +// TestK8sSecretProvider_GetDSN_InvalidURL tests that getDSN returns an error for an invalid k8ssecret URL. +func TestK8sSecretProvider_GetDSN_InvalidURL(t *testing.T) { + k8sProviderInstance = &k8sSecretProvider{ + clientset: fake.NewSimpleClientset(), + namespace: "default", + } + t.Cleanup(func() { k8sProviderInstance = nil }) + + ref, err := url.Parse("k8ssecret://") + if err != nil { + t.Fatalf("parse ref: %v", err) + } + + p := k8sSecretProvider{} + _, err = p.getDSN(context.Background(), ref) + if err == nil { + t.Fatal("expected error for invalid k8ssecret URL, got nil") + } + if !strings.Contains(err.Error(), "invalid k8ssecret URL format") { + t.Fatalf("unexpected error message: %v", err) + } +} + +// TestSecretResolver_K8sSecret_MultipleJobsDifferentKeys tests that the secretResolver correctly resolves different +// keys from the same k8ssecret across multiple jobs without caching issues. +func TestSecretResolver_K8sSecret_MultipleJobsDifferentKeys(t *testing.T) { + clientset := newFakeClientsetWithSecret("default", "my-db-secret", + map[string][]byte{ + "writer": []byte("postgres://writer-dsn"), + "reader": []byte("postgres://reader-dsn"), + }, + nil, + ) + + k8sProviderInstance = &k8sSecretProvider{ + clientset: clientset, + namespace: "default", + } + t.Cleanup(func() { k8sProviderInstance = nil }) + + origProviders := secretProviders + defer func() { secretProviders = origProviders }() + secretProviders = map[string]secretProvider{ + "k8ssecret": k8sSecretProvider{}, + } + + r := &secretResolver{} + ctx := context.Background() + + // Simulates job1's static_config DSN. + gotWriter, err := r.resolve(ctx, "k8ssecret://default/my-db-secret?key=writer") + if err != nil { + t.Fatalf("resolve writer (job1) returned error: %v", err) + } + if gotWriter != "postgres://writer-dsn" { + t.Fatalf("job1 (writer) DSN mismatch: got %q want %q", gotWriter, "postgres://writer-dsn") + } + + // Simulates job2's static_config DSN, same secret, different key. Before the fix, this would incorrectly return + // job1's writer DSN. + gotReader, err := r.resolve(ctx, "k8ssecret://default/my-db-secret?key=reader") + if err != nil { + t.Fatalf("resolve reader (job2) returned error: %v", err) + } + if gotReader != "postgres://reader-dsn" { + t.Fatalf("job2 (reader) DSN mismatch: got %q want %q (bug: got job1's cached DSN)", gotReader, "postgres://reader-dsn") + } + + // A third job re-referencing job1's key should still work from cache. + gotWriterAgain, err := r.resolve(ctx, "k8ssecret://default/my-db-secret?key=writer") + if err != nil { + t.Fatalf("resolve writer (job3) returned error: %v", err) + } + if gotWriterAgain != "postgres://writer-dsn" { + t.Fatalf("job3 (writer, cached) DSN mismatch: got %q want %q", gotWriterAgain, "postgres://writer-dsn") + } + + // Confirm only one underlying k8s API call was made (shared fetch), not one per job/key. + getActions := 0 + for _, action := range clientset.Actions() { + if action.GetVerb() == "get" && action.GetResource().Resource == "secrets" { + getActions++ + } + } + if getActions != 1 { + t.Fatalf("expected exactly 1 k8s secret fetch (shared across jobs), got %d", getActions) + } +} diff --git a/config/secret_resolver.go b/config/secret_resolver.go index 3ed920ca..08f70749 100644 --- a/config/secret_resolver.go +++ b/config/secret_resolver.go @@ -6,6 +6,7 @@ import ( "fmt" "log/slog" "net/url" + "strings" "sync" "golang.org/x/sync/singleflight" @@ -79,17 +80,24 @@ func (r *secretResolver) resolve(ctx context.Context, value string) (string, err // extractKey pulls the appropriate key from a raw secret value (JSON or plain string). func extractKey(raw string, u *url.URL, originalValue string) (string, error) { + var resolved string var payload map[string]string if jsonErr := json.Unmarshal([]byte(raw), &payload); jsonErr == nil { key := u.Query().Get("key") if key == "" { key = "data_source_name" } - val, ok := payload[key] + fieldValue, ok := payload[key] if !ok { return "", fmt.Errorf("key %q not found in secret %q", key, originalValue) } - return val, nil + resolved = fieldValue + } else { + resolved = raw } - return raw, nil + + if tmpl := u.Query().Get("template"); tmpl != "" { + return strings.ReplaceAll(tmpl, "DSN_VALUE", resolved), nil + } + return resolved, nil } diff --git a/config/secret_resolver_test.go b/config/secret_resolver_test.go new file mode 100644 index 00000000..63ee9ae7 --- /dev/null +++ b/config/secret_resolver_test.go @@ -0,0 +1,62 @@ +package config + +import ( + "context" + "encoding/json" + "net/url" + "testing" +) + +// fakeSecretProvider is a mock implementation of secretProvider for testing purposes. +type fakeSecretProvider struct { + payload map[string]string + calls int +} + + +// getDSN simulates fetching a DSN from a secret provider. It returns the JSON-encoded payload and increments the call count. +func (p *fakeSecretProvider) getDSN(_ context.Context, _ *url.URL) (string, error) { + p.calls++ + b, err := json.Marshal(p.payload) + if err != nil { + return "", err + } + return string(b), nil +} + +// TestSecretResolver_SharedFetch_DifferentKeys tests that the secretResolver fetches the secret only once when resolving different keys from the same secret. +func TestSecretResolver_SharedFetch_DifferentKeys(t *testing.T) { + orig := secretProviders + defer func() { secretProviders = orig }() + + fp := &fakeSecretProvider{ + payload: map[string]string{ + "writer": "postgres://writer-dsn", + "reader": "postgres://reader-dsn", + }, + } + secretProviders = map[string]secretProvider{"k8ssecret": fp} + + r := &secretResolver{} + ctx := context.Background() + + gotWriter, err := r.resolve(ctx, "k8ssecret://default/my-db-secret?key=writer") + if err != nil { + t.Fatalf("resolve writer: %v", err) + } + if gotWriter != "postgres://writer-dsn" { + t.Fatalf("writer mismatch: got %q", gotWriter) + } + + gotReader, err := r.resolve(ctx, "k8ssecret://default/my-db-secret?key=reader") + if err != nil { + t.Fatalf("resolve reader: %v", err) + } + if gotReader != "postgres://reader-dsn" { + t.Fatalf("reader mismatch: got %q", gotReader) + } + + if fp.calls != 1 { + t.Fatalf("expected shared fetch (1 call), got %d", fp.calls) + } +} diff --git a/config/secret_vault.go b/config/secret_vault.go index ac38535f..eba029e7 100644 --- a/config/secret_vault.go +++ b/config/secret_vault.go @@ -2,6 +2,7 @@ package config import ( "context" + "encoding/json" "fmt" "net/url" @@ -43,21 +44,22 @@ func (p vaultProvider) getDSN(ctx context.Context, ref *url.URL) (string, error) return "", fmt.Errorf("unable to read Vault secret at %q: %w", secretPath, err) } - // key query param specifies which field to extract, defaults to "data_source_name". - key := q.Get("key") - if key == "" { - key = "data_source_name" + // Return the raw secret payload (all keys) as JSON. Per-call ?key= selection is + // handled by secretResolver.extractKey, since this provider's result is cached + // (and shared) across all references to the same secret path regardless of + // query params. + raw := make(map[string]string, len(secret.Data)) + for k, v := range secret.Data { + str, ok := v.(string) + if !ok { + return "", fmt.Errorf("value for key %q in Vault secret at %q is not a string", k, secretPath) + } + raw[k] = str } - val, ok := secret.Data[key] - if !ok { - return "", fmt.Errorf("key %q not found in Vault secret at %q", key, secretPath) - } - - str, ok := val.(string) - if !ok { - return "", fmt.Errorf("key %q in Vault secret at %q is not a string", key, secretPath) + b, err := json.Marshal(raw) + if err != nil { + return "", fmt.Errorf("unable to marshal Vault secret at %q: %w", secretPath, err) } - - return str, nil + return string(b), nil } diff --git a/config/secret_vault_test.go b/config/secret_vault_test.go new file mode 100644 index 00000000..4e10ef98 --- /dev/null +++ b/config/secret_vault_test.go @@ -0,0 +1,174 @@ +package config + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + vault "github.com/hashicorp/vault/api" +) + +// newVaultTestServer starts an httptest server that serves a fixed KV v2 response for any path, +// mimicking Vault's /v1//data/ shape. +func newVaultTestServer(t *testing.T, data map[string]any) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + resp := map[string]any{ + "data": map[string]any{ + "data": data, + "metadata": map[string]any{ + "version": 1, + }, + }, + } + _ = json.NewEncoder(w).Encode(resp) + })) +} + +// newVaultTestServerV1 mimics a KV v1 response shape (no nested "data"). +func newVaultTestServerV1(t *testing.T, data map[string]any) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + resp := map[string]any{ + "data": data, + } + _ = json.NewEncoder(w).Encode(resp) + })) +} + +// setVaultAddrEnv sets the VAULT_ADDR and VAULT_TOKEN environment variables for testing. +func setVaultAddrEnv(t *testing.T, addr string) { + t.Helper() + t.Setenv("VAULT_ADDR", addr) + // Ensure no stray token requirement blocks the read. + t.Setenv("VAULT_TOKEN", "test-token") +} + +// TestVaultProvider_GetDSN_ReturnsRawJSONPayload tests that the vaultProvider returns the raw JSON payload +func TestVaultProvider_GetDSN_ReturnsRawJSONPayload(t *testing.T) { + srv := newVaultTestServer(t, map[string]any{ + "writer": "postgres://writer-dsn", + "reader": "postgres://reader-dsn", + }) + defer srv.Close() + setVaultAddrEnv(t, srv.URL) + + ref, err := url.Parse("hashivault://secret/my-db-secret") + if err != nil { + t.Fatalf("parse ref: %v", err) + } + + p := vaultProvider{} + raw, err := p.getDSN(context.Background(), ref) + if err != nil { + t.Fatalf("getDSN returned error: %v", err) + } + + var payload map[string]string + if err := json.Unmarshal([]byte(raw), &payload); err != nil { + t.Fatalf("expected raw JSON payload, got %q (unmarshal err: %v)", raw, err) + } + if payload["writer"] != "postgres://writer-dsn" { + t.Fatalf("writer mismatch: got %q", payload["writer"]) + } + if payload["reader"] != "postgres://reader-dsn" { + t.Fatalf("reader mismatch: got %q", payload["reader"]) + } +} + +// TestVaultProvider_GetDSN_KVv1 tests that the vaultProvider correctly handles KV v1 secrets. +func TestVaultProvider_GetDSN_KVv1(t *testing.T) { + srv := newVaultTestServerV1(t, map[string]any{ + "data_source_name": "postgres://v1-dsn", + }) + defer srv.Close() + setVaultAddrEnv(t, srv.URL) + + ref, err := url.Parse("hashivault://secret/my-db-secret?engine_version=1") + if err != nil { + t.Fatalf("parse ref: %v", err) + } + + p := vaultProvider{} + raw, err := p.getDSN(context.Background(), ref) + if err != nil { + t.Fatalf("getDSN returned error: %v", err) + } + + var payload map[string]string + if err := json.Unmarshal([]byte(raw), &payload); err != nil { + t.Fatalf("expected raw JSON payload, got %q (unmarshal err: %v)", raw, err) + } + if payload["data_source_name"] != "postgres://v1-dsn" { + t.Fatalf("data_source_name mismatch: got %q", payload["data_source_name"]) + } +} + +// TestVaultProvider_GetDSN_NonStringValue_Errors tests that the vaultProvider returns an error when a secret value is +// not a string. +func TestVaultProvider_GetDSN_NonStringValue_Errors(t *testing.T) { + srv := newVaultTestServer(t, map[string]any{ + "writer": 12345, // non-string value should trigger an error + }) + defer srv.Close() + setVaultAddrEnv(t, srv.URL) + + ref, err := url.Parse("hashivault://secret/my-db-secret") + if err != nil { + t.Fatalf("parse ref: %v", err) + } + + p := vaultProvider{} + _, err = p.getDSN(context.Background(), ref) + if err == nil { + t.Fatal("expected error for non-string secret value, got nil") + } + if !strings.Contains(err.Error(), "is not a string") { + t.Fatalf("unexpected error message: %v", err) + } +} + +// TestVaultProvider_GetDSN_SecretFetchError tests that the vaultProvider returns an error when the secret cannot be +// fetched. +func TestVaultProvider_GetDSN_SecretFetchError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + setVaultAddrEnv(t, srv.URL) + + ref, err := url.Parse("hashivault://secret/missing-secret") + if err != nil { + t.Fatalf("parse ref: %v", err) + } + + p := vaultProvider{} + _, err = p.getDSN(context.Background(), ref) + if err == nil { + t.Fatal("expected error for missing secret, got nil") + } + if !strings.Contains(err.Error(), "unable to read Vault secret") { + t.Fatalf("unexpected error message: %v", err) + } +} + +// TestVaultProvider_HonorsVaultAddrEnv tests that the vaultProvider honors the VAULT_ADDR environment variable. +func TestVaultProvider_HonorsVaultAddrEnv(t *testing.T) { + srv := newVaultTestServer(t, map[string]any{"data_source_name": "x"}) + defer srv.Close() + setVaultAddrEnv(t, srv.URL) + + cfg := vault.DefaultConfig() + if err := cfg.ReadEnvironment(); err != nil { + t.Fatalf("ReadEnvironment: %v", err) + } + if cfg.Address != srv.URL { + t.Fatalf("expected address %q, got %q", srv.URL, cfg.Address) + } +} From abd6f92c7b3eb8f6b3f5ff37e1ec41b6554e4480 Mon Sep 17 00:00:00 2001 From: Sergei Zyubin Date: Tue, 11 Aug 2026 13:05:29 +0200 Subject: [PATCH 2/2] fix: formatting --- config/secret_k8s_test.go | 2 +- config/secret_resolver_test.go | 1 - config/secret_vault_test.go | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/config/secret_k8s_test.go b/config/secret_k8s_test.go index ccd90299..4b107f23 100644 --- a/config/secret_k8s_test.go +++ b/config/secret_k8s_test.go @@ -15,7 +15,7 @@ import ( // newTestK8sProvider builds a k8sSecretProvider via a fake clientset, bypassing getK8sProvider in-cluster setup func newTestK8sProvider(namespace string, objects ...*corev1.Secret) k8sSecretProvider { // k8sSecretProvider.getDSN calls getK8sProvider() internally; to keep this test hermetic we exercise the - // provider's logic via a lightweight wrapper. + // provider's logic via a lightweight wrapper. return k8sSecretProvider{} } diff --git a/config/secret_resolver_test.go b/config/secret_resolver_test.go index 63ee9ae7..12b699a6 100644 --- a/config/secret_resolver_test.go +++ b/config/secret_resolver_test.go @@ -13,7 +13,6 @@ type fakeSecretProvider struct { calls int } - // getDSN simulates fetching a DSN from a secret provider. It returns the JSON-encoded payload and increments the call count. func (p *fakeSecretProvider) getDSN(_ context.Context, _ *url.URL) (string, error) { p.calls++ diff --git a/config/secret_vault_test.go b/config/secret_vault_test.go index 4e10ef98..f2fec58c 100644 --- a/config/secret_vault_test.go +++ b/config/secret_vault_test.go @@ -12,7 +12,7 @@ import ( vault "github.com/hashicorp/vault/api" ) -// newVaultTestServer starts an httptest server that serves a fixed KV v2 response for any path, +// newVaultTestServer starts an httptest server that serves a fixed KV v2 response for any path, // mimicking Vault's /v1//data/ shape. func newVaultTestServer(t *testing.T, data map[string]any) *httptest.Server { t.Helper()