From f22d9f9ef831d5c8977074d03e053444227d7b23 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Mon, 14 Apr 2025 07:51:24 +0000 Subject: [PATCH 1/2] fix: handle user deletion config drift --- internal/provider/user_resource.go | 18 ++++++++++++++++++ internal/provider/user_resource_test.go | 14 ++++++++++++++ internal/provider/util.go | 13 ++++++++++++- 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/internal/provider/user_resource.go b/internal/provider/user_resource.go index 3fa570e..b184d31 100644 --- a/internal/provider/user_resource.go +++ b/internal/provider/user_resource.go @@ -248,6 +248,7 @@ func (r *UserResource) Read(ctx context.Context, req resource.ReadRequest, resp client := r.data.Client + // Lookup by ID to handle imports user, err := client.User(ctx, data.ID.ValueString()) if err != nil { if isNotFound(err) { @@ -274,6 +275,23 @@ func (r *UserResource) Read(ctx context.Context, req resource.ReadRequest, resp data.LoginType = types.StringValue(string(user.LoginType)) data.Suspended = types.BoolValue(user.Status == codersdk.UserStatusSuspended) + // Also query by username to check for deletion or username reassignment + userByName, err := client.User(ctx, data.Username.ValueString()) + if err != nil { + if isNotFound(err) { + resp.Diagnostics.AddWarning("Client Warning", fmt.Sprintf("User with ID %q not found. Marking as deleted.", data.ID.ValueString())) + resp.State.RemoveResource(ctx) + return + } + resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to get current user, got error: %s", err)) + return + } + if userByName.ID != data.ID.ValueUUID() { + resp.Diagnostics.AddWarning("Client Error", fmt.Sprintf("The username %q has been reassigned to a new user. Marking as deleted.", user.Username)) + resp.State.RemoveResource(ctx) + return + } + // Save updated data into Terraform state resp.Diagnostics.Append(resp.State.Set(ctx, &data)...) } diff --git a/internal/provider/user_resource_test.go b/internal/provider/user_resource_test.go index d717bfa..8c78acc 100644 --- a/internal/provider/user_resource_test.go +++ b/internal/provider/user_resource_test.go @@ -9,6 +9,7 @@ import ( "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/terraform-provider-coderd/integration" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/terraform" "github.com/stretchr/testify/require" ) @@ -100,6 +101,19 @@ func TestAccUserResource(t *testing.T) { resource.TestCheckResourceAttr("coderd_user.test", "login_type", "github"), ), }, + // Verify config drift via deletion is handled + { + Config: cfg4.String(t), + Check: func(*terraform.State) error { + user, err := client.User(ctx, "exampleNew") + if err != nil { + return err + } + return client.DeleteUser(ctx, user.ID) + }, + // The Plan should be to create the entire resource + ExpectNonEmptyPlan: true, + }, }, }) } diff --git a/internal/provider/util.go b/internal/provider/util.go index e409738..3f35a25 100644 --- a/internal/provider/util.go +++ b/internal/provider/util.go @@ -8,6 +8,7 @@ import ( "net/http" "os" "path/filepath" + "strings" "github.com/coder/coder/v2/codersdk" "github.com/google/uuid" @@ -110,5 +111,15 @@ func memberDiff(currentMembers []uuid.UUID, plannedMembers []UUID) (add, remove func isNotFound(err error) bool { var sdkErr *codersdk.Error - return errors.As(err, &sdkErr) && sdkErr.StatusCode() == http.StatusNotFound + if !errors.As(err, &sdkErr) { + return false + } + if sdkErr.StatusCode() == http.StatusNotFound { + return true + } + // `httpmw/ExtractUserContext` returns a 400 w/ this message if the user is not found + if sdkErr.StatusCode() == http.StatusBadRequest && strings.Contains(sdkErr.Message, "must be an existing uuid or username") { + return true + } + return false } From 24990cbea4dde2db8f175d1f9a3f7938fba5843a Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Tue, 15 Apr 2025 02:28:27 +0000 Subject: [PATCH 2/2] review --- internal/provider/user_resource.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/internal/provider/user_resource.go b/internal/provider/user_resource.go index b184d31..79d248d 100644 --- a/internal/provider/user_resource.go +++ b/internal/provider/user_resource.go @@ -252,11 +252,11 @@ func (r *UserResource) Read(ctx context.Context, req resource.ReadRequest, resp user, err := client.User(ctx, data.ID.ValueString()) if err != nil { if isNotFound(err) { - resp.Diagnostics.AddWarning("Client Warning", fmt.Sprintf("User with ID %q not found. Marking as deleted.", data.ID.ValueString())) + resp.Diagnostics.AddWarning("Client Warning", fmt.Sprintf("User with ID %q not found. Marking resource as deleted.", data.ID.ValueString())) resp.State.RemoveResource(ctx) return } - resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to get current user, got error: %s", err)) + resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to get current user by ID, got error: %s", err)) return } if len(user.OrganizationIDs) < 1 { @@ -275,19 +275,26 @@ func (r *UserResource) Read(ctx context.Context, req resource.ReadRequest, resp data.LoginType = types.StringValue(string(user.LoginType)) data.Suspended = types.BoolValue(user.Status == codersdk.UserStatusSuspended) - // Also query by username to check for deletion or username reassignment + // The user-by-ID API returns deleted users if the authorized user has + // permission. It does not indicate whether the user is deleted or not. + // The user-by-username API will never return deleted users. + // So, we do another lookup by username. userByName, err := client.User(ctx, data.Username.ValueString()) if err != nil { if isNotFound(err) { - resp.Diagnostics.AddWarning("Client Warning", fmt.Sprintf("User with ID %q not found. Marking as deleted.", data.ID.ValueString())) + resp.Diagnostics.AddWarning("Client Warning", fmt.Sprintf( + "User with username %q not found. Marking resource as deleted.", + data.Username.ValueString())) resp.State.RemoveResource(ctx) return } - resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to get current user, got error: %s", err)) + resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to get current user by username, got error: %s", err)) return } if userByName.ID != data.ID.ValueUUID() { - resp.Diagnostics.AddWarning("Client Error", fmt.Sprintf("The username %q has been reassigned to a new user. Marking as deleted.", user.Username)) + resp.Diagnostics.AddWarning("Client Error", fmt.Sprintf( + "The username %q has been reassigned to a new user not managed by this Terraform resource. Marking resource as deleted.", + user.Username)) resp.State.RemoveResource(ctx) return }