Repository: sacloud/terraform-provider-sakuracloud (go)

Issue:
user_ip_addressを修正すると予期せぬ再起動が発生する
related: https://github.com/sacloud/libsacloud/issues/551

以下のようにtfファイルを更新した上で`terraform apply`するとサーバの再起動が発生する。

Before:

```tf
resource "sakuracloud_switch" "sw" {
  name = "foobar"
}

data "sakuracloud_archive" "foobar" {
  os_type = "centos8"
}

resource "sakuracloud_disk" "foobar" {
  name              = "terraform-test"
  source_archive_id = data.sakuracloud_archive.foobar.id
}

resource "sakuracloud_server" "foobar" {
  name  = "foobar"
  disks = [sakuracloud_disk.foobar.id]

  network_interface {
    upstream = "shared"
  }
  network_interface {
    upstream        = sakuracloud_switch.sw.id
    user_ip_address = "192.168.0.1"
  }

  disk_edit_parameter {
    password = "password"
  }
}
```

After:

```tf
resource "sakuracloud_switch" "sw" {
  name = "foobar"
}

data "sakuracloud_archive" "foobar" {
  os_type = "centos8"
}

resource "sakuracloud_disk" "foobar" {
  name              = "terraform-test"
  source_archive_id = data.sakuracloud_archive.foobar.id
}

resource "sakuracloud_server" "foobar" {
  name  = "foobar"
  disks = [sakuracloud_disk.foobar.id]

  network_interface {
    upstream = "shared"
  }
  network_interface {
    upstream        = sakuracloud_switch.sw.id
    user_ip_address = "192.168.0.2" # **UPDATE**
  }

  disk_edit_parameter {
    password = "password"
  }
}

```



Pull request description:
Modify how to determine whether to pass disk_edit_parameter to ServerBuilder
closes #736 

sakuracloud_serverでのUpdate時に以下がtrueを返した場合にディスクの修正パラメータをlibsacloudのServerBuilderに渡していた。

```go
func isServerDiskConfigChanged(d *schema.ResourceData) bool {		
	return d.HasChanges("disks", "network_interface", "disk_edit_parameter")		
}
```

しかし、libsacloudのServerBuilderでは更新時にディスクの修正パラメータを受け取ると一旦シャットダウンする実装となっているため、呼び出し側で変更に応じて渡す/渡さないを判定する必要があった。

このため既存の実装では`network_interface.user_ip_address`や`network_interface.packet_filter_id`などの再起動が不要な項目の変更でも再起動されてしまっていた。
このPRでは上記の判定部分を修正し、~ディスクの修正パラメータ `disk_edit_parameter`が変更された場合にのみパラメータをServerBuilderに渡すようにする。~

** UPDATE** 以下の場合にパラメータをServerBuilderに渡すようにする。

- `disks`が変更された場合
- `network_interface`が追加/削除された場合
- `network_interface`の`upstream`が変更された場合
- `disk_edit_parameter`が変更された場合

この場合、`disk_edit_parameter`が削除された場合にもパラメータが渡されてしまうが、現実的ではないケースなため考慮しない。

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0299fd77..ae8ac42e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,9 @@
 ## 2.3.1 (Unreleased)
 
+FIXES
+
+* Modify how to determine whether to pass disk_edit_parameter to ServerBuilder [GH-737] (@yamamoto-febc)
+
 MISC
 
 * Fix broken CI - install golangci-lint via install script [GH-735] (@yamamoto-febc)
diff --git a/sakuracloud/resource_sakuracloud_server.go b/sakuracloud/resource_sakuracloud_server.go
index 7cfc2296..24741d28 100644
--- a/sakuracloud/resource_sakuracloud_server.go
+++ b/sakuracloud/resource_sakuracloud_server.go
@@ -439,7 +439,3 @@ func setServerResourceData(ctx context.Context, d *schema.ResourceData, client *
 	d.Set("zone", zone) // nolint
 	return d.Set("tags", flattenTags(data.Tags))
 }
