feat: add KVStoreWrapper with owner migration adapter for vault org_id migration#21691
feat: add KVStoreWrapper with owner migration adapter for vault org_id migration#21691prashantkumar1982 wants to merge 3 commits intodevelopfrom
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.
|
👋 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! |
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
|
|
||
| var _ ReadKVStore = (*OwnerMigrationReadStore)(nil) | ||
|
|
||
| func NewOwnerMigrationReadStore(inner ReadKVStore, orgID, workflowOwner string) *OwnerMigrationReadStore { |
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
There was a problem hiding this comment.
Yes makes sense. Let me change it
| } | ||
|
|
||
| // org_id entries take priority in deduplication. | ||
| addEntries(orgMd) |
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?
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.
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
|
2 participants
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.