Allow recreation of recently deleted project and org custom roles #1681

emilymye · 2018-06-20T00:02:28Z

Undelete-update recently soft-deleted roles instead of giving error

danawillow · 2018-06-22T00:28:38Z

google/resource_google_organization_iam_custom_role.go

+		}
+	}
+
+	// If role is not deleted, make sure it exists and undelete if needed.


Because this gets called on create, the next bit about d.HasChange will actually return true and send a patch request. It's not a huge deal, but it's still an extra API call that we don't really need. If it's not hard, I think the Undelete should probably just get called straight from Create.

I might be confused - isn't that what we want? If this gets called on create, what we want is the 1. role.undelete 2. role.patch. If we just call undelete then we still have to call patch to essentially reset the previously deleted role to whatever value we are trying to create.

paddycarver · 2018-06-22T19:09:22Z

The context of this one scares me, but on the assumption that we can update roles as if they were just now created (i.e., all user-specifiable fields can be updated), I can't see why not.

Projects, on the other hand (I'm not sure if those are intended, but as they have a similar soft-delete, I just want to be explicit about this) I'd argue against taking this approach with, because there are a lot of things about a project that would need to be cleaned up, and some of them can't be cleaned up, so we'd be saying the user has a fresh project when they, in fact, do not.

Anyways, just wanted to call that subtlety out. I'll take a pass at reviewing the code now :)

paddycarver · 2018-06-22T19:21:37Z

google/resource_google_organization_iam_custom_role_test.go

+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckGoogleOrganizationIamCustomRoleDestroy,
+		Steps: []resource.TestStep{


This is going to sound really nitpicky, but could we verify that recreating it gets us the results we expect? I'd just hate for it to do the deletion regularly, only for us to find out its failing to set the other properties, or something.

paddycarver · 2018-06-22T19:22:15Z

google/resource_google_project_iam_custom_role.go

+		}
+	}
+
+	// If role is not deleted, make sure it exists and undelete if needed.


Same comment Dana pointed out about the unnecessary PATCH applies here, I think.

paddycarver · 2018-06-22T19:23:24Z

google/resource_google_project_iam_custom_role_test.go

