Skip to content

Multi tenancy 2#81

Open
JimA-cyborg wants to merge 8 commits into
mainfrom
multi-tenancy-2
Open

Multi tenancy 2#81
JimA-cyborg wants to merge 8 commits into
mainfrom
multi-tenancy-2

Conversation

@JimA-cyborg
Copy link
Copy Markdown
Contributor

@JimA-cyborg JimA-cyborg commented May 27, 2026

Multi tenancy 2 (#81)

Summary

Adds per-index KMS / BYOK support to the TypeScript SDK, bringing it to parity
with the Python (cyborgdb-py) and Go (cyborgdb-go) SDKs. index_key becomes
optional across the API and a new kmsName parameter lets the service manage
the encryption key, so an index can be created and used without the SDK ever
holding a key.

Three key-management modes are now supported:

  1. SDK-managed (legacy) — pass indexKey, omit kmsName.
  2. KMS-fully-managed — pass kmsName (real provider: aws-kms / aws),
    omit indexKey; the service generates and wraps the KEK.
  3. provider: none + SDK KEK — pass both; the registry slot tracks the
    index while the SDK supplies the KEK on each request.

Motivation

Tracks the cyborgdb-service per-index KMS / multi-tenancy work (BYOK). Customers
want index keys managed by a KMS in their own account rather than held by the
SDK. The SDK side of that is small but cross-cutting: every request that carried
an index_key must now be able to omit it, and createIndex must accept a KMS
slot reference. Keeping the JS SDK one-for-one with py/go avoids divergent
behavior across language clients against the same service.

Changes

  • Regenerated the OpenAPI client against the v0.16.1 spec (copied from
    cyborgdb-py): index_key is optional on every request model and
    CreateIndexRequest gains kms_name.
  • createIndex: added optional kmsName, made indexKey optional, added a
    local guard ("requires indexKey, kmsName, or both") and shared 32-byte
    key-length validation; absent fields are omitted from the wire.
  • loadIndex / describeIndex: indexKey optional; key omitted from the
    request when absent; same key-length validation as createIndex.
  • EncryptedIndex: holds an optional key, hex-encoded once in the constructor
    and applied to every data-plane request via a withKey() helper (matches
    py's _index_key_hex / go's stored hex).
  • EncryptedIndex metadata caching to match py/go: getIndexName() returns the
    in-memory name with no API call; getIndexType() / getIndexConfig() are
    fetched once and cached (immutable); isTrained() always fetches fresh.
  • Extracted the duplicated handleApiError (+ type guards) and
    extractErrorDetail into a shared src/errors.ts; diagnostic logging is now
    gated behind CYBORGDB_DEBUG (off by default).
  • LangChain vector store: optional indexKey + kmsName pass-through.
  • Dropped a dead _embeddingModel constructor param.
  • Tooling: fixed the update-openapi-client.sh clean-line (src/models /
    src/apis) and added index.ts to src/.openapi-generator-ignore so regen
    no longer clobbers the hand-written public exports.
  • README: added a BYOK / three-mode section.
  • kms.md: internal re-implementation guide — will be deleted before merge.

Testing

  • Unit tests added/updated — new src/__tests__/kms.test.ts:
    • Offline contract suite (no service required, runs in CI): create guard,
      key-length validation on create + load, CreateIndexRequest wire shape,
      keyless vs keyed request bodies across all data-plane ops including the
      binary upsert/query paths, and the keyless loadIndex describe shape.
    • Live suites (env-gated): the three KMS modes, negative paths (unknown
      slot, real-provider + key rejected, provider:none missing/wrong key), and
      mixed-mode concurrency.
  • Integration tests pass — live suites require a service configured with
    kms.registry slots; run with CYBORGDB_KMS_NAME_{REAL,SM,NONE} set against
    such a service (not exercised in standard CI).
  • Manual testing notes: offline suite verified locally (11 passed);
    build + lint + tsc all green. Live KMS suites not yet run against a
    configured service — needs a deployment with the three registry slots.

Companion negative tests were also added to cyborgdb-py and cyborgdb-go
in their respective repos so the "real-provider + key is rejected" contract is
covered identically across all three SDKs.

Risk assessment

  • Blast radius: Touches the core client surface (createIndex/loadIndex)
    and every data-plane request in EncryptedIndex, plus regenerated models.
    However the change is additive/relaxing — index_key went from required to
    optional, existing keyed call sites are unchanged, and the offline tests pin
    the exact wire shapes for both keyed and keyless paths. The metadata-caching
    change makes getIndexName() stop hitting the server (it returns the
    in-memory name) — behavior-aligned with py/go but worth noting for anyone who
    relied on it as an implicit existence probe.
  • Rollback plan: Self-contained on the multi-tenancy-2 branch; revert the
    PR. The SDK is versioned/published independently, so reverting the package
    release restores prior behavior with no service-side coordination. No data
    migration and no persisted state change on the client.

Reviewers, please focus on:

  • The keyless request plumbing in EncryptedIndex (withKey() + cached hex):
    confirm no data-plane path can leak or drop index_key incorrectly.
  • Parity with py/go for the three modes and the load-time key validation.
  • The getIndexName() behavior change (no longer an API call) — acceptable?
  • The shared errors.ts extraction (handleApiError consolidation) and the
    CYBORGDB_DEBUG logging gate.
  • Confirm kms.md is removed before merge.

Comment thread src/__tests__/kms.test.ts Outdated
Comment thread README.md
Comment thread src/client.ts
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ API Contract Change Detected

This PR modifies the public API contract of the CyborgDB JS SDK.

Please provide an explanation for this change:

  • Why is this change necessary?
  • Is this a breaking change or backward compatible?
  • Have you updated the documentation?

This review must be dismissed or addressed before the PR can be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants