Skip to content

Commit c545a85

Browse files
VerolopVerolop
authored
Merge pull request #83 from authzed/fix-state-drift
fix: prevent state drift in special fields for all resources
2 parents a66e77f + 212a123 commit c545a85

File tree

8 files changed

+319
-55
lines changed

8 files changed

+319
-55
lines changed

‎internal/provider/policy_resource.go‎

Lines changed: 41 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ import (
1111
"github.com/hashicorp/terraform-plugin-framework/path"
1212
"github.com/hashicorp/terraform-plugin-framework/resource"
1313
"github.com/hashicorp/terraform-plugin-framework/resource/schema"
14+
"github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
15+
"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
1416
"github.com/hashicorp/terraform-plugin-framework/types"
1517
)
1618

@@ -51,6 +53,9 @@ func (r *policyResource) Schema(_ context.Context, _ resource.SchemaRequest, res
5153
"id": schema.StringAttribute{
5254
Computed: true,
5355
Description: "Unique identifier for this resource",
56+
PlanModifiers: []planmodifier.String{
57+
stringplanmodifier.UseStateForUnknown(),
58+
},
5459
},
5560
"name": schema.StringAttribute{
5661
Required: true,
@@ -76,14 +81,22 @@ func (r *policyResource) Schema(_ context.Context, _ resource.SchemaRequest, res
7681
"created_at": schema.StringAttribute{
7782
Computed: true,
7883
Description: "Timestamp when the policy was created",
84+
PlanModifiers: []planmodifier.String{
85+
stringplanmodifier.UseStateForUnknown(),
86+
},
7987
},
8088
"creator": schema.StringAttribute{
8189
Computed: true,
8290
Description: "User who created the policy",
91+
PlanModifiers: []planmodifier.String{
92+
stringplanmodifier.UseStateForUnknown(),
93+
},
8394
},
8495
"etag": schema.StringAttribute{
85-
Computed: true,
86-
Description: "Version identifier used to prevent conflicts from concurrent updates, ensuring safe resource modifications",
96+
Computed: true,
97+
PlanModifiers: []planmodifier.String{
98+
stringplanmodifier.UseStateForUnknown(),
99+
},
87100
},
88101
},
89102
}
@@ -137,18 +150,21 @@ func (r *policyResource) Create(ctx context.Context, req resource.CreateRequest,
137150

138151
createdPolicyWithETag, err := r.client.CreatePolicy(ctx, policy)
139152
if err != nil {
140-
resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to create policy, got error: %s", err))
153+
resp.Diagnostics.AddError("Error creating Policy", err.Error())
141154
return
142155
}
143156

144-
createdPolicy := createdPolicyWithETag.Policy
145-
data.ID = types.StringValue(createdPolicy.ID)
146-
data.CreatedAt = types.StringValue(createdPolicy.CreatedAt)
147-
data.Creator = types.StringValue(createdPolicy.Creator)
157+
data.ID = types.StringValue(createdPolicyWithETag.Policy.ID)
158+
data.CreatedAt = types.StringValue(createdPolicyWithETag.Policy.CreatedAt)
159+
if createdPolicyWithETag.Policy.Creator == "" {
160+
data.Creator = types.StringNull()
161+
} else {
162+
data.Creator = types.StringValue(createdPolicyWithETag.Policy.Creator)
163+
}
148164
data.ETag = types.StringValue(createdPolicyWithETag.ETag)
149165

150166
// Update role IDs in case the order or values changed
151-
roleIDList, diags := types.ListValueFrom(ctx, types.StringType, createdPolicy.RoleIDs)
167+
roleIDList, diags := types.ListValueFrom(ctx, types.StringType, createdPolicyWithETag.Policy.RoleIDs)
152168
resp.Diagnostics.Append(diags...)
153169
if resp.Diagnostics.HasError() {
154170
return
@@ -181,11 +197,16 @@ func (r *policyResource) Read(ctx context.Context, req resource.ReadRequest, res
181197
policy := policyWithETag.Policy
182198

183199
// Map response to model
200+
data.ID = types.StringValue(policy.ID)
184201
data.Name = types.StringValue(policy.Name)
185202
data.Description = types.StringValue(policy.Description)
186203
data.PrincipalID = types.StringValue(policy.PrincipalID)
187204
data.CreatedAt = types.StringValue(policy.CreatedAt)
188-
data.Creator = types.StringValue(policy.Creator)
205+
if policy.Creator == "" {
206+
data.Creator = types.StringNull()
207+
} else {
208+
data.Creator = types.StringValue(policy.Creator)
209+
}
189210
data.ETag = types.StringValue(policyWithETag.ETag)
190211

191212
// Map role IDs
@@ -195,7 +216,6 @@ func (r *policyResource) Read(ctx context.Context, req resource.ReadRequest, res
195216
return
196217
}
197218
data.RoleIDs = roleIDList
198-
199219
resp.Diagnostics.Append(resp.State.Set(ctx, &data)...)
200220
}
201221

@@ -220,38 +240,37 @@ func (r *policyResource) Update(ctx context.Context, req resource.UpdateRequest,
220240
return
221241
}
222242

223-
// Create policy with updated data
243+
// Create policy with updated data - use state values for immutable fields
224244
policy := &models.Policy{
225-
ID: data.ID.ValueString(),
245+
ID: state.ID.ValueString(), // Use state for immutable ID
226246
Name: data.Name.ValueString(),
227247
Description: data.Description.ValueString(),
228248
PermissionsSystemID: data.PermissionsSystemID.ValueString(),
229249
PrincipalID: data.PrincipalID.ValueString(),
230250
RoleIDs: roleIDs,
251+
CreatedAt: state.CreatedAt.ValueString(), // Preserve immutable CreatedAt
252+
}
253+
254+
// Handle Creator field - it might be null in state
255+
if !state.Creator.IsNull() {
256+
policy.Creator = state.Creator.ValueString()
231257
}
232258

233259
// Coordinate operations to prevent conflicts
234260
permissionSystemID := data.PermissionsSystemID.ValueString()
235261
r.fgamCoordinator.Lock(permissionSystemID)
236262
defer r.fgamCoordinator.Unlock(permissionSystemID)
237263

238-
// Use the ETag from state for optimistic concurrency control
239264
updatedPolicyWithETag, err := r.client.UpdatePolicy(ctx, policy, state.ETag.ValueString())
240265
if err != nil {
241266
resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to update policy, got error: %s", err))
242267
return
243268
}
244269

245270
// Update resource data with the response
246-
data.ID = types.StringValue(updatedPolicyWithETag.Policy.ID)
247-
248-
// If the ID is empty, preserve the original ID
249-
if data.ID.ValueString() == "" {
250-
data.ID = state.ID
251-
}
252-
253-
data.CreatedAt = types.StringValue(updatedPolicyWithETag.Policy.CreatedAt)
254-
data.Creator = types.StringValue(updatedPolicyWithETag.Policy.Creator)
271+
data.ID = state.ID
272+
data.CreatedAt = state.CreatedAt
273+
data.Creator = state.Creator
255274
data.ETag = types.StringValue(updatedPolicyWithETag.ETag)
256275

257276
// Update role IDs in case the order or values changed

‎internal/provider/resource_policy_test.go‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,59 @@ func TestAccAuthzedPolicy_validation(t *testing.T) {
136136
})
137137
}
138138

139+
func TestAccAuthzedPolicy_noDrift(t *testing.T) {
140+
resourceName := "authzed_policy.test"
141+
testID := helpers.GenerateTestID("test-policy-drift")
142+
roleName := fmt.Sprintf("%s-role", testID)
143+
updatedDescription := "Updated test policy description"
144+
145+
resource.ParallelTest(t, resource.TestCase{
146+
PreCheck: func() { testAccPreCheck(t) },
147+
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
148+
CheckDestroy: testAccCheckPolicyDestroy,
149+
Steps: []resource.TestStep{
150+
// Create initial policy
151+
{
152+
Config: testAccPolicyConfig_basic(testID, roleName),
153+
Check: resource.ComposeTestCheckFunc(
154+
testAccCheckPolicyExists(resourceName),
155+
resource.TestCheckResourceAttr(resourceName, "name", testID),
156+
resource.TestCheckResourceAttr(resourceName, "description", "Test policy description"),
157+
resource.TestCheckResourceAttrSet(resourceName, "id"),
158+
resource.TestCheckResourceAttrSet(resourceName, "created_at"),
159+
resource.TestCheckResourceAttrSet(resourceName, "creator"),
160+
resource.TestCheckResourceAttrSet(resourceName, "etag"),
161+
),
162+
},
163+
// Verify no drift on second plan - this is the key test
164+
{
165+
Config: testAccPolicyConfig_basic(testID, roleName),
166+
PlanOnly: true,
167+
// If there's drift in computed fields (id, created_at, creator, etag),
168+
// this step will fail because Terraform will detect changes
169+
},
170+
// Update mutable fields and verify computed fields don't drift
171+
{
172+
Config: testAccPolicyConfig_update(testID, roleName, updatedDescription),
173+
Check: resource.ComposeTestCheckFunc(
174+
testAccCheckPolicyExists(resourceName),
175+
resource.TestCheckResourceAttr(resourceName, "name", testID),
176+
resource.TestCheckResourceAttr(resourceName, "description", updatedDescription),
177+
resource.TestCheckResourceAttrSet(resourceName, "id"),
178+
resource.TestCheckResourceAttrSet(resourceName, "created_at"),
179+
resource.TestCheckResourceAttrSet(resourceName, "creator"),
180+
resource.TestCheckResourceAttrSet(resourceName, "etag"),
181+
),
182+
},
183+
{
184+
Config: testAccPolicyConfig_update(testID, roleName, updatedDescription),
185+
PlanOnly: true,
186+
// This verifies that after an update, computed fields don't show as changing
187+
},
188+
},
189+
})
190+
}
191+
139192
// Helper functions
140193

141194
func testAccCheckPolicyExists(resourceName string) resource.TestCheckFunc {

‎internal/provider/resource_role_test.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,55 @@ func TestAccAuthzedRole_validation(t *testing.T) {
176176
})
177177
}
178178