-
-func isServerDiskConfigChanged(d *schema.ResourceData) bool {
-	return d.HasChanges("disks", "network_interface", "disk_edit_parameter")
-}
diff --git a/sakuracloud/structure.go b/sakuracloud/structure.go
index c2028fa5..4b6d7fd5 100644
--- a/sakuracloud/structure.go
+++ b/sakuracloud/structure.go
@@ -33,6 +33,11 @@ import (
 	"github.com/sacloud/libsacloud/v2/sacloud/types"
 )
 
+type resourceValueChangeHandler interface {
+	HasChanges(keys ...string) bool
+	GetChange(key string) (interface{}, interface{})
+}
+
 type resourceValueGettable interface {
 	Get(key string) interface{}
 	GetOk(key string) (interface{}, bool)
diff --git a/sakuracloud/structure_server.go b/sakuracloud/structure_server.go
index e97562d3..5c2080c8 100644
--- a/sakuracloud/structure_server.go
+++ b/sakuracloud/structure_server.go
@@ -56,7 +56,8 @@ func expandServerDisks(d *schema.ResourceData, client *APIClient) []diskBuilder.
 			ID:     diskID,
 			Client: diskBuilder.NewBuildersAPIClient(client),
 		}
-		if i == 0 && isServerDiskConfigChanged(d) {
+		// set only when value was changed
+		if i == 0 && isDiskEditParameterChanged(d) {
 			if diskEdit, ok := d.GetOk("disk_edit_parameter"); ok {
 				v := mapToResourceData(diskEdit.([]interface{})[0].(map[string]interface{}))
 				log.Printf("[INFO] disk_edit_parameter is specified for Disk[%s]", diskID)
@@ -196,3 +197,37 @@ func flattenServerNetworkInfo(server *sacloud.Server) (ip, gateway string, nwMas
 	}
 	return
 }
+
+func isDiskEditParameterChanged(d resourceValueChangeHandler) bool {
+	if d.HasChanges("network_interface") && isUpstreamChanged(d.GetChange("network_interface")) {
+		return true
+	}
+	return d.HasChanges("disks", "disk_edit_parameter")
+}
+
+func isUpstreamChanged(old, new interface{}) bool {
+	oldIsNil := old == nil
+	newIsNil := new == nil
+
+	if oldIsNil && newIsNil {
+		return false
+	}
+	if oldIsNil != newIsNil {
+		return true
+	}
+
+	oldNICs := old.([]interface{})
+	newNICs := new.([]interface{})
+	if len(oldNICs) != len(newNICs) {
+		return true
+	}
+
+	for i := range oldNICs {
+		oldUpstream := mapToResourceData(oldNICs[i].(map[string]interface{})).Get("upstream").(string)
+		newUpstream := mapToResourceData(newNICs[i].(map[string]interface{})).Get("upstream").(string)
+		if oldUpstream != newUpstream {
+			return true
+		}
+	}
+	return false
+}


Test patch:
diff --git a/sakuracloud/resource_sakuracloud_server_test.go b/sakuracloud/resource_sakuracloud_server_test.go
index 9b2e484f..ff23f2c3 100644
--- a/sakuracloud/resource_sakuracloud_server_test.go
+++ b/sakuracloud/resource_sakuracloud_server_test.go
@@ -96,6 +96,51 @@ func TestAccSakuraCloudServer_basic(t *testing.T) {
 	})
 }
 
