fix(instance): handle root volume with no size as default (#681)

Author: remyleone

go.mod

@@ -3,6 +3,7 @@ module github.com/terraform-providers/terraform-provider-scaleway
 require (
 	github.com/aws/aws-sdk-go v1.34.32
 	github.com/dnaeon/go-vcr v1.0.1
+	github.com/dustin/go-humanize v1.0.0
 	github.com/google/go-cmp v0.5.2
 	github.com/hashicorp/go-retryablehttp v0.6.7
 	github.com/hashicorp/terraform-plugin-sdk/v2 v2.2.0

go.sum

@@ -79,6 +79,8 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/dnaeon/go-vcr v1.0.1 h1:r8L/HqC0Hje5AXMu1ooW8oyQyOFv4GxqpL0nRP7SLLY=
 github.com/dnaeon/go-vcr v1.0.1/go.mod h1:aBB1+wY4s93YsC3HHjMBMrwTj2R9FHDzUr9KyGc8n1E=
+github.com/dustin/go-humanize v1.0.0 h1:VSnTsYCnlFHaM2/igO1h6X3HA71jcobQuxemgkq4zYo=
+github.com/dustin/go-humanize v1.0.0/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk=
 github.com/emirpasic/gods v1.12.0 h1:QAUIPSaCu4G+POclxeqb3F+WPpdKqFGlw36+yOzGlrg=
 github.com/emirpasic/gods v1.12.0/go.mod h1:YfzfFFoVP/catgzJb4IKIqXjX78Ha8FMSDh3ymbK86o=
 github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4=
@@ -297,12 +299,6 @@ github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZN
 github.com/posener/complete v1.1.1/go.mod h1:em0nMJCgc9GFtwrmVmEMR/ZL6WyhyjMBndrE9hABlRI=
 github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA=
 github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4=
-github.com/scaleway/scaleway-sdk-go v1.0.0-beta.7.0.20201012095732-98ec365545de h1:yhT7zTv3WAMD3S3GqkpAFIL8O76emUsBKsiBcVBb4Bg=
-github.com/scaleway/scaleway-sdk-go v1.0.0-beta.7.0.20201012095732-98ec365545de/go.mod h1:CJJ5VAbozOl0yEw7nHB9+7BXTJbIn6h7W+f6Gau5IP8=
-github.com/scaleway/scaleway-sdk-go v1.0.0-beta.7.0.20201106174615-fd524d38cee8 h1:OUM8gK4Frdx3yhDJWQSToADtrvCTGaZXYrm6O3j3JaQ=
-github.com/scaleway/scaleway-sdk-go v1.0.0-beta.7.0.20201106174615-fd524d38cee8/go.mod h1:CJJ5VAbozOl0yEw7nHB9+7BXTJbIn6h7W+f6Gau5IP8=
-github.com/scaleway/scaleway-sdk-go v1.0.0-beta.7.0.20201109203223-9446a84bfb2a h1:bcVe/pXc4HWi8RwkiiYOdIjYWjQvR5eeMRaepXGW298=
-github.com/scaleway/scaleway-sdk-go v1.0.0-beta.7.0.20201109203223-9446a84bfb2a/go.mod h1:CJJ5VAbozOl0yEw7nHB9+7BXTJbIn6h7W+f6Gau5IP8=
 github.com/scaleway/scaleway-sdk-go v1.0.0-beta.7.0.20201113152841-1153aa56e20e h1:KW5n7q2CMM/MsFNAwdZWB0UioVa3E3XQSrKRZcjmaGo=
 github.com/scaleway/scaleway-sdk-go v1.0.0-beta.7.0.20201113152841-1153aa56e20e/go.mod h1:CJJ5VAbozOl0yEw7nHB9+7BXTJbIn6h7W+f6Gau5IP8=
 github.com/sergi/go-diff v1.0.0 h1:Kpca3qRNrduNnOQeazBd0ysaKrUJiIuISHxogkT9RPQ=

scaleway/helpers_instance.go

@@ -7,6 +7,7 @@ import (
 	"sort"
 	"time"
 
+	"github.com/dustin/go-humanize"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/scaleway/scaleway-sdk-go/api/instance/v1"
 	"github.com/scaleway/scaleway-sdk-go/scw"
@@ -159,3 +160,82 @@ func reachState(ctx context.Context, instanceAPI *instance.API, zone scw.Zone, s
 	}
 	return nil
 }
+
+// getServerType is a util to get a instance.ServerType by its commercialType
+func getServerType(apiInstance *instance.API, zone scw.Zone, commercialType string) *instance.ServerType {
+	serverType := (*instance.ServerType)(nil)
+
+	serverTypesRes, err := apiInstance.ListServersTypes(&instance.ListServersTypesRequest{
+		Zone: zone,
+	})
+	if err != nil {
+		l.Warningf("cannot get server types: %s", err)
+	} else {
+		serverType = serverTypesRes.Servers[commercialType]
+		if serverType == nil {
+			l.Warningf("unrecognized server type: %s", commercialType)
+		}
+	}
+
+	return serverType
+}
+
+// validateLocalVolumeSizes validates the total size of local volumes.
+func validateLocalVolumeSizes(volumes map[string]*instance.VolumeTemplate, serverType *instance.ServerType, commercialType string) error {
+	// Calculate local volume total size.
+	var localVolumeTotalSize scw.Size
+	for _, volume := range volumes {
+		if volume.VolumeType == instance.VolumeVolumeTypeLSSD {
+			localVolumeTotalSize += volume.Size
+		}
+	}
+
+	volumeConstraint := serverType.VolumesConstraint
+
+	// If no root volume provided, count the default root volume size added by the API.
+	if rootVolume := volumes["0"]; rootVolume == nil {
+		localVolumeTotalSize += volumeConstraint.MinSize
+	}
+
+	if localVolumeTotalSize < volumeConstraint.MinSize || localVolumeTotalSize > volumeConstraint.MaxSize {
+		min := humanize.Bytes(uint64(volumeConstraint.MinSize))
+		if volumeConstraint.MinSize == volumeConstraint.MaxSize {
+			return fmt.Errorf("%s total local volume size must be equal to %s", commercialType, min)
+		}
+
+		max := humanize.Bytes(uint64(volumeConstraint.MaxSize))
+		return fmt.Errorf("%s total local volume size must be between %s and %s", commercialType, min, max)
+	}
+
+	return nil
+}
+
+// sanitizeVolumeMap removes extra data for API validation.
+//
+// On the api side, there are two possibles validation schemas for volumes and the validator will be chosen dynamically depending on the passed JSON request
+// - With an image (in that case the root volume can be skipped because it is taken from the image)
+// - Without an image (in that case, the root volume must be defined)
+func sanitizeVolumeMap(serverName string, volumes map[string]*instance.VolumeTemplate) map[string]*instance.VolumeTemplate {
+	m := make(map[string]*instance.VolumeTemplate)
+
+	for index, v := range volumes {
+		v.Name = serverName + "-" + index
+
+		// Remove extra data for API validation.
+		switch {
+		// If a volume already got an ID it is passed as it to the API without specifying the volume type.
+		// TODO: Fix once instance accept volume type in the schema validation
+		case v.ID != "":
+			v = &instance.VolumeTemplate{ID: v.ID, Name: v.Name}
+		// For the root volume (index 0) if the specified size is not 0 it is considered as a new volume
+		// It does not have yet a volume ID, it is passed to the API with only the size to be dynamically created by the API
+		case index == "0" && v.Size != 0:
+			v = &instance.VolumeTemplate{Size: v.Size}
+		// If none of the above conditions are met, the volume is passed as it to the API
+		default:
+		}
+		m[index] = v
+	}
+
+	return m
+}

scaleway/resource_instance_server.go

@@ -278,22 +278,50 @@ func resourceScalewayInstanceServerCreate(ctx context.Context, d *schema.Resourc
 		req.PlacementGroup = expandStringPtr(expandZonedID(placementGroupID).ID)
 	}
 
+	serverType := getServerType(instanceAPI, req.Zone, req.CommercialType)
+	if serverType == nil {
+		return diag.FromErr(fmt.Errorf("could not find a server type associated with %s", req.CommercialType))
+	}
+
 	req.Volumes = make(map[string]*instance.VolumeTemplate)
 	if size, ok := d.GetOk("root_volume.0.size_in_gb"); ok {
 		req.Volumes["0"] = &instance.VolumeTemplate{
 			Size: scw.Size(uint64(size.(int)) * gb),
 		}
+	} else {
+		// We had a local root volume if it is not already present
+		req.Volumes["0"] = &instance.VolumeTemplate{
+			Name:       newRandomName("vol"),
+			VolumeType: instance.VolumeVolumeTypeLSSD,
+			Size:       serverType.VolumesConstraint.MinSize,
+		}
 	}
 
 	if raw, ok := d.GetOk("additional_volume_ids"); ok {
 		for i, volumeID := range raw.([]interface{}) {
+			// We have to get the volume to know whether it is a local or a block volume
+			vol, err := instanceAPI.GetVolume(&instance.GetVolumeRequest{
+				VolumeID: expandZonedID(volumeID).ID,
+			})
+			if err != nil {
+				return diag.FromErr(err)
+			}
 			req.Volumes[strconv.Itoa(i+1)] = &instance.VolumeTemplate{
-				ID:   expandZonedID(volumeID).ID,
-				Name: newRandomName("vol"),
+				ID:         vol.Volume.ID,
+				Name:       vol.Volume.Name,
+				VolumeType: vol.Volume.VolumeType,
 			}
 		}
 	}
 
+	// Validate total local volume sizes.
+	if err := validateLocalVolumeSizes(req.Volumes, serverType, req.CommercialType); err != nil {
+		return diag.FromErr(err)
+	}
+
+	// Sanitize the volume map to respect API schemas
+	req.Volumes = sanitizeVolumeMap(req.Name, req.Volumes)
+
 	res, err := instanceAPI.CreateServer(req, scw.WithContext(ctx))
 	if err != nil {
 		return diag.FromErr(err)
@@ -441,10 +469,6 @@ func resourceScalewayInstanceServerRead(ctx context.Context, d *schema.ResourceD
 			rootVolume["volume_id"] = newZonedID(zone, volume.ID).String()
 			rootVolume["size_in_gb"] = int(uint64(volume.Size) / gb)
 
-			if _, exist := rootVolume["delete_on_termination"]; !exist {
-				rootVolume["delete_on_termination"] = true // default value does not work on list
-			}
-
 			_ = d.Set("root_volume", []map[string]interface{}{rootVolume})
 		} else {
 			additionalVolumesIDs = append(additionalVolumesIDs, newZonedID(zone, volume.ID).String())

scaleway/resource_instance_server_test.go

@@ -863,3 +863,35 @@ func TestAccScalewayInstanceServer_AlterTags(t *testing.T) {
 		},
 	})
 }
+
+func TestAccScalewayInstanceServer_WithDefaultRootVolumeAndAdditionalVolume(t *testing.T) {
+	tt := NewTestTools(t)
+	defer tt.Cleanup()
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:          func() { testAccPreCheck(t) },
+		ProviderFactories: tt.ProviderFactories,
+		CheckDestroy:      testAccCheckScalewayInstanceServerDestroy(tt),
+		Steps: []resource.TestStep{
+			{
+				Config: `
+					resource "scaleway_instance_volume" "data" {
+						size_in_gb = 100
+						type = "b_ssd"
+					}
+
+					resource "scaleway_instance_server" "main" {
+						type = "DEV1-S"
+						image = "ubuntu-bionic"
+						root_volume {
+							delete_on_termination = false
+					  	}
+						additional_volume_ids = [ scaleway_instance_volume.data.id ]
+					}
+				`,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckScalewayInstanceServerExists(tt, "scaleway_instance_server.main"),
+				),
+			},
+		},
+	})
+}