Skip to content

feat: add encrypt doc && refactoring encrypt code#893

Merged
qin-ctx merged 5 commits intovolcengine:mainfrom
baojun-zhang:encrypt-feat
Mar 23, 2026
Merged

feat: add encrypt doc && refactoring encrypt code#893
qin-ctx merged 5 commits intovolcengine:mainfrom
baojun-zhang:encrypt-feat

Conversation

@baojun-zhang
Copy link
Collaborator

@baojun-zhang baojun-zhang commented Mar 23, 2026

Description

feat: add encrypt doc && fix Partial reads/wrong location problem && refactoring duplicated code && add vault / volcengine ms mock/integration test.

Related Issue

#827
prev pr:
#828

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Changes Made

  • add encrypt document
  • fix Partial reads problem
  • fix wrong location problem
  • VaultProvider/VolcengineKMSProvider support different accounts use independent encryption keys for isolation
  • refactoring duplicated code
  • add vault / volcengine ms mock/integration test.

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Screenshots (if applicable)

Additional Notes

@github-actions
Copy link

Failed to generate code suggestions for PR

Copy link
Collaborator

@qin-ctx qin-ctx left a comment

Choose a reason for hiding this comment

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

Summary

This PR adds encryption documentation, refactors provider code (extracting BaseProvider, unifying encrypt_file_key/decrypt_file_key signatures), fixes partial-read bugs in VikingFS, and adds Vault/Volcengine KMS integration tests.

4 blocking issues found:

  1. VaultProvider._get_or_create_root_key references an undefined encrypted_root_key variable in the except branch, preventing root key persistence on first deployment.
  2. Vault KV storage hardcodes mount_point="secret" and uses KV v1 API — incompatible with KV v2 or custom mount paths.
  3. Both VaultProvider and VolcengineKMSProvider silently fall back to in-memory root keys when persistence fails, risking permanent data loss on restart.
  4. This PR removes auth_mode, trusted mode, Prometheus metrics, and stats/metrics routers — breaking changes unrelated to encryption that are not mentioned in the PR description.

@baojun-zhang baojun-zhang requested a review from qin-ctx March 23, 2026 09:29
Copy link
Collaborator

@qin-ctx qin-ctx left a comment

Choose a reason for hiding this comment

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

Review Summary

Great progress from the previous iteration — all 7 issues from the last review have been addressed. The BaseProvider extraction, unified encrypt_file_key/decrypt_file_key signatures, and proper root key persistence with fatal-on-failure semantics are all solid improvements.

1 blocking issue found: VaultProvider._ensure_root_key_exists references the removed class constant ROOT_KEY_NAME (should be instance variable root_key_name), which will crash at runtime.

Previous review issues status:

  • encrypted_root_key undefined in except branch — fixed
  • ✅ Hardcoded mount_point="secret" / KV v1 only — fixed (configurable kv_mount_path + kv_version)
  • ✅ Silent fallback to ephemeral root key — fixed (raises ConfigError)
  • ✅ Same issue in VolcengineKMSProvider — fixed
  • ✅ Unrelated auth_mode/metrics removals — reverted from PR
  • ✅ Hardcoded root key file path — fixed (configurable key_file)
  • ✅ Unrelated clean_stale_rocksdb_locks — reverted from PR

@baojun-zhang baojun-zhang requested a review from qin-ctx March 23, 2026 10:16
Copy link
Collaborator

@qin-ctx qin-ctx left a comment

Choose a reason for hiding this comment

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

All issues from the previous review have been resolved:

  • self.ROOT_KEY_NAMEself.root_key_name in _ensure_root_key_exists (3 occurrences)
  • VolcengineKMSProvider._encrypt_with_kms / _decrypt_with_kms now use asyncio.to_thread
  • ✅ Design doc with typo filename removed from PR

LGTM. The encryption refactoring is clean — unified provider interface via BaseProvider, proper root key persistence with fatal-on-failure, and correct partial-read handling in VikingFS.

@qin-ctx qin-ctx merged commit 5bbb22a into volcengine:main Mar 23, 2026
5 checks passed
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants