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

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

Generated at: 2026-05-25 12:40:11

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 pull request introduces the core implementation of the new kv package, a comprehensive system for managing secrets from various key-value stores. This moves significantly beyond the initial scaffolding, delivering a functional, tested, and robust foundation for secret management.

Files Changed Analysis

New Files: 12 new Go files and 2 modified configuration files.
Additions: 2,982 lines of new code, primarily focused on implementation and extensive testing.
Deletions: 0.
Notable Patterns: The PR introduces a complete, production-ready implementation for the secret store, caching layer, and registry. It is characterized by a clean separation of concerns, dependency injection, and thorough test coverage for complex concurrent operations. The resolver component remains stubbed for future work.

Architecture & Impact Assessment

What this PR accomplishes: It delivers the core engine for a pluggable secret management system. This includes a sophisticated caching layer, request deduplication to prevent load spikes, and a full lifecycle management system for secret providers.
Key technical changes introduced:
- kv/internal/cache: A thread-safe, TTL-based in-memory cache. It supports positive caching for successful lookups, negative caching for transient errors and "not found" errors (with separate TTLs), and a "stale-while-revalidate" mechanism to perform background refreshes of expiring secrets without blocking requests.
- kv/internal/store: A SecretStore decorator that wraps any secret Provider. It uses golang.org/x/sync/singleflight to coalesce concurrent requests for the same secret into a single backend call and integrates the caching layer to serve subsequent requests from memory.
- kv/registry: A fully implemented Registry that manages the lifecycle of multiple named KV stores. It handles concurrent initialization of providers, wraps them with the SecretStore decorator, and ensures graceful shutdown and resource cleanup, even in cases of partial initialization failure.
- kv/provider.go: Defines the central Provider interface and several optional interfaces (Initializer, Closer, Lister) that allow providers to hook into the registry's lifecycle management. Includes a generic As helper for safely unwrapping decorated providers.
- Extensive Testing: Adds over 1,800 lines of unit and concurrency tests, ensuring the reliability of the cache, store, and registry components under various conditions, including race conditions and lifecycle edge cases.
Affected system components: This is a new, self-contained package. While it doesn't impact existing code directly, it is now a functional module ready for integration into the application's configuration loading and secret management workflows.

Component Relationships

graph TD
    subgraph Application [Application Consumer]
        Resolver["kv.Resolver (Stubbed)"] -- Uses --> Registry[kv.registry.Registry]
    end

    subgraph KV_Package ["KV Package Core (This PR)"]
        Registry -- Manages --> SecretStore[store.SecretStore]
        SecretStore -- Wraps --> Provider[kv.Provider]
        SecretStore -- Uses --> Cache[cache.Cache]
        SecretStore -- Uses --> Singleflight[singleflight.Group]
    end

    subgraph ProviderImplementations ["Provider Implementations (Future)"]
        Provider
    end

    Registry -- Uses Factory to Create --> Provider

Scope Discovery & Context Expansion

