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	Merge
Summary	Implement Enhanced KV Storage Interfaces into Storage Library

Generated at: 2026-07-27 13:28:45

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 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

_{You can retrigger this bot by commenting recheck in this Pull Request}

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

This PR introduces a new kv package, which provides a comprehensive and extensible framework for secret management. The core goal is to abstract away the underlying secret storage, allowing developers to retrieve secrets using a unified syntax regardless of whether they are stored in environment variables, HashiCorp Vault, AWS Secrets Manager, or other key-value stores.

Key components of this implementation include:

Provider-based Architecture: A central kv.Provider interface allows for different KV backends to be implemented and used interchangeably.
Registry: The kv.registry.Registry manages the lifecycle of named KV store instances, handling their configuration, concurrent initialization, and graceful shutdown.
Resolver: The kv/resolver/resolver.go provides the public API for parsing and replacing secret references. It supports two formats: whole-value (kv://store/path#field) and inline (...$kv{store:path#field}...). It can process individual strings or recursively resolve all string values within a JSON document.
Caching & Deduplication: The kv/internal/store/store.go acts as a decorator, adding a sophisticated caching layer (kv/internal/cache/cache.go) with TTL, negative caching for errors, and a "stale-while-revalidate" background refresh strategy. It also uses singleflight to prevent the "thundering herd" problem by coalescing concurrent requests for the same secret.
CI/Build Integration: The new package is integrated into the existing CI pipeline with a dedicated test job.

Files Changed Analysis

This pull request is a large, self-contained feature addition, introducing 19 new files and modifying 2 build-related files. With 4,274 additions and 0 deletions, the changes establish the entire kv package and its sub-packages (internal/cache, internal/resolve, internal/store, registry, resolver). The implementation is accompanied by an extensive suite of unit and concurrency tests, indicating a strong focus on correctness and reliability. The only modifications to existing files are in .github/workflows/ci-tests.yml and Taskfile.yml to add a test step for the new package.

Architecture & Impact Assessment

What this PR accomplishes: It delivers the core engine for a generic, extensible secret management system. This allows other components to abstract away the specifics of secret storage, using a consistent syntax to retrieve sensitive values from configuration.
Key technical changes introduced:
- kv/provider.go: Defines the central Provider interface and optional interfaces (Initializer, Closer, Lister) for advanced lifecycle management.
- kv/registry/registry.go: Implements the Registry to manage provider factories and initialized store instances concurrently.
- kv/internal/store/store.go: Introduces the SecretStore decorator that wraps providers to add caching, request deduplication, and timeouts.
- kv/resolver/resolver.go: The public-facing API that consumers will use to resolve secret references.
Affected system components: This is a new, self-contained package. It does not yet impact existing application code but provides a foundational module ready for integration into configuration loading and secret management workflows across the platform.

Component Relationships

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

    subgraph "kv Package"
        R -- GetStore --> REG[registry.Registry]
        REG -- Manages --> SS[store.SecretStore]
        SS -- Decorates --> P[kv.Provider]
        SS -- Uses --> C[cache.Cache]
        SS -- Uses --> SF[singleflight.Group]
    end

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

    REG -- Creates from factory --> P

Scope Discovery & Context Expansion

This PR delivers the complete backend and frontend logic for the secret management framework. It is fully functional and tested but does not yet include any concrete provider implementations (e.g., for Vault, environment variables, etc.).
The logical next steps to leverage this feature are:
1. Implement Concrete Providers: Create implementations for the kv.Provider interface for specific backends like environment variables, HashiCorp Vault, or AWS Secrets Manager.
2. Application Integration: Integrate the resolver.Resolver into the application's startup and configuration loading sequence to replace existing secret handling mechanisms.
3. Deprecate Old Mechanisms: Once integrated, existing bespoke secret-fetching logic across the codebase can be refactored to use this unified system.

Metadata

Review Effort: 5 / 5
Primary Label: feature

Powered by Visor from Probelabs

Last updated: 2026-07-27T13:30:50.519Z | Triggered by: pr_updated | Commit: b8db2de

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

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

Security Issues (2)

Severity	Location	Issue
🟡 Warning	`kv/internal/cache/cache.go:28`	The in-memory cache lacks a size limit, which can lead to uncontrolled memory growth and potential Denial of Service if a large number of unique keys are resolved. An attacker who can influence the keys being looked up could potentially cause an Out-Of-Memory error by forcing the cache to store an excessive number of entries. 💡 Suggestion Implement a cache eviction policy, such as Least Recently Used (LRU), and add a configuration option to set a maximum number of entries in the cache. This will protect the service from excessive memory consumption.
🟡 Warning	`kv/internal/resolve/helpers.go:16`	The JSON Pointer helper parses the entire payload from a KV store into memory before extracting a value. A very large JSON document returned by a provider could lead to excessive memory consumption and a potential Denial of Service. 💡 Suggestion Introduce a configurable limit on the maximum size of the JSON payload that the resolver will attempt to parse. This can be achieved by wrapping the reader with `io.LimitReader` before passing it to `json.NewDecoder`. Payloads exceeding this limit should be rejected with an error.

Architecture Issues (1)

Severity	Location	Issue
🟠 Error	`kv/registry/registry.go:306-316`	The `Close` method uses `wg.Go(...)`, which is not a method on `sync.WaitGroup`. This will cause a compilation error. Additionally, the goroutine captures loop variables (`name`, `store`) by reference, which is a classic concurrency bug in Go. All goroutines would likely operate on the values from the last loop iteration. 💡 Suggestion Replace `wg.Go` with the standard `wg.Add(1)` and `go func() { defer wg.Done() ... }()` pattern. Also, capture the loop variables within the loop body before starting the goroutine to ensure each goroutine works with the correct store instance. 🔧 Suggested Fix `for name, store := range stores { name, store := name, store // Capture loop variables wg.Add(1) go func() { 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() } } }() }`

Security Issues (2)

Severity	Location	Issue
🟡 Warning	`kv/internal/cache/cache.go:28`	The in-memory cache lacks a size limit, which can lead to uncontrolled memory growth and potential Denial of Service if a large number of unique keys are resolved. An attacker who can influence the keys being looked up could potentially cause an Out-Of-Memory error by forcing the cache to store an excessive number of entries. 💡 Suggestion Implement a cache eviction policy, such as Least Recently Used (LRU), and add a configuration option to set a maximum number of entries in the cache. This will protect the service from excessive memory consumption.
🟡 Warning	`kv/internal/resolve/helpers.go:16`	The JSON Pointer helper parses the entire payload from a KV store into memory before extracting a value. A very large JSON document returned by a provider could lead to excessive memory consumption and a potential Denial of Service. 💡 Suggestion Introduce a configurable limit on the maximum size of the JSON payload that the resolver will attempt to parse. This can be achieved by wrapping the reader with `io.LimitReader` before passing it to `json.NewDecoder`. Payloads exceeding this limit should be rejected with an error.

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

Severity	Location	Issue
🟠 Error	`kv/registry/registry.go:306-316`	The `Close` method uses `wg.Go(...)`, which is not a method on `sync.WaitGroup`. This will cause a compilation error. Additionally, the goroutine captures loop variables (`name`, `store`) by reference, which is a classic concurrency bug in Go. All goroutines would likely operate on the values from the last loop iteration. 💡 Suggestion Replace `wg.Go` with the standard `wg.Add(1)` and `go func() { defer wg.Done() ... }()` pattern. Also, capture the loop variables within the loop body before starting the goroutine to ensure each goroutine works with the correct store instance. 🔧 Suggested Fix `for name, store := range stores { name, store := name, store // Capture loop variables wg.Add(1) go func() { 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() } } }() }`

\n\n ### Performance Issues (2)

Severity	Location	Issue
🟡 Warning	`kv/internal/cache/cache.go:180-208`	The `cleanup` function iterates through all entries in the cache map to find and remove expired items. This O(N) operation, where N is the number of cached items, can become a performance bottleneck if the cache grows very large. The function holds a read lock during the entire scan and allocates a slice for expired keys, which could also be large and cause memory pressure. 💡 Suggestion For improved scalability with very large caches, consider a data structure that tracks expirations more efficiently, such as a time-ordered list (e.g., a min-heap or a doubly-linked list) of entries. This would allow the cleanup process to find expired items in O(k log N) or O(k) time (where k is the number of expired items) instead of O(N), reducing CPU usage and lock contention. For the expected use case, the current implementation may be sufficient, but this is a key consideration for scalability.
🟡 Warning	`kv/internal/resolve/resolver.go:126-161`	The `ResolveAll` function decodes the entire JSON document into memory (`var doc any`) before walking it to resolve KV references. For very large JSON documents (e.g., multiple megabytes), this can lead to high memory consumption and pressure on the garbage collector. 💡 Suggestion If handling very large configuration files is a potential use case, consider implementing a streaming resolver. This could be achieved by using `json.NewDecoder().Token()` to read the JSON token by token and writing the (potentially resolved) tokens to an output stream, which would avoid loading the entire document into memory. This approach trades some implementation complexity for significantly better memory efficiency.

Quality Issues (2)

Severity	Location	Issue
🔴 Critical	`kv/registry/registry.go:189-231`	Loop variables `name` and `storeCfg` are captured by reference in a goroutine launched within a `for` loop. This will cause all goroutines to use the values from the final iteration, leading to incorrect store initialization. The stores will be initialized with the wrong names and configurations, or the same store will be initialized multiple times. 💡 Suggestion Create a local copy of the loop variables at the beginning of each iteration to ensure each goroutine captures the correct values. 🔧 Suggested Fix `for name, storeCfg := range config.Stores { name, storeCfg := name, storeCfg // Capture loop variables for the goroutine`
🔴 Critical	`kv/registry/registry.go:278-286`	The code attempts to call a `Go` method on a `sync.WaitGroup` variable (`wg`). `sync.WaitGroup` does not have a `Go` method; this will cause a compilation failure. The standard pattern is to use `wg.Add(1)` before starting a goroutine and `defer wg.Done()` inside it. Additionally, the loop variables `name` and `store` are captured by the closure, which would lead to incorrect behavior if the code compiled. 💡 Suggestion Replace the incorrect `wg.Go` call with the standard `wg.Add(1)` and `go func() { ... }()` pattern. Also, capture the loop variables `name` and `store` to prevent race conditions. 🔧 Suggested Fix `for name, store := range stores { name, store := name, store // Capture loop variables wg.Add(1) go func() { 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() } } }() }`

Powered by Visor from Probelabs

Last updated: 2026-07-27T13:30:00.017Z | Triggered by: pr_updated | Commit: b8db2de

💡 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>

sonarqubecloud · 2026-07-27T13:35:51Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

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

vladzabolotnyi and others added 5 commits July 6, 2026 23:55

merge master and resolve conflicts

e56c91c

feat: update the logger interface to avoid breaking change in future

1ea7c5f

fix: address sonarqube issues

5842537

refactor: decrease cognitive complexity of init stores func

b8db2de

vladzabolotnyi removed the do-not-merge label Jul 27, 2026

vladzabolotnyi merged commit 2d3a47d into main Jul 27, 2026
45 of 49 checks passed

vladzabolotnyi deleted the TT-17100 branch July 27, 2026 13:46

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 merged 15 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!

sonarqubecloud Bot commented Jul 27, 2026

Uh oh!

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 (2)

Architecture Issues (1)

Security Issues (2)

Quality Issues (2)

Uh oh!

sonarqubecloud Bot commented May 18, 2026

Quality Gate passed

Uh oh!

sonarqubecloud Bot commented May 25, 2026

Quality Gate passed

Uh oh!

sonarqubecloud Bot commented Jul 27, 2026

Quality Gate passed

Uh oh!

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