179+
func TestAccAuthzedRole_noDrift(t *testing.T) {
180+
resourceName := "authzed_role.test"
181+
testID := helpers.GenerateTestID("test-role-drift")
182+
183+
resource.ParallelTest(t, resource.TestCase{
184+
PreCheck: func() { testAccPreCheck(t) },
185+
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
186+
CheckDestroy: testAccCheckRoleDestroy,
187+
Steps: []resource.TestStep{
188+
// Create initial role
189+
{
190+
Config: testAccRoleConfig_basic(testID),
191+
Check: resource.ComposeTestCheckFunc(
192+
testAccCheckRoleExists(resourceName),
193+
resource.TestCheckResourceAttr(resourceName, "name", testID),
194+
resource.TestCheckResourceAttr(resourceName, "description", "Test role description"),
195+
resource.TestCheckResourceAttrSet(resourceName, "id"),
196+
resource.TestCheckResourceAttrSet(resourceName, "created_at"),
197+
resource.TestCheckResourceAttrSet(resourceName, "creator"),
198+
resource.TestCheckResourceAttrSet(resourceName, "etag"),
199+
),
200+
},
201+
// Verify no drift on second plan. Critical path.
202+
{
203+
Config: testAccRoleConfig_basic(testID),
204+
PlanOnly: true,
205+
},
206+
// Update permissions and verify computed fields don't drift
207+
{
208+
Config: testAccRoleConfig_updatePermissions(testID),
209+
Check: resource.ComposeTestCheckFunc(
210+
testAccCheckRoleExists(resourceName),
211+
resource.TestCheckResourceAttr(resourceName, "name", testID),
212+
resource.TestCheckResourceAttrSet(resourceName, "id"),
213+
resource.TestCheckResourceAttrSet(resourceName, "created_at"),
214+
resource.TestCheckResourceAttrSet(resourceName, "creator"),
215+
resource.TestCheckResourceAttrSet(resourceName, "etag"),
216+
),
217+
},
218+
// Verify no drift after update - computed fields should remain stable
219+
{
220+
Config: testAccRoleConfig_updatePermissions(testID),
221+
PlanOnly: true,
222+
// This verifies that after an update, computed fields don't show as changing
223+
},
224+
},
225+
})
226+
}
227+
179228
// Helper functions
180229

