Skip to content

feat: add cloud storage support#142

Closed
ddupg wants to merge 1 commit intoCortexReach:masterfrom
ddupg:feat-storage-options-on-master
Closed

feat: add cloud storage support#142
ddupg wants to merge 1 commit intoCortexReach:masterfrom
ddupg:feat-storage-options-on-master

Conversation

@ddupg
Copy link

@ddupg ddupg commented Mar 10, 2026

closes: #75

Add cloud storage support for LanceDB

  • This PR adds support for connecting to cloud-based LanceDB instances by introducing a new storageOptions configuration parameter.
  • This PR only adds cloud storage support for the primary store and does not cover the migration path.

Changes

  • Plugin configuration: Added storageOptions field to accept key-value string pairs for LanceDB connection options
  • Cloud URL detection: Automatically detects cloud storage URLs (containing ://) and skips local filesystem validation
  • LanceDB connection: Passes storageOptions to the lancedb.connect() call for cloud authentication and configuration
  • Schema updates: Updated plugin JSON schema and UI labels for the new configuration option

Usage

Users can now configure cloud storage connections by providing storage options in the plugin configuration:

{                                                                                                                                                                                                           
  dbPath: s3://my-bucket/path,                                                                                                                                                                              
  storageOptions: {
    access_key_id: your-key,
    secret_access_key: your-secret,
    aws_endpoint: bucket endpoint,
  }                                                                                                                                                                                                         
}                                                                                                                                                                                                           

This enables the plugin to work with LanceDB Cloud, S3-backed storage, and other remote storage backends without requiring local filesystem access.

Change-Id: I809cd1d768ad818c6c831bed622d1ab42cd29dbc
@ddupg
Copy link
Author

ddupg commented Mar 10, 2026

The failing cli-smoke test to be unrelated to the changes in my PR, and I have opened #154 to fix this.

@AliceLJY
Copy link
Collaborator

Codex static security review summary:

I think this should also be fix-then-merge. Cloud storage support is useful, but I see a few security / correctness gaps in the current implementation:

  1. The new CLI flag --storage-options <json> encourages users to place secrets directly in command-line arguments. That exposes credentials to shell history and, on some systems, process inspection.

  2. Cloud-path detection is currently scheme://-based, which is too broad. It also matches file:// and other unknown schemes, so local-file URIs can bypass the existing local path validation path.

  3. MemoryStore starts reading config.storageOptions, but StoreConfig itself is not updated to declare that field. That breaks the TypeScript contract.

  4. The new test/plugin-config.test.mjs coverage is not wired into npm test, so the added validation / env-resolution / path-detection cases are not protected by the main CI path.

  5. CLI validation is weaker than plugin-config validation right now: non-string storage-option values are silently dropped in CLI mode instead of being rejected.

Suggested merge criteria:

  • redesign or remove the raw CLI JSON secret path
  • narrow remote-path detection and treat file:// as local
  • add storageOptions to StoreConfig
  • hook the new test file into npm test
  • align CLI validation with plugin-config validation

My conclusion: fix-then-merge.

@AliceLJY AliceLJY closed this Mar 10, 2026
@ddupg
Copy link
Author

ddupg commented Mar 11, 2026

Thanks @AliceLJY for reviewing. I'm currently working on addressing your comments.

May I ask why this PR was closed? I'd be happy to reopen it if needed.

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