Skip to content

Commit

Permalink
refactor!: change the way an ip is attached to a server (#365)
Browse files Browse the repository at this point in the history 
BREAKING CHANGE: 

* `scaleway_instance_server`: The attribute `disable_dynamic_ip` has been removed in favor of `enable_dynamic_ip`.
* `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`.
  • Loading branch information
@jerome-quere
jerome-quere authored Dec 10, 2019
1 parent 35dddff commit 1e895d3
Show file tree
Hide file tree
Showing 10 changed files with 117 additions and 251 deletions.
6 changes: 5 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
98 changes: 9 additions & 89 deletions scaleway/resource_instance_ip.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand All @@ -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(),
Expand All @@ -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)
}

Expand Down Expand Up @@ -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 {
Expand Down
115 changes: 12 additions & 103 deletions scaleway/resource_instance_ip_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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", ""),
),
},
},
Expand All @@ -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"),
Expand All @@ -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]
Expand Down Expand Up @@ -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)
}
36 changes: 24 additions & 12 deletions scaleway/resource_instance_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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",
},
},
}
Expand Down Expand Up @@ -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")),
}

Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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{}
Expand Down Expand Up @@ -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,
Expand Down
Loading

0 comments on commit 1e895d3

Please sign in to comment.