181230
func testAccCheckRoleExists(resourceName string) resource.TestCheckFunc {

‎internal/provider/resource_service_account_test.go‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,54 @@ func TestAccAuthzedServiceAccount_validation(t *testing.T) {
124124
})
125125
}
126126

127+
func TestAccAuthzedServiceAccount_noDrift(t *testing.T) {
128+
resourceName := "authzed_service_account.test"
129+
testID := helpers.GenerateTestID("test-service-account-drift")
130+
131+
resource.ParallelTest(t, resource.TestCase{
132+
PreCheck: func() { testAccPreCheck(t) },
133+
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
134+
CheckDestroy: testAccCheckServiceAccountDestroy,
135+
Steps: []resource.TestStep{
136+
// Create initial service account
137+
{
138+
Config: testAccServiceAccountConfig_basic(testID),
139+
Check: resource.ComposeTestCheckFunc(
140+
testAccCheckServiceAccountExists(resourceName),
141+
resource.TestCheckResourceAttr(resourceName, "name", testID),
142+
resource.TestCheckResourceAttr(resourceName, "description", "Test service account description"),
143+
resource.TestCheckResourceAttrSet(resourceName, "id"),
144+
resource.TestCheckResourceAttrSet(resourceName, "created_at"),
145+
resource.TestCheckResourceAttrSet(resourceName, "creator"),
146+
resource.TestCheckResourceAttrSet(resourceName, "etag"),
147+
),
148+
},
149+
{
150+
Config: testAccServiceAccountConfig_basic(testID),
151+
PlanOnly: true,
152+
},
153+
{
154+
Config: testAccServiceAccountConfig_updated(testID),
155+
Check: resource.ComposeTestCheckFunc(
156+
testAccCheckServiceAccountExists(resourceName),
157+
resource.TestCheckResourceAttr(resourceName, "name", testID),
158+
resource.TestCheckResourceAttr(resourceName, "description", "Updated test service account description"),
159+
resource.TestCheckResourceAttrSet(resourceName, "id"),
160+
resource.TestCheckResourceAttrSet(resourceName, "created_at"),
161+
resource.TestCheckResourceAttrSet(resourceName, "creator"),
162+
resource.TestCheckResourceAttrSet(resourceName, "etag"),
163+
),
164+
},
165+
// Verify no drift after update - computed fields should remain stable
166+
{
167+
Config: testAccServiceAccountConfig_updated(testID),
168+
PlanOnly: true,
169+
// This verifies that after an update, computed fields don't show as changing
170+
},
171+
},
172+
})
173+
}
174+
127175
// Helper functions
128176

