refactor!: change the way an ip is attached to a server

CHANGELOG.md

@@ -3,7 +3,11 @@
 BREAKING CHANGES:
 
 * `scaleway_instance_placement_group`: change default policy from `low_latency` to `max_availability` ([#329](https://github.com/terraform-providers/terraform-provider-scaleway/pull/329)).
-This change was made to keep coherence with default API behaviour.
+This change was made to keep coherence with default API behavior.
+* `scaleway_instance_server`: The attribute `disable_dynamic_ip` has been removed in favor of `enable_dynamic_ip` ([#365](https://github.com/terraform-providers/terraform-provider-scaleway/pull/365)).
+* `scaleway_instance_ip`: In order to resolve circular dependencies that can happen in some use case we changed
+the way an IP is attached to a server. The attribute `scaleway_instance_ip.server_id` has been removed in
+favor of `scaleway_instance_server.ip_id` ([#365](https://github.com/terraform-providers/terraform-provider-scaleway/pull/365)).
 
 FEATURE:
 

scaleway/resource_instance_ip.go

@@ -9,8 +9,12 @@ func resourceScalewayInstanceIP() *schema.Resource {
 	return &schema.Resource{
 		Create: resourceScalewayInstanceIPCreate,
 		Read:   resourceScalewayInstanceIPRead,
-		Update: resourceScalewayInstanceIPUpdate,
 		Delete: resourceScalewayInstanceIPDelete,
+
+		// Because of removed attribute server_id we must add an update func that does nothing. This could be removed on
+		// next major release.
+		Update: resourceScalewayInstanceIPRead,
+
 		Importer: &schema.ResourceImporter{
 			State: schema.ImportStatePassthrough,
 		},
@@ -23,19 +27,14 @@ func resourceScalewayInstanceIP() *schema.Resource {
 			},
 			"reverse": {
 				Type:        schema.TypeString,
-				Optional:    true,
 				Computed:    true,
-				Deprecated:  "Deprecated in favor of scaleway_instance_ip_reverse_dns resource",
 				Description: "The reverse DNS for this IP",
 			},
 			"server_id": {
-				Type:             schema.TypeString,
-				Optional:         true,
-				Computed:         true,
-				Description:      "The server associated with this IP",
-				ValidateFunc:     validationUUIDorUUIDWithLocality(),
-				DiffSuppressFunc: suppressLocality,
-				Deprecated:       "Use the ip_id in scaleway_instance_server",
+				Type:        schema.TypeString,
+				Optional:    true,
+				Removed:     "server_id has been removed in favor of scaleway_instance_server.ip_id",
+				Description: "The server associated with this IP",
 			},
 			"zone":            zoneSchema(),
 			"organization_id": organizationIDSchema(),
@@ -57,32 +56,7 @@ func resourceScalewayInstanceIPCreate(d *schema.ResourceData, m interface{}) err
 		return err
 	}
 
-	reverse := d.Get("reverse").(string)
-	if reverse != "" {
-		_, err = instanceAPI.UpdateIP(&instance.UpdateIPRequest{
-			Zone:    zone,
-			IP:      res.IP.ID,
-			Reverse: &instance.NullableStringValue{Value: reverse},
-		})
-		if err != nil {
-			return err
-		}
-	}
-
 	d.SetId(newZonedId(zone, res.IP.ID))
-
-	serverID := expandID(d.Get("server_id"))
-	if serverID != "" {
-		_, err = instanceAPI.AttachIP(&instance.AttachIPRequest{
-			Zone:     zone,
-			IP:       res.IP.ID,
-			ServerID: serverID,
-		})
-		if err != nil {
-			return err
-		}
-
-	}
 	return resourceScalewayInstanceIPRead(d, m)
 }
 
@@ -111,63 +85,9 @@ func resourceScalewayInstanceIPRead(d *schema.ResourceData, m interface{}) error
 	d.Set("organization_id", res.IP.Organization)
 	d.Set("reverse", res.IP.Reverse)
 
-	if res.IP.Server != nil {
-		d.Set("server_id", res.IP.Server.ID)
-	} else {
-		d.Set("server_id", "")
-	}
-
 	return nil
 }
 
-func resourceScalewayInstanceIPUpdate(d *schema.ResourceData, m interface{}) error {
-	instanceAPI, zone, ID, err := getInstanceAPIWithZoneAndID(m, d.Id())
-	if err != nil {
-		return err
-	}
-
-	if d.HasChange("reverse") {
-		l.Debugf("updating IP %q reverse to %q\n", d.Id(), d.Get("reverse"))
-
-		updateReverseReq := &instance.UpdateIPRequest{
-			Zone: zone,
-			IP:   ID,
-		}
-
-		reverse := d.Get("reverse").(string)
-		if reverse == "" {
-			updateReverseReq.Reverse = &instance.NullableStringValue{Null: true}
-		} else {
-			updateReverseReq.Reverse = &instance.NullableStringValue{Value: reverse}
-		}
-		_, err = instanceAPI.UpdateIP(updateReverseReq)
-		if err != nil {
-			return err
-		}
-	}
-
-	if d.HasChange("server_id") {
-		serverID := expandID(d.Get("server_id"))
-		if serverID != "" {
-			_, err = instanceAPI.AttachIP(&instance.AttachIPRequest{
-				Zone:     zone,
-				IP:       ID,
-				ServerID: serverID,
-			})
-		} else {
-			_, err = instanceAPI.DetachIP(&instance.DetachIPRequest{
-				Zone: zone,
-				IP:   ID,
-			})
-		}
-		if err != nil {
-			return err
-		}
-	}
-
-	return resourceScalewayInstanceIPRead(d, m)
-}
-
 func resourceScalewayInstanceIPDelete(d *schema.ResourceData, m interface{}) error {
 	instanceAPI, zone, ID, err := getInstanceAPIWithZoneAndID(m, d.Id())
 	if err != nil {

scaleway/resource_instance_ip_test.go

@@ -16,23 +16,13 @@ func TestAccScalewayInstanceIP(t *testing.T) {
 		CheckDestroy: testAccCheckScalewayInstanceIPDestroy,
 		Steps: []resource.TestStep{
 			{
-				Config: testAccScalewayInstanceIPConfig[0],
+				Config: `
+					resource "scaleway_instance_ip" "base" {}
+					resource "scaleway_instance_ip" "scaleway" {}
+				`,
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckScalewayInstanceIPExists("scaleway_instance_ip.base"),
 					testAccCheckScalewayInstanceIPExists("scaleway_instance_ip.scaleway"),
-					resource.TestCheckResourceAttr("scaleway_instance_ip.base", "reverse", ""),
-					resource.TestCheckResourceAttr("scaleway_instance_ip.scaleway", "reverse", "www.scaleway.com"),
-				),
-			},
-			{
-				Config: testAccScalewayInstanceIPConfig[1],
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckScalewayInstanceIPExists("scaleway_instance_ip.base"),
-					testAccCheckScalewayInstanceIPExists("scaleway_instance_ip.scaleway"),
-					// Do not work anymore because of scaleway_instance_ip_reverse_dns new resource.
-					// Anyway the reverse attribute is deprecated.
-					//resource.TestCheckResourceAttr("scaleway_instance_ip.base", "reverse", "www.scaleway.com"),
-					//resource.TestCheckResourceAttr("scaleway_instance_ip.scaleway", "reverse", ""),
 				),
 			},
 		},
@@ -46,14 +36,20 @@ func TestAccScalewayInstanceIP_Zone(t *testing.T) {
 		CheckDestroy: testAccCheckScalewayInstanceIPDestroy,
 		Steps: []resource.TestStep{
 			{
-				Config: testAccScalewayInstanceIPZoneConfig[0],
+				Config: `
+					resource "scaleway_instance_ip" "base" {}
+				`,
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckScalewayInstanceIPExists("scaleway_instance_ip.base"),
 					resource.TestCheckResourceAttr("scaleway_instance_ip.base", "zone", "fr-par-1"),
 				),
 			},
 			{
-				Config: testAccScalewayInstanceIPZoneConfig[1],
+				Config: `
+					resource "scaleway_instance_ip" "base" {
+						zone = "nl-ams-1"	
+					}
+				`,
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckScalewayInstanceIPExists("scaleway_instance_ip.base"),
 					resource.TestCheckResourceAttr("scaleway_instance_ip.base", "zone", "nl-ams-1"),
@@ -63,40 +59,6 @@ func TestAccScalewayInstanceIP_Zone(t *testing.T) {
 	})
 }
 
-func TestAccScalewayInstanceServerIP(t *testing.T) {
-	t.Skip("Attaching an IP to a server via an IP block is deprecated")
-	resource.ParallelTest(t, resource.TestCase{
-		PreCheck:     func() { testAccPreCheck(t) },
-		Providers:    testAccProviders,
-		CheckDestroy: testAccCheckScalewayInstanceServerDestroy,
-		Steps: []resource.TestStep{
-			{
-				Config: testAccCheckScalewayInstanceServerConfigIP("base1"),
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckScalewayInstanceServerExists("scaleway_instance_server.base1"),
-					testAccCheckScalewayInstanceServerExists("scaleway_instance_server.base2"),
-					testAccCheckScalewayInstanceIPExists("scaleway_instance_ip.base_ip"),
-					testAccCheckScalewayInstanceIPPairWithServer("scaleway_instance_ip.base_ip", "scaleway_instance_server.base1"),
-				),
-			},
-			{
-				Config: testAccCheckScalewayInstanceServerConfigIP("base2"),
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckScalewayInstanceIPPairWithServer("scaleway_instance_ip.base_ip", "scaleway_instance_server.base2"),
-				),
-			},
-			{
-				Config: testAccCheckScalewayInstanceServerConfigIP(""),
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckScalewayInstanceServerNoIPAssigned("scaleway_instance_server.base1"),
-					testAccCheckScalewayInstanceServerNoIPAssigned("scaleway_instance_server.base2"),
-					resource.TestCheckResourceAttr("scaleway_instance_ip.base_ip", "server_id", ""),
-				),
-			},
-		},
-	})
-}
-
 func testAccCheckScalewayInstanceIPExists(n string) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		rs, ok := s.RootModule().Resources[n]
@@ -220,56 +182,3 @@ func testAccCheckScalewayInstanceIPDestroy(s *terraform.State) error {
 
 	return nil
 }
-
-// Check that reverse is handled at creation and update time
-var testAccScalewayInstanceIPConfig = []string{
-	`
-		resource "scaleway_instance_ip" "base" {}
-		resource "scaleway_instance_ip" "scaleway" {
-			reverse = "www.scaleway.com"
-		}
-	`,
-	`
-		resource "scaleway_instance_ip" "base" {
-			reverse = "www.scaleway.com"	
-		}
-		resource "scaleway_instance_ip" "scaleway" {}
-	`,
-}
-
-// Check that we can change the zone of an ip (delete + create)
-var testAccScalewayInstanceIPZoneConfig = []string{
-	`
-		resource "scaleway_instance_ip" "base" {}
-	`,
-	`
-		resource "scaleway_instance_ip" "base" {
-			zone = "nl-ams-1"	
-		}
-	`,
-}
-
-func testAccCheckScalewayInstanceServerConfigIP(attachedBase string) string {
-	attachedServer := ""
-	if attachedBase != "" {
-		attachedServer = `server_id = "${scaleway_instance_server.` + attachedBase + `.id}"`
-	}
-	return fmt.Sprintf(`
-resource "scaleway_instance_ip" "base_ip" {
-  %s
-}
-
-resource "scaleway_instance_server" "base1" {
-  image = "ubuntu-bionic"
-  type  = "DEV1-S"
-
-  tags  = [ "terraform-test", "scaleway_instance_server", "attach_ip" ]
-}
-
-resource "scaleway_instance_server" "base2" {
-  image = "ubuntu-bionic"
-  type  = "DEV1-S"
-
-  tags  = [ "terraform-test", "scaleway_instance_server", "attach_ip" ]
-}`, attachedServer)
-}

scaleway/resource_instance_server.go

@@ -151,6 +151,13 @@ func resourceScalewayInstanceServer() *schema.Resource {
 				Optional:    true,
 				Default:     false,
 				Description: "Disable dynamic IP on the server",
+				Removed:     "Removed in favor of enable_dynamic_ip",
+			},
+			"enable_dynamic_ip": {
+				Type:        schema.TypeBool,
+				Optional:    true,
+				Default:     false,
+				Description: "Enable dynamic IP on the server",
 			},
 			"state": {
 				Type:        schema.TypeString,
@@ -200,7 +207,7 @@ func resourceScalewayInstanceServer() *schema.Resource {
 				Type:     schema.TypeBool,
 				Optional: true,
 				Default:  false,
-				Removed:  "Please use disable_dynamic_ip instead",
+				Removed:  "Please use enable_dynamic_ip instead",
 			},
 		},
 	}
@@ -240,7 +247,7 @@ func resourceScalewayInstanceServerCreate(d *schema.ResourceData, m interface{})
 		CommercialType:    commercialType,
 		EnableIPv6:        d.Get("enable_ipv6").(bool),
 		SecurityGroup:     expandStringPtr(expandID(d.Get("security_group_id"))),
-		DynamicIPRequired: Bool(!d.Get("disable_dynamic_ip").(bool)),
+		DynamicIPRequired: scw.BoolPtr(d.Get("enable_dynamic_ip").(bool)),
 		Tags:              expandStrings(d.Get("tags")),
 	}
 
@@ -348,7 +355,7 @@ func resourceScalewayInstanceServerRead(d *schema.ResourceData, m interface{}) e
 	d.Set("tags", response.Server.Tags)
 	d.Set("security_group_id", response.Server.SecurityGroup.ID)
 	d.Set("enable_ipv6", response.Server.EnableIPv6)
-	d.Set("disable_dynamic_ip", !response.Server.DynamicIPRequired)
+	d.Set("enable_dynamic_ip", response.Server.DynamicIPRequired)
 
 	// Image could be empty in an import context.
 	image := d.Get("image").(string)
@@ -372,12 +379,15 @@ func resourceScalewayInstanceServerRead(d *schema.ResourceData, m interface{}) e
 			"type": "ssh",
 			"host": response.Server.PublicIP.Address.String(),
 		})
-		// Waiting for new breaking change version
-		// if response.Server.PublicIP.Dynamic == false {
-		// 	d.Set("ip_id", newZonedId(zone, response.Server.PublicIP.ID))
-		// } else {
-		// 	d.Set("ip_id", "")
-		// }
+		if response.Server.PublicIP.Dynamic == false {
+			d.Set("ip_id", newZonedId(zone, response.Server.PublicIP.ID))
+		} else {
+			d.Set("ip_id", "")
+		}
+	} else {
+		d.Set("public_ip", "")
+		d.Set("ip_id", "")
+		d.SetConnInfo(nil)
 	}
 
 	if response.Server.IPv6 != nil {
@@ -485,8 +495,8 @@ func resourceScalewayInstanceServerUpdate(d *schema.ResourceData, m interface{})
 		updateRequest.EnableIPv6 = scw.BoolPtr(d.Get("enable_ipv6").(bool))
 	}
 
-	if d.HasChange("disable_dynamic_ip") {
-		updateRequest.DynamicIPRequired = scw.BoolPtr(!d.Get("disable_dynamic_ip").(bool))
+	if d.HasChange("enable_dynamic_ip") {
+		updateRequest.DynamicIPRequired = scw.BoolPtr(d.Get("enable_dynamic_ip").(bool))
 	}
 
 	volumes := map[string]*instance.VolumeTemplate{}
@@ -533,7 +543,9 @@ func resourceScalewayInstanceServerUpdate(d *schema.ResourceData, m interface{})
 			return err
 		}
 		newIPID := expandID(d.Get("ip_id"))
-		if server.Server.PublicIP != nil {
+
+		// If an IP is already attached and it's not a dynamic IP we detach it.
+		if server.Server.PublicIP != nil && server.Server.PublicIP.Dynamic == false {
 			_, err = instanceAPI.DetachIP(&instance.DetachIPRequest{
 				Zone: zone,
 				IP:   server.Server.PublicIP.ID,