feat: support v2 backing image cloning and encryption #316
Conversation
|
Warning Rate limit exceeded@ChanYiLin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (6)
WalkthroughThis pull request introduces encryption support for backing images by adding a new package to handle LUKS-based encryption. It enhances the backing image management workflow by updating client, SPDK, and server components to accept new encryption parameters and source backing image names, and by adding helper methods for encryption operations. In addition, a new encryption type and associated constants are added to define various encryption states. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant S as SPDKServer
participant SC as SPDKClient
participant BI as BackingImage
participant EM as EncryptionModule
participant LUKS as LUKS System
C->>S: BackingImageCreate(request with encryption, srcBackingImageName, credential)
S->>SC: Invoke BackingImageCreate method
SC->>BI: Call Create(srcBackingImageName, encryption, credential, ...)
BI->>EM: Check if encryption is enabled and setup encryption
EM->>LUKS: Execute LUKS encryption commands (EncryptBackingImage, etc.)
LUKS-->>EM: Return encryption result/status
EM-->>BI: Return encryption status and mapping info
BI-->>SC: Return created backing image details
SC-->>S: Forward backing image details
S-->>C: Return final backing image response
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
ChanYiLin
force-pushed
the
LH9996_support_v2_backing_image_cloning
branch
from
4e12490 to
5201477
Compare
ChanYiLin
requested review from
derekbit and
shuo-wu
and removed request for
derekbit
ChanYiLin
force-pushed
the
LH9996_support_v2_backing_image_cloning
branch
from
5201477 to
af348dc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
pkg/spdk/backing_image.go (1)
695-772: Ensure robust cleanup for encryption workflows.
In “prepareFromSync”, there’s more complexity around device mapper usage, opening/closing devices, and potential encryption or decryption steps. Confirm each error path properly closes or cleans up device operations to avoid device mapper leaks.
🧹 Nitpick comments (8)
pkg/backing_image_crypto/backing_image_crypto.go (2)
26-26: Consider documenting the 16MB offset.
Although 16MB is a reasonable space for encryption metadata, you might want to document how this was chosen. Explicit clarification can help others maintain or tune it in the future.
180-183: Address the hard-coded name prefix.
The function “GetLuksBackingImageName” uses the fixed prefix "v2backing". The “TODO” implies you plan a more generic or configurable approach.Do you want help to make this prefix configurable and open a new issue to track the refactoring?
pkg/spdk/backing_image.go (5)
420-421: Check for state validation early.
The new parameters (srcBackingImageName, encryption, credential) add logic complexity; consider validating them before launching this goroutine if invalid configurations would quickly fail.
582-590: Verify BIM download vs. SPDK sync logic.
The branching between “prepareFromURL” and “prepareFromSync” might need thorough tests for multi-step encryption scenarios.
793-793: Fix the spelling mistake.
Typo in the error message: “soruce” should be “source”.- return nil, errors.Wrapf(err, "failed to setup the crypto device with the soruce %v during syncing", sourcePath) + return nil, errors.Wrapf(err, "failed to setup the crypto device with the source %v during syncing", sourcePath)🧰 Tools
🪛 GitHub Check: codespell
[failure] 793-793:
soruce ==> source, spruce🪛 GitHub Actions: Codespell
[error] 793-793: soruce ==> source, spruce
786-810: Consider improved error context for openSource.
When “openSource” fails, it’s unclear if the passphrase or crypto parameters caused problems. Including more structured logs or wrapping errors with relevant details can help debugging.🧰 Tools
🪛 GitHub Check: codespell
[failure] 793-793:
soruce ==> source, spruce🪛 GitHub Actions: Codespell
[error] 793-793: soruce ==> source, spruce
812-836: Safety checks on the “openTarget” device.
It’s good that you verify the device path existence. For better reliability, confirm the device is not already in use by a leftover mapping before proceeding.pkg/client/client.go (1)
828-852: LGTM with a minor suggestion for error handling.The function signature and implementation correctly support backing image cloning with encryption. Consider enhancing the error message to be more descriptive when parameters are missing.
Apply this diff to improve error messages:
- return nil, fmt.Errorf("failed to start SPDK backing image: missing required parameters") + return nil, fmt.Errorf("failed to start SPDK backing image: missing required parameters (name: %v, uuid: %v, lvsUUID: %v, srcBackingImageName: %v, size: %v)", + name, backingImageUUID, lvsUUID, srcBackingImageName, size)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/longhorn/types/pkg/generated/spdkrpc/spdk.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (5)
pkg/backing_image_crypto/backing_image_crypto.go(1 hunks)pkg/client/client.go(1 hunks)pkg/spdk/backing_image.go(11 hunks)pkg/spdk/server.go(4 hunks)pkg/types/types.go(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
pkg/backing_image_crypto/backing_image_crypto.go
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 GitHub Check: codespell
pkg/spdk/backing_image.go
[failure] 793-793:
soruce ==> source, spruce
🪛 GitHub Actions: Codespell
pkg/spdk/backing_image.go
[error] 793-793: soruce ==> source, spruce
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (9)
pkg/backing_image_crypto/backing_image_crypto.go (1)
19-19: Ignore Gitleaks false positive.
The identifier "aes-xts-plain64" is not an actual key but a default cipher name. No sensitive credential is leaked here.🧰 Tools
🪛 Gitleaks (8.21.2)
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
pkg/spdk/backing_image.go (3)
26-36: Imports organization looks good.
The newly added imports for “lhtypes” and “backingimagecrypto” enhance encryption support.
451-453: Handle adjusted “ProcessedSize” carefully.
When encrypting, you’re adding 16MB to ProcessedSize. Ensure that you account for partial writes or other corner cases so that the final size is consistent once the operation completes.
838-862: Enhance “setupCryptoDevice” validations.
It’s beneficial that you verify the key provider. Consider validating passphrase length or restricting acceptable ciphers/hashes as a defense against misconfigurations.pkg/types/types.go (1)
57-63: EncryptionType additions look good.
Defining distinct string constants for encrypt, decrypt, and ignore helps keep the logic readable and avoids magic strings throughout the codebase. Consider adding short docstrings describing each encryption type’s effect.pkg/spdk/server.go (4)
289-290: LGTM! More precise size calculation.The change from using
NumAllocatedClusterstoNumBlocks * BlockSizeprovides a more accurate size calculation for backing images.
1581-1583: Verify the removal of checksum validation.The checksum validation has been commented out. Please clarify why this validation is no longer required, as it could impact data integrity verification.
Could you explain the rationale behind removing the checksum validation? This seems like a potential security concern.
1584-1589: LGTM! Good default encryption handling.The changes correctly:
- Validate source backing image name for cloning
- Set a default encryption type for backward compatibility
1618-1618: LGTM! Proper encryption parameter handling.The
Createcall correctly includes all necessary parameters for backing image cloning with encryption support.
ChanYiLin
force-pushed
the
LH9996_support_v2_backing_image_cloning
branch
from
af348dc to
ae89412
Compare
ref: longhorn/longhorn 9996 Signed-off-by: Jack Lin <[email protected]>
ChanYiLin
force-pushed
the
LH9996_support_v2_backing_image_cloning
branch
from
ae89412 to
bc8c25d
Compare
ref: longhorn/longhorn#9996
What this PR does / why we need it:
When cloning from another BackingImage with encryption and decryption
Special notes for your reviewer:
Current support table
Additional documentation or context