Google bucket should allow update of predefined_acl

[sparkprime]

This is possible via the GCE API's patch call.

No pressing need for this, but logging the issue so it doesn't get forgotten.

[sparkprime]

Summoning: @phinze @mtekel @dhilton

Ok so I had a go at this but there are number of potentially insurmountable challenges:

1. In the APIs, predefined_acl is really just syntax sugar -- the backend quickly expands it into low level acls and then when you query the API it doesn't tell you that the user gave a predefined acl, you just get the actual acls out. I paste the mapping below, which I derived by doing exactly this today, for each predefined_acl value. This makes diffing pretty hard. One answer is to simply not update that attribute in Terraform's Read() callback, which is sleazy and has the effect of not detecting drift. This is what the current code actually does. You can I think just about get away with that in a ForceNew attribute because maybe we don't want to detect drift there. But it's less defensible in a fully updateable attribute.

2. The ACLs mapped down from these predefined acls are often insufficient to allow Terraform to manage the bucket. Note that this really means modifying ACLs, as Terraform will always be able to delete the bucket and even manage the objects in it as you don't need to be an owner to do that. Google will ensure the project owner can continue to have owner privileges on the bucket by adding that ACL even if you don't specify it. But Terraform uses service accounts to authenticate, and these are only project editors, not project owners. If you look at the list below, you'll notice that only project-private promotes project-editors to the role OWNER for the bucket. All of the other ones will exclude Terraform from modifying the Bucket ACLs, which will generate 403s. That's not good.

So what to do about this? Here are some policy assumptions I'm making, feel free to tell me they're wrong:

1. We want to keep predefined_acl for backwards compatibility reasons and because for most people at least some of those values are probably reasonable.
2. We do want to be able to modify fine grained acls
3. We do want to be able to detect and correct "drift" in fine-grained acls

Here is a plan:

1. predefined_acl remains ForceNew and will not correct drift
2. acl is an array of fine grained acls and is not ForceNew
3. The user may not specify both predefined_acl and acl.

Read() will not attempt to update predefined_acl
Read() will ignore the acls returned from Google Cloud Storage if predefined_acl is set
=> This means there will be no drift detection when using predefined_acl

If a bucket is created with predefined_acl, the acls can only be changed by recreating the bucket (losing the contents). We should make it clear in the docs that you are digging your own grave by using predefined_acl.

If a bucket is created with acl = [ ... ] then you can change the acls all you like, but you can't switch to predefined acls (because that will recreate the bucket).

The actual specification of acls is somewhat verbose (see below) but I think we can convert it into a nicer string representation (and convert it back in Read()), e.g. make it look like this:

["OWNER:project-owners", "OWNER:project-editors", "READER:project-viewers"]

these project-owners strings are usually suffixed by the project id -123123123 but we can strip it if it's the project given in the provider. This is important because nobody knows the id number anyway, people use the more readable string representation. But we can let people specify it if they want, or if they're referring to a different project.

Google will not let you clear out all the acls, the project-owners always keep the ownership of the bucket. Essentially this means Google implicitly adds the OWNER:project-owners to your ACLs whenever you try to change them. So we'll need to strip that off when we Read() and also in a StateFunc I think (so the diff will work).

Likewise, we need the service account used by Terraform to keep ownership permission on the Bucket. So I think Terraform should also add that permission, which will look like "OWNER:user-$PROJECT_ID-$SOME_CODE@developer.gserviceaccount.com". This will behave the same as the implicit acl given by Google.

Note that we don't need to add that extra acl if predefined_acl was used, because we don't need OWNER permission if we're not going to update the predefined_acl (it's ForceNew).

The only other option I can think of is to get rid of predefined_acl, on the grounds that it's a pain.

I think that covers everything. Here's the mapping table I promised:

$PROJECT_ID is the project numeric id, not the more human readable fooing-bar-123 form.

