oxide_instance: allow in-place external_ips modification (#381)

Closes
#378.

This patch updates the `oxide_instance` resource to support in-place
modifications to its `external_ips` attribute. That is, the instance
will no longer be destroyed and recreated when `external_ips` is
modified. Instead, the instance will be stopped, external IPs will be
detached and/or attached, and the instance will be started.

This patch also updates the `Read` method to refresh the external IPs
attached to the instance. Previously, since the external IPs required a
resource replacement there was no need to refresh them.

There are 2 types of external IPs, ephemeral and floating. There can be
at most 1 ephemeral external IP attached to an instance and any number
of floating external IPs. That means in order to modify ephemeral
external IPs the external IPs must be detached first and then attached.

This patch needs the following work before it can be merged.
- [x] Add tests for the new `external_ips` modification logic.
- [x] Add validation logic to `external_ips` to enforce at most 1
ephemeral external IP. This was previously "validated" during instance
creation but is not updated during instance update.

Here's the error that's returned when an instance is created with more
than 1 ephemeral external IP.

```
oxide_instance.foo: Creating...
╷
│ Error: Error creating instance
│
│   with oxide_instance.foo,
│   on main.tf line 24, in resource "oxide_instance" "foo":
│   24: resource "oxide_instance" "foo" {
│
│ API error: POST https://oxide.example.com/v1/instances?project=5476ccc9-464d-4dc4-bfc0-5154de1c986f
│ ----------- RESPONSE -----------
│ Status: 400 InvalidRequest
│ Message: An instance may not have more than 1 ephemeral IP address
│ RequestID: fc6144e5-fa76-4583-a024-2e49ce17140e
│ ------- RESPONSE HEADERS -------
│ Content-Type: [application/json]
│ X-Request-Id: [fc6144e5-fa76-4583-a024-2e49ce17140e]
│ Date: [Thu, 09 Jan 2025 03:28:48 GMT]
│ Content-Length: [166]
│
╵
```

Author: sudomateo

internal/provider/resource_instance.go

@@ -21,6 +21,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/boolplanmodifier"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/setplanmodifier"
+	"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringdefault"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
 	"github.com/hashicorp/terraform-plugin-framework/schema/validator"
 	"github.com/hashicorp/terraform-plugin-framework/types"
@@ -256,11 +257,20 @@ func (r *instanceResource) Schema(ctx context.Context, _ resource.SchemaRequest,
 			"external_ips": schema.SetNestedAttribute{
 				Optional:    true,
 				Description: "External IP addresses provided to this instance.",
+				Validators: []validator.Set{
+					instanceExternalIPValidator{},
+				},
 				NestedObject: schema.NestedAttributeObject{
 					Attributes: map[string]schema.Attribute{
+						// The id attribute is optional, computed, and has a default to account for the
+						// case where an instance created with an external IP using the default IP pool
+						// (i.e., id = null) would drift when read (e.g., id = "") and require updating
+						// in place.
 						"id": schema.StringAttribute{
 							Description: "If type is ephemeral, ID of the IP pool to retrieve addresses from, or all available pools if not specified. If type is floating, ID of the floating IP",
 							Optional:    true,
+							Computed:    true,
+							Default:     stringdefault.StaticString(""),
 						},
 						"type": schema.StringAttribute{
 							Description: "Type of external IP.",
@@ -274,9 +284,6 @@ func (r *instanceResource) Schema(ctx context.Context, _ resource.SchemaRequest,
 						},
 					},
 				},
-				PlanModifiers: []planmodifier.Set{
-					setplanmodifier.RequiresReplace(),
-				},
 			},
 			"user_data": schema.StringAttribute{
 				Optional: true,
@@ -510,6 +517,16 @@ func (r *instanceResource) Read(ctx context.Context, req resource.ReadRequest, r
 	state.TimeCreated = types.StringValue(instance.TimeCreated.String())
 	state.TimeModified = types.StringValue(instance.TimeModified.String())
 
+	externalIPs, diags := newAttachedExternalIPModel(ctx, r.client, state.ID.ValueString())
+	resp.Diagnostics.Append(diags...)
+	if resp.Diagnostics.HasError() {
+		return
+	}
+	// Only set the external IPs if there are any to avoid drift.
+	if len(externalIPs) > 0 {
+		state.ExternalIPs = externalIPs
+	}
+
 	keySet, diags := newAssociatedSSHKeysOnCreateSet(ctx, r.client, state.ID.ValueString())
 	resp.Diagnostics.Append(diags...)
 	if resp.Diagnostics.HasError() {
@@ -605,6 +622,25 @@ func (r *instanceResource) Update(ctx context.Context, req resource.UpdateReques
 	}
 	tflog.Trace(ctx, fmt.Sprintf("stopped instance with ID: %v", state.ID.ValueString()), map[string]any{"success": true})
 
+	// Update external IPs.
+	// We detach external IPs first to account for the case where an ephemeral
+	// external IP's IP Pool is modified. This is because there can only be one
+	// ephemeral external IP attached to an instance at a given time and the
+	// last detachment/attachment wins.
+	{
+		externalIPsToDetach := sliceDiff(state.ExternalIPs, plan.ExternalIPs)
+		resp.Diagnostics.Append(detachExternalIPs(ctx, r.client, externalIPsToDetach, state.ID.ValueString())...)
+		if resp.Diagnostics.HasError() {
+			return
+		}
+
+		externalIPsToAttach := sliceDiff(plan.ExternalIPs, state.ExternalIPs)
+		resp.Diagnostics.Append(attachExternalIPs(ctx, r.client, externalIPsToAttach, state.ID.ValueString())...)
+		if resp.Diagnostics.HasError() {
+			return
+		}
+	}
+
 	// Update disk attachments
 	//
 	// We attach new disks first in case the new boot disk is one of the newly added
@@ -728,6 +764,15 @@ func (r *instanceResource) Update(ctx context.Context, req resource.UpdateReques
 	plan.TimeCreated = types.StringValue(instance.TimeCreated.String())
 	plan.TimeModified = types.StringValue(instance.TimeModified.String())
 
+	externalIPs, diags := newAttachedExternalIPModel(ctx, r.client, state.ID.ValueString())
+	resp.Diagnostics.Append(diags...)
+	if resp.Diagnostics.HasError() {
+		return
+	}
+	if len(externalIPs) > 0 {
+		plan.ExternalIPs = externalIPs
+	}
+
 	// TODO: should I do this or read from the newly created ones?
 	diskSet, diags := newAttachedDisksSet(ctx, r.client, state.ID.ValueString())
 	resp.Diagnostics.Append(diags...)
@@ -1020,6 +1065,32 @@ func newAttachedNetworkInterfacesModel(ctx context.Context, client *oxide.Client
 	return nicSet, nil
 }
 
+func newAttachedExternalIPModel(ctx context.Context, client *oxide.Client, instanceID string) (
+	[]instanceResourceExternalIPModel, diag.Diagnostics) {
+	var diags diag.Diagnostics
+	externalIPs := make([]instanceResourceExternalIPModel, 0)
+
+	externalIPResponse, err := client.InstanceExternalIpList(ctx, oxide.InstanceExternalIpListParams{
+		Instance: oxide.NameOrId(instanceID),
+	})
+	if err != nil {
+		diags.AddError(
+			"Unable to list instance external ips:",
+			"API error: "+err.Error(),
+		)
+		return nil, diags
+	}
+
+	for _, externalIP := range externalIPResponse.Items {
+		externalIPs = append(externalIPs, instanceResourceExternalIPModel{
+			ID:   types.StringValue(externalIP.Id),
+			Type: types.StringValue(string(externalIP.Kind)),
+		})
+	}
+
+	return externalIPs, nil
+}
+
 type vpcAndSubnetNames struct {
 	vpc    string
 	subnet string
@@ -1183,6 +1254,115 @@ func deleteNICs(ctx context.Context, client *oxide.Client, models []instanceReso
 	return nil
 }
 
+// attachExternalIPs attaches the external IPs specified by externalIPs to the
+// instance specified by instanceID.
+func attachExternalIPs(ctx context.Context, client *oxide.Client, externalIPs []instanceResourceExternalIPModel, instanceID string) diag.Diagnostics {
+	var diags diag.Diagnostics
+
+	for _, v := range externalIPs {
+		externalIPID := v.ID
+		externalIPType := v.Type
+
+		switch oxide.ExternalIpKind(externalIPType.ValueString()) {
+		case oxide.ExternalIpKindEphemeral:
+			params := oxide.InstanceEphemeralIpAttachParams{
+				Instance: oxide.NameOrId(instanceID),
+				Body: &oxide.EphemeralIpCreate{
+					Pool: oxide.NameOrId(externalIPID.ValueString()),
+				},
+			}
+
+			if _, err := client.InstanceEphemeralIpAttach(ctx, params); err != nil {
+				diags.AddError(
+					fmt.Sprintf("Error attaching ephemeral external IP with ID %s", externalIPID.ValueString()),
+					"API error: "+err.Error(),
+				)
+
+				return diags
+			}
+
+		case oxide.ExternalIpKindFloating:
+			params := oxide.FloatingIpAttachParams{
+				FloatingIp: oxide.NameOrId(externalIPID.ValueString()),
+				Body: &oxide.FloatingIpAttach{
+					Kind:   oxide.FloatingIpParentKindInstance,
+					Parent: oxide.NameOrId(instanceID),
+				},
+			}
+
+			if _, err := client.FloatingIpAttach(ctx, params); err != nil {
+				diags.AddError(
+					fmt.Sprintf("Error attaching floating external IP with ID %s", externalIPID.ValueString()),
+					"API error: "+err.Error(),
+				)
+
+				return diags
+			}
+		default:
+			diags.AddError(
+				fmt.Sprintf("Cannot attach invalid external IP type %q", externalIPType.ValueString()),
+				fmt.Sprintf("The external IP type must be one of: %q, %q", oxide.ExternalIpCreateTypeEphemeral, oxide.ExternalIpCreateTypeFloating),
+			)
+			return diags
+		}
+
+		tflog.Trace(ctx, fmt.Sprintf("successfully attached %s external IP with ID %s", externalIPType.ValueString(), externalIPID.ValueString()), map[string]any{"success": true})
+	}
+
+	return nil
+}
+
+// detachExternalIPs detaches the external IPs specified by externalIPs from the
+// instance specified by instanceID.
+func detachExternalIPs(ctx context.Context, client *oxide.Client, externalIPs []instanceResourceExternalIPModel, instanceID string) diag.Diagnostics {
+	var diags diag.Diagnostics
+
+	for _, v := range externalIPs {
+		externalIPID := v.ID
+		externalIPType := v.Type
+
+		switch oxide.ExternalIpKind(externalIPType.ValueString()) {
+		case oxide.ExternalIpKindEphemeral:
+			params := oxide.InstanceEphemeralIpDetachParams{
+				Instance: oxide.NameOrId(instanceID),
+			}
+
+			if err := client.InstanceEphemeralIpDetach(ctx, params); err != nil {
+				diags.AddError(
+					fmt.Sprintf("Error detaching ephemeral external IP with ID %s", externalIPID.ValueString()),
+					"API error: "+err.Error(),
+				)
+
+				return diags
+			}
+
+		case oxide.ExternalIpKindFloating:
+			params := oxide.FloatingIpDetachParams{
+				FloatingIp: oxide.NameOrId(externalIPID.ValueString()),
+			}
+
+			if _, err := client.FloatingIpDetach(ctx, params); err != nil {
+				diags.AddError(
+					fmt.Sprintf("Error detaching floating external IP with ID %s", externalIPID.ValueString()),
+					"API error: "+err.Error(),
+				)
+
+				return diags
+			}
+		default:
+			diags.AddError(
+				fmt.Sprintf("Cannot detach invalid external IP type %q", externalIPType.ValueString()),
+				fmt.Sprintf("The external IP type must be one of: %q, %q", oxide.ExternalIpCreateTypeEphemeral, oxide.ExternalIpCreateTypeFloating),
+			)
+			return diags
+		}
+
+		tflog.Trace(ctx, fmt.Sprintf("successfully detached %s external IP with ID %s", externalIPType.ValueString(), externalIPID.ValueString()), map[string]any{"success": true})
+	}
+
+	return nil
+}
+
 func attachDisks(ctx context.Context, client *oxide.Client, disks []attr.Value, instanceID string) diag.Diagnostics {
 	var diags diag.Diagnostics
 
@@ -1327,3 +1507,71 @@ func attrValueSliceContains(s []attr.Value, str string) (bool, error) {
 	}
 	return false, nil
 }
+
+// Ensure the concrete validator satisfies the [validator.Set] interface.
+var _ validator.Set = instanceExternalIPValidator{}
+
+// instanceExternalIPValidator is a custom validator that validates the
+// external_ips attribute on an oxide_instance resource.
+type instanceExternalIPValidator struct{}
+
+// Description describes the validation in plain text formatting.
+func (f instanceExternalIPValidator) Description(context.Context) string {
+	return "cannot have more than one ephemeral external ip"
+}
+
+// MarkdownDescription describes the validation in Markdown formatting.
+func (f instanceExternalIPValidator) MarkdownDescription(context.Context) string {
+	return "cannot have more than one ephemeral external ip"
+}
+
+// ValidateSet validates whether a set of [instanceResourceExternalIPModel]
+// objects has at most one object with type = "ephemeral". The Terraform SDK
+// already deduplicates sets within configuration. For example, the following
+// configuration in Terraform results in a single ephemeral external IP.
+//
+//	resource "oxide_instance" "example" {
+//	  external_ips = [
+//	    { type = "ephemeral"},
+//	    { type = "ephemeral"},
+//	  ]
+//	}
+//
+// However, that deduplication does not extend to sets that contain different
+// attributes, like so.
+//
+//	resource "oxide_instance" "example" {
+//	  external_ips = [
+//	    { type = "ephemeral", id = "a58dc21d-896d-4e5a-bb77-b0922a04e553"},
+//	    { type = "ephemeral"},
+//	  ]
+//	}
+//
+// That's where this validator comes in. This validator errors with the above
+// configuration, preventing a user from using multiple ephemeral external IPs.
+func (f instanceExternalIPValidator) ValidateSet(ctx context.Context, req validator.SetRequest, resp *validator.SetResponse) {
+	if req.ConfigValue.IsNull() || req.ConfigValue.IsUnknown() {
+		return
+	}
+
+	var externalIPs []instanceResourceExternalIPModel
+	diags := req.ConfigValue.ElementsAs(ctx, &externalIPs, false)
+	resp.Diagnostics.Append(diags...)
+	if resp.Diagnostics.HasError() {
+		return
+	}
+
+	var ephemeralExternalIPs int
+	for _, externalIP := range externalIPs {
+		if externalIP.Type.ValueString() == string(oxide.ExternalIpCreateTypeEphemeral) {
+			ephemeralExternalIPs++
+		}
+	}
+	if ephemeralExternalIPs > 1 {
+		resp.Diagnostics.AddError(
+			fmt.Sprintf("Too many external IPs with type = %q", oxide.ExternalIpCreateTypeEphemeral),
+			fmt.Sprintf("Only 1 external IP with type = %q is allowed, but found %d.", oxide.ExternalIpCreateTypeEphemeral, ephemeralExternalIPs),
+		)
+		return
+	}
+}