129177
func testAccCheckServiceAccountExists(resourceName string) resource.TestCheckFunc {

‎internal/provider/resource_token_test.go‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,56 @@ func TestAccAuthzedToken_validation(t *testing.T) {
161161
})
162162
}
163163

164+
func TestAccAuthzedToken_noDrift(t *testing.T) {
165+
resourceName := "authzed_token.test"
166+
testID := helpers.GenerateTestID("test-token-drift")
167+
168+
resource.ParallelTest(t, resource.TestCase{
169+
PreCheck: func() { testAccPreCheck(t) },
170+
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
171+
CheckDestroy: testAccCheckTokenDestroy,
172+
Steps: []resource.TestStep{
173+
// Create initial token
174+
{
175+
Config: testAccTokenConfig_basic(testID),
176+
Check: resource.ComposeTestCheckFunc(
177+
testAccCheckTokenExists(resourceName),
178+
resource.TestCheckResourceAttr(resourceName, "name", testID),
179+
resource.TestCheckResourceAttr(resourceName, "description", "Test token description"),
180+
resource.TestCheckResourceAttrSet(resourceName, "id"),
181+
resource.TestCheckResourceAttrSet(resourceName, "created_at"),
182+
resource.TestCheckResourceAttrSet(resourceName, "creator"),
183+
resource.TestCheckResourceAttrSet(resourceName, "etag"),
184+
resource.TestCheckResourceAttrSet(resourceName, "hash"),
185+
),
186+
},
187+
{
188+
Config: testAccTokenConfig_basic(testID),
189+
PlanOnly: true,
190+
},
191+
// Update mutable fields and verify computed fields don't drift
192+
{
193+
Config: testAccTokenConfig_updated(testID),
194+
Check: resource.ComposeTestCheckFunc(
195+
testAccCheckTokenExists(resourceName),
196+
resource.TestCheckResourceAttr(resourceName, "name", testID),
197+
resource.TestCheckResourceAttr(resourceName, "description", "Updated test token description"),
198+
resource.TestCheckResourceAttrSet(resourceName, "id"),
199+
resource.TestCheckResourceAttrSet(resourceName, "created_at"),
200+
resource.TestCheckResourceAttrSet(resourceName, "creator"),
201+
resource.TestCheckResourceAttrSet(resourceName, "etag"),
202+
resource.TestCheckResourceAttrSet(resourceName, "hash"),
203+
),
204+
},
205+
{
206+
Config: testAccTokenConfig_updated(testID),
207+
PlanOnly: true,
208+
// This verifies that after an update, computed fields don't show as changing
209+
},
210+
},
211+
})
212+
}
213+
164214
// Helper functions
165215

166216
func testAccCheckTokenExists(resourceName string) resource.TestCheckFunc {

0 commit comments

Comments
 (0)