Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deal with undeletable bucket ACLs in storage. #439

Merged
merged 4 commits into from
Sep 25, 2017
Merged

Deal with undeletable bucket ACLs in storage. #439

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7211463
Deal with undeleatable bucket ACLs in storage.
paddycarver Sep 15, 2017
755edf6
Fix make test.
paddycarver Sep 15, 2017
b343829
Stop fighting with the test framework.
paddycarver Sep 15, 2017
5060f07
Pull ProjectNumber off bucket, not getProject.
paddycarver Sep 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 49 additions & 8 deletions google/resource_storage_bucket_acl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package google
import (
"fmt"
"log"
"strconv"
"strings"

"github.com/hashicorp/terraform/helper/schema"
Expand Down Expand Up @@ -101,9 +102,26 @@ func resourceStorageBucketAclCreate(d *schema.ResourceData, meta interface{}) er

}
if len(role_entity) > 0 {
current, err := config.clientStorage.BucketAccessControls.List(bucket).Do()
if err != nil {
return fmt.Errorf("Error retrieving current ACLs: %s", err)
}
for _, v := range role_entity {
pair, err := getRoleEntityPair(v.(string))

if err != nil {
return err
}
var alreadyInserted bool
for _, cur := range current.Items {
if cur.Entity == pair.Entity && cur.Role == pair.Role {
alreadyInserted = true
break
}
}
if alreadyInserted {
log.Printf("[DEBUG]: pair %s-%s already exists, not trying to insert again\n", pair.Role, pair.Entity)
continue
}
bucketAccessControl := &storage.BucketAccessControl{
Role: pair.Role,
Entity: pair.Entity,
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err != nil {
return err
}

if d.HasChange("role_entity") {
p, err := config.clientResourceManager.Projects.Get(project).Do()
if err != nil {
return fmt.Errorf("Error retrieving project %q: %s", project, err)
}
o, n := d.GetChange("role_entity")
old_re, new_re := o.([]interface{}), n.([]interface{})

Expand All @@ -197,12 +223,8 @@ func resourceStorageBucketAclUpdate(d *schema.ResourceData, meta interface{}) er
Entity: pair.Entity,
}

// If the old state is missing this entity, it needs to
// be created. Otherwise it is updated
if _, ok := old_re_map[pair.Entity]; ok {
_, err = config.clientStorage.BucketAccessControls.Update(
bucket, pair.Entity, bucketAccessControl).Do()
} else {
// If the old state is missing this entity, it needs to be inserted
if _, ok := old_re_map[pair.Entity]; !ok {
_, err = config.clientStorage.BucketAccessControls.Insert(
bucket, bucketAccessControl).Do()
}
Expand All @@ -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" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

log.Printf("Skipping %s-%s; not deleting owner ACL.", role, entity)
continue
}
log.Printf("[DEBUG]: removing entity %s", entity)
err := config.clientStorage.BucketAccessControls.Delete(bucket, entity).Do()

Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above re: which project owns the bucket

if err != nil {
return err
}

bucket := d.Get("bucket").(string)

p, err := config.clientResourceManager.Projects.Get(project).Do()
if err != nil {
return fmt.Errorf("Error retrieving project %q: %s", project, err)
}

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" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above re: string concat

log.Printf("Skipping %s-%s; not deleting owner ACL.", res.Role, res.Entity)
continue
}

log.Printf("[DEBUG]: removing entity %s", res.Entity)

err = config.clientStorage.BucketAccessControls.Delete(bucket, res.Entity).Do()
Expand Down
32 changes: 19 additions & 13 deletions google/resource_storage_bucket_acl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,32 @@ package google

import (
"fmt"
"os"
"testing"

"github.com/hashicorp/terraform/helper/acctest"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/terraform"
//"google.golang.org/api/storage/v1"
)

var roleEntityBasic1 = "OWNER:[email protected]"
var (
roleEntityBasic1 = "OWNER:[email protected]"
roleEntityBasic2 = "READER:[email protected]"
roleEntityBasic3_owner = "OWNER:[email protected]"
roleEntityBasic3_reader = "READER:[email protected]"

var roleEntityBasic2 = "READER:[email protected]"

var roleEntityBasic3_owner = "OWNER:[email protected]"

var roleEntityBasic3_reader = "READER:[email protected]"
roleEntityOwners = "OWNER:project-owners-" + os.Getenv("GOOGLE_PROJECT_NUMBER")
roleEntityEditors = "OWNER:project-editors-" + os.Getenv("GOOGLE_PROJECT_NUMBER")
roleEntityViewers = "READER:project-viewers-" + os.Getenv("GOOGLE_PROJECT_NUMBER")
)

func testBucketName() string {
return fmt.Sprintf("%s-%d", "tf-test-acl-bucket", acctest.RandInt())
}

func TestAccGoogleStorageBucketAcl_basic(t *testing.T) {
bucketName := testBucketName()
skipIfEnvNotSet(t, "GOOGLE_PROJECT_NUMBER")
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Expand All @@ -42,6 +46,7 @@ func TestAccGoogleStorageBucketAcl_basic(t *testing.T) {

func TestAccGoogleStorageBucketAcl_upgrade(t *testing.T) {
bucketName := testBucketName()
skipIfEnvNotSet(t, "GOOGLE_PROJECT_NUMBER")
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Expand Down Expand Up @@ -77,6 +82,7 @@ func TestAccGoogleStorageBucketAcl_upgrade(t *testing.T) {

func TestAccGoogleStorageBucketAcl_downgrade(t *testing.T) {
bucketName := testBucketName()
skipIfEnvNotSet(t, "GOOGLE_PROJECT_NUMBER")
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Expand Down Expand Up @@ -186,9 +192,9 @@ resource "google_storage_bucket" "bucket" {

resource "google_storage_bucket_acl" "acl" {
bucket = "${google_storage_bucket.bucket.name}"
role_entity = ["%s", "%s"]
role_entity = ["%s", "%s", "%s", "%s", "%s"]
}
`, bucketName, roleEntityBasic1, roleEntityBasic2)
`, bucketName, roleEntityOwners, roleEntityEditors, roleEntityViewers, roleEntityBasic1, roleEntityBasic2)
}

func testGoogleStorageBucketsAclBasic2(bucketName string) string {
Expand All @@ -199,9 +205,9 @@ resource "google_storage_bucket" "bucket" {

resource "google_storage_bucket_acl" "acl" {
bucket = "${google_storage_bucket.bucket.name}"
role_entity = ["%s", "%s"]
role_entity = ["%s", "%s", "%s", "%s", "%s"]
}
`, bucketName, roleEntityBasic2, roleEntityBasic3_owner)
`, bucketName, roleEntityOwners, roleEntityEditors, roleEntityViewers, roleEntityBasic2, roleEntityBasic3_owner)
}

func testGoogleStorageBucketsAclBasicDelete(bucketName string) string {
Expand All @@ -225,9 +231,9 @@ resource "google_storage_bucket" "bucket" {

resource "google_storage_bucket_acl" "acl" {
bucket = "${google_storage_bucket.bucket.name}"
role_entity = ["%s", "%s"]
role_entity = ["%s", "%s", "%s", "%s", "%s"]
}
`, bucketName, roleEntityBasic2, roleEntityBasic3_reader)
`, bucketName, roleEntityOwners, roleEntityEditors, roleEntityViewers, roleEntityBasic2, roleEntityBasic3_reader)
}

func testGoogleStorageBucketsAclPredefined(bucketName string) string {
Expand Down