vpc_firewall_rules: fix provider produced invalid plan

sudomateo · sudomateo · commit 3335ef6a38ac · 2025-06-25T15:08:27.000-04:00
Added acceptance tests to surface the issue reported in #453. See #453 (comment) for an understanding of what's happening. Removed the computed attributes from the firewall rules since those computed fields were being updated for all of the rules, even when Terraform did not expect a rule to be updated. Unfortunately all of the computed attributes had to be removed for this to work and keep the update in-place logic.
diff --git a/.changelog/0.11.0.toml b/.changelog/0.11.0.toml
@@ -1,6 +1,6 @@
 [[breaking]]
-title = ""
-description = ""
+title = "Removed computed attributes from `vpc_firewall_rules`"
+description = "Removed all computed attributes from the `rules` attribute within the `vpc_firewall_rules` resource to fix an invalid plan error while maintaining in-place update of VPC firewall rules. [#456](https://github.com/oxidecomputer/terraform-provider-oxide/pull/456)"
 
 [[features]]
 title = ""
@@ -12,4 +12,4 @@ description = ""
 
 [[bugs]]
 title = ""
-description = ""
+description = ""
diff --git a/internal/provider/resource_vpc_firewall_rules.go b/internal/provider/resource_vpc_firewall_rules.go
@@ -44,24 +44,30 @@ type vpcFirewallRulesResource struct {
 
 type vpcFirewallRulesResourceModel struct {
 	// This ID is specific to Terraform only
-	ID       types.String                        `tfsdk:"id"`
-	Rules    []vpcFirewallRulesResourceRuleModel `tfsdk:"rules"`
-	Timeouts timeouts.Value                      `tfsdk:"timeouts"`
-	VPCID    types.String                        `tfsdk:"vpc_id"`
+	ID           types.String                        `tfsdk:"id"`
+	Rules        []vpcFirewallRulesResourceRuleModel `tfsdk:"rules"`
+	Timeouts     timeouts.Value                      `tfsdk:"timeouts"`
+	VPCID        types.String                        `tfsdk:"vpc_id"`
+
+	// Populated from the same fields within [vpcFirewallRulesResourceRuleModel].
+	TimeCreated  types.String                        `tfsdk:"time_created"`
+	TimeModified types.String                        `tfsdk:"time_modified"`
 }
 
 type vpcFirewallRulesResourceRuleModel struct {
-	Action       types.String                              `tfsdk:"action"`
-	Description  types.String                              `tfsdk:"description"`
-	Direction    types.String                              `tfsdk:"direction"`
-	Filters      *vpcFirewallRulesResourceRuleFiltersModel `tfsdk:"filters"`
-	ID           types.String                              `tfsdk:"id"`
-	Name         types.String                              `tfsdk:"name"`
-	Priority     types.Int64                               `tfsdk:"priority"`
-	Status       types.String                              `tfsdk:"status"`
-	Targets      []vpcFirewallRulesResourceRuleTargetModel `tfsdk:"targets"`
-	TimeCreated  types.String                              `tfsdk:"time_created"`
-	TimeModified types.String                              `tfsdk:"time_modified"`
+	Action      types.String                              `tfsdk:"action"`
+	Description types.String                              `tfsdk:"description"`
+	Direction   types.String                              `tfsdk:"direction"`
+	Filters     *vpcFirewallRulesResourceRuleFiltersModel `tfsdk:"filters"`
+	Name        types.String                              `tfsdk:"name"`
+	Priority    types.Int64                               `tfsdk:"priority"`
+	Status      types.String                              `tfsdk:"status"`
+	Targets     []vpcFirewallRulesResourceRuleTargetModel `tfsdk:"targets"`
+
+	// Used to retrieve the timestamps from the API and populate the same fields
+	// within [vpcFirewallRulesResourceModel].
+	TimeCreated  types.String `tfsdk:"-"`
+	TimeModified types.String `tfsdk:"-"`
 }
 
 type vpcFirewallRulesResourceRuleTargetModel struct {
@@ -110,6 +116,10 @@ func (r *vpcFirewallRulesResource) Schema(ctx context.Context, _ resource.Schema
 					stringplanmodifier.RequiresReplace(),
 				},
 			},
+			// The rules attribute cannot contain computed attributes since the upstream API
+			// returns updated attributes for every rule, irrespective of which rules actually
+			// change. See https://github.com/oxidecomputer/terraform-provider-oxide/issues/453
+			// for more information.
 			"rules": schema.SetNestedAttribute{
 				Required:    true,
 				Description: "Associated firewall rules.",
@@ -202,10 +212,6 @@ func (r *vpcFirewallRulesResource) Schema(ctx context.Context, _ resource.Schema
 								},
 							},
 						},
