Merge pull request kubernetes#24989 from gnufied/detect-in64-overflow

Bug 1592653: detect int64 overflow when converting volume sizes Origin-commit: 2ca1c4f2d59ea4fb171c10923b2f24d910be79f5
openshift · May 18, 2020 · 8162985 · 8162985
2 parents e942573 + 03aa008
commit 8162985
Show file tree

Hide file tree

Showing 14 changed files with 318 additions and 135 deletions.
diff --git a/pkg/volume/azure_file/azure_file.go b/pkg/volume/azure_file/azure_file.go
@@ -179,8 +179,13 @@ func (plugin *azureFilePlugin) ExpandVolumeDevice(
 	if err != nil {
 		return oldSize, err
 	}
+	requestGiB, err := volumehelpers.RoundUpToGiBInt(newSize)
 
-	if err := azure.ResizeFileShare(accountName, accountKey, shareName, int(volumehelpers.RoundUpToGiB(newSize))); err != nil {
+	if err != nil {
+		return oldSize, err
+	}
+
+	if err := azure.ResizeFileShare(accountName, accountKey, shareName, requestGiB); err != nil {
 		return oldSize, err
 	}
 

diff --git a/pkg/volume/azure_file/azure_provision.go b/pkg/volume/azure_file/azure_provision.go
@@ -147,7 +147,11 @@ func (a *azureFileProvisioner) Provision(selectedNode *v1.Node, allowedTopologie
 	var sku, resourceGroup, location, account, shareName string
 
 	capacity := a.options.PVC.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)]
-	requestGiB := int(volumehelpers.RoundUpToGiB(capacity))
+	requestGiB, err := volumehelpers.RoundUpToGiBInt(capacity)
+	if err != nil {
+		return nil, err
+	}
+
 	secretNamespace := a.options.PVC.Namespace
 	// Apply ProvisionerParameters (case-insensitive). We leave validation of
 	// the values to the cloud provider.

diff --git a/pkg/volume/gcepd/gce_util.go b/pkg/volume/gcepd/gce_util.go
@@ -101,7 +101,10 @@ func (util *GCEDiskUtil) CreateVolume(c *gcePersistentDiskProvisioner, node *v1.
 	name := volumeutil.GenerateVolumeName(c.options.ClusterName, c.options.PVName, 63) // GCE PD name can have up to 63 characters
 	capacity := c.options.PVC.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)]
 	// GCE PDs are allocated in chunks of GiBs
-	requestGB := volumehelpers.RoundUpToGiB(capacity)
+	requestGB, err := volumehelpers.RoundUpToGiB(capacity)
+	if err != nil {
+		return "", 0, nil, "", err
+	}
 
 	// Apply Parameters.
 	// Values for parameter "replication-type" are canonicalized to lower case.
