Skip to content

Commit 7af9fa6

Browse files
sudomateosudomateo
authored
resource(oxide_instance): fix inconsistent result after apply (#460)
Updated the `oxide_instance` resource to fix the issue documented in #459 where the provider would produce an inconsistent result after apply. The changes here do the following. 1. Use the ephemeral external IP pool ID from the plan since the upstream Oxide API does not provide that. 1. Use the `.IpPoolId` field for floating external IPs rather than the incorrect `.Id` field. Creating an acceptance test for this proved to be difficult since it required either using an existing IP pool for the external IP or creating and linking an IP pool on the fly. I opted to assume an IP pool named `default` exists in the silo and allowing the user to override that IP pool name via the `OXIDE_TEST_IP_POOL_NAME` environment variable. ``` > TEST_ACC_NAME=TestAccCloudResourceInstance_extIPs OXIDE_TEST_IP_POOL_NAME=private make testacc -> Running terraform acceptance tests === RUN TestAccCloudResourceInstance_extIPs === PAUSE TestAccCloudResourceInstance_extIPs === CONT TestAccCloudResourceInstance_extIPs --- PASS: TestAccCloudResourceInstance_extIPs (59.90s) PASS ok github.com/oxidecomputer/terraform-provider-oxide/internal/provider 59.912s ```
1 parent f9b42d1 commit 7af9fa6

File tree

4 files changed

+138
-20
lines changed

4 files changed

+138
-20
lines changed

‎.changelog/0.12.0.toml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,5 +11,5 @@ title = ""
1111
description = ""
1212

1313
[[bugs]]
14-
title = ""
15-
description = ""
14+
title = "`oxide_instance`"
15+
description = "Fixed the `inconsistent result after apply` error when applying a subsequent plan where the `external_ips` attribute contained an ephemeral IP with a non-empty ID. [#460](https://github.com/oxidecomputer/terraform-provider-oxide/pull/460)"

‎.github/workflows/acceptance.yml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,5 @@ jobs:
1818
env:
1919
OXIDE_TOKEN: ${{ secrets.COLO_OXIDE_TOKEN }}
2020
OXIDE_HOST: ${{ secrets.COLO_OXIDE_HOST }}
21-
TEST_ACC_NAME: TestAccCloud
21+
OXIDE_TEST_IP_POOL_NAME: private
22+
TEST_ACC_NAME: TestAccCloud

‎internal/provider/resource_instance.go

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -527,7 +527,7 @@ func (r *instanceResource) Read(ctx context.Context, req resource.ReadRequest, r
527527
state.TimeCreated = types.StringValue(instance.TimeCreated.String())
528528
state.TimeModified = types.StringValue(instance.TimeModified.String())
529529

530-
externalIPs, diags := newAttachedExternalIPModel(ctx, r.client, state.ID.ValueString())
530+
externalIPs, diags := newAttachedExternalIPModel(ctx, r.client, state)
531531
resp.Diagnostics.Append(diags...)
532532
if resp.Diagnostics.HasError() {
533533
return
@@ -774,7 +774,9 @@ func (r *instanceResource) Update(ctx context.Context, req resource.UpdateReques
774774
plan.TimeCreated = types.StringValue(instance.TimeCreated.String())
775775
plan.TimeModified = types.StringValue(instance.TimeModified.String())
776776

777-
externalIPs, diags := newAttachedExternalIPModel(ctx, r.client, state.ID.ValueString())
777+
// We use the plan here instead of the state to capture the desired IP pool ID
778+
// value for the ephemeral external IP rather than the previous value.
779+
externalIPs, diags := newAttachedExternalIPModel(ctx, r.client, plan)
778780
resp.Diagnostics.Append(diags...)
779781
if resp.Diagnostics.HasError() {
780782
return
@@ -1075,13 +1077,32 @@ func newAttachedNetworkInterfacesModel(ctx context.Context, client *oxide.Client
10751077
return nicSet, nil
10761078
}
10771079

1078-
func newAttachedExternalIPModel(ctx context.Context, client *oxide.Client, instanceID string) (
1080+
// newAttachedExternalIPModel fetches the external IP addresses for the instance
1081+
// specified by model, keeping the IP pool ID from the ephemeral external IP
1082+
// from the model if one is present.
1083+
func newAttachedExternalIPModel(ctx context.Context, client *oxide.Client, model instanceResourceModel) (
10791084
[]instanceResourceExternalIPModel, diag.Diagnostics) {
10801085
var diags diag.Diagnostics
10811086
externalIPs := make([]instanceResourceExternalIPModel, 0)
10821087

1088+
// The [oxide.Client.InstanceExternalIpList] method does not return the IP pool
1089+
// ID for ephemeral external IPs. See https://github.com/oxidecomputer/omicron/issues/6825.
1090+
//
1091+
// Pull the IP pool ID out of the model to populate the external IP model that
1092+
// we're building to prevent erroneous Terraform diffs (e.g., state contains
1093+
// IP pool ID and refresh doesn't). It's safe to stop at the first ephemeral
1094+
// external IP encountered since the configuration enforces at most one
1095+
// ephemeral external IP.
1096+
ephemeralIPPoolID := ""
1097+
for _, externalIP := range model.ExternalIPs {
1098+
if externalIP.Type.ValueString() == string(oxide.ExternalIpCreateTypeEphemeral) {
1099+
ephemeralIPPoolID = externalIP.ID.ValueString()
1100+
break
1101+
}
1102+
}
1103+
10831104
externalIPResponse, err := client.InstanceExternalIpList(ctx, oxide.InstanceExternalIpListParams{
1084-
Instance: oxide.NameOrId(instanceID),
1105+
Instance: oxide.NameOrId(model.ID.ValueString()),
10851106
})
10861107
if err != nil {
10871108
diags.AddError(
@@ -1092,10 +1113,23 @@ func newAttachedExternalIPModel(ctx context.Context, client *oxide.Client, insta
10921113
}
10931114

10941115
for _, externalIP := range externalIPResponse.Items {
1095-
externalIPs = append(externalIPs, instanceResourceExternalIPModel{
1096-
ID: types.StringValue(externalIP.Id),
1097-
Type: types.StringValue(string(externalIP.Kind)),
1098-
})
1116+
switch externalIP.Kind {
1117+
case oxide.ExternalIpKindEphemeral:
1118+
externalIPs = append(externalIPs, instanceResourceExternalIPModel{
1119+
ID: types.StringValue(ephemeralIPPoolID),
1120+
Type: types.StringValue(string(externalIP.Kind)),
1121+
})
1122+
case oxide.ExternalIpKindFloating:
1123+
externalIPs = append(externalIPs, instanceResourceExternalIPModel{
1124+
ID: types.StringValue(externalIP.IpPoolId),
1125+
Type: types.StringValue(string(externalIP.Kind)),
1126+
})
1127+
default:
1128+
diags.AddError(
1129+
"Invalid external IP kind:",
1130+
fmt.Sprintf("Encountered unexpected external IP kind: %s", externalIP.Kind),
1131+
)
1132+
}
10991133
}
11001134

11011135
return externalIPs, nil

‎internal/provider/resource_instance_test.go

Lines changed: 92 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package provider
77
import (
88
"context"
99
"fmt"
10+
"os"
1011
"reflect"
1112
"testing"
1213
"time"
@@ -196,18 +197,32 @@ resource "oxide_instance" "{{.BlockName}}" {
196197
})
197198
}
198199

200+
// TestAccCloudResourceInstance_extIPs tests whether Terraform can create
201+
// `oxide_instance` resources with the `external_ips` attribute populated. It
202+
// assumes an IP pool named `default` already exists in the silo the test is
203+
// running against. The `OXIDE_TEST_IP_POOL_NAME` environment variable can be
204+
// used to override the IP pool if necessary.
205+
//
206+
// This test is also meant to catch regressions of the following issue.
207+
// https://github.com/oxidecomputer/terraform-provider-oxide/issues/459.
199208
func TestAccCloudResourceInstance_extIPs(t *testing.T) {
200209
type resourceInstanceConfig struct {
201210
BlockName string
202211
InstanceName string
203212
SupportBlockName string
213+
IPPoolBlockName string
214+
IPPoolName string
204215
}
205216

206217
resourceInstanceExternalIPConfigTpl := `
207218
data "oxide_project" "{{.SupportBlockName}}" {
208219
name = "tf-acc-test"
209220
}
210221
222+
data "oxide_ip_pool" "{{.IPPoolBlockName}}" {
223+
name = "{{.IPPoolName}}"
224+
}
225+
211226
resource "oxide_instance" "{{.BlockName}}" {
212227
project_id = data.oxide_project.{{.SupportBlockName}}.id
213228
description = "a test instance"
@@ -219,12 +234,13 @@ resource "oxide_instance" "{{.BlockName}}" {
219234
external_ips = [
220235
{
221236
type = "ephemeral"
237+
id = data.oxide_ip_pool.{{.IPPoolBlockName}}.id
222238
}
223239
]
224240
}
225241
`
226242

227-
resourceInstanceExternalIPConfigUpdateTpl := `
243+
resourceInstanceExternalIPConfigUpdate1Tpl := `
228244
data "oxide_project" "{{.SupportBlockName}}" {
229245
name = "tf-acc-test"
230246
}
@@ -237,56 +253,105 @@ resource "oxide_instance" "{{.BlockName}}" {
237253
memory = 1073741824
238254
ncpus = 1
239255
start_on_create = false
256+
external_ips = [
257+
{
258+
type = "ephemeral"
259+
}
260+
]
240261
}
241262
`
242263

264+
resourceInstanceExternalIPConfigUpdate2Tpl := `
265+
data "oxide_project" "{{.SupportBlockName}}" {
266+
name = "tf-acc-test"
267+
}
268+
269+
resource "oxide_instance" "{{.BlockName}}" {
270+
project_id = data.oxide_project.{{.SupportBlockName}}.id
271+
description = "a test instance"
272+
name = "{{.InstanceName}}"
273+
host_name = "terraform-acc-myhost"
274+
memory = 1073741824
275+
ncpus = 1
276+
start_on_create = false
277+
}
278+
`
279+
280+
// Some test environments may not have an IP pool named `default`. This allows a
281+
// user to override the IP pool name used for this test.
282+
ipPoolName, ok := os.LookupEnv("OXIDE_TEST_IP_POOL_NAME")
283+
if !ok || ipPoolName == "" {
284+
ipPoolName = "default"
285+
}
286+
243287
instanceName := newResourceName()
244288
blockName := newBlockName("instance")
245289
supportBlockName := newBlockName("support")
290+
ipPoolBlockName := newBlockName("ip-pool")
246291
resourceName := fmt.Sprintf("oxide_instance.%s", blockName)
247292
initialConfig, err := parsedAccConfig(
248293
resourceInstanceConfig{
249294
BlockName: blockName,
250295
InstanceName: instanceName,
251296
SupportBlockName: supportBlockName,
297+
IPPoolBlockName: ipPoolBlockName,
298+
IPPoolName: ipPoolName,
252299
},
253300
resourceInstanceExternalIPConfigTpl,
254301
)
255302
if err != nil {
256303
t.Errorf("error parsing initial config template data: %e", err)
257304
}
258305

259-
updateConfig, err := parsedAccConfig(
306+
updateConfig1, err := parsedAccConfig(
307+
resourceInstanceConfig{
308+
BlockName: blockName,
309+
InstanceName: instanceName,
310+
SupportBlockName: supportBlockName,
311+
},
312+
resourceInstanceExternalIPConfigUpdate1Tpl,
313+
)
314+
if err != nil {
315+
t.Errorf("error parsing first update config template data: %e", err)
316+
}
317+
318+
updateConfig2, err := parsedAccConfig(
260319
resourceInstanceConfig{
261320
BlockName: blockName,
262321
InstanceName: instanceName,
263322
SupportBlockName: supportBlockName,
264323
},
265-
resourceInstanceExternalIPConfigUpdateTpl,
324+
resourceInstanceExternalIPConfigUpdate2Tpl,
266325
)
267326
if err != nil {
268-
t.Errorf("error parsing update config template data: %e", err)
327+
t.Errorf("error parsing second update config template data: %e", err)
269328
}
270329

271330
resource.ParallelTest(t, resource.TestCase{
272331
PreCheck: func() { testAccPreCheck(t) },
273332
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories(),
274333
CheckDestroy: testAccInstanceDestroy,
275334
Steps: []resource.TestStep{
335+
// Ephemeral external IP with specified IP pool ID.
276336
{
277337
Config: initialConfig,
278338
Check: checkResourceInstanceIP(resourceName, instanceName),
279339
},
280-
// Detach the external IP.
340+
// Ephemeral external IP with default silo IP pool ID.
281341
{
282-
Config: updateConfig,
283-
Check: checkResourceInstanceIPUpdate(resourceName, instanceName),
342+
Config: updateConfig1,
343+
Check: checkResourceInstanceIPUpdate1(resourceName, instanceName),
284344
},
285-
// Attach an external IP.
345+
// Ephemeral external IP with specified IP pool ID.
286346
{
287347
Config: initialConfig,
288348
Check: checkResourceInstanceIP(resourceName, instanceName),
289349
},
350+
// Detach all external IPs.
351+
{
352+
Config: updateConfig2,
353+
Check: checkResourceInstanceIPUpdate2(resourceName, instanceName),
354+
},
290355
{
291356
ResourceName: resourceName,
292357
ImportState: true,
@@ -1007,14 +1072,32 @@ func checkResourceInstanceIP(resourceName, instanceName string) resource.TestChe
10071072
resource.TestCheckResourceAttr(resourceName, "memory", "1073741824"),
10081073
resource.TestCheckResourceAttr(resourceName, "ncpus", "1"),
10091074
resource.TestCheckResourceAttr(resourceName, "external_ips.0.type", "ephemeral"),
1075+
resource.TestCheckResourceAttrSet(resourceName, "external_ips.0.id"),
10101076
resource.TestCheckResourceAttr(resourceName, "start_on_create", "false"),
10111077
resource.TestCheckResourceAttrSet(resourceName, "project_id"),
10121078
resource.TestCheckResourceAttrSet(resourceName, "time_created"),
10131079
resource.TestCheckResourceAttrSet(resourceName, "time_modified"),
10141080
}...)
10151081
}
10161082

1017-
func checkResourceInstanceIPUpdate(resourceName, instanceName string) resource.TestCheckFunc {
1083+
func checkResourceInstanceIPUpdate1(resourceName, instanceName string) resource.TestCheckFunc {
1084+
return resource.ComposeAggregateTestCheckFunc([]resource.TestCheckFunc{
1085+
resource.TestCheckResourceAttrSet(resourceName, "id"),
1086+
resource.TestCheckResourceAttr(resourceName, "description", "a test instance"),
1087+
resource.TestCheckResourceAttr(resourceName, "name", instanceName),
1088+
resource.TestCheckResourceAttr(resourceName, "host_name", "terraform-acc-myhost"),
1089+
resource.TestCheckResourceAttr(resourceName, "memory", "1073741824"),
1090+
resource.TestCheckResourceAttr(resourceName, "ncpus", "1"),
1091+
resource.TestCheckResourceAttr(resourceName, "start_on_create", "false"),
1092+
resource.TestCheckResourceAttrSet(resourceName, "project_id"),
1093+
resource.TestCheckResourceAttrSet(resourceName, "time_created"),
1094+
resource.TestCheckResourceAttrSet(resourceName, "time_modified"),
1095+
resource.TestCheckResourceAttr(resourceName, "external_ips.0.type", "ephemeral"),
1096+
resource.TestCheckResourceAttr(resourceName, "external_ips.0.id", ""),
1097+
}...)
1098+
}
1099+
1100+
func checkResourceInstanceIPUpdate2(resourceName, instanceName string) resource.TestCheckFunc {
10181101
return resource.ComposeAggregateTestCheckFunc([]resource.TestCheckFunc{
10191102
resource.TestCheckResourceAttrSet(resourceName, "id"),
10201103
resource.TestCheckResourceAttr(resourceName, "description", "a test instance"),

0 commit comments

Comments
 (0)