project-private
[
  {
    "entity": "project-owners-$PROJECT_ID",
    "role": "OWNER"
  },
  {
    "entity": "project-editors-$PROJECT_ID",
    "role": "OWNER"
  },
  {
    "entity": "project-viewers-$PROJECT_ID",
    "role": "READER"
  }
]

private
[
  {
    "entity": "project-owners-$PROJECT_ID",
    "role": "OWNER"
  }
]

public-read
[
  {
    "entity": "project-owners-$PROJECT_ID",
    "role": "OWNER"
  },
  {
    "entity": "allUsers",
    "role": "READER"
  }
]

public-read-write
[
  {
    "entity": "project-owners-$PROJECT_ID",
    "role": "OWNER"
  },
  {
    "entity": "allUsers",
    "role": "WRITER"
  }
]

authenticated-read
[
  {
    "entity": "project-owners-$PROJECT_ID",
    "role": "OWNER"
  },
  {
    "entity": "allAuthenticatedUsers",
    "role": "WRITER"
  }
]

[dhilton]

So in the majority (I presume, I don't know) of cases where a terraform user will make use of canned acls, if you change to a different acl that will force_new destroying the old bucket and creating a new one named the same, with the new canned acl applied?

And in the other case, when using finer grained acl's the bucket will be updated to the new state however still retaining the exceptions ( service account access) that you mention above.

Am I understanding your approach correctly?

[mtekel]

• Is google willing to freeze mapping of the canned-to-real ACL? If so, then we can detect changes using mapping. If not, then leave current behaviour. It will be impossible to detect drifts.
• if user changes canned ACL and this can be updated, then why not attempt the update? Before applying, perhaps do some checks if that specific ACL would not cut terraform off... If that's only the default ACL... then don't support changing pre-canned ACL at all.
• if there is indeed just only one pre-canned ACL that can be supported, then perhaps remove canned-ACLs at all and set the default full proper ACL to whatever would be configured by this single supported pre-canned ACL. This way you keep both user friendliness (default is quite OK and just works) and you can configure the ACLs properly if you wish to, including updates.

[sparkprime]

@dhilton yes and you'll need the force_destroy attribute to delete the objects in the bucket for that to work as well

I'm proposing automatically adding the service account acl in the case of fine-grained acls, but we could also require the user to add it (same effect, different experience).

[sparkprime]

@metel after sleeping on it, yes I think we probably could detect drift, if we

1. preprocess the predefined_acls in the client and augment them with the service account acl
2. In Read(), recognise that predefined_acls is being used and use a StateFunc to convert that down to the low level acls. I think StateFunc has to take a string and return a string, but we could use a comma separated list I guess.

@phinze I think that's how a StateFunc works?

Basically Terraform diff only needs to know whether an attribute is different between the two states right?

There may be issues with getting a canonical form, i.e. sorting the acls. But we may have to face that anyway.

And yeah canned acls are a bit crap tbh. If we could get rid of them now I'd seriously consider it, as long as we can make the acls list user-friendly.

"OWNER:project-owners" is implicit so the mapping would be:

project-private: ["OWNER:project-editors", "READER:project-viewers"]
private: [ ]
public-read: ["READER:allUsers"]
public-read-write: ["WRITER:allUsers"]
authenticated-read: ["WRITER:allAuthenticatedUsers"]

So it's only a mild win from using them.

[phinze]

Whew - lots of activity here! I haven't fully grokked the domain concepts yet but I'll help where I can here.

I think that's how a StateFunc works?

Sounds like you have it right. The StateFunc is meant for canonicalizing a value.

And yeah canned acls are a bit crap tbh. If we could get rid of them now I'd seriously consider it, as long as we can make the acls list user-friendly.

I'd be willing to consider dropping canned ACLs if we're only keeping them around for backwards compatibility, provided we figure out what the story would be for folks using them.

We could also consider adding support for a resource-level Deprecated flag/message.

Feel free to call out anything else you'd like me to weigh in on here.

[lwander]

@sparkprime - This is now fixed.

[ghost]

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.