This PR delivers the complete backend logic for the secret management system. The only remaining piece of the core framework is the kv/resolver/resolver.go, which is responsible for parsing configuration files to find and replace secret references (e.g., kv://...).

The logical next steps for this feature are:

Implement the Resolver: Build the logic in kv/resolver/resolver.go to parse strings and use the now-functional Registry to fetch secret values.
Implement Concrete Providers: Create initial provider implementations, such as for environment variables (env) or HashiCorp Vault (hashicorp_vault), by implementing the kv.Provider interface.
Application Integration: Integrate the Resolver into the application's startup sequence to resolve secrets in configuration files before services are initialized.

Metadata

Review Effort: 4 / 5
Primary Label: feature

Powered by Visor from Probelabs

Last updated: 2026-05-25T12:40:32.421Z | Triggered by: pr_updated | Commit: 05403cd

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

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

Security Issues (3)

Severity	Location	Issue
🟡 Warning	`kv/errors.go:42-44`	Error messages in `StoreUnavailableError` include the underlying provider error. If these errors are propagated to external systems or logs with insufficient access controls, they could reveal sensitive internal architecture details, such as network addresses or stack traces from the provider's SDK. 💡 Suggestion Consider redacting sensitive information from errors that may leave the service boundary. A common pattern is to have a detailed internal error for logging and a generic, sanitized error for external responses. For `StoreUnavailableError`, consider logging the full `e.Err` internally but not including its string representation in the error message returned to the caller.
🟡 Warning	`kv/internal/cache/cache.go:25`	The in-memory cache (`Cache.entries`) has no upper bound on its size. It only evicts entries when their TTL expires. An attacker or a misbehaving client capable of requesting a large number of unique keys could cause the cache to grow indefinitely, leading to excessive memory consumption and a potential denial-of-service (DoS) attack. 💡 Suggestion Introduce a configurable capacity limit for the cache. When the limit is reached, an eviction strategy such as LRU (Least Recently Used) should be used to remove an existing item to make space for a new one. Consider using a library that provides a bounded cache implementation.
🟡 Warning	`kv/internal/cache/cache.go:40`	Secrets are cached in memory as `string` values within the `cacheEntry` struct. In Go, strings are immutable, which makes it impossible to deterministically wipe the memory segment containing the secret after it's no longer needed. The secret data may persist in memory until it is overwritten by the garbage collector, making it vulnerable to being captured in memory dumps or accessed by other processes on a compromised host. 💡 Suggestion For enhanced security, store secrets as byte slices (`[]byte`) instead of strings. This allows the memory to be explicitly zeroed out (e.g., by iterating over the slice and setting each byte to zero) as soon as the secret is no longer required, minimizing its exposure time in memory. For even stronger guarantees, consider using a specialized library to handle secrets in protected memory regions.

Security Issues (3)

Severity	Location	Issue
🟡 Warning	`kv/errors.go:42-44`	Error messages in `StoreUnavailableError` include the underlying provider error. If these errors are propagated to external systems or logs with insufficient access controls, they could reveal sensitive internal architecture details, such as network addresses or stack traces from the provider's SDK. 💡 Suggestion Consider redacting sensitive information from errors that may leave the service boundary. A common pattern is to have a detailed internal error for logging and a generic, sanitized error for external responses. For `StoreUnavailableError`, consider logging the full `e.Err` internally but not including its string representation in the error message returned to the caller.
🟡 Warning	`kv/internal/cache/cache.go:25`	The in-memory cache (`Cache.entries`) has no upper bound on its size. It only evicts entries when their TTL expires. An attacker or a misbehaving client capable of requesting a large number of unique keys could cause the cache to grow indefinitely, leading to excessive memory consumption and a potential denial-of-service (DoS) attack. 💡 Suggestion Introduce a configurable capacity limit for the cache. When the limit is reached, an eviction strategy such as LRU (Least Recently Used) should be used to remove an existing item to make space for a new one. Consider using a library that provides a bounded cache implementation.
🟡 Warning	`kv/internal/cache/cache.go:40`	Secrets are cached in memory as `string` values within the `cacheEntry` struct. In Go, strings are immutable, which makes it impossible to deterministically wipe the memory segment containing the secret after it's no longer needed. The secret data may persist in memory until it is overwritten by the garbage collector, making it vulnerable to being captured in memory dumps or accessed by other processes on a compromised host. 💡 Suggestion For enhanced security, store secrets as byte slices (`[]byte`) instead of strings. This allows the memory to be explicitly zeroed out (e.g., by iterating over the slice and setting each byte to zero) as soon as the secret is no longer required, minimizing its exposure time in memory. For even stronger guarantees, consider using a specialized library to handle secrets in protected memory regions.

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

Severity	Location	Issue
🟠 Error	`kv/registry/registry.go:275-287`	The loop for closing stores contains two separate concurrency issues. First, the goroutine captures the loop variables `name` and `store` by reference, which will cause all goroutines to operate on the values from the final loop iteration. Second, it calls `wg.Go(...)`, but `sync.WaitGroup` does not have a `Go` method, which will cause a compilation error. 💡 Suggestion To fix both issues, create local copies of the loop variables for each iteration, and use the standard `wg.Add(1)` and `go func() { defer wg.Done() ... }()` pattern to manage the goroutines. 🔧 Suggested Fix `for name, store := range stores { name, store := name, store // Capture loop variables for the goroutine 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() } } }() }`

Performance Issues (2)

Severity	Location	Issue
🟠 Error	`kv/registry/registry.go:281-291`	The code uses `wg.Go(...)` on a `sync.WaitGroup` variable. The `Go` method does not exist on `sync.WaitGroup` in the Go version (1.21) used by this project's CI, and this will cause a compilation error. This non-standard pattern is also used in `kv/registry/registry_test.go`, `kv/internal/store/store_test.go`, and `kv/internal/cache/cache_test.go`. 💡 Suggestion Replace the non-standard `wg.Go(...)` call with the standard `wg.Add(1)` and `go func() { defer wg.Done(); ... }()` pattern to ensure correct concurrent execution and prevent compilation failures. Remember to capture loop variables when using this pattern inside a loop.
🟡 Warning	`kv/internal/cache/cache.go:189-215`	The cache `cleanup` function performs a full scan over all items in the cache (an O(N) operation) to find expired entries. While acceptable for small caches, this can lead to performance degradation, including CPU spikes and lock contention, in applications with a very large number of cached keys. The read lock held during the scan can starve writers, and the subsequent write lock to delete items blocks all cache access. 💡 Suggestion For improved scalability, consider using a more efficient data structure for tracking expirations, such as a min-heap or a time-ordered doubly-linked list. This would allow the cleanup process to find expired items in O(1) or O(log N) time, avoiding a full scan and reducing the duration of lock contention. This would increase complexity in the `Set` operation but would make the cleanup process much more efficient.

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-05-25T12:39:56.241Z | Triggered by: pr_updated | Commit: 05403cd

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

github-actions · 2026-05-25T12:40:14Z

🚨 Jira Linter Failed

Commit: 05403cd
Failed at: 2026-05-25 12:40:12 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 'In Refinement' but must be one of: In Code Review, Ready For Dev, Dod Check, In Dev

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.

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

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

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

TT-17100: Add core implementation of kv package#135
vladzabolotnyi wants to merge 10 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!

github-actions Bot commented May 25, 2026

Uh oh!

sonarqubecloud Bot commented May 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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!

github-actions Bot commented May 25, 2026

🚨 Jira Linter Failed

Next Steps

Uh oh!

sonarqubecloud Bot commented May 25, 2026

Quality Gate passed

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