Skip to content

Commit 4725767

Browse files
modular-magicianmodular-magician
authored
Revert "Revert "Fix permadiff that reorders stateful_external_ip blocks on google_compute_instance_group_manager and google_compute_region_instance_group_manager resources"" (#9758) (#16910)
* Revert "Revert "Fix permadiff that reorders `stateful_external_ip` blocks on `google_compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources" (#9754)" This reverts commit 24ff156. * Sort unexpected IPs not present in TF config, update unit test [upstream:fabeca9e96b1c3d4e9536d78d57e9e8eecdfb92d] Signed-off-by: Modular Magician <[email protected]>
1 parent 7992618 commit 4725767

5 files changed

+402
-21
lines changed

‎.changelog/9758.txt

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
```release-note:bug
2+
compute: fixed a permadiff that reordered `stateful_external_ip` and `stateful_internal_ip` blocks on `google_compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources
3+
```

‎google/services/compute/resource_compute_instance_group_manager.go

+55-19
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"fmt"
77
"log"
88
"regexp"
9+
"sort"
910
"strings"
1011
"time"
1112

@@ -718,10 +719,10 @@ func resourceComputeInstanceGroupManagerRead(d *schema.ResourceData, meta interf
718719
if err = d.Set("stateful_disk", flattenStatefulPolicy(manager.StatefulPolicy)); err != nil {
719720
return fmt.Errorf("Error setting stateful_disk in state: %s", err.Error())
720721
}
721-
if err = d.Set("stateful_internal_ip", flattenStatefulPolicyStatefulInternalIps(manager.StatefulPolicy)); err != nil {
722+
if err = d.Set("stateful_internal_ip", flattenStatefulPolicyStatefulInternalIps(d, manager.StatefulPolicy)); err != nil {
722723
return fmt.Errorf("Error setting stateful_internal_ip in state: %s", err.Error())
723724
}
724-
if err = d.Set("stateful_external_ip", flattenStatefulPolicyStatefulExternalIps(manager.StatefulPolicy)); err != nil {
725+
if err = d.Set("stateful_external_ip", flattenStatefulPolicyStatefulExternalIps(d, manager.StatefulPolicy)); err != nil {
725726
return fmt.Errorf("Error setting stateful_external_ip in state: %s", err.Error())
726727
}
727728
if err := d.Set("fingerprint", manager.Fingerprint); err != nil {
@@ -1212,36 +1213,71 @@ func flattenStatefulPolicy(statefulPolicy *compute.StatefulPolicy) []map[string]
12121213
return result
12131214
}
12141215

1215-
func flattenStatefulPolicyStatefulInternalIps(statefulPolicy *compute.StatefulPolicy) []map[string]interface{} {
1216+
func flattenStatefulPolicyStatefulInternalIps(d *schema.ResourceData, statefulPolicy *compute.StatefulPolicy) []map[string]interface{} {
12161217
if statefulPolicy == nil || statefulPolicy.PreservedState == nil || statefulPolicy.PreservedState.InternalIPs == nil {
12171218
return make([]map[string]interface{}, 0, 0)
12181219
}
1219-
result := make([]map[string]interface{}, 0, len(statefulPolicy.PreservedState.InternalIPs))
1220-
for interfaceName, internalIp := range statefulPolicy.PreservedState.InternalIPs {
1221-
data := map[string]interface{}{
1222-
"interface_name": interfaceName,
1223-
"delete_rule": internalIp.AutoDelete,
1224-
}
12251220

1226-
result = append(result, data)
1227-
}
1228-
return result
1221+
return flattenStatefulPolicyStatefulIps(d, "stateful_internal_ip", statefulPolicy.PreservedState.InternalIPs)
12291222
}
12301223

1231-
func flattenStatefulPolicyStatefulExternalIps(statefulPolicy *compute.StatefulPolicy) []map[string]interface{} {
1224+
func flattenStatefulPolicyStatefulExternalIps(d *schema.ResourceData, statefulPolicy *compute.StatefulPolicy) []map[string]interface{} {
12321225
if statefulPolicy == nil || statefulPolicy.PreservedState == nil || statefulPolicy.PreservedState.ExternalIPs == nil {
1233-
return make([]map[string]interface{}, 0, 0)
1226+
return make([]map[string]interface{}, 0)
12341227
}
1235-
result := make([]map[string]interface{}, 0, len(statefulPolicy.PreservedState.ExternalIPs))
1236-
for interfaceName, externalIp := range statefulPolicy.PreservedState.ExternalIPs {
1228+
1229+
return flattenStatefulPolicyStatefulIps(d, "stateful_external_ip", statefulPolicy.PreservedState.ExternalIPs)
1230+
}
1231+
1232+
func flattenStatefulPolicyStatefulIps(d *schema.ResourceData, ipfieldName string, ips map[string]compute.StatefulPolicyPreservedStateNetworkIp) []map[string]interface{} {
1233+
1234+
// statefulPolicy.PreservedState.ExternalIPs and statefulPolicy.PreservedState.InternalIPs are affected by API-side reordering
1235+
// of external/internal IPs, where ordering is done by the interface_name value.
1236+
// Below we intend to reorder the IPs to match the order in the config.
1237+
// Also, data is converted from a map (client library's statefulPolicy.PreservedState.ExternalIPs, or .InternalIPs) to a slice (stored in state).
1238+
// Any IPs found from the API response that aren't in the config are appended to the end of the slice.
1239+
1240+
configIpOrder := d.Get(ipfieldName).([]interface{})
1241+
order := map[string]int{} // record map of interface name to index
1242+
for i, el := range configIpOrder {
1243+
ip := el.(map[string]interface{})
1244+
interfaceName := ip["interface_name"].(string)
1245+
order[interfaceName] = i
1246+
}
1247+
1248+
orderedResult := make([]map[string]interface{}, len(configIpOrder))
1249+
unexpectedIps := []map[string]interface{}{}
1250+
for interfaceName, ip := range ips {
12371251
data := map[string]interface{}{
12381252
"interface_name": interfaceName,
1239-
"delete_rule": externalIp.AutoDelete,
1253+
"delete_rule": ip.AutoDelete,
12401254
}
12411255

1242-
result = append(result, data)
1256+
index, found := order[interfaceName]
1257+
if !found {
1258+
unexpectedIps = append(unexpectedIps, data)
1259+
continue
1260+
}
1261+
orderedResult[index] = data // Put elements from API response in order that matches the config
12431262
}
1244-
return result
1263+
sort.Slice(unexpectedIps, func(i, j int) bool {
1264+
return unexpectedIps[i]["interface_name"].(string) < unexpectedIps[j]["interface_name"].(string)
1265+
})
1266+
1267+
// Remove any nils from the ordered list. This can occur if the API doesn't include an interface present in the config.
1268+
finalResult := []map[string]interface{}{}
1269+
for _, item := range orderedResult {
1270+
if item != nil {
1271+
finalResult = append(finalResult, item)
1272+
}
1273+
}
1274+
1275+
if len(unexpectedIps) > 0 {
1276+
// Additional IPs returned from API but not in the config are appended to the end of the slice
1277+
finalResult = append(finalResult, unexpectedIps...)
1278+
}
1279+
1280+
return finalResult
12451281
}
12461282

12471283
func flattenUpdatePolicy(updatePolicy *compute.InstanceGroupManagerUpdatePolicy) []map[string]interface{} {

‎google/services/compute/resource_compute_instance_group_manager_internal_test.go

+236
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,12 @@
33
package compute
44

55
import (
6+
"reflect"
67
"testing"
8+
9+
"github.com/hashicorp/terraform-provider-google/google/tpgresource"
10+
11+
"google.golang.org/api/compute/v1"
712
)
813

914
func TestInstanceGroupManager_parseUniqueId(t *testing.T) {
@@ -78,3 +83,234 @@ func TestInstanceGroupManager_convertUniqueId(t *testing.T) {
7883
}
7984
}
8085
}
86+
87+
func TestFlattenStatefulPolicyStatefulIps(t *testing.T) {
88+
cases := map[string]struct {
89+
ConfigValues []interface{}
90+
Ips map[string]compute.StatefulPolicyPreservedStateNetworkIp
91+
Expected []map[string]interface{}
92+
}{
93+
"No IPs in config nor API data": {
94+
ConfigValues: []interface{}{},
95+
Ips: map[string]compute.StatefulPolicyPreservedStateNetworkIp{},
96+
Expected: []map[string]interface{}{},
97+
},
98+
"Single IP (nic0) in config and API data": {
99+
ConfigValues: []interface{}{
100+
map[string]interface{}{
101+
"interface_name": "nic0",
102+
"delete_rule": "NEVER",
103+
},
104+
},
105+
Ips: map[string]compute.StatefulPolicyPreservedStateNetworkIp{
106+
"nic0": {
107+
AutoDelete: "NEVER",
108+
},
109+
},
110+
Expected: []map[string]interface{}{
111+
{
112+
"interface_name": "nic0",
113+
"delete_rule": "NEVER",
114+
},
115+
},
116+
},
117+
"Two IPs (nic0, nic1). Unordered in config and sorted in API data": {
118+
ConfigValues: []interface{}{
119+
map[string]interface{}{
120+
"interface_name": "nic1",
121+
"delete_rule": "NEVER",
122+
},
123+
map[string]interface{}{
124+
"interface_name": "nic0",
125+
"delete_rule": "NEVER",
126+
},
127+
},
128+
Ips: map[string]compute.StatefulPolicyPreservedStateNetworkIp{
129+
"nic0": {
130+
AutoDelete: "NEVER",
131+
},
132+
"nic1": {
133+
AutoDelete: "NEVER",
134+
},
135+
},
136+
Expected: []map[string]interface{}{
137+
{
138+
"interface_name": "nic1",
139+
"delete_rule": "NEVER",
140+
},
141+
{
142+
"interface_name": "nic0",
143+
"delete_rule": "NEVER",
144+
},
145+
},
146+
},
147+
"Two IPs (nic0, nic1). Only nic0 in config and both stored in API data": {
148+
ConfigValues: []interface{}{
149+
map[string]interface{}{
150+
"interface_name": "nic0",
151+
"delete_rule": "NEVER",
152+
},
153+
},
154+
Ips: map[string]compute.StatefulPolicyPreservedStateNetworkIp{
155+
"nic0": {
156+
AutoDelete: "NEVER",
157+
},
158+
"nic1": {
159+
AutoDelete: "NEVER",
160+
},
161+
},
162+
Expected: []map[string]interface{}{
163+
{
164+
"interface_name": "nic0",
165+
"delete_rule": "NEVER",
166+
},
167+
{
168+
"interface_name": "nic1",
169+
"delete_rule": "NEVER",
170+
},
171+
},
172+
},
173+
"Five IPs (nic0 - nic4). None stored in config and all stored in API data": {
174+
ConfigValues: []interface{}{},
175+
Ips: map[string]compute.StatefulPolicyPreservedStateNetworkIp{
176+
// Out of order here to encourage randomness
177+
"nic3": {
178+
AutoDelete: "NEVER",
179+
},
180+
"nic0": {
181+
AutoDelete: "NEVER",
182+
},
183+
"nic1": {
184+
AutoDelete: "NEVER",
185+
},
186+
"nic4": {
187+
AutoDelete: "NEVER",
188+
},
189+
"nic2": {
190+
AutoDelete: "NEVER",
191+
},
192+
},
193+
Expected: []map[string]interface{}{
194+
{
195+
"interface_name": "nic0",
196+
"delete_rule": "NEVER",
197+
},
198+
{
199+
"interface_name": "nic1",
200+
"delete_rule": "NEVER",
201+
},
202+
{
203+
"interface_name": "nic2",
204+
"delete_rule": "NEVER",
205+
},
206+
{
207+
"interface_name": "nic3",
208+
"delete_rule": "NEVER",
209+
},
210+
{
211+
"interface_name": "nic4",
212+
"delete_rule": "NEVER",
213+
},
214+
},
215+
},
216+
"Three IPs (nic0, nic1, nic2). Only nic1, nic2 in config and all 3 stored in API data": {
217+
ConfigValues: []interface{}{
218+
map[string]interface{}{
219+
"interface_name": "nic1",
220+
"delete_rule": "NEVER",
221+
},
222+
map[string]interface{}{
223+
"interface_name": "nic2",
224+
"delete_rule": "NEVER",
225+
},
226+
},
227+
Ips: map[string]compute.StatefulPolicyPreservedStateNetworkIp{
228+
"nic0": {
229+
AutoDelete: "NEVER",
230+
},
231+
"nic1": {
232+
AutoDelete: "NEVER",
233+
},
234+
"nic2": {
235+
AutoDelete: "NEVER",
236+
},
237+
},
238+
Expected: []map[string]interface{}{
239+
{
240+
"interface_name": "nic1",
241+
"delete_rule": "NEVER",
242+
},
243+
{
244+
"interface_name": "nic2",
245+
"delete_rule": "NEVER",
246+
},
247+
{
248+
"interface_name": "nic0",
249+
"delete_rule": "NEVER",
250+
},
251+
},
252+
},
253+
"Three IPs (nic0, nic1, nic2). Only nic0, nic2 in config and only nic1, nic2 stored in API data": {
254+
ConfigValues: []interface{}{
255+
map[string]interface{}{
256+
"interface_name": "nic2",
257+
"delete_rule": "NEVER",
258+
},
259+
map[string]interface{}{
260+
"interface_name": "nic0",
261+
"delete_rule": "NEVER",
262+
},
263+
},
264+
Ips: map[string]compute.StatefulPolicyPreservedStateNetworkIp{
265+
"nic1": {
266+
AutoDelete: "NEVER",
267+
},
268+
"nic2": {
269+
AutoDelete: "NEVER",
270+
},
271+
},
272+
Expected: []map[string]interface{}{
273+
{
274+
"interface_name": "nic2",
275+
"delete_rule": "NEVER",
276+
},
277+
{
278+
"interface_name": "nic1",
279+
"delete_rule": "NEVER",
280+
},
281+
},
282+
},
283+
}
284+
285+
for tn, tc := range cases {
286+
t.Run(tn, func(t *testing.T) {
287+
288+
// Terraform config
289+
schema := ResourceComputeRegionInstanceGroupManager().Schema
290+
config := map[string]interface{}{
291+
"stateful_external_ip": tc.ConfigValues,
292+
"stateful_internal_ip": tc.ConfigValues,
293+
}
294+
d := tpgresource.SetupTestResourceDataFromConfigMap(t, schema, config)
295+
296+
// API response
297+
statefulPolicyPreservedState := compute.StatefulPolicyPreservedState{
298+
ExternalIPs: tc.Ips,
299+
InternalIPs: tc.Ips,
300+
}
301+
statefulPolicy := compute.StatefulPolicy{
302+
PreservedState: &statefulPolicyPreservedState,
303+
}
304+
305+
outputExternal := flattenStatefulPolicyStatefulExternalIps(d, &statefulPolicy)
306+
if !reflect.DeepEqual(tc.Expected, outputExternal) {
307+
t.Fatalf("expected external IPs output to be %#v, but got %#v", tc.Expected, outputExternal)
308+
}
309+
310+
outputInternal := flattenStatefulPolicyStatefulInternalIps(d, &statefulPolicy)
311+
if !reflect.DeepEqual(tc.Expected, outputInternal) {
312+
t.Fatalf("expected internal IPs output to be %#v, but got %#v", tc.Expected, outputInternal)
313+
}
314+
})
315+
}
316+
}

‎google/services/compute/resource_compute_region_instance_group_manager.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -692,10 +692,10 @@ func resourceComputeRegionInstanceGroupManagerRead(d *schema.ResourceData, meta
692692
if err = d.Set("status", flattenStatus(manager.Status)); err != nil {
693693
return fmt.Errorf("Error setting status in state: %s", err.Error())
694694
}
695-
if err = d.Set("stateful_internal_ip", flattenStatefulPolicyStatefulInternalIps(manager.StatefulPolicy)); err != nil {
695+
if err = d.Set("stateful_internal_ip", flattenStatefulPolicyStatefulInternalIps(d, manager.StatefulPolicy)); err != nil {
696696
return fmt.Errorf("Error setting stateful_internal_ip in state: %s", err.Error())
697697
}
698-
if err = d.Set("stateful_external_ip", flattenStatefulPolicyStatefulExternalIps(manager.StatefulPolicy)); err != nil {
698+
if err = d.Set("stateful_external_ip", flattenStatefulPolicyStatefulExternalIps(d, manager.StatefulPolicy)); err != nil {
699699
return fmt.Errorf("Error setting stateful_external_ip in state: %s", err.Error())
700700
}
701701
// If unset in state set to default value

0 commit comments

Comments
 (0)