Add more roles (for storage.buckets.get in particular) about terraform-google-cloud-storage HOT 18 CLOSED

Frankly, I think that sort of case should simply be handled by invoking the module twice (for the buckets with different permission sets). If we try to handle every possible case through variables, the interface ends up overly complicated IMO.

from terraform-google-cloud-storage.

It could, it complicates things a bit. One solution would be this:

roles = {
  'roles/storage.objectCreator' = ["bucket-one", "bucket-two"],
  'roles/storage.legacyObjectReader' = ["bucket-two"]
}
role_members = {
  'roles/storage.objectCreator' = [
    "user:[email protected]", "group:[email protected]"
  ],
  'roles/storage.legacyObjectReader' = [
    "user:[email protected]", "group:[email protected]"
  ]
}

It's a bit repetitive but much easier than dealing with 20 variables (one boolean per role, plus one containing the role members). Roles could maybe be swapped, with buckets as keys and bucket roles as values, and at that point we could probably have bucket => { role => members}. At which point 20 variables start to make sense again, probably. I need a bit more time to think it through, but definitely welcome comments. :)

from terraform-google-cloud-storage.

@morgante absolutely, I want to push this idea to the limit and then dial it down to find a balance between complexity vs using more instances of the module.

from terraform-google-cloud-storage.

This may benefit from renaming of some variables (e.g. admin should probably be reserved for storage.admin, we could use object_admin for what admin is now. And the bucket_admins|creators|viewers might be confusing with storage.legacyBucket(Reader|Writer|Owner) if they were to be added

from terraform-google-cloud-storage.

Hi @ludoo I was wondering if a PR was actively in the works and what will be your approach? Thanks

from terraform-google-cloud-storage.

Hey! I'm sorry I could not get around to doing it yet, I hope to have it by week(end)'s end.

from terraform-google-cloud-storage.

No worries at all, I was just curious :) Grazie mille @ludoo !!

from terraform-google-cloud-storage.

So, I found half an hour to mull this over, and I'm thinking it's probably a good time to change and simplify the interface. What I have in mind is having only two variables for roles that will be applied to all buckets: roles and role_members. This would look something like this in a tfvars file:

roles = ['roles/storage.objectCreator', 'roles/storage.legacyObjectReader']
role_members = {
  'roles/storage.objectCreator' = [
    "user:[email protected]", "group:[email protected]"
  ],
  'roles/storage.legacyObjectReader' = [
    "user:[email protected]", "group:[email protected]"
  ]
}

The drawback is one needs to know the role names, the big advantages are a) hyper flexible, and b) it would use for_each so allow removing/adding roles and members without impacting all resources.

I did not think about the rest of the resources, but thinking about bucket-wide roles, would this suit your use case?

PS - we need a list of plain role names so as to avoid using dynamic values in for_each, hence the duplication of role names in both variables.

from terraform-google-cloud-storage.

I like the idea of simplifying the interface, however if I understand correctly, the ability to define different bindings per bucket will be eliminated?

Random example: Say I create buckets A, B. I would like team_1 as storage.admin of A but object.viewer of B I would also like that team_2 be object.creator of bucket B only and no rights on bucket A .

Is this kind of thing still possible with the suggested refactoring?

from terraform-google-cloud-storage.

Given it a bit more thought, and my preference would be for:

• merging the three variables that apply to all buckets (admins, creators, viewers) into two new roles and role_members variables that handle any role like described in my comment above; this will allow setting any role uniformly to all buckets, including currently unsupported roles
• adding support for the two per-bucket missing roles (storage.admin and storage.hmacKeyAdmin) so that we cover all non-legacy roles at the individual bucket level

This IMHO would make the module more flexible by a) supporting any role applied to all buckets, and b) covering all non-legacy roles at the individual bucket level. The additional complexity in adding the two new per-bucket roles is balanced by simplifying the all-bucket roles variables.

The number of resources will increase slightly, and as they will be incompatible with previous versions it will also be a good time to migrate from count to for_each, thus providing better resiliency to changes.

And as Morgante wrote above, the right pattern for cases not directly covered (eg where different legacy roles are needed for specific buckets) is to have more module invocations for the different buckets or groups of buckets.

