Deal with undeletable bucket ACLs in storage. #439

paddycarver · 2017-09-15T19:10:15Z

When GCS buckets are created, they're created with a set of default
ACLs:

OWNER:project-owners-{project_number}
OWNER:project-editors-{project_number}
READER:project-viewers-{project_number}

Normally, this would be fine, or a minor inconvenience. Terraform could
either delete them itself, or the first apply of a user would overwrite
them.

However, trying to remove the OWNER:project-owners-{project_number}
ACL yields an API error that the bucket owner must maintain OWNER access
to the bucket. This breaks things like terraform destroy, but also
means any config without that line in it will fail to apply, not just
overwrite the value.

To make matters worse, trying to add the
OWNER:project-owners-{project_number} ACL to any bucket that already
has it also yields the same error about not being able to remove it.

To get around this, the storage_bucket_acl resource has been updated to
largely ignore just this ACL. It will not try to add it if it already
exists, will not try to remove it at all. This does mean that Terraform
is incapable of removing this ACL from a bucket, but I'm not sure it's
possible to do that with the API, anyways.

Tests were also updated to keep the default ACLs as part of the config,
and to change the email addresses to addresses we actually own. I tried
changing to non-existent hashicorp.com email addresses, but was
rejected; only email addresses that are backed by actual Google accounts
can be used, sadly.

This fixes the broken storage bucket ACL tests.

When GCS buckets are created, they're created with a set of default ACLs: * `OWNER:project-owners-{project_number}` * `OWNER:project-editors-{project_number}` * `READER:project-viewers-{project_number}` Normally, this would be fine, or a minor inconvenience. Terraform could either delete them itself, or the first apply of a user would overwrite them. However, trying to remove the `OWNER:project-owners-{project_number}` ACL yields an API error that the bucket owner must maintain OWNER access to the bucket. This breaks things like `terraform destroy`, but also means any config without that line in it will fail to apply, not just overwrite the value. To make matters worse, trying to *add* the `OWNER:project-owners-{project_number}` ACL to any bucket that already has it _also_ yields the same error about not being able to remove it. To get around this, the storage_bucket_acl resource has been updated to largely ignore _just this_ ACL. It will not try to add it if it already exists, will not try to remove it at all. This does mean that Terraform is incapable of removing this ACL from a bucket, but I'm not sure it's possible to do that with the API, anyways. Tests were also updated to keep the default ACLs as part of the config, and to change the email addresses to addresses we actually own. I tried changing to non-existant hashicorp.com email addresses, but was rejected; only email addresses that are backed by actual Google accounts can be used, sadly.

Because we were instantiating a client outside of resource.TestCase, it was being instantiated even for unit tests, which have no credentials, causing the unit tests to fail. Sadly, this is the only way I could figure out how to get a client inside resource.TestCase, which is very sad making, but works.

Project number is now set through an environment variable instead of being inferred at runtime using the API.

paddycarver · 2017-09-15T19:56:35Z

This is ready for review now, now that I've finished arguing with the test framework :( Test strategy isn't as clean as I'd like it to be, but them's the breaks.

selmanj · 2017-09-15T21:46:28Z

google/resource_storage_bucket_acl.go

@@ -172,8 +190,16 @@ func resourceStorageBucketAclUpdate(d *schema.ResourceData, meta interface{}) er
 	config := meta.(*Config)

 	bucket := d.Get("bucket").(string)
+	project, err := getProject(d, config)


technically this might not match the project that owns the bucket, right? If someone is managing a bucket in terraform that doesn't belong to their provider's configured project this might cause issues...

Fixing this might be too painful right now so if what I said is true, I'm fine with just acknowledging it with a TODO

This is a good point, and is actually easier to implement; we can just read the bucket from the API, and pull the project number off it. Great point, and thanks for pointing it out. I like that solution much better.

I didn't realize the project id is on the bucket resource; that makes this change a lot easier

selmanj · 2017-09-15T21:48:13Z

google/resource_storage_bucket_acl.go

