feat: add KVStoreWrapper with owner migration adapter for vault org_id migration#21691
Merged
prashantkumar1982 merged 6 commits intodevelopfrom Apr 1, 2026
Merged
Conversation
Add a transparent adapter layer (OwnerMigrationReadStore / OwnerMigrationWriteStore) that sits between the vault plugin and the KV store to handle the migration of secrets from workflow_owner-keyed entries to org_id-keyed entries. The adapter implements the same ReadKVStore / WriteKVStore interfaces and provides: - Dual-owner lookup on reads (org_id first, workflow_owner fallback) - Metadata merging and deduplication across both owners for list operations - org_id-based writes for all new/updated secrets - Lazy migration: deletes legacy workflow_owner entries on update - Dual-owner deletion with cleanup of both owners - Pass-through for pending queue operations (not owner-scoped) This is a standalone component (not wired into the plugin yet) with comprehensive unit tests covering all operations and migration scenarios.
Contributor
|
👋 prashantkumar1982, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
Contributor
|
I see you updated files related to
|
Contributor
|
✅ No conflicts with other open PRs targeting |
|
|
||
| var _ ReadKVStore = (*OwnerMigrationReadStore)(nil) | ||
|
|
||
| func NewOwnerMigrationReadStore(inner ReadKVStore, orgID, workflowOwner string) *OwnerMigrationReadStore { |
Contributor
There was a problem hiding this comment.
I'm not sure how this would be transparent since this implies that we'll have one read store per owner/org ID. Atm we typically instantiate one readStore for every plugin function call
Contributor
Author
There was a problem hiding this comment.
Yes makes sense. Let me change it
| } | ||
|
|
||
| // org_id entries take priority in deduplication. | ||
| addEntries(orgMd) |
Contributor
There was a problem hiding this comment.
Is there a risk that the merged list will end up having more than the max allowed number of secrets?
Contributor
Author
There was a problem hiding this comment.
No — the merged list cannot exceed the max. mergeMetadata deduplicates entries by namespace::key, so a secret that temporarily exists under both org_id and workflow_owner (transient mid-migration state) is counted only once. The deduplicated count reflects the true number of unique secrets the owner has, which is what MaxSecretsPerOwner was originally enforced against during creation.
Added a clarifying comment on GetMetadata in the updated code.
Contributor
Author
There was a problem hiding this comment.
Basically the enforcement of max secrets will be done in CreateSecrets flow.
Restructure the owner migration layer into two types: - KVStoreWrapper (exported): the abstraction the plugin interacts with - ownerMigrationAdapter (unexported): internal migration logic orgID/workflowOwner are passed per method call rather than stored in the struct, matching the plugin's one-store-per-call pattern where a single instance processes batches with different owners. Add Criticalw log on unexpected duplicate during metadata merge. Rename files: owner_migration_store.go → kvstore_wrapper.go. Made-with: Cursor
prashantkumar1982
changed the title
… into feat/pr5-owner-migration-adapter
Add migrationEnabled bool to KVStoreWrapper constructor. When false, all methods pass through directly to the inner KVStore, bypassing the ownerMigrationAdapter entirely. The caller resolves the gate from cresettings.Default.VaultOrgIdAsSecretOwnerEnabled. Add 14 unit tests verifying passthrough behavior when disabled. Made-with: Cursor
cedric-cordenier
previously approved these changes
Apr 1, 2026
timothyF95
previously approved these changes
Apr 1, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
prashantkumar1982
dismissed stale reviews from cedric-cordenier and timothyF95
via
2cbdcec
…r develop merge - Add context.Context as first param to all KVStoreWrapper and adapter methods - Rename test helper to newMigrationTestStore to avoid redeclaration with kvstore_test.go - Update NewWriteStore calls to include pluginMetrics parameter Made-with: Cursor
prashantkumar1982
force-pushed
the
feat/pr5-owner-migration-adapter
branch
from
2cbdcec to
cbf9999
Compare
|
vreff
approved these changes
Apr 1, 2026
jmank88
reviewed
Apr 1, 2026
| } | ||
|
|
||
| func TestKVStoreWrapper_CreateOldFlow_ListNewFlow(t *testing.T) { | ||
| ctx := context.Background() |
Contributor
There was a problem hiding this comment.
Should use the test context for these:
Suggested change
| ctx := context.Background() | |
| ctx := t.Context() |
jmank88
approved these changes
Apr 1, 2026
prashantkumar1982
added a commit
that referenced
this pull request
Apr 2, 2026
…d migration (#21691) * feat: add OwnerMigrationStore adapter for vault org_id migration Add a transparent adapter layer (OwnerMigrationReadStore / OwnerMigrationWriteStore) that sits between the vault plugin and the KV store to handle the migration of secrets from workflow_owner-keyed entries to org_id-keyed entries. The adapter implements the same ReadKVStore / WriteKVStore interfaces and provides: - Dual-owner lookup on reads (org_id first, workflow_owner fallback) - Metadata merging and deduplication across both owners for list operations - org_id-based writes for all new/updated secrets - Lazy migration: deletes legacy workflow_owner entries on update - Dual-owner deletion with cleanup of both owners - Pass-through for pending queue operations (not owner-scoped) This is a standalone component (not wired into the plugin yet) with comprehensive unit tests covering all operations and migration scenarios. * refactor: rename to KVStoreWrapper with internal ownerMigrationAdapter Restructure the owner migration layer into two types: - KVStoreWrapper (exported): the abstraction the plugin interacts with - ownerMigrationAdapter (unexported): internal migration logic orgID/workflowOwner are passed per method call rather than stored in the struct, matching the plugin's one-store-per-call pattern where a single instance processes batches with different owners. Add Criticalw log on unexpected duplicate during metadata merge. Rename files: owner_migration_store.go → kvstore_wrapper.go. Made-with: Cursor * feat: gate KVStoreWrapper with migrationEnabled flag Add migrationEnabled bool to KVStoreWrapper constructor. When false, all methods pass through directly to the inner KVStore, bypassing the ownerMigrationAdapter entirely. The caller resolves the gate from cresettings.Default.VaultOrgIdAsSecretOwnerEnabled. Add 14 unit tests verifying passthrough behavior when disabled. Made-with: Cursor * fix: update KVStoreWrapper for context.Context interface changes after develop merge - Add context.Context as first param to all KVStoreWrapper and adapter methods - Rename test helper to newMigrationTestStore to avoid redeclaration with kvstore_test.go - Update NewWriteStore calls to include pluginMetrics parameter Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
KVStoreWrapperincore/services/ocr2/plugins/vault/kvstore_wrapper.go— a migration-aware layer over the vault KVStore that handles the transition fromworkflow_owner-keyed entries toorg_id-keyed entries.KVStoreWrapperis the public abstraction the plugin will interact with. Internally it delegates to an unexportedownerMigrationAdapterthat contains the dual-owner migration logic.orgIDandworkflowOwnerare passed per method call (not stored in the struct), matching the plugin's one-store-per-call pattern where a single instance processes batches with different owners.This is a standalone component in the JWT auth rollout plan — not wired into the plugin yet. No behavior change.