@naseemkullah would this cover your case?

from terraform-google-cloud-storage.

@averbuks can you also share your opinion on the proposed changes?

from terraform-google-cloud-storage.

This proposed refactor will allow as you mentioned

a) supporting any role applied to all buckets, and b) covering all non-legacy roles at the individual bucket level

Is it possible to go as for as b) supporting any role applied at the individual bucket level or would that be too complex?

If that is not possible, what do you think about using the same method for both all buckets and per bucket permissions settings, so for all buckets something like:

For all buckets:

all_buckets_object_admins
all_buckets_object_creators
all_buckets_object_viewers
all_buckets_admins
all_buckets_hmac_key_admins

and for per bucket:

bucket_scoped_object_admins
bucket_scoped_object_creators
bucket_scoped_object_viewers
bucket_scoped_admins
bucket_scoped_hmac_key_admins

I think it is a bit confusing to have very different ways of specifying roles on all buckets vs per bucket level.

But yes this would cover my use case, because i realize that storage.admin can be used in favor of mixing storage.objectCreator with storage.legacyBucketReader !

from terraform-google-cloud-storage.

FWIW I am also fine with removing the bucket_scoped permissions as Morgante mentioned and just invocating the module multiple times, in my case i would probably define just 1 bucket per incovation, but the module is still useful in this case.

from terraform-google-cloud-storage.

Can we consider switching to the simplified interface where the same permissions are applied to all buckets?

It's rather unclear to me why you would want/need buckets which have entirely different permissions to be created using the same module invocation. In such cases, it seems like the benefit is almost 0 while increasing complexity for users who only want to (a) create one bucket or (b) apply the same roles across all buckets.

from terraform-google-cloud-storage.

If you mean by that that we will be able to have our own module versions in Fabric with support for multiple resources and roles, I'll be glad to modify this. Otherwise, I don't see how this could happen.

from terraform-google-cloud-storage.

I'm just trying to understand why interface A is so much worse than interface B for you. They both accomplish the same outcome and A seems much much more intuitive (based on actual user feedback):

Interface A:

module "my-foo-bucket" {
  source = "../modules/simple_bucket"
  
  names = ["foo-bucket"]
  project = "foo-project"
  location       = "us-central1"
  iam_members = [
    { role   = "roles/storage.objectAdmin", member = "user:[email protected]" }
  ]
}

module "my-bar-bucket" {
  source = "../modules/simple_bucket"
  
  names = ["bar-bucket"]
  project = "foo-project"
  location       = "us-central1"
  iam_members = [
    { role   = "roles/storage.objectAdmin", member = "user:[email protected]" }
  ]
}

Interface B:

module "my-simple-buckets" {
  source = "../modules/simple_bucket"
  
  names = ["foo-bucket", "bar-bucket"]
  project = "foo-project"
  location       = "us-central1"
  iam_members = [
    "foo-bucket" = { role   = "roles/storage.objectAdmin", member = "user:[email protected]" }
   "bar-bucket" =  { role   = "roles/storage.objectAdmin", member = "user:[email protected]" }
  ]
}

Anyways, what if we moved the multi-bucket, multi-role assignment into a submodule? Just trying to find a solution which is intuitive for everyone.

from terraform-google-cloud-storage.

Hi @morgante, that discussion absolutely makes sense from my point of view since the module is supporting something which is not commonly used, but some fabric examples require per_bucket_admins functionality. Like for ex. where we create a SA and bucket per environment and giving the SAs with permissions to the buckets respectively https://github.com/terraform-google-modules/cloud-foundation-fabric/blob/master/foundations/environments/main.tf#L61-L73. It also answers the question above.
Or imagine someone creates a list of buckets for storing sensitive data for different teams, it that case they would want to have different buckets for diff teams without a need to call the module len(teams) times.
That problem will be simplified a lot once terraform has for_each functionality for modules.

from terraform-google-cloud-storage.

@averbuks Thanks for providing an example link, that makes the use case much clearer for me. I still think it's complex, but clearly necessary for some users.

Given this, I think we should move this complexity into a submodule while providing a simpler interface (for a single bucket) through the root module.

from terraform-google-cloud-storage.