@@ -159,7 +162,7 @@ func (util *GCEDiskUtil) CreateVolume(c *gcePersistentDiskProvisioner, node *v1.
 			name,
 			diskType,
 			selectedZones,
-			int64(requestGB),
+			requestGB,
 			*c.options.CloudTags)
 		if err != nil {
 			klog.V(2).Infof("Error creating regional GCE PD volume: %v", err)
@@ -176,7 +179,7 @@ func (util *GCEDiskUtil) CreateVolume(c *gcePersistentDiskProvisioner, node *v1.
 			name,
 			diskType,
 			selectedZone,
-			int64(requestGB),
+			requestGB,
 			*c.options.CloudTags)
 		if err != nil {
 			klog.V(2).Infof("Error creating single-zone GCE PD volume: %v", err)

diff --git a/pkg/volume/glusterfs/glusterfs.go b/pkg/volume/glusterfs/glusterfs.go
@@ -1212,11 +1212,18 @@ func (plugin *glusterfsPlugin) ExpandVolumeDevice(spec *volume.Spec, newSize res
 	}
 
 	// Find out delta size
-	expansionSize := resource.NewScaledQuantity((newSize.Value() - oldSize.Value()), 0)
-	expansionSizeGiB := int(volumehelpers.RoundUpToGiB(*expansionSize))
+	expansionSize := newSize
+	expansionSize.Sub(oldSize)
+	expansionSizeGiB, err := volumehelpers.RoundUpToGiBInt(expansionSize)
+	if err != nil {
+		return oldSize, err
+	}
 
 	// Find out requested Size
-	requestGiB := volumehelpers.RoundUpToGiB(newSize)
+	requestGiB, err := volumehelpers.RoundUpToGiB(newSize)
+	if err != nil {
+		return oldSize, err
+	}
 
 	//Check the existing volume size
 	currentVolumeInfo, err := cli.VolumeInfo(volumeID)

diff --git a/pkg/volume/portworx/portworx_util.go b/pkg/volume/portworx/portworx_util.go
@@ -25,7 +25,7 @@ import (
 	volumeclient "github.com/libopenstorage/openstorage/api/client/volume"
 	osdspec "github.com/libopenstorage/openstorage/api/spec"
 	volumeapi "github.com/libopenstorage/openstorage/volume"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	volumehelpers "k8s.io/cloud-provider/volume/helpers"
@@ -60,7 +60,10 @@ func (util *portworxVolumeUtil) CreateVolume(p *portworxVolumeProvisioner) (stri
 
 	capacity := p.options.PVC.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)]
 	// Portworx Volumes are specified in GiB
-	requestGiB := volumehelpers.RoundUpToGiB(capacity)
+	requestGiB, err := volumehelpers.RoundUpToGiB(capacity)
+	if err != nil {
+		return "", 0, nil, err
+	}
 
 	// Perform a best-effort parsing of parameters. Portworx 1.2.9 and later parses volume parameters from
 	// spec.VolumeLabels. So even if below SpecFromOpts() fails to parse certain parameters or
@@ -211,7 +214,11 @@ func (util *portworxVolumeUtil) ResizeVolume(spec *volume.Spec, newSize resource
 	}
 
 	vol := vols[0]
-	newSizeInBytes := uint64(volumehelpers.RoundUpToGiB(newSize) * volumehelpers.GiB)
+	tBytes, ok := newSize.AsInt64()
+	if !ok {
+		return fmt.Errorf("quantity %v is too great, overflows int64", newSize)
+	}
+	newSizeInBytes := uint64(tBytes)
 	if vol.Spec.Size >= newSizeInBytes {
 		klog.Infof("Portworx volume: %s already at size: %d greater than or equal to new "+
 			"requested size: %d. Skipping resize.", spec.Name(), vol.Spec.Size, newSizeInBytes)

diff --git a/pkg/volume/rbd/rbd_util.go b/pkg/volume/rbd/rbd_util.go
@@ -649,7 +649,11 @@ func (util *rbdUtil) ExpandImage(rbdExpander *rbdVolumeExpander, oldSize resourc
 	var err error
 
 	// Convert to MB that rbd defaults on.
-	sz := int(volumehelpers.RoundUpToMiB(newSize))
+	sz, err := volumehelpers.RoundUpToMiBInt(newSize)
+	if err != nil {
+		return oldSize, err
+	}
+
 	newVolSz := fmt.Sprintf("%d", sz)
 	newSizeQuant := resource.MustParse(fmt.Sprintf("%dMi", sz))
 

diff --git a/pkg/volume/scaleio/BUILD b/pkg/volume/scaleio/BUILD
@@ -23,6 +23,7 @@ go_test(
         "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
         "//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library",
         "//staging/src/k8s.io/client-go/util/testing:go_default_library",
+        "//staging/src/k8s.io/cloud-provider/volume/helpers:go_default_library",
         "//vendor/github.com/thecodeteam/goscaleio/types/v1:go_default_library",
         "//vendor/k8s.io/klog:go_default_library",
         "//vendor/k8s.io/utils/exec/testing:go_default_library",

diff --git a/pkg/volume/scaleio/sio_volume.go b/pkg/volume/scaleio/sio_volume.go
@@ -55,6 +55,10 @@ type sioVolume struct {
 	volume.MetricsNil
 }
 
+const (
+	minimumVolumeSizeGiB = 8
+)
+
 // *******************
 // volume.Volume Impl
 var _ volume.Volume = &sioVolume{}
@@ -272,21 +276,17 @@ func (v *sioVolume) Provision(selectedNode *api.Node, allowedTopologies []api.To
 
 	// setup volume attrributes
 	genName := v.generateName("k8svol", 11)
-	eightGig := int64(8 * volumehelpers.GiB)
 
 	capacity := v.options.PVC.Spec.Resources.Requests[api.ResourceName(api.ResourceStorage)]
 
-	volSizeBytes := capacity.Value()
-	volSizeGB := int64(volumehelpers.RoundUpToGiB(capacity))
-
-	if volSizeBytes == 0 {
-		return nil, fmt.Errorf("invalid volume size of 0 specified")
+	volSizeGiB, err := volumehelpers.RoundUpToGiB(capacity)
+	if err != nil {
+		return nil, err
 	}
 
-	if volSizeBytes < eightGig {
-		eightGiBCapacity := resource.NewQuantity(eightGig, resource.BinarySI)
-		volSizeGB = int64(volumehelpers.RoundUpToGiB(*eightGiBCapacity))
-		klog.V(4).Info(log("capacity less than 8Gi found, adjusted to %dGi", volSizeGB))
+	if volSizeGiB < minimumVolumeSizeGiB {
+		volSizeGiB = minimumVolumeSizeGiB
+		klog.V(4).Info(log("capacity less than 8Gi found, adjusted to %dGi", volSizeGiB))
 
 	}
 
@@ -298,7 +298,7 @@ func (v *sioVolume) Provision(selectedNode *api.Node, allowedTopologies []api.To
 
 	// create volume
 	volName := genName
-	vol, err := v.sioMgr.CreateVolume(volName, volSizeGB)
+	vol, err := v.sioMgr.CreateVolume(volName, volSizeGiB)
 	if err != nil {
 		klog.Error(log("provision failed while creating volume: %v", err))
 		return nil, err
@@ -333,7 +333,7 @@ func (v *sioVolume) Provision(selectedNode *api.Node, allowedTopologies []api.To
 			AccessModes:                   v.options.PVC.Spec.AccessModes,
 			Capacity: api.ResourceList{
 				api.ResourceName(api.ResourceStorage): resource.MustParse(
-					fmt.Sprintf("%dGi", volSizeGB),
+					fmt.Sprintf("%dGi", volSizeGiB),
 				),
 			},
 			PersistentVolumeSource: api.PersistentVolumeSource{

diff --git a/pkg/volume/scaleio/sio_volume_test.go b/pkg/volume/scaleio/sio_volume_test.go
@@ -23,6 +23,7 @@ import (
 	"strings"
 	"testing"
 
+	volumehelpers "k8s.io/cloud-provider/volume/helpers"
 	"k8s.io/klog"
 
 	api "k8s.io/api/core/v1"
@@ -425,7 +426,7 @@ func TestVolumeProvisionerWithIncompleteConfig(t *testing.T) {
 	}
 }
 
-func TestVolumeProvisionerWithZeroCapacity(t *testing.T) {
+func TestVolumeProvisionerWithMinimumCapacity(t *testing.T) {
 	plugMgr, tmpDir := newPluginMgr(t, makeScaleIOSecret(testSecret, testns))
 	defer os.RemoveAll(tmpDir)
 
@@ -441,7 +442,7 @@ func TestVolumeProvisionerWithZeroCapacity(t *testing.T) {
 	options := volume.VolumeOptions{
 		ClusterName:                   "testcluster",
 		PVName:                        "pvc-sio-dynamic-vol",
-		PVC:                           volumetest.CreateTestPVC("0Mi", []api.PersistentVolumeAccessMode{api.ReadWriteOnce}),
+		PVC:                           volumetest.CreateTestPVC("100Mi", []api.PersistentVolumeAccessMode{api.ReadWriteOnce}),
 		PersistentVolumeReclaimPolicy: api.PersistentVolumeReclaimDelete,
 	}
 	options.PVC.Namespace = testns
@@ -466,11 +467,25 @@ func TestVolumeProvisionerWithZeroCapacity(t *testing.T) {
 	}
 	sioVol.sioMgr.client = sio
 
-	_, err = provisioner.Provision(nil, nil)
-	if err == nil {
-		t.Fatalf("call to Provision() should fail with invalid capacity")
+	pv, err :=
+		provisioner.Provision(nil, nil)
+	if err != nil {
+		t.Fatalf("call to Provision() failed %v", err)
+	}
+
+	pvSize := pv.Spec.Capacity.Storage()
+	if pvSize == nil {
+		t.Fatalf("unexpected pv size: nil")
 	}
 
+	gibSize, err := volumehelpers.RoundUpToGiB(*pvSize)
+	if err != nil {
+		t.Fatalf("unexpected error while converting size to GiB: %v", err)
+	}
+
+	if gibSize != minimumVolumeSizeGiB {
+		t.Fatalf("expected GiB size to be %v got %v", minimumVolumeSizeGiB, gibSize)
+	}
 }
 
 func TestVolumeProvisionerWithSecretNamespace(t *testing.T) {