TT-17100: Add core implementation of kv package by vladzabolotnyi · Pull Request #135 · TykTechnologies/storage

vladzabolotnyi · 2026-05-13T07:30:02Z

Description

The tickets below are the part of current change. Every PR was merged after review.
https://tyktech.atlassian.net/browse/TT-17100
https://tyktech.atlassian.net/browse/TT-17238
https://tyktech.atlassian.net/browse/TT-17239
https://tyktech.atlassian.net/browse/TT-17240

Related Issue

Motivation and Context

Test Coverage For This Change

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Refactoring or add test (improvements in base code or adds test coverage to functionality)
Documentation updates or improvements.

Checklist

Ticket Details

TT-17100


Status	Blocked
Summary	Implement Enhanced KV Storage Interfaces into Storage Library

Generated at: 2026-07-07 06:57:37

github-actions · 2026-05-13T07:30:15Z

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

0 out of 2 committers have signed the CLA.
❌ @vlad Zabolotnyi
❌ @vladzabolotnyi
Vlad Zabolotnyi seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You can retrigger this bot by commenting recheck in this Pull Request}

probelabs · 2026-05-13T07:30:55Z

This PR introduces the core implementation of a new kv package, designed to provide a unified, pluggable framework for secret management from various key-value (KV) stores. It establishes a robust foundation with caching, request deduplication, and a full provider lifecycle management system.

Files Changed Analysis

Total Files Changed: 21 (19 new files, 2 modified CI/build files).
Additions: 4,231 lines of new code and tests.
Deletions: 0.
Notable Patterns: This is a self-contained feature addition, creating the kv package and its sub-packages (internal/cache, internal/store, internal/resolve, registry, resolver). The implementation is accompanied by an extensive suite of unit and concurrency tests, indicating a focus on reliability and correctness.

Architecture & Impact Assessment