+func TestAccSakuraCloudServer_withoutShutdown(t *testing.T) {
+	resourceName := "sakuracloud_server.foobar"
+	rand := randomName()
+	password := randomPassword()
+
+	var created, updated, editParamChanged sacloud.Server
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testCheckSakuraCloudServerDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: buildConfigWithArgs(testAccSakuraCloudServer_withoutShutdownWhenUpdate, rand, password),
+				Check: resource.ComposeTestCheckFunc(
+					testCheckSakuraCloudServerExists(resourceName, &created),
+				),
+			},
+			{
+				Config: buildConfigWithArgs(testAccSakuraCloudServer_updateWithoutShutdownWhenUpdate, rand, password),
+				Check: resource.ComposeTestCheckFunc(
+					testCheckSakuraCloudServerExists(resourceName, &updated),
+					func(state *terraform.State) error {
+						if created.InstanceStatusChangedAt != updated.InstanceStatusChangedAt {
+							return errors.New("unexpected shutdown has happened")
+						}
+						return nil
+					},
+				),
+			},
+			{
+				Config: buildConfigWithArgs(testAccSakuraCloudServer_updateWithShutdownWhenUpdate, rand, password),
+				Check: resource.ComposeTestCheckFunc(
+					testCheckSakuraCloudServerExists(resourceName, &editParamChanged),
+					func(state *terraform.State) error {
+						if updated.InstanceStatusChangedAt == editParamChanged.InstanceStatusChangedAt {
+							return errors.New("expected shutdown has not happened")
+						}
+						return nil
+					},
+				),
+			},
+		},
+	})
+}
+
 func TestAccSakuraCloudServer_interfaces(t *testing.T) {
 	resourceName := "sakuracloud_server.foobar"
 	rand := randomName()
@@ -582,3 +627,110 @@ resource "sakuracloud_server" "foobar" {
   }
 }
 `
+
+const testAccSakuraCloudServer_withoutShutdownWhenUpdate = `
+resource "sakuracloud_switch" "sw" {
+  name = "{{ .arg0 }}"
+}
+
+data "sakuracloud_archive" "ubuntu" {
+  os_type = "ubuntu"
+}
+resource "sakuracloud_disk" "foobar" {
+  name              = "{{ .arg0 }}"
+  source_archive_id = data.sakuracloud_archive.ubuntu.id
+}
+
+resource "sakuracloud_server" "foobar" {
+  name        = "{{ .arg0 }}"
+  disks       = [sakuracloud_disk.foobar.id]
+  description = "description"
+  tags        = ["tag1", "tag2"]
+
+  network_interface {
+    upstream = "shared"
+  }
+
+  network_interface {
+    upstream        = sakuracloud_switch.sw.id
+    user_ip_address = "192.168.0.11"
+  }
+
+  disk_edit_parameter {
+    hostname        = "{{ .arg0 }}"
+    password        = "{{ .arg1 }}"
+  }
+}
+`
+
+const testAccSakuraCloudServer_updateWithoutShutdownWhenUpdate = `
+resource "sakuracloud_switch" "sw" {
+  name = "{{ .arg0 }}"
+}
+
+data "sakuracloud_archive" "ubuntu" {
+  os_type = "ubuntu"
+}
+
+resource "sakuracloud_disk" "foobar" {
+  name              = "{{ .arg0 }}-upd"
+  source_archive_id = data.sakuracloud_archive.ubuntu.id
+}
+
+resource "sakuracloud_server" "foobar" {
+  name             = "{{ .arg0 }}-upd"
+  disks            = [sakuracloud_disk.foobar.id]
+  description      = "description-upd"
+  tags             = ["tag1-upd", "tag2-upd"]
+
+  network_interface {
+    upstream = "shared"
+  }
+
+  network_interface {
+    upstream        = sakuracloud_switch.sw.id
+    user_ip_address = "192.168.0.12"
+  }
+
+  disk_edit_parameter {
+    hostname        = "{{ .arg0 }}"
+    password        = "{{ .arg1 }}"
+  }
+}
+`
+
+const testAccSakuraCloudServer_updateWithShutdownWhenUpdate = `
+resource "sakuracloud_switch" "sw" {
+  name = "{{ .arg0 }}"
+}
+
+data "sakuracloud_archive" "ubuntu" {
+  os_type = "ubuntu"
+}
+
+resource "sakuracloud_disk" "foobar" {
+  name              = "{{ .arg0 }}-upd"
+  source_archive_id = data.sakuracloud_archive.ubuntu.id
+}
+
+resource "sakuracloud_server" "foobar" {
+  name             = "{{ .arg0 }}-upd"
+  disks            = [sakuracloud_disk.foobar.id]
+  description      = "description-upd"
+  tags             = ["tag1-upd", "tag2-upd"]
+
+  network_interface {
+    upstream = "shared"
+  }
+
+  network_interface {
+    upstream        = sakuracloud_switch.sw.id
+    user_ip_address = "192.168.0.12"
+  }
+
+  disk_edit_parameter {
+    hostname        = "{{ .arg0 }}"
+    password        = "{{ .arg1 }}-upd"
+  }
+}
+`
diff --git a/sakuracloud/structure_server_test.go b/sakuracloud/structure_server_test.go
new file mode 100644
index 00000000..5e02249a
--- /dev/null
+++ b/sakuracloud/structure_server_test.go
@@ -0,0 +1,346 @@
+// Copyright 2016-2020 terraform-provider-sakuracloud authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package sakuracloud
+
+import (
+	"reflect"
+	"testing"
+)
+
+type dummyResourceValueChangeHandler struct {
+	oldState resourceValueGettable
+	newState resourceValueGettable
+}
+
+func (d *dummyResourceValueChangeHandler) HasChanges(keys ...string) bool {
+	for _, key := range keys {
+		old := d.oldState.Get(key)
+		new := d.newState.Get(key)
+		if !reflect.DeepEqual(old, new) {
+			return true
+		}
+	}
+	return false
+}
+
+func (d *dummyResourceValueChangeHandler) GetChange(key string) (interface{}, interface{}) {
+	return d.oldState.Get(key), d.newState.Get(key)
+}
+
+func TestStructureServer_isDiskEditParameterChanged(t *testing.T) {
+	cases := []struct {
+		msg    string
+		in     *dummyResourceValueChangeHandler
+		expect bool
+	}{
+		{
+			msg: "nil",
+			in: &dummyResourceValueChangeHandler{
+				oldState: &resourceMapValue{},
+				newState: &resourceMapValue{},
+			},
+			expect: false,
+		},
+		{
+			msg: "added: disks",
+			in: &dummyResourceValueChangeHandler{
+				oldState: &resourceMapValue{},
+				newState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disks": []interface{}{"1"},
+					},
+				},
+			},
+			expect: true,
+		},
+		{
+			msg: "added: disk_edit_parameter",
+			in: &dummyResourceValueChangeHandler{
+				oldState: &resourceMapValue{},
+				newState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{},
+						},
+					},
+				},
+			},
+			expect: true,
+		},
+		{
+			msg: "added: network_interface",
+			in: &dummyResourceValueChangeHandler{
+				oldState: &resourceMapValue{},
+				newState: &resourceMapValue{
+					value: map[string]interface{}{
+						"network_interface": []interface{}{
+							map[string]interface{}{},
+						},
+					},
+				},
+			},
+			expect: true,
+		},
+		{
+			msg: "updated: no changes",
+			in: &dummyResourceValueChangeHandler{
+				oldState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disks": []interface{}{"1"},
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{
+								"password": "password",
+							},
+						},
+						"network_interface": []interface{}{
+							map[string]interface{}{
+								"upstream": "shared",
+							},
+						},
+					},
+				},
+				newState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disks": []interface{}{"1"},
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{
+								"password": "password",
+							},
+						},
+						"network_interface": []interface{}{
+							map[string]interface{}{
+								"upstream": "shared",
+							},
+						},
+					},
+				},
+			},
+			expect: false,
+		},
+		{
+			msg: "updated: disks",
+			in: &dummyResourceValueChangeHandler{
+				oldState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disks": []interface{}{"1"},
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{
+								"password": "password",
+							},
+						},
+						"network_interface": []interface{}{
+							map[string]interface{}{
+								"upstream": "shared",
+							},
+						},
+					},
+				},
+				newState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disks": []interface{}{"2"},
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{
+								"password": "password",
+							},
+						},
+						"network_interface": []interface{}{
+							map[string]interface{}{
+								"upstream": "shared",
+							},
+						},
+					},
+				},
+			},
+			expect: true,
+		},
+		{
+			msg: "updated: disk_edit_parameter",
+			in: &dummyResourceValueChangeHandler{
+				oldState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disks": []interface{}{"1"},
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{
+								"password": "password",
+							},
+						},
+						"network_interface": []interface{}{
+							map[string]interface{}{
+								"upstream": "shared",
+							},
+						},
+					},
+				},
+				newState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disks": []interface{}{"1"},
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{
+								"password": "password-upd",
+							},
+						},
+						"network_interface": []interface{}{
+							map[string]interface{}{
+								"upstream": "shared",
+							},
+						},
+					},
+				},
+			},
+			expect: true,
+		},
+		{
+			msg: "updated: network_interface.upstream",
+			in: &dummyResourceValueChangeHandler{
+				oldState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disks": []interface{}{"1"},
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{
+								"password": "password",
+							},
+						},
+						"network_interface": []interface{}{
+							map[string]interface{}{
+								"upstream": "1",
+							},
+						},
+					},
+				},
+				newState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disks": []interface{}{"1"},
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{
+								"password": "password",
+							},
+						},
+						"network_interface": []interface{}{
+							map[string]interface{}{
+								"upstream": "2",
+							},
+						},
+					},
+				},
+			},
+			expect: true,
+		},
+		{
+			msg: "updated: network_interface.other",
+			in: &dummyResourceValueChangeHandler{
+				oldState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disks": []interface{}{"1"},
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{
+								"password": "password",
+							},
+						},
+						"network_interface": []interface{}{
+							map[string]interface{}{
+								"upstream":         "1",
+								"user_ip_address":  "192.168.0.1",
+								"packet_filter_id": "1",
+							},
+						},
+					},
+				},
+				newState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disks": []interface{}{"1"},
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{
+								"password": "password",
+							},
+						},
+						"network_interface": []interface{}{
+							map[string]interface{}{
+								"upstream":         "1",
+								"user_ip_address":  "192.168.0.2",
+								"packet_filter_id": "2",
+							},
+						},
+					},
+				},
+			},
+			expect: false,
+		},
+		{
+			msg: "deleted: disks",
+			in: &dummyResourceValueChangeHandler{
+				oldState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disks": []interface{}{"1"},
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{},
+						},
+						"network_interface": []interface{}{
+							map[string]interface{}{
+								"upstream": "shared",
+							},
+						},
+					},
+				},
+				newState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{},
+						},
+						"network_interface": []interface{}{
+							map[string]interface{}{
+								"upstream": "shared",
+							},
+						},
+					},
+				},
+			},
+			expect: true,
+		},
+		{
+			msg: "deleted: network_interface",
+			in: &dummyResourceValueChangeHandler{
+				oldState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disks": []interface{}{"1"},
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{},
+						},
+						"network_interface": []interface{}{
+							map[string]interface{}{
+								"upstream": "shared",
+							},
+						},
+					},
+				},
+				newState: &resourceMapValue{
+					value: map[string]interface{}{
+						"disks": []interface{}{"1"},
+						"disk_edit_parameter": []interface{}{
+							map[string]interface{}{},
+						},
+					},
+				},
+			},
+			expect: true,
+		},
+	}
+
+	for _, tc := range cases {
+		got := isDiskEditParameterChanged(tc.in)
+		if got != tc.expect {
+			t.Fatalf("got unexpected state: pattern: %s expected: %t actual: %t", tc.msg, tc.expect, got)
+		}
+	}
+}