-						"id": schema.StringAttribute{
-							Computed:    true,
-							Description: "Unique, immutable, system-controlled identifier of the firewall rule.",
-						},
 						"name": schema.StringAttribute{
 							Required:    true,
 							Description: "Name of the VPC firewall rule.",
@@ -255,14 +261,6 @@ func (r *vpcFirewallRulesResource) Schema(ctx context.Context, _ resource.Schema
 								},
 							},
 						},
-						"time_created": schema.StringAttribute{
-							Computed:    true,
-							Description: "Timestamp of when this VPC firewall rule was created.",
-						},
-						"time_modified": schema.StringAttribute{
-							Computed:    true,
-							Description: "Timestamp of when this VPC firewall rule was last modified.",
-						},
 					},
 				},
 			},
@@ -276,6 +274,14 @@ func (r *vpcFirewallRulesResource) Schema(ctx context.Context, _ resource.Schema
 				Computed:    true,
 				Description: "Unique, immutable, system-controlled identifier of the firewall rules. Specific only to Terraform.",
 			},
+			"time_created": schema.StringAttribute{
+				Computed:    true,
+				Description: "Timestamp of when the VPC firewall rules were last created.",
+			},
+			"time_modified": schema.StringAttribute{
+				Computed:    true,
+				Description: "Timestamp of when the VPC firewall rules were last modified.",
+			},
 		},
 	}
 }
@@ -327,6 +333,13 @@ func (r *vpcFirewallRulesResource) Create(ctx context.Context, req resource.Crea
 		return
 	}
 
+	plan.TimeCreated = types.StringNull()
+	plan.TimeModified = types.StringNull()
+	if len(plan.Rules) > 0 {
+		plan.TimeCreated = plan.Rules[0].TimeCreated
+		plan.TimeModified = plan.Rules[0].TimeModified
+	}
+
 	// Save plan into Terraform state
 	resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...)
 	if resp.Diagnostics.HasError() {
@@ -384,6 +397,13 @@ func (r *vpcFirewallRulesResource) Read(ctx context.Context, req resource.ReadRe
 
 	state.Rules = rules
 
+	state.TimeCreated = types.StringNull()
+	state.TimeModified = types.StringNull()
+	if len(state.Rules) > 0 {
+		state.TimeCreated = state.Rules[0].TimeCreated
+		state.TimeModified = state.Rules[0].TimeModified
+	}
+
 	// Save updated data into Terraform state
 	resp.Diagnostics.Append(resp.State.Set(ctx, &state)...)
 	if resp.Diagnostics.HasError() {
@@ -447,6 +467,13 @@ func (r *vpcFirewallRulesResource) Update(ctx context.Context, req resource.Upda
 		return
 	}
 
+	plan.TimeCreated = types.StringNull()
+	plan.TimeModified = types.StringNull()
+	if len(plan.Rules) > 0 {
+		plan.TimeCreated = plan.Rules[0].TimeCreated
+		plan.TimeModified = plan.Rules[0].TimeModified
+	}
+
 	// Save plan into Terraform state
 	resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...)
 	if resp.Diagnostics.HasError() {
@@ -536,7 +563,6 @@ func newVPCFirewallRulesModel(rules []oxide.VpcFirewallRule) ([]vpcFirewallRules
 			Action:      types.StringValue(string(rule.Action)),
 			Description: types.StringValue(rule.Description),
 			Direction:   types.StringValue(string(rule.Direction)),
-			ID:          types.StringValue(rule.Id),
 			Name:        types.StringValue(string(rule.Name)),
 			// We can safely dereference rule.Priority as it's a required field
 			Priority:     types.Int64Value(int64(*rule.Priority)),
diff --git a/internal/provider/resource_vpc_firewall_rules_test.go b/internal/provider/resource_vpc_firewall_rules_test.go