What this PR accomplishes: It delivers the engine for a generic secret management system. Components can now resolve secret references from configuration strings or entire JSON documents, abstracting away the specific backend (e.g., Vault, Consul, environment variables).
Key technical changes introduced:
- kv/provider.go: Defines the central Provider interface that all KV backends must implement, along with optional interfaces (Initializer, Closer, Lister) for lifecycle management.
- kv/registry: Implements a Registry to manage the lifecycle of named KV store providers. It handles concurrent initialization, configuration, and graceful shutdown.
- kv/internal/store: Introduces a SecretStore decorator that wraps providers to add caching and request deduplication. It uses golang.org/x/sync/singleflight to prevent thundering herd problems on secret lookups.
- kv/internal/cache: A thread-safe, TTL-based in-memory cache. It supports positive caching, negative caching for different error types (not found vs. transient), and a "stale-while-revalidate" strategy for background refreshes.
- kv/resolver: The public-facing API that consumers will use. It parses two types of secret references: whole-value (kv://store/path) and inline (...$kv{store:path}...). It can process individual strings or walk and resolve all string values within a JSON document.
Affected system components: This is a new, self-contained package. It does not yet impact existing application code but is now a functional module ready for integration into configuration loading and secret management workflows.

Component Relationships

graph TD
    subgraph Application
        A[Config Loader] --> R["resolver.Resolver"]
    end

    subgraph "kv Package"
        R --|"GetStore(name)"|--> REG("registry.Registry")
        REG --|Manages lifecycle of|--> S[store.SecretStore]
        S --|Decorates|--> P("kv.Provider")
        S --|Uses for caching|--> C[cache.Cache]
        S --|Uses for deduplication|--> SF["singleflight.Group"]
    end

    subgraph "Provider Implementations (Future)"
        P_Vault[Vault Provider] --|Implements|--> P
        P_Env[Env Provider] --|Implements|--> P
    end

    REG --|Creates via factory|--> P

Scope Discovery & Context Expansion

This PR delivers the complete backend and frontend logic for the secret management system. The framework is fully functional and tested.

The logical next steps for leveraging this feature are:

Implement Concrete Providers: Create implementations for the kv.Provider interface for specific backends like environment variables, HashiCorp Vault, or AWS Secrets Manager.
Application Integration: Integrate the resolver.Resolver into the application's startup and configuration loading sequence to replace existing secret handling mechanisms.
CI/Build Integration: The PR already includes initial updates to Taskfile.yml and .github/workflows/ci-tests.yml to incorporate the new package into the test pipeline.

Metadata

Review Effort: 5 / 5
Primary Label: feature

Powered by Visor from Probelabs

Last updated: 2026-07-07T06:58:58.318Z | Triggered by: pr_updated | Commit: 7cbd904

💡 TIP: You can chat with Visor using /visor ask <your question>

probelabs · 2026-05-13T07:31:15Z

Security Issues (3)

Severity	Location	Issue
🟠 Error	`kv/internal/resolve/resolver.go:163`	The `Resolver` extracts a path from a reference string and passes it directly to the underlying provider's `Get` method without any sanitization (e.g., for `../` sequences). This creates a significant risk of path traversal vulnerabilities. If any provider implementation (especially one reading from a filesystem) uses this path without its own robust sanitization, an attacker could potentially read arbitrary files from the system by crafting a malicious configuration value (e.g., `kv://file-provider/../../etc/passwd`). The framework should not delegate this critical security check entirely to provider implementations. 💡 Suggestion Implement path sanitization within the `Resolver` as a defense-in-depth measure. Before passing the path to the provider, validate it to ensure it is a clean, canonical path and does not contain any directory traversal elements. A library like `path/filepath.Clean` can be helpful, but you must also check that the cleaned path does not start with `../` or contain `../`.
🟡 Warning	`kv/internal/cache/cache.go:20`	The in-memory cache for secrets is unbounded. If a large number of unique secret keys are requested, the cache can grow indefinitely, potentially leading to excessive memory consumption and a denial-of-service (DoS) condition. 💡 Suggestion Implement a mechanism to limit the cache size. This could be a simple maximum number of entries or a more sophisticated LRU (Least Recently Used) eviction policy.
🟡 Warning	`kv/internal/resolve/resolver.go:125`	The `ResolveAll` function recursively processes an entire JSON document. A malicious or large configuration file containing a high number of unique KV references could trigger a large volume of requests to backend secret stores. This could overwhelm the secret providers (e.g., Vault, AWS Secrets Manager), leading to a denial of service for the application and other systems relying on those providers. 💡 Suggestion Consider introducing safeguards against abuse. Options include adding a configurable limit to the number of KV references that can be processed in a single `ResolveAll` call, or implementing rate limiting on requests to the underlying stores. At a minimum, this risk should be documented for users of the library.

Security Issues (3)

Severity	Location	Issue
🟠 Error	`kv/internal/resolve/resolver.go:163`	The `Resolver` extracts a path from a reference string and passes it directly to the underlying provider's `Get` method without any sanitization (e.g., for `../` sequences). This creates a significant risk of path traversal vulnerabilities. If any provider implementation (especially one reading from a filesystem) uses this path without its own robust sanitization, an attacker could potentially read arbitrary files from the system by crafting a malicious configuration value (e.g., `kv://file-provider/../../etc/passwd`). The framework should not delegate this critical security check entirely to provider implementations. 💡 Suggestion Implement path sanitization within the `Resolver` as a defense-in-depth measure. Before passing the path to the provider, validate it to ensure it is a clean, canonical path and does not contain any directory traversal elements. A library like `path/filepath.Clean` can be helpful, but you must also check that the cleaned path does not start with `../` or contain `../`.
🟡 Warning	`kv/internal/cache/cache.go:20`	The in-memory cache for secrets is unbounded. If a large number of unique secret keys are requested, the cache can grow indefinitely, potentially leading to excessive memory consumption and a denial-of-service (DoS) condition. 💡 Suggestion Implement a mechanism to limit the cache size. This could be a simple maximum number of entries or a more sophisticated LRU (Least Recently Used) eviction policy.
🟡 Warning	`kv/internal/resolve/resolver.go:125`	The `ResolveAll` function recursively processes an entire JSON document. A malicious or large configuration file containing a high number of unique KV references could trigger a large volume of requests to backend secret stores. This could overwhelm the secret providers (e.g., Vault, AWS Secrets Manager), leading to a denial of service for the application and other systems relying on those providers. 💡 Suggestion Consider introducing safeguards against abuse. Options include adding a configurable limit to the number of KV references that can be processed in a single `ResolveAll` call, or implementing rate limiting on requests to the underlying stores. At a minimum, this risk should be documented for users of the library.

\n\n ### Architecture Issues (1)

Severity	Location	Issue
🟠 Error	`kv/registry/registry.go:264-275`	The `Close` method uses `wg.Go(...)` on a `sync.WaitGroup` variable, which is not a valid method. This appears to be a pattern copied from test code that uses the `testing/synctest` package, which is not available in production code. This will cause a compilation error, breaking the registry's shutdown mechanism. 💡 Suggestion Refactor the concurrent shutdown logic to use the standard `sync.WaitGroup` pattern. Use `wg.Add(1)` before the goroutine, `go func() { ... }()` to launch it, and `defer wg.Done()` within it. Also, ensure loop variables are passed as arguments to the goroutine to prevent capturing incorrect values in the closure. 🔧 Suggested Fix `for name, s := range stores { wg.Add(1) go func(name string, store kv.Provider) { defer wg.Done() if closer, ok := kv.AsCloser(store); ok { if err := closer.Close(ctx); err != nil { mu.Lock() errs = append(errs, fmt.Errorf("failed to close store %q: %w", name, err)) mu.Unlock() } } }(name, s) }`

Performance Issues (2)

Severity	Location	Issue
🟡 Warning	`kv/internal/cache/cache.go:193-217`	The cache cleanup mechanism iterates through all keys in the cache to find expired entries. This O(N) scan, where N is the number of items in the cache, can become a performance bottleneck if the cache grows to a very large size (e.g., hundreds of thousands or millions of keys). The scan holds a read lock, which can delay write operations, and the subsequent deletion holds a write lock, blocking all other operations. 💡 Suggestion For improved scalability, consider a cache eviction strategy that doesn't require a full scan. Common approaches include: 1. Using a separate data structure for expirations: Maintain a priority queue (min-heap) or a time-ordered list (like a doubly-linked list) of entries based on their expiration time. This allows the cleanup process to efficiently find all expired items without scanning the entire cache. 2. Cache sharding: Partition the cache into several smaller, independently locked maps. This reduces lock contention, as cleanup and access operations only lock one shard at a time. While the current implementation is likely sufficient for a moderate number of secrets, these alternative designs would offer better performance characteristics at a very large scale.
🟡 Warning	`kv/internal/resolve/resolver.go:120-161`	The `ResolveAll` function unmarshals and then re-marshals the entire JSON document if any KV reference is found. For very large JSON documents, this process can lead to high memory allocation and CPU usage due to the creation of intermediate data structures (`map[string]any`, `[]any`) for the entire document. 💡 Suggestion For very large documents where performance is critical, consider a streaming-based JSON processor. A streaming parser would read the JSON token by token, allowing for in-place replacement of string values that contain KV references without having to load the entire document into memory. This would significantly reduce memory overhead and could improve CPU efficiency. However, this approach is more complex to implement than the current unmarshal/walk/marshal pattern.

Quality Issues (4)

Severity	Location	Issue
🟠 Error	`kv/registry/registry.go:1-73`	The new `kv` package and its sub-packages (`registry`, `resolver`) are introduced without any unit tests. This introduces a significant maintainability risk, as there is no automated way to verify the correctness of the implemented logic (e.g., `registry.Add`, `registry.GetStore`, `provider.As`) or to prevent regressions. The SonarQube quality gate has failed due to 0% test coverage on new code. 💡 Suggestion Add a corresponding `_test.go` file for each new `.go` file that contains logic. Implement unit tests covering the public functions. For example, `registry_test.go` should test adding a factory, getting an existing store, and getting a non-existent store. `provider_test.go` should test the `As` function with various provider wrapping scenarios.
🟡 Warning	`kv/registry/registry.go:51-53`	The core function `InitStores` is stubbed and returns `nil`. This introduces non-functional code and technical debt. The `Close` function in the same file (line 71), and `Resolve` and `ResolveConfig` in `kv/resolver/resolver.go`, are also stubbed. 💡 Suggestion Implement the logic for creating, initializing, and storing providers based on the input configuration. If this work is planned for a subsequent pull request, the function body should contain a `// TODO` comment referencing the follow-up ticket to make the technical debt explicit and trackable.
🟡 Warning	`kv/resolver/resolver.go:40-42`	The function `ResolveConfig` is stubbed and returns `nil`. This introduces non-functional code and technical debt. 💡 Suggestion Implement the logic to recursively traverse a configuration structure and apply the `Resolve` function to all string values. If this work is planned for a subsequent pull request, add a `// TODO` comment referencing the follow-up ticket.
🟡 Warning	`kv/resolver/resolver.go:52-54`	The core `Resolve` method is stubbed and returns an empty string. This leaves the primary functionality of the resolver unimplemented. 💡 Suggestion Implement the logic to parse the input string for `kv://` or `$kv{}` patterns, look up the corresponding store and key, and return the resolved value. If this work is planned for a subsequent pull request, add a `// TODO` comment referencing the follow-up ticket.

Powered by Visor from Probelabs

Last updated: 2026-07-07T06:57:54.987Z | Triggered by: pr_updated | Commit: 7cbd904

💡 TIP: You can chat with Visor using /visor ask <your question>

sonarqubecloud · 2026-05-18T08:09:03Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

* feat: add core interfaces * feat: add resolver interface * feat: add registry type with methods * feat: add config types, registry type with empty methods and resolver type and empty methods * feat: add errors and cache surface implementation * feat: update registry and secret store with base impl * feat: add get and set base impl for the cache * feat: update the errors with adding custom types for key not found and store unavailable that would be used by providers * feat: update cache implementation with a set of optimizations * chore: fixing linter errors * fix: move checking for expired keys to the get method with RLock * test: add concurrency test to the cache * chore: clear unused errors * fix: update errors file with linting fixes * feat: add secret store implementation * fix: replace batch approah on cache cleanup with write lock and clean in place to avoid race conditions and make code simpler * feat: update negative caching to avoid storing context errors * feat: update secret store implementation with correct handling of singleflight and add tests validating the logic * test: update store tests with missing edge cases and reworked tests structure to distinguish edge cases * docs: update config docs string * test: add missing tests for cache * refactor: separate logic for ttl selection * refactor: update tests with using parallel and testing context * docs: add docs to cache get method * refactor: split single package solution to multiple to apply better structure and readability * refactor: use t.Context() instead of background as we have >1.24 Go * feat: rework secret store to make it fit a Provider interface with Unwrap() method and unwrap helpers * refactor: move errors to kv package * fix: update the cache clenup interval to min ttl to avoid casees when ttl is set as too high to rely on * fix: updat foreground fetch with removing handling canceled context because it can be only timed out * feat: rework foreground fetch for secret to avoid blocking request goroutines until they're timeout * feat: rework cache cleanup with separate locks and adding condition to check if its expired between locks * chore: return errro value instead of nil for background fetch * test: update store tests with wrapping them with synctest * refactor: use wg.Go() instead of legacy pattern * refactor: move config to kv package * build: add test-kv to make sonarqube to get corrent coverage * refactor: decrease cognitive complexity for NewCache func * fix: return error intead of loggin * fix: add generic error if the provider returns a non-string which is unreal * fix: pass value instead of whole result type to error * docs: add comment explaining the logic behind a test * chore: make generic error less descriptive * chore: replace fmt.sprintf with string concat * feat: add close methods * feat: add conditions to avoid calling Get methods on secret store and avoid store or return value if its closed * feat: cleanup the cache entries when its closed * fix: add mu locks to the cache close method and tests * feat: add provider timeout enforcement * feat: add separate singleflight group to avoid name collision between foreground and background tasks * fix: add handling of empty ttl field on cache config * TT-17239: Imlement registry (#138) * feat: add implementation for Add and InitStores methods * feat: update init stores with secret store wrapper * feat: add close imlementation to registry * feat: add logger * feat: add initial registry impl * fix: update init stores logic with clearing defer and redundant mutex locks * test: update registry test for close method and init stores edge case * test: update concurrency test * refactor: update registry tests with changing structure * chore: update kv config name * test: add more tests for init stores func * test: add more tests covering edge cases * refactor: smash two defers to one as they reference similar condition * feat: rework Close method to run closing concurrently * feat: add provider type with constants to make code more documented and understandable * chore: update comment * test: add tests covering concurrent logic when close is called while init stores are running * chore: replace literals with constants on condition * feat: update return err logic to avoid redundant assigning with explanatory docs * test: add test to check circular dependency to avoid stack-overflow if self referencing * chore: replace dependency * refactor: simplify logic with isInitialize * refactor: add private func with init a single store to avoid boilerplate code * revert: isInitialized should be used with swap to prevent concurrent requests to initiStores * refactor: update init stores func * refactor: update init stores with detached func * feat: add direct provider interface to fixing open-closed principle * fix: typo * feat: update registry logic with using standalone and timeouter interfaces * feat: rework init stores and make it fully concurrent * chore: remove check for empty stores * chore: add t.Parallel() to tests * fix: distinguish init context with lifetime context and remove dependency between cache context cancelation and init ctx * fix: minor adjustments and comment clean-up --------- Co-authored-by: Vlad Zabolotnyi <vlad.z@tyk.io> --------- Co-authored-by: Vlad Zabolotnyi <vlad.z@tyk.io>

sonarqubecloud · 2026-05-25T12:41:42Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
90.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

* feat: add implementation to Resolve func * feat: add resolveAll implementation * test: add test cases for json pointer extraction * refactor: update error messages and make error handlin more consistent * refactor: update resolve tests * refactor: update resolveAll tests with grouping error cases * fix: added malformed err with tests, added docs string * test: add test suite with path routing * fix: remove redundant assertions and update comment * test: add test case that resolve all resolved mixed values * fix: use decoding with numbers to avoid losing large integers and update doc strings explaining inner behavior * fix: add checks with tests to avoid processing values when store or key is empty string * fix: remove spaces around em-dashes and add missing tests for missing stores and keys * chore: remove redundant comments * fix: address sonarqube comment with avoiding duplication * fix: add json validation on document even if it doesn't contain kv refs * refactor: split resolver for public and private parts to hide inner logic with lenient mode in future * chore: replace background context with test one --------- Co-authored-by: Vlad Zabolotnyi <vlad.z@tyk.io>

github-actions · 2026-07-07T06:57:40Z

🚨 Jira Linter Failed

Commit: 7cbd904
Failed at: 2026-07-07 06:57:38 UTC

The Jira linter failed to validate your PR. Please check the error details below:

🔍 Click to view error details

failed to validate Jira issue: jira ticket TT-17100 has status 'Blocked' but must be one of: Merge, In Design Review, In Dev, In Code Review, Ready For Dev, Dod Check

Next Steps

Ensure your branch name contains a valid Jira ticket ID (e.g., ABC-123)
Verify your PR title matches the branch's Jira ticket ID
Check that the Jira ticket exists and is accessible

This comment will be automatically deleted once the linter passes.

vladzabolotnyi added the do-not-merge label May 13, 2026

vladzabolotnyi closed this May 18, 2026

vladzabolotnyi force-pushed the TT-17100 branch from 857cc25 to b24f7c1 Compare May 18, 2026 08:03

chore: empty commit to keep the parent PR opened

39acd15

vladzabolotnyi reopened this May 18, 2026

feat: add interfaces, errors and config types

d3b7214

vladzabolotnyi changed the title ~~TT-17100: Add implementations to Provider, Registry and Resolver~~ TT-17100: Add core implementation of kv package May 18, 2026

vladzabolotnyi and others added 7 commits May 18, 2026 15:54

Merge branch 'main' into TT-17100

8e77746

build: add test-kv to make sonarqube to get corrent coverage

b2df469

feat: add AsCloser extracting func

42c043f

feat: add logger

f55c322

fix: change KVConfig to Config name for type

4431a72

refactor: rework as funcs to avoid recursion and self-reference problem

92b89d5

feat: add provider type with constants

cd76f9c

MFCaballero approved these changes May 22, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

TT-17100: Add core implementation of kv package#135

TT-17100: Add core implementation of kv package#135
vladzabolotnyi wants to merge 11 commits into
mainfrom
TT-17100

vladzabolotnyi commented May 13, 2026 •

edited by github-actions Bot

Loading

Uh oh!

github-actions Bot commented May 13, 2026 •

edited

Loading

Uh oh!

probelabs Bot commented May 13, 2026 •

edited

Loading

Uh oh!

probelabs Bot commented May 13, 2026 •

edited

Loading

Uh oh!

sonarqubecloud Bot commented May 18, 2026

Uh oh!

sonarqubecloud Bot commented May 25, 2026

Uh oh!

github-actions Bot commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

vladzabolotnyi commented May 13, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

Test Coverage For This Change

Screenshots (if appropriate)

Types of changes

Checklist

Ticket Details

Uh oh!

github-actions Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

probelabs Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Files Changed Analysis

Architecture & Impact Assessment

Component Relationships

Scope Discovery & Context Expansion

Uh oh!

probelabs Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Issues (3)

Security Issues (3)

Performance Issues (2)

Quality Issues (4)

Uh oh!

sonarqubecloud Bot commented May 18, 2026

Quality Gate passed

Uh oh!

sonarqubecloud Bot commented May 25, 2026

Quality Gate passed

Uh oh!

github-actions Bot commented Jul 7, 2026

🚨 Jira Linter Failed

Next Steps

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vladzabolotnyi commented May 13, 2026 •

edited by github-actions Bot

Loading

github-actions Bot commented May 13, 2026 •

edited

Loading

probelabs Bot commented May 13, 2026 •

edited

Loading

probelabs Bot commented May 13, 2026 •

edited

Loading