Merge pull request #3355 from landreasyan/validate-bd-size-before-expand

fix: validating Block Device Size Before Resizing FileSystem

Author: k8s-ci-robot

pkg/azuredisk/azure_controller_common_test.go

@@ -1276,17 +1276,17 @@ func TestConcurrentDetachDisk(t *testing.T) {
 	// Simulate concurrent detach requests that would be batched
 	wg := &sync.WaitGroup{}
 	errorChan := make(chan error, 1)
-	for i := 0; i < 2; i++ {
+	for i := range 2 {
 		diskURI := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/disks/%s",
 			testCloud.SubscriptionID, testCloud.ResourceGroup, fmt.Sprintf("disk-batched-%d", i))
 		wg.Add(1)
-		go func() {
+		go func(i int, diskURI string) {
 			defer wg.Done()
 			err := common.DetachDisk(ctx, fmt.Sprintf("disk-batched-%d", i), diskURI, "vm1")
 			if err != nil {
 				errorChan <- err
 			}
-		}()
+		}(i, diskURI)
 	}
 
 	time.Sleep(1005 * time.Millisecond) // Wait for the batching timeout

pkg/azuredisk/fake_azuredisk.go

@@ -103,6 +103,7 @@ type FakeDriver interface {
 	setThrottlingCache(key string, value string)
 	getUsedLunsFromVolumeAttachments(context.Context, string) ([]int, error)
 	getUsedLunsFromNode(context.Context, types.NodeName) ([]int, error)
+	validateBlockDeviceSize(devicePath string, requestGiB int64) (int64, error)
 }
 
 type fakeDriver struct {

pkg/azuredisk/nodeserver.go

@@ -538,34 +538,43 @@ func (d *Driver) NodeExpandVolume(_ context.Context, req *csi.NodeExpandVolumeRe
 		}
 	}
 
+	// CRITICAL SAFETY CHECK: Verify block device size BEFORE filesystem resize
+	// This prevents filesystem corruption if the underlying disk hasn't been expanded yet
+	actualDevicePath := devicePath
+	if runtime.GOOS == "windows" && d.enableWindowsHostProcess {
+		// in windows host process mode, this driver could get the volume size from the volume path
+		actualDevicePath = volumePath
+	}
+
+	currentBlockSizeBytes, err := d.validateBlockDeviceSize(actualDevicePath, requestGiB)
+	if err != nil {
+		return nil, status.Errorf(codes.FailedPrecondition, "NodeExpandVolume: block device size did not match requested size: %v", err)
+	}
+
+	klog.V(2).Infof("NodeExpandVolume: block device size verified (%d bytes >= %d GiB requested), proceeding with filesystem resize",
+		currentBlockSizeBytes, requestGiB)
+
+	// Now safe to resize filesystem since we've verified the block device is large enough
 	var retErr error
 	if err := resizeVolume(devicePath, volumePath, d.mounter); err != nil {
 		retErr = status.Errorf(codes.Internal, "could not resize volume %q (%q):  %v", volumeID, devicePath, err)
 		klog.Errorf("%v, will continue checking whether the volume has been resized", retErr)
 	}
 
-	if runtime.GOOS == "windows" && d.enableWindowsHostProcess {
-		// in windows host process mode, this driver could get the volume size from the volume path
-		devicePath = volumePath
-	}
-	gotBlockSizeBytes, err := getBlockSizeBytes(devicePath, d.mounter)
+	// Final verification after filesystem resize
+	finalBlockSizeBytes, err := d.validateBlockDeviceSize(actualDevicePath, requestGiB)
 	if err != nil {
-		return nil, status.Error(codes.Internal, fmt.Sprintf("could not get size of block volume at path %s: %v", devicePath, err))
-	}
-	gotBlockGiB := volumehelper.RoundUpGiB(gotBlockSizeBytes)
-	if gotBlockGiB < requestGiB {
 		if retErr != nil {
 			return nil, retErr
 		}
-		// Because size was rounded up, getting more size than requested will be a success.
-		return nil, status.Errorf(codes.Internal, "resize requested for %v, but after resizing volume size was %v", requestGiB, gotBlockGiB)
+		return nil, status.Errorf(codes.Internal, "NodeExpandVolume: block device size did not match requested size after filesystem resize: %v", err)
 	}
 
-	klog.V(2).Infof("NodeExpandVolume succeeded on resizing volume %v to %v", volumeID, gotBlockSizeBytes)
+	klog.V(2).Infof("NodeExpandVolume succeeded on resizing volume %v to %v", volumeID, finalBlockSizeBytes)
 
 	isOperationSucceeded = true
 	return &csi.NodeExpandVolumeResponse{
-		CapacityBytes: gotBlockSizeBytes,
+		CapacityBytes: finalBlockSizeBytes,
 	}, nil
 }
 
