Skip to content

Commit

Permalink
Deal with undeleatable bucket ACLs in storage.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@paddycarver
paddycarver committed Sep 15, 2017
1 parent b694d5a commit 7211463
Show file tree
Hide file tree
Showing 2 changed files with 105 additions and 29 deletions.
57 changes: 49 additions & 8 deletions google/resource_storage_bucket_acl.go
View file
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)
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" {
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)
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" {
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
77 changes: 56 additions & 21 deletions google/resource_storage_bucket_acl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,35 +2,62 @@ package google

import (
"fmt"
"strconv"
"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 roleEntityBasic2 = "READER:[email protected]"

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

var roleEntityBasic3_reader = "READER:[email protected]"
func defaultRoleEntities() ([]string, error) {
creds := multiEnvSearch(credsEnvVars)
project := multiEnvSearch(projectEnvVars)
region := multiEnvSearch(regionEnvVars)
config := Config{
Credentials: creds,
Project: project,
Region: region,
}
if err := config.loadAndValidate(); err != nil {
return nil, fmt.Errorf("Error setting up client: %s", err)
}
p, err := config.clientResourceManager.Projects.Get(project).Do()
if err != nil {
return nil, fmt.Errorf("Error retrieving test project %q: %s", project, err)
}
projectNumber := p.ProjectNumber
return []string{
"OWNER:project-owners-" + strconv.FormatInt(projectNumber, 10),
"OWNER:project-editors-" + strconv.FormatInt(projectNumber, 10),
"READER:project-viewers-" + strconv.FormatInt(projectNumber, 10),
}, nil
}

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

func TestAccGoogleStorageBucketAcl_basic(t *testing.T) {
bucketName := testBucketName()
entities, err := defaultRoleEntities()
if err != nil {
t.Fatal(err)
}
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccGoogleStorageBucketAclDestroy,
Steps: []resource.TestStep{
resource.TestStep{
Config: testGoogleStorageBucketsAclBasic1(bucketName),
Config: testGoogleStorageBucketsAclBasic1(bucketName, entities),
Check: resource.ComposeTestCheckFunc(
testAccCheckGoogleStorageBucketAcl(bucketName, roleEntityBasic1),
testAccCheckGoogleStorageBucketAcl(bucketName, roleEntityBasic2),
Expand All @@ -42,21 +69,25 @@ func TestAccGoogleStorageBucketAcl_basic(t *testing.T) {

func TestAccGoogleStorageBucketAcl_upgrade(t *testing.T) {
bucketName := testBucketName()
entities, err := defaultRoleEntities()
if err != nil {
t.Fatal(err)
}
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccGoogleStorageBucketAclDestroy,
Steps: []resource.TestStep{
resource.TestStep{
Config: testGoogleStorageBucketsAclBasic1(bucketName),
Config: testGoogleStorageBucketsAclBasic1(bucketName, entities),
Check: resource.ComposeTestCheckFunc(
testAccCheckGoogleStorageBucketAcl(bucketName, roleEntityBasic1),
testAccCheckGoogleStorageBucketAcl(bucketName, roleEntityBasic2),
),
},

resource.TestStep{
Config: testGoogleStorageBucketsAclBasic2(bucketName),
Config: testGoogleStorageBucketsAclBasic2(bucketName, entities),
Check: resource.ComposeTestCheckFunc(
testAccCheckGoogleStorageBucketAcl(bucketName, roleEntityBasic2),
testAccCheckGoogleStorageBucketAcl(bucketName, roleEntityBasic3_owner),
Expand All @@ -77,21 +108,25 @@ func TestAccGoogleStorageBucketAcl_upgrade(t *testing.T) {

func TestAccGoogleStorageBucketAcl_downgrade(t *testing.T) {
bucketName := testBucketName()
entities, err := defaultRoleEntities()
if err != nil {
t.Fatal(err)
}
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccGoogleStorageBucketAclDestroy,
Steps: []resource.TestStep{
resource.TestStep{
Config: testGoogleStorageBucketsAclBasic2(bucketName),
Config: testGoogleStorageBucketsAclBasic2(bucketName, entities),
Check: resource.ComposeTestCheckFunc(
testAccCheckGoogleStorageBucketAcl(bucketName, roleEntityBasic2),
testAccCheckGoogleStorageBucketAcl(bucketName, roleEntityBasic3_owner),
),
},

resource.TestStep{
Config: testGoogleStorageBucketsAclBasic3(bucketName),
Config: testGoogleStorageBucketsAclBasic3(bucketName, entities),
Check: resource.ComposeTestCheckFunc(
testAccCheckGoogleStorageBucketAcl(bucketName, roleEntityBasic2),
testAccCheckGoogleStorageBucketAcl(bucketName, roleEntityBasic3_reader),
Expand Down Expand Up @@ -178,30 +213,30 @@ func testAccGoogleStorageBucketAclDestroy(s *terraform.State) error {
return nil
}

func testGoogleStorageBucketsAclBasic1(bucketName string) string {
func testGoogleStorageBucketsAclBasic1(bucketName string, entities []string) string {
return fmt.Sprintf(`
resource "google_storage_bucket" "bucket" {
name = "%s"
}
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, entities[0], entities[1], entities[2], roleEntityBasic1, roleEntityBasic2)
}

func testGoogleStorageBucketsAclBasic2(bucketName string) string {
func testGoogleStorageBucketsAclBasic2(bucketName string, entities []string) string {
return fmt.Sprintf(`
resource "google_storage_bucket" "bucket" {
name = "%s"
}
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, entities[0], entities[1], entities[2], roleEntityBasic2, roleEntityBasic3_owner)
}

func testGoogleStorageBucketsAclBasicDelete(bucketName string) string {
Expand All @@ -217,17 +252,17 @@ resource "google_storage_bucket_acl" "acl" {
`, bucketName)
}

func testGoogleStorageBucketsAclBasic3(bucketName string) string {
func testGoogleStorageBucketsAclBasic3(bucketName string, entities []string) string {
return fmt.Sprintf(`
resource "google_storage_bucket" "bucket" {
name = "%s"
}
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, entities[0], entities[1], entities[2], roleEntityBasic2, roleEntityBasic3_reader)
}

func testGoogleStorageBucketsAclPredefined(bucketName string) string {
Expand Down

0 comments on commit 7211463

Please sign in to comment.