Simplifies various code paths for iSCSI and LUKS

NetApp · Sep 11, 2024 · 09b9dd3 · 09b9dd3
1 parent a871573
commit 09b9dd3
Show file tree

Hide file tree

Showing 5 changed files with 181 additions and 213 deletions.
diff --git a/frontend/csi/node_server.go b/frontend/csi/node_server.go
@@ -500,12 +500,7 @@ func (p *Plugin) nodeExpandVolume(
 		return err
 	}
 
-	isLUKS, err := strconv.ParseBool(publishInfo.LUKSEncryption)
-	if err != nil {
-		Logc(ctx).WithField("volumeId", volumeId).Warnf("Could not parse LUKSEncryption into a bool, got %v.",
-			publishInfo.LUKSEncryption)
-	}
-	if isLUKS {
+	if utils.ParseBool(publishInfo.LUKSEncryption) {
 		Logc(ctx).WithField("volumeId", volumeId).Info("Resizing the LUKS mapping.")
 		// Refresh the luks device
 		// cryptsetup resize <luks-device-path> << <passphrase>
@@ -1127,13 +1122,7 @@ func (p *Plugin) nodeStageISCSIVolume(
 		return err
 	}
 
-	var isLUKS bool
-	if req.PublishContext["LUKSEncryption"] != "" {
-		isLUKS, err = strconv.ParseBool(req.PublishContext["LUKSEncryption"])
-		if err != nil {
-			return fmt.Errorf("could not parse LUKSEncryption into a bool, got %v", publishInfo.LUKSEncryption)
-		}
-	}
+	isLUKS := utils.ParseBool(req.PublishContext["LUKSEncryption"])
 	publishInfo.LUKSEncryption = strconv.FormatBool(isLUKS)
 
 	err = unstashIscsiTargetPortals(publishInfo, req.PublishContext)
@@ -1309,42 +1298,31 @@ func (p *Plugin) nodeUnstageISCSIVolume(
 	// Remove Portal/LUN entries in self-healing map.
 	utils.RemoveLUNFromSessions(ctx, publishInfo, &publishedISCSISessions)
 
-	if publishInfo.LUKSEncryption != "" {
-		isLUKS, err := strconv.ParseBool(publishInfo.LUKSEncryption)
+	if utils.ParseBool(publishInfo.LUKSEncryption) {
+		fields := LogFields{"luksDevicePath": publishInfo.DevicePath, "lunID": publishInfo.IscsiLunNumber}
+
+		// Before closing the LUKS device, get the underlying mapper device from cryptsetup.
+		mapperPath, err := utils.GetUnderlyingDevicePathForLUKSDevice(ctx, publishInfo.DevicePath)
 		if err != nil {
-			return fmt.Errorf("could not parse LUKSEncryption into a bool, got %v", publishInfo.LUKSEncryption)
+			// No need to return an error
+			Logc(ctx).WithFields(fields).WithError(err).Error("Could not determine underlying device for LUKS.")
 		}
-		if isLUKS {
-			// Before closing the device, get the corresponding DM device.
-			publishedLUKsDevice, err := utils.GetUnderlyingDevicePathForLUKSDevice(ctx, publishInfo.DevicePath)
-			if err != nil {
-				// No need to return an error
-				Logc(ctx).WithFields(LogFields{
-					"devicePath": publishInfo.DevicePath,
-					"lun":        publishInfo.IscsiLunNumber,
-					"err":        err,
-				}).Error("Failed to verify the multipath device, could not determine" +
-					" underlying device for LUKS mapping.")
-			}
-
-			err = utils.EnsureLUKSDeviceClosed(ctx, publishInfo.DevicePath)
-			if err != nil {
-				return err
-			}
+		fields["mapperPath"] = mapperPath
 
-			publishedLUKsDMDevice := ""
-			// Get the DM device for the LUKS device
-			if publishedLUKsDMDevice, err = utils.GetDMDeviceForMapperPath(ctx, publishedLUKsDevice); err != nil {
-				Logc(ctx).WithFields(LogFields{
-					"devicePath": publishedLUKsDevice,
-					"err":        err,
-				}).Warning("Failed to get the DM device for mapper device.")
-			}
+		if err = utils.EnsureLUKSDeviceClosed(ctx, publishInfo.DevicePath); err != nil {
+			Logc(ctx).WithFields(fields).WithError(err).Error("Failed to close LUKS device.")
+			return err
+		}
 
-			// For the future steps LUKs device path is not really useful, either it should be
-			// DM device or empty.
-			publishInfo.DevicePath = publishedLUKsDMDevice
+		// Get the underlying device mapper device for the block device used by LUKS.
+		dmDevicePath, err := utils.GetDMDeviceForMapperPath(ctx, mapperPath)
+		if err != nil {
+			Logc(ctx).WithFields(fields).WithError(err).Error("Failed to determine dm device from device mapper.")
 		}
+
+		// At this point, the LUKs device path is no longer useful.
+		// However, the device mapper device path is (/dev/dm-#).
+		publishInfo.DevicePath = dmDevicePath
 	}
 
 	// Delete the device from the host.
@@ -1397,6 +1375,7 @@ func (p *Plugin) nodeUnstageISCSIVolume(
 		if err := utils.ISCSILogout(ctx, publishInfo.IscsiTargetIQN, publishInfo.IscsiTargetPortal); err != nil {
 			Logc(ctx).Error(err)
 		}
+
 		for _, portal := range publishInfo.IscsiPortals {
 			if err := utils.ISCSILogout(ctx, publishInfo.IscsiTargetIQN, portal); err != nil {
 				Logc(ctx).Error(err)
@@ -1479,24 +1458,20 @@ func (p *Plugin) nodePublishISCSIVolume(
 	if req.GetReadonly() {
 		publishInfo.MountOptions = utils.AppendToStringList(publishInfo.MountOptions, "ro", ",")
 	}
-	if publishInfo.LUKSEncryption != "" {
-		isLUKS, err := strconv.ParseBool(publishInfo.LUKSEncryption)
+
+	if utils.ParseBool(publishInfo.LUKSEncryption) {
+		// Rotate the LUKS passphrase if needed, on failure, log and continue to publish
+		luksDevice, err := utils.NewLUKSDeviceFromMappingPath(ctx, publishInfo.DevicePath,
+			req.VolumeContext["internalName"])
 		if err != nil {
-			return nil, fmt.Errorf("could not parse LUKSEncryption into a bool, got %v", publishInfo.LUKSEncryption)
+			return nil, status.Error(codes.Internal, err.Error())
 		}
-		if isLUKS {
-			// Rotate the LUKS passphrase if needed, on failure, log and continue to publish
-			luksDevice, err := utils.NewLUKSDeviceFromMappingPath(ctx, publishInfo.DevicePath,
-				req.VolumeContext["internalName"])
-			if err != nil {
-				return nil, status.Error(codes.Internal, err.Error())
-			}
-			err = ensureLUKSVolumePassphrase(ctx, p.restClient, luksDevice, req.GetVolumeId(), req.GetSecrets(), false)
-			if err != nil {
-				Logc(ctx).WithError(err).Error("Failed to ensure current LUKS passphrase.")
-			}
+		err = ensureLUKSVolumePassphrase(ctx, p.restClient, luksDevice, req.GetVolumeId(), req.GetSecrets(), false)
+		if err != nil {
+			Logc(ctx).WithError(err).Error("Failed to ensure current LUKS passphrase.")
 		}
 	}
+
 	isRawBlock := publishInfo.FilesystemType == tridentconfig.FsRaw
 	if isRawBlock {
 
@@ -2308,16 +2283,7 @@ func (p *Plugin) nodeStageNVMeVolume(
 	ctx context.Context, req *csi.NodeStageVolumeRequest,
 	publishInfo *models.VolumePublishInfo,
 ) error {
-	var isLUKS bool
-	var err error
-
-	if req.PublishContext["LUKSEncryption"] != "" {
-		isLUKS, err = strconv.ParseBool(req.PublishContext["LUKSEncryption"])
-		if err != nil {
-			return fmt.Errorf("could not parse LUKSEncryption into a bool, got %v", publishInfo.LUKSEncryption)
-		}
-	}
-
+	isLUKS := utils.ParseBool(publishInfo.LUKSEncryption)
 	publishInfo.LUKSEncryption = strconv.FormatBool(isLUKS)
 	publishInfo.MountOptions = req.PublishContext["mountOptions"]
 	publishInfo.NVMeSubsystemNQN = req.PublishContext["nvmeSubsystemNqn"]
@@ -2483,24 +2449,20 @@ func (p *Plugin) nodePublishNVMeVolume(
 	if req.GetReadonly() {
 		publishInfo.MountOptions = utils.AppendToStringList(publishInfo.MountOptions, "ro", ",")
 	}
-	if publishInfo.LUKSEncryption != "" {
-		isLUKS, err := strconv.ParseBool(publishInfo.LUKSEncryption)
+
+	if utils.ParseBool(publishInfo.LUKSEncryption) {
+		// Rotate the LUKS passphrase if needed, on failure, log and continue to publish
+		luksDevice, err := utils.NewLUKSDeviceFromMappingPath(ctx, publishInfo.DevicePath,
+			req.VolumeContext["internalName"])
 		if err != nil {
-			return nil, fmt.Errorf("could not parse LUKSEncryption into a bool, got %v", publishInfo.LUKSEncryption)
+			return nil, status.Error(codes.Internal, err.Error())
 		}
-		if isLUKS {
-			// Rotate the LUKS passphrase if needed, on failure, log and continue to publish
-			luksDevice, err := utils.NewLUKSDeviceFromMappingPath(ctx, publishInfo.DevicePath,
-				req.VolumeContext["internalName"])
-			if err != nil {
-				return nil, status.Error(codes.Internal, err.Error())
-			}
-			err = ensureLUKSVolumePassphrase(ctx, p.restClient, luksDevice, req.GetVolumeId(), req.GetSecrets(), false)
-			if err != nil {
-				Logc(ctx).WithError(err).Error("Failed to ensure current LUKS passphrase.")
-			}
+		err = ensureLUKSVolumePassphrase(ctx, p.restClient, luksDevice, req.GetVolumeId(), req.GetSecrets(), false)
+		if err != nil {
+			Logc(ctx).WithError(err).Error("Failed to ensure current LUKS passphrase.")
 		}
 	}
+
 	isRawBlock := publishInfo.FilesystemType == tridentconfig.FsRaw
 	if isRawBlock {
 

diff --git a/utils/devices.go b/utils/devices.go
@@ -872,8 +872,8 @@ func findDevicesForMultipathDevice(ctx context.Context, device string) []string
 }
 
 // compareWithPublishedDevicePath verifies that published path matches the discovered device path
-func compareWithPublishedDevicePath(ctx context.Context, publishInfo *models.VolumePublishInfo,
-	deviceInfo *ScsiDeviceInfo,
+func compareWithPublishedDevicePath(
+	ctx context.Context, publishInfo *models.VolumePublishInfo, deviceInfo *ScsiDeviceInfo,
 ) (bool, error) {
 	isProbablyGhostDevice := false
 	discoverMpath := strings.TrimPrefix(deviceInfo.MultipathDevice, devPrefix)
@@ -926,8 +926,8 @@ func compareWithPublishedDevicePath(ctx context.Context, publishInfo *models.Vol
 }
 
 // compareWithPublishedSerialNumber verifies that device serial number matches the discovered LUNs
-func compareWithPublishedSerialNumber(ctx context.Context, publishInfo *models.VolumePublishInfo,
-	deviceInfo *ScsiDeviceInfo,
+func compareWithPublishedSerialNumber(
+	ctx context.Context, publishInfo *models.VolumePublishInfo, deviceInfo *ScsiDeviceInfo,
 ) (bool, error) {
 	isProbablyGhostDevice := false
 	lunSerialCheckPassed := false
@@ -999,7 +999,8 @@ func compareWithPublishedSerialNumber(ctx context.Context, publishInfo *models.V
 // compareWithAllPublishInfos comparing all publications (allPublishInfos) for
 // LUN number uniqueness, if more than one publication exists with the same LUN number
 // then it indicates a larger problem that user needs to manually fix
-func compareWithAllPublishInfos(ctx context.Context, publishInfo *models.VolumePublishInfo,
+func compareWithAllPublishInfos(
+	ctx context.Context, publishInfo *models.VolumePublishInfo,
 	allPublishInfos []models.VolumePublishInfo, deviceInfo *ScsiDeviceInfo,
 ) error {
 	// During unstaging at least 1 publish info should exist else
@@ -1046,8 +1047,9 @@ func compareWithAllPublishInfos(ctx context.Context, publishInfo *models.VolumeP
 // verifyMultipathDevice verifies that device being removed is correct based on published device path,
 // device serial number (if present) or comparing all publications (allPublishInfos) for
 // LUN number uniqueness.
-func verifyMultipathDevice(ctx context.Context, publishInfo *models.VolumePublishInfo, allPublishInfos []models.VolumePublishInfo,
-	deviceInfo *ScsiDeviceInfo,
+func verifyMultipathDevice(
+	ctx context.Context, publishInfo *models.VolumePublishInfo,
+	allPublishInfos []models.VolumePublishInfo, deviceInfo *ScsiDeviceInfo,
 ) (bool, error) {
 	// Ensure a correct multipath device is being discovered.
 	// Following steps can be performed:
@@ -1072,8 +1074,9 @@ func verifyMultipathDevice(ctx context.Context, publishInfo *models.VolumePublis
 // also verifies that device being removed is correct based on published device path,
 // device serial number (if present) or comparing all publications (allPublishInfos) for
 // LUN number uniqueness.
-func PrepareDeviceForRemoval(ctx context.Context, publishInfo *models.VolumePublishInfo, allPublishInfos []models.VolumePublishInfo, ignoreErrors,
-	force bool,
+func PrepareDeviceForRemoval(
+	ctx context.Context, publishInfo *models.VolumePublishInfo,
+	allPublishInfos []models.VolumePublishInfo, ignoreErrors, force bool,
 ) (string, error) {
 	GenerateRequestContextForLayer(ctx, LogLayerUtils)
 
@@ -1088,35 +1091,37 @@ func PrepareDeviceForRemoval(ctx context.Context, publishInfo *models.VolumePubl
 	Logc(ctx).WithFields(fields).Debug(">>>> devices.PrepareDeviceForRemoval")
 	defer Logc(ctx).WithFields(fields).Debug("<<<< devices.PrepareDeviceForRemoval")
 
-	var multipathDevice string
-	var performDeferredDeviceRemoval bool
-
 	deviceInfo, err := getDeviceInfoForLUN(ctx, lunID, iSCSINodeName, false, true)
 	if err != nil {
-		Logc(ctx).WithFields(LogFields{
-			"error": err,
-			"lunID": lunID,
-		}).Warn("Could not get device info for removal, skipping host removal steps.")
-		return multipathDevice, err
+		Logc(ctx).WithField(
+			"lunID", lunID,
+		).WithError(err).Warn("Could not get device info for removal, skipping host removal steps.")
+		return "", err
 	}
 
 	if deviceInfo == nil {
-		Logc(ctx).WithFields(LogFields{
-			"lunID": lunID,
-		}).Debug("No device found for removal, skipping host removal steps.")
-		return multipathDevice, nil
+		Logc(ctx).WithField(
+			"lunID", lunID,
+		).Debug("No device found for removal, skipping host removal steps.")
+		return "", nil
 	}
 
-	if publishInfo.IscsiTargetPortal != "" /* CSI Case */ {
+	// CSI Case
+	if publishInfo.IscsiTargetPortal != "" {
 		_, err = verifyMultipathDevice(ctx, publishInfo, allPublishInfos, deviceInfo)
 		if err != nil {
-			return multipathDevice, err
+			return "", err
 		}
 	}
 
-	performDeferredDeviceRemoval, err = removeSCSIDevice(ctx, deviceInfo, ignoreErrors, force)
+	var multipathDevice string
+	performDeferredDeviceRemoval, err := removeSCSIDevice(ctx, deviceInfo, ignoreErrors, force)
 	if performDeferredDeviceRemoval && deviceInfo.MultipathDevice != "" {
 		multipathDevice = devPrefix + deviceInfo.MultipathDevice
+		Logc(ctx).WithFields(LogFields{
+			"lunID":           lunID,
+			"multipathDevice": multipathDevice,
+		}).Debug("Discovered unmapped multipath device when removing SCSI device.")
 	}
 
 	return multipathDevice, err
@@ -1502,7 +1507,9 @@ func GetLUKSPassphrasesFromSecretMap(secrets map[string]string) (string, string,
 }
 
 // EnsureLUKSDeviceMappedOnHost ensures the specified device is LUKS formatted, opened, and has the current passphrase.
-func EnsureLUKSDeviceMappedOnHost(ctx context.Context, luksDevice LUKSDeviceInterface, name string, secrets map[string]string) (bool, error) {
+func EnsureLUKSDeviceMappedOnHost(
+	ctx context.Context, luksDevice LUKSDeviceInterface, name string, secrets map[string]string,
+) (bool, error) {
 	// Try to Open with current luks passphrase
 	luksPassphraseName, luksPassphrase, previousLUKSPassphraseName, previousLUKSPassphrase := GetLUKSPassphrasesFromSecretMap(secrets)
 	if luksPassphrase == "" {