@@ -252,15 +278,30 @@ func resourceStorageBucketAclUpdate(d *schema.ResourceData, meta interface{}) er
 func resourceStorageBucketAclDelete(d *schema.ResourceData, meta interface{}) error {
 	config := meta.(*Config)

+	project, err := getProject(d, config)


Same comment as above re: which project owns the bucket

selmanj · 2017-09-15T21:49:23Z

google/resource_storage_bucket_acl.go

@@ -215,7 +237,11 @@ func resourceStorageBucketAclUpdate(d *schema.ResourceData, meta interface{}) er
 			}
 		}

-		for entity, _ := range old_re_map {
+		for entity, role := range old_re_map {
+			if entity == "project-owners-"+strconv.FormatInt(p.ProjectNumber, 10) && role == "OWNER" {


Would it be easier to read to do fmt.Sprintf("project-owners-%d", p.ProjectNumber) instead of string concatenating strconv.FormatInt?

Fair, I'll do that. Sorry, I'm just used to the non-reflect-based string construction methods from an old performance-sensitive project, and now they're reflex. Thanks for catching it :)

selmanj · 2017-09-15T21:50:23Z

google/resource_storage_bucket_acl.go

 	re_local := d.Get("role_entity").([]interface{})
 	for _, v := range re_local {
 		res, err := getRoleEntityPair(v.(string))
 		if err != nil {
 			return err
 		}

+		if res.Entity == "project-owners-"+strconv.FormatInt(p.ProjectNumber, 10) && res.Role == "OWNER" {


Same as above re: string concat

Use the project reported by the bucket, not the one Terraform is configured to use.

paddycarver · 2017-09-25T13:03:33Z

Updated, all tests pass, concerns were addressed.

selmanj

👍

…etable_bucket_acls Deal with undeletable bucket ACLs in storage.

ghost · 2018-11-16T13:53:37Z

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!

paddycarver requested review from selmanj and rosbo September 15, 2017 19:10

paddycarver added 2 commits September 15, 2017 12:28

Stop fighting with the test framework.

b343829

Project number is now set through an environment variable instead of being inferred at runtime using the API.

selmanj reviewed Sep 15, 2017

View reviewed changes

Pull ProjectNumber off bucket, not getProject.

5060f07

Use the project reported by the bucket, not the one Terraform is configured to use.

paddycarver mentioned this pull request Sep 25, 2017

google_storage_bucket_acl tries to delete owner access #428

Closed

selmanj approved these changes Sep 25, 2017

View reviewed changes

paddycarver merged commit c31bfdc into master Sep 25, 2017

negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017

Merge pull request hashicorp#439 from terraform-providers/paddy_undel…

1d5a4f9

…etable_bucket_acls Deal with undeletable bucket ACLs in storage.

paultyng deleted the paddy_undeletable_bucket_acls branch October 29, 2018 19:29

ghost locked and limited conversation to collaborators Nov 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deal with undeletable bucket ACLs in storage. #439

Deal with undeletable bucket ACLs in storage. #439

paddycarver commented Sep 15, 2017 •

edited

Loading

paddycarver commented Sep 15, 2017

selmanj Sep 15, 2017

paddycarver Sep 16, 2017

selmanj Sep 17, 2017

selmanj Sep 15, 2017

selmanj Sep 15, 2017

paddycarver Sep 16, 2017

selmanj Sep 15, 2017

paddycarver commented Sep 25, 2017

selmanj left a comment

ghost commented Nov 16, 2018

Deal with undeletable bucket ACLs in storage. #439

Deal with undeletable bucket ACLs in storage. #439

Conversation

paddycarver commented Sep 15, 2017 • edited Loading

paddycarver commented Sep 15, 2017

selmanj Sep 15, 2017

Choose a reason for hiding this comment

paddycarver Sep 16, 2017

Choose a reason for hiding this comment

selmanj Sep 17, 2017

Choose a reason for hiding this comment

selmanj Sep 15, 2017

Choose a reason for hiding this comment

selmanj Sep 15, 2017

Choose a reason for hiding this comment

paddycarver Sep 16, 2017

Choose a reason for hiding this comment

selmanj Sep 15, 2017

Choose a reason for hiding this comment

paddycarver commented Sep 25, 2017

selmanj left a comment

Choose a reason for hiding this comment

ghost commented Nov 16, 2018

paddycarver commented Sep 15, 2017 •

edited

Loading