feat(crypto): v2 volume encryption support #424
Conversation
mantissahz
requested review from
innobead,
derekbit,
shuo-wu,
james-munson and
a team
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant CLI as Command Line Interface
participant Server as Server Command
participant Volume as Volume Manager
participant Crypto as Crypto Service
participant RPC as RPC Server
CLI->>Server: Specify dataEngine flag
Server->>Volume: Create Volume with DataEngine
Volume-->>Crypto: Open Volume with DataEngine
Crypto-->>RPC: Perform Volume Operations
RPC->>Volume: Generate Device Path
Volume-->>Crypto: Close Volume with DataEngine
The sequence diagram illustrates the flow of data engine specification from CLI through volume management, crypto operations, and RPC server interactions, highlighting the new Finishing Touches
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
pkg/types/types.go (2)
20-21: Consider using an enum-like type for DataEngine valuesTo prevent potential typos and invalid values, consider defining a custom type with constants:
-DataEngineTypeV1 = "v1" -DataEngineTypeV2 = "v2" +type DataEngineType string + +const ( + DataEngineTypeV1 DataEngineType = "v1" + DataEngineTypeV2 DataEngineType = "v2" +)
31-31: Follow Go naming conventions for parametersThe parameter
EncryptedDeviceshould beencryptedDeviceto follow Go naming conventions for function parameters.pkg/volume/volume.go (1)
14-14: Add documentation for DataEngine fieldPlease add a comment explaining the valid values and purpose of this field.
+ // DataEngine specifies the volume data engine type (v1 or v2) DataEngine stringpkg/crypto/crypto.go (1)
67-72: Improve error message clarityThe error message could be more descriptive about the actual issue.
- return fmt.Errorf("volume %v encrypto device %s is closed for resizing", volume, encryptedDevPath) + return fmt.Errorf("cannot resize volume %v: encrypted device %s is not open", volume, encryptedDevPath)pkg/server/share_manager.go (3)
204-206: Consider enhancing error handling for crypto operationsWhile the v2 volume support integration is correct, consider wrapping the crypto-related errors with more context to aid in debugging:
cryptoDevice := types.GetVolumeDevicePath(vol.Name, vol.DataEngine, true) m.logger.Infof("Volume %s requires crypto device %s", vol.Name, cryptoDevice) -if err := crypto.OpenVolume(vol.Name, vol.DataEngine, devicePath, vol.Passphrase); err != nil { - m.logger.WithError(err).Error("Failed to open encrypted volume") - return "", err +if err := crypto.OpenVolume(vol.Name, vol.DataEngine, devicePath, vol.Passphrase); err != nil { + return "", errors.Wrapf(err, "failed to open encrypted volume %v with data engine %v", vol.Name, vol.DataEngine) }
220-225: Consider enhancing error handling for crypto device teardownWhile the v2 volume support integration is correct, consider wrapping the crypto-related errors with more context:
cryptoDevice := types.GetVolumeDevicePath(vol.Name, vol.DataEngine, true) if isOpen, err := crypto.IsDeviceOpen(cryptoDevice); err != nil { - return err + return errors.Wrapf(err, "failed to check if crypto device %v is open", cryptoDevice) } else if isOpen { m.logger.Infof("Volume %s has active crypto device %s", vol.Name, cryptoDevice) if err := crypto.CloseVolume(vol.Name, vol.DataEngine); err != nil { - return err + return errors.Wrapf(err, "failed to close crypto device %v with data engine %v", cryptoDevice, vol.DataEngine) } m.logger.Infof("Volume %s closed active crypto device %s", vol.Name, cryptoDevice) }
Line range hint
204-225: Architectural feedback on v2 volume encryption supportThe integration of v2 volume support is well-structured and maintains a clear separation of concerns. The changes consistently handle the data engine parameter across all relevant operations (setup, teardown, and device path resolution).
Consider documenting the differences between v1 and v2 volume encryption in the code comments to help future maintainers understand the architectural decisions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/cmd/server.go(2 hunks)pkg/crypto/crypto.go(3 hunks)pkg/rpc/server.go(4 hunks)pkg/server/share_manager.go(3 hunks)pkg/types/types.go(1 hunks)pkg/volume/volume.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (4)
pkg/rpc/server.go (3)
72-72: LGTM: Device path handling updated for v2 volume supportThe addition of
vol.DataEngineparameter is consistent with the v2 volume encryption support objective.
137-137: LGTM: Filesystem resize handling updated for v2 volume supportThe changes properly integrate v2 volume support in both device path retrieval and crypto device resizing operations.
Also applies to: 164-164
312-312: LGTM: Mount operation updated for v2 volume supportThe device path retrieval has been properly updated to support v2 volumes.
pkg/server/share_manager.go (1)
106-106: LGTM: Run method updated for v2 volume supportThe device path retrieval has been properly updated to support v2 volumes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/crypto/crypto.go (2)
32-33: Enhance parameter documentation.The function documentation should describe all parameters. Add documentation for
volume,dataEngine, andpassphraseparameters.// OpenVolume opens volume so that it can be used by the client. // attachingVolumeHostPath is the path of the volume on the host that will be opened. +// volume is the unique identifier of the volume +// dataEngine specifies the data engine type (v1 or v2) +// passphrase is the encryption key for the LUKS device
73-73: Enhance error message clarity.Consider including the data engine type in the error message for better debugging context.
- return fmt.Errorf("volume %v encrypto device %s is closed for resizing", volume, encryptedDevPath) + return fmt.Errorf("volume %v encrypto device %s (data engine: %s) is closed for resizing", volume, encryptedDevPath, dataEngine)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/cmd/server.go(5 hunks)pkg/crypto/crypto.go(3 hunks)pkg/rpc/server.go(4 hunks)pkg/server/share_manager.go(3 hunks)pkg/types/types.go(1 hunks)pkg/volume/volume.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/volume/volume.go
- app/cmd/server.go
- pkg/types/types.go
- pkg/server/share_manager.go
- pkg/rpc/server.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/crypto/crypto.go (1)
55-64: LGTM! Clean implementation of CloseVolume.The changes to include the dataEngine parameter are consistent with the overall design, and the implementation maintains proper error handling.
ref: longhorn/longhorn 7355 Signed-off-by: James Lu <jamesluhz@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/types/types.go (1)
24-29: Consider adding validation for dataEngine parameter.The function logic is correct and aligns with the default behavior. However, consider adding validation for the dataEngine parameter to prevent runtime errors with invalid values.
func GetEncryptVolumeName(volume, dataEngine string) string { + if dataEngine != DataEngineTypeV1 && dataEngine != DataEngineTypeV2 { + logrus.Warnf("Invalid data engine type: %s, defaulting to v1", dataEngine) + return volume + } if dataEngine == DataEngineTypeV2 { return volume + MapperV2VolumeSuffix } return volume }pkg/crypto/crypto.go (3)
32-33: Document the dataEngine parameter.The comment explains the devicePath parameter but should also document the dataEngine parameter and its expected values (v1/v2).
// OpenVolume opens volume so that it can be used by the client. // devicePath is the path of the volume on the host that will be opened for instance '/dev/longhorn/volume1' +// dataEngine specifies the volume data engine type (v1 or v2)
56-56: Add function documentation.Add documentation for the function parameters to improve code maintainability.
+// CloseVolume closes encrypted volume so it can be detached. +// volume is the name of the volume to close +// dataEngine specifies the volume data engine type (v1 or v2) func CloseVolume(volume, dataEngine string) error {
75-75: Fix typo in error message."encrypto" should be "encrypted" in the error message.
- return fmt.Errorf("volume %v encrypto device %s is closed for resizing", volume, devPath) + return fmt.Errorf("volume %v encrypted device %s is closed for resizing", volume, devPath)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/cmd/server.go(5 hunks)pkg/crypto/crypto.go(3 hunks)pkg/rpc/server.go(4 hunks)pkg/server/share_manager.go(3 hunks)pkg/types/types.go(1 hunks)pkg/volume/volume.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/volume/volume.go
- app/cmd/server.go
- pkg/server/share_manager.go
- pkg/rpc/server.go
🧰 Additional context used
📓 Learnings (1)
pkg/types/types.go (1)
Learnt from: mantissahz
PR: longhorn/longhorn-share-manager#424
File: pkg/types/types.go:24-29
Timestamp: 2025-01-10T08:08:22.207Z
Learning: In the Longhorn Share Manager, the `dataEngine` parameter defaults to `v1`. This is reflected in the `GetEncryptVolumeName` function's behavior where any non-v2 data engine type returns the volume name without modification.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/types/types.go (2)
15-21: LGTM! Well-documented constants.The new constants are clearly documented and follow Go naming conventions.
31-37: LGTM! Well-documented device path handling.The function clearly handles different device path scenarios with good documentation explaining the v2 volume behavior.
|
@mergify backport v1.8.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
Issue # longhorn/longhorn#7355
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context