+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckGoogleProjectIamCustomRoleDestroy,
+		Steps: []resource.TestStep{


Same thing here about verifying the resource has the expected values.

morgante · 2018-06-25T17:26:04Z

Projects, on the other hand (I'm not sure if those are intended, but as they have a similar soft-delete, I just want to be explicit about this) I'd argue against taking this approach with, because there are a lot of things about a project that would need to be cleaned up, and some of them can't be cleaned up, so we'd be saying the user has a fresh project when they, in fact, do not.

I want to echo that this is potentially very dangerous. There's a lot of undefined behavior with project recreation and this could cause more bugs than it fixes IMO.

emilymye · 2018-06-25T19:04:32Z

Looking at the IAM custom roles docs, it seems like there are two stages of deletion for custom roles:

Stage 1, 0-7 days after deletion: The role can be undeleted but otherwise cannot be used.
Stage 2, 7-37 days after deletion: The role cannot be undeleted but the role ID cannot be used to make a new role. I'm not sure whether calling roles.get() during this stage will return anything.

There are a couple of bugs with the current implementation.

Currently roles can be deleted through either setting role.deleted=true or terraform destroy. Problems with this:

If the user runs terraform destroy and terraform apply within 37 days without changing the custom role ID, this will error as shown in issue Terraform doesn't undelete Custom roles on apply #1668
If the user makes change in config deleted=true, the role is deleted in GCP. role.get() returns a value for at least 7 days, no changes can be made to this value other than role.undelete(). That means running terraform apply with any other changed attributes will fail because it attempts to patch a deleted role. Running terraform destroy will fail because it attempts to redelete a deleted object.

IMO deleted should be a computed value - I don't think deleted roles should be treated as existing in terraform state at any point. This is also a fairly large breaking change so we probably can't make this change right away.

What I think this PR should do is:

If Role in Stage 1: When user runs terraform apply, undelete and patch to have role/$ROLE_ID be the expected value from Terraform state. This is regardless of whether the terraform plan is running ResourceCreate (e.g. apply after destroy) or ResourceUpdate (if the state has or was refreshed and found role deleted=true).
If Role in Stage 2: If user runs terraform apply, return an error. We can't do anything about this, so the user pretty much has to abandon that role id. If user runs terraform destroy,

In either case, I'll need to add a check to ResourceDelete so terraform destroy just ignores roles if the role has 'deleted=true' or if role.get() returns nothing. I'm not sure what reading the role returns at during stage 2 - I'll need to find someone to ask in IAM or find a role that has been deleted for > 7 days but < 37 days, neither of which give timely answers.

tl;dr: @paddycarver @danawillow: I think the current solution should run undelete-patch for either create or update, and we should make deleted Computed. Also, I need to add a check for deletion.

danawillow · 2018-06-25T21:57:24Z

@paddycarver @morgante why is project deletion relevant here? This PR is just about custom roles.

morgante · 2018-06-25T22:39:06Z

@danawillow I parsed the title as "Allow recreation of recently deleted (projects) and (org custom roles)" but now realize it's "Allow recreation of recently deleted (project and org) custom roles" so retract my comments. 😄

paddycarver · 2018-06-25T23:06:42Z

Parsed the title similarly to @morgante, and am also wary of setting precedent. :/

I'm still parsing and processing @emilymye's awesome comment, to try and wrap my brain about this, then I'll take another review pass.

emilymye · 2018-06-26T00:46:48Z

@paddycarver @danawillow I made some new changes that I think (think??) will not cause breakage and actually handle all errors. I added a deprecation message for deleted (as advised by Dana) and tried to make the Create/Update logic easier to follow, but pls re-review.

Also, I don't know if I have access to the team city still but can someone run the org tests for me? I need to set that up -__-

project test run:
TF_ACC=1 go test ./google -v --run="TestAccProjectIamCustomRole" -timeout 120m
=== RUN TestAccProjectIamCustomRole_import
=== PAUSE TestAccProjectIamCustomRole_import
=== RUN TestAccProjectIamCustomRole_basic
=== PAUSE TestAccProjectIamCustomRole_basic
=== RUN TestAccProjectIamCustomRole_undelete
=== PAUSE TestAccProjectIamCustomRole_undelete
=== RUN TestAccProjectIamCustomRole_createAfterDestroy
=== PAUSE TestAccProjectIamCustomRole_createAfterDestroy
=== CONT TestAccProjectIamCustomRole_import
=== CONT TestAccProjectIamCustomRole_undelete
=== CONT TestAccProjectIamCustomRole_createAfterDestroy
=== CONT TestAccProjectIamCustomRole_basic
--- PASS: TestAccProjectIamCustomRole_import (2.24s)
--- PASS: TestAccProjectIamCustomRole_basic (3.54s)
--- PASS: TestAccProjectIamCustomRole_undelete (3.84s)
--- PASS: TestAccProjectIamCustomRole_createAfterDestroy (4.14s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google 4.313s

paddycarver · 2018-06-26T00:59:18Z

I'll take a look and run the tests tonight, hopefully. In the meantime, I also made sure you have access to the CI server. Feel free to drop an email to paddy@hashicorp if you're having trouble, and I can give you more info.

danawillow · 2018-06-27T18:02:46Z

google/resource_google_organization_iam_custom_role.go

+			// If a role with same name exists and is enabled, just return error
+			return fmt.Errorf("Custom project role %s already exists and must be imported", roleId)
+		}
+	} else {


Do you want to check that the error was actually a 404?

paddycarver

Tests pass, and I'm mostly really happy with this. Awesome work, @emilymye! The only thing I'd ask is just, as @danawillow pointed out, that we check the error is actually a 404 and not some other error. Otherwise, I think it's good to merge.

emilymye · 2018-08-27T23:48:37Z

WHOOPS I forgot about this PR

Updates made, also added a warning to docs so custom roles might be less confusing in general. This is a real pickle

$ make testacc TEST=./google TESTARGS='--run="TestAccProjectIamCustomRole"'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google -v --run="TestAccProjectIamCustomRole" -timeout 120m
=== RUN TestAccProjectIamCustomRole_import
=== PAUSE TestAccProjectIamCustomRole_import
=== RUN TestAccProjectIamCustomRole_basic
=== PAUSE TestAccProjectIamCustomRole_basic
=== RUN TestAccProjectIamCustomRole_undelete
=== PAUSE TestAccProjectIamCustomRole_undelete
=== RUN TestAccProjectIamCustomRole_createAfterDestroy
=== PAUSE TestAccProjectIamCustomRole_createAfterDestroy
=== CONT TestAccProjectIamCustomRole_import
=== CONT TestAccProjectIamCustomRole_createAfterDestroy
=== CONT TestAccProjectIamCustomRole_undelete
=== CONT TestAccProjectIamCustomRole_basic
--- PASS: TestAccProjectIamCustomRole_import (2.78s)
--- PASS: TestAccProjectIamCustomRole_basic (3.79s)
--- PASS: TestAccProjectIamCustomRole_createAfterDestroy (4.19s)
--- PASS: TestAccProjectIamCustomRole_undelete (4.49s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google 4.672s

… 'create'

…shicorp#1681) * undelete-update recently soft-deleted custom roles * remove my TODO statements * check values on soft-delete-recreate for custom role tests * final fixes to make sure delete works; return read() when updating to 'create' * check for non-404 errors for custom role get * add warnings to custom roles docs

ghost · 2018-11-16T14:19:04Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

emilymye requested review from nat-henderson and danawillow June 20, 2018 00:02

danawillow reviewed Jun 22, 2018

View reviewed changes

paddycarver suggested changes Jun 22, 2018

View reviewed changes

emilymye removed the request for review from nat-henderson June 25, 2018 17:42

danawillow approved these changes Jun 25, 2018

View reviewed changes

danawillow reviewed Jun 27, 2018

View reviewed changes

paddycarver added the enhancement label Jul 6, 2018

paddycarver suggested changes Jul 6, 2018

View reviewed changes

nat-henderson assigned emilymye Jul 26, 2018

emilymye force-pushed the custom_roles branch from 2fd16dd to b36397c Compare August 27, 2018 23:35

ghost added the size/l label Aug 27, 2018

paddycarver approved these changes Aug 30, 2018

View reviewed changes

emilymye added 6 commits September 10, 2018 10:26

undelete-update recently soft-deleted custom roles

9668f6b

remove my TODO statements

3e70126

check values on soft-delete-recreate for custom role tests

29bf2d1

final fixes to make sure delete works; return read() when updating to…

8adf8d8

… 'create'

check for non-404 errors for custom role get

6076e6e

add warnings to custom roles docs

2f80af9

emilymye force-pushed the custom_roles branch from 2fc22d6 to 2f80af9 Compare September 10, 2018 17:27

ghost added the size/l label Sep 10, 2018

emilymye merged commit b1338b4 into hashicorp:master Sep 10, 2018

ghost locked and limited conversation to collaborators Nov 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow recreation of recently deleted project and org custom roles #1681

Allow recreation of recently deleted project and org custom roles #1681

emilymye commented Jun 20, 2018

danawillow Jun 22, 2018

emilymye Jun 25, 2018

paddycarver commented Jun 22, 2018

paddycarver Jun 22, 2018

emilymye Jun 25, 2018

paddycarver Jun 22, 2018

paddycarver Jun 22, 2018

morgante commented Jun 25, 2018 •

edited

Loading

emilymye commented Jun 25, 2018

danawillow commented Jun 25, 2018

morgante commented Jun 25, 2018 •

edited

Loading

paddycarver commented Jun 25, 2018

emilymye commented Jun 26, 2018

paddycarver commented Jun 26, 2018

danawillow Jun 27, 2018

paddycarver left a comment

emilymye commented Aug 27, 2018

ghost commented Nov 16, 2018

Allow recreation of recently deleted project and org custom roles #1681

Allow recreation of recently deleted project and org custom roles #1681

Conversation

emilymye commented Jun 20, 2018

danawillow Jun 22, 2018

Choose a reason for hiding this comment

emilymye Jun 25, 2018

Choose a reason for hiding this comment

paddycarver commented Jun 22, 2018

paddycarver Jun 22, 2018

Choose a reason for hiding this comment

emilymye Jun 25, 2018

Choose a reason for hiding this comment

paddycarver Jun 22, 2018

Choose a reason for hiding this comment

paddycarver Jun 22, 2018

Choose a reason for hiding this comment

morgante commented Jun 25, 2018 • edited Loading

emilymye commented Jun 25, 2018

danawillow commented Jun 25, 2018

morgante commented Jun 25, 2018 • edited Loading

paddycarver commented Jun 25, 2018

emilymye commented Jun 26, 2018

paddycarver commented Jun 26, 2018

danawillow Jun 27, 2018

Choose a reason for hiding this comment

paddycarver left a comment

Choose a reason for hiding this comment

emilymye commented Aug 27, 2018

ghost commented Nov 16, 2018

morgante commented Jun 25, 2018 •

edited

Loading

morgante commented Jun 25, 2018 •

edited

Loading