From 75fdb6d554d760cb3c4664b174c3f9bf9673f6a3 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Fri, 8 May 2020 15:53:52 -0400 Subject: [PATCH] When casting resource.Quantity to int64 it may overflow We need to ensure that when converting resource.Quantity to int64 it does not overflow and if it does, then an error is returned back to the use rather than attempting expansion and provisioning operations with scaled value. --- pkg/volume/azure_file/azure_file.go | 8 +- pkg/volume/azure_file/azure_provision.go | 6 +- pkg/volume/gcepd/gce_util.go | 9 +- pkg/volume/glusterfs/glusterfs.go | 13 +- pkg/volume/portworx/portworx_util.go | 13 +- pkg/volume/rbd/rbd_util.go | 6 +- pkg/volume/scaleio/BUILD | 1 + pkg/volume/scaleio/sio_volume.go | 24 +- pkg/volume/scaleio/sio_volume_test.go | 25 +- .../cloud-provider/volume/helpers/rounding.go | 100 +++++--- .../volume/helpers/rounding_test.go | 230 ++++++++++++------ .../k8s.io/legacy-cloud-providers/aws/aws.go | 5 +- .../azure/azure_managedDiskController.go | 6 +- .../legacy-cloud-providers/gce/gce_disks.go | 8 +- 14 files changed, 319 insertions(+), 135 deletions(-) diff --git a/pkg/volume/azure_file/azure_file.go b/pkg/volume/azure_file/azure_file.go index 3db30787f60..451f4f3ca3f 100644 --- a/pkg/volume/azure_file/azure_file.go +++ b/pkg/volume/azure_file/azure_file.go @@ -183,7 +183,13 @@ func (plugin *azureFilePlugin) ExpandVolumeDevice( return oldSize, err } - if err := azure.ResizeFileShare(resourceGroup, accountName, shareName, int(volumehelpers.RoundUpToGiB(newSize))); err != nil { + requestGiB, err := volumehelpers.RoundUpToGiBInt(newSize) + + if err != nil { + return oldSize, err + } + + if err := azure.ResizeFileShare(resourceGroup, accountName, 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 index abb5bb8750b..2db069ca27f 100644 --- a/pkg/volume/azure_file/azure_provision.go +++ b/pkg/volume/azure_file/azure_provision.go @@ -158,7 +158,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 index ce18270a1b1..c1a607948e2 100644 --- 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 index 698c4e895f0..673f4a13d73 100644 --- 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 index e76031132ab..a8373d9ff2a 100644 --- 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 index 94b409aa88a..7bb6c3e6945 100644 --- 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 index f1410f50657..156e91f99c0 100644 --- 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 index 0d9efeb181b..139ec9e37c1 100644 --- 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 index ca871817589..3968704366d 100644 --- 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) { diff --git a/staging/src/k8s.io/cloud-provider/volume/helpers/rounding.go b/staging/src/k8s.io/cloud-provider/volume/helpers/rounding.go index a4ccaa03480..a3b8ec57a8e 100644 --- a/staging/src/k8s.io/cloud-provider/volume/helpers/rounding.go +++ b/staging/src/k8s.io/cloud-provider/volume/helpers/rounding.go @@ -39,74 +39,88 @@ const ( KiB = 1024 ) -// RoundUpToGB rounds up given quantity to chunks of GB -func RoundUpToGB(size resource.Quantity) int64 { - requestBytes := size.Value() - return roundUpSize(requestBytes, GB) -} - // RoundUpToGiB rounds up given quantity upto chunks of GiB -func RoundUpToGiB(size resource.Quantity) int64 { - requestBytes := size.Value() - return roundUpSize(requestBytes, GiB) +func RoundUpToGiB(size resource.Quantity) (int64, error) { + requestBytes, ok := size.AsInt64() + if !ok { + return 0, fmt.Errorf("quantity %v is too great, overflows int64", size) + } + return roundUpSize(requestBytes, GiB), nil } // RoundUpToMB rounds up given quantity to chunks of MB -func RoundUpToMB(size resource.Quantity) int64 { - requestBytes := size.Value() - return roundUpSize(requestBytes, MB) +func RoundUpToMB(size resource.Quantity) (int64, error) { + requestBytes, ok := size.AsInt64() + if !ok { + return 0, fmt.Errorf("quantity %v is too great, overflows int64", size) + } + return roundUpSize(requestBytes, MB), nil } // RoundUpToMiB rounds up given quantity upto chunks of MiB -func RoundUpToMiB(size resource.Quantity) int64 { - requestBytes := size.Value() - return roundUpSize(requestBytes, MiB) +func RoundUpToMiB(size resource.Quantity) (int64, error) { + requestBytes, ok := size.AsInt64() + if !ok { + return 0, fmt.Errorf("quantity %v is too great, overflows int64", size) + } + return roundUpSize(requestBytes, MiB), nil } // RoundUpToKB rounds up given quantity to chunks of KB -func RoundUpToKB(size resource.Quantity) int64 { - requestBytes := size.Value() - return roundUpSize(requestBytes, KB) +func RoundUpToKB(size resource.Quantity) (int64, error) { + requestBytes, ok := size.AsInt64() + if !ok { + return 0, fmt.Errorf("quantity %v is too great, overflows int64", size) + } + return roundUpSize(requestBytes, KB), nil } // RoundUpToKiB rounds up given quantity upto chunks of KiB -func RoundUpToKiB(size resource.Quantity) int64 { - requestBytes := size.Value() - return roundUpSize(requestBytes, KiB) -} - -// RoundUpToGBInt rounds up given quantity to chunks of GB. It returns an -// int instead of an int64 and an error if there's overflow -func RoundUpToGBInt(size resource.Quantity) (int, error) { - requestBytes := size.Value() - return roundUpSizeInt(requestBytes, GB) +func RoundUpToKiB(size resource.Quantity) (int64, error) { + requestBytes, ok := size.AsInt64() + if !ok { + return 0, fmt.Errorf("quantity %v is too great, overflows int64", size) + } + return roundUpSize(requestBytes, KiB), nil } // RoundUpToGiBInt rounds up given quantity upto chunks of GiB. It returns an // int instead of an int64 and an error if there's overflow func RoundUpToGiBInt(size resource.Quantity) (int, error) { - requestBytes := size.Value() + requestBytes, ok := size.AsInt64() + if !ok { + return 0, fmt.Errorf("quantity %v is too great, overflows int64", size) + } return roundUpSizeInt(requestBytes, GiB) } // RoundUpToMBInt rounds up given quantity to chunks of MB. It returns an // int instead of an int64 and an error if there's overflow func RoundUpToMBInt(size resource.Quantity) (int, error) { - requestBytes := size.Value() + requestBytes, ok := size.AsInt64() + if !ok { + return 0, fmt.Errorf("quantity %v is too great, overflows int64", size) + } return roundUpSizeInt(requestBytes, MB) } // RoundUpToMiBInt rounds up given quantity upto chunks of MiB. It returns an // int instead of an int64 and an error if there's overflow func RoundUpToMiBInt(size resource.Quantity) (int, error) { - requestBytes := size.Value() + requestBytes, ok := size.AsInt64() + if !ok { + return 0, fmt.Errorf("quantity %v is too great, overflows int64", size) + } return roundUpSizeInt(requestBytes, MiB) } // RoundUpToKBInt rounds up given quantity to chunks of KB. It returns an // int instead of an int64 and an error if there's overflow func RoundUpToKBInt(size resource.Quantity) (int, error) { - requestBytes := size.Value() + requestBytes, ok := size.AsInt64() + if !ok { + return 0, fmt.Errorf("quantity %v is too great, overflows int64", size) + } return roundUpSizeInt(requestBytes, KB) } @@ -117,6 +131,16 @@ func RoundUpToKiBInt(size resource.Quantity) (int, error) { return roundUpSizeInt(requestBytes, KiB) } +// RoundUpToGiBInt32 rounds up given quantity up to chunks of GiB. It returns an +// int32 instead of an int64 and an error if there's overflow +func RoundUpToGiBInt32(size resource.Quantity) (int32, error) { + requestBytes, ok := size.AsInt64() + if !ok { + return 0, fmt.Errorf("quantity %v is too great, overflows int64", size) + } + return roundUpSizeInt32(requestBytes, GiB) +} + // roundUpSizeInt calculates how many allocation units are needed to accommodate // a volume of given size. It returns an int instead of an int64 and an error if // there's overflow @@ -129,6 +153,18 @@ func roundUpSizeInt(volumeSizeBytes int64, allocationUnitBytes int64) (int, erro return roundedUpInt, nil } +// roundUpSizeInt32 calculates how many allocation units are needed to accommodate +// a volume of given size. It returns an int32 instead of an int64 and an error if +// there's overflow +func roundUpSizeInt32(volumeSizeBytes int64, allocationUnitBytes int64) (int32, error) { + roundedUp := roundUpSize(volumeSizeBytes, allocationUnitBytes) + roundedUpInt32 := int32(roundedUp) + if int64(roundedUpInt32) != roundedUp { + return 0, fmt.Errorf("quantity %v is too great, overflows int32", roundedUp) + } + return roundedUpInt32, nil +} + // roundUpSize calculates how many allocation units are needed to accommodate // a volume of given size. E.g. when user wants 1500MiB volume, while AWS EBS // allocates volumes in gibibyte-sized chunks, diff --git a/staging/src/k8s.io/cloud-provider/volume/helpers/rounding_test.go b/staging/src/k8s.io/cloud-provider/volume/helpers/rounding_test.go index 6b2fa12b1b7..24bc17a955e 100644 --- a/staging/src/k8s.io/cloud-provider/volume/helpers/rounding_test.go +++ b/staging/src/k8s.io/cloud-provider/volume/helpers/rounding_test.go @@ -22,61 +22,12 @@ import ( "k8s.io/apimachinery/pkg/api/resource" ) -func Test_RoundUpToGB(t *testing.T) { - testcases := []struct { - name string - resource resource.Quantity - roundedVal int64 - }{ - { - name: "round Ki to GB", - resource: resource.MustParse("1000Ki"), - roundedVal: int64(1), - }, - { - name: "round k to GB", - resource: resource.MustParse("1000k"), - roundedVal: int64(1), - }, - { - name: "round Mi to GB", - resource: resource.MustParse("1000Mi"), - roundedVal: int64(2), - }, - { - name: "round M to GB", - resource: resource.MustParse("1000M"), - roundedVal: int64(1), - }, - { - name: "round G to GB", - resource: resource.MustParse("1000G"), - roundedVal: int64(1000), - }, - { - name: "round Gi to GB", - resource: resource.MustParse("1000Gi"), - roundedVal: int64(1074), - }, - } - - for _, test := range testcases { - t.Run(test.name, func(t *testing.T) { - val := RoundUpToGB(test.resource) - if val != test.roundedVal { - t.Logf("actual rounded value: %d", val) - t.Logf("expected rounded value: %d", test.roundedVal) - t.Error("unexpected rounded value") - } - }) - } -} - func Test_RoundUpToGiB(t *testing.T) { testcases := []struct { - name string - resource resource.Quantity - roundedVal int64 + name string + resource resource.Quantity + roundedVal int64 + expectError bool }{ { name: "round Ki to GiB", @@ -108,11 +59,25 @@ func Test_RoundUpToGiB(t *testing.T) { resource: resource.MustParse("1000Gi"), roundedVal: int64(1000), }, + { + name: "round overflowed quantity to int64", + resource: resource.MustParse("73786976299133170k"), + roundedVal: int64(0), + expectError: true, + }, } for _, test := range testcases { t.Run(test.name, func(t *testing.T) { - val := RoundUpToGiB(test.resource) + val, err := RoundUpToGiB(test.resource) + if !test.expectError && err != nil { + t.Errorf("expected no error got: %v", err) + } + + if test.expectError && err == nil { + t.Errorf("expected error but got nothing") + } + if val != test.roundedVal { t.Logf("actual rounded value: %d", val) t.Logf("expected rounded value: %d", test.roundedVal) @@ -124,9 +89,10 @@ func Test_RoundUpToGiB(t *testing.T) { func Test_RoundUpToMB(t *testing.T) { testcases := []struct { - name string - resource resource.Quantity - roundedVal int64 + name string + resource resource.Quantity + roundedVal int64 + expectError bool }{ { name: "round Ki to MB", @@ -158,11 +124,25 @@ func Test_RoundUpToMB(t *testing.T) { resource: resource.MustParse("1000Gi"), roundedVal: int64(1073742), }, + { + name: "round overflowed quantity to int64", + resource: resource.MustParse("73786976299133170k"), + roundedVal: int64(0), + expectError: true, + }, } for _, test := range testcases { t.Run(test.name, func(t *testing.T) { - val := RoundUpToMB(test.resource) + val, err := RoundUpToMB(test.resource) + if !test.expectError && err != nil { + t.Errorf("expected no error got: %v", err) + } + + if test.expectError && err == nil { + t.Errorf("expected error but got nothing") + } + if val != test.roundedVal { t.Logf("actual rounded value: %d", val) t.Logf("expected rounded value: %d", test.roundedVal) @@ -174,9 +154,10 @@ func Test_RoundUpToMB(t *testing.T) { func Test_RoundUpToMiB(t *testing.T) { testcases := []struct { - name string - resource resource.Quantity - roundedVal int64 + name string + resource resource.Quantity + roundedVal int64 + expectError bool }{ { name: "round Ki to MiB", @@ -208,11 +189,25 @@ func Test_RoundUpToMiB(t *testing.T) { resource: resource.MustParse("1000Gi"), roundedVal: int64(1024000), }, + { + name: "round overflowed quantity to int64", + resource: resource.MustParse("73786976299133170k"), + roundedVal: int64(0), + expectError: true, + }, } for _, test := range testcases { t.Run(test.name, func(t *testing.T) { - val := RoundUpToMiB(test.resource) + val, err := RoundUpToMiB(test.resource) + if !test.expectError && err != nil { + t.Errorf("expected no error got: %v", err) + } + + if test.expectError && err == nil { + t.Errorf("expected error but got nothing") + } + if val != test.roundedVal { t.Logf("actual rounded value: %d", val) t.Logf("expected rounded value: %d", test.roundedVal) @@ -224,9 +219,10 @@ func Test_RoundUpToMiB(t *testing.T) { func Test_RoundUpToKB(t *testing.T) { testcases := []struct { - name string - resource resource.Quantity - roundedVal int64 + name string + resource resource.Quantity + roundedVal int64 + expectError bool }{ { name: "round Ki to KB", @@ -258,11 +254,25 @@ func Test_RoundUpToKB(t *testing.T) { resource: resource.MustParse("1000Gi"), roundedVal: int64(1073741824), }, + { + name: "round overflowed quantity to int64", + resource: resource.MustParse("73786976299133170k"), + roundedVal: int64(0), + expectError: true, + }, } for _, test := range testcases { t.Run(test.name, func(t *testing.T) { - val := RoundUpToKB(test.resource) + val, err := RoundUpToKB(test.resource) + if !test.expectError && err != nil { + t.Errorf("expected no error got: %v", err) + } + + if test.expectError && err == nil { + t.Errorf("expected error but got nothing") + } + if val != test.roundedVal { t.Logf("actual rounded value: %d", val) t.Logf("expected rounded value: %d", test.roundedVal) @@ -274,9 +284,10 @@ func Test_RoundUpToKB(t *testing.T) { func Test_RoundUpToKiB(t *testing.T) { testcases := []struct { - name string - resource resource.Quantity - roundedVal int64 + name string + resource resource.Quantity + roundedVal int64 + expectError bool }{ { name: "round Ki to KiB", @@ -308,11 +319,90 @@ func Test_RoundUpToKiB(t *testing.T) { resource: resource.MustParse("1000Gi"), roundedVal: int64(1048576000), }, + { + name: "round overflowed quantity to int64", + resource: resource.MustParse("73786976299133170k"), + roundedVal: int64(0), + expectError: true, + }, } for _, test := range testcases { t.Run(test.name, func(t *testing.T) { - val := RoundUpToKiB(test.resource) + val, err := RoundUpToKiB(test.resource) + if !test.expectError && err != nil { + t.Errorf("expected no error got: %v", err) + } + + if test.expectError && err == nil { + t.Errorf("expected error but got nothing") + } + + if val != test.roundedVal { + t.Logf("actual rounded value: %d", val) + t.Logf("expected rounded value: %d", test.roundedVal) + t.Error("unexpected rounded value") + } + }) + } +} + +func Test_RoundUpToGiBInt32(t *testing.T) { + testcases := []struct { + name string + resource resource.Quantity + roundedVal int32 + expectError bool + }{ + { + name: "round Ki to GiB", + resource: resource.MustParse("1000Ki"), + roundedVal: int32(1), + }, + { + name: "round k to GiB", + resource: resource.MustParse("1000k"), + roundedVal: int32(1), + }, + { + name: "round Mi to GiB", + resource: resource.MustParse("1000Mi"), + roundedVal: int32(1), + }, + { + name: "round M to GiB", + resource: resource.MustParse("1000M"), + roundedVal: int32(1), + }, + { + name: "round G to GiB", + resource: resource.MustParse("1000G"), + roundedVal: int32(932), + }, + { + name: "round Gi to GiB", + resource: resource.MustParse("1000Gi"), + roundedVal: int32(1000), + }, + { + name: "round overflowed quantity to int32", + resource: resource.MustParse("73786976299133170k"), + roundedVal: int32(0), + expectError: true, + }, + } + + for _, test := range testcases { + t.Run(test.name, func(t *testing.T) { + val, err := RoundUpToGiBInt32(test.resource) + if !test.expectError && err != nil { + t.Errorf("expected no error got: %v", err) + } + + if test.expectError && err == nil { + t.Errorf("expected error but got nothing") + } + if val != test.roundedVal { t.Logf("actual rounded value: %d", val) t.Logf("expected rounded value: %d", test.roundedVal) diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go b/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go index 89c529d4bc2..8e198fdb9c3 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go +++ b/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go @@ -2815,7 +2815,10 @@ func (c *Cloud) ResizeDisk( return oldSize, descErr } // AWS resizes in chunks of GiB (not GB) - requestGiB := volumehelpers.RoundUpToGiB(newSize) + requestGiB, err := volumehelpers.RoundUpToGiB(newSize) + if err != nil { + return oldSize, err + } newSizeQuant := resource.MustParse(fmt.Sprintf("%dGi", requestGiB)) // If disk already if of greater or equal size than requested we return diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_managedDiskController.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_managedDiskController.go index 13f6c59c38d..6df0f317095 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_managedDiskController.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_managedDiskController.go @@ -284,7 +284,11 @@ func (c *ManagedDiskController) ResizeDisk(diskURI string, oldSize resource.Quan } // Azure resizes in chunks of GiB (not GB) - requestGiB := int32(volumehelpers.RoundUpToGiB(newSize)) + requestGiB, err := volumehelpers.RoundUpToGiBInt32(newSize) + if err != nil { + return oldSize, err + } + newSizeQuant := resource.MustParse(fmt.Sprintf("%dGi", requestGiB)) klog.V(2).Infof("azureDisk - begin to resize disk(%s) with new size(%d), old size(%v)", diskName, requestGiB, oldSize) diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_disks.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_disks.go index 0600f087172..37b244b03f7 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_disks.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_disks.go @@ -25,7 +25,7 @@ import ( "net/http" "strings" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/types" @@ -809,7 +809,11 @@ func (g *Cloud) ResizeDisk(diskToResize string, oldSize resource.Quantity, newSi } // GCE resizes in chunks of GiBs - requestGIB := volumehelpers.RoundUpToGiB(newSize) + requestGIB, err := volumehelpers.RoundUpToGiB(newSize) + if err != nil { + return oldSize, err + } + newSizeQuant := resource.MustParse(fmt.Sprintf("%dGi", requestGIB)) // If disk is already of size equal or greater than requested size, we simply return