feat(crypto): v2 volume encryption #3438
Conversation
WalkthroughThe pull request introduces modifications across multiple files to enhance volume management, particularly for encrypted devices and data engine types. The changes focus on integrating Longhorn V2 engine support, updating crypto-related functions to handle different data engine types, and modifying validation logic for volume creation. The primary goal is to improve volume handling by adding more context-aware operations that can distinguish between different data engine versions. Changes
Sequence DiagramsequenceDiagram
participant NodeServer
participant CryptoModule
participant Volume
NodeServer->>CryptoModule: NodeStageVolume(volume, dataEngine)
CryptoModule->>CryptoModule: VolumeMapper(volume, dataEngine)
alt DataEngine is V2
CryptoModule-->>CryptoModule: Append '-encrypted' to volume name
end
CryptoModule->>Volume: Open/Stage Volume
Volume-->>CryptoModule: Volume Staged
CryptoModule-->>NodeServer: Return Status
The sequence diagram illustrates the enhanced volume staging process, showing how the data engine type influences volume mapping and staging operations. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (10)
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
|
mantissahz
force-pushed
the
issue7355
branch
2 times, most recently
from
76e1361 to
cc5802d
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
🧹 Nitpick comments (4)
csi/crypto/crypto.go (2)
Line range hint
103-120: Consider consolidating volume name handlingThe volume name handling logic is duplicated between
OpenVolumeandCloseVolume. Consider extracting this to a helper function.+func getEncryptVolumeName(volume, dataEngine string) string { + if dataEngine == string(longhorn.DataEngineTypeV2) { + return volume + mapperV2VolumeSuffix + } + return volume +} func OpenVolume(volume, dataEngine, devicePath, passphrase string) error { - encryptVolumeName := volume - if dataEngine == string(longhorn.DataEngineTypeV2) { - encryptVolumeName = volume + mapperV2VolumeSuffix - } + encryptVolumeName := getEncryptVolumeName(volume, dataEngine) logrus.Infof("Opening device %s with LUKS on %s", devicePath, encryptVolumeName)
128-140: Use consistent error handlingThe error handling in
CloseVolumecould be improved to match the pattern inOpenVolumewhere errors are logged with context.func CloseVolume(volume, dataEngine string) error { namespaces := []lhtypes.Namespace{lhtypes.NamespaceMnt, lhtypes.NamespaceIpc} nsexec, err := lhns.NewNamespaceExecutor(lhtypes.ProcessNone, lhtypes.HostProcDirectory, namespaces) if err != nil { return err } - encryptVolumeName := volume - if dataEngine == string(longhorn.DataEngineTypeV2) { - encryptVolumeName = volume + mapperV2VolumeSuffix - } + encryptVolumeName := getEncryptVolumeName(volume, dataEngine) logrus.Infof("Closing LUKS device %s", encryptVolumeName) _, err = nsexec.LuksClose(encryptVolumeName, lhtypes.LuksTimeout) + if err != nil { + logrus.WithError(err).Warnf("Failed to close LUKS device %s", encryptVolumeName) + } return err }webhook/resources/volume/validator.go (1)
170-171: LGTM: Clear validation for V2 volume limitationsThe validation correctly prevents the use of backing images with V2 volumes. The error message is clear and descriptive.
However, consider adding a comment explaining why backing images are not supported for V2 volumes to help future maintainers.
if types.IsDataEngineV2(volume.Spec.DataEngine) { + // Backing images are not supported for V2 volumes due to architectural limitations + // in the current implementation of the V2 data engine. if volume.Spec.BackingImage != "" { return werror.NewInvalidError("backing image is not supported for data engine v2", "") }csi/node_server.go (1)
519-519: Consider adding error recovery for crypto operationsThe crypto device operations could benefit from additional error recovery mechanisms. If
OpenVolumefails, we might want to attempt cleanup.+func (ns *NodeServer) cleanupCryptoDevice(volumeID, dataEngine string) { + if err := crypto.CloseVolume(volumeID, dataEngine); err != nil { + ns.log.WithError(err).Warnf("Failed to cleanup crypto device for volume %s", volumeID) + } +} if err := crypto.OpenVolume(volumeID, dataEngine, devicePath, passphrase); err != nil { + ns.cleanupCryptoDevice(volumeID, dataEngine) return nil, status.Error(codes.Internal, err.Error()) }Also applies to: 529-529, 534-534
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
csi/crypto/crypto.go(4 hunks)csi/node_server.go(6 hunks)webhook/resources/volume/validator.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (6)
csi/crypto/crypto.go (3)
13-13: LGTM: Clean addition of V2 supportThe import of the Longhorn API package and the addition of the V2 volume suffix constant are well-placed and follow the existing pattern.
Also applies to: 18-18
74-77: LGTM: Clear data engine type handlingThe
VolumeMapperfunction now correctly handles different data engine types by appending the-encryptedsuffix for V2 volumes. The implementation is clean and straightforward.
144-145: Verify error handling in ResizeEncryptoDeviceThe function checks if the device is open but doesn't verify if it's a V2 volume before proceeding. Consider adding validation for V2 volume support.
csi/node_server.go (3)
491-492: LGTM: Enhanced logging with data engine infoGood addition of data engine information in the log message for better debugging.
632-644: LGTM: Improved crypto device cleanup logicThe changes properly handle crypto device cleanup based on data engine type and shared access requirements.
Line range hint
825-853: Verify volume expansion support for V2 volumesThe code handles encrypted volume expansion, but we should verify if V2 volumes support this operation.
✅ Verification successful
V2 volumes support encrypted volume expansion
The implementation in
csi/node_server.gois engine-agnostic and works for both V1 and V2 volumes. The code only checks if the volume is encrypted and handles the crypto operations accordingly, using the dataEngine parameter only for constructing the appropriate mapper path.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if volume expansion is supported for V2 volumes # by looking for any restrictions or validations in the codebase. rg "DataEngineTypeV2.*expansion"Length of output: 34
Script:
#!/bin/bash # Search for V2 volume handling and expansion-related code rg "DataEngineTypeV2" -A 5 -B 5 # Search for volume expansion validations rg "NodeExpandVolume|ControllerExpandVolume" -A 10 # Search for data engine type checks during expansion rg "dataEngine.*expand|expand.*dataEngine" -A 5 -B 5Length of output: 116265
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
csi/crypto/crypto.go (1)
Line range hint
103-117: Enhance error message clarityWhile the implementation is correct, the error message could be more informative for debugging purposes.
Consider including the data engine type in the error message:
- logrus.WithError(err).Warnf("Failed to open LUKS device %s", devicePath) + logrus.WithError(err).Warnf("Failed to open LUKS device %s (engine: %s)", devicePath, dataEngine)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
csi/crypto/crypto.go(4 hunks)csi/node_server.go(6 hunks)webhook/resources/volume/validator.go(0 hunks)
💤 Files with no reviewable changes (1)
- webhook/resources/volume/validator.go
🚧 Files skipped from review as they are similar to previous changes (1)
- csi/node_server.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (5)
csi/crypto/crypto.go (5)
13-13: LGTM! Clean addition of imports and constantsThe new import and constant are well-structured and necessary for V2 engine support.
Also applies to: 18-18
124-129: LGTM! Well-structured helper functionClean implementation that properly encapsulates the volume name logic for different engine types.
132-132: LGTM! Clean adaptation for V2 engine supportThe changes maintain consistency with the rest of the implementation while properly supporting V2 volumes.
Also applies to: 139-141
Line range hint
74-156: Verify path handling securityWhile the implementation looks secure, let's ensure proper path handling throughout the crypto operations.
Let's check for potential path traversal vulnerabilities:
✅ Verification successful
Path handling implementation is secure
The crypto operations use proper path handling throughout:
- All paths are constructed using
path.Join- Fixed
/dev/mapperprefix with proper validation- Standard library path cleaning prevents directory traversal
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for path validation and sanitization rg -A 5 "path\.(Clean|Join|Base)" csi/ # Check for direct path concatenation rg -A 5 "\/.*\+" csi/Length of output: 2744
Script:
#!/bin/bash # Check for path-related constants in crypto package rg -A 2 "const.*[Pp]ath|var.*[Pp]ath" csi/crypto/ # Check for mapper-related constants rg -A 2 "const.*mapper|var.*mapper" csi/crypto/Length of output: 101
Script:
#!/bin/bash # Get the complete crypto.go file content cat csi/crypto/crypto.go # Search for variable declarations and imports rg -p "^(var|import|const)" csi/crypto/Length of output: 7298
74-77: LGTM! Verify volume name validationThe V2 engine path construction logic is clean and consistent. The
-encryptedsuffix addition is properly handled.Let's verify that volume names are properly validated upstream to prevent path traversal:
✅ Verification successful
Volume name validation is secure against path traversal
The implementation is safe because:
- Volume names are validated using strict regex
^[a-z0-9][a-z0-9_.-]+$- Path construction uses
path.Joinwith fixed prefix/suffix- Validation occurs in webhook before reaching the mapper
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for volume name validation in the codebase rg -A 5 "func.*[Vv]alidate.*[Vv]olume[Nn]ame"Length of output: 47
Script:
#!/bin/bash # Search for volume name patterns in CRD definitions and validation contexts rg -g '*.yaml' -g '*.go' -A 5 'volume.*name.*pattern|volume.*name.*validation' # Search for volume name usage in crypto context rg -g '*.go' -A 5 'volume.*name.*=|volume.*:=|volume\s+string' # Search for general name validation patterns ast-grep --pattern 'func $_(volume string) $$ { $$$ }'Length of output: 68340
Script:
#!/bin/bash # Search for ValidateName implementation rg -g '*.go' -A 10 'func ValidateName' # Search for volume name validation usage in crypto context rg -g '*.go' -B 5 -A 5 'util\.ValidateName.*volume'Length of output: 11211
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
csi/crypto/crypto.go (2)
20-20: Add documentation for the V2 volume suffix constantPlease add a comment explaining why V2 volumes require the "-encrypted" suffix and how it prevents naming conflicts with the default dm device.
120-122: Ensure consistent device path usage in logs and operationsThe logging message uses
devicePathbut the actual operation usesencryptVolumeName. Consider updating the log message to include both for better debugging:- logrus.Infof("Opening device %s with LUKS on %s", devicePath, encryptVolumeName) + logrus.Infof("Opening device %s with LUKS using mapper name %s", devicePath, encryptVolumeName)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
csi/crypto/crypto.go(5 hunks)csi/node_server.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- csi/node_server.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (4)
csi/crypto/crypto.go (4)
76-82: Well-documented V2 volume mapping implementation!The implementation correctly handles the V2 engine case with clear documentation explaining the naming strategy.
129-134: Clean and focused helper function implementation!The function follows the single responsibility principle and handles the V2 engine case correctly.
Line range hint
150-164: Solid implementation with proper error handling!The implementation correctly uses the encrypted volume name and includes comprehensive error checking.
Line range hint
76-164: Consider adding input validation for dataEngine parameterWhile the implementation is secure, consider adding validation for the
dataEngineparameter in public functions to ensure it's not empty and contains a valid engine type. This would prevent potential issues with undefined behavior.Example validation:
func VolumeMapper(volume, dataEngine string) string { + if dataEngine == "" { + logrus.Warn("Empty dataEngine parameter, defaulting to non-V2 behavior") + return path.Join(mapperFilePathPrefix, volume) + } + if !types.IsValidDataEngineType(longhorn.DataEngineType(dataEngine)) { + logrus.Warnf("Invalid dataEngine type %s, defaulting to non-V2 behavior", dataEngine) + return path.Join(mapperFilePathPrefix, volume) + } if types.IsDataEngineV2(longhorn.DataEngineType(dataEngine)) {Let's verify if the types package has the necessary validation function:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
csi/crypto/crypto.go (1)
Line range hint
108-122: Improve error logging clarity.The error message uses
devicePathwhile the operation usesencryptVolumeName, which could make debugging harder. Consider including both in the error message.- logrus.WithError(err).Warnf("Failed to open LUKS device %s", devicePath) + logrus.WithError(err).Warnf("Failed to open LUKS device %s (encrypted name: %s)", devicePath, encryptVolumeName)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
csi/crypto/crypto.go(5 hunks)csi/node_server.go(6 hunks)webhook/resources/volume/validator.go(0 hunks)
💤 Files with no reviewable changes (1)
- webhook/resources/volume/validator.go
🚧 Files skipped from review as they are similar to previous changes (1)
- csi/node_server.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (6)
csi/crypto/crypto.go (6)
11-15: LGTM! Clean import additions and well-defined constant.The new imports and constant are appropriately added to support V2 data engine functionality.
Also applies to: 20-20
76-82: LGTM! Well-documented V2 volume handling.The function correctly handles the V2 engine case with clear comments explaining the naming conflict resolution strategy.
129-134: LGTM! Well-designed helper function.Clean implementation that centralizes the V2 volume naming logic and promotes code reuse.
137-146: LGTM! Consistent implementation.The changes follow the same pattern as other functions and correctly handle V2 volumes.
Line range hint
150-164: LGTM! Successfully addressed previous review feedback.The implementation now correctly uses the encrypted volume name, resolving the previous review comment about potential V2 volume issues.
Line range hint
1-1: Consider adding unit tests for V2 volume handling.The changes look good, but it would be beneficial to add unit tests specifically covering the V2 volume name handling scenarios.
Would you like me to help generate unit tests for the V2 volume handling cases?
🧰 Tools
🪛 Gitleaks (8.21.2)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
csi/crypto/crypto.go (2)
20-20: Add documentation for the new constant.Please add a comment explaining the purpose of
mapperV2VolumeSuffixand why it's needed for V2 volumes.
Line range hint
108-127: Consider improving error handling for LUKS operations.While the function correctly handles V2 volumes, the error logging could be more descriptive:
- logrus.WithError(err).Warnf("Failed to open LUKS device %s", attachingVolumeHostPath) + logrus.WithError(err).Warnf("Failed to open LUKS device %s with name %s", attachingVolumeHostPath, encryptVolumeName)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controller/share_manager_controller.go(2 hunks)csi/crypto/crypto.go(5 hunks)csi/node_server.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- csi/node_server.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (4)
csi/crypto/crypto.go (3)
76-82: LGTM! Well-structured V2 volume handling.The function correctly handles the naming conflict between V2 volumes and their encrypted counterparts by appending the
-encryptedsuffix. The comment explains the rationale clearly.
130-135: LGTM! Clean helper function implementation.The
getEncryptVolumeNamehelper function is well-implemented and encapsulates the V2 volume name logic cleanly.
164-165: Verify error handling in ResizeEncryptoDevice.The function uses the correct encrypted volume name, but ensure that all error paths are properly handled in the upstream code.
Run this script to check for potential error handling issues:
✅ Verification successful
Error handling in ResizeEncryptoDevice is properly implemented
All error paths are properly handled and propagated:
- Device open check errors
- Namespace executor creation errors
- LUKS resize operation errors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check error handling patterns in resize operations rg -A 5 "ResizeEncryptoDevice.*error"Length of output: 435
Script:
#!/bin/bash # Get complete implementation of ResizeEncryptoDevice ast-grep --pattern 'func ResizeEncryptoDevice($$$) error { $$$ }' # Check LuksResize error handling patterns rg -B 2 -A 2 "LuksResize.*err.*"Length of output: 1764
controller/share_manager_controller.go (1)
1414-1420: LGTM! Clean integration of data engine parameter.The function signature update and argument passing are implemented correctly. The dataEngine parameter is properly propagated to the share manager daemon.
mantissahz
force-pushed
the
issue7355
branch
2 times, most recently
from
c16c92d to
c7a4727
Compare
|
@mergify backport v1.8.x |
✅ Backports have been created
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
csi/crypto/crypto.go (1)
76-82: Consider adding error handling for invalid dataEngineWhile the code handles V2 engine correctly, it might be worth adding validation for the dataEngine parameter to handle invalid values gracefully.
func VolumeMapper(volume, dataEngine string) string { + if dataEngine == "" { + logrus.Warnf("Empty dataEngine provided for volume %v, defaulting to non-V2 path", volume) + return path.Join(mapperFilePathPrefix, volume) + } if types.IsDataEngineV2(longhorn.DataEngineType(dataEngine)) { // v2 volume will use a dm device as default to control IO path when attaching. // This dm device will be created with the same name as the volume name.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
controller/share_manager_controller.go(2 hunks)csi/crypto/crypto.go(5 hunks)csi/node_server.go(6 hunks)webhook/resources/volume/validator.go(0 hunks)
💤 Files with no reviewable changes (1)
- webhook/resources/volume/validator.go
🚧 Files skipped from review as they are similar to previous changes (1)
- csi/node_server.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (8)
csi/crypto/crypto.go (6)
11-15: LGTM: Required imports added for V2 engine supportThe new imports are necessary for handling the data engine types and Longhorn API.
20-20: LGTM: Clear constant definitionThe
mapperV2VolumeSuffixconstant is well-defined and its purpose is clear from the context.
Line range hint
108-127: LGTM: Improved OpenVolume function with clear documentationThe function has been updated correctly to handle V2 volumes and includes clear documentation about the devicePath parameter.
130-135: LGTM: Well-structured helper functionThe
getEncryptVolumeNamehelper function is well-designed and encapsulates the V2 volume naming logic.
138-149: LGTM: CloseVolume function updated consistentlyThe function has been updated to use the new helper function for encrypted volume names.
Line range hint
151-166: LGTM: ResizeEncryptoDevice function updated consistentlyThe function has been updated to handle V2 volumes correctly.
controller/share_manager_controller.go (2)
1414-1419: LGTM: Clear function signature updateThe function signature has been updated to include the dataEngine parameter while maintaining good organization of parameters.
1420-1420: LGTM: Data engine argument added correctlyThe data engine information is properly passed to the share manager daemon.
ref: longhorn/longhorn 7355 Signed-off-by: James Lu <james.lu@suse.com>
when opening and closing a luks2 format device. ref: longhorn/longhorn 7355 Signed-off-by: James Lu <james.lu@suse.com>
ref: longhorn/longhorn 7355 Signed-off-by: James Lu <james.lu@suse.com>
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