@@ -682,6 +691,23 @@ func (d *Driver) ensureBlockTargetFile(target string) error {
 	return nil
 }
 
+// validateBlockDeviceSize checks if the block device is large enough for the requested size
+// Returns the actual block size in bytes if validation passes, otherwise returns an error
+func (d *Driver) validateBlockDeviceSize(devicePath string, requestGiB int64) (int64, error) {
+	blockSizeBytes, err := getBlockSizeBytes(devicePath, d.mounter)
+	if err != nil {
+		return 0, status.Error(codes.Internal, fmt.Sprintf("could not get size of block volume at path %s: %v", devicePath, err))
+	}
+
+	blockGiB := volumehelper.RoundUpGiB(blockSizeBytes)
+
+	if blockGiB < requestGiB {
+		return 0, status.Error(codes.Internal, fmt.Sprintf("block volume at path %s size check failed: current %d GiB < requested %d GiB", devicePath, blockGiB, requestGiB))
+	}
+
+	return blockSizeBytes, nil
+}
+
 func collectMountOptions(fsType string, mntFlags []string) []string {
 	var options []string
 	options = append(options, mntFlags...)

pkg/azuredisk/nodeserver_test.go

@@ -1058,15 +1058,15 @@ func TestNodeExpandVolume(t *testing.T) {
 		WindowsError: status.Errorf(codes.NotFound, "error reading link for mount D:\\a\\azuredisk-csi-driver\\azuredisk-csi-driver\\pkg\\azuredisk\\target_test. target  err: readlink D:\\a\\azuredisk-csi-driver\\azuredisk-csi-driver\\pkg\\azuredisk\\target_test: The file or directory is not a reparse point."),
 	}
 	blockSizeErr := testutil.TestError{
-		DefaultError: status.Error(codes.Internal, "could not get size of block volume at path test: error when getting size of block volume at path test: output: , err: exit status 1"),
+		DefaultError: status.Error(codes.FailedPrecondition, "NodeExpandVolume: block device size did not match requested size: rpc error: code = Internal desc = block volume at path test size check failed: current 0 GiB < requested 15 GiB"),
 		WindowsError: status.Errorf(codes.NotFound, "error reading link for mount D:\\a\\azuredisk-csi-driver\\azuredisk-csi-driver\\pkg\\azuredisk\\target_test. target  err: readlink D:\\a\\azuredisk-csi-driver\\azuredisk-csi-driver\\pkg\\azuredisk\\target_test: The file or directory is not a reparse point."),
 	}
 	resizeErr := testutil.TestError{
 		DefaultError: status.Errorf(codes.Internal, "could not resize volume \"test\" (\"test\"):  resize of device test failed: %v. resize2fs output: ", notFoundErr),
 		WindowsError: status.Errorf(codes.NotFound, "error reading link for mount D:\\a\\azuredisk-csi-driver\\azuredisk-csi-driver\\pkg\\azuredisk\\target_test. target  err: readlink D:\\a\\azuredisk-csi-driver\\azuredisk-csi-driver\\pkg\\azuredisk\\target_test: The file or directory is not a reparse point."),
 	}
 	sizeTooSmallErr := testutil.TestError{
-		DefaultError: status.Errorf(codes.Internal, "resize requested for %v, but after resizing volume size was %v", volumehelper.RoundUpGiB(stdCapacityRange.RequiredBytes), volumehelper.RoundUpGiB(stdCapacityRange.RequiredBytes/2)),
+		DefaultError: status.Error(codes.Internal, "NodeExpandVolume: block device size did not match requested size after filesystem resize: rpc error: code = Internal desc = block volume at path test size check failed: current 8 GiB < requested 15 GiB"),
 		WindowsError: status.Errorf(codes.NotFound, "error reading link for mount D:\\a\\azuredisk-csi-driver\\azuredisk-csi-driver\\pkg\\azuredisk\\target_test. target  err: readlink D:\\a\\azuredisk-csi-driver\\azuredisk-csi-driver\\pkg\\azuredisk\\target_test: The file or directory is not a reparse point."),
 	}
 
@@ -1091,6 +1091,10 @@ func TestNodeExpandVolume(t *testing.T) {
 	blockdevAction := func() ([]byte, []byte, error) {
 		return []byte(fmt.Sprintf("%d", stdCapacityRange.RequiredBytes)), []byte{}, nil
 	}
+	// Action that returns 0 bytes (invalid block size)
+	blockdevZeroAction := func() ([]byte, []byte, error) {
+		return []byte("0"), []byte{}, nil
+	}
 
 	tests := []struct {
 		desc          string
@@ -1148,7 +1152,7 @@ func TestNodeExpandVolume(t *testing.T) {
 			},
 			expectedErr:   blockSizeErr,
 			skipOnDarwin:  true, // ResizeFs not supported on Darwin
-			outputScripts: []testingexec.FakeAction{findmntAction, blkidAction, resize2fsAction, notFoundErrAction},
+			outputScripts: []testingexec.FakeAction{findmntAction, blockdevZeroAction},
 		},
 		{
 			desc: "Resize failure",
@@ -1158,9 +1162,10 @@ func TestNodeExpandVolume(t *testing.T) {
 				VolumeId:          "test",
 				StagingTargetPath: "test",
 			},
-			expectedErr:   resizeErr,
-			skipOnDarwin:  true, // ResizeFs not supported on Darwin
-			outputScripts: []testingexec.FakeAction{findmntAction, blkidAction, resize2fsFailedAction, blockdevSizeTooSmallAction},
+			expectedErr:  resizeErr,
+			skipOnDarwin: true, // ResizeFs not supported on Darwin
+			// First blockdev call succeeds (pre-resize validation), blkid checks filesystem, resize2fs fails, second blockdev for post-resize validation
+			outputScripts: []testingexec.FakeAction{findmntAction, blockdevAction, blkidAction, resize2fsFailedAction, blockdevSizeTooSmallAction},
 		},
 		{
 			desc: "Resize too small failure",
@@ -1170,9 +1175,10 @@ func TestNodeExpandVolume(t *testing.T) {
 				VolumeId:          "test",
 				StagingTargetPath: "test",
 			},
-			expectedErr:   sizeTooSmallErr,
-			skipOnDarwin:  true, // ResizeFs not supported on Darwin
-			outputScripts: []testingexec.FakeAction{findmntAction, blkidAction, resize2fsAction, blockdevSizeTooSmallAction},
+			expectedErr:  sizeTooSmallErr,
+			skipOnDarwin: true, // ResizeFs not supported on Darwin
+			// First blockdev call succeeds (pre-resize validation), resize2fs succeeds, second blockdev shows size too small
+			outputScripts: []testingexec.FakeAction{findmntAction, blockdevAction, blkidAction, resize2fsAction, blockdevSizeTooSmallAction},
 		},
 		{
 			desc: "Successfully expanded",
@@ -1184,7 +1190,8 @@ func TestNodeExpandVolume(t *testing.T) {
 			},
 			skipOnWindows: true,
 			skipOnDarwin:  true, // ResizeFs not supported on Darwin
-			outputScripts: []testingexec.FakeAction{findmntAction, blkidAction, resize2fsAction, blockdevAction},
+			// First blockdev call (pre-resize validation), resize2fs succeeds, second blockdev call (post-resize validation)
+			outputScripts: []testingexec.FakeAction{findmntAction, blockdevAction, blkidAction, resize2fsAction, blockdevAction},
 		},
 		{
 			desc: "Block volume expansion",
@@ -1475,3 +1482,89 @@ func TestNodePublishVolumeIdempotentMount(t *testing.T) {
 	err = os.RemoveAll(targetTest)
 	assert.NoError(t, err)
 }
+
+func TestValidateBlockDeviceSize(t *testing.T) {
+	cntl := gomock.NewController(t)
+	defer cntl.Finish()
+
+	d, _ := NewFakeDriver(cntl)
+	fakeMounter, err := mounter.NewFakeSafeMounter()
+	assert.NoError(t, err)
+	d.setMounter(fakeMounter)
+
+	notFoundErr := errors.New("exit status 1")
+	blockdevAction := func() ([]byte, []byte, error) {
+		return []byte(fmt.Sprintf("%d", volumehelper.GiBToBytes(20))), []byte{}, nil
+	}
+	blockdevSmallAction := func() ([]byte, []byte, error) {
+		return []byte(fmt.Sprintf("%d", volumehelper.GiBToBytes(5))), []byte{}, nil
+	}
+	blockdevErrorAction := func() ([]byte, []byte, error) {
+		return []byte{}, []byte{}, notFoundErr
+	}
+
+	tests := []struct {
+		desc          string
+		devicePath    string
+		requestGiB    int64
+		outputScript  testingexec.FakeAction
+		expectedErr   error
+		expectedBytes int64
+		skipOnWindows bool
+	}{
+		{
+			desc:          "Error getting block size",
+			devicePath:    "/dev/sda",
+			requestGiB:    10,
+			outputScript:  blockdevErrorAction,
+			expectedErr:   status.Error(codes.Internal, fmt.Sprintf("could not get size of block volume at path %s: %v", "/dev/sda", fmt.Errorf("error when getting size of block volume at path /dev/sda: output: , err: exit status 1"))),
+			skipOnWindows: true,
+		},
+		{
+			desc:          "Block size too small",
+			devicePath:    "/dev/sdb",
+			requestGiB:    10,
+			outputScript:  blockdevSmallAction,
+			expectedErr:   status.Error(codes.Internal, fmt.Sprintf("block volume at path %s size check failed: current %d GiB < requested %d GiB", "/dev/sdb", 5, 10)),
+			skipOnWindows: true,
+		},
+		{
+			desc:          "Successful validation",
+			devicePath:    "/dev/sdc",
+			requestGiB:    10,
+			outputScript:  blockdevAction,
+			expectedErr:   nil,
+			expectedBytes: volumehelper.GiBToBytes(20),
+			skipOnWindows: true,
+		},
+		{
+			desc:          "Exact size match",
+			devicePath:    "/dev/sdd",
+			requestGiB:    20,
+			outputScript:  blockdevAction,
+			expectedErr:   nil,
+			expectedBytes: volumehelper.GiBToBytes(20),
+			skipOnWindows: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			if test.skipOnWindows && runtime.GOOS == "windows" {
+				t.Skip("Skipping test on Windows")
+			}
+
+			d.setNextCommandOutputScripts(test.outputScript)
+
+			actualBytes, err := d.validateBlockDeviceSize(test.devicePath, test.requestGiB)
+
+			if test.expectedErr == nil {
+				assert.NoError(t, err)
+				assert.Equal(t, test.expectedBytes, actualBytes)
+			} else {
+				assert.EqualError(t, err, test.expectedErr.Error())
+				assert.Equal(t, int64(0), actualBytes)
+			}